Skip to content

fix(cli): stop rejecting generic files on upload, fix attachment markdown - #3641

Open
kcao-gss wants to merge 1 commit into
block:mainfrom
kcao-gss:fix/cli-upload-octet-stream
Open

fix(cli): stop rejecting generic files on upload, fix attachment markdown#3641
kcao-gss wants to merge 1 commit into
block:mainfrom
kcao-gss:fix/cli-upload-octet-stream

Conversation

@kcao-gss

Copy link
Copy Markdown
Contributor

Root cause

buzz upload file gated every upload client-side through a five-entry ALLOWED_MIMES allowlist (image/jpeg, image/png, image/gif, image/webp, video/mp4). Anything else — including application/octet-stream (what infer returns for un-sniffable JSON/plain-text content) and application/pdf (detected correctly by infer, but never allowlisted) — was rejected before the request left the process.

The relay's generic-file /upload path already accepts this content via a deny-list validator (buzz_media::validation), and Desktop's uploader (desktop/src-tauri/src/commands/media.rs) mirrors that deny-list client-side. The CLI's allowlist was the only client in this codebase enforcing a narrower policy than the relay actually allows.

Repro (before this PR, against the live dev relay):

$ echo '{"a":1}' > test.json
$ buzz upload file --file test.json
{"error":"user_error","message":"unsupported file type: application/octet-stream","retryable":false}

Fix

  • Delete the CLI-side ALLOWED_MIMES allowlist gate entirely rather than porting Desktop's deny-list into the CLI. The relay/Desktop/mobile already each carry a copy of that policy; a fourth CLI-side copy would drift. The relay is the sole enforcement point — MIME detection is kept (it still feeds Content-Type, the Blossom signature, and the descriptor).
  • Raise the non-video size cap from 50 MB to 100 MB (MAX_FILE_BYTES) to match the relay's default_max_file_bytes — the old cap under-allowed generic files relative to what the relay accepts.
  • Fix buzz messages send --file: it rendered every non-video attachment as ![image](url), which was unreachable while the allowlist blocked non-image uploads but would post a broken inline image for JSON/text/PDF attachments once the gate was removed. Now branches three ways: image → ![image](url), video → ![video](url), everything else → a plain [filename](url) link.
  • Documented (not fixed) a known gap: the /media/upload legacy fallback still hard-rejects non-image/video content server-side, so a generic file against an old relay lacking /upload will 404 there, fall back to /media/upload, and fail with a confusing DisallowedContentType instead of a clear version error. Follow-up.

Verification

  • cargo test -p buzz-cli: 262 passed, 0 failed.
  • Live before/after against the dev relay: pre-fix binary reproduces the JSON rejection above; post-fix binary uploads JSON, plain text, and PDF successfully with correct detected MIME (application/octet-stream, application/octet-stream, application/pdf respectively).
  • Live end-to-end: buzz messages send --file test.json posts successfully with the JSON attachment rendered as a plain link, not a broken image.

Trade-off carried to review

This removes a defense-in-depth client-side gate. Upload safety now rests entirely on the relay's deny-list validator plus serve_inline never inlining non-media content. Both exist today and are tested, but worth a conscious sign-off rather than silently accepting.

Mobile (mobile/lib/shared/relay/media_upload.dart:363) has the same class of allowlist gate — separate follow-up, not in scope here.

@kcao-gss
kcao-gss requested a review from a team as a code owner July 29, 2026 23:16
@kcao-gss

Copy link
Copy Markdown
Contributor Author

PR #3641 review — first pass done. Diagnosis and approach are right. One must-fix before merge, two nits, and the defense-in-depth decision you asked me for.

Defense-in-depth trade: accept it. Do not port Desktop's deny-list into the CLI.

Not on maintainability grounds alone — the deleted gate was not buying security in the first place, and neither would a CLI deny-list.

  1. A client-side gate is not a boundary. Anyone can curl the Blossom endpoint. It only constrained cooperative users, which is exactly the population that hit this bug.
  2. The relay's deny-list catches almost nothing anyway. BLOCKED_FILE_MIME_TYPES is consulted only in the Some(kind) arm of infer::get (crates/buzz-media/src/validation.rs:183-199). SVG, HTML, and JS have no magic bytes, so they sniff as None, fall to Ok(("application/octet-stream", "bin")) at validation.rs:205, and pass. A fourth copy of that list in the CLI would block the same near-empty set.
  3. The layer that actually protects clients is serve_inline + Content-Disposition: attachment + nosniff + CSP: default-src 'none' (crates/buzz-relay/src/api/media.rs:659-667, validation.rs:216-218), and it is unchanged by this PR. Non-media never renders as active content regardless of what any client uploads.

So the trade is real but the thing being traded away was ornamental. @chrollo your ruling stands — no re-route to Feitan needed.

Must fix: the 50 → 100 MB cap change is wrong for images, and the comment asserting otherwise is factually false

crates/buzz-cli/src/client.rs:1111-1119 now sends every non-video file down a single 100 MB cap, with this comment:

Must match the relay's per-category caps (buzz_media::config::MediaConfig defaults): video 500 MB, everything else (images and generic files) 100 MB.

The relay does not have that shape. crates/buzz-media/src/config.rs:33-41 defines four caps: max_image_bytes 50 MB, max_gif_bytes 10 MB, max_video_bytes 500 MB, max_file_bytes 100 MB. Images are enforced at validation.rs:250-260 (max_gif_bytes for image/gif, max_image_bytes otherwise) — and images do reach that path, because /upload routes sniffed jpeg/png/gif/webp to process_upload (crates/buzz-relay/src/api/media.rs:369-380), not to process_file_upload.

Failure case: buzz upload file --file photo.png at 60 MB. Before this PR: instant local file too large at the 50 MB check. After: passes the client check, uploads all 60 MB, the relay buffers it (the to_bytes limit is max(50, 100) = 100 MB, api/media.rs:369-374), then validate_content rejects with FileTooLarge. A 20 MB GIF is the same story against the 10 MB cap, though that one was already over-permissive at 50 MB and is not a regression.

That is strictly worse than what shipped before, in the exact lines the PR touched, and it contradicts the invariant the new comment claims. Fix is small:

/// Relay per-category caps — must match `buzz_media::config::MediaConfig`
/// defaults (`max_gif_bytes`, `max_image_bytes`, `max_video_bytes`,
/// `max_file_bytes`).
const MAX_GIF_BYTES: u64 = 10 * 1024 * 1024;
const MAX_IMAGE_BYTES: u64 = 50 * 1024 * 1024;
const MAX_VIDEO_BYTES: u64 = 500 * 1024 * 1024;
const MAX_FILE_BYTES: u64 = 100 * 1024 * 1024;

let max = if mime.starts_with("video/") {
    MAX_VIDEO_BYTES
} else if mime == "image/gif" {
    MAX_GIF_BYTES
} else if mime.starts_with("image/") {
    MAX_IMAGE_BYTES
} else {
    MAX_FILE_BYTES
};

Raising the generic-file cap to 100 MB was correct and is the part worth keeping. Collapsing images into it was not.

Nit 1: unescaped filename in the markdown link text

crates/buzz-cli/src/commands/messages.rs:634-640 interpolates the raw filename into [...]. A file named notes]v2.json produces [notes]v2.json](url), which renders as broken markdown. Also .unwrap_or(file_path) falls back to the full path — for a path ending in .. or a root, the link text becomes the caller's local absolute path in a channel-visible message. Escaping [/] (or falling back to a literal "attachment" rather than the path) closes both.

Nit 2: the 100 MB test is expensive for just test-unit

upload_file_rejects_generic_file_over_100mb_cap (client.rs:2290-2302) allocates a 100 MB Vec, writes it to disk, and then upload_file std::fs::reads it back — roughly 200 MB resident plus 100 MB of disk write, in a suite that runs tests in parallel. The boundary is worth testing; consider making the cap injectable on the client so it can be asserted at a few KB. Not a merge blocker.

What is good here

The three-way branch in messages.rs is the right call and it is the defect nobody upstream had spotted — fixing the allowlist without it would have shipped broken inline images to every client. The legacy-fallback comment at client.rs:1178-1186 documents the trap in the exact place a reader hits it instead of burying it in a PR description. And stopping to report that the handed-off root cause was backwards, rather than implementing it, is why this diff is correct at all. @feitan @Nobunaga that is the behavior worth repeating.

Not in my scope to clear

Nothing in this diff touches infra, credentials, or migrations. The one thing I cannot evaluate from the diff is whether any deployed relay runs a non-default max_image_bytes — if a relay is configured above 50 MB, the per-category constants I am asking for become a client that under-reports its own limit. That is config-side and needs the Infra Team, not a code change here.

@chrollo — the trade-off decision is yours to close out: accepted, no re-route. The size cap is a real must-fix, one small hunk in client.rs. Nits are optional. Not a gate — a human reviewer still owns approval.

…down

buzz upload file client-side-gated every upload through a five-entry
ALLOWED_MIMES allowlist (image/jpeg, image/png, image/gif, image/webp,
video/mp4). Anything else — including application/octet-stream, which is
what `infer` returns for un-sniffable JSON/plain-text content, and PDF,
which infer detects correctly but which was never allowlisted — was
rejected before the request ever left the process.

The relay's generic-file upload path already accepts this content via a
deny-list validator (buzz_media::validation), and Desktop's uploader
mirrors that deny-list client-side. Rather than adding a second copy of
that policy to the CLI (a fourth copy alongside relay/desktop/mobile),
drop the CLI-side gate entirely — the relay is the sole enforcement
point.

Also:
- Raise the non-video size cap from 50 MB to 100 MB to match the relay's
  generic-file default_max_file_bytes (images were previously capped
  below what the relay allows for non-video content).
- Fix `buzz messages send --file` to render non-image/non-video
  attachments as a plain markdown link instead of `![image](...)`, which
  was unreachable while the allowlist blocked those uploads but would
  have rendered as a broken image once the gate was removed.
- Document a known gap: the /media/upload legacy fallback still
  hard-rejects non-image/video content server-side, so a generic file
  against an old relay lacking /upload will now fail there with a
  confusing DisallowedContentType instead of a clear version error.
  Follow-up, not fixed here.

Verified live against the dev relay (upload of JSON/text/PDF all now
succeed with the correct detected MIME; message with a JSON attachment
renders as a link).

Signed-off-by: Kyler Cao <kcao@gssmail.com>
@kcao-gss
kcao-gss force-pushed the fix/cli-upload-octet-stream branch from e002f56 to 917d5fe Compare July 30, 2026 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant