test(node): real-node deny harness for trust-boundary regressions (INV-1/2/8)#194
test(node): real-node deny harness for trust-boundary regressions (INV-1/2/8)#194beardthelion wants to merge 42 commits into
Conversation
…can spawn a real node Move the module tree and boot logic from main.rs into a new lib.rs crate root exposing the boot surface (build_router, AppState, Config, migrations) as pub; main.rs becomes a thin #[tokio::main] shim over run(). No behavior change: both targets build and the full node suite (488 tests) stays green. Prerequisite for the real-node deny harness (U1).
…-U4, U5a) Add a feature-gated (test-harness) spawn surface (src/test_harness.rs) that boots a real node on 127.0.0.1:0 over an ephemeral #[sqlx::test] pool through the production axum::serve stack with connect-info, and an integration crate (tests/deny_harness.rs) that drives deny paths with a real reqwest client: - U2 signing client: wraps gitlawb_core::http_sig::sign_request for reqwest; self-checks that a valid signature clears require_signature and a tampered body is rejected (400 content_digest_mismatch). - U3 spawn_node: real socket, p2p disabled, per-test DB, shutdown-on-drop. - U4 assert_denied: 4xx AND body-no-leak AND not-empty-200 (INV-8); pure core unit-tested for clean-403 / empty-200 / leaking-403 / wrong-status. - U5a INV-8: unsigned git-receive-pack is denied 401 with no leak. Widens the three cfg(test) test builders (Db/RepoStore::for_testing, run_migrations) to also compile under the feature. No production behavior change: prod build (no feature) excludes test_harness; node suite stays green (488) and the 7 integration tests pass.
A validly signed non-owner PUT /visibility is rejected 403 by require_owner (no x-ucan, so require_ucan_chain passes through to the gate); the owner's signed PUT reaches the handler (reachability proof, guards against a 404/415 masquerading as a pass). Adds seed_repo/withhold_path seeding helpers to the test harness. Mutation-verified load-bearing: with require_owner forced Ok the non-owner PUT returns 201 and the INV-8 assertion flips the test RED.
Adds seed_bare_repo (shells git to build a real bare repo at the served path,
sha1 or sha256 object format) and two INV-2 deny cases over the real stack:
- U7: a public repo with a /secret/** withhold rule denies an anonymous blob
read of the withheld path (404) with no content/OID leak, while the sibling
public path is served (path-scoped, not blanket).
- U5b: the same withhold denies an anonymous /ipfs/{cid} read of the withheld
blob's content-addressed id (404, no leak), while the public blob's CID is
served. Completes U5 (INV-8) alongside U5a.
Both mutation-verified load-bearing: forcing visibility_check to allow leaks
the secret at 200 and the INV-8 assertion flips each test RED.
Drives the git-upload-pack POST directly (v0 stateless-RPC: want HEAD, flush, done) via a bounded reqwest client rather than a vanilla `git clone` (which negotiates protocol v2 and deadlocks against the node's v0 server, and would otherwise wedge the suite). The served pack is indexed with git index-pack and its objects listed with verify-pack -v: a packfile-aware assertion, since a raw byte scan cannot see an OID inside the zlib-compressed stream. A public repo with a /secret/** withhold rule must serve a pack that omits the withheld blob's object while keeping the sibling public blob. Mutation-verified load-bearing: forcing visibility_check to allow puts the withheld blob back in the pack and flips the test RED. Completes the harness (8 units, 11 integration tests). Prod build (no feature) and the 488-test node suite stay green.
cargo test --workspace skips the harness because it lives behind the test-harness feature (kept off the production binary). Add an explicit step that runs it with the feature and the same Postgres service, so the INV-1/ INV-2/INV-8 trust-boundary regression cases execute on every PR instead of only when run by hand.
Add unit tests for the two remaining check_denied branches: a non-4xx expected status is rejected as a test bug, and an empty withheld token is skipped rather than matching every body. Closes the last unexecuted branches in the deny assertion.
Fan-out of U6 to the security-sensitive owner-gated mutations that had only the source-level authz-table guard and no runtime deny test: protect_branch, unprotect_branch, create_webhook, delete_webhook, remove_visibility. Each rejects a validly-signed non-owner with 403 and lets the owner reach the handler (not 403). Mutation-verified load-bearing on their shared root gate: did_matches forced true opens all five (non-owner protect_branch returns 201) and the test flips RED. Replica register/unregister were intentionally excluded: they are signer-self (you register your own node), not owner-gated, so there is no owner-deny to assert.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe node startup code moves into a reusable library, while a feature-gated ChangesReusable node library and boot lifecycle
Feature-gated harness runtime and CI wiring
Signed requests and denial-path integration tests
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant DenyHarness
participant TestNode
participant PostgreSQL
CI->>DenyHarness: run deny_harness with test-harness
DenyHarness->>TestNode: spawn_node(pool)
TestNode->>PostgreSQL: run migrations and seed state
DenyHarness->>TestNode: send signed or anonymous requests
TestNode-->>DenyHarness: return denial or filtered Git response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:node
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/gitlawb-node/tests/deny_harness.rs (1)
36-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInconsistent request timeouts across the suite.
Only the clone test (line 344-347) builds its
reqwest::Clientwith an explicit timeout, with a comment explaining why (avoiding a wedged suite). Every other test here (e.g. this one, and lines 67, 98, 123, 192, 249, 420) usesreqwest::Client::new()with no timeout. If the real node under test ever hangs on any of these paths, the test blocks until the 45-minute CI job timeout instead of failing fast with a clear cause.♻️ Suggested fix: a shared bounded client helper
fn bounded_client() -> reqwest::Client { reqwest::Client::builder() .timeout(std::time::Duration::from_secs(30)) .build() .expect("client builds") }Then swap each
reqwest::Client::new()in this file forbounded_client().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/tests/deny_harness.rs` around lines 36 - 37, Introduce a shared bounded client helper in the deny harness, such as bounded_client, that builds reqwest::Client with a 30-second timeout. Replace every reqwest::Client::new() usage in this file, including the clone test, with the helper while preserving the existing request behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/lib.rs`:
- Around line 539-572: Update the HTTP shutdown flow around axum::serve and the
with_graceful_shutdown future to enforce the configured grace duration, aborting
the server drain when it expires instead of waiting indefinitely for long-lived
requests. Use the existing grace value derived from config.shutdown_grace_secs
and remove the unused grace discard while preserving normal shutdown signaling
and serve_result handling.
- Around line 1007-1027: Update the identity-key creation and loading flow
around Keypair generation and key_path.exists() to eliminate the TOCTOU race and
disclosure window: create the file with OpenOptions::create_new(true) and Unix
mode 0o600, write the PEM through that handle, and handle AlreadyExists by
retrying the existing-key load path. When loading an existing key, validate or
tighten its permissions to 0600 before reading it, while preserving the existing
PEM parsing and error behavior.
---
Nitpick comments:
In `@crates/gitlawb-node/tests/deny_harness.rs`:
- Around line 36-37: Introduce a shared bounded client helper in the deny
harness, such as bounded_client, that builds reqwest::Client with a 30-second
timeout. Replace every reqwest::Client::new() usage in this file, including the
clone test, with the helper while preserving the existing request behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da2a7166-0514-4eaf-9449-ef5be4e258e0
📒 Files selected for processing (11)
.github/workflows/pr-checks.ymlcrates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/lib.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/test_harness.rscrates/gitlawb-node/tests/deny_harness.rscrates/gitlawb-node/tests/support/assert.rscrates/gitlawb-node/tests/support/mod.rscrates/gitlawb-node/tests/support/signing.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Create identity keys atomically with owner-only permissions
crates/gitlawb-node/src/lib.rs:1007
The new library retains the existingexists()→fs::write()flow. On Unix,fs::writecreates the PEM using umask-derived permissions and only then changes it to0600; a local process can read the node private key in that window. The separate existence check also lets concurrent node starts overwrite each other's generated identity. Create the file withcreate_newand mode0600, then handleAlreadyExistsby loading the winning key. -
[P2] Enforce the configured HTTP shutdown grace period
crates/gitlawb-node/src/lib.rs:539
with_graceful_shutdownbegins draining when the signal fires but has no deadline; the computedgraceis explicitly discarded at line 571. A long-lived request can therefore prevent termination until the orchestrator hard-kills the process, defeatingGITLAWB_SHUTDOWN_GRACE_SECSand risking interrupted cleanup. Bound the drain with that duration and force completion once it expires.
- Create the node identity key atomically with create_new + mode 0600, closing the umask-derived 0644 disclosure window and the exists()->write overwrite race; on AlreadyExists load the winner's key (bounded retry so a loser can't read a half-written PEM) and tighten looser perms on load. - Enforce the configured shutdown grace: bound the axum drain by grace measured from the signal (extracted as drive_serve_with_grace), abandoning in-flight requests once it expires instead of waiting indefinitely. Removes the discarded grace value. - Route deny-harness reqwest clients through a shared bounded_client (30s timeout) so a wedged node path fails fast instead of hanging to the CI limit. Tests: 0600-on-create, load-tighten, concurrent-start convergence (create race), and grace-race abandon / normal-drain / signal-gated-clock. All RED-then-GREEN by execution.
- Create the node identity key atomically with create_new + mode 0600, closing the umask-derived 0644 disclosure window and the exists()->write overwrite race; on AlreadyExists load the winner's key (bounded retry so a loser can't read a half-written PEM) and tighten looser perms on load. - Enforce the configured shutdown grace: bound the axum drain by grace measured from the signal (extracted as drive_serve_with_grace), abandoning in-flight requests once it expires instead of waiting indefinitely. Removes the discarded grace value. - Route the deny-harness reqwest clients through a shared bounded_client (30s timeout) so a wedged node path fails fast instead of hanging to the CI limit. Tests: 0600-on-create, load-tighten, concurrent-start convergence (create race), and grace-race abandon / normal-drain / signal-gated-clock. All RED-then-GREEN by execution.
|
Addressed the review feedback in 30672cf. P1 (identity key). Created atomically with P2 (shutdown grace). The drain is now bounded by the configured grace, measured from the signal rather than server start (extracted as Unit tests cover 0600-on-create, tighten-on-load, 8-thread concurrent-start convergence, and the grace abandon / normal-drain / signal-gated paths. Also took CodeRabbit's nitpick: the deny-harness clients now go through a shared 30s |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/lib.rs`:
- Around line 1120-1125: Update the key-writing logic in create_new for both
Unix and non-Unix branches so any write_all failure removes the partially
written file at key_path before returning the error. Preserve the existing
contextual error and successful write behavior, and ensure cleanup is attempted
consistently in both branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 620c5ae9-78f1-476e-badb-3e054d1f583a
📒 Files selected for processing (2)
crates/gitlawb-node/src/lib.rscrates/gitlawb-node/tests/deny_harness.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gitlawb-node/tests/deny_harness.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Remove a failed first-write identity file
crates/gitlawb-node/src/lib.rs:1123
The newcreate_new(true)path fixes the original permission window, but if the PEM write itself fails after the file has been created, the just-created key path is left behind as an empty or partial PEM. Every later start then takes thekey_path.exists()branch, retries parsing that same bad file inload_racing, and exits withinvalid PEM keyinstead of generating a fresh identity. A transient ENOSPC/EIO/quota failure during first boot can therefore permanently wedge the node until an operator manually deletes the file. Please remove the newly-created file onwrite_allfailure in both the Unix and non-Unix branches before returning the error. -
[P2] Do not ignore failed key permission tightening
crates/gitlawb-node/src/lib.rs:1059
The load path now advertises that loose existing identity keys are tightened to0600, but theset_permissionsresult is discarded. If the file is readable but chmod fails, for example on a read-only mount or an ownership/ACL mismatch, the node still reads and uses a world/group-readable private key while logging a normal "loaded existing identity" path. That leaves the exact key exposure this follow-up is trying to close. Please surface the chmod failure or otherwise verify the final mode before continuing with the key.
…r tightening (#194) F1 (P1): create_new(true) closed the permission window, but a write_all failure after the file was created left an empty/partial PEM behind. Every later start then took the key_path.exists() branch, re-parsed that corrupt file in load_racing, and exited 'invalid PEM key' instead of regenerating — a transient ENOSPC/EIO on first boot permanently wedged the node. Extract write_key_or_cleanup, which removes the just-created file on write failure, and wire it into both the unix and non-unix create branches. F2 (P2): the load path tightened a loose existing key to 0600 but discarded the set_permissions result. A chmod that failed (read-only mount, ownership/ACL mismatch) left a world/group-readable private key in use while logging a normal 'loaded existing identity'. Surface the tighten failure (propagate it) and add ensure_key_mode_0600, which fails closed if the key is not 0600 after the attempt. RED->GREEN: failed_write_removes_the_partial_key_file (a failed write leaves no file; RED without the remove_file). loose_key_mode_is_rejected_not_used (a 0644 key is rejected; RED without the mode check). Existing created_key_is_mode_0600, existing_key_is_loaded_and_tightened, and concurrent_starts_converge_on_one_identity stay green. Full node lib+bin suite 497 passed, fmt + clippy clean.
|
Both addressed on Remove a failed first-write (F1). Extracted Do not ignore a failed permission tighten (F2). The load path now propagates the One behavior note on F2: a loose key that genuinely cannot be tightened is now rejected rather than used, which narrows the original "never reject a loose key" leniency — but only in the exposed-and-unfixable case, which is the exposure this follow-up closes. The real ENOSPC/chmod-fail I/O triggers are not driven end to end (no portable fault injection), but the error handling is proven at the helper level and the wiring is a one-line pass-through of the write/chmod result into it. RED->GREEN for each; no other production behavior changes. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Do not fail concurrent startup after an arbitrary 100 ms key-write window
crates/gitlawb-node/src/lib.rs:1123
create_newexposes the final key path before the winner has completedwrite_all, and every other process that sees that inode gives up after 50 2-ms retries. On a slow or temporarily stalled filesystem, a winner can legitimately take longer than that interval, so all losing node starts returninvalid PEM keyeven though the winning write later succeeds. This reintroduces an availability failure for the concurrent-start case the new code is intended to make safe. Keep retrying until a meaningful startup deadline, or publish a fully written temporary key atomically so readers never observe a partial final file.
The create path did create_new(final)+write_all, so the final path appeared as an empty inode before the PEM was flushed, and a losing/fast-path start only retried ~100ms (50x2ms). On a slow or stalled filesystem the winner's write can exceed that window, so every other start failed boot with 'invalid PEM key' (#194). Publish atomically instead: write the full PEM to a sibling temp, then hard_link it into place. hard_link is atomic and fails if the target exists, so the final path only ever appears COMPLETE (no partial-read window), a lost race never clobbers the winner, and a crashed writer leaves only a temp rather than a partial final that would wedge later starts. load_racing_key now polls on a wall-clock KEY_RACE_DEADLINE (5s) instead of a fixed 100ms count. Hoisted load_existing_key/load_racing_key to module level for testability. Adversarial RED->GREEN: a 250ms-slow winner is waited out (RED at the old 100ms); a reader watching the final never sees a partial file (RED with create_new+write); a losing publish does not clobber the winner; 500 tests pass.
|
Confirmed and fixed at The finding is right. The create path did I took option (b), keeping the single-winner guarantee: write the full PEM to a sibling temp, then Vetted both ways:
Full suite 500 pass; Two deliberate tradeoffs worth flagging:
@jatmn ready for another look. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Recover from a stale pre-publication key file
crates/gitlawb-node/src/lib.rs:1174
The temporary path is deterministically derived from the PID and a process-local counter. If the process dies after writing.identity.pem.tmp.<pid>.0but before the cleanup at line 1195, that file remains. A restarted container commonly runs as PID 1 again and starts its counter at zero, socreate_newfails on the same stale name before the final key exists; the node cannot start until an operator deletes the temp file. Retry a new temporary name (or safely handle stale temps) so an interrupted first provision remains recoverable. -
[P1] Make identity-key publication durable before exposing it
crates/gitlawb-node/src/lib.rs:1185
write_allfollowed byhard_linkonly atomically changes the namespace; neither the completed temporary file nor the containing directory is synced. A host/power crash after the link can therefore leaveidentity.pemdurable while its PEM bytes are missing or truncated. On restart the existing-path branch retries that invalid key and then exits, recreating the permanent startup wedge this change is intended to eliminate. Sync the temporary file before linking and the parent directory after publication, with appropriate error/cleanup handling. -
[P2] Wait for the spawned node before SQLx drops its database
crates/gitlawb-node/src/test_harness.rs:48
DroppingTestNodeonly sends the shutdown watch signal; the detachedaxum::servetask, which owns the router'sPgPool, is never joined.#[sqlx::test]startscleanup_testimmediately after the async test future returns, so it can issueDROP DATABASEwhile that server still retains connections. SQLx prints and ignores that cleanup failure, making the new CI job leak test databases and race/flap under parallel execution. Retain the task handle and add an async shutdown/teardown path that signals, awaits the server, and releases the pool before the test returns.
…n TestNode teardown (#194) - pick the temp key name with a bounded per-call retry instead of a process-global counter: a crashed start's leftover temp no longer wedges a PID-stable restart, and cross-container publishers stop colliding - fsync the temp PEM before it can be linked and the key directory after publication, so a power crash cannot leave a durable-but-truncated identity.pem that permanently blocks startup - TestNode::shutdown() joins the serve task and closes the pool before sqlx's DROP DATABASE cleanup runs; every deny-harness test now ends with it instead of relying on drop-only teardown
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/test_harness.rs (1)
142-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate
axum::servefailures.
let _ = ...awaithides I/O failures, making the task appear to exit successfully. The harness may report an unrelated connection failure—or pass teardown—without exposing that the server stopped unexpectedly. Return the result orexpectit so shutdown surfaces server failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/test_harness.rs` around lines 142 - 151, Update the spawned server task around axum::serve to propagate its awaited result instead of discarding it with let _. Return the result or use expect so axum::serve I/O failures remain visible to the test harness while preserving graceful shutdown behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gitlawb-node/src/test_harness.rs`:
- Around line 142-151: Update the spawned server task around axum::serve to
propagate its awaited result instead of discarding it with let _. Return the
result or use expect so axum::serve I/O failures remain visible to the test
harness while preserving graceful shutdown behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd43aeed-185d-43f3-af41-8a37b2da755f
📒 Files selected for processing (3)
crates/gitlawb-node/src/lib.rscrates/gitlawb-node/src/test_harness.rscrates/gitlawb-node/tests/deny_harness.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/gitlawb-node/tests/deny_harness.rs
- crates/gitlawb-node/src/lib.rs
|
Both findings fixed in F1 (fallback retains an unsynced identity key). The hard-link fallback now removes the final on a file-fsync failure instead of keeping it, so a bytes-accepted-but-not-durable final can't survive to wedge the next start. I split the write and the fsync so each removes-on-failure but keeps a distinct error context (write-rejected vs durability-failed), which preserves the phase signal for an operator debugging ENOSPC/EIO on a network mount. Verified: I reversed the fallback-fsync test to assert the final is gone. It was RED on the old keep behavior (file still present) and GREEN after; neutralizing the remove line turns it RED again. All identity-key tests stay green. This is the cleanup path you asked for over keep. The tradeoff is the concurrent-start case: a loser that already loaded the key via F2 (serve task detached on teardown timeout). Reworking that path, I also closed three teardown gaps in the same change: a task that finishes in the timeout race is now honored as a graceful exit rather than panicking as wedged; a genuine serve-task panic is propagated instead of being flattened into a timeout; and the pool is closed on every path, including the wedged arm, before the panic. The previous order panicked first and skipped Full suite is green at |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Make the hard-link fallback recoverable after interruption
crates/gitlawb-node/src/lib.rs:1401
This fallback is explicitly used for filesystems that rejecthard_link, but it createsidentity.pembefore writing and syncing the PEM. A SIGKILL or power loss in that window leaves an empty or partial final key; the next boot takes theexists()path atload_or_create_keypairand only retries parsing it, so it fails permanently instead of provisioning a key. The base direct-write path had this interruption risk too, but the new atomic primary path fixes it and this newly added compatibility fallback retains it for the network/overlay filesystems it is meant to support. Use an atomic no-clobber publish primitive there as well, or leave enough recoverable state to distinguish and remove an interrupted first write.
…ss-check) (#195) * test(node): U1 deny-bearing route registry (owner-gate + signature tranches) Foundation for the invariant deny-prober (plan 002): a declarative registry of ONLY the routes that carry a runtime deny (owner-gate 403 / read-gate 404 / signature 401), each classified by reading its handler and recording the fn name. Owner-gate (12 rows: merge/close_pr, close_issue, webhooks, labels, protect, visibility set/remove/list incl the GET-is-403 correction) and signature-required (git-receive-pack) tranches are verified; read-gate tranche stubbed with a TODO (per-handler 404-vs-filter verification pending). A consistency self-test asserts no dup method+path, every row names its handler, and every read-gate row carries a positive-twin Reach. Decoupled from the #194 primitives (U1 uses none), so classification is not hostage to harness churn. * test(node): U2 fixture state matrix + per-gate-class probe generators Two-repo fixture (public owner-gate substrate + private read-gate substrate) plus probes_for(): each deny-bearing row expands to a hostile probe asserting the exact deny status and a positive twin (owner-reachability for owner-gate, authorized read for read-gate) so a deny for the wrong reason cannot false-pass. Three self-checks cover the owner-gate, read-gate, and signature classes. * test(node): U1 read-gate tranche (14 repo-scoped 404-deny reads) Each row's handler was read to confirm it gates on "/" via authorize_repo_read / visibility_check and returns RepoNotFound (404) to a non-reader of a private repo, with the owner re-read as the ReaderReads positive twin. Sub-entity gets (get_issue/get_pr/get_cert/get_bounty and the comment/review lists) are deferred to the fixture expansion that seeds them; global list-filter surfaces and the KNOWN_UNGATED reads are excluded with reasons. Runtime behavior is asserted by U3. * test(node): U3 drive the deny-bearing registry over a real node Boots a real node, seeds the two-repo fixture (plus a PR and an issue so the author-or-owner close gates are reached, not 404'd on an absent entity), then walks every deny-bearing route: the hostile probe must return the exact deny status and leak nothing, and the positive twin must reach the handler (owner: not 403; read: 2xx). A terminal invariant asserts one hostile probe per row was driven and the count equals the registry, so a row that produced no probe fails loud rather than passing vacuously. Adds seed_pr/seed_issue to TestNode. Mutation-verified load-bearing: breaking did_matches drove the merge_pr owner-gate hostile to 500 (RED, named); neutering authorize_repo_read's deny drove the get_repo read-gate hostile to 200 (RED, named); both reverted green. * test(node): U4 completeness cross-check keeps the registry from drifting A pure source scrape of server.rs (multiline .route parser, floor 90) that the runtime sweep cannot give: no deny-bearing row points at a route that no longer mounts (anti-stale), and no handler carrying an unambiguous owner-gate marker (require_repo_owner / require_owner) escapes the registry (orphan). The api dir is read at test time so a new module is covered. Complements authz_guard (which proves handlers are gated) by proving the deny-bearing ones are actually driven, and reaches the non-API git mounts authz_guard never sees. Mutation-verified: a bogus row path tripped anti-stale (named the row); removing the merge_pr row tripped the orphan guard (named merge_pr); both reverted green. * test(node): close vacuous-green gaps in the deny prober's own assertions Vetting the harness against itself surfaced branches that ran green but were never exercised adversarially: - Ok2xx twin dropped its unused token Option; the arm now asserts the authorized read returns a NON-EMPTY 2xx, so an empty-200 denial-rendered-as-success is caught (executed by every read twin). - U4 orphan guard gains a floor on the owner-marker scan (>=6), so a marker-scan regression to zero fails loud instead of passing by checking nothing. - Added probe unit tests that execute the previously-uncovered probes_for arms: the SiblingPublic path-scoped twin, and the Reach::None fail-loud panic. All 22 harness tests green. Every send_probe/probes_for branch is now executed; the Deny(401), Not403, and Ok2xx-status arms were additionally mutation-verified RED (signature-expect flip; did_matches->false; authorize_repo_read->deny) and both U4 floors RED via threshold flips, all reverted. * test(node): close 3 completeness gaps in the deny prober's own guards (#195) jatmn's review flagged three ways the prober's self-guards stay green when the protection they check is removed: - F1: the U4 completeness check enforced owner-gate orphans but not read-gate ones, so dropping authorize_repo_read on get_issue/get_cert/ get_bounty/etc. left the sweep green. Add a symmetric read-gate orphan guard that scans src/api for authorize_repo_read/visibility_check handlers and requires each to be a driven ReadGate row or an enumerated READ_GATE_NOT_DRIVEN entry with a reason, replacing the free-text prose. A staleness check keeps the allowlist from carrying dead exemptions. - F2: the U3 registry sweep called assert_denied with an empty withheld list, proving status only. Seed the private fixture with a distinctive secret and its blob OID, carry them on the read-gate hostile probes, and pass them through so a denial body echoing withheld data fails. - F3: the owner-gate orphan scan matched only require_owner/require_repo_owner and missed the inline did_matches(caller, &record.owner_did) idiom that protect_branch/unprotect_branch use. Fold in a has_owner_did_matches discriminator that catches the owner form without false-matching the signer-self (register_replica) or author (close_pr) did_matches forms, pinned by unit tests on the exact bodies. * test(node): make the deny-prober guards load-bearing (#195, F1-F4) Resolves jatmn's four INV-21 completeness findings — guards that were green but not load-bearing: F1: every ReadGate hostile probe was anonymous, so the signed-non-reader branch of each private read was untested — a regression treating any valid signature as authorized would leak while the sweep stayed green. Add a Signer::Stranger 404/ no-leak probe per read row, with a dedicated readgate_stranger counter so a dropped probe fails loudly. RED: authorize any signed caller -> the stranger probe leaks (200) while the anon probe stays green. F2: get_star_status, get_icaptcha_proof, and replicate_encrypted_blobs were source-only exemptions (a marker scan, never driven). Drive them as real ReadGate rows and remove the allowlist entries. RED: a runtime bypass that keeps the authorize_repo_read marker leaks -> the driven probe RED, the source scan green. F3: git_receive_pack's protected-branch owner 403 was never driven (its registry row is the unsigned-401 path). Add signed_stranger_protected_branch_push_is_forbidden (a signed non-owner push to a protected branch -> 403, owner control -> not-403), plus a seed_protected_branch harness helper. RED: invert the gate -> the probe RED. F4: the read-gate no-leak assertion omitted the private repo id (seed discarded it). Preserve it on the Fixture and withhold it; a synthetic 404 body carrying the UUID must fail check_denied. RED: a UUID-bearing body passed before the token was added. Each guard proven load-bearing by reverting the exact production auth-line (reverts not shipped). deny_harness suite 30 passed, fmt + clippy clean. * test(node): drive every arm of the multi-principal close/dispute gates close_pr, close_issue (owner-or-author) and dispute_bounty (creator-or-claimant) were each driven with a single owner/creator twin, so reverting the untested arm stayed green. Model gates by their authorizing-principal set (MultiPrincipalGate + Principal), seed distinct author/creator/claimant identities, and emit one Not403 twin per arm plus the stranger 403. registry_internal_consistency now invokes probes_for and asserts a twin per declared arm, so a future multi-arm handler cannot register with an untested arm. Each arm proven load-bearing: reverting is_owner/is_author on close_pr/close_issue and is_creator/is_claimant on dispute_bounty turns that arm's twin RED. * test(node): drive the deferred private reads with per-read leak assertions The deferred reads (get_issue, get_pr, get_pr_diff, list_issue_comments, list_reviews, list_comments, get_cert, get_bounty, path-scoped get_tree) sat in READ_GATE_NOT_DRIVEN, checked only for an authorize_repo_read marker, so an ignored result or early return stayed green. Seed each sub-entity in the private repo (issues/PRs via the real create handlers, a cert via a signed owner push, a private-repo bounty), thread id-keyed paths through IdSource, and drive each as a ReadGate row: anon/signed-stranger 404, owner 2xx. Each seeded entity carries a distinctive marker added to the read-gate withheld set, so a 404 that leaks the entity's own content fails (not just a status check). get_pr_diff exercises both its repo-root gate and its per-path visibility_check loop. get_encrypted_blob stays deferred (owner twin needs live IPFS; reclassified structurally next). Load-bearing: an ignored-result bypass on each read turns its hostile probe RED; a 404 leaking a seeded marker fails the body assertion. * test(node): close the prose-excuse hatch with a mounted-route completeness gate READ_GATE_NOT_DRIVEN allowed a free-text reason to park a read-gated handler out of the runtime sweep. Replace the reasons with a closed NotDrivenReason enum (ReadGatingMutation, GitSmartHttpRead, ContentAddressedRead, GlobalListFilter, ExternalDependencyUnavailable); get_encrypted_blob moves to the last. A drivable 404-deny GET no longer type-checks a reason and is caught by a driven-not- relabeled assertion. Add a completeness gate that derives the required read-gate set from the mounted route table (scrape_mounts, INV-21b), not only the source-marker scan: every mounted repo-scoped GET must be a driven ReadGate row, a deny-bearing GET, structurally excused, or a declared public GET — else it fails. Catches a read that gates via a helper the marker scan does not recognize. Load-bearing: re-parking a driven read trips the relabel assert; removing a driven ReadGate row trips the mounts cross-check. * test(review): harden multi-principal twin check against signer collision Add a distinct-signer assertion to multi_principal_rows_emit_one_twin_per_arm: the per-arm any() check is satisfied by a single twin when two Principals map to the same Signer, so a signer_for_principal collision would leave one arm silently untested. Assert the emitted twins carry principals.len() distinct signers (proven RED by colliding Author->Owner). Also correct the get_cert / priv_markers comments, which claimed a withheld cert-content marker that is not seeded — no cert marker is needed because get_cert 404s at the repo-root gate before the cert is fetched, so cert fields cannot reach the deny body. * style(test): apply rustfmt to the deny-prober additions * test(node): drive the bounty mutation gates and pin fixture object format (#195) - submit/approve/cancel_bounty become MultiPrincipalGate registry rows with status-appropriate bounty fixtures (claimed/submitted/open): their status check fires before the auth gate, so each row seeds its own bounty through the real API and the arm twins consume the status - pin --object-format=sha1 in both fixture worktrees so a sha256 default object format cannot break the OID equality with the served sha1 repos * test(node): drive the caller-self task/register gates in the deny-prober (#195) Resolve the three review findings on head 07abcc6. Bound the two new real-node probes (protected-branch push, PR-diff) on a timeout-bearing client, so a wedged git subprocess fails the suite rather than hanging it until CI kills the job. Withhold the pushed secret blob's OID (full and [..12] short form) in the PR-diff denial. The old branch was dead code: the OID was never in the fixture seed map, so build_branch_push_body now returns the pushed blob OIDs. Drive every caller-self runtime deny the registry claimed but only source-marker checked. create_task, claim_task, and register are a new SignerSelfGate class (a body DID field bound to the authenticated signer); complete_task and fail_task are a MultiPrincipalGate assignee arm (the caller must equal the task's stored assignee). Each runs hostile-then-authorized against a real node. A completeness guard, derived from a scan of src/api, now requires every did_matches(caller, X) handler to be a driven registry row, and the module doc is reconciled to the REST surface (the parallel GraphQL mutation gates are fenced separately, #219). Every gate is mutation-verified load-bearing: reverting a production did_matches line drives its hostile probe RED, and the completeness guard goes RED when a driven row is dropped or the classifier is starved. No production code changed. --------- Co-authored-by: t <t@t>
…arker The hard-link fallback exposes the final key name before the PEM is complete, so a crash in that window used to leave a partial final that wedged every later start. Create a bounded-name .publishing marker and fsync its name durable before create_new(final), dispose of it on every exit with the final's removal ordered first, and remove it only once the final is complete and durable. The marker dir fsync is warn-and-continue so dir-fsync-hostile mounts degrade to reduced power-loss protection instead of never provisioning.
A marker plus a content-failed final (empty or unparseable PEM) is now provably an interrupted first write: boot claims one marker by atomic rename to a .recovering file, quarantines the final preserving its bytes, sweeps the remaining markers, and regenerates, at most once per start. Every other failure keeps today's loud fail bit-for-bit: a bad final without a marker, any non-content load failure (transient EIO, mode, stat), and a second-iteration failure never touch the key. The error class is carried as a typed root error so recovery cannot key on message text.
…d winners A fallback winner stalled past the race deadline can have its round claimed by a recovering peer: the peer renames the marker, quarantines the in-progress final, and republishes. The winner's own-marker removal now decides the round: removal succeeding linearizes it ahead of any recovery, while any failure demotes it to Lost, after waiting out live or stale .recovering claims so the follow-up load cannot observe the pre-quarantine key. Demotion is always safe: absent interference the Lost path re-loads the winner's own key.
A marker orphaned between its durability fsync and create_new(final), or a claim left by a crashed recovery, must not linger to misclassify a much-later corruption of a good key as an interrupted first write. Both healthy-boot arms (successful load, and a Won publish on the generate path) now sweep stale .publishing and .recovering entries, but only after the sweeper itself has made the final durable: a loadable final is not necessarily synced, and removing a live winner's marker before its sync_all would recreate the marker-less partial-final wedge on power loss. Any durability failure skips the sweep and leaves the markers in place.
…recoverable Three review findings against the marker protocol. Recovery now claims every publishing marker and stale recovering claim (per-item bounded claim names), so a live publisher's own-marker commit check can no longer falsely succeed while its round is stolen through a stale marker; a post-claim re-parse aborts to reload when a publisher completed in the window. Claims count toward the crash signature and are claimable by rename, so a recoverer crash between claim and quarantine no longer recreates the permanent wedge. Claims are released on every exit including publish failures, closing the leak that made demoted publishers ride the full race deadline on a dead claim. The crash signature also needs two consecutive observations, giving a live mid-write publisher one poll of grace before a reader can classify its round as a crash.
Add the missing coverage on recover_crashed_publish's degraded arms: quarantine-name exhaustion must fall back to removing the bad final and still regenerate, and a blocked removal must surface the chained error loudly, with the leftover claim proven claimable on the next boot. Route all marker-class name construction through publishing_prefix and recovering_prefix as the single source of truth, state the dir-fsync degradation caveat in publish_key_atomically's guarantee prose, and pin the crash-signature short-circuit with a timing assertion so recovery never silently regresses to riding the full race deadline.
…d keys Second-model review findings. The healthy-boot sweep now age-gates recovering claims (2x the race deadline; a live claim is always younger), because sweeping a live recovery's claim cancelled a demoted publisher's clearance wait and reopened two-sided divergence; publishing markers keep the unconditional sweep since stealing one only forces a safe demotion. The post-claim re-parse quarantines only on content-class failures (a transient error there releases the claims and stays loud), and a key completed inside the re-parse-to-quarantine window is detected by parsing the quarantined bytes and restored via no-clobber link instead of being discarded. The crash signature must now persist for 250ms of consecutive observations, above realistic mid-write latency on shared volumes, so concurrent starts stop hijacking live publishes.
…p to owned inodes The restore now republishes the quarantined bytes through the atomic publisher, so every tier refuses an existing final and a concurrent winner survives; a lost restore keeps the quarantine as forensics and loads the winner. The fallback's post-write error arms remove the final only after proving the name still refers to the inode this publish created, so a failing publisher whose round was claimed away can no longer unlink the recoverer's republished key. Pre-write arms keep by-name removal: they complete inside the crash-signature persistence window, before any recovery can have claimed the round.
…laim freshness A failed write or fsync on the fallback path now removes nothing: the partial final and its marker stay in place as exactly the crash state boot recovery claims, quarantines, and regenerates from. That deletes the stat-then-unlink cleanup whose race could destroy a recoverer's republished key, and can no longer strand a marker-less partial final when an unlink fails. Pre-write failures keep removal and disposal; they complete inside the crash-signature persistence window. Claim renames also stamp the claim's mtime to now, because rename preserves the source mtime and an aged stale marker otherwise births a live claim the healthy-boot sweep would treat as orphaned mid-round.
…ess stamp Two PID-1 containers on a shared identity volume derive identical claim names, and rename replaces an existing destination, so one recoverer's release could unlink the other's live claim. Claim names now carry a per-process nonce between pid and counter, so the release set can never alias another process's claims. The claim-freshness stamp gains a write fallback (a one-byte append bumps mtime where set_modified is unsupported), and its failure warning names the real hazard. Also lock the vanished-final re-parse arm with a dedicated regression test.
…tealing A completed key stranded in quarantine by a failed or crash-interrupted restore is no longer abandoned: the generate arm first scans quarantines newest-first and republishes the first parseable one through the atomic publisher, preserving identity continuity instead of silently minting a fresh DID; unparseable quarantines stay as forensics. Recovering claims become claimable only past the same age floor the sweep uses, so a second starter can never steal a live recoverer's round mid-flight; a claim-crash state recovers after the bounded age floor passes. The crash-signature persistence doc now states the slow-mount trade-off plainly: a pre-write window exceeding the floor yields churn that converges, never divergence.
… waiter Adoption of a stranded quarantined identity is now bounded by the claim age floor, so a current round's completed key is preserved while a historical forensic quarantine can never resurrect an old DID or defeat the delete-identity-for-fresh-DID operator procedure. A healthy boot also schedules one delayed re-sweep after the age floor passes, so claims spared as possibly-live cannot linger as aged residue that would let later real corruption of a good final masquerade as a crash state and silently regenerate. The demotion waiter now shares the sweep's liveness rule: it waits out claims younger than the age floor rather than a flat race deadline, and skips aged orphans quickly.
…nes, fail closed on unsettled rounds Every arm that returns a loaded, adopted, or reloaded key now funnels through one sweep-then-return closure, so stale markers cannot outlive any healthy boot path. Quarantines that provably lost their round (a Lost restore, or siblings of an adopted quarantine) move to an inert superseded class: bytes preserved as forensics, never adoptable, so the delete-identity-for-fresh-DID operator procedure can no longer resurrect an outcompeted key. The demotion waiter fails closed when its bound expires while a live young claim persists: an unsettled round now surfaces as a loud failed publish instead of resolving a key that disk may be about to contradict.
… old forensics Complete the supersede rule across its mirror arms: a lost adoption and a restore-Won round now both demote every outcompeted adoptable quarantine, so no arm leaves bytes the fresh-DID procedure could resurrect. Live recovery rounds now heartbeat their claim mtimes every quarter of the age floor under an RAII stop-and-join guard, making age-as-orphan sound: an aged claim provably lost its recoverer, and a stalled-but-live round on a slow volume can no longer be mistaken for residue by the waiter, the sweep, or a competing recoverer. Superseded forensic files are retention-swept after 24 hours, bounding growth on long-lived key directories.
…s, key the waiter on absence Both Won arms now supersede sibling quarantines before consuming the chosen one, so a crash in the gap cannot leave an outcompeted key adoptable. Plain quarantine forensics get the same 24h retention sweep as superseded files, bounding growth on long-lived key directories. The demotion waiter drops age logic entirely: it returns Ok only when every claim file is gone and fails the publish closed while any claim survives the bound, young or aged, so a heartbeat failure on a hostile volume can no longer convert a live round into orphan residue mid-wait. Doc prose reconciled with the shipped wait contract.
|
Fixed at The finding is right, and it overruled round five's stated trade-off for the right reason. That trade-off rested on a partial first write being indistinguishable from a corrupted real key. The fix removes the indistinguishability instead of accepting the wedge: the fallback brackets its non-atomic window with a durable publish marker, created and fsynced before The must-not cases are locked by execution, not inspection:
The concurrency half went through repeated adversarial review beyond the finding itself, and the protocol that survived is stricter than the first cut: recovery must atomically claim a round before touching anything, with per-process nonces in claim names so PID-1 containers on a shared volume cannot alias each other; live rounds heartbeat their claims under an RAII stop-and-join guard, which makes claim age a sound orphan signal for sweeping and claimability; the winner's own-marker removal is its commit check, and a demoted publisher waits for claim absence and fails closed if any claim survives the bound; post-write publish failures remove nothing and hand the marker-plus-partial state to boot recovery; a quarantined key that turns out complete is restored through the atomic no-clobber publisher; a stranded quarantine younger than the age floor is adopted for identity continuity, while anything that provably lost its round is superseded, so deleting Honest residuals, all stated in the code docs: on dir-fsync-hostile mounts the marker fsync is warn-and-continue, so recovery there narrows to SIGKILL-class crashes rather than never provisioning; a live publish whose pre-write window outlasts the 250ms crash-signature persistence can be hijacked by a concurrent starter, which converges through demotion and restore at the cost of churn; non-Unix keeps this file's existing reasoned-not-run posture. The adversarial passes had converged to P2-and-below for several rounds when I stopped them, but I would not claim the multi-actor lifecycle space is exhausted; extra scrutiny on the shared-volume interleavings is welcome. One settled design call: the identity machinery is now large enough to deserve its own module, but I kept it in place this round so the diff stays reviewable against the finding it fixes; the split is queued as a follow-up. |
After merging main, the completeness fence flagged three repo-scoped list GETs that #113 gated but the harness still treated as ungated public reads: list_webhooks (authorize_repo_read + require_repo_owner), list_replicas and list_protected_branches (authorize_repo_read). The runtime marker scan caught its own hand-maintained PUBLIC_REPO_GETS list drifting stale. Drive each real deny instead of exempting it: - list_replicas and list_protected_branches take Option auth, so they slot in as standard ReadGate rows (anon and signed-non-reader both 404, owner 2xx, no-leak body). - list_webhooks is doubly gated and 401s a headerless caller before any lookup, so a vanilla ReadGate row's hardcoded anon->404 leg does not fit. Add a per-Row anon_read signal (Deny404 default, Deny401 for auth-required reads) so its anon leg expects 401 while the signed-non-reader still drives the existence-hiding 404. Its owner layer is a sibling OwnerGate row on the public repo (signed stranger -> 403). The two share the /hooks path, so the consistency dedup key moves from (method, path) to (method, path, gate), which still catches an accidental same-gate double-registration. - Empty PUBLIC_REPO_GETS: the three are now driven rows, classified via the driven-path set rather than declared public. The mechanism stays for the next genuinely-public repo-GET. Each new row proven load-bearing: reverting the production gate turns the matching hostile probe red (403->200 owner, 404->403/200 read), and flipping list_webhooks' anon_read to Deny404 reddens its anon probe (got 401). Full deny_harness green (39/39). No production changes.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Fail when the denial body cannot be read
crates/gitlawb-node/tests/support/assert.rs:55
assert_deniedturns aresp.text()error into an empty body. A denial can send its expected 4xx headers and then reset or truncate the response body; this helper then accepts the status and observes no withheld token, so the new no-leak regression passes without inspecting the body that might contain the leak. Propagate the body-read failure before applyingcheck_denied. -
[P2] Close every ordinary spawned node before SQLx cleanup
crates/gitlawb-node/tests/deny_harness.rs:154
The protected-branch test returns withoutnode.shutdown().await; the same is true for the PR-diff and registry-sweep tests. They therefore useTestNode::Drop, which cannot await/reap the Axum task or close its pool, while#[sqlx::test]immediately drops the test database. Server-held pool connections can race that cleanup and leak databases or make the new CI job flaky. End each normal test with the awaited shutdown (leaving only the tests intentionally exercisingDropas exceptions). -
[P2] Bound the fixture setup requests as well as the probes
crates/gitlawb-node/tests/support/probe.rs:319
The registry test builds its fixture before it reaches its bounded probe client, but the fixture seeders still use unboundedreqwest::Client::new()calls. A hung create/claim/push route during setup can therefore wait until the 45-minute CI job timeout, despite the harness's stated fast-fail timeout contract. Reusesupport::bounded_client()(or pass a bounded client through the seed helpers) for these setup requests.
Resolve three review findings on the real-node deny harness, all test-only. - assert_denied no longer folds a body-read error into an empty body: an unreadable denial (a mid-body reset/truncation) now fails loud instead of being certified leak-free by a withheld-token scan over "". - The three sqlx::test cases that returned via TestNode's async Drop (signed_stranger_protected_branch_push_is_forbidden, get_pr_diff_withheld_path_is_denied, deny_bearing_registry_denies_hostile_and_admits_authorized) now end with node.shutdown().await, releasing the serve task's pool clones before sqlx's synchronous DROP DATABASE cleanup instead of racing it. - A source-scrape guard, every_spawn_node_test_shuts_the_node_down, makes that contract load-bearing for every spawn_node test, with a closed allowlist for the two Drop-regression tests that deliberately skip shutdown. Removing any one shutdown() turns it RED (verified). - The eight fixture seeders in support/probe.rs use the bounded client, so a hung setup route fails fast rather than running to the CI job timeout. Full deny_harness suite green (40 tests).
An adversarial review flagged that the new every_spawn_node_test_shuts_the_
node_down guard could itself pass vacuously - the exact class it exists to
prevent (INV-21 on the guard).
- A spawn_node test whose only `node.shutdown(` occurrence was a comment
(`// node.shutdown().await`) satisfied the raw substring check while still
returning via Drop.
- An unbalanced `{` inside a string literal in a test body could over-extend
the brace-matched body slice and swallow a later test's shutdown, clearing
the earlier one (latent: no current test triggers it).
Both are closed by sanitizing the source through a new `code_only` pass
(blanks comment and string/raw-string content, length-preserving) before the
scan sees it, and the offender check is extracted to a pure
spawn_node_tests_missing_shutdown so it can be driven with synthetic sources.
Three adversarial unit tests pin both holes shut; stubbing code_only to a
no-op turns the comment and brace tests RED (verified), so the sanitizer is
load-bearing.
deny_harness suite green (43 tests); clippy clean.
|
All three are addressed in 8d81cbe, with a follow-up in 996e1b3. Full Fail when the denial body cannot be read ( Close every ordinary spawned node before SQLx cleanup ( Bound the fixture setup requests ( @jatmn ready for another look. |
2 participants
What
A real-socket, end-to-end security regression harness for gitlawb-node. It boots a real node on
127.0.0.1:0, drives trust-boundary DENY paths through a real reqwest client with real RFC-9421 signing, and asserts both the refusal status and that no withheld data leaks. It turns the per-PR real-node-verify step into executed tests, and covers groundtower::oneshotcan't: the full production middleware stack over an actual socket, plus a systematic no-empty-200 (denial-as-success) assertion.Why the crate split
gitlawb-node was binary-only, so an out-of-crate integration test could not reach
build_router/AppState/Config. The first commit splits it into lib+bin: the module tree and boot logic move tosrc/lib.rs(exposing a minimalrun()plus the boot surface), andsrc/main.rsbecomes a thin#[tokio::main]shim. No behavior change, the 488-test node suite stays green and the production binary is unaffected. The harness itself lives behind atest-harnessfeature so its spawn surface never compiles into the release binary.Coverage
Fourteen executed cases, one strong case per invariant plus a high-value owner-gate fan-out:
/ipfs/{cid}read of a withheld blob denied 404 with no leak./ipfssurface, and the git-upload-pack replication path where the served pack must omit the withheld blob while keeping the sibling public one.Every case is mutation-verified load-bearing: the specific gate was broken, the test observed to go red (the secret leaking, or the withheld object appearing in the pack), then reverted.
Notes for review
git-upload-packPOST directly (v0 stateless-RPC) instead ofgit clone, because a defaultgit clonenegotiates protocol v2 and hangs against the node's v0 server. That hang (rather than a clean error) may be worth a separate look if standard-client interop matters. The assertion is packfile-aware (git index-pack+verify-pack), not a raw byte scan, since a leaked OID would otherwise hide inside the zlib stream.--features test-harness, sincecargo test --workspaceskips it by design.Summary by CodeRabbit