Skip to content

BUZZ-269: Cover community-ban entry points - #8239

Open
bradseiler wants to merge 13 commits into
mainfrom
codex/buzz-269-draft-prep
Open

bradseiler wants to merge 13 commits into
mainfrom
codex/buzz-269-draft-prep

Conversation

@bradseiler

@bradseiler bradseiler commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Classify all 74 current relay HTTP routes and bind community-ban enforcement to a signed request matrix with allowed controls. Cover the new sidebar/read-state routes and classify operator routes explicitly. Fail the inventory guard when a new production route has no policy row.

Fix a mesh echo timeout by retaining the pending validated receive across housekeeping ticks. Add a deterministic production-consumer regression, cancellation-safety documentation, and diagnostics that retain the last scrape or error until the deadline. Enable the accessory API when CI runs its route matrix.

Invitation claim and webhook-owner coverage remain explicitly pending BUZZ-268 and BUZZ-272 in this standalone draft. Final integration must replace those rows with real tests, include BUZZ-271 revalidation regressions, and require zero pending coverage.

Related issue

BUZZ-269. Companion invitation implementation: #8237. Staging verification runbook: #8235.

Testing

Validated 893f81ffed0fa50048f786c7f698e9b8bf3a2ac6: just ci and just test passed; full DB/relay packages and doctests passed; the complete discovered PostgreSQL suite passed 940/940. Ordinary lanes retain configured infrastructure skips; the separate PostgreSQL run uses isolated services. Existing ACP pipe-test leak warnings were reported with passing exit status.

The isolated signed route suite passes all three tests: HTTP matrix, root WebSocket refusal, and host-scoped bans. The inventory reports 23 HTTP behavior cases and 19 supporting regressions. Adding an unclassified route to the production router source makes the guard fail. Removing the mesh consumer fix makes its regression fail.

Independent agent review found no remaining blockers. Draft only: sibling integration and human testing remain outstanding.

Companion drafts

Live-session revalidation: BUZZ-271. Webhook owner policy: BUZZ-272.

Generated with Codex.

🤖 Documentation review correction

At 94c9ce5f478313030d747b0a0a6383152249bf17, TESTING.md labels the BUZZ-268 invitation lifecycle/cutoff and BUZZ-272 webhook-owner contract as future sibling behavior. The two pending inventory rows remain explicit. Exact-head content/diff checks, the route guard (74 routes, 23 HTTP cases, 19 regressions, 2 pending), and normal commit/push hooks passed. Hook application-test lanes skipped for this documentation-only delta. The runtime checks above remain evidence for 893f81ffed0fa50048f786c7f698e9b8bf3a2ac6, not new-head reruns. No staging, fleet, human readiness, or security closure is claimed.

🤖 Shared preparation work and integration

🤖 The preparation pass carried equivalent mesh receive fixes, boot/scraper regressions, CI selection, and push-lease fixture corrections into four drafts: #8237, #8239, #8241, and #8242. The shared mesh, scraper, and fixture blobs match at the recorded published heads. #8237 carries its own preparation commits (0486f66a, ba60dcc7, 72875251, and fdd2db5c). It is based on 19aa619fdd70e916269b343232276d9ee68276df and does not contain the pinned-main merge 326e2301cb4b1edcb8a72d01ac4b19a83545365f; that merge is present in 269/271/272. These are four carriers of shared validation prerequisites, not four distinct fixes. The preparation ledger assigns no exclusive merge owner. Coordinate the landing order across all four drafts, preserving the effective shared fixes if any sibling lands first.

🤖 Reviewer V2 found an integration conflict between the original 269 and 271 heads in crates/buzz-relay/src/audio/handler.rs, around the test config.database_url. Prefer 271’s credential-less unreachable URL; retain a sadscan suppression only if the resolved line still requires one. This is an integration note, not a sibling merge performed here. Final integrated coverage must include invitation, webhook-owner, and live-revalidation production regressions with zero pending rows.

🤖 Follow-up documentation correction

At dda7ef8815cf7675b399bcee4512da3fd6ec7945, invite claim is described as classified restriction-enforced in the inventory, not currently enforced by this standalone head. Enforcement and coverage remain pending the invitation sibling. Exact-head documentation content/link/diff checks, the route guard (74 routes, 23 HTTP cases, 19 regressions, 2 pending), and normal commit/push hooks passed; application-test lanes skipped for the documentation-only delta. No new runtime-suite run is claimed.

bradseiler and others added 11 commits October 9, 2026 19:11
Signed-off-by: seiler <seiler@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
(cherry picked from commit dcb83eaec669ed044ab2f16c8b7612bd54ed1d9c)
Signed-off-by: Brad Seiler <seiler@squareup.com>
Signed-off-by: seiler <seiler@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
(cherry picked from commit 5bbf2b64a82115c40fdadeb981dbfdf7122241f2)
Signed-off-by: Brad Seiler <seiler@squareup.com>
Signed-off-by: seiler <seiler@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
(cherry picked from commit 2374e1c05352db37eafa42d502a148b57dc21080)
Signed-off-by: Brad Seiler <seiler@squareup.com>
Signed-off-by: seiler <seiler@squareup.com>
Co-authored-by: Codex <noreply@openai.com>
(cherry picked from commit 31968772ebe1b30c2862966e46b75a9ad4b967b6)
Signed-off-by: Brad Seiler <seiler@squareup.com>
Keep invitation and webhook-owner gaps explicit until their implementations are integrated.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Brad Seiler <seiler@squareup.com>
Complete the preserved 66663e19 remediation with a deterministic production-consumer regression, cancellation-safety documentation, and deadline diagnostics. Original recovered refs and their attribution remain unchanged.

Co-authored-by: Codex <codex@openai.com>
Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Codex <codex@openai.com>
Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Codex <codex@openai.com>
Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Codex <codex@openai.com>
Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Codex <codex@openai.com>
Signed-off-by: Brad Seiler <seiler@squareup.com>
Avoid comparing host and PostgreSQL clocks in the async enqueue guard regression. Assert strict lease ordering before releasing activation.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Brad Seiler <seiler@squareup.com>
Local Builder added 2 commits October 11, 2026 22:33
Signed-off-by: Local Builder <1c476cb1127ea270cb900bdc3891060140bdd1173f91498030ee9b4db01c2f20@buzz.block.builderlab.xyz>
Signed-off-by: Local Builder <1c476cb1127ea270cb900bdc3891060140bdd1173f91498030ee9b4db01c2f20@buzz.block.builderlab.xyz>
@bradseiler
bradseiler marked this pull request as ready for review October 12, 2026 03:52
@bradseiler
bradseiler requested a review from a team as a code owner October 12, 2026 03:52
@bradseiler
bradseiler deployed to codex-review October 12, 2026 03:52 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 12, 2026
@github-actions

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: 30873ba46b5e08602ae61a2a0592ecedd37f7289...dda7ef8815cf7675b399bcee4512da3fd6ec7945
  • Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability findings were identified in the authorized PR range.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

  • Review used read-only source and git history inspection. Scripts, builds, tests, and changed binaries were not executed.

Generated by Codex Security Review |
Requested by: @bradseiler |
Workflow run

router_ranges = [
(builder[6], builder[7]) for builder in builders if builder[2] == path
]
for match in re.finditer(r"\.\s*route\s*\(", mask):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard passes on some route shapes it doesn't classify. Tested by mutating an archive of this head:

  • Adding .route_service("/probe", …) or .nest_service("/probe", …) to build_router: the script prints passed: 74 registered routes classified, exit 0. Only .route( is matched here.
  • Adding .nest("/zz", crate::api::buzz_v1::router(…)) inside api::admin::router: also exit 0. builder_prefixes only reads .nest( in router::build_router, and :299/:302 silently skip unresolvable or ambiguous targets.

Since the point is that an unclassified route fails, could these be hard errors? That means route_service / nest_service / fallback_service unless allow-listed (the SPA fallback), .nest( outside build_router, and a raise instead of continue at :299 and when len(candidates) != 1. A small fixture self-test for each shape would keep it honest. scripts/ci-selection.test.mjs is the precedent.

@TheSentinel454

Copy link
Copy Markdown
Contributor

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 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

This branch was successfully deployed

1 active deployment
codex-review — dda7ef88 Deployed Oct 12, 2026 by bradseiler via Run Codex Security Review #7707
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants