[ngoodger-block]

Why?

Media is readable by anyone in a community who has its URL, even when it was posted in a private channel or DM. Restricting reads to the channels a file is shared in needs a record of where each file is shared — including everything posted before — so enforcement can land without hiding existing media.

What?

Migration 0067 adds uploader proof and a separate file grant for each source event. Each grant records the source message’s event_id and event_created_at, plus its channel or the whole community for a channel-less reference. A leaked URL creates no grant: the author must have uploaded the exact bytes or already have a readable share. Author-only and recipient-gated events share nothing. An agent’s kind:0 picture may use its registered owner’s upload proof; other fields cannot.

Deleted or purged source events cannot authorize new shares. Profile and emoji replacements preserve references they are still entitled to publish before retiring the old head. The backfill skips deleted events and preserves each surviving reference as an independent grant. Download behavior stays unchanged here; #8444 uses these records to enforce reads.

Run buzz-admin backfill-media-shares after every relay pod records shares. It trusts events received before the cutoff stored on each community and checks later references like live writes. The cutoff exists in both desired-state and SQLx-managed databases. Reruns deduplicate rows but re-check later references using current permissions, so an earlier refusal can become a grant after access changes. This migration limitation is accepted for recording and log-only rollout. Before enabling enforcement, explicitly accept this behavior or prevent reevaluation.

Related issue

First half of BUZZ-358.

Testing

Bootstrapped isolated PostgreSQL 17 with pgschema apply and the reconciliation script, then ran buzz-admin backfill-media-shares --batch-size 1 without a SQLx ledger. On the stacked local relay, backed by PostgreSQL, Redis, and an S3 HTTP fixture, all 27 media end-to-end tests passed. They cover private reads, owner-uploaded agent avatars, and author/moderator deletions that revoke only the removed post’s grant. The profile journey also retains a copied avatar across a name update, then revokes access when the picture is removed. The actual Desktop edit/upload/profile workflow smoke test is pending.

Generated with Codex

[github-actions]

🔐 Codex Security Review

Status: review required for the current range.

The current range is 7e6461242aa7d98f132919dd672f7cc5f4abbba6...61ef9189a91d3fc48d4620050a877f65d2529c0f.
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 61ef9189a91d3fc48d4620050a877f65d2529c0f to authorize a new review.
Any previous review applies only to its recorded range.

[wpfleger96]

🤖 thanks for splitting this out from the enforcement PR. two blocking issues, both about how a share gets created, plus a schema question and some smaller items inline.

1. a file with no channel shares yet can be made community-wide by anyone who has its hash, because the "no channel shares" arm of readable_sql! also decides who may create a share.
2. backfill-media-shares replays every stored event without the author check, so it turns references the live path refused into community-wide shares.

we reproduced both in a throwaway Postgres using migration 0067 and the production SQL pulled out of media_reference.rs. a non-uploader's kind:1 note created a public share before the backfill and before a fresh uploader's first post. for a reference the live path had refused (0 rows written), the backfill then wrote the public share (1 row). that was a SQL-level check, not a full relay run. #8444's stricter read check still returned true for that user in all three cases, so it can't undo grants this PR already created. these need fixing here before enforcement in #8444 protects anything.

[wpfleger96]

@buzz-security-review 86525c2

[ngoodger-block]

🤖 Both blocking grant-creation issues are fixed in this PR with regression coverage, and source-event provenance now ensures deleted messages stop granting file access while surviving shares continue working.

[wpfleger96]

🤖 thanks for the quick turnaround. this round closes nearly everything from the first review: hash-only sharing is gone, shares now carry their source event so deletion revokes them, the savepoint ordering is fixed, and the migration-67 fixture is updated. one blocker is left, in the backfill (inline). two nonblocking notes are inline too.

the deferred follow-ups (lease-lost logging, engram 30174, excluded-kind coverage, uploader recovery from audit_log) are fine as follow-ups.

[wpfleger96]

🤖 blocking: this re-checks post-cutoff events against the permissions people have when the backfill runs, not the decision live recording already made. that means a refusal can turn into a grant later:

1. an outsider posts a channel-less kind:1 with a leaked /media/<hash> URL from a private channel. live recording correctly writes nothing.
2. later they're legitimately added to that private channel.
3. someone runs backfill-media-shares (documented as safe to rerun). it replays the old note, the author can read the file now, and it writes a community-wide share.
4. that share is sourced from the outsider's own note, so it survives their removal from the channel and the deletion of the original private post.

nobody published anything new, but the file is now readable community-wide. the author could repost while they're a member, sure, but here the backfill publishes on their behalf from a note we once refused, and the grant outlives the access that justified it. this was reproduced with the production insert/read SQL and migration 0067 in an isolated Postgres: community-wide shares went 0 to 1 with no new event.

could live recording persist an "evaluated" marker per referencing event in the same transaction, including when it records zero shares, and have the backfill skip marked events and mark the ones it evaluates? events old pods stored during the rolling deploy stay unmarked, so they still get their one evaluation. skipping every post-cutoff event would be simpler, but attachments posted through old pods during the deploy window would then never get a share and would 404 once #8444 enforces. a regression test for refused → gains access → backfill → still no community share would pin it; the current tests keep permissions fixed so they miss this.

[ngoodger-block]

🤖 We’re accepting this as a nonblocking limitation for recording and log-only rollout and have documented that reruns can grant previously refused references after access changes, with enforcement still requiring an explicit decision.

[wpfleger96]

🤖 this guarantee only holds while permissions stay the same (see the comment on the recorded_live loop). worth updating along with that fix.

[ngoodger-block]

🤖 Updated the backfill API docs and CLI help to state that reruns recheck post-cutoff references using current permissions and can grant previously refused references after access changes.

[wpfleger96]

🤖 nonblocking: nothing in this PR fails if this call is deleted, since reads aren't enforced until #8444 and the media_reference tests seed media_uploaders directly. a relay test that uploads through the handler and asserts the uploader row (or that a later post records its share) would cover it here.

[wpfleger96]

🤖 nonblocking: rows sourced from replaced or hard-deleted events stay behind forever, one per profile or emoji-set update. they grant nothing since the read joins the live event, but nothing ever cleans them up. a periodic sweep or a delete in the purge/replace path would keep the table from growing with dead rows.

[wesbillman]

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

Two new P2 findings inline: retaining a legacy avatar during a profile update before backfill loses its grant, and deleting an edited message leaves the edit’s independent grant active. These affect the grant population created here; download enforcement remains in #8444.

The existing permission-drift finding in #8451 (comment) still applies; this review does not duplicate it. The rolling-deploy gap also remains an enforcement prerequisite: schema installation precedes the last legacy writer, so old-pod uploads/references after the cutoff may have neither proof nor shares. Preserve the accepted historical-uploader recovery follow-up and resolve that gap before relying on complete coverage. Merely moving the cutoff later can trust references the new recorder deliberately refused.

Public material: please remove the internal tracker URL from the public description’s Related issue section, replacing it with a public description of the work. This is a P3 publication cleanup, not a credential finding. No attached images were present in the description/discussion inspected; the linked issue’s contents were unavailable to this review.

Minimalism 9/10; elegance 9/10; correctness 7/10 because the two inline lifecycle cases need explicit authorization provenance and regression coverage. Existing deferred logging, kind-exclusion and cleanup suggestions remain follow-ups.

Reviewed head 7f7f21c7ea0a5c6ddd39d807dcadf13233ea6723 against base/merge-base 6e6173acd58029bac800583c4cc7482dafed693a. Source only: no PR code, tests, SQL, builds, apps, live workflows or benchmarks executed. One head-bound CI snapshot showed 55 successful and 30 skipped checks; this does not establish runtime correctness. Database races, rollout recovery, performance, and native upload/error/retry focus behavior remain unverified; the PR itself records its Desktop workflow smoke test as pending. Non-blocking COMMENT review, not approval or merge authorization.

[wesbillman]

🤖 [P2] Bootstrap retained legacy references before retiring their source

A pre-upgrade kind:0 avatar can have neither an uploader row nor a media share when a name-only profile update reaches this call before backfill. The authorization check therefore records no share for the unchanged picture; the following UPDATE retires the old head. Backfill excludes that deleted pre-cutoff row and subjects the new post-cutoff row to the same empty-proof check, so rerunning it cannot restore the grant. The parameterized replacement path has the same problem for legacy emoji sets. Once #8444 enforces reads, an ordinary name update can make the retained avatar unreadable.

While the old head is still live, materialize its eligible pre-cutoff historical references before authorizing the replacement, or gate replacement until those grants exist. Keep this inside the replacement transaction/savepoint. Add a regression starting with an unbackfilled pre-cutoff profile/emoji set, replace it while retaining the media, then backfill and verify the surviving head still authorizes the blob.

[wesbillman]

🤖 [P2] Tie edit-derived grants to the original message’s lifetime

Kind:40003 edits pass shared_blob_hashes and are stored with their own event ID, so an edit retaining or adding an attachment creates a separate share. Deleting the displayed message only tombstones its original ID (side_effects.rs:2050 and event.rs:1494–1503); the edit row stays undeleted. This join therefore continues authorizing the edit’s share after the message disappears. Desktop explicitly suppresses edits whose target was deleted (formatTimelineMessages.ts:275–277), so the visible deletion and the new grant model disagree. The same remaining grant can authorize a later share even before download enforcement lands.

Record the edit target as authorization provenance and require that target to remain live, or revoke the associated edit grants transactionally on original-message deletion. Apply the same lifetime rule during backfill so it cannot recreate them. Cover an attachment-bearing edit followed by both author and moderator deletion of the original message.

[ngoodger-block]

🤖 @wpfleger96 Please re-review the corrected backfill guarantees and the explicitly accepted limitation for recording/log-only rollout at 61ef9189a91d3fc48d4620050a877f65d2529c0f; enforcement still requires an explicit decision on replaying earlier refusals. The stack is refreshed from main, and focused media, database, membership-write, and event-insert checks passed. The fresh security workflow stopped before analysis because actions/checkout refuses fork code in a pull_request_target run; a maintainer needs to address the workflow before the security review can complete.

[wesbillman]

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

One new P2 integration finding inline: the main merge brings in the database architecture contract from #8273, while this PR still introduces the fence triggers and foreign keys that contract prohibits. This is a source-demonstrated repository-contract conflict, not a newly demonstrated runtime exploit.

The six previous feature patches remain unchanged. The NIP-43 snapshot merge resolution uses the updated event hook consistently; no new runtime defect found in that integration. The backfill documentation now describes current-permission reevaluation. Optional wording: use “any backfill run” rather than “reruns” in the admin help and Db docs, since the first backfill can also reconsider a reference refused by live recording.

Prior review findings remain outstanding; this documentation/merge follow-up does not resolve the legacy-avatar or edit-lifetime issues or the public-description cleanup. They are not duplicated here. The stated acceptance of permission drift for recording/log-only rollout does not establish enforcement readiness.

Reviewed head 61ef918 against base 7e64612, focusing on changes since 7f7f21c. Source only: no tests, builds, SQL, app or CLI execution. CI snapshot: 44 successful, 29 skipped, 2 in progress, and Run Codex Security Review failed; failure cause unverified. Runtime migration parity, rollout behavior and Desktop edit/upload/profile/error-retry flows remain unverified. Public description inspected, no attached images; linked issue content not inspected. Non-blocking COMMENT review; no approval or merge authorization.

[wesbillman]

🤖 [P2] Reconcile both schema paths with the newly merged database contract

The main merge now includes docs/database.md Rules 1–2: new tables must not call attach_community_write_fence or add foreign keys, and tenant-owned tables must wait for the prerequisite removal of blanket fence attachment from desired-state bootstrap. These calls still add two triggers; lines 11 and 22 also introduce REFERENCES communities, repeated in schema/tables/public/media_{uploaders,shares}.sql. Explicit schema/triggers files and schema/community_write_fence.sql also install the fences, so removing just these migration calls leaves the bootstrap paths inconsistent.

This is new integration evidence from #8273: both paths add objects prohibited by the current schema contract and unsupported by its stated out-of-band apply model. Land the prerequisite bootstrap change, then remove the new fence/FK declarations consistently while retaining application transaction admission and deletion coverage. An intentional exception needs an explicit repository-owner decision. This finding does not claim a new runtime authorization failure.