Repository navigation
BUZZ-269: Cover community-ban entry points - #8239
bradseiler wants to merge 13 commits into
Conversation
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>
Signed-off-by: Local Builder <1c476cb1127ea270cb900bdc3891060140bdd1173f91498030ee9b4db01c2f20@buzz.block.builderlab.xyz>
Signed-off-by: Local Builder <1c476cb1127ea270cb900bdc3891060140bdd1173f91498030ee9b4db01c2f20@buzz.block.builderlab.xyz>
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
| router_ranges = [ | ||
| (builder[6], builder[7]) for builder in builders if builder[2] == path | ||
| ] | ||
| for match in re.finditer(r"\.\s*route\s*\(", mask): |
There was a problem hiding this comment.
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", …)tobuild_router: the script printspassed: 74 registered routes classified, exit 0. Only.route(is matched here. - Adding
.nest("/zz", crate::api::buzz_v1::router(…))insideapi::admin::router: also exit 0.builder_prefixesonly reads.nest(inrouter::build_router, and:299/:302silently 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.
|
|
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 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.rsmoderation-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. Themesh_boot.rsecho loop keeps one in-flight receive across drain ticks and drops it only on shutdown. Theingest.rs/relay_admin.rsedits 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 forcode == "forbidden", on media only forrelay 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 inrouter.rs.TESTING.mdlistsroute_service/nest_serviceas known gaps but not this one. TESTING.mdrestates the unmerged invitation contract (72h / 30-day lifetimes, v1invalid_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
left a comment
There was a problem hiding this comment.
🤖 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]] = prefixoverwrites 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–465accepts any 2xx, 400, 404, 409, 415, or 422 for every control. A valid/countor 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.
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 ciandjust testpassed; 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.mdlabels 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 for893f81ffed0fa50048f786c7f698e9b8bf3a2ac6, 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, andfdd2db5c). It is based on19aa619fdd70e916269b343232276d9ee68276dfand does not contain the pinned-main merge326e2301cb4b1edcb8a72d01ac4b19a83545365f; 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 testconfig.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.