fix(media): recognize safe iCalendar attachments - #6385
Conversation
Signed-off-by: liowald <l.diwald@gmail.com>
Signed-off-by: liowald <l.diwald@gmail.com>
Signed-off-by: liowald <l.diwald@gmail.com>
Signed-off-by: liowald <l.diwald@gmail.com>
Signed-off-by: liowald <l.diwald@gmail.com>
liowald
changed the title
Signed-off-by: liowald <l.diwald@gmail.com>
Signed-off-by: liowald <l.diwald@gmail.com>
Signed-off-by: liowald <l.diwald@gmail.com>
Signed-off-by: liowald <l.diwald@gmail.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: ae6a770e14
ℹ️ 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".
Signed-off-by: liowald <l.diwald@gmail.com>
Signed-off-by: liowald <l.diwald@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 389cefa646
ℹ️ 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".
| fn signals_calendar_content(bytes: &[u8]) -> bool { | ||
| let unfolded = unfold_calendar_bytes(bytes); |
There was a problem hiding this comment.
When members concurrently upload large non-calendar attachments, signals_calendar_content copies each entire payload into a new Vec before determining that it is not a calendar. With the default 100 MiB generic-file limit and eight concurrent uploads, this adds up to 800 MiB on top of the already buffered request bodies and can cause relay OOMs. Detect the ASCII calendar envelope without cloning the whole body, reserving unfolding for payloads that actually require calendar validation.
Useful? React with 👍 / 👎.
| if declared_mime != Some("text/calendar") || extension != Some("ics") { | ||
| return Err(MediaError::DisallowedContentType( | ||
| declared_mime | ||
| .unwrap_or("application/octet-stream") | ||
| .to_string(), | ||
| )); |
There was a problem hiding this comment.
When a standard Blossom client uploads valid calendar bytes with Content-Type: text/calendar but no Buzz-specific X-Buzz-File-Extension, this branch rejects the request even though the same bytes succeed when sent as application/octet-stream via the content-based detector. Since validation already establishes that the body is a safe calendar, requiring both hints unnecessarily prevents interoperable clients from using the advertised Blossom-compatible upload endpoint.
Useful? React with 👍 / 👎.
|
Resolved the conflict with current @block/buzz-oss-team |
Signed-off-by: liowald <l.diwald@gmail.com> # Conflicts: # crates/buzz-cli/src/commands/messages.rs # desktop/src-tauri/src/commands/media.rs
Signed-off-by: liowald <l.diwald@gmail.com>
🔐 Codex Security Review
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fc248629d
ℹ️ 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".
| for raw_line in text.split('\n') { | ||
| let line = raw_line.strip_suffix('\r').unwrap_or(raw_line); |
There was a problem hiding this comment.
When an upload mixes CRLF separators with bare \r characters, splitting only on \n leaves the bare carriage returns embedded in a property value, so structural content lines can bypass the component checks. For example, SUMMARY:x\rBEGIN:VCALENDAR\rEND:VCALENDAR is treated as one harmless SUMMARY line and the upload is accepted, although calendar consumers that recognize bare-CR line endings see the hidden nested envelope. Reject bare carriage returns or tokenize all accepted line endings before validating component balance.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b8383c0. Calendar validation now rejects every bare CR before unfolding/parsing, with regressions for both hidden-component and bare-CR-separator inputs.
| // Canonicalize valid iCalendar bytes even when an older/generic client did | ||
| // not send hints. Sidecars are keyed only by the byte hash, so the same | ||
| // bytes must never alternate between `.bin` and `.ics` classifications. | ||
| if signals_calendar_content(bytes) { | ||
| return validate_calendar_content(bytes, config); |
There was a problem hiding this comment.
When these bytes were uploaded before this change, the generic path stored them as <hash>.bin with a hash-keyed sidecar whose authoritative extension is bin; mobile and desktop already allowed such generic uploads. Reuploading the same calendar after an upgrade now returns ics, causing the buffered pipeline to write <hash>.ics and overwrite that shared sidecar. The serve path rejects an explicit extension that differs from the sidecar, so every immutable message still referencing <hash>.bin starts returning 404. Preserve an existing classification or provide an alias/migration before changing the authoritative sidecar.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b8383c0. Existing hash sidecars are authoritative for buffered and video re-uploads, and first-writer classification claims plus conditional sidecar publication prevent updated writers from replacing immutable URLs. Live-MinIO regressions cover the pre-existing generic URL and moderation-record classification.
| let mut lines: Vec<String> = Vec::new(); | ||
| for raw_line in text.split('\n') { |
There was a problem hiding this comment.
The 10 MiB byte limit does not bound this derived representation: a valid envelope containing millions of short X:\n properties makes this loop allocate a separate String for every line plus a 24-byte vector entry and allocator metadata. With the relay's default eight concurrent uploads, small authenticated requests can therefore amplify into roughly gigabytes of heap and exhaust the process. Parse without per-line ownership or impose a content-line/component limit before populating this vector.
AGENTS.md reference: AGENTS.md:L194-L202
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b8383c0. Parsing keeps borrowed line slices, enforces a 100,000 physical/content-line cap before structural work can amplify, and includes adversarial non-empty and blank-line regressions.
Signed-off-by: liowald <l.diwald@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8383c0197
ℹ️ 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".
| uploaded_at, | ||
| }, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
When upload records are enabled during a rolling deployment, a legacy writer can publish a different sidecar after this request has already persisted its record; if this corrective PUT then fails transiently, the original record remains permanently committed with the wrong MIME and extension. Retrying the upload creates another record but never repairs the incorrect append-only moderation/legal record, so determine the canonical facts before the initial write or durably retry the correction.
AGENTS.md reference: AGENTS.md:L204-L209
Useful? React with 👍 / 👎.
| if !filename_value.is_empty() && !filename_value.to_ascii_lowercase().ends_with(".ics") | ||
| { |
There was a problem hiding this comment.
When a client submits a text/calendar imeta tag without filename, this conditional accepts it, leaving file-card consumers to display the hash-based URL tail instead of the calendar's user-facing name. Fresh evidence in this exact commit is the new !filename_value.is_empty() guard and regression test accepting omission, whereas the review thread records that the approved contract requires one safe .ics filename; reject an empty filename as well as a non-.ics one.
Useful? React with 👍 / 👎.
| || (components.is_empty() && !value.eq_ignore_ascii_case("VCALENDAR")) | ||
| || (!components.is_empty() && value.eq_ignore_ascii_case("VCALENDAR")) |
There was a problem hiding this comment.
When an upload concatenates two complete calendars, closing the first empties components, so this condition permits another BEGIN:VCALENDAR; the final checks then see a balanced stack and the second closing line and accept the whole payload. This violates the validator's one-envelope contract and creates parser-dependent contents where consumers may process only the first calendar or both, so track that the outer calendar has already been opened or closed and reject any subsequent top-level envelope.
Useful? React with 👍 / 👎.
1 participant
Problem
On current
origin/main(e5d1dfef7bf24ad527c9c8c1785b613abad574f7),.icshas noinfermagic-byte signature. A valid calendar becomesapplication/octet-stream, andbuzz upload file --file Planning.icsis rejected before network I/O as unsupported.Buzz deliberately supports generic non-media attachments through a deny-list plus inert forced downloads (#5569). This PR keeps PDF, ZIP, Office, text, HTML, archives, audio, images, and video unchanged.
Smallest approach
.ics/text/calendarmetadata as untrusted hints on standard Blossom/upload.VCALENDARenvelope, and balanced components..icsfilename from the relay-authoritative returned MIME, including generic or extensionless input names./media/uploadmedia-only and never retry an explicitly signaled calendar there.Existing boundaries remain authoritative: hash-bound Blossom auth, tenant sidecars, byte-identical storage,
Content-Disposition: attachment,nosniff, and CSPdefault-src none.Related work
The closest overlapping generic CLI contribution is #4880. Also related: #3641, #2251, #2813, closed duplicate #5585, and issues #2963, #3453, and #5675. This PR does not duplicate their broader generic attachment or preview work.
Verification
.ics; old client logic fails the new descriptor-authoritative regression by emittingPlanning.txtinstead ofPlanning.ics.cargo test -p buzz-media— 123 passed.cargo test -p buzz-cli— 353 passed.pnpm run check, Rust formatting, and diff checks — passed (only unrelated pre-existing desktop Biome warnings/infos).buzz messages send --fileround-trip produced event4450fba09e617b314739775ed3f5cefdcbc72dae659b6192231b01e7fc7c890a,m text/calendar,filename calendar-roundtrip.ics, and byte-identical SHA-256030c68c5a7f509b07117e617d646e6683de135eee09d9996e721ea20dc6fb4d5.just cicannot run the desktop Tauri Rust lane on this host because GTK/GLib/GIO development packages (glib-2.0.pc,gobject-2.0.pc,gdk-3.0.pc) are absent. Tauri tests are included for supported CI hosts; all runnable focused gates above pass.Non-goals
No new dependency, endpoint, schema, preview UI, renderer, or generic attachment-policy rewrite. This PR is ready for Block maintainer review.
Reconciliation update
block/buzz:mainat24ec6a468ec9d0d425ee58fbfc4d416412c446ad; GitHub now reports the PR mergeable..icscalendar filename.389cefa6465525232f0ea20e5b41afc3bdb2a18d(signed off).pnpm run check, Rust formatting, diff checks, and the three focused regression tests passed. Two independent factory reviewers approved the identical tree.