[ngoodger-block]

Stacked on #8451 (records where media is shared and backfills it). Until it merges, this diff includes it — review the commits after the merge of feat/media-shares. The default read mode is log-only; decide whether to accept persistent backfill grants before the first backfill, then repair gaps before enforce.

Why?

GET/HEAD /media only checks community membership, so anyone in a community who learns an attachment's hash — which is in every media URL — can download a file posted in a private channel or DM they cannot read.

What?

BUZZ_MEDIA_READ_MODE controls file access independently of authentication and community checks: off skips the new check, log-only (default) counts outcomes without denying reads, and enforce applies it. The setting loads at startup, so mode changes need a rolling restart but no code rebuild. Unknown values fail startup.

In enforce mode, GET and HEAD require a live share or uploader preview permission. A private file is readable only through an accessible channel; deleted or purged source messages grant nothing. Edit grants also require the original message to remain live. Author deletion and moderator removal revoke that message’s grants, including retained and edit-added attachments. Other undeleted shares keep working. Unauthorized reads return 404; lookup failures return 503 and are logged.

Community owners and admins who can view the moderation queue may also review a file attached to a live message with an open report. Each exceptional GET or HEAD requires a durable audit entry before storage access. Closing the report, deleting the logical message, or removing the queue role ends that exception; edit attachments follow the same rule. Report notes, unrelated hashes, blob-only reports, and other communities grant no access; reports create no public shares. Disabled or failed auditing blocks the exception.

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.

Rollout

Before the first backfill, explicitly accept grants from reconsidered references or defer until refusal-preserving behavior exists. Then deploy recording on every pod, run buzz-admin backfill-media-shares, and inspect buzz_media_read_access_total{mode="log-only",outcome="deny"} and outcome="error" before enforcing. The backfill now restores historical uploader proof from valid retained upload audits; missing evidence and unparsed references still need repair before enforcement. See operator instructions for mode changes, counters, and rollback.

Any backfill run, including the first, can grant previously refused references after their authors gain access. Those grants persist across read-mode changes and later prevention of reevaluation. Every invocation restarts the scan; unwanted existing grants need a provenance-based repair before enforcement.

The conformance table and NIP-FI captured-header note now describe this policy. Share creation already requires upload proof or a readable share in #8451.

Related issue

Closes the known limitation noted in conformance_multitenant.rs media_blossom. Read-enforcement half of the file-access stack after #8451.

Testing

On an isolated relay backed by PostgreSQL 17, Redis, and a disposable S3 HTTP fixture, ran RELAY_HTTP_URL=http://localhost:13071 cargo test -p buzz-test-client --test e2e_media_extended --test e2e_media_review -- --ignored --nocapture --test-threads=1 with BUZZ_MEDIA_READ_MODE=enforce: all 29 tests passed.

Repeated test_media_read_rollout_policy with the mode unset and with off: both passed, and Prometheus recorded two would-deny reads in the default log-only mode. The report journey submits a real NIP-56 report, verifies moderator GET/HEAD and their audit entries, rejects an unrelated attachment and another member, then confirms report closure revokes access. It also covers reported edit attachments and deletion of the original message while the report remains open. Removing the open-status SQL condition made the regression fail; restoring it passed.

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.

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

[github-actions]

🔐 Codex Security Review

Status: review required for the current range.

The current range is 347ce88d5dd8bc631c073f1947fea6b75d5a43c0...648358800bf07fea2ce2c6984d163396740f498d.
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 648358800bf07fea2ce2c6984d163396740f498d to authorize a new review.
Any previous review applies only to its recorded range.

[wpfleger96]

🤖 thanks for this. the GET/HEAD check itself looks right: both run it before touching storage, thumbnails are checked against the original hash, and a refused read gets the same 404 as a missing file. one blocking issue inline (agent avatars uploaded by the owner end up readable only by the owner), plus a few smaller items.

two things about the stack as a whole:

1. tightening the no-shares arm of readable_sql! is what closes the "anyone with the hash can share an unshared file" hole from the #8451 review, since record_media_shares uses the same macro. landing it here means it reaches prod after #8451 and the backfill, so the hole stays open that whole time. could the predicate change move into #8451, leaving this PR as just the read enforcement? it also can't revoke public grants created while only #8451 is deployed, and the backfill replay issue from that review is the same code here.
2. the PostgreSQL lane is red at this head, 1051 of 1052: migration_schema_fence_objects_are_optional_for_serving_and_deletion fails with community deletion catalog drift (missing=media_shares,media_uploaders). the fixture migrates through 66 (deletion.rs:4402) but EXPECTED_SCOPED_TABLES now includes the 0067 tables, so bumping it to 67 should fix it. the cause is in #8451, so it fails there too.

a few questions on rollout:

• there's no off switch or log-only mode, so at deploy any file the backfill missed (unparsed references, files uploaded before #8451 with no uploader row, the avatar case inline) 404s for everyone but its uploader, and the only way back is a redeploy. NIP-FI already has off / log-only / enforce. is the same worth doing here, so the "would deny" counts can be checked before enforcing?
• NIP-56 reports only land in moderation_reports, never events, so a report never creates a share. a mod who isn't in the private channel now gets a 404 on the reported file. should mods reviewing an open report be able to read it?
• the media row in docs/multi-tenant-conformance.md still lists the channel-ACL read gate as a remaining gap, and the NIP-FI read-proof note still says a captured header reads any media blob. is updating those meant to be part of this?

[wpfleger96]

@buzz-security-review ffaa6fb

[ngoodger-block]

🤖 Added off/log-only/enforce with a log-only default, audited owner/admin access limited to open reports’ live attachments, and both documentation corrections in dd02e04, with passing runtime regressions and Desktop smoke testing still pending.

[wpfleger96]

🤖 thanks for working through all of this. the earlier feedback is addressed: owner-uploaded agent avatars now get a share and have a cross-signer E2E test, the strict share rule lives in #8451, the migration-67 fixture is fixed, the off / log-only / enforce switch has counters and rollback docs, moderators can review attachments on open reports with audited reads, lookup failures are logged, and the conformance table and NIP-FI note are updated. the two deferrals (recovering past uploaders from audit_log, the removed-member regression) are fine as follow-ups.

two things before this can be approved:

1. this branch contains #8451 unchanged, so it also carries the backfill blocker from that review: post-cutoff events are re-checked with current permissions, so a refused reference can become a community-wide share once its author gains access. once that fix lands in #8451 and is merged here, this clears.
2. one wording fix in the NIP-FI note (inline).

[wesbillman]

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

Reviewed head dd02e04fb7bff7dc5ec533cd03bd623005d48415 against base 6e6173acd58029bac800583c4cc7482dafed693a, including the stacked #8451 changes. Two P2 correctness findings are inline. The backfill finding also confirms the existing review discussion; the edited-message deletion case is additional.

[P3] Public PR material: remove the internal Linear tracker link from “Related issue” and replace it with a public issue or a self-contained description. This public repository currently exposes an internal workspace/issue identifier there. No credential exposure is claimed, and the internal issue contents were not accessed.

Optional documentation precision: in docs/nips/NIP-FI.md:722–726, describe the file policy in protocol terms and keep deployment-mode names in the operator guide. Qualify “upload proof” as preview permission only while no channel share exists.

Validation: source only; no tests, builds, SQL, or apps executed. One CI snapshot showed 65 successful checks and 31 skipped; that does not validate the reported journeys. Desktop enforce-mode edit/upload/profile behavior, recovery/focus after media 404/503 responses, rolling-deploy provenance recovery, and writer-pool performance remain unverified. The PR description has no attached images. Minimalism/elegance: 9/10; correctness: 7/10 pending the two inline repairs. This is a non-blocking COMMENT review, not approval or merge authorization.

[ngoodger-block]

🤖 @wpfleger96 Please re-review the behavior-only NIP-FI wording and inherited backfill clarification at a8b3dd55e7e4082587f078b8e4abd5f90af74166; 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 flows) reviews.

Reviewed head a8b3dd55e7e4082587f078b8e4abd5f90af74166, base 7e6461242aa7d98f132919dd672f7cc5f4abbba6. This follow-up covers the three new documentation/test commits and membership-writer integration since dd02e04fb7bff7dc5ec533cd03bd623005d48415; the preceding 24 feature patches are unchanged.

One new P2 rollout-documentation finding is inline. No new defect found in the adapted fixture calls: membership changes commit before the database-backed authorization check, so discarding the returned snapshot is harmless in this test. The NIP-FI wording now describes protocol policy separately from rollout configuration.

The existing lifecycle/backfill and public-description observations remain in the previous review; the documentation changes do not repair those code paths, and they are not duplicated here.

Source-only review; no code, SQL, tests, builds, apps, or deployments executed. Public description inspected; no attached images. Linked internal issue content was not inspected. One CI snapshot showed 20 successful, 20 skipped, 18 in-progress and 1 failed check; failure cause and runtime correctness are unverified. The description’s test results were not independently reproduced or established for this exact head. Desktop edit/upload/profile workflow, database concurrency, rollout behavior, and error/retry focus remain unverified. This COMMENT is not 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/user-flow) reviews.

Reviewed head b267129ff7d64e87b23cda3a89977e8eeadb4a3e against base 347ce88d5dd8bc631c073f1947fea6b75d5a43c0. This follow-up compares the previously reviewed a8b3dd55e7e4082587f078b8e4abd5f90af74166 with the refreshed stack and checks integration with the new base. The feature interdiff is limited to migration 0067→0068 and corresponding migration-test constants; the media authorization and rollout text are unchanged.

No new findings in the renumber or inspected membership, transaction, TTL, and statement-limit integration. The existing edit-grant liveness and backfill findings in the initial review, and the rollout-ordering finding, remain applicable. The previously reported public-description tracker link also remains. These requested repairs are retained without duplicating inline comments; this is not a clean bill of health for the feature. The stacked schema-contract concern remains documented on #8451.

Validation: source only, immutable git objects; no tests, SQL, builds, apps, or deployments executed. Public description and discussion inspected; no attached images, and linked internal issue contents were not accessed. One CI snapshot showed 48 successful, 29 skipped, 7 in-progress and 1 failed security-review check; the failure cause and runtime correctness were not independently verified. Desktop edit/upload/profile behavior, error/retry focus after 404/503, database concurrency/performance, and rolling-deploy or previously applied PR-migration compatibility remain unverified. This non-blocking COMMENT is not approval or merge authorization.

[ngoodger-block]

🤖 @wpfleger96 please re-review this updated stack at 2cf0a83: 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.

[wesbillman]

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

Reviewed head 7b8bee1e07f43ff4b7653a480865766197ce1007 against base 347ce88d5dd8bc631c073f1947fea6b75d5a43c0, focusing on the follow-up since b267129ff7d64e87b23cda3a89977e8eeadb4a3e and its supported callers.

The prior edit-lifetime defect is repaired in source: ordinary and report-based reads require the edit’s original message to remain live, while independent live shares survive. Legacy profile/emoji replacement recovers eligible pre-cutoff references before retiring the old head within the admitted transaction. The schema prerequisite removes blanket bootstrap fencing and the new media tables’ FKs/fences consistently, retaining application admission and purge inventory. The rollout now requires the accept/defer decision before the first backfill, and the internal tracker link has been removed from the public description. No new defect found in these follow-up repairs.

Retained P2: bound migration 0068’s lock wait. The existing finding on stacked #8451 still applies here: migrations/0068_media_access.sql:2–3 requests ACCESS EXCLUSIVE on communities with no local timeout, while runtime/migration.rs disables session lock/statement timeouts. A conflicting transaction can leave the migration and subsequent community operations waiting indefinitely. Add a short SET LOCAL lock_timeout before the ALTER. This is carried code, independently confirmed in this review; the existing inline is linked rather than duplicated.

Backfill still reevaluates earlier refusals using current permissions. This remains an explicitly documented retained limitation, not a repaired code path; the revised instructions correctly require accepting the persistent resulting grants before any run or deferring backfill. Changing read modes does not revoke those grants.

Validation: source only; no tests, SQL, builds, apps, backfill, or deployments executed. Public description/discussion inspected; no attached images. The described 29-test run was not independently reproduced or bound to this exact head. One CI snapshot showed 46 successful, 29 skipped, 8 running, and 1 failed security-review check; failure cause was not independently verified. Runtime revocation/concurrency, query cost, rollout behavior, and native edit/upload/profile recovery and focus after 404/503 remain unverified; the Desktop smoke test is still pending. Minimalism/elegance 9/10; correctness 8/10 pending the retained lock bound. Non-blocking COMMENT, not approval or merge authorization.

[ngoodger-block]

🤖 @wpfleger96 please re-review 6483588: 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.