From 49639108de3bc2241712118b3cea8ab86d91507f Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Wed, 15 Jul 2026 23:05:58 -0500 Subject: [PATCH 1/3] docs(agents): add AGENTS.md carrying the authorization rules PRs most often break Encode the contributor-facing gate rules (the three owner-gate forms in use and the by-design non-owner exceptions, path-scoped reads and the listings contract, denial semantics, deny-test expectations, migration versioning, client denial surfacing, MSRV and the exact CI gate set) in a root AGENTS.md that coding agents load automatically, and point CONTRIBUTING.md at it. Enforcement labels state only what is on main today. --- AGENTS.md | 38 ++++++++++++++++++++++++++++++++++++++ CONTRIBUTING.md | 1 + 2 files changed, 39 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..aa5fd33a --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,38 @@ +# Contributor guide for coding agents + +This file carries the rules a patch author cannot recover from the code alone, for humans and coding agents alike. For build, test, and PR basics, read `CONTRIBUTING.md` first. + +## Authorization invariants (the rules PRs most often break) + +Every repo-scoped endpoint must bind the caller to an authorization decision before serving or mutating anything. + +- Owner-only mutations (visibility, webhooks, protected branches, merges) are gated against the repo owner. Three gate forms are in use and all are recognized: webhooks and merges call `require_repo_owner` (`crates/gitlawb-node/src/api/mod.rs`), visibility uses its module-local `require_owner`, and protected branches compare inline with `did_matches`. Use the form the surrounding module already uses. +- Not every write is owner-only, and that is by design: `star_repo`/`unstar_repo` bind to the signer (they still require repo read access), `register_replica`/`unregister_replica` bind to the replica's own DID, the bounty actions (claim, submit, approve, cancel, dispute) have their own multi-party rules, and closing a PR or issue is owner-or-author. Do not add owner gates to these. +- Content-serving reads (blobs, trees, raw files) gate through `authorize_repo_read` with the specific path being served, so a withheld subtree is denied even on an otherwise-public repo; repo-level reads (listings) pass `"/"` by the helper's own contract. The source scans below cannot check this argument's value, so review will. +- A route that reaches repo data through a global id, with no repo in its path, or through an extractor shape the scans do not recognize still needs the same gate; the scans cannot see those. +- Read-surface denials must not reveal existence: a caller who may not read a repo (or a withheld subtree) gets the same 404 as a missing repo, never a 403. (Owner-gated mutations currently return 403 after the repo lookup; do not widen that shape to read paths.) +- Two tests in module `authz_guard` (`crates/gitlawb-node/src/api/mod.rs`) scan handler sources and fail on a repo-scoped handler without a recognized gate: `every_in_scope_mutation_has_its_gate` and `every_repo_scoped_handler_is_gated`. If they fail on your handler, add the gate. The second test carries a `known_ungated` allowlist of tracked gaps being closed; never add an entry to it. + +## Adding or changing routes + +- A new gated handler needs a test proving the gate fires: sign as a non-owner and as an anonymous caller, assert the exact denial status each way, and assert the body leaks nothing. The non-owner tests in `crates/gitlawb-node/src/test_support.rs` show the shape. +- A new mutation handler also gets a row in `every_in_scope_mutation_has_its_gate` naming its gate type; the per-handler row is the point of that test. +- A table-driven deny suite covering every deny-bearing route is in review (#194, #195). If it is present in your checkout, add a probe row there for any new deny-bearing route. + +## Removing a serving path + +When a fix removes a path that used to serve content, the same PR must add a test asserting the removed path now denies. The deletion is what turns that test green; without it the path can quietly come back. + +## Database migrations + +Schema changes are code-defined in `MIGRATIONS` (`crates/gitlawb-node/src/db/mod.rs`). Always append a new versioned entry; never edit an entry that has merged (operators deploy from main). Test runs build the schema from scratch, so editing an applied version stays green in CI while breaking every existing deployment. + +## Client behavior (`gl`, `git-remote-gitlawb`) + +When a node denies a request, surface the denial to the user. Never render a denial as an empty list or a silent success; that turns an authorization boundary into invisible data loss. + +## Toolchain and CI reality + +- MSRV is Rust 1.91 (`rust-version` in the workspace `Cargo.toml`). The MSRV CI job runs `cargo check --workspace --all-targets` on exactly 1.91, so check that an API you rely on is stable there; the full test suite runs on stable. +- CI blocks merges on: `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo test --workspace` (against Postgres), a release build, `cargo audit`, the MSRV check, and a Docker build. +- Conventional commit titles (`feat:`, `fix:`, `docs:`, ...) are required by convention and drive releases, but no CI job checks them; a bad title fails review, not the pipeline. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fd939c9f..4ad8e8c1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -54,6 +54,7 @@ Smart contracts live in a separate repo: [github.com/Gitlawb/contracts](https:// 3. **No breaking changes without discussion.** Open an issue first for protocol-level changes. 4. **Conventional commits.** Use `feat:`, `fix:`, `docs:`, `refactor:`, `test:`, `chore:`. Releases are automated by [release-please](https://github.com/googleapis/release-please) — your commit prefixes drive the next version bump. 5. **Format and lint.** Run `cargo fmt --all` and `cargo clippy --workspace --all-targets -- -D warnings` before submitting. CI will reject anything that fails these. +6. **Security-surface rules.** [`AGENTS.md`](AGENTS.md) carries the authorization invariants PRs most often break; it is written for coding agents and humans alike, so read it before touching endpoints, visibility, or migrations. ## What gets merged, what gets closed From 8397a4aedc95b29fe62238220ac4bc015c19c6b9 Mon Sep 17 00:00:00 2001 From: t Date: Fri, 24 Jul 2026 16:27:48 -0500 Subject: [PATCH 2/3] docs(agents): correct three inaccurate claims flagged in review Scope the denial-test guidance to each route's actual authorization rule instead of a blanket non-owner assertion, since several documented routes (star_repo, replica actions, bounty actions, author-based closing) intentionally authorize non-owners. Name the known_ungated exceptions (list_webhooks, list_replicas, list_protected_branches) instead of implying full repo-scoped read coverage. Describe the listed jobs as workflow checks that run on every PR rather than merge-blocking CI, matching the live main branch ruleset (one required review, no required status checks). --- AGENTS.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index aa5fd33a..179aeffd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,7 @@ This file carries the rules a patch author cannot recover from the code alone, f ## Authorization invariants (the rules PRs most often break) -Every repo-scoped endpoint must bind the caller to an authorization decision before serving or mutating anything. +Every repo-scoped endpoint must bind the caller to an authorization decision before serving or mutating anything. Three handlers are tracked exceptions still being closed, not yet gated: `list_webhooks`, `list_replicas`, and `list_protected_branches` are exempted via the `known_ungated` allowlist in `authz_guard::every_repo_scoped_handler_is_gated` (#94, PR #113). Do not treat their repo-metadata exposure as protected. - Owner-only mutations (visibility, webhooks, protected branches, merges) are gated against the repo owner. Three gate forms are in use and all are recognized: webhooks and merges call `require_repo_owner` (`crates/gitlawb-node/src/api/mod.rs`), visibility uses its module-local `require_owner`, and protected branches compare inline with `did_matches`. Use the form the surrounding module already uses. - Not every write is owner-only, and that is by design: `star_repo`/`unstar_repo` bind to the signer (they still require repo read access), `register_replica`/`unregister_replica` bind to the replica's own DID, the bounty actions (claim, submit, approve, cancel, dispute) have their own multi-party rules, and closing a PR or issue is owner-or-author. Do not add owner gates to these. @@ -15,7 +15,7 @@ Every repo-scoped endpoint must bind the caller to an authorization decision bef ## Adding or changing routes -- A new gated handler needs a test proving the gate fires: sign as a non-owner and as an anonymous caller, assert the exact denial status each way, and assert the body leaks nothing. The non-owner tests in `crates/gitlawb-node/src/test_support.rs` show the shape. +- A new gated handler needs tests proving its route-specific gate fires: exercise an unauthorized authenticated caller and an anonymous caller where applicable, assert the exact denial status each way, and assert the body leaks nothing. The non-owner tests in `crates/gitlawb-node/src/test_support.rs` show the shape for owner-gated routes. - A new mutation handler also gets a row in `every_in_scope_mutation_has_its_gate` naming its gate type; the per-handler row is the point of that test. - A table-driven deny suite covering every deny-bearing route is in review (#194, #195). If it is present in your checkout, add a probe row there for any new deny-bearing route. @@ -34,5 +34,5 @@ When a node denies a request, surface the denial to the user. Never render a den ## Toolchain and CI reality - MSRV is Rust 1.91 (`rust-version` in the workspace `Cargo.toml`). The MSRV CI job runs `cargo check --workspace --all-targets` on exactly 1.91, so check that an API you rely on is stable there; the full test suite runs on stable. -- CI blocks merges on: `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo test --workspace` (against Postgres), a release build, `cargo audit`, the MSRV check, and a Docker build. +- The PR workflow runs these jobs on every PR: `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo test --workspace` (against Postgres), a release build, `cargo audit`, the MSRV check, and a Docker build. The `main` branch ruleset currently requires one approving review but does not require these checks to pass before merge; a failing job should block review approval in practice, but nothing enforces that mechanically today. - Conventional commit titles (`feat:`, `fix:`, `docs:`, ...) are required by convention and drive releases, but no CI job checks them; a bad title fails review, not the pipeline. From e3f7d2de240af3c571d6391629a1f1dca319dfd4 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sat, 25 Jul 2026 12:00:33 -0500 Subject: [PATCH 3/3] docs(agents): add signature and certificate verification rules Three rules a patch author cannot recover from the code alone, all three found by review rounds on #224 and #236: Anchor the verifying key outside the artifact. Deriving it from a field of the object under verification proves self-consistency, not authenticity, and identities here are permissionless. Found on #236 in the gl client and again on #224 in the server-side twin, where a self-signed certificate verified against an unauthenticated route. Drive the accepting verdict in a test. #224's verify endpoint shipped with sixteen tests, every one asserting the invalid path, which left it unable to observe a forged artifact being accepted. Treat a signed payload's field set as a versioned format. #224 grew it from seven fields to thirteen with no discriminator and no fallback, so every certificate issued before it now reports as tampered. --- AGENTS.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 179aeffd..0ccd5a27 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,6 +19,12 @@ Every repo-scoped endpoint must bind the caller to an authorization decision bef - A new mutation handler also gets a row in `every_in_scope_mutation_has_its_gate` naming its gate type; the per-handler row is the point of that test. - A table-driven deny suite covering every deny-bearing route is in review (#194, #195). If it is present in your checkout, add a probe row there for any new deny-bearing route. +## Verifying signatures and certificates + +- Derive the verifying key from something outside the artifact you are checking: the node's own DID, a pinned or configured value, a peers lookup. A key read from a field of the object being verified (`Did::from_str(&cert.node_did)`) only proves the artifact is self-consistent, and identities here are permissionless, so anyone can mint a keypair, name it in the artifact, self-sign, and be told it is valid. A mismatch against that anchor has to fail the result, not log a warning. +- A handler whose correct answer is "valid" needs at least one test that drives it to valid using an artifact built to be accepted for the wrong reason: well-formed, fully populated, and forged. A suite that only asserts the invalid cases cannot tell "rejects what it should" apart from "accepts everything." +- The fields a signature covers are a versioned format. Adding one invalidates every artifact already signed, so give the payload a version, keep a path that verifies the older form, and add a test that verifies an artifact signed before your change. + ## Removing a serving path When a fix removes a path that used to serve content, the same PR must add a test asserting the removed path now denies. The deletion is what turns that test green; without it the path can quietly come back.