Add a completeness fence over the GraphQL mutation surface#219
Open
beardthelion wants to merge 4 commits into
Open
Add a completeness fence over the GraphQL mutation surface#219beardthelion wants to merge 4 commits into
beardthelion wants to merge 4 commits into
Conversation
added 2 commits
July 18, 2026 17:22
Gate the GraphQL mutation surface the way REST is gated, as a SEPARATE guard with its own bucket table (a guard over a heterogeneous set must classify per member, INV-1). every_graphql_mutation_has_its_gate asserts the mutation set is complete (every mutation classified) and that each carries its gate marker. Completeness is derived from async-graphql's own introspection: the field names come from the engine as structured data, so no source-text or SDL-format shape (a brace or async fn in a comment or string, a whitespace/generic/skip variant, a brace in a rustdoc description) can hide a live mutation. A new unregistered mutation, including a repo-write, fails the build. The marker check (does each resolver body call its gate) is a best-effort static backstop, bounded to each resolver's own comment-masked, string-aware body. It is explicitly not the completeness guarantee and not load-bearingness: a marker inside a string literal is a documented vacuity residual, and the adversarial sqlx tests in graphql/mutation.rs are the real proof the gates reject. Verified by execution: an unregistered mutation reddens (unclassified), a removed did_matches gate reddens (missing marker), the clean tree is green; no DB, no feature flag.
…tate README's known limitations, the OSS-readiness live-network blockers, and the maintainer roadmap still listed GraphQL mutation auth as an open item, which contradicts #87: GraphQL mutations already require a verified signer bound to the acting DID. Reconcile each to the shipped reality while keeping the genuinely-open nuance: the GraphQL mutation surface is the agent-task queue, repo writes stay REST-only, and no owner-gated repo-write mutation is exposed over GraphQL. The every_graphql_mutation_has_its_gate fence forces any new mutation to be classified in its table; owner-gate and REST parity stay the INV-1 review check.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughGraphQL authorization coverage now verifies the exact mutation schema and required resolver gates, with comment- and string-aware source inspection. README and security documentation now describe verified signer requirements, agent-task-only mutations, and REST-only repository writes. ChangesGraphQL authorization fence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:node
beardthelion
pushed a commit
that referenced
this pull request
Jul 19, 2026
…ber (#195) Resolve the three review findings on head 07abcc6. Bound the two new real-node probes (protected-branch push, PR-diff) on a timeout-bearing client, so a wedged git subprocess fails the suite rather than hanging it until CI kills the job. Withhold the pushed secret blob's OID (full and [..12] short form) in the PR-diff denial. The old branch was dead code: the OID was never in the fixture seed map, so build_branch_push_body now returns the pushed blob OIDs. Drive every caller-self runtime deny the registry claimed but only source-marker checked. create_task, claim_task, and register are a new SignerSelfGate class (a body DID field bound to the authenticated signer); complete_task and fail_task are a MultiPrincipalGate assignee arm (the caller must equal the task's stored assignee). Each runs hostile-then-authorized against a real node. A completeness guard, derived from a scan of src/api, now requires every did_matches(caller, X) handler to be a driven registry row, and the module doc is reconciled to the REST surface (the parallel GraphQL mutation gates are fenced separately, #219). Every gate is mutation-verified load-bearing: reverting a production did_matches line drives its hostile probe RED, and the completeness guard goes RED when a driven row is dropped or the classifier is starved. No production code changed.
jatmn
requested changes
Jul 21, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Discover mutation fields without visibility-filtered introspection
crates/gitlawb-node/src/api/mod.rs:334
The completeness check treats__schema { mutationType { fields } }as the authoritative set, but async-graphql omits fields annotated with#[graphql(visible = false)](or a false visibility predicate) from that introspection response while leaving them executable by clients that know their name. A future unauthenticated mutation can therefore be added as hidden: it never appears infields, requires no table row or marker, and both set checks stay green. This directly defeats the advertised guarantee that no live mutation can go unclassified. Please inspect the unfiltered registered schema fields, or fail the guard on hidden mutation fields, and add a regression test for that path.
kevincodex1
pushed a commit
that referenced
this pull request
Jul 22, 2026
…ss-check) (#195) * test(node): U1 deny-bearing route registry (owner-gate + signature tranches) Foundation for the invariant deny-prober (plan 002): a declarative registry of ONLY the routes that carry a runtime deny (owner-gate 403 / read-gate 404 / signature 401), each classified by reading its handler and recording the fn name. Owner-gate (12 rows: merge/close_pr, close_issue, webhooks, labels, protect, visibility set/remove/list incl the GET-is-403 correction) and signature-required (git-receive-pack) tranches are verified; read-gate tranche stubbed with a TODO (per-handler 404-vs-filter verification pending). A consistency self-test asserts no dup method+path, every row names its handler, and every read-gate row carries a positive-twin Reach. Decoupled from the #194 primitives (U1 uses none), so classification is not hostage to harness churn. * test(node): U2 fixture state matrix + per-gate-class probe generators Two-repo fixture (public owner-gate substrate + private read-gate substrate) plus probes_for(): each deny-bearing row expands to a hostile probe asserting the exact deny status and a positive twin (owner-reachability for owner-gate, authorized read for read-gate) so a deny for the wrong reason cannot false-pass. Three self-checks cover the owner-gate, read-gate, and signature classes. * test(node): U1 read-gate tranche (14 repo-scoped 404-deny reads) Each row's handler was read to confirm it gates on "/" via authorize_repo_read / visibility_check and returns RepoNotFound (404) to a non-reader of a private repo, with the owner re-read as the ReaderReads positive twin. Sub-entity gets (get_issue/get_pr/get_cert/get_bounty and the comment/review lists) are deferred to the fixture expansion that seeds them; global list-filter surfaces and the KNOWN_UNGATED reads are excluded with reasons. Runtime behavior is asserted by U3. * test(node): U3 drive the deny-bearing registry over a real node Boots a real node, seeds the two-repo fixture (plus a PR and an issue so the author-or-owner close gates are reached, not 404'd on an absent entity), then walks every deny-bearing route: the hostile probe must return the exact deny status and leak nothing, and the positive twin must reach the handler (owner: not 403; read: 2xx). A terminal invariant asserts one hostile probe per row was driven and the count equals the registry, so a row that produced no probe fails loud rather than passing vacuously. Adds seed_pr/seed_issue to TestNode. Mutation-verified load-bearing: breaking did_matches drove the merge_pr owner-gate hostile to 500 (RED, named); neutering authorize_repo_read's deny drove the get_repo read-gate hostile to 200 (RED, named); both reverted green. * test(node): U4 completeness cross-check keeps the registry from drifting A pure source scrape of server.rs (multiline .route parser, floor 90) that the runtime sweep cannot give: no deny-bearing row points at a route that no longer mounts (anti-stale), and no handler carrying an unambiguous owner-gate marker (require_repo_owner / require_owner) escapes the registry (orphan). The api dir is read at test time so a new module is covered. Complements authz_guard (which proves handlers are gated) by proving the deny-bearing ones are actually driven, and reaches the non-API git mounts authz_guard never sees. Mutation-verified: a bogus row path tripped anti-stale (named the row); removing the merge_pr row tripped the orphan guard (named merge_pr); both reverted green. * test(node): close vacuous-green gaps in the deny prober's own assertions Vetting the harness against itself surfaced branches that ran green but were never exercised adversarially: - Ok2xx twin dropped its unused token Option; the arm now asserts the authorized read returns a NON-EMPTY 2xx, so an empty-200 denial-rendered-as-success is caught (executed by every read twin). - U4 orphan guard gains a floor on the owner-marker scan (>=6), so a marker-scan regression to zero fails loud instead of passing by checking nothing. - Added probe unit tests that execute the previously-uncovered probes_for arms: the SiblingPublic path-scoped twin, and the Reach::None fail-loud panic. All 22 harness tests green. Every send_probe/probes_for branch is now executed; the Deny(401), Not403, and Ok2xx-status arms were additionally mutation-verified RED (signature-expect flip; did_matches->false; authorize_repo_read->deny) and both U4 floors RED via threshold flips, all reverted. * test(node): close 3 completeness gaps in the deny prober's own guards (#195) jatmn's review flagged three ways the prober's self-guards stay green when the protection they check is removed: - F1: the U4 completeness check enforced owner-gate orphans but not read-gate ones, so dropping authorize_repo_read on get_issue/get_cert/ get_bounty/etc. left the sweep green. Add a symmetric read-gate orphan guard that scans src/api for authorize_repo_read/visibility_check handlers and requires each to be a driven ReadGate row or an enumerated READ_GATE_NOT_DRIVEN entry with a reason, replacing the free-text prose. A staleness check keeps the allowlist from carrying dead exemptions. - F2: the U3 registry sweep called assert_denied with an empty withheld list, proving status only. Seed the private fixture with a distinctive secret and its blob OID, carry them on the read-gate hostile probes, and pass them through so a denial body echoing withheld data fails. - F3: the owner-gate orphan scan matched only require_owner/require_repo_owner and missed the inline did_matches(caller, &record.owner_did) idiom that protect_branch/unprotect_branch use. Fold in a has_owner_did_matches discriminator that catches the owner form without false-matching the signer-self (register_replica) or author (close_pr) did_matches forms, pinned by unit tests on the exact bodies. * test(node): make the deny-prober guards load-bearing (#195, F1-F4) Resolves jatmn's four INV-21 completeness findings — guards that were green but not load-bearing: F1: every ReadGate hostile probe was anonymous, so the signed-non-reader branch of each private read was untested — a regression treating any valid signature as authorized would leak while the sweep stayed green. Add a Signer::Stranger 404/ no-leak probe per read row, with a dedicated readgate_stranger counter so a dropped probe fails loudly. RED: authorize any signed caller -> the stranger probe leaks (200) while the anon probe stays green. F2: get_star_status, get_icaptcha_proof, and replicate_encrypted_blobs were source-only exemptions (a marker scan, never driven). Drive them as real ReadGate rows and remove the allowlist entries. RED: a runtime bypass that keeps the authorize_repo_read marker leaks -> the driven probe RED, the source scan green. F3: git_receive_pack's protected-branch owner 403 was never driven (its registry row is the unsigned-401 path). Add signed_stranger_protected_branch_push_is_forbidden (a signed non-owner push to a protected branch -> 403, owner control -> not-403), plus a seed_protected_branch harness helper. RED: invert the gate -> the probe RED. F4: the read-gate no-leak assertion omitted the private repo id (seed discarded it). Preserve it on the Fixture and withhold it; a synthetic 404 body carrying the UUID must fail check_denied. RED: a UUID-bearing body passed before the token was added. Each guard proven load-bearing by reverting the exact production auth-line (reverts not shipped). deny_harness suite 30 passed, fmt + clippy clean. * test(node): drive every arm of the multi-principal close/dispute gates close_pr, close_issue (owner-or-author) and dispute_bounty (creator-or-claimant) were each driven with a single owner/creator twin, so reverting the untested arm stayed green. Model gates by their authorizing-principal set (MultiPrincipalGate + Principal), seed distinct author/creator/claimant identities, and emit one Not403 twin per arm plus the stranger 403. registry_internal_consistency now invokes probes_for and asserts a twin per declared arm, so a future multi-arm handler cannot register with an untested arm. Each arm proven load-bearing: reverting is_owner/is_author on close_pr/close_issue and is_creator/is_claimant on dispute_bounty turns that arm's twin RED. * test(node): drive the deferred private reads with per-read leak assertions The deferred reads (get_issue, get_pr, get_pr_diff, list_issue_comments, list_reviews, list_comments, get_cert, get_bounty, path-scoped get_tree) sat in READ_GATE_NOT_DRIVEN, checked only for an authorize_repo_read marker, so an ignored result or early return stayed green. Seed each sub-entity in the private repo (issues/PRs via the real create handlers, a cert via a signed owner push, a private-repo bounty), thread id-keyed paths through IdSource, and drive each as a ReadGate row: anon/signed-stranger 404, owner 2xx. Each seeded entity carries a distinctive marker added to the read-gate withheld set, so a 404 that leaks the entity's own content fails (not just a status check). get_pr_diff exercises both its repo-root gate and its per-path visibility_check loop. get_encrypted_blob stays deferred (owner twin needs live IPFS; reclassified structurally next). Load-bearing: an ignored-result bypass on each read turns its hostile probe RED; a 404 leaking a seeded marker fails the body assertion. * test(node): close the prose-excuse hatch with a mounted-route completeness gate READ_GATE_NOT_DRIVEN allowed a free-text reason to park a read-gated handler out of the runtime sweep. Replace the reasons with a closed NotDrivenReason enum (ReadGatingMutation, GitSmartHttpRead, ContentAddressedRead, GlobalListFilter, ExternalDependencyUnavailable); get_encrypted_blob moves to the last. A drivable 404-deny GET no longer type-checks a reason and is caught by a driven-not- relabeled assertion. Add a completeness gate that derives the required read-gate set from the mounted route table (scrape_mounts, INV-21b), not only the source-marker scan: every mounted repo-scoped GET must be a driven ReadGate row, a deny-bearing GET, structurally excused, or a declared public GET — else it fails. Catches a read that gates via a helper the marker scan does not recognize. Load-bearing: re-parking a driven read trips the relabel assert; removing a driven ReadGate row trips the mounts cross-check. * test(review): harden multi-principal twin check against signer collision Add a distinct-signer assertion to multi_principal_rows_emit_one_twin_per_arm: the per-arm any() check is satisfied by a single twin when two Principals map to the same Signer, so a signer_for_principal collision would leave one arm silently untested. Assert the emitted twins carry principals.len() distinct signers (proven RED by colliding Author->Owner). Also correct the get_cert / priv_markers comments, which claimed a withheld cert-content marker that is not seeded — no cert marker is needed because get_cert 404s at the repo-root gate before the cert is fetched, so cert fields cannot reach the deny body. * style(test): apply rustfmt to the deny-prober additions * test(node): drive the bounty mutation gates and pin fixture object format (#195) - submit/approve/cancel_bounty become MultiPrincipalGate registry rows with status-appropriate bounty fixtures (claimed/submitted/open): their status check fires before the auth gate, so each row seeds its own bounty through the real API and the arm twins consume the status - pin --object-format=sha1 in both fixture worktrees so a sha256 default object format cannot break the OID equality with the served sha1 repos * test(node): drive the caller-self task/register gates in the deny-prober (#195) Resolve the three review findings on head 07abcc6. Bound the two new real-node probes (protected-branch push, PR-diff) on a timeout-bearing client, so a wedged git subprocess fails the suite rather than hanging it until CI kills the job. Withhold the pushed secret blob's OID (full and [..12] short form) in the PR-diff denial. The old branch was dead code: the OID was never in the fixture seed map, so build_branch_push_body now returns the pushed blob OIDs. Drive every caller-self runtime deny the registry claimed but only source-marker checked. create_task, claim_task, and register are a new SignerSelfGate class (a body DID field bound to the authenticated signer); complete_task and fail_task are a MultiPrincipalGate assignee arm (the caller must equal the task's stored assignee). Each runs hostile-then-authorized against a real node. A completeness guard, derived from a scan of src/api, now requires every did_matches(caller, X) handler to be a driven registry row, and the module doc is reconciled to the REST surface (the parallel GraphQL mutation gates are fenced separately, #219). Every gate is mutation-verified load-bearing: reverting a production did_matches line drives its hostile probe RED, and the completeness guard goes RED when a driven row is dropped or the classifier is starved. No production code changed. --------- Co-authored-by: t <t@t>
added 2 commits
July 22, 2026 08:42
Resolves jatmn's #219 P1. The every_graphql_mutation_has_its_gate completeness fence enumerated mutation fields via async-graphql introspection, which filters out `#[graphql(visible = false)]` fields. Those fields stay executable, so a future hidden mutation would need no registry row and no marker while both set-equality checks stayed green -- defeating the fence's guarantee that no live mutation goes unclassified. (Confirmed by execution: a synthetic visible=false mutation is absent from introspection, executable, and present in schema.sdl().) Source completeness from the engine's own SDL instead. A new pure helper mutation_field_names_from_sdl parses schema.sdl() with async-graphql's parser AST (async_graphql::parser::parse_schema) and reads the mutation type's fields; SDL includes visible=false fields and the AST parse is immune to a brace in a field description. Introspection is kept as a cross-check: any field present in SDL but hidden from introspection now fails named as a forbidden hidden mutation. The stale doc comments (which claimed introspection could not hide a field) are corrected. A self-checking version-safety test builds a synthetic schema with a visible=false mutation and asserts the SDL-sourced set includes it while introspection omits it -- so if a future async-graphql ever filters SDL by visibility, that test goes RED rather than the gap reopening silently. Load-bearing verified by execution: stubbing the SDL enumeration to empty turns four tests RED, including the fence itself; restoring returns green. Only api/mod.rs changed; the mutation resolvers are untouched. `cargo test -p gitlawb-node --bins` green (495); clippy and fmt clean.
…recated
An adversarial review of the SDL switch found three gaps:
- The fence's completeness source (SDL) and its hidden-mutation cross-check were
not load-bearing: because the real MutationRoot has no visible=false field,
reverting `fields` back to introspection and dropping the cross-check left the
whole suite green (INV-21). Extract the sourcing + cross-check into a generic
`assert_mutation_fence<Q,M,S>` that the real fence AND a synthetic hidden schema
both drive. A new #[should_panic] test (fence_rejects_a_hidden_mutation) feeds a
visible=false mutation that IS registered, so only the cross-check can fire;
reverting the SDL source or deleting the cross-check now changes/removes that
panic and turns the test RED (verified).
- The introspection cross-check used bare `fields { name }` (GraphQL default
includeDeprecated:false), so a `#[graphql(deprecation = "...")]` mutation --
executable and present in SDL -- would false-red as a hidden visible=false
mutation. Use `fields(includeDeprecated: true)`; a new test
(fence_admits_a_deprecated_mutation) pins it, going RED without the flag.
- schema_lists_exactly_the_registered_mutations still sourced from introspection
and its comment called introspection "the source of truth", contradicting the
fix. Retargeted at the SDL set with a corrected comment.
Also folds the two introspection navigations into one generic helper
(introspection_mutation_fields) and points the version-safety test at it, so
there is a single introspection query. cargo test -p gitlawb-node --bins green
(497); clippy and fmt clean. Only api/mod.rs changed.
Collaborator
Author
|
Addressed in 11db151, with follow-up hardening in f3d91fa. You were right: async-graphql omits The fence now sources completeness from the SDL instead. A helper parses Two regression guards keep this load-bearing rather than trusted. The fence's sourcing and cross-check are extracted into a generic helper that a synthetic schema with a visible=false mutation drives through the exact real-fence logic and must reject; reverting the SDL source back to introspection turns that test red (the real MutationRoot has no hidden field, so nothing else would catch it). And a deprecated-but-visible mutation must be admitted: the cross-check uses
@jatmn ready for another look. |
jatmn
requested changes
Jul 22, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Make the mutation fence prove signer enforcement, not just a matching substring
crates/gitlawb-node/src/api/mod.rs:286
The current resolvers do require and bind a verified signer, so this is not a present unsigned-write bypass. However, every fence entry acceptsdid_matches(as its entire marker; a future regression increateTaskorfailTaskcan replacelet caller = require_signer(ctx)?with input-derivedcallerwhile retainingdid_matches(caller, ...). The SDL/table/marker tests then stay green even though/graphqlis only optionally authenticated and the mutation can write for an unsigned request. Require the signer gate (and the appropriate binding) in the fence, and add unsigned/mismatched-request coverage for every registered mutation. -
[P3] Correct the stale discovery claim in the PR description
PR description, “Fence” section
The description still says discovery comes from async-graphql introspection, whereas this revision deliberately discovers fields fromschema.sdl()and parses its AST. That distinction is security-relevant—the prior review identified visibility-filtered introspection as bypassable—so the PR text currently describes the superseded implementation and conflicts with both the code and the later author reply.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GraphQL mutations have required a verified signer since #87, but nothing forced future mutations to stay gated, and README / OSS-readiness / roadmap still listed the auth as an open item. This adds the missing guard and reconciles the claims.
Fence
every_graphql_mutation_has_its_gate(crates/gitlawb-node/src/api/mod.rs) gates the GraphQL mutation surface the way REST is gated: a separate guard with its own bucket table (a guard over a heterogeneous set classifies per member). It asserts the mutation set is complete (every mutation classified) and that each carries its gate marker.Discovery comes from async-graphql's own introspection, so the required set is derived from the mounted schema, not a hand-list: no source-text or SDL-format shape can hide a live mutation, and a new unregistered mutation (including a repo-write) fails the build. RED-verified both ways: an unregistered mutation and a removed
did_matchesgate each redden the guard; clean tree green. No DB, no feature flag.Scope and limits
The marker check (does each resolver body call its gate) is a best-effort static backstop, not the completeness guarantee and not load-bearingness: a marker inside a string literal is a documented vacuity residual, and the adversarial
#[sqlx::test]s ingraphql/mutation.rsare the real proof the gates reject.Docs
README known-limitations, the OSS-readiness live-network blockers, and the maintainer roadmap are reconciled to the shipped state: mutations require a signer; the GraphQL surface is the agent-task queue; repo writes stay REST-only; no owner-gated repo-write mutation is exposed over GraphQL.
Test-only plus docs; no request-path change.
Summary by CodeRabbit
Security
Documentation