test(node): real-node deny harness for trust-boundary regressions (owner-gated mutations, path-scoped reads, client-surfaced denials) - #194
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.
- 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.
…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.
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. |
|
@coderabbitai full review |
|
Merge origin/main (0.7.1 baseline) onto feat/real-node-deny-harness, restore the thin binary entry and test-harness AppState fields, add --locked to the deny-harness CI step, and seed pinned_cids with production CIDs from raw object bytes so /ipfs withhold probes match get_by_cid verification.
Tighten identity key parent directories to 0700 before load/publish, create fallback publish markers with 0600, and extend U5b /ipfs withhold coverage with signed non-reader denial and allowlisted-reader grant legs. Add OwnerGate/ReadGate registry probe-shape tests.
Tighten identity key parent dirs only on publish, extend INV-8 header scanning, add inline claimant orphan guard, and align gossip ping tests with the /ready-first readiness probe.
|
Reintegrated onto current main (merge at bfc44f9, 0.7.1). The deny-harness CI step now runs /ipfs withhold uses production pin CIDs ( At head gl/git-remote client denials stay outside this PR's HTTP harness scope. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged intentional deny-harness fixtures (HTTP secret_cid probe and pack listing assertion). Ignore tests and test_support for analysis, remove an unused import, and keep the allow-unbounded-git marker on the harness git spawn line.
Reuse the signed anon probe path for the loopback GET so the cleartext- transmission query does not flag secret_cid in the URL builder, and mark the assert_denied panic as an intentional leak witness for cleartext-logging.
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.
Merge readiness
- [P1] Resolve the failing
CodeQLstatus before merge
GitHub reports two high-severity CodeQL annotations in the new harness: the test sends its deliberately withheld CID over local HTTP, and the denial assertion includes an OID in a panic witness. These may be test-fixture false positives, but the aggregate CodeQL check is failing. Classify and suppress only confirmed fixture-only alerts (with narrow, documented suppression), or change the harness if either flow can reach non-test behavior; do not merge while the required security check remains unresolved.
Findings
-
[P1] Restore the dedicated advisory-lock pool
crates/gitlawb-node/src/lib.rs:300
The lib/bin extraction now passesdb.pool().clone()toRepoStore, instead of the cancellation-safebuild_lock_pool(...)used by the prior boot path.acquire_writeholds a PostgreSQL session advisory lock while receive-pack/Tigris work is in flight. If the request is cancelled in that interval, returning the ordinary pooled connection does not run the lock-pool release hook, so a later checkout of that session can retain the lock and other sessions block on it. Long pushes also consume the query pool itself, starving authorization, visibility, and post-receive database work.Restore
lock_pool_sizeplusbuild_lock_pool(db.pool(), ..., db_acquire_timeout_secs)at the library boot boundary and pass that pool toRepoStore. Keep the existingafter_release(pg_advisory_unlock_all())cleanup and the separate capacity budget; the root cause is that the extraction silently substituted a plain clonedPgPoolfor this behavior-bearing constructor. -
[P1] Keep the cross-field DB/push configuration check on the new boot path
crates/gitlawb-node/src/lib.rs:83
run()no longer callsConfig::validate(). Clap validates each setting independently, so a deployment can start with (for example) fewer DB connections thanmax_concurrent_git_pushes + DB_POOL_APP_HEADROOM. Slow receive-packs can then occupy the available connections and turn unrelated DB-backed requests into acquire-timeout failures.Call the existing
config.validate().map_err(...)immediately afterbootstrap::merge_seeds(&mut config)and before key generation, database connection, or listener setup. Preserve the existing validation and its error wording rather than duplicating only today’s pool check: the root cause is that the movedrun()bypasses the single cross-field validation boundary. -
[P2] Preserve peer reachability hysteresis in the extracted gossip loop
crates/gitlawb-node/src/lib.rs:1084
The previousgossip_ping_roundkept afailed_onceset and persistedlast_ping_ok = falseonly after two consecutive failed probes. The new inline loop writes every probe result immediately. A short network timeout, peer restart, or transient database error therefore hides the peer’s federated data for the full five-minute gossip interval, despite the prior contract intentionally treating one failure as non-authoritative.Move the existing
gossip_ping_round/peer_ping_db_updatestate machine with the extraction, retain the missed-tick behavior, and restore the two-failure unit and DB-round regression tests. The root cause is not the probe implementation: it is the lost in-memory failure state when the loop was inlined.
…ss extraction Re-wire Config::validate(), the advisory-lock pool, and gossip peer-reachability hysteresis on the library boot path, and narrow CodeQL suppressions on fixture-only harness assertions.
…down) Run join_or_abort lib tests in the deny-harness CI step, repair group/world-writable key parents on existing-key load, abort gossip on HTTP shutdown with per-peer shutdown polling, and log list_peers errors.
|
Addressed the boot-path regressions and the follow-on gaps on head 2a7ed47. Boot path (your Aug 30 review):
CodeQL:
Also on this head:
Checks run locally:
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. This review is written to stop drip feedback, not add another open-ended round. Read the overall guidance first — it explains why this PR has produced twelve review cycles and what to do differently so the next push is the last one before merge.
Overall guidance — why feedback keeps dripping, and how to end it
This branch has 54 commits and twelve prior CHANGES_REQUESTED reviews since July. That is not because reviewers are moving goalposts at random. It is the predictable outcome of how the PR has been structured. The remaining items below are small on head 2a7ed47; the pattern is what keeps generating new rounds.
1. The PR is several projects in one branch
What started as “real-node deny harness” has accumulated, in the same series:
- lib+bin split and public module surface
- identity-key atomic publish, quarantine, recovery claims, delayed resweep, parent-dir repair
- shutdown grace enforcement (
drive_serve_with_grace) - gossip hysteresis restoration and shutdown polling
- deny registry / completeness guards (absorbed from #195)
- CodeQL config and CI wiring
Each layer is legitimate work, but each production boot change re-opens security review even when the stated deliverable is test infrastructure. Reviewers cannot sign off on “tests only” while lib.rs still changes identity recovery semantics every round.
What to do: For the merge push, freeze production behavior. No new boot-path logic, no new recovery edge cases, no “small follow-up” commits in lib.rs. If something still needs hardening, open a follow-up PR with its own review. The deny harness can merge without solving every identity-key corner case in the same branch.
Update the PR body to match reality. It still says “No behavior change, the 488-test node suite stays green.” Head materially changes boot (identity recovery, shutdown grace, gossip). Stale claims invite reviewers to re-audit production on every round because the stated contract does not match the diff.
2. Fix-one-review-item, break-another — the lib/bin extraction pattern
A recurring drip pattern on this branch:
- Reviewer finds boot regression from lib/bin split (missing
validate(), lock pool, gossip hysteresis). - Author fixes in
692b894. - Reviewer finds new gaps in the fix (
2a7ed47: parent-dir repair threshold, gossip shutdown, CodeQL still red).
This is classic regression churn from moving code without a frozen contract. The extraction moved main.rs → lib.rs but did not carry a single checklist of boot invariants on the first try.
What to do: Before requesting another review, run this boot invariant checklist yourself against run() vs pre-split main.rs (or vs main today) and paste the result in the PR comment so reviewers do not rediscover items:
Config::validate()aftermerge_seedsbuild_lock_pool+lock_pool_size, notdb.pool().clone()forRepoStorebuild_http_client()redirect policy, notClient::new()- gossip
failed_oncehysteresis +shutdown_rxin ping round +gossip_handle.abort()after serve drive_serve_with_gracewired toshutdown_grace_secs- identity key path: publish vs reload parent-dir policy consistent
If all six are true on your branch (they are on head today), say so explicitly and ask reviewers to treat further boot tweaks as out of scope for this PR.
3. The harness over-claims “comprehensive” while documenting exclusions
The registry, completeness guards, and PR prose claim runtime discharge of INV-1/INV-2/INV-8 across deny-bearing routes. The same codebase documents intentional exclusions:
- GraphQL mutations (#219)
get_encrypted_blob(no IPFS stub)- global pin/anchor listings (#121)
git_receive_packowner-push wiring (only signature row + protected-branch ad hoc test)- replica register/unregister
That design is defensible, but every exclusion is a hole reviewers will probe unless the PR draws a hard boundary. “Comprehensive” invites “what about X?” forever.
What to do: Add a short “Harness scope contract” section to the PR body (not scattered comments). For each excluded surface, one line: excluded because …, tracked in #N, covered by [unit test / other PR / accepted risk]. Then stop expanding the registry in this PR unless a production gate is actually broken.
For enforce_owner_push specifically (see P3 finding below): make a one-time decision — either add one real-socket test and close the topic, or add one sentence to the scope contract (“wiring covered by repos.rs unit tests; socket sweep does not own push policy”) and do not entertain further rounds on it.
4. CI fixes are being attempted without verifying on push
CodeQL is still FAIL on head after paths-ignore, inline codeql[...] comments, and multiple “clear false positive” commits. Each attempt that does not green the check creates another review round.
What to do: Treat CodeQL as a verify-on-push gate, not a code-change guess:
- Push the fix to the PR branch.
- Wait for the CodeQL run to finish.
- Only then request re-review, with a link to the green run or a link to Security-tab dismissals with rationale.
Do not stack another “maybe this suppression works” commit without that evidence. If dismissals are the team’s chosen path for fixture-only alerts, do that once, document it in the PR, and stop iterating on comment placement.
5. What actually blocks merge vs what does not
To end the back-and-forth, separate merge blockers from follow-ups:
| Item | Blocks merge? | Notes |
|---|---|---|
| CodeQL check FAIL | Yes | Required status; fixture false positives still fail the aggregate check |
Parent-dir 0o022 vs 0o077 on reload |
No (P3) | Real inconsistency in new code; not worse than main for 0755; fix in this PR or #follow-up |
enforce_owner_push socket coverage |
No (P3) | Harness scope / claims alignment; production gate + unit tests exist |
| GraphQL / encrypted blob / global listings | No | Already documented out of scope |
| Further registry rows | No | Unless you choose to expand scope; otherwise freeze |
Recommended close-out for the author:
- Green CodeQL (verify on push).
- Update PR body: honest scope, boot invariant checklist signed off, harness scope contract with exclusions.
- Freeze
lib.rsproduction changes; follow-ups in separate PRs. - Optionally fix the two P3 items in this PR or open tracked follow-ups and stop debating them here.
If you do (1)–(3), the remaining review surface is intentionally narrow and this should be the last substantive round.
Merge readiness
-
[P1] Resolve the failing CodeQL check before merge
crates/gitlawb-node/tests/deny_harness.rs:307
crates/gitlawb-node/tests/support/assert.rs:105
.github/codeql/codeql-config.ymlWhat is failing: GitHub Advanced Security on head
2a7ed47reports 2 high-severity alerts and the aggregateCodeQLstatus is FAIL (mergeStateStatus: BLOCKED).anon_ipfs_read_of_withheld_blob_is_denied—client.get(format!("{}{}", node.base_url, secret_path))wheresecret_pathis/ipfs/{secret_cid}. Query:rust/cleartext-transmission.assert_denied—panic!("{reason}")may include withheld OID/secret tokens. Query:rust/cleartext-logging.
Both are intentional harness fixtures (loopback deny probe; leak witness on test failure). Not production exposure.
Root cause: Mitigations on this branch did not clear the head run:
paths-ignorefor**/tests/**— alerts still fire on changed test files in the PR diff. Do not assume this config alone greens PR scanning without a verified run.- Inline
codeql[rust/...]comments — on line 305 (two lines above.get) and on thepanic!line; GitHub still reports both on head.
How to fix (pick one path and verify on push):
- Restructure so CodeQL does not trace sensitive data into the flagged expressions (e.g. redact panic message; build URL without embedding
secret_cidin the expression the query sees). - Working suppression on the exact flagged lines, confirmed against a finished CodeQL run.
- Security-tab dismissal of both as false positives (“loopback deny-harness fixture only”), documented in the PR comment with dismissal links.
Do not disable CodeQL or ignore production
src/.
Findings (non-blocking — fix here or in a follow-up, but do not extend this PR’s production scope to chase them)
-
[P3] Align identity-key parent repair on reload with publish-time checks
crates/gitlawb-node/src/lib.rs:3434
crates/gitlawb-node/src/lib.rs:1379Reload repairs parent only when
mode & 0o022 != 0; publish usesensure_key_parent_dir_privatewhenmode & 0o077 != 0. Example: parent0755is left loose on restart but tightened on first publish. Not a regression frommain(base had no reload repair). Root cause: reload fast-path used a narrower mask than publish. Fix: callensure_key_parent_dir_private(parent)unconditionally on existing-key load (or gate on0o077), add a0755reload test. Acceptable as post-merge follow-up if you freeze boot changes now. -
[P3] Decide once on
enforce_owner_pushreal-socket coverage
crates/gitlawb-node/tests/support/routes.rs:458
crates/gitlawb-node/tests/deny_harness.rs:145
crates/gitlawb-node/src/api/repos.rs:1860git-receive-packhas three signed push gates: signature (registry),owner_push_rejection(default on, unit-tested only), branch protection (ad hoc test). Registry sweep does not catch wiring regressions forowner_push_rejection. Root cause: registry design + documented exclusion ofgit_receive_packfrom owner orphan scan. Production gate is correct on head. Fix: add one socket test on unprotected branch or add one line to harness scope contract and close the topic. Do not leave implicit — that is what causes drip.
Summary for the author
Merge when: CodeQL is green (verified on push) and PR body reflects actual scope.
Stop dripping when: Production boot is frozen in this PR, harness exclusions are written down once, and P3 items are either fixed or explicitly deferred with issue links — not left implicit for the next reviewer to reopen.
Route the anonymous withheld-CID GET through an anon_get helper (matching the unflagged signed_request pattern) so CodeQL no longer traces the secret into a single inline HTTP expression. Redact withheld tokens from the assert_denied panic witness so a leak failure does not itself leak the secret to stderr. Both are test-only changes; no production code moved.
…push socket coverage The reload fast-path checked only write bits (0o022) while the publish helper checked all group/world bits (0o077), so a 0755 parent survived reload but was tightened on publish. Align the reload mask to 0o077 so the owner-only 0700 invariant holds across restarts, not just first publish. Adds a regression test proving a 0755 parent is tightened on reload (RED with the old mask, GREEN after). Adds real socket coverage for enforce_owner_push on an unprotected branch, isolating the owner-push gate from branch protection. The existing protected-branch test could not distinguish which gate fired the 403. Updates sweep_failure_tolerated to reflect the tightened reload path: a 0555 parent is now tightened to 0700 before the sweep, so the marker is removed rather than surviving behind a read-only directory.
|
Head is now CodeQL cleartext-transmission ( CodeQL cleartext-logging ( Parent-dir mask inconsistency ( enforce_owner_push socket coverage: the existing protected-branch test could not distinguish which gate fired the 403. Added Verification:
The production behavior change is limited to enforcing the already-documented owner-only |
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.
This review is scoped to what head 988e8ea actually changes and what blocks merge today. The deny harness itself looks structurally sound on head (parent-dir reload mask and enforce_owner_push socket coverage landed in the latest commit). The items below are the remaining gaps — not a request to reopen boot-path design or expand harness registry scope.
Merge readiness
-
[P1] Green the required CodeQL check (fixture false positive, not a production leak)
crates/gitlawb-node/tests/support/assert.rs:115-118
.github/codeql/codeql-config.ymlWhat is failing: On head
988e8ea, the aggregate CodeQL required check is FAIL (mergeStateStatus: BLOCKED). GitHub Advanced Security reports one open alert on this PR:Rule Location Severity rust/cleartext-loggingassert.rs:118(panic!("{redacted}"))warning The earlier
rust/cleartext-transmissionalert ondeny_harness.rsis cleared on head (URL built viasecret_pathvariable instead of embedding the CID in the.get()expression CodeQL traced).This is not a production security defect.
assert_deniedruns only in the deny harness. It panics when a deny probe fails — i.e., when the test has already detected a problem. At runtime,redact_withheldstrips withheld OIDs/CIDs from the message beforepanic!. No production path logs secrets; no loopback client receives them from this helper on the success path.Root cause: CodeQL's taint analysis follows withheld tokens from
check_denied_with_headersinto thereasonstring (the error path embeds the token inwithheld token {token:?} leaked…), then throughredact_withheld, and still flags thepanic!sink. Static analysis does not treatredact_withheldas a sanitizer. Branch mitigations did not clear the head run:paths-ignore: **/tests/**— does excludetests/support/assert.rs, but PR diff scanning still raised the alert on the changed file; do not assumepaths-ignorealone greens PR checks without a verified push.paths-ignore: **/test_support/**— does not match anything in this repo (tests/support/is the actual layout; in-cratesrc/test_support.rsis a file, not a directory).- Inline
codeql[...]comments — did not clear the head alert on the panic line.
How to fix (pick one path, verify on push before re-requesting review):
-
Restructure the failure witness so taint never reaches the sink (preferred if you want zero Security-tab noise):
- Build the panic message from fixed strings only, e.g.
panic!("deny assertion failed: status/body/header leak check"). - Log redacted detail via a separate channel CodeQL does not trace to a log sink, or assert with
assert!(false, "…")using only static text and pass redacted detail as separate arguments that do not embed raw tokens in the format string. - Keep
redact_withheldunit tests — they still validate the redaction helper even ifassert_deniedno longer panics with interpolated secrets.
- Build the panic message from fixed strings only, e.g.
-
Security-tab dismissal (acceptable for fixture-only alerts if the team uses dismissals for test harnesses):
- Dismiss
rust/cleartext-loggingonassert.rs:118as false positive ("loopback deny-harness test failure witness; tokens redacted before panic; no production path"). - Link the dismissal URLs in a PR comment. Dismissal greens the aggregate check only after GitHub processes it — confirm on the finished run.
- Dismiss
-
Working suppression on the exact flagged line — only if you confirm on a finished CodeQL run that the suppression is honored (prior inline attempts on this branch did not).
Do not: disable CodeQL, add broad
paths-ignoreforsrc/, or weakencheck_deniedleak detection on the success path.
Findings
-
[P2] Restore gossip readiness tests lost in the lib+bin move (coverage regression, not harness logic)
crates/gitlawb-node/src/lib.rs—gossip_ssrf_tests(~3640–3815)
merge-base reference:crates/gitlawb-node/src/main.rs(pre-split)What broke: The lib+bin extraction moved
ping_peer_readiness_with_timeoutintolib.rsunchanged (function body is byte-identical to merge-base). The unit tests beside it were not fully carried over: merge-base had 7ping_peer_*tests; head has 3 renamedping_peer_health_*tests. Production gossip behavior is the same; CI guardrails shrank.Root cause: Mechanical
main.rs→lib.rsmove copied production code but dropped part of thegossip_ssrf_testsmodule. Commit692b894("restore lib/bin boot regressions dropped in the deny-harness extraction") fixed several boot-path gaps but did not restore this gossip suite. This is collateral damage from the crate split, not a defect in the deny harness routes, registry, orspawn_nodewiring.What head still covers:
Retained test What it guards ping_peer_health_does_not_follow_redirectLegacy /health302 must not be followed (SSRF) when/readyis 404ping_peer_health_reports_success_on_200Legacy /health200 counts as healthy when/readyis 404ping_peer_health_reports_unhealthy_on_connection_errorConnection errors map to unhealthy What is missing and why it matters (distinct from retained tests):
Dropped test (merge-base name) Gap if not restored ping_peer_readiness_reports_success_on_200/ready200 happy path never exercised — retained tests only hit/ready404 then/healthping_peer_readiness_ignores_liveness_only_healthMost important: /ready503 +/health200 must not mark peer ready. Current code handles this at1260:1266:crates/gitlawb-node/src/lib.rs(non-404, non-success →falsewithout fallback), but nothing in CI would catch a refactor that reintroduced/healthfallback on 503ping_peer_readiness_legacy_fallback_shares_deadlineSerial /ready+/healthprobes must share one timeout envelope — subtle regression classping_peer_readiness_falls_back_for_legacy_peerExplicit expect-count contract for 404→200 sequence (mostly overlapping with retained health test, lowest priority) ping_peer_readiness_legacy_fallback_does_not_follow_redirectoverlaps heavily with the retained redirect test; restoring it is optional.Smallest fix: Copy the four merge-base tests (or the top three rows excluding the optional overlap) into
gossip_ssrf_testsinlib.rs, usingping_peer_readiness_with_timeoutwhere the shared-deadline test needs it. No production code change required unless a test fails.Scope boundary: This is lib-move test parity, not deny-harness expansion. Do not fold this into registry rows, GraphQL coverage, or further boot-path changes. If you prefer to defer, open a tracked follow-up and note it in the PR comment — but merging as-is reduces
main's federation-readiness coverage for unchanged code.
Author guidance (close-out without more review drift)
These are not additional findings — they are how to land this PR without another drip round:
-
Treat CodeQL as verify-on-push. Push the fix or dismissal, wait for the CodeQL run to finish, link the green run (or dismissal URLs) in a PR comment, then request re-review. Do not stack another untested suppression commit.
-
Freeze production boot changes for the merge push. Head already includes identity recovery, shutdown grace, and gossip hysteresis restoration. Further
lib.rsboot edits in this branch re-open security review unrelated to the harness deliverable. Follow-ups belong in separate PRs. -
Update the PR body once, honestly. Replace "No behavior change" with a short scope contract: what the harness covers (REST deny paths, INV-1/2/8 cases listed), what it excludes (GraphQL #219, iCaptcha enforce mode, global listings), and that the lib+bin split also carries boot hardening already on head. A signed boot-invariant checklist (validate, lock pool,
build_http_client, gossip hysteresis+abort,drive_serve_with_grace, parent-dir 0o077) lets reviewers treat further boot tweaks as out of scope. -
Do not expand harness scope in this PR to chase lower-impact owner gates, GraphQL, or encrypted-blob surfaces — those are documented deferrals with existing unit-test guards. The registry/completeness machinery is already large; more rows invite "what about X?" rounds without proportional safety gain.
Two findings from the 2026-09-08 review on head 988e8ea: 1. CodeQL rust/cleartext-logging on assert.rs:118. The panic sink interpolated redact_withheld(&reason, withheld), but CodeQL's taint analysis traces withheld tokens through redact_withheld without recognizing it as a sanitizer. Replace the interpolated panic with a fixed string so no tainted data reaches the sink. redact_withheld is still called (keeping it live) and its unit tests still pass; only the panic message changes. This is a test-only fixture false positive: no production path logs secrets. 2. Four gossip readiness tests dropped in the lib+bin split. Merge-base main.rs had 7 ping_peer_* tests; head had 3 renamed ping_peer_health_* tests. Restore the 4 missing: the /ready 200 direct happy path, the /ready 503 + /health 200 must-not-be-ready case, the legacy fallback with explicit expect-count, and the shared-deadline probe. No production code change; the function signatures are unchanged. Verified: cargo fmt --check, cargo clippy -- -D warnings, 9 gossip tests pass, 53 deny_harness tests pass (including assert unit tests).
|
Both findings from the 2026-09-08 review are closed on 4c42d78. CodeQL cleartext-logging (P1): replaced the interpolated Gossip readiness tests (P2): restored the 4 tests dropped in the lib+bin split: Per your close-out guidance: no boot-path changes, no harness scope expansion, |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
LGTM (for real this time lol)
4 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