fix(weixin): confirm the WEBP signature, not just the RIFF prefix#5285
Conversation
detectImageMime treated any data starting with the four RIFF bytes as image/webp, but RIFF is a generic container also used by WAV and AVI. A non-WebP RIFF file (e.g. a WAV renamed to .webp) therefore passed the magic-byte check in validateImagePath, which exists to confirm a file's real type. Verify the 'WEBP' marker at bytes 8-11 as well, matching the stricter check already in core's imageTokenizer and the earlier full-signature PNG fix.
| data[1] === 0x49 && | ||
| data[2] === 0x46 && | ||
| data[3] === 0x46 | ||
| data[3] === 0x46 && |
There was a problem hiding this comment.
[Suggestion] The WebP check accesses data[8] through data[11] without an explicit length guard. This works in practice — Buffer[i] returns undefined for out-of-bounds indices, and undefined === 0x57 is false, so short buffers fall through safely. However, this relies on implicit JS behavior rather than an explicit check. A future refactor (e.g., switching to data.readUInt32BE(8)) could silently break this.
Consider adding an explicit guard:
| data[3] === 0x46 && | |
| if ( | |
| data.length >= 12 && | |
| data[0] === 0x52 && | |
| data[1] === 0x49 && | |
| data[2] === 0x46 && | |
| data[3] === 0x46 && | |
| data[8] === 0x57 && | |
| data[9] === 0x45 && | |
| data[10] === 0x42 && | |
| data[11] === 0x50 |
— qwen3.7-max via Qwen Code /review
| 0x45, | ||
| 0x42, | ||
| 0x50, // "WEBP" | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] The old test used a 4-byte RIFF buffer which implicitly covered the short-buffer path. The replacement uses a full 12-byte header, so there's no longer a test asserting that a truncated RIFF buffer (< 12 bytes) falls through gracefully rather than crashing. Consider adding:
it('does not crash on a truncated RIFF buffer (< 12 bytes)', () => {
const buf = Buffer.from([0x52, 0x49, 0x46, 0x46]); // only 4 bytes
expect(() => detectImageMime(buf)).toThrow('Unrecognized image format');
});This locks in the graceful fall-through behavior so future refactors cannot silently break it.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ On direction: solid correctness fix. On approach: scope is tight — two files, minimal diff. The implementation adds the four-byte WEBP check at offset 8-11, mirrors core's approach exactly, and the test coverage is good (WAV misidentification test + strengthened existing WebP test to use a proper 12-byte header). Nothing to cut. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是一个明确的正确性修复。 方案：范围紧凑——两个文件，最小化 diff。实现在偏移 8-11 处添加了四字节 WEBP 校验，与 core 的做法完全一致，测试覆盖良好（WAV 误识别测试 + 将现有 WebP 测试加强为使用完整的 12 字节头部）。无需删减。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent baseline: I'd add the WEBP marker check at offset 8-11, matching how core's Reviewed the diff — no correctness issues, no security concerns, no AGENTS.md violations. The implementation handles short buffers correctly (JavaScript returns Code is clean. No blockers. Test ResultsOn
|
|
Straightforward call. The PR is a textbook minimal fix: one function, four extra byte comparisons, mirrors core's existing approach, and the test suite proves the bug on Approving. ✅ 中文说明判断很直接。这个 PR 是一个教科书级别的最小修复：一个函数，四个额外的字节比较，与 core 现有做法一致，测试在 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Maintainer verification — built-artifact end-to-end A/B + tests + mutationVerified on a clean build of the PR head ( What the change does
Root cause (security relevance)
Evidence
End‑to‑end A/B — I ran the built
Correctness notes
VerdictA small, well‑reasoned security fix (blocks content‑type spoofing of WeChat image uploads), fully covered by a non‑vacuous regression test, and confirmed end‑to‑end against the real built artifact + real files. Recommend merge. 🇨🇳 中文版（点击展开）✅ 维护者验证 —— 构建产物端到端 A/B + 测试 + 变异测试在隔离 worktree 中对 PR head（ 改动做了什么
根因（安全相关）
证据
端到端 A/B —— 我用构建产物
正确性说明
结论一个小而论证充分的安全修复（阻断微信图片上传的内容类型伪造），有非空过场的回归测试完整覆盖，并用真实构建产物 + 真实文件端到端确认。建议合并。 Verification method: worktree build + declared unit test + source‑revert mutation test + end‑to‑end harness importing the built |
3 participants
What this PR does
detectImageMimein the WeChat channel now confirms theWEBPmarker at bytes 8-11 before classifying data asimage/webp, instead of accepting any input that merely begins with the fourRIFFbytes.Why it's needed
detectImageMimeis the magic-byte check insidevalidateImagePath, which exists to confirm that an image file's real content matches its claimed extension before it is uploaded. RIFF is a generic container also used by WAV and AVI, so theRIFFprefix alone does not prove the data is WebP: a non-WebP RIFF file (for example a WAV renamed to.webp) previously returnedimage/webpand so passed the verification. This is the same incomplete-signature problem #2970 fixed for the PNG branch, and core'simageTokenizeralready validates bothRIFFandWEBP, so this brings the weixin channel in line.Reviewer Test Plan
How to verify
A new unit test feeds a
RIFF....WAVEbuffer and assertsdetectImageMimethrowsUnrecognized image format; onmainit wrongly returnsimage/webp. The existing WebP test was updated to use a fullRIFF....WEBPheader (12 bytes) rather than the 4-byteRIFFprefix.Expected: the
detectImageMimesuite passes (6 tests); the new WAV case fails onmainand passes with this change.Evidence (Before & After)
N/A — non-user-visible parsing fix covered by unit tests.
Tested on
Note: three failures in
validateImagePath/sendImagealready reproduce on an unmodifiedmainon Windows (realpath/backslash mocking) and are unrelated to this change; thedetectImageMimesuite is green.Environment (optional)
N/A — unit tests only.
Risk & Scope
RIFF....WEBPheader, so rejecting shorter input is correct.detectImageMimeis touched; other channels and image paths are unchanged.Linked Issues
None.