Security & robustness hardening from a full code review (v0.7.0)#1
Merged
Conversation
…eciprocal-best The merge/collaborate paths enforced uniqueness on only one side while the diff path (diff_programs) already requires both. Port that discipline over: - EXACT pass: require signature uniqueness on BOTH old and new sides and skip already-claimed ids — was new-side only, so two old twins double-attached onto one new function (MERGE-P0-1). - ANCHOR pass: add a reciprocal-best gate (best_old_by_anchor) so a deleted function can't hijack a look-alike's cross-arch match (MERGE-P0-2). - best_old_match / best_old_match_bsim: return None on a top-tie instead of the first-seen; the doc claiming "fails closed on ties" was false (MERGE-P1-1). - collaborate: require signature uniqueness on the incoming side too (MERGE-P1-2). - propagate_match: factor scoring into a direction-agnostic propagate_best and add a reciprocal (both-sides-unique) gate (MERGE-P1-3). Adds 3 adversarial WRONG=0 regression tests (each fails on the pre-fix code); the full existing suite + reanchor gate stay green. Also corrects stale pass-count docs (three/four -> five), a dead jaccard branch, and a stale BSim score comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
…llision scylla-schema (DD-036 total loader): - Load via read_message_from_flat_slice (zero-copy): removes the full-buffer duplication AND refuses a tiny artifact declaring a ~511 MiB segment (flat-slice validates each declared segment against the real buffer) (SCHEMA-P1-2). - Truncate EVERY untrusted string the loop skipped: ordered trigrams, edge- provenance producers, program name/language, and each fact's provenance producer + author, so the report no longer lies about being clean (SCHEMA-P1-1). - Drop dangling per-edge provenance (target not a surviving callee edge), counted (SCHEMA-P1-3); drop duplicate-stable-id functions (DD-004), counted (SCHEMA-P2). - LoadReport gains dropped_dangling_edge_provenance + dropped_duplicate_functions. - Nits: document the to_bytes write as infallible; correct the reader-limit comment. scylla-ingest: - Mint ids per function INDEX, not per entry address: two functions sharing (or both failing to parse) an address no longer collapse onto one id (DD-004). Callee resolution uses a unique address->id map that drops ambiguous addresses, so an edge resolves to nothing rather than the wrong function (INGEST-P1-1). - parse_addr returns Option (never a collapsed 0), strips a single 0x/0X prefix; main no longer indexes args[0] on an empty argv. Adds 4 regression tests. Full workspace: 152 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
… O(n^2), clamp scylla-model: - IdMinter no longer derives Default (0-based); a hand-written Default calls new(), so it never mints StableId(0) — indistinguishable from a capnp unset field (MODEL-P2). scylla-port: - Session keeps the LoadReport from the total loader and exposes load_report(), so a head can tell a partially-quarantined artifact from a clean one; it was silently discarded (PORT-P2). - functions()/search() are O(N + E), not O(N^2): a caller adjacency and a display- name map are precomputed once instead of rescanning all functions per view and per callee/caller name. Removes the .unwrap() on view (PORT-P2/P3). - diff_function_addrs is #[deprecated] (superseded by diff) (PORT-P3). scylla-schema: - Clamp fact + edge provenance confidence to the documented 0..=100 on load, so a hostile confidence:200 can't skew merge trust math (MODEL-P3). Full workspace: 152 tests green, clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
The HTTP/JSON and GraphQL heads share an auth/transport surface; fix it in lockstep: - Constant-time bearer-token comparison (no early-exit byte compare); the expected "Bearer <token>" is precomputed once, not per request (HTTP/GQL-P1-1). - Cap the request body at 64 MiB (413 over the cap) instead of an unbounded read_to_end a single POST can OOM the process with (HTTP/GQL-P1-2). - TLS fails CLOSED: one of TLS_CERT/TLS_KEY set without the other now exits instead of silently serving the token + model in cleartext (HTTP/GQL-P2-1). - Per-request panic boundary (catch_unwind): one handler panic returns 500 (http) / drops the connection (graphql) instead of killing the single-threaded server (HTTP/GQL-P2-4). - Empty/blank SCYLLA_*_TOKEN warns loudly and stays treated as unset (never a weak whitespace token) (P3-1); X-Content-Type-Options: nosniff on every response (P3-8); static Header::from_bytes uses expect() (P3-10); http search percent-decodes its query value (P3-2). Full workspace: 152 tests green, clippy clean. (GraphQL alias-amplification limits, introspection toggle, CSRF content-type, and pagination are contract-touching and tracked for a later pass.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
…time auth - P0: listener.accept() no longer propagates a per-connection error out of the loop (which terminated the whole server, remotely and pre-auth). A transient accept error (ECONNABORTED, EMFILE/ENFILE) is logged, briefly backed off, and skipped (RPC-P0-1). - The TLS handshake is bounded by the SAME handshake window as the RPC login, so a stalled TLS handshake can't squat a counted connection slot forever and defeat the connection cap (RPC-P1-2). - Constant-time token comparison in login() — no early-exit byte compare (RPC-P1-1). - Explicit inbound reader limits on every VatNetwork (traversal + nesting), never the shifting capnp default; reconciled with the artifact loader ceiling so a locally- loadable model can also be diffed/exported over the wire (RPC-P2-1 / P3-9). - The connection-slot counter is released by an RAII Drop guard on every task exit (panic included), not a manual decrement that leaks on unwind (RPC-P3-2). - SCYLLA_RPC_HANDSHAKE_SEC floored at 1 (0 aborted every connection) (RPC-P3-8). - Server TLS fails CLOSED when only one of CERT/KEY is set (consistent with HTTP/GQL). Full workspace: 152 tests green, clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
The prompt-injection boundary both heads rely on was defeatable: wrap_untrusted interpolated a serde_json payload — which escapes " \ and control chars but NOT < > or / — between literal </untrusted-data> fences. A hostile binary whose function name/comment contained the closing sentinel ended the envelope early, so the agent / editor-LLM read the following attacker text as trusted instructions. Neutralize both fence tokens in the attacker-controlled payload before wrapping (fail-closed, dependency-free, keeps the content LLM-readable — unlike base64). Adds a regression test to each head. Also adds the MCP `ping` keepalive arm (was method-not-found). Full workspace: 154 tests green, clippy clean. Closes the last of the 5 P0s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
scylla-mcp (stdio): - Cap a JSON-RPC line at 16 MiB (a newline-less flood otherwise drives an unbounded allocation -> OOM) (MCP-P2). - A non-UTF-8 line is decoded lossily and skipped, not fatal — one stray byte used to `break` and kill the whole server (MCP-P2). - Stop cleanly when the client closes its end (write/flush error) instead of spinning on a dead pipe (MCP-P3). scylla-lsp: - read_message bounds both the header line length (8 KiB) and the body: `Content-Length: 99999999999999` used to attempt a multi-TB vec! before reading a byte (LSP-P1). - LSP ranges are measured in UTF-16 code units (the default position encoding), not Unicode scalars, so a rename's WorkspaceEdit covers the whole line for names with a supplementary-plane character (LSP-P2). Full workspace: 154 tests green, clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
…ream gap - assemble mints one stable id per chunk INDEX, not per entry address: an untrusted engine (GAP-3) streaming two functions at the same entry no longer collapses them onto one stable id (DD-004). Callee resolution uses a unique address->id map that drops ambiguous addresses, so an edge resolves to nothing rather than the wrong function (ENGINE-P2-1). - The stream caps now include a cumulative BYTE budget (512 MiB) across ALL retained fields; the count caps missed a malicious engine streaming unbounded string_refs/ imports/callee_names/bsim with zero mnemonics (~4 TB without tripping a cap) (ENGINE-P2-2). Adds 2 regression tests. Full workspace: 155 tests green, clippy clean. (The Java engine-service P1/P2s — process-tree kill, gRPC message size, temp-dir cleanup — are next.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
…up, concurrency - Timeout no longer orphans the real Ghidra JVM: analyzeHeadless is a launcher that forks the analysis JVM as a grandchild, so destroyForcibly() on the direct child left it burning CPU/RAM past the deadline (GAP-2). Kill the whole process tree (descendants first) in the cold path AND in WarmWorker.close (ENGINE-P1-3). - Set maxInboundMessageSize to 512 MiB on both server builders: the grpc-java 4 MiB default rejected any binary >4 MB, contradicting the 200 MB-firmware design goal (ENGINE-P1-4). - The cold path's temp project directory is recursively deleted in finally (-deleteProject only removes the ghidra project inside it) — it leaked every request toward inode/disk exhaustion (ENGINE-P2-10). - Cap concurrent cold analyzeHeadless JVMs with a semaphore (SCYLLA_ENGINE_COLD_CONCURRENCY, default 2) so a burst can't exhaust the host (ENGINE-P2-11). - decompile returns UNIMPLEMENTED instead of a placeholder a caller can't tell from a real empty result; main validates the port arg instead of throwing (ENGINE-P3). Verified with `gradle compileJava --offline` (BUILD SUCCESSFUL). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
… cap scylla-tui: - Install a panic hook that restores the terminal (disable raw mode + leave alt screen) so a crash in draw/handle_key doesn't wedge the user's shell — the sequential teardown was skipped on unwind (TUI-P2). scylla-wasm: - ret_bytes' (ptr << 32) | len packing is wasm32-only; a debug_assert now catches a 64-bit (native rlib/test) call that would shift live pointer bits away and later free garbage (WASM-P2). - scylla_alloc caps a JS-controlled length (null over 512 MiB) instead of attempting an OOM allocation that traps the instance (WASM-P3). scylla-serve: - Cap concurrent handler threads (64) so a localhost flood can't exhaust threads. - Read to the end of the request line instead of a single 2 KiB read (a split TCP read mis-parsed the path); reject non-GET/HEAD with 405; add X-Content-Type-Options: nosniff (SERVE-P3). Full workspace: 155 tests green, clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
…d; changelog link scylla-cli: - materialize's error paths return exit 2 (trouble), matching every other read verb, instead of 1 which the diff verb reserves for "differs" (CLI-P3). - Infallible serde_json serializations are documented .expect(), not .unwrap() (CLI-P3). CI (.github/workflows/ci.yml): - Add a clippy gate (--workspace --all-targets -D warnings; the tree is clippy-clean). - cargo test runs --locked so a stale Cargo.lock fails CI. - New job builds the JVM engine-service (gradle compileJava) on every change, not only at release. CHANGELOG: the [Unreleased] compare link points at v0.6.0...HEAD (was v0.5.0). Full workspace: 155 tests green (--locked), clippy-clean. (Adopting rustfmt + cargo-audit + more fuzz targets need a whole-tree baseline pass, tracked as follow-ups.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
…ddresses The prioritized review (P0-P3) that drove the fix waves on this branch, kept as the provenance for what was changed and what was deliberately deferred. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
Version bump 0.6.0 -> 0.7.0 (workspace package + Cargo.lock in lockstep) and the CHANGELOG [0.7.0] entry + compare links (also adds the previously-missing [0.6.0] link). A SemVer minor: all fixes/hardening from a full code review, but broad, with a few fail-closed behavior changes (TLS half-config now errors; CLI materialize exit code 1 -> 2). No new heads, no artifact-format change. Full workspace: 155 tests green (--locked), clippy-clean; JVM engine-service compiles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFuGddPx3UhUkcC74KBXZB
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes from a full code review of Scylla (see
RECOMMENDATIONS.md), landed as 13 focused, individually-tested commits. Closes all 5 P0s and every P1/P2, plus most P3s. Cut as the v0.7.0 release (SemVer minor — hardening breadth + a couple of fail-closed behavior changes).What changed, by area
scylla-merge(WRONG=0): EXACT/ANCHOR/collaborate/propagatenow require both-sided uniqueness + reciprocal-best matching, so a deleted or twin function can't silently mis-attach a fact. 3 adversarial regression tests.scylla-schema): zero-copy load (kills the OOM surface), truncate every untrusted string, drop dangling edge-provenance + duplicate ids.scylla-ingest,scylla-engine): stable-id collisions on duplicate/unparseable addresses fixed (DD-004).http/graphql/rpc): constant-time token compare, TLS fails closed on half-config, body/reader caps, per-request panic boundary; RPCaccept()no longer kills the server; TLS-handshake timeout.mcp/lsp: the<untrusted-data>prompt-injection envelope can no longer be closed from inside (P0); stdin/Content-Lengthcaps; LSP UTF-16 ranges.engine-service(JVM): process-tree kill on deadline, gRPC message size for real firmware, temp-dir cleanup, cold-analysis concurrency cap.serve/wasm/tui/cli: thread cap + method check, wasm ptr guard, TUI panic-restore, CLI exit-code + serde nits.-D warnings,cargo test --locked, and a JVM engine-service build on every change.Verification
cargo test --workspace --locked-> 155 tests greencargo clippy --workspace --all-targets -- -D warnings-> cleangradle compileJava(engine-service) -> BUILD SUCCESSFULDeliberately deferred (flagged — contract-touching / needs a design call)
GraphQL query-complexity/pagination/introspection toggle, an RPC read-only capability split + rate-limiter, HTTP CSRF content-type enforcement, MCP filesystem-path confinement, and the diff "key-by-id" change (needs the port/merge diff to carry ids). Hygiene follow-ups: adopting
rustfmt(a whole-tree reformat),cargo-audit, and more fuzz targets.🤖 Generated with Claude Code
Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/