fix(middleware): detect and handle gzip/zstd automatically and return 415 for invalid encodings - #4936
fix(middleware): detect and handle gzip/zstd automatically and return 415 for invalid encodings#4936nerimoe wants to merge 1 commit into
Conversation
WalkthroughAdds buffered reading and Content-Encoding normalization to DecompressRequestMiddleware, auto-detects gzip/zstd by peeking magic bytes when the header is empty/identity, adjusts decoder paths and close semantics, returns structured JSON errors for invalid encodings, and marks the compress module as a direct go.mod dependency. ChangesRequest Decompression Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/gzip.go (1)
73-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the compressed
Content-Lengthafter swapping in a decompressed body.These branches replace
c.Request.Bodywith a decompressor, butc.Request.ContentLengthand theContent-Lengthheader still describe the compressed payload. Downstream code that trusts those values can under-read the body or forward an invalid length.💡 Suggested fix
origBody := c.Request.Body wrapMaxBytes := func(body io.ReadCloser) io.ReadCloser { return http.MaxBytesReader(c.Writer, body, maxBytes) } + clearContentLength := func() { + c.Request.ContentLength = -1 + c.Request.Header.Del("Content-Length") + } @@ case "gzip": gzipReader, err := gzip.NewReader(br) @@ c.Request.Body = wrapMaxBytes(&readCloser{ Reader: gzipReader, @@ }) + clearContentLength() c.Request.Header.Del("Content-Encoding") case "br": @@ c.Request.Body = wrapMaxBytes(&readCloser{ Reader: reader, @@ }) + clearContentLength() c.Request.Header.Del("Content-Encoding") case "zstd": @@ c.Request.Body = wrapMaxBytes(&readCloser{ Reader: decoder, @@ }) + clearContentLength() c.Request.Header.Del("Content-Encoding")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/gzip.go` around lines 73 - 109, After replacing c.Request.Body with a decompressor (in the gzip, br, and zstd branches using gzipReader, brotli.NewReader, and zstd.NewReader via wrapMaxBytes/readCloser), clear the compressed length so downstream consumers don't trust stale values: delete the "Content-Length" header (c.Request.Header.Del("Content-Length")) and set c.Request.ContentLength = -1 (unknown) immediately after swapping in the decompressed body in each branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware/gzip.go`:
- Around line 91-100: zstd.NewReader(br) defers validation until Read, so create
the decoder as you do (decoder := zstd.NewReader(br)) and then perform an
immediate small validation read from decoder (e.g., try reading a single byte or
call a decoder-specific validate method) and if that read returns an error close
origBody and decoder and call c.AbortWithStatusJSON with the same invalid zstd
response; ensure you also close decoder on success/failure to avoid leaks and
keep the same error message/path used in the current block (origBody.Close(),
c.AbortWithStatusJSON(...)).
- Around line 65-70: Replace the direct c.AbortWithStatusJSON(...) calls in
middleware/gzip.go (the blocks emitting JSON for "invalid gzip body" and the
similar responses at the other two locations) with manual response writing that
uses common.Marshal() to serialize the body, sets the "Content-Type" header to
"application/json", writes the appropriate HTTP status code, writes the
marshaled bytes to c.Writer, and then calls c.Abort(); if common.Marshal()
returns an error, write a 500 status (and optionally an empty body) to fail
safely. Locate the response blocks by the error JSON structure (message/type
keys) and update them to use common.Marshal() for marshaling instead of
AbortWithStatusJSON.
---
Outside diff comments:
In `@middleware/gzip.go`:
- Around line 73-109: After replacing c.Request.Body with a decompressor (in the
gzip, br, and zstd branches using gzipReader, brotli.NewReader, and
zstd.NewReader via wrapMaxBytes/readCloser), clear the compressed length so
downstream consumers don't trust stale values: delete the "Content-Length"
header (c.Request.Header.Del("Content-Length")) and set c.Request.ContentLength
= -1 (unknown) immediately after swapping in the decompressed body in each
branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4556e208-bffc-40ec-9227-4f8ebaa31619
📒 Files selected for processing (2)
go.modmiddleware/gzip.go
… 415 for invalid encodings
|
这个PR什么时候可以合并 现在codex 用了zstd都没法使用 |
|
ping @Calcium-Ion |
1 similar comment
|
ping @Calcium-Ion |
|
@t0ng7u @seefs001 @QuentinHsu 这个PR是否可以合并 或者目前在做什么其他方案吗？codex现在默认都是使用zstd压缩了 如果不自己改源码的话 codex 基本上无法使用 |
…ddleware Closes QuantumNous#6313 Adds a zstd decompression branch to DecompressRequestMiddleware, mirroring the existing gzip/br branches. Also incorporates improvements from PR QuantumNous#4936: - Magic-byte peek auto-detection when Content-Encoding is missing or 'identity' (no false-positives: JSON first byte ≠ 0x1f/0x28) - Content-Length/Content-Encoding cleanup after decompression - OpenAI-style JSON error responses for invalid gzip/zstd and unsupported encodings (415) - bufio.Reader only created when peek is needed (not on explicit headers) 15 testify tests covering: zstd regression, encoding parity, peek detection, identity passthrough, 415 unsupported, 400 invalid gzip, Content-Length cleanup, GET skip, empty body, invalid zstd no-leak, end-to-end JSON unmarshal.
|
Hi @nerimoe, great work on this PR — the magic-byte peek auto-detection, Content-Length cleanup, and 415 for unsupported encodings are all solid improvements over the basic zstd branch. I have a competing PR (#6348) that originally only added the
One design difference I introduced: I am happy to close #6348 in favor of this PR if you would like to incorporate the test suite. Either way, the important thing is getting the fix merged for the users who need it (Codex CLI etc.). Let me know how you would like to proceed. |
…ddleware Closes QuantumNous#6313 Adds a zstd decompression branch to DecompressRequestMiddleware, mirroring the existing gzip/br branches. Also incorporates improvements from PR QuantumNous#4936: - Magic-byte peek auto-detection when Content-Encoding is missing or 'identity' (no false-positives: JSON first byte ≠ 0x1f/0x28) - Content-Length/Content-Encoding cleanup after decompression - OpenAI-style JSON error responses for invalid gzip/zstd and unsupported encodings (415) - bufio.Reader only created when peek is needed (not on explicit headers) 15 testify tests covering: zstd regression, encoding parity, peek detection, identity passthrough, 415 unsupported, 400 invalid gzip, Content-Length cleanup, GET skip, empty body, invalid zstd no-leak, end-to-end JSON unmarshal.
…ddleware Closes QuantumNous#6313 Adds a zstd decompression branch to DecompressRequestMiddleware, mirroring the existing gzip/br branches. Also incorporates improvements from PR QuantumNous#4936 by @nerimoe (with permission — see PR comments): - Magic-byte peek auto-detection when Content-Encoding is missing or 'identity' (no false-positives: JSON first byte ≠ 0x1f/0x28) - Content-Length/Content-Length-header cleanup after decompression - OpenAI-style JSON error responses for invalid gzip/zstd and unsupported encodings (415) - bufio.Reader only created when peek is needed (not on explicit headers) 15 testify tests covering: zstd regression, encoding parity, peek detection, identity passthrough, 415 unsupported, 400 invalid gzip, Content-Length cleanup, GET skip, empty body, invalid zstd no-leak, end-to-end JSON unmarshal. Co-authored-by: nerimoe <i@neri.moe>
如果可以的话还是想挂到这边吧 |
|
官方迟迟没有处理此PR，没办法自己打包了一版，有需要的可以临时替换，会定期同步上游变更 |
|
牛逼 |
6 participants
📝 变更描述 / Description
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit
Bug Fixes
Chores