test(node): invariant deny-prober (driven registry + completeness cross-check)#195
test(node): invariant deny-prober (driven registry + completeness cross-check)#195beardthelion wants to merge 8 commits into
Conversation
…anches) 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.
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.
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.
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.
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.
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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:node
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] Cover the deferred read-gate routes in the completeness invariant
crates/gitlawb-node/tests/support/routes.rs:351
The registry explicitly leaves outget_issue,get_pr, PR diff/review/comment reads,get_cert,get_bounty,get_encrypted_blob, and path-scopedget_tree, although these handlers return the same 404 authorization denial before looking up their entities. U3 only drives registry rows, and U4 checks mounted rows plus owner-gate markers only, so removing or bypassingauthorize_repo_readon any of these endpoints leaves the new real-node sweep green. Seed the required entities and include the routes (with positive twins), or make the completeness check derive and require every mounted read-denying handler. -
[P2] Assert that hostile registry responses do not expose the private fixture data
crates/gitlawb-node/tests/deny_harness.rs:507
Every hostile probe callsassert_deniedwith an empty withheld-token list. That verifies only the status: a 403/404 body containing the private blob content, object ID, or repository metadata would pass even though this sweep claims the denial "leak[s] nothing." Carry the relevant private fixture tokens on the probes/rows and pass them toassert_deniedfor the affected read routes. -
[P2] Do not let inline owner gates bypass the registry drift check
crates/gitlawb-node/tests/deny_harness.rs:788
The orphan scan deliberately looks only forrequire_repo_owner(/require_owner(and excludesdid_matches, but existing owner-only handlers such asprotect_branchandunprotect_branchuse the latter idiom. A future mounted owner-only handler using that established pattern can be omitted fromdeny_bearing_routes()while U4 still passes, so the advertised completeness guard does not keep the owner-gate registry complete. Extend the classifier to recognize the owner-specific inline pattern (or otherwise tie mounted owner routes to the registry) rather than excluding that gate class.
beardthelion
force-pushed
the
feat/invariant-deny-prober
branch
from
7841244 to
5a4dc1c
Compare
…#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.
|
All three fixed; each guard now goes red when the protection it checks is removed. Read-gate orphans — added a read-gate orphan guard symmetric to the owner one: it scans Sweep leak assertion — the private fixture now seeds a distinctive secret and captures its blob OID; the read-gate hostile probes carry those tokens into Owner did_matches idiom — the owner-orphan scan now folds 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
-
[P2] Exercise read denial for a signed non-reader
crates/gitlawb-node/tests/support/probe.rs:185
EveryReadGatehostile probe is anonymous, although the fixture also creates a signedstranger. This leaves the signed-non-reader branch of every private read untested: a regression that keeps anonymous callers at 404 but treats any valid HTTP signature as authorized exposes private repositories while the registry sweep remains green. Generate aSigner::Stranger404/no-leak hostile probe for each read row (and adjust the hostile-count invariant accordingly). -
[P2] Do not exempt probeable private root reads from the runtime sweep
crates/gitlawb-node/tests/deny_harness.rs:1082
get_star_status,get_icaptcha_proof, andreplicate_encrypted_blobsare allowlisted despite needing no additional fixture entity. The new guard only proves that their functions retain anauthorize_repo_read(/visibility_check(marker; it never drives a request. Consequently an early return or conditional bypass that leaves the marker in the body can leak private-repo metadata while every new test passes. Add these asReadGaterows with hostile and authorized probes rather than treating them as a source-only exemption. -
[P2] Keep the protected-push owner gate under a real probe
crates/gitlawb-node/tests/deny_harness.rs:929
The owner-marker scan recognizes thedid_matches(&auth.0, &record.owner_did)check ingit_receive_pack, then removes that handler because its registry row only tests the unsigned 401 path. A signed stranger pushing to a protected branch therefore never reaches a test; deleting or inverting the 403 atsrc/api/repos.rs:897-923leaves both the sweep and completeness checks green. Add a signed-stranger protected-branch push probe with an owner control, or keep this gate in the completeness invariant until it is driven. -
[P2] Withhold private repository metadata in the denial-body assertion
crates/gitlawb-node/tests/support/probe.rs:199
The leak assertion carries only the private blob content and its object ID.Fixture::seeddiscards the private repository record/ID, so a 404 that serializes the private record's internal UUID or visibility metadata still passesassert_denied. That leaves the repository-metadata portion of the earlier no-leak request unverified. Preserve distinctive private repository metadata in the fixture, include it in the hostile probes' withheld tokens, and add the corresponding synthetic leaking-body check.
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.
|
All four addressed on Signed non-reader (F1). Every ReadGate row now drives a Driven, not source-only (F2). Protected-push 403 under a real probe (F3). Added Repo metadata withheld (F4). The fixture preserves the private repo id and the read-gate no-leak assertion withholds it; a synthetic 404 body carrying the UUID fails No production behavior ships — the auth-line reverts were RED-proof scaffolding, all restored. |
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] Restore the configured shutdown-grace deadline
crates/gitlawb-node/src/lib.rs:539
This replaces the base branch'sdrive_serve_with_gracerace with a plainwith_graceful_shutdown(...).await, then explicitly discardsgraceat line 571. Axum will now wait indefinitely for a stalled in-flight request after SIGTERM, so teardown and a deployment restart can hang forever despiteGITLAWB_SHUTDOWN_GRACE_SECSdocumenting that the server exits after that duration. Rebase onto the target (or restore the deadline race and its tests) rather than rolling this behavior back. -
[P1] Do not revert the atomic identity-key creation path
crates/gitlawb-node/src/lib.rs:1007
The target branch'screate_new(...).mode(0o600)/AlreadyExists handling is replaced byexists()followed byfs::write()and a later chmod. Concurrent first starts can both generate and overwrite the same PEM, leaving running nodes with divergent DIDs; on Unix the PEM is also created with umask-derived permissions before line 1027 narrows it, exposing the private signing key during that window. Keep the atomic create, winner-load retry, and existing-key permission tightening from the base branch. -
[P2] Exercise both sides of the owner-or-author close rule
crates/gitlawb-node/tests/support/probe.rs:65
The fixture seeds each close target withauthor_did == owner_did, and the generated positive probe only signs as that same key. Consequently the new sweep passes if either close handler loses itsis_authorbranch: a real contributor can no longer close their own PR/issue, but the owner-as-author control still reaches the handler and the stranger still receives 403. Seed these resources with a distinct author and add an author-signed positive probe (with the owner control retained) so the declared owner-or-author contract is actually load-bearing. -
[P2] Drive the remaining deferred private read endpoints
crates/gitlawb-node/tests/deny_harness.rs:1073
get_issue,get_pr, the PR/comment/review reads,get_cert,get_bounty,get_encrypted_blob, and path-scopedget_treeare still source-only allowlist entries. The new guard only checks that their bodies contain an authorization-marker call, so an early return or ignored authorization result that leaves that call present keeps the guard green while exposing private data. This leaves the earlier request to cover the deferred 404-denying reads unresolved; seed the necessary entities and run their anonymous/signed-stranger denial and authorized positive probes.
2 participants
Stacks on #194 (base
feat/real-node-deny-harness), reusing its real-node spawn rig and RFC-9421 signing client. Review after #194.This turns the hand-written deny cases into a driven registry: one declarative table of every route that carries a runtime deny, and a sweep that boots a real node and drives each one hostile-then-authorized.
What's here
tests/support/routes.rs): every deny-bearing route as a row (owner-gate 403, read-gate 404, signature 401), each classified by reading its handler, not the route name. 12 owner-gate + 1 signature + 14 repo-scoped read rows.tests/support/probe.rs): a two-repo fixture (public owner-gate substrate, private read-gate substrate, plus a seeded PR and issue so the author-or-owner close gates are reached rather than 404'd on an absent entity) andprobes_for, which expands each row into the hostile request plus a positive twin so a deny for the wrong reason cannot false-pass.deny_bearing_registry_...): walks the registry against a real node. Hostile probe must return the exact deny and leak nothing; owner-gate twins must reach the handler (not 403); read twins must return a non-empty 2xx (an empty 2xx is a denial rendered as success). A terminal invariant asserts one hostile probe per row was actually driven, so the sweep can't pass by testing nothing.completeness::..., no DB): a source scrape ofserver.rs(multiline.routeparser, floor 90) that keeps the registry honest: no row points at a route that no longer mounts, and no handler carrying an owner-gate marker escapes the registry. Complements the in-crateauthz_guardegress guard (which proves handlers are gated) by proving the deny-bearing ones are actually driven, and reaches the non-API git mounts it never sees.Verification
Every deny class was mutation-verified load-bearing, RED-then-GREEN by execution: breaking
did_matchesdrove the owner-gate hostile to 500 and, inverted, drove the owner twin to 403; neuteringauthorize_repo_readdrove the read hostile to 200 and, as always-deny, the read twin to 404; the signature assertion was isolated atgit-receive-pack; both completeness floors and both drift guards were tripped and named their target. All reverted. 22 harness tests green.CI already runs this file on every PR (the
--features test-harness --test deny_harnessstep added in #194), so U3 and U4 run there with no config change.