fix(provider-utils): bound media-type sniffing decode for ID3-prefixed input - #17386
Conversation
…d input Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bugfix reviewOutcome: changes-required Fixes issueStatus: partially-addresses The implementation removes the attachment-sized decode and copy, but it does not preserve the claimed detection behavior for ID3 tags up to the 128 KiB limit. Concerns:
Side effectsRisk: low Legitimate MP3 inputs near the new scan boundary can stop auto-detecting, with representation-dependent results at one boundary value. Concerns:
PerformanceRisk: low Work is now bounded independently of attachment size, and subarray avoids copying raw input; ID3-prefixed base64 still incurs a bounded allocation of roughly 128 KiB. Backwards compatibilityRisk: low The change does not mutate stored data, but existing stored MP3 attachments near the cutoff can produce different detection results or fail subtype resolution. Concerns:
SecurityRisk: none The change reduces denial-of-service exposure by replacing attachment-sized decoding and copying with a fixed upper bound and introduces no new privileged operations. TestingStatus: needs-more The 67-test detection suite passes in Node and Edge, but the added test covers only an oversized tag and misses the failing cutoff and representation-parity cases. Concerns:
VerificationInspected the merge-base diff and media-type callers, ran the complete detect-media-type suite in Node and Edge, package type-checking, formatting and lint checks, and focused boundary probes. Standard checks passed, while boundary probes exposed missed detection at the advertised limit and a raw/base64 mismatch. |
…ID3 limit detectable Trim the base64 decode to exactly the requested byte count so string and raw-byte inputs sniff identically, and reserve signature room past the ID3 tag so a tag at the size limit still matches. Add exact-cutoff parity tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks, both concerns were real and are fixed in 947aabd. Signature headroom at the limitThe scan bound was the total decoded prefix, but
Cutoff is now exact and testable: a tag of Base64 vs Uint8Array parityRoot cause: Tests
Side-effect note standsAn ID3v2 tag larger than 128 KiB is still not scanned past, so such an MP3 falls back to the caller-supplied media type ( |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bugfix reviewOutcome: approved Fixes issueStatus: fully-addresses Media sniffing now decodes only a fixed-size prefix, uses bounded subarray views for byte input, preserves signature headroom after supported ID3 tags, and produces matching results for raw-byte and base64 representations at the cutoff. Side effectsRisk: low Normal media signatures and ID3-tagged MP3s through the documented 128 KiB limit remain detectable; larger ID3 tags now intentionally require a caller-supplied full media type. Concerns:
PerformanceRisk: none Attachment-sized base64 decoding and copying are eliminated. ID3-prefixed base64 input incurs only a bounded decode of roughly 128 KiB, while raw input uses non-copying subarray views. Backwards compatibilityRisk: low The change does not mutate stored data, although stored MP3 attachments with oversized ID3 tags may now fail automatic subtype resolution when reused without a full media type. Concerns:
SecurityRisk: none The change reduces denial-of-service exposure by bounding work independently of attachment size and introduces no new privileged operations or unsafe parsing. TestingStatus: appropriate Regression coverage includes ordinary ID3 MP3s, the exact supported cutoff, one byte beyond it, and raw-byte/base64 parity. Both Node and Edge provider-utils suites passed. VerificationInspected the merge-base diff, callers, changeset, and follow-up boundary fix; ran the complete provider-utils Node and Edge suites, package type checking, repository formatting and lint checks, diff validation, and a focused instrumentation probe confirming that a multi-megabyte ID3-prefixed base64 attachment is decoded only through the fixed prefix. |
lgrammel
added
task-bugfix-review-approved
and removed
task-bugfix-review-changes-required
labels
|
🚀 Published in:
|
…prefixed input (#17725) ## Summary Media-type sniffing is documented as an O(1) check on the first ~18 bytes (24 base64 chars). An `ID3`/`SUQz`-prefixed attachment bypassed that bound: `stripID3TagsIfPresent` ran before the truncation and decoded the entire base64 attachment (plus a full-size `.slice()` copy), then returned a `Uint8Array` so the 24-char cap was skipped. A 4-character prefix flipped intended O(1) sniffing into an O(N) decode + copy of the whole attachment, on every path including image detection. ## Hardening - Decode a bounded prefix first, then strip ID3 within that bound, so no input can force a full decode. - Detection now runs on decoded bytes (`ID3`) instead of the base64 string (`startsWith('SUQz')`), so string and raw-byte inputs are checked uniformly and base64 alignment tricks cannot fool it. - The raw-`Uint8Array` path is bounded too, and `stripID3` uses `subarray` (a view) instead of `slice` (a full copy), removing the second O(N) copy. - ID3 skip is capped at `ID3_MAX_SCAN_BYTES` (128 KB), large enough to cover typical tags with embedded cover art while keeping cost constant in attachment size. ## Happy path - Non-ID3 attachments (PNG, JPEG, WEBP, PDF, WAV, and so on) decode only ~18 bytes exactly as before, so common detection is unchanged. - MP3s with an ID3v2 tag up to 128 KB, including typical embedded cover art, still auto-detect as `audio/mpeg`. - `data:` URLs, base64url, `ArrayBuffer`, `Buffer`, and `text` inputs are normalized upstream and behave the same. ## Unhappy path - An `ID3`/`SUQz`-prefixed attachment (benign or crafted) now decodes at most ~128 KB regardless of attachment size, so sniffing stays constant-time instead of scaling with the input. - An ID3v2 tag larger than 128 KB is no longer scanned past, so such an MP3 is not auto-detected and falls back to the caller-supplied media type (`resolveFullMediaType` throws its existing "must specify subtype" error if none was provided). Raise `ID3_MAX_SCAN_BYTES` if larger tags must be detected; cost stays O(1) either way. - A crafted oversized `id3Size` cannot read out of bounds: `subarray` clamps to length and returns empty, so no signature matches and detection returns `undefined`. ## Testing - Added a regression test that an ID3 tag larger than the scan bound is not detected (locks in the O(1) behavior). - Full `detect-media-type` suite passes (67 tests). Fixes #17709 ## Related Issues Backport of #17386 --------- Co-authored-by: Mukund Sarma <23266464+dnukumamras@users.noreply.github.com>
lgrammel
added
task-merge-tracking-done
and removed
task-merge-tracking-no-issue
labels
…prefixed input (#17746) ## Summary Media-type sniffing is documented as an O(1) check on the first ~18 bytes (24 base64 chars). An `ID3`/`SUQz`-prefixed attachment bypassed that bound: `stripID3TagsIfPresent` ran before the truncation and decoded the entire base64 attachment (plus a full-size `.slice()` copy), then returned a `Uint8Array` so the 24-char cap was skipped. A 4-character prefix flipped intended O(1) sniffing into an O(N) decode + copy of the whole attachment, on every path including image detection. ## Hardening - Decode a bounded prefix first, then strip ID3 within that bound, so no input can force a full decode. - Detection now runs on decoded bytes (`ID3`) instead of the base64 string (`startsWith('SUQz')`), so string and raw-byte inputs are checked uniformly and base64 alignment tricks cannot fool it. - The raw-`Uint8Array` path is bounded too, and `stripID3` uses `subarray` (a view) instead of `slice` (a full copy), removing the second O(N) copy. - ID3 skip is capped at `ID3_MAX_SCAN_BYTES` (128 KB), large enough to cover typical tags with embedded cover art while keeping cost constant in attachment size. ## Happy path - Non-ID3 attachments (PNG, JPEG, WEBP, PDF, WAV, and so on) decode only ~18 bytes exactly as before, so common detection is unchanged. - MP3s with an ID3v2 tag up to 128 KB, including typical embedded cover art, still auto-detect as `audio/mpeg`. - `data:` URLs, base64url, `ArrayBuffer`, `Buffer`, and `text` inputs are normalized upstream and behave the same. ## Unhappy path - An `ID3`/`SUQz`-prefixed attachment (benign or crafted) now decodes at most ~128 KB regardless of attachment size, so sniffing stays constant-time instead of scaling with the input. - An ID3v2 tag larger than 128 KB is no longer scanned past, so such an MP3 is not auto-detected and falls back to the caller-supplied media type (`resolveFullMediaType` throws its existing "must specify subtype" error if none was provided). Raise `ID3_MAX_SCAN_BYTES` if larger tags must be detected; cost stays O(1) either way. - A crafted oversized `id3Size` cannot read out of bounds: `subarray` clamps to length and returns empty, so no signature matches and detection returns `undefined`. ## Testing - Added a regression test that an ID3 tag larger than the scan bound is not detected (locks in the O(1) behavior). - Full `detect-media-type` suite passes (67 tests). Fixes #17709 ## Related Issues Backport of #17386 --------- Co-authored-by: Mukund Sarma <23266464+dnukumamras@users.noreply.github.com>
2 participants
Summary
Media-type sniffing is documented as an O(1) check on the first ~18 bytes (24 base64 chars). An
ID3/SUQz-prefixed attachment bypassed that bound:stripID3TagsIfPresentran before the truncation and decoded the entire base64 attachment (plus a full-size.slice()copy), then returned aUint8Arrayso the 24-char cap was skipped. A 4-character prefix flipped intended O(1) sniffing into an O(N) decode + copy of the whole attachment, on every path including image detection.Hardening
ID3) instead of the base64 string (startsWith('SUQz')), so string and raw-byte inputs are checked uniformly and base64 alignment tricks cannot fool it.Uint8Arraypath is bounded too, andstripID3usessubarray(a view) instead ofslice(a full copy), removing the second O(N) copy.ID3_MAX_SCAN_BYTES(128 KB), large enough to cover typical tags with embedded cover art while keeping cost constant in attachment size.Happy path
audio/mpeg.data:URLs, base64url,ArrayBuffer,Buffer, andtextinputs are normalized upstream and behave the same.Unhappy path
ID3/SUQz-prefixed attachment (benign or crafted) now decodes at most ~128 KB regardless of attachment size, so sniffing stays constant-time instead of scaling with the input.resolveFullMediaTypethrows its existing "must specify subtype" error if none was provided). RaiseID3_MAX_SCAN_BYTESif larger tags must be detected; cost stays O(1) either way.id3Sizecannot read out of bounds:subarrayclamps to length and returns empty, so no signature matches and detection returnsundefined.Testing
detect-media-typesuite passes (67 tests).Fixes #17709