Repository navigation
BUZZ-272: Check webhook owner bans - #8242
bradseiler wants to merge 9 commits into
Conversation
Signed-off-by: seiler <seiler@squareup.com> Co-authored-by: Codex <noreply@openai.com> (cherry picked from commit 6ecd74515c01c407f817f7c25597337fcffa688a) 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> (cherry picked from commit f1674a2) Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Codex <codex@openai.com> Signed-off-by: Brad Seiler <seiler@squareup.com> (cherry picked from commit 3d02a44) Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Codex <codex@openai.com> Signed-off-by: Brad Seiler <seiler@squareup.com> (cherry picked from commit d43b957) 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> (cherry picked from commit 893f81f) Signed-off-by: Brad Seiler <seiler@squareup.com>
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
| // The workflow_runs insert is the admission boundary. This is the last | ||
| // authorization gate before it. The restriction read is authoritative and | ||
| // tenant-scoped; lookup failures deny admission just like an active ban. It | ||
| // is intentionally a point-in-time check rather than a transaction with ban | ||
| // writes, so a ban racing after this read may follow the run insertion. Runs | ||
| // already inserted are unaffected. | ||
| let restrictions = state | ||
| .db | ||
| .moderation_restriction_state(community_id, &workflow.owner_pubkey) | ||
| .await | ||
| .map_err(|_| not_found("workflow not found"))?; | ||
| if restrictions.banned { | ||
| return Err(not_found("workflow not found")); | ||
| } |
There was a problem hiding this comment.
This closes the webhook door, but a banned owner's event-triggered and cron workflows still create runs. We reproduced both locally at this head: a bystander message and the real scheduler each produced a completed run with message output while the owner was banned. Bans don't remove channel membership, and the action sink posts relay-signed, so the ingest ban gate never applies.
To be clear, this isn't a regression from this PR. It's the rest of the stated goal. All four run-creation doors (webhook here, command_executor.rs:868, and buzz-workflow/src/lib.rs:394 and :607) already call WorkflowEngine::check_owner_authority. Its doc comment says every door must recheck current authority. Moving moderation_restriction_state(community, owner) into that function, failing closed with Unauthorized, covers all four and lets this handler-local branch go away. It would need event and cron test cases asserting zero new workflow_runs for a banned owner.
If you'd rather keep this PR scoped to BUZZ-272, that's reasonable given the body. In that case I'd track the other doors as a follow-up.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Thanks for this. No blocking findings at d1c9dd34.
The new check sits right before create_workflow_run and uses the same host-bound community_id the run is inserted into, so changing the host picks another tenant's workflow rather than skipping the banned tenant's check. moderation_restriction_state reads from the writer, honors ban expiry, and covers an agent owner's human owner. A ban and a failed lookup both return the same 404 / {"error":"workflow not found"} as the earlier lifecycle and role denials, with no run row. The other HTTP path that creates runs, a signed workflow-trigger event, is owner-only, and ingest already rejects a banned signer, so it doesn't bypass the new check.
What we checked:
- The new PostgreSQL test calls the production handler. Deleting the ban check fails it at line 281 (
202instead of404). Making a failed lookup admit fails it at line 338. Because bans leave channel membership in place, the existing role check can't mask either case. The lookup-failure fixture breaks onlycommunity_bans, so the error branch is really exercised. - A real HTTP run against a locally built relay: ban gives
404with no new run through bothX-Webhook-Secretand?secret=. Unban admits again. The same owner in another community is still admitted. An expired ban and a timeout-only owner are admitted. An unknown host and an injected restriction-read failure give404. - CI runs the new test by name in the PostgreSQL job (938/938) and the mesh receive regression in Unit Tests. The mesh, scraper, and push-lease fixture files are byte-identical to what #8239 carries.
- The Desktop Smoke failure (
project-conversation-load-failure.spec.ts) passed on rerun. The desktop tree is unchanged from the base, and all checks are green now.
Minimalism 9, elegance 9, correctness 9: one existing authoritative query at the insertion boundary, no new abstraction, and both failure branches are proven by mutation.
Optional
- Cron and event-triggered workflows of a banned owner still create runs. That's out of scope per the description, and Luke covered it inline. Moving the restriction read into
WorkflowEngine::check_owner_authorityas a follow-up would close all four doors and let this handler-local branch go away. - The test only sends the secret in the header. A query-only banned-owner request, plus a conflicting header/query request showing the header wins, would pin the converged credential path against a later split.
- The description is stale. It still lists the three failed Desktop checks, says "Draft only" on a ready PR, and has no
buzz-review-completed.
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/error outcomes/public material) review lanes.
No actionable findings in the reviewed diff. The webhook check uses the host-bound community and an uncached writer-side effective restriction lookup immediately before run insertion. Active bans and lookup failures deny without creating a run; the existing secret and channel-role gates remain intact. The source tests bind the production handler and assert run counts across denial, other-tenant admission and unban. The shared mesh change retains the same receive future across housekeeping ticks and ends its borrow before shutdown handling; the regression is selected by both unit-test entry points.
The documented point-in-time ban race, existing runs, timeout behavior and cron/event-triggered workflows remain outside this change’s scope. The latter already has discussion at #8242 (comment); no duplicate finding is added here. The public description and discussion have no attached images and yielded no actionable disclosure finding.
Reviewed head: d1c9dd341c3f8468ca976a99393ad1a7a0abefc6.
Base/merge-base: 326e2301cb4b1edcb8a72d01ac4b19a83545365f.
Validation: source inspection only; no builds, tests, SQL, app/relay execution or live HTTP/mesh rehearsal. The GitHub check snapshot showed 68 successful and 32 skipped checks, superseding the description’s older failed-check snapshot. GitHub currently reports a merge conflict; this review does not validate conflict resolution, sibling integration, runtime performance or deployment behavior. Minimalism, elegance and source correctness: 9/10 each within this scope. This is a COMMENT review, not approval or merge authorization.
Main added authenticate_member_post beside this branch's workflow_webhook_postgres_tests module declaration; keep both. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Luke Tornquist <tornquist@squareup.com>
The webhook handler checked the owner's ban itself, so cron and event-triggered workflows of a banned owner still created runs. Bans leave channel membership in place, so the shared SEC-006 owner authority gate passed them. Move the restriction read into WorkflowEngine::check_owner_authority, which the webhook, event, cron and signed-trigger doors all call before creating a run. A lookup failure still denies. The handler-local branch goes away; the webhook keeps its generic 404. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Luke Tornquist <tornquist@squareup.com>
Main now answers a missing or invalid webhook secret with the same non-disclosing 404 as an unknown workflow. The merged Postgres regression still expected 401, so it stopped at its first assertion before exercising the ban, tenant-scope, unban and lookup-failure arms. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Luke Tornquist <tornquist@squareup.com>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 No blocking findings at 81c367f4. Moving the ban check into WorkflowEngine::check_owner_authority closes the cron, event and signed-trigger gaps from the last review, and the webhook handler no longer needs its own branch.
All four production paths that create a run call the gate first: channel events (lib.rs:413), cron and interval schedules (lib.rs:626, before the schedule claim), webhooks (bridge.rs:2758) and signed triggers (command_executor.rs:886). The insert at workflow_sink.rs:883 that looks ungated is a helper inside #[cfg(test)] mod postgres_tests. Step execution and approval resume don't call the gate, so a ban stops new runs but doesn't kill one already in flight, which matches the description. The restriction read goes to the writer, ignores expired bans and covers an agent owner's human owner. The 44f46993 merge keeps both the test module declaration and main's authenticate_member_post.
What we checked:
- Removing the
restrictions.bannedrejection fails both regressions: the event test creates one run instead of zero, and the webhook test gets202instead of404. - Making a failed restriction lookup admit fails only the webhook regression (
workflow_webhook_postgres_tests.rs:339). The event test still passes, so the webhook test is the only one covering fail-closed. - Cron against a locally built relay: on the first tick, a banned owner's workflow and an agent workflow whose human owner was banned each had zero runs and zero schedule claims, while an unbanned control ran. After unbanning, both ran on the next tick and completed. A denial doesn't consume the scheduled claim, and one banned owner doesn't stop other workflows on the same tick. Bans were set with SQL here, not through the moderation commands.
- CI: the PostgreSQL job passed 1169/1169 and lists both regressions by name. Unit and Relay E2E passed. Desktop Tauri (mesh) failed because
apt-get installof the Tauri Linux packages timed out three times on the runner. I reran it. It isn't related to this PR.
Minimalism 9, elegance 9, correctness 9: one authoritative read in the gate every path already shares, no new abstraction, and mutation testing and a live run both catch the ban branch.
Optional
- Each denied workflow logs one WARN per matching event, and a scheduled one logs one WARN per tick per pod for as long as it stays enabled, with no rate limit (
lib.rs:416-419,:629-632). Once bans are routine that's steady noise. Rate-limiting expected ban denials per workflow while keeping lookup failures loud would fix it. - Every matching workflow adds one uncached writer read, done in sequence, even when they share an owner. I'd keep the authoritative read over a cache. Combining reads per owner is only worth it if it shows up under load.
- No dedicated test for cron, signed triggers or agent-owned workflows, and nothing in the event test covers a failed lookup. The live cron run above passed, so this doesn't block. A cron or agent-owner case in the
buzz-workflowPostgres lane would pin it. - The title still says "webhook" even though every trigger is covered now. Something like "Deny workflow runs for banned owners" would match.
- The description has no
buzz-review-completedyet.
Summary
A workflow whose saved owner is banned from the community can no longer create runs through any trigger: webhook, channel-event, cron, or signed command trigger. Every one of those doors already calls
WorkflowEngine::check_owner_authority, which re-checks the owner's channel role. That gate now also reads the owner's authoritative restriction state. A ban denies the run, and a failed lookup fails closed. Unbanning restores admission.On the webhook door the denial is the same generic 404 as before, and no run is queued. The handler-local ban lookup in
api/bridge.rsis removed because the shared gate covers it.This is a point-in-time check: a ban committed after the lookup can race with run creation. It does not cancel existing runs. Community timeouts do not block runs in this change.
This PR also carries the shared mesh receive fix needed for independent validation (see Shared carry).
Related
BUZZ-272.
Testing
on_event_denies_run_while_owner_banned, in thebuzz-workflowPostgres lane: a channel-event trigger for a banned owner creates no run; after unban it creates one.81c367f48updates its wrong-secret arm to expect main's generic 404, so the later arms run:Results at
81c367f48:Shared carry
#8237, #8239, #8241 and #8242 each carry the same mesh-receive fix, boot and scraper regressions, and push-lease fixture correction. Whichever lands first owns them, and the others drop them on their next main merge.
What got simpler
The ban check now lives in the one gate every run-creation door already shares, so there is one fewer handler-local branch. Cron, event and signed triggers get the same protection without per-door code.
🤖 Generated with Claude Code