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 14 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 | ✅ 5✅ Passed checks (5 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
…#194) The added temp-file and directory fsyncs can push the hard_link past the reader's old 400ms window under parallel-suite disk load, flaking publish_never_exposes_a_partial_final (observed ~1 in 5 local runs). The reader now polls a 5s deadline and stops at the first complete observation; the never-see-partial assertion still runs before the break, so the exposure mutation still fails the test. Also moves the allow-unbounded-git marker in seed_bare_repo directly above the spawn line, where the pre-push gate looks for it.
|
All three addressed on 688b506, with a follow-up in de21c64 for a test-timing consequence of the durability fix. Stale pre-publication temp file (P1).
The process-global counter is gone. The temp name is now picked by a bounded per-call retry over Durability (P1).
The temp write now chains The added fsyncs consumed the timing margin of TestNode teardown (P2).
Full run: 502 unit plus 15 integration tests green, clippy and fmt clean at both heads. One incidental change: the pre-push gate flagged the pre-existing bare git spawn in |
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] Verify the mode before using a newly published identity key
crates/gitlawb-node/src/lib.rs:1277
The new-key path returnskpimmediately afterpublish_key_atomically, while the existing-key path is the only place that callsensure_key_mode_0600.OpenOptionsExt::mode(0o600)is only a creation request, and mounts with mode/ACL remapping can leave the freshly linked file group- or world-readable even though the node then starts with that private key. Apply the same post-publication verification (and fail closed) before logging and returning the winner. -
[P1] Do not make first startup depend on hard-link support
crates/gitlawb-node/src/lib.rs:1231
publish_key_atomicallyunconditionally publishes withhard_link, so a new node whose configured key directory is on a filesystem without hard links (for example a supported removable, FUSE, or network-backed volume) cannot start at all. The previous create/write path worked on those paths; preserve the no-clobber/complete-publish guarantee with a platform-appropriate fallback instead of turning--key-pathinto a hard-link-only storage contract. -
[P2] Surface an unexpected real-server failure to the deny harness
crates/gitlawb-node/src/test_harness.rs:142
The spawned task discards the result ofaxum::serve, soshutdown()observes only a successfulJoinHandleeven if the real socket server exited with an I/O error before teardown. That lets the test harness represent an unexpected server lifecycle failure as a clean shutdown, weakening the new full-stack regression tests. Propagate the serve result through the task (and makeshutdown()fail on it). -
[P2] Abort the server on panic-path test teardown
crates/gitlawb-node/src/test_harness.rs:59
When a deny test panics before its explicitnode.shutdown().await,Droponly signals the watch channel and immediately returns while the detached Axum task can still holdPgPoolclones.#[sqlx::test]then startsDROP DATABASE, racing the graceful server shutdown and leaking per-test databases or cascading cleanup failures exactly when a regression makes a test fail. The fallback needs to abort/release the server-owned resources rather than relying on an unawaited graceful signal. -
[P2] Align the enforced shutdown behavior with the configured contract
crates/gitlawb-node/src/lib.rs:610
When the grace timer wins,drive_serve_with_gracedrops the Axumservefuture and reportsOk(()); in-flight clients receive a closed connection/reset, not the 503 promised byGITLAWB_SHUTDOWN_GRACE_SECSinconfig.rs. This matters especially for mutations, where a client cannot distinguish a completed request from an interrupted one. Either implement an explicit bounded 503/error response path or update the operator-facing contract and test the actual real-socket behavior.
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