feat(media): write per-upload-event records for moderation#1551
Conversation
Add env-gated per-event upload records at
_uploads/{community_id}/{sha256}/{event_id}.json in the media bucket, one
per accepted upload (ULID event id). Each record is a versioned JSON
document (version=1) carrying the facts a moderation pipeline needs
without any further lookups: sha256, ext, mime type, size, upload time,
community id + hostname, uploader pubkey (hex + npub) and latest display
name, and optionally the connecting IP+port.
Key decisions:
- Records are written LAST, after blob + thumbnail + sidecar are durably
stored, and a record write failure fails the upload. The record's
S3 ObjectCreated notification is the moderation scan trigger, so record
existence must imply everything it references is readable.
- The idempotent both-exist short-circuit also writes a record: a
re-upload of known bytes is a distinct upload event, and without it
takedown re-uploads would be invisible to moderation.
- IP collection is opt-in and fail-empty: only read from the
operator-configured trusted edge header (BUZZ_MEDIA_UPLOAD_IP_HEADER),
validated as a public address, never the socket address. Port is only
kept alongside a valid IP. Config coherence (records off + IP header
set, port header without IP header, invalid header names) fails loudly
at startup rather than silently no-oping.
- The media serve path rejects _uploads/ and _meta/ key shapes so records
are never client-readable.
Env vars: BUZZ_MEDIA_UPLOAD_RECORDS, BUZZ_MEDIA_UPLOAD_IP_HEADER,
BUZZ_MEDIA_UPLOAD_PORT_HEADER, with matching Helm values
(relay.uploadRecords/uploadIpHeader/uploadPortHeader).
Co-authored-by: Bradley Axen <baxen@squareup.com>
Signed-off-by: Bradley Axen <baxen@squareup.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4d187cc71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || v4.is_broadcast() | ||
| || v4.is_documentation() | ||
| || v4.is_multicast() | ||
| || v4.is_unspecified() |
There was a problem hiding this comment.
When BUZZ_MEDIA_UPLOAD_IP_HEADER is enabled, this check only filters the single 0.0.0.0 address via is_unspecified(), so other addresses in the reserved 0.0.0.0/8 block (for example 0.1.2.3) pass through and get written into _uploads records as the uploader IP. That violates the fail-empty contract for non-public addresses and can misattribute moderation/legal reports if the trusted edge emits one of these reserved values; add an explicit 0/8 exclusion here.
Useful? React with 👍 / 👎.
| ("BUZZ_MEDIA_UPLOAD_PORT_HEADER", &self.upload_port_header), | ||
| ] { | ||
| if let Some(h) = value { | ||
| if h.is_empty() || !h.chars().all(|c| c.is_ascii_graphic() && c != ':') { |
There was a problem hiding this comment.
When an operator mistypes the configured header as valid ASCII but not a valid HTTP field-name, such as x/client-ip or client(ip), this validation accepts it even though requests cannot carry that header name and the later lookup will never collect an IP. Because these knobs are meant to fail loudly rather than silently omit legally relevant attribution, validate with HTTP token/HeaderName syntax instead of ascii_graphic.
Useful? React with 👍 / 👎.
Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
Reviewed this with a second independent pass — flagging two blocking items and two smaller ones inline. The feature's well-built overall: off-by-default, fail-empty IP handling, fail-loud config coherence, tight privacy blast radius, and a good test matrix. My main concern is the fresh-upload ordering (comment on upload.rs) — a record-write failure can leave media servable via the sidecar with the moderation hook never firing, which is the exact case this feature exists to prevent. The clippy one is a hard CI fail. The other two are non-blocking but I'd like the IP-range tightening in before this ships enabled anywhere.
One thing we looked at and explicitly did NOT block on: the unbounded _uploads/ record fan-out per blob under repeated re-upload. Auth + the existing upload rate limiter run first, and the unit of moderation is the accepted event — so it's a cost/noise vector, not a correctness bug. Worth pairing with retention/lifecycle if cost ever bites.
| // durably stored, so the record's ObjectCreated event (the | ||
| // moderation scan trigger) implies everything it references is | ||
| // readable. | ||
| if let Some(attribution) = &attribution { |
There was a problem hiding this comment.
hey Brad — I think there's a real hole in the fresh-upload ordering here. The record is written last, after the sidecar is already committed inside generate_and_store_metadata (put_sidecar at the tail). But the serve path gates only on the sidecar — get_blob does read_sidecar_mime(...).ok_or(NotFound)? in media.rs and never looks at the _uploads/ record (head_blob is the same). So if the record PUT fails here, the sidecar's already durable and the blob is fully servable by hash, but the record's ObjectCreated event — the only moderation-scan trigger — never fired. That's the exact case this feature exists to prevent: publishable media that skipped the hook. A retry re-drives the idempotent branch and can write the record, but nothing forces a client to retry, and a bad _uploads/ prefix policy would make every upload error out while still publishing via the sidecar. Could we make the record part of the publish gate — either hold the sidecar write until the record succeeds, or have get_blob/head_blob require the record? The record already carries ext/mime_type/size so reordering looks viable. Same ordering applies on the video path further down (sidecar then record).
| /// accepted upload — including the idempotent short-circuit, which does no | ||
| /// blob PUT and would otherwise be invisible to the moderation pipeline. The | ||
| /// record is written last, after all other objects are durably stored. | ||
| async fn process_buffered_upload<V, M, Fut>( |
There was a problem hiding this comment.
CI's red on this one — clippy::too_many_arguments (8/7) at the signature, and Rust Lint / Windows Rust run -D warnings so it's a hard fail, not just a warning. Rather than #[allow(...)], I think the cleaner fix is bundling the six stable request-context params (storage, config, ctx, auth_event, body, attribution) into a small internal BufferedUploadInput<'a> and leaving validate + store_metadata as the two real strategy hooks — it makes the invariant-vs-strategy split explicit instead of papering over it. Your call on the shape, but something that clears -D warnings structurally.
| || v4.is_broadcast() | ||
| || v4.is_documentation() | ||
| || v4.is_multicast() | ||
| || v4.is_unspecified() |
There was a problem hiding this comment.
one thing on the public-IP check: the v4 branch rejects only 0.0.0.0 via is_unspecified(), so an address like 0.1.2.3 in 0.0.0.0/8 (IANA "this-network", not globally reachable) gets recorded as a public IP. The v6 branch just below is also missing a few non-globally-reachable ranges — 6to4 2002::/16, Teredo 2001::/32, benchmarking 2001:2::/48, documentation 3fff::/20, discard 100::/64. Given the module's own note that a false positive is "a wrong address in a federal report," the false-positive direction is the one that actually hurts here, so I'd tighten it: reject 0.0.0.0/8 on v4 and add a v6 deny table for those special ranges, with tests. Not blocking the merge but I'd like it in before this ships enabled anywhere.
| ] { | ||
| if let Some(h) = value { | ||
| if h.is_empty() || !h.chars().all(|c| c.is_ascii_graphic() && c != ':') { | ||
| return Err(format!("{name} is not a valid header name: {h:?}")); |
There was a problem hiding this comment.
small one — this accepts any ASCII graphic except :, so a malformed name like bad/header or bad,header passes startup validation and then just fails empty at header lookup. That quietly weakens the otherwise-nice fail-loud config posture (the whole point of the coherence checks above is to catch operator mistakes at startup). Could validate with http::HeaderName::from_bytes instead — ideally storing the parsed HeaderName rather than a string so the lookup can't diverge from what you validated.
2 participants
Summary
Adds env-gated per-event records at
_uploads/{community_id}/{sha256}/{event_id}.jsonin the media bucket — one per accepted upload, ULID event id.Record shape (version 1)
Versioned/extensible JSON with everything the moderation pipeline needs with no further lookups:
{ "version": 1, "event_id": "01J...", // ULID, also the object key suffix "sha256": "...", "ext": "png", "mime_type": "image/png", "size": 12345, "uploaded_at": 1751820000, "community_id": "...", "community_host": "example.buzz.host", "uploader_id": "<hex pubkey>", "uploader_npub": "npub1...", "uploader_name": "display name at upload time", // optional "ip": "203.0.114.7", // optional, opt-in "port": 51234 // optional, only with ip }Design decisions
ObjectCreatednotification is the moderation scan trigger, so record existence must imply everything it references is readable. This makes a single record-driven consumer path sufficient — no legacy blob-event path needed.{sha256}/prefix.BUZZ_MEDIA_UPLOAD_IP_HEADER, e.g.cf-connecting-ip), validated as a public address (v4+v6; private/loopback/CGNAT/documentation ranges rejected). The socket address is never used — behind a sidecar it's meaningless, and a wrong address is worse than none. Port kept only alongside a valid IP._uploads/and_meta/key shapes (tested).Configuration
BUZZ_MEDIA_UPLOAD_RECORDSrelay.uploadRecordsBUZZ_MEDIA_UPLOAD_IP_HEADERrelay.uploadIpHeaderBUZZ_MEDIA_UPLOAD_PORT_HEADERrelay.uploadPortHeaderTesting
upload_record.rs(key shape, serialization round-trip, public-IP validation incl. v6 edge cases, port parsing) andconfig.rs(validation coherence matrix)api/media.rscargo test -p buzz-media(53 passed) and-p buzz-relay(478 passed);cargo check --locked --workspacecleanulidpackage addition