Skip to content

fix(desktop): detect WebP and AVI in RIFF magic-byte sniffing - #5336

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
he-yufeng:fix/riff-webp-magic-detection
Jun 18, 2026
Merged

fix(desktop): detect WebP and AVI in RIFF magic-byte sniffing#5336
wenshao merged 1 commit into
QwenLM:mainfrom
he-yufeng:fix/riff-webp-magic-detection

Conversation

@he-yufeng

Copy link
Copy Markdown
Contributor

What

detectExtensionFromMagic (desktop binary-detection.ts) now distinguishes RIFF container subtypes by the four-character form tag at bytes 8-11, so WebP and AVI are no longer reported as .wav.

Why

The MAGIC_SIGNATURES table mapped the bare RIFF prefix (bytes 0-3) straight to .wav. But RIFF is a shared container — WAV, WebP and AVI all start with RIFF, and only the form tag at bytes 8-11 (WAVE / WEBP / AVI ) tells them apart. As a result any WebP or AVI binary flowing through the magic-byte fallback (getMimeExtension when MIME is unknown, extractBase64Binary, saveBinaryResponse) was mislabeled .wav.

This is the same RIFF-prefix-is-not-enough issue already fixed for the weixin channel (ea97f297d, "confirm the WEBP signature, not just the RIFF prefix"); the desktop binary detector still had the bare-prefix check. Unknown RIFF subtypes now return '' instead of guessing .wav.

Reviewer Test Plan

cd packages/desktop/packages/shared && bun test src/utils/__tests__/binary-detection.test.ts

Added cases assert a RIFF/WEBP buffer resolves to .webp (previously .wav) while a RIFF/WAVE buffer still resolves to .wav.

Risk

Low. Only changes how RIFF-prefixed buffers are classified; WAV detection is preserved by a regression test, and non-RIFF signatures are untouched.

detectExtensionFromMagic matched the bare "RIFF" prefix and always
returned .wav, so WebP and AVI files (which share the RIFF container)
were mislabeled as .wav. Disambiguate by the four-character form tag at
bytes 8-11, mirroring the existing weixin WebP signature check.
@wenshao

wenshao commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @he-yufeng!

Template covers the essentials ✓ — What, Why, Test Plan, and Risk are all present. A couple of optional sections (Tested on table, Linked Issues, 中文说明) are missing, but for a focused bugfix like this that's fine.

On direction: this is a clear-cut correctness fix. RIFF is a shared container — mapping all RIFF-prefixed buffers to .wav is wrong, and WebP/AVI misidentification has real downstream impact on getMimeExtension, extractBase64Binary, and saveBinaryResponse. The PR body even calls out the prior weixin-channel fix (ea97f297d) that addressed the same class of bug there. Aligned and well-motivated. CHANGELOG has a related precedent: "weixin: check full 4-byte PNG magic signature" (#2970) — same pattern, different surface.

On approach: the scope is tight — remove the bare RIFF entry from the flat lookup, add a dedicated branch that reads the form tag at bytes 8-11, and cover WebP + WAV with regression tests. One small gap: the title mentions AVI detection and the code handles it, but there's no corresponding AVI test case. Not blocking, but a one-liner expect(detectExtensionFromMagic(riffBuffer('AVI '))).toBe('.avi') would close the loop. Otherwise, minimal and correct.

Moving on to code review. 🔍

中文说明

感谢 @he-yufeng 的贡献!

模板基本完整 ✓ — What、Why、Test Plan、Risk 均已涵盖。部分可选小节(测试平台表、关联 Issue、中文说明)未填写,但对于这种聚焦的 bugfix 来说没有问题。

方向:这是一个明确的正确性修复。RIFF 是共享容器格式,将所有 RIFF 前缀的 buffer 都映射为 .wav 是错误的,WebP/AVI 的误识别会实际影响 getMimeExtensionextractBase64BinarysaveBinaryResponse 的下游行为。PR 描述中还引用了此前 weixin 渠道的同类修复(ea97f297d),说明作者了解问题背景。CHANGELOG 也有先例:"weixin: check full 4-byte PNG magic signature" (#2970)——同一类问题,不同入口。

方案:范围紧凑——从平面查找表中移除裸 RIFF 条目,新增一个专门分支读取第 8-11 字节的 form tag,并为 WebP 和 WAV 添加回归测试。一个小缺口:标题提到了 AVI 检测,代码也处理了 AVI,但缺少对应的 AVI 测试用例。不阻塞合并,但加一行 expect(detectExtensionFromMagic(riffBuffer('AVI '))).toBe('.avi') 就能补全。整体精简且正确。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

The implementation is clean and matches exactly what I'd do independently. Remove the bare RIFF → .wav entry from the flat lookup, add a dedicated branch in detectExtensionFromMagic that reads the form tag at bytes 8-11, and dispatch on the four-character code. The buffer.length >= 12 guard is correct, and returning '' for unknown RIFF subtypes is the right conservative choice.

The test helper riffBuffer() is well-designed — constructs a minimal valid RIFF header with the form tag parameterized, making it easy to add more subtypes later.

One minor gap noted in Stage 1: the code handles AVI but there's no corresponding test. Not blocking.

No critical issues. No AGENTS.md violations. The diff is focused — only the two files needed for the fix and its tests, no drive-by changes.

Test Results

All 37 tests pass on the PR branch, including the two new regression tests:

$ bun test src/utils/__tests__/binary-detection.test.ts

✓ detectExtensionFromMagic > detects WebP from the RIFF form tag, not as WAV [0.03ms]
✓ detectExtensionFromMagic > still detects WAV from the RIFF/WAVE form tag [0.01ms]

 37 pass
 0 fail

Before / After Verification

Ran the detection function against synthetic RIFF buffers on both branches:

=== BEFORE (main, without fix) ===
RIFF/WEBP -> ".wav"    ← wrong
RIFF/WAVE -> ".wav"    ← correct
RIFF/AVI  -> ".wav"    ← wrong
RIFF/UNKN -> ".wav"    ← wrong

=== AFTER (PR #5336, with fix) ===
RIFF/WEBP -> ".webp"   ← fixed
RIFF/WAVE -> ".wav"    ← still correct
RIFF/AVI  -> ".avi"    ← fixed
RIFF/UNKN -> ""        ← fixed (no longer guesses)

The fix works as described. WebP and AVI are now correctly identified, WAV detection is preserved, and unknown RIFF subtypes no longer get a wrong .wav label.

中文说明

代码审查

实现干净,与我独立设想的方案完全一致:从平面查找表中移除裸 RIFF → .wav 条目,在 detectExtensionFromMagic 中新增专门分支读取第 8-11 字节的 form tag 进行分发。buffer.length >= 12 的守卫条件正确,对未知 RIFF 子类型返回 '' 是合理的保守选择。

测试辅助函数 riffBuffer() 设计良好——构造了一个最小化的合法 RIFF 头部,form tag 可参数化,便于后续扩展更多子类型。

Stage 1 中提到的小缺口:代码处理了 AVI 但缺少对应测试,不阻塞合并。

无关键问题,无 AGENTS.md 违规。Diff 聚焦——仅修改了修复和测试所需的两个文件,无夹带改动。

测试结果

PR 分支全部 37 个测试通过,包括两个新增的回归测试。

修复前后对比

在两个分支上分别对合成 RIFF buffer 运行检测函数,结果清晰展示了修复效果:WebP 和 AVI 被正确识别,WAV 检测保持不变,未知 RIFF 子类型不再被错误标记为 .wav

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This is a textbook bugfix PR — clear problem, minimal solution, solid tests.

The RIFF container misidentification was a real correctness gap: any WebP or AVI flowing through the magic-byte fallback path (getMimeExtension, extractBase64Binary, saveBinaryResponse) was silently labeled .wav. The fix is the right approach — don't guess from the prefix, read the form tag that actually distinguishes subtypes.

The before/after verification left no ambiguity: WebP went from .wav to .webp, AVI from .wav to .avi, WAV stayed .wav, and unknown RIFF subtypes now return empty instead of a wrong guess. All 37 tests pass including the two new regression tests.

The only note for the author: consider adding a one-liner AVI test (riffBuffer('AVI ').avi) for completeness, since the code handles it but the test suite doesn't cover it. Not blocking.

Approving. ✅

中文说明

这是一个教科书级的 bugfix PR——问题清晰、方案精简、测试扎实。

RIFF 容器误识别是一个真实的正确性缺陷:任何通过 magic-byte 回退路径(getMimeExtensionextractBase64BinarysaveBinaryResponse)的 WebP 或 AVI 数据都会被静默标记为 .wav。修复方案正确——不从前缀猜测,而是读取真正区分子类型的 form tag。

修复前后对比验证结果明确:WebP 从 .wav 变为 .webp,AVI 从 .wav 变为 .avi,WAV 保持 .wav,未知 RIFF 子类型返回空而非错误猜测。全部 37 个测试通过,包括两个新增回归测试。

给作者的一个小建议:考虑补一行 AVI 测试(riffBuffer('AVI ').avi),因为代码已处理但测试未覆盖。不阻塞合并。

批准合并 ✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao

wenshao commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

✅ Maintainer verification — real-file A/B (real helpers) + source-revert mutation

Verified against the PR head in an isolated worktree. Verdict: correct, no regression — recommend merge. The mislabeling is reproduced end-to-end against the real binary-detection helpers on real files, and the fix resolves it.

Note: the desktop @craft-agent/shared package runs its tests with bun test and is not part of the main npm workspace build. Bun isn't installed on this machine, so I executed the new tests' assertions — plus the downstream getMimeExtension / extractBase64Binary paths — directly against the real source via tsx, and did a source‑revert A/B. For a pure byte‑sniffing function this is equivalent to the declared bun test cases.

What the change does

detectExtensionFromMagic now distinguishes RIFF container subtypes by the 4‑char form tag at bytes 8‑11 (WEBP.webp, AVI .avi, WAVE.wav, unknown→''), and the bare RIFF.wav row is removed from MAGIC_SIGNATURES.

Root cause

RIFF is a shared container (WAV/WebP/AVI). Mapping the bare 4‑byte RIFF prefix straight to .wav mislabeled every WebP/AVI binary that flows through the magic‑byte fallback (getMimeExtension on unknown MIME, extractBase64Binary, saveBinaryResponse). Same class as the weixin fix ea97f297d ("confirm the WEBP signature, not just the RIFF prefix"), which this brings to the desktop detector.

Evidence — real-file A/B (run in a real tmux/shell session)

Real inputs: a genuine afconvert WAV, a sips PNG, and hand‑written real RIFF/WEBP, RIFF/AVI and RIFF/RMID (unknown subtype) buffers. The same files were read from disk and pushed through the real helpers; the only change between runs was reverting the one source file.

Input (real file) BASE (main) FIXED (this PR)
real.webp (RIFF/WEBP) .wav .webp
real.avi (RIFF/AVI ) .wav .avi
unknown.riff (RIFF/RMID) .wav '' ✅ (no guess)
real.wav (RIFF/WAVE) .wav .wav ✅ (preserved)
tiny.png (non‑RIFF control) .png .png
end‑to‑end: base64 WebP → extractBase64Binary().ext .wav .webp
end‑to‑end: getMimeExtension(null, WebP) .wav .webp
FIXED: === ALL EXPECTATIONS MET ===
BASE : real.webp -> .wav ❌, real.avi -> .wav ❌, unknown.riff -> .wav ❌,
       base64 WebP -> .wav ❌, getMimeExtension(null,WebP) -> .wav ❌   === 5 MISMATCH(es) ===

Reverting only binary-detection.ts to base flips exactly these 5 outcomes (this doubles as the mutation test — the PR's two new cases, WEBP.webp and WAVE.wav, are both covered and the WebP one fails on base).

Correctness notes

  • WAV is preserved (RIFF/WAVE.wav) and non‑RIFF signatures (PNG/GIF/JPEG/BMP/MP3/OGG/FLAC) are untouched — confirmed by the real PNG control and the WAV case.
  • AVI is matched with its mandatory trailing space (the real AVI fourcc) — the real RIFF/AVI file resolves to .avi.
  • Behavior change for unknown RIFF subtypes: now '' instead of a wrong .wav. Downstream this becomes .bin (extractBase64Binary uses … || '.bin') / a caller‑decided name; saveBinaryResponse only takes the already‑built filename, so no crash. A generic .bin is strictly better than a misleading .wav.
  • A RIFF buffer shorter than 12 bytes can't carry a form tag, so it now returns '' (was .wav) — correct, the subtype is genuinely unknowable.

Verdict

A small, correctly‑scoped fix that stops WebP/AVI binaries from being saved as .wav, aligned with the existing weixin fix, with WAV detection preserved and verified end‑to‑end against the real helpers on real files. Recommend merge.

🇨🇳 中文版(点击展开)

✅ 维护者验证 —— 真实文件 A/B(真实 helper)+ 源码还原变异测试

在隔离 worktree 中对 PR head 验证。结论:正确、无回归 —— 建议合并。 误标问题在真实 binary-detection helper、真实文件上端到端复现,修复后消失。

说明:desktop 的 @craft-agent/shared 包用 bun test 跑测试,且在主 npm workspace 构建里。本机未装 bun,所以我用 tsx 直接对真实源码执行了新测试的断言 —— 外加下游 getMimeExtension / extractBase64Binary 路径 —— 并做了源码还原 A/B。对一个纯字节嗅探函数,这等价于声明的 bun test 用例。

改动做了什么

detectExtensionFromMagic 现在用字节 8‑11 的 4 字符 form tag 区分 RIFF 容器子类型(WEBP.webpAVI .aviWAVE.wav、未知→''),并从 MAGIC_SIGNATURES 删除了裸 RIFF.wav 那一行。

根因

RIFF 是共用容器(WAV/WebP/AVI)。把裸 4 字节 RIFF 前缀直接映射成 .wav,会让所有经过魔数 fallback(MIME 未知时的 getMimeExtensionextractBase64BinarysaveBinaryResponse)的 WebP/AVI 二进制被误标。与 weixin 的 ea97f297d("确认 WEBP 签名,而不只是 RIFF 前缀")同类,本 PR 把它带到 desktop 检测器。

证据 —— 真实文件 A/B(在真实 tmux/shell 会话中运行)

真实输入:afconvert 生成的真 WAV、sips 生成的真 PNG,以及手写的真实 RIFF/WEBP、RIFF/AVI 、RIFF/RMID(未知子类型)。同样的文件从磁盘读取后送进真实 helper;两次运行唯一区别是还原那一个源文件。

输入(真实文件) BASE(main FIXED(本 PR)
real.webp(RIFF/WEBP .wav .webp
real.avi(RIFF/AVI .wav .avi
unknown.riff(RIFF/RMID .wav '' ✅(不猜)
real.wav(RIFF/WAVE .wav .wav ✅(保留)
tiny.png(非 RIFF 对照) .png .png
端到端:base64 WebP → extractBase64Binary().ext .wav .webp
端到端getMimeExtension(null, WebP) .wav .webp
FIXED: === ALL EXPECTATIONS MET ===
BASE : real.webp -> .wav ❌, real.avi -> .wav ❌, unknown.riff -> .wav ❌,
       base64 WebP -> .wav ❌, getMimeExtension(null,WebP) -> .wav ❌   === 5 MISMATCH(es) ===

仅把 binary-detection.ts 还原到 base,正好翻转这 5 个结果(这同时充当变异测试 —— PR 的两个新用例 WEBP.webpWAVE.wav 都被覆盖,其中 WebP 用例在 base 上失败)。

正确性说明

  • WAV 保留(RIFF/WAVE.wav),非 RIFF 签名(PNG/GIF/JPEG/BMP/MP3/OGG/FLAC)不受影响 —— 由真实 PNG 对照和 WAV 用例确认。
  • AVI 按其必需的尾随空格匹配(真实 AVI fourcc)—— 真实 RIFF/AVI 文件解析为 .avi
  • 未知 RIFF 子类型的行为变化:现在返回 '' 而非错误的 .wav。下游会变成 .binextractBase64Binary… || '.bin')/ 由调用方决定的名字;saveBinaryResponse 只接收已构造好的文件名,所以不会崩。通用的 .bin 严格优于误导性的 .wav
  • 短于 12 字节的 RIFF 缓冲无法携带 form tag,所以现在返回 ''(原 .wav)—— 正确,子类型确实无从判断。

结论

一个小而界定正确的修复,阻止 WebP/AVI 二进制被存成 .wav,与既有 weixin 修复对齐,保留 WAV 检测,并在真实 helper、真实文件上端到端验证。建议合并。

Verification method: tsx‑run of the real source helpers (bun not installed) + real‑file A/B (afconvert WAV, sips PNG, hand‑written real RIFF/WEBP·AVI·RMID) through detectExtensionFromMagic / getMimeExtension / extractBase64Binary + source‑revert mutation (base vs PR).

@wenshao
wenshao merged commit 5479b44 into QwenLM:main Jun 18, 2026
25 checks passed
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.

3 participants