[bradseiler]

Summary

Classify all 77 registered relay routes and bind community-ban checks to signed requests with exact allowed controls. New or opaque router composition fails the inventory guard, including unclassified .merge(...) calls and an existing root merge moved into a nested target.

Replace broad positive-status allowances with case-specific statuses and response bodies or shapes. Ordinary-member routes also exercise a clear owner-delegated agent. GIF controls require no provider key. Media reads, Git reads and workflows use absent resources; malformed JPEG upload and an invalid Git-push repository name assert their exact validation errors.

The moderation-read gate now uses the signed NIP-98 creation time for owner-proof validation. Integrated invitation, workflow and live-session regressions replace stale pending-sibling rows. The inventory requires zero pending cases.

Related issue

BUZZ-269. Stack: #8529 → #8241 → #8239 → #8235. #8237 and #8242 are merged. Shared preparation fixes already on main were dropped from this layer.

Testing

The integrated candidate at 45efe62f passed cargo test --locked -p buzz-test-client --test community_ban_routes -- --ignored --nocapture (3/3) and --test regression_relay_admin_ban_gate -- --ignored --nocapture (1/1) against a private relay, PostgreSQL and Redis. The roster regression used its REPRO_RELAY_HTTP and REPRO_HOST settings.

The relay, database and test-client source matches this rebased layer at 69edf322. Object storage used a private missing-object response fixture; this is not MinIO upload/download validation. The Git conformance startup probe was disabled. Failed launcher and fixture-assumption attempts remain in the local receipts; the final controls retain exact outcomes.

Independent source review accepted the corrections. Previous-head review completion was removed. Ready for review at Brad’s request; fresh review and human verification are not attested by this status change.

Generated with Codex

[github-actions]

🔐 Codex Security Review

Status: review required for the current range.

The current range is fae3f0346c44bbdf9629a7b5967965391c2bd22f...69edf3220e5a60853355d39608d75321e29025f9.
A new review must complete for this exact range. When manual authorization
is required, a user with write access must comment exactly
@buzz-security-review 69edf3220e5a60853355d39608d75321e29025f9 to authorize a new review.
Any previous review applies only to its recorded range.

[TheSentinel454]

authorize_moderation_read (api/bridge.rs:~2736) now passes signed_created_at where it used to pass None. Previously, a moderator agent whose owner was freshly banned via NIP-OA could still read the moderation queue. That's a security fix, and moderation_read_honors_fresh_and_stored_banned_agent_owners pins it well. Could it be named in the title and summary, or split into its own PR so it can be reverted or backported on its own?

[wpfleger96]

🤖 Thanks for this. Two blocking problems, both in what the new coverage proves rather than in production behavior: the inventory guard misses a supported router shape, and the positive controls pass when the allowed operation is broken. Both were reproduced by mutation at this head.

Blocking

1. A second direct mount of the same router escapes the inventory (correctness). check-community-ban-route-inventory.py L286–304 keeps one prefix per router builder (prefixes[candidates[0]] = prefix), and L340–363 emits that builder's routes only under that one prefix. Adding .nest("/extra", api::buzz_v1::router(state.clone())) before the existing /buzz/v1 mount exposes /extra/me/sidebar and /extra/me/read-state on the relay, but the guard still exits 0 and reports 74 routes. An ordinary added .route(...) does fail it, as intended. This is the direct, literal .nest shape TESTING.md says is supported, not one of the listed gaps (route_service, macros, opaque composition), so new endpoints can skip both classification and the signed route matrix.

Fix: collect every mount prefix per builder (or fail closed when a builder is mounted more than once), and add a guard regression with two direct mounts of the same builder.

2. Positive controls accept unrelated failures (correctness). assert_allowed_control in community_ban_routes.rs L443–465 accepts any 2xx, 400, 404, 409, 415, or 422 for every control case and only checks that the body isn't a ban denial. Keeping the /count membership/ban gate but returning 404 immediately after it leaves all three new tests passing (community_http_ban_matrix, banned_root_websocket_is_refused, ban_is_scoped_to_host_resolved_community). Removing the gate instead does fail the matrix (/count returns 200 {"count":1} to the banned member), so the ban assertions are sound. The problem is that the unbanned side doesn't prove the operation still works, and moderation reads with valid inputs could likewise return 400/404 and pass.

Fix: give each control case its expected status and response: 200 plus response shape for valid reads and submissions (e.g. /count returns a numeric count), and the exact validation or missing-resource response for fixtures that are deliberately malformed or absent. Seed an accessible resource where resource authorization is part of what the case claims.

Scores

• Minimalism 8/10: the PR also carries the mesh receive, scraper, and push-lease fixture fixes that #8237 carries. They look correct; landing them once would keep this diff to the coverage change.
• Elegance 9/10: the route table, guard script, and signed matrix are a clean split, and the public/internal classifications and the two pending invitation/webhook rows are explicit rather than silently skipped.
• Correctness 7/10: the two items above. What holds up: ban denials require 403 plus the route's specific denial, not any 401/404; an empty matrix fails the case-set comparison; the ignored tests run by name in CI (Relay E2E and PostgreSQL jobs) and pass locally. The bridge.rs moderation-read change is a real fix: fresh owner proofs are now evaluated at the verified NIP-98 time, like the other NIP-98 routes, with a PostgreSQL regression. The mesh_boot.rs echo loop keeps one in-flight receive across drain ticks and drops it only on shutdown. The ingest.rs / relay_admin.rs edits are comment-only, and the 200 → 403 timed-out-admin expectation matches the shared gate on main.

Nonblocking

• The banned delegated agent has no matching unbanned-agent control outside the moderation reads. On /buzz/v1/* the denial check only looks for code == "forbidden", on media only for relay membership required, and HEAD is bodyless, so those cases can't tell an owner-ban denial from some other agent-specific denial. A clear owner/agent pair per case would fix that.
• The guard ignores fallback / fallback_service, including the SPA fallback in router.rs. TESTING.md lists route_service / nest_service as known gaps but not this one.
• TESTING.md restates the unmerged invitation contract (72h / 30-day lifetimes, v1 invalid_after), which will drift if #8237 changes.
• The description doesn't mention the moderation-read fix in bridge.rs, and still says "Draft only" while the PR is ready for review.

[wesbillman]

🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/source-visible flows) lanes.

Two existing P2 coverage defects remain at this head, independently confirmed from source. They are already described in review 5511682177, so I am linking that feedback rather than duplicating inline comments:

• Preserve every direct router mount. In scripts/check-community-ban-route-inventory.py:286–304, prefixes[candidates[0]] = prefix overwrites an earlier mount; lines 342–363 emit only the retained prefix. A second direct mount of the same builder can therefore expose endpoints absent from the inventory without changing the guard’s result. Collect all prefixes, or fail explicitly on multiple mounts, and cover that supported composition shape with a guard regression.
• Make allowed controls assert the expected outcome. crates/buzz-test-client/tests/community_ban_routes.rs:443–465 accepts any 2xx, 400, 404, 409, 415, or 422 for every control. A valid /count or moderation read returning an unrelated 404 after admission still passes. Assert each case’s expected status and response shape; use exact validation/missing-resource responses for deliberately invalid fixtures, and accessible resources where resource authorization is part of the claim.

The mesh echo retains its in-flight receive across housekeeping ticks, and the moderation-read change supplies the verified NIP-98 timestamp to owner-ban evaluation. No additional actionable defect was established in this review. The explicitly pending invitation/webhook siblings and final integrated zero-pending requirement remain unchanged.

Coverage limitation: the HTTP matrix’s CI relay has membership enforcement off; closed-relay admission is covered by separate targeted regressions, not by that full route matrix.

Minimalism 9/10; elegance 9/10; correctness 8/10 because of the two coverage defects above.

Reviewed head dda7ef8815cf7675b399bcee4512da3fd6ec7945 against base/merge-base 326e2301cb4b1edcb8a72d01ac4b19a83545365f. Source-only: no tests, guard scripts, mutation checks, builds, applications, or live services were executed. A read-only CI snapshot showed 74 successful and 33 skipped checks; GitHub also reports merge conflicts. These checks do not establish runtime, cross-pod, or deployment readiness. Public PR description and discussion were inspected; no attached images or actionable publication disclosure found. This is a non-blocking COMMENT review; humans decide what must change.

[wpfleger96]

🤖 Thanks for the scanner hardening. The repeated-nest gap from my last review is fixed, and the duplicate .nest and .route_service registrations now fail the guard, which I confirmed by adding each one to a copy of this head. Two blockers remain: the positive controls are unchanged, and the guard misses routers composed with .merge.

Blocking

1. Positive controls still accept unrelated failures (correctness). This is unchanged since my last review. assert_allowed_control in community_ban_routes.rs L446–469 still accepts any 2xx, 400, 404, 409, 415, or 422 for every control and only checks that the body isn't a ban denial. A handler that admits the unbanned member and then returns 404 (the /count mutation from the last review) still passes the matrix. The new profile_search case inherits the same check, and its control body is a deliberately malformed {, so it proves only that the request got past auth.

Fix: give each control case its expected status and response: 200 plus the response shape for valid reads and writes (e.g. /count returns a numeric count), and the exact validation or missing-resource response for fixtures that are deliberately malformed or absent.

2. The guard misses router builders composed with .merge (correctness). The registration check at L392 looks at route, nest, the *_service calls, and fallback, but not merge. builder_prefixes (L317–347) only reads .nest in build_router, and L430 emits each builder's routes once, under that builder's nest prefix or "". So a builder merged into a nested router, or merged in more than one place, exposes routes the inventory never sees. To check, I added .merge(crate::api::git::git_policy_router(state.clone())) inside api::buzz_v1::router in a copy of this head. The guard still exits 0 and reports the same 77 routes, even though /buzz/v1/internal/git/policy now has no row. This is the same class of gap as the repeated .nest and the shapes in the earlier inline thread, through the one composition call the scanner doesn't read. The relay already uses it (build_router merges four routers, git_router merges settings::router).

Fix: track .merge(<builder>(...)) the way .nest is tracked, so a merged builder's routes are emitted under the merging router's effective prefix, and reject a builder that is merged or nested more than once. Alternatively, reject .merge of a builder call outside build_router. Add a fixture for each case.

Scores

• Minimalism 8/10: the PR still carries the mesh-receive, boot, scraper, and push-lease fixture fixes shared with #8237, #8241, and #8242. They look right, but landing them once would keep this diff to the coverage change.
• Elegance 9/10: keying constants by module and failing every unclassified shape through one error path is a clear improvement. The allow-listed fallbacks are pinned to their builder and exact argument, which keeps the exemptions honest.
• Correctness 7/10: the two items above. What holds up: the duplicate-nest and route_service mutations both fail the guard (exit 1), all 13 scanner fixtures pass, and the baseline guard reports 77 routes, 24 HTTP cases, 19 supporting regressions, and 2 pending. The two seed commits (6eb83c0e0, 9a2c8704a) only adapt the test seeds to the new add_relay_member signature and MembershipWrite return type.

Nonblocking

• TESTING.md L106–114 still says the scanner doesn't discover .route_service(...) or .nest_service(...), which contradicts L43–47 now that both are hard errors. It also lists "conditional/opaque router composition" as a gap. Once item 2 is fixed, that paragraph should say which composition the guard handles.
• Still open from the last review: the banned delegated agent has no matching unbanned non-moderator agent control, so on /buzz/v1/*, media, and HEAD cases the denial check can't tell an owner-ban denial from some other agent-specific denial.
• Still open from the last review: TESTING.md L78–81 restates the unmerged invitation contract from #8237 (72-hour default, 30-day maximum, invalid_after), which will drift if #8237 changes.
• The description still doesn't mention the moderation-read fix in bridge.rs (fresh owner proofs now evaluated at the verified NIP-98 time). Luke asked about this too. It's a real security fix and should be named, or split out so it can be reverted or backported on its own.

[wpfleger96]

🤖 Both blockers from my last review are fixed. Every positive control now pins an exact status plus an exact body or shape, and the guard rejects any new or repeated .merge. One gap is left, in the one merge the guard still allows.

Blocking

1. The allowed Git settings merge keeps its root path when its parent is nested (correctness). builder_prefixes (L317–348) only gives a prefix to the builder a build_router .nest names directly. The api::git::transport::git_router → super::settings::router() merge is allowed at L361 no matter what prefix its parent has. Then L451 emits the settings routes with prefixes.get(..., ""). So if build_router mounts api::git::transport::git_router(state.clone()) under .nest("/alternate", ...) instead of merging it at the root, the guard moves the three transport routes under /alternate. The two default-branch routes stay at /git/{owner}/{repo}/default-branch, even though Axum nests all five. Once someone updates the three transport rows, the guard passes with two stale settings rows and without the two real routes. The root-merge-inside-nest check at L419–427 doesn't catch this, because the allowed merge sits inside the transport builder, not inside a build_router nest argument. This is the last case of the nested-merge gap from my last review. It's also the case the description promises to fail ("an existing root merge moved into a nested target").

Fix: either carry the parent's prefix through the allowed settings merge, or reject that merge when git_router has a prefix. Add a scanner fixture that nests git_router and expects the prefixed settings routes or a hard error.

Scores

• Minimalism 9/10: the shared prep fixes are gone. The CI, Justfile and run-tests.sh changes only wire in the guard and the matrix.
• Elegance 9/10: per-case outcome contracts in one place, plus one error path for every unsupported shape, read clearly and fit the existing approach.
• Correctness 8/10: item 1. What holds up: I ran the PR head against a private relay, PostgreSQL, Redis and MinIO. community_ban_routes passes 3/3, regression_relay_admin_ban_gate passes 1/1, the guard reports 77 routes, 24 HTTP cases, 23 supporting regressions and 0 pending, and all 18 scanner fixtures pass. Both false greens from my last review now fail. With .merge(crate::api::git::git_policy_router(state.clone())) added inside api::buzz_v1::router, the guard exits 1 ("production .merge registration is not classified"). With /count returning 404 after admission, replay and membership checks, the matrix fails at community_ban_routes.rs:475 (404 vs 200). The bridge.rs moderation-read change passes signed_created_at to enforce_relay_membership the same way the other NIP-98 bridge routes do, and its new test separates the fresh-owner, stored-owner and clear-owner cases. The ingest.rs, relay_admin.rs and roster-regression changes match the restriction gate already on main, which runs before relay-admin dispatch and refuses timeouts, so they don't change policy.

Nonblocking

• Two lines in the new TESTING.md section aren't wrapped like the rest of it: the one ending in `scripts/check-community-ban-route-inventory.test.py` and the "or a resolved string constant and a direct call to another recognized router function" line.
• The four nonblocking items from my last review are all handled: the scanner limits are documented consistently, ordinary-member routes run a clear delegated-agent control, the invitation section links to the authoritative contract, and the description names the moderation-read fix.