[ngoodger-block]

Stacked on #8535 (explicit schema fence bootstrap). This prerequisite must land before the media tables.

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 0068 starts with a five-second local lock timeout, so blocked DDL fails for retry. It 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 shares. Edit grants record the original message ID and timestamp; both the edit and its target must remain live. Profile and emoji replacements recover eligible pre-cutoff references before retiring the old head, in the same transaction. The backfill applies the same lifetime rules. Both schema paths use application admission and explicit deletion coverage, without new foreign keys or fence triggers. Download behavior stays unchanged here; #8444 uses these records to enforce reads.

Before recording shares, backfill restores uploader proof from retained server-authored media_uploaded audit records in bounded, tenant-admitted batches. Only attributed 32-byte uploader keys and valid SHA-256 object IDs establish ownership. Missing or malformed evidence requires uploading the exact bytes again; it creates no ownership. The CLI rejects non-positive batch sizes before connecting. Agent engrams (kind 30174) are excluded from grants, alongside other private event kinds.

The elected usage-poller leader reclaims inactive grants in bounded pages. It removes obsolete public profile/emoji grants and compacts inactive channel history to at most one row per community and file. That last row preserves the uploader-preview restriction after message deletion; it grants no read access. Cleanup uses community admission and skips quiescing/deleting communities.

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. Every run, including the first, rechecks later references using current permissions, so an earlier refusal can become a lasting grant. Before the first backfill, explicitly accept the resulting grants or defer backfill until refusal-preserving behavior exists. Changing read modes does not revoke existing grants. Every invocation restarts the scan and deduplicates rows; it does not preserve earlier authorization decisions.

Related issue

Recording half of the file-access stack; #8444 adds read enforcement.

Testing

Bootstrapped isolated PostgreSQL 17 with pgschema apply and the reconciliation script, then exercised backfill against that database without a SQLx ledger. On the stacked local relay, backed by PostgreSQL, Redis, and an S3 HTTP fixture, all 29 media end-to-end tests passed. They cover private reads, owner-uploaded agent avatars, and author/moderator deletions that revoke retained and edit-added attachments while independent shares survive. 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 remains pending: an isolated native demo build started, but UI automation returned cgWindowNotFound before the edit flow could be exercised.

Held a conflicting communities lock in isolated PostgreSQL and ran the exact migration SQL with session timeouts disabled: it failed with SQLSTATE 55P03 after 5.04 seconds, left no partial schema, then committed on retry after the lock was released. Removing the migration's timeout made the embedded-migration assertion fail.

The real PNG upload journey passed against the recording-only relay and checked the tenant-scoped authenticated uploader row; removing the handler’s uploader write made it fail. A real upload database failure returned 503 and logged the database error without a false lease-loss warning. After removing an uploader row from a real server upload, the actual backfill CLI restored it from its retained audit record. PostgreSQL regressions passed on both desired-state and migrated schemas; persisted private-kind references created no grants, and malformed/unattributed audit evidence conferred no ownership. Mutations that admitted an engram or treated a non-upload audit as ownership both failed.

All 29 live media journeys passed again with a five-second usage-poller interval. Deletion and profile journeys waited for periodic cleanup: old public grants disappeared, and deleted channel attachments remained 404 after compaction. The private-channel journey added then removed a member using real signed membership events, verifying GET and HEAD changed from 200 to 404 while the owner retained access. Removing the membership tombstone check made that regression fail. Removing the live-source check or deleting the final channel boundary also failed authorization regressions.

The fresh security workflow failed before analysis because checkout refused fork code in a privileged pull_request_target job; the protection remains enabled, and this run provides no security review result.

Generated with Codex

Linear: BUZZ-358

[github-actions]

🔐 Codex Security Review

Status: review required for the current range.

The current range is dc25198d530acc6237a0320b697365f29daf0ea6...bb447892964c4b208416b722700489dd55b3e6bf.
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 bb447892964c4b208416b722700489dd55b3e6bf 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.

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

[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]

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

The rebase integration and migration 0067→0068 rename introduce no newly identified regression. The embedded migration assertions and fixtures follow the renumber consistently; the rebased snapshot writers retain the full-event follow-up hook inside their admitted transactions.

One additional P2 inline: bound the media migration’s lock wait. This is a newly identified material rollout risk in unchanged code, not a regression caused by the renumber: tracing the migrator shows that it explicitly disables session lock/statement timeouts, so the migration has no fallback bound when waiting for the communities table.

The legacy-reference and edit-lifetime findings and database-contract finding remain unresolved; they are not duplicated inline. Preserve the stated recording/log-only permission-reevaluation decision and resolve the previously identified rollout coverage gap before download enforcement. The prior public-description cleanup also remains: its internal tracker link is still present. No attached images appeared in the inspected description/discussion; linked issue contents were not inspected.

Minimalism 9/10; elegance 9/10; correctness 7/10 while the linked lifecycle/schema issues and migration lock bound remain unresolved.

Reviewed head 3d77f74 against base/merge-base 347ce88, focusing on changes since 61ef918 and the concrete migration risk above. Source only: no tests, SQL, builds, apps, backfill or PR code executed. One head-bound CI snapshot showed 49 successful, 29 skipped, one in-progress and one failed check; failure cause unverified. Runtime rollout, concurrency, performance and native upload/edit/profile error-retry/focus behavior remain unverified; the description still records the Desktop smoke test as pending. Non-blocking COMMENT review, not approval or merge authorization.

[ngoodger-block]

🤖 @wpfleger96 please re-review this updated stack at 11f017f: edit grants follow the original message lifetime, legacy profile/emoji replacements recover eligible references, the rollout decision precedes the first backfill, and the media migration now has a tested five-second lock budget; #8535 is the schema prerequisite, all 29 real relay media journeys passed, and the actual Desktop UI smoke test remains pending.

The security workflow has failed before analysis because checkout blocks fork PR code under pull_request_target; this is not a security pass.

[ngoodger-block]

🤖 @wpfleger96 please re-review 2a67653: the deferred feedback is now implemented, including uploader-handler coverage, inactive-grant cleanup, historical uploader recovery, private-kind exclusions, accurate error logging, positive batch sizes, and removed-member access coverage; all 29 live media journeys and the targeted mutation checks passed.

The author’s accepted backfill replay limitation remains documented; its conversations were closed as an accepted limitation, not a code fix. The actual Desktop UI smoke test remains pending. The fresh security workflow failed at the fork-checkout guard before analysis; the protection remains enabled, so there is no new security review result.