Skip to content

fix(relay): deny channel writes when the channel lookup fails - #8007

Merged
wpfleger96 merged 5 commits into
mainfrom
duncan/archive-fail-closed
Oct 1, 2026
Merged

wpfleger96 merged 5 commits into
mainfrom
duncan/archive-fail-closed

Conversation

@wpfleger96

@wpfleger96 wpfleger96 commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

The ingest channel lookup and check_channel_write (crates/buzz-relay/src/handlers/ingest.rs) both turned every lookup error into "no row" with .ok(). The archive check only runs when a row exists, and a membership check could still allow the write. So a database error let posts into archived channels.

A new helper, load_channel_for_write, treats only DbError::ChannelNotFound as "no row". Every other lookup error denies the write as an internal error (IngestError::Internal), so no caller reports it as a client rejection or shows the database error text to the client. check_channel_write returns IngestError, which means the artifact-move source check maps a lookup failure to an internal error, while token, membership and archive denials stay Rejected. Only these two archive-gate lookups use it. The other direct channel reads on the write path (huddle validation, the membership and edit open-visibility fallbacks, and command/admin validators) already deny on error and are unchanged.

Behavior:

  • A channel that doesn't exist yet is still "no row", so channel create is unchanged.
  • Unarchive keeps its exemption from the archive check, but a database error during unarchive is now denied.
  • No other exceptions were added.

Tests:

  • check_channel_write_denies_when_channel_lookup_fails uses an unreachable database and a cached channel membership, the case where the old code allowed the write.
  • cluster_global_ingest_denies_post_when_channel_lookup_fails sends a kind-9 post through ingest_event_inner, from a cached member, to an archived channel. Its database role can do everything ingest needs except read channels. The post is denied with the channel-lookup error and nothing is stored. A control run with a working lookup is denied as archived, which proves the post reaches the archive gate.
  • cluster_global_artifact_move_source_lookup_failure_is_internal moves an artifact when only the source-channel lookup fails, and requires an internal error, not a rejection.
  • All three are marked #[ignore = "requires Postgres"] and run in the PostgreSQL CI job.
  • Each test fails when only its own caller goes back to .ok(), and passes with the fix.

Hook note: this branch was pushed with LEFTHOOK=0, so the pre-push hook did not run on it. Pre-push runs on this machine were failing under heavy load, in crates this diff doesn't touch: the buzz-acp timing tests idle_resets_on_stdout_activity and keepalive_resets_idle_past_deadline, rust-tests and desktop-tauri-test (buzz-desktop). CI is the gate for the full suite.

🤖 Implemented by Duncan (agent).

@wpfleger96
wpfleger96 requested a review from a team as a code owner September 30, 2026 22:13
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026
@github-actions

github-actions Bot commented Sep 30, 2026 •

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: 16839a07738cf6189ee2f21c02d923b0f9dbe80d...371699947c5495ded189095d5cafbf94b82aca8a
  • Model: gpt-5.6-sol

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


Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability regressions found. Database lookup failures now fail closed, preserve tenant scoping, and are sanitized at both HTTP and WebSocket boundaries.

Findings

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

Notes

  • Review was limited to read-only inspection; tests and repository code were not executed as instructed.

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

@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026
@github-actions github-actions Bot added codex-security-review-current The posted Codex security review matches its recorded range. and removed codex-security-review-current The posted Codex security review matches its recorded range. labels Sep 30, 2026
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 30, 2026
@wpfleger96
wpfleger96 force-pushed the duncan/archive-fail-closed branch from 632e991 to a25b0c0 Compare October 1, 2026 17:27
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 1, 2026
@wpfleger96
wpfleger96 deployed to codex-review October 1, 2026 17:27 — 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 1, 2026
@wpfleger96
wpfleger96 force-pushed the duncan/archive-fail-closed branch from a25b0c0 to 409ca53 Compare October 1, 2026 19:05
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 1, 2026
@wpfleger96
wpfleger96 deployed to codex-review October 1, 2026 19:05 — 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 1, 2026

@bradseiler bradseiler left a comment

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.

Approved at Brad Seiler’s request based on the delegated code review of 409ca53, which reported no actionable findings in the fail-closed channel lookup handling and preservation of missing-channel semantics. Validation limits: repository-context static review and diff checks only; no local tests were run, and PostgreSQL regression CI was still pending at review time. This approval is not a clean full-suite or live-local sign-off; required CI checks still need to pass.

@wpfleger96
wpfleger96 enabled auto-merge (squash) October 1, 2026 19:32
Duncan and others added 5 commits October 1, 2026 15:43
Both the ingest channel-row fetch and check_channel_write turned any lookup error into 'no row' with .ok(), which skipped the archive check while membership could still authorize the write. A shared helper now keeps ChannelNotFound as no row and returns every other DB error, so the write is denied.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Both tests lacked #[ignore], so neither CI lane selected them. The ingest
regression now runs setup fallibly and drops its server-wide role before
asserting, so a setup failure cannot leak the role.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
check_channel_write returned a plain String, so the artifact-move path
turned a source-channel DB error into a client rejection (HTTP 400) that
carried the raw database error text. The lookup failure now stays typed as
IngestError::Internal; authorization and archive denials stay Rejected.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Moves the restricted-role setup into shared helpers used by both
lookup-failure regressions.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 force-pushed the duncan/archive-fail-closed branch from 409ca53 to 3716999 Compare October 1, 2026 19:43
@wpfleger96
wpfleger96 deployed to codex-review October 1, 2026 19:44 — with GitHub Actions Active
@github-actions github-actions Bot added codex-security-review-current The posted Codex security review matches its recorded range. and removed codex-security-review-current The posted Codex security review matches its recorded range. labels Oct 1, 2026
@wpfleger96
wpfleger96 merged commit 15772c5 into main Oct 1, 2026
81 checks passed
@wpfleger96
wpfleger96 deleted the duncan/archive-fail-closed branch October 1, 2026 20:27
wpfleger96 added a commit that referenced this pull request Oct 1, 2026
* commit '9b083957f^':
  Include thread roots in agent activity events (#8029)
  fix(relay): gate owner-only kinds in shared fan-out access filter (#8006)
  feat(acp): add BUZZ_GIT_IDENTITY switch for agent commit identity (#8024)
  fix(relay): deny channel writes when the channel lookup fails (#8007)

Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
tlongwell-block pushed a commit that referenced this pull request Oct 2, 2026
Main added nine commits since the previous merge. The ones that matter
here rework NIP-FI admission under /buzz/v1: shared assertion evaluation
across adapters (#7990), binding assertions to the request Host's
community (#8028), and the community ban gaps (#8005, #8006, #8007,
#8036). Main changes 55 files and adds no migration.

Git merged the three files both sides change without conflict:
buzz-relay's api/bridge.rs, api/mod.rs and router.rs. No hand edit was
needed. This branch's diff against main is line for line the same before
and after the merge: 31 files, +5,726 / -81.

/buzz/v1 still authenticates through admit_nip_fi_http_on_state. Its
signature is unchanged and it now resolves the Host's community itself,
so the accessory routes take the new binding from the same shared
admission as the bridge routes. The three /buzz/v1 NIP-FI wire-contract
tests pass on the merged tree.

Co-authored-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Signed-off-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>

This branch was successfully deployed

1 active deployment
codex-review — 37169994 Deployed Oct 1, 2026 by wpfleger96 via Run Codex Security Review #6442
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.

2 participants