fix: [mcp-gateway] 修复unla因未按Accept-Encoding解压 HTTP 响应引发的解析乱码问题 #234#302
Merged
iFurySt merged 1 commit intoAmoyLab:mainfrom Mar 5, 2026
Merged
fix: [mcp-gateway] 修复unla因未按Accept-Encoding解压 HTTP 响应引发的解析乱码问题 #234#302iFurySt merged 1 commit intoAmoyLab:mainfrom
iFurySt merged 1 commit intoAmoyLab:mainfrom
Conversation
…gzip,br,zstd,deflate 4种压缩方式的支持
Contributor
Reviewer's GuideAdds transparent decompression of HTTP responses in mcp-gateway’s HTTP tool execution path, supporting gzip, brotli, zstd, and deflate encodings, and introduces tests to validate decoding behavior and error handling. Sequence diagram for HTTP tool response decompression in executeHTTPToolsequenceDiagram
actor User
participant MCPGateway
participant HTTPClient
participant HTTPServer
User->>MCPGateway: Trigger HTTP tool
MCPGateway->>HTTPClient: Send HTTP request
HTTPClient->>HTTPServer: HTTP request
HTTPServer-->>HTTPClient: HTTP response
HTTPClient-->>MCPGateway: *http.Response (possibly compressed)
MCPGateway->>MCPGateway: readDecodedResponseBody(resp)
activate MCPGateway
MCPGateway->>MCPGateway: io.ReadAll(resp.Body)
MCPGateway->>MCPGateway: parseContentEncodings(resp.Header.Content-Encoding)
loop For each encoding in reverse order
MCPGateway->>MCPGateway: decodeBodyBytesByEncoding(body, encoding)
alt Supported encoding (gzip, br, zstd, deflate)
MCPGateway->>MCPGateway: Decode body to plain bytes
else Unsupported encoding
MCPGateway-->>MCPGateway: Error
end
end
MCPGateway-->>MCPGateway: Decoded respBodyBytes
deactivate MCPGateway
MCPGateway->>MCPGateway: resp.Body = io.NopCloser(decodedBody)
MCPGateway->>MCPGateway: resp.Header.Del(Content-Encoding)
MCPGateway->>MCPGateway: resp.Header.Del(Content-Length)
MCPGateway->>User: Return parsed HTTP tool result using decoded body
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
decodeBodyBytesByEncoding, the raw deflate fallback usesflate.NewReader(bytes.NewReader(body))without checking the returned error, which can lead to panics or misleading decode errors if the raw stream is invalid; capture and handle the error similarly to the zlib branch. - For unsupported
Content-Encodingvalues,decodeBodyBytesByEncodingcurrently fails the entire HTTP tool execution; consider falling back to returning the original body with a logged warning to avoid breaking tools on unknown or custom encodings. - You added support for multiple content encodings via
parseContentEncodingsand reverse-order decoding; it would be good to ensure this works for stacked encodings (e.g.,gzip, br) in real-world usage and that the order matches RFC semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `decodeBodyBytesByEncoding`, the raw deflate fallback uses `flate.NewReader(bytes.NewReader(body))` without checking the returned error, which can lead to panics or misleading decode errors if the raw stream is invalid; capture and handle the error similarly to the zlib branch.
- For unsupported `Content-Encoding` values, `decodeBodyBytesByEncoding` currently fails the entire HTTP tool execution; consider falling back to returning the original body with a logged warning to avoid breaking tools on unknown or custom encodings.
- You added support for multiple content encodings via `parseContentEncodings` and reverse-order decoding; it would be good to ensure this works for stacked encodings (e.g., `gzip, br`) in real-world usage and that the order matches RFC semantics.
## Individual Comments
### Comment 1
<location path="internal/core/tool.go" line_range="68-77" />
<code_context>
+ closeFn = gzReader.Close
+ case "br":
+ reader = brotli.NewReader(reader)
+ case "zstd":
+ zstdReader, err := zstd.NewReader(reader)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create zstd reader: %w", err)
+ }
+ reader = zstdReader
+ closeFn = func() error {
+ zstdReader.Close()
+ return nil
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Preserve and surface errors from zstdReader.Close instead of discarding them
In the `zstd` branch, `zstdReader.Close()` is wrapped in a `closeFn` that always returns nil, so any close error is silently dropped while `closeErr` is still being checked by callers. To align with the other codecs and preserve potential failures, you can wire the close error through:
```go
case "zstd":
zstdReader, err := zstd.NewReader(reader)
if err != nil {
return nil, fmt.Errorf("failed to create zstd reader: %w", err)
}
reader = zstdReader
closeFn = func() error {
return zstdReader.Close()
}
```
This keeps error handling consistent and ensures zstd close errors are observable to callers.
```suggestion
case "zstd":
zstdReader, err := zstd.NewReader(reader)
if err != nil {
return nil, fmt.Errorf("failed to create zstd reader: %w", err)
}
reader = zstdReader
closeFn = func() error {
return zstdReader.Close()
}
```
</issue_to_address>
### Comment 2
<location path="internal/core/tool.go" line_range="444-447" />
<code_context>
}
- // Restore response body for further processing
+ // Restore decoded response body for further processing.
resp.Body = io.NopCloser(bytes.NewBuffer(respBodyBytes))
+ resp.Header.Del("Content-Encoding")
+ resp.Header.Del("Content-Length")
// Log response status
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider updating resp.ContentLength alongside the Content-Length header after decoding
Right now the `resp.ContentLength` field still reflects the original (encoded) length, so it can disagree with the decoded body you’ve set on `resp.Body`. Any caller using `resp.ContentLength` will see the wrong value. After decoding, also update the field, e.g.:
```go
resp.ContentLength = int64(len(respBodyBytes)) // or -1 if you want it treated as unknown
```
```suggestion
// Restore decoded response body for further processing.
resp.Body = io.NopCloser(bytes.NewBuffer(respBodyBytes))
resp.ContentLength = int64(len(respBodyBytes))
resp.Header.Del("Content-Encoding")
resp.Header.Del("Content-Length")
```
</issue_to_address>
### Comment 3
<location path="internal/core/execute_tool_test.go" line_range="124-81" />
<code_context>
+ }
+}
+
+func TestReadDecodedResponseBody_GzipInvalidData(t *testing.T) {
+ resp := &http.Response{
+ Header: http.Header{"Content-Encoding": []string{"gzip"}},
+ Body: io.NopCloser(bytes.NewBufferString("not-gzip-data")),
+ }
+
+ _, err := readDecodedResponseBody(resp)
+ assert.Error(t, err)
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for multiple Content-Encoding values and encoding order in readDecodedResponseBody
Since readDecodedResponseBody handles multiple encodings by iterating them in reverse, please add at least one test with a chained Content-Encoding (e.g. `gzip, br` and/or `br, gzip`) to verify that: (1) decoding happens in the correct order, and (2) errors from any step in the chain are surfaced. This will validate the reverse-iteration logic and protect against regressions for multi-encoding responses.
Suggested implementation:
```golang
func TestReadDecodedResponseBody_GzipInvalidData(t *testing.T) {
resp := &http.Response{
Header: http.Header{"Content-Encoding": []string{"gzip"}},
Body: io.NopCloser(bytes.NewBufferString("not-gzip-data")),
}
_, err := readDecodedResponseBody(resp)
assert.Error(t, err)
}
func encodeWithGzipThenZlib(t *testing.T, data []byte) []byte {
var gzipBuf bytes.Buffer
gzipWriter := gzip.NewWriter(&gzipBuf)
_, err := gzipWriter.Write(data)
if err != nil {
t.Fatalf("failed to gzip-compress data: %v", err)
}
if err := gzipWriter.Close(); err != nil {
t.Fatalf("failed to close gzip writer: %v", err)
}
var zlibBuf bytes.Buffer
zlibWriter := zlib.NewWriter(&zlibBuf)
_, err = zlibWriter.Write(gzipBuf.Bytes())
if err != nil {
t.Fatalf("failed to zlib-compress data: %v", err)
}
if err := zlibWriter.Close(); err != nil {
t.Fatalf("failed to close zlib writer: %v", err)
}
return zlibBuf.Bytes()
}
func TestReadDecodedResponseBody_MultiEncoding_GzipThenDeflate_Success(t *testing.T) {
original := []byte(`{"hello":"multi"}`)
encoded := encodeWithGzipThenZlib(t, original)
resp := &http.Response{
Header: http.Header{
// Encodings are listed in the order they were applied.
// Body is zlib(deflate(gzip(original))).
"Content-Encoding": []string{"gzip", "deflate"},
},
Body: io.NopCloser(bytes.NewReader(encoded)),
}
body, err := readDecodedResponseBody(resp)
if assert.NoError(t, err) {
assert.Equal(t, original, body)
}
}
func TestReadDecodedResponseBody_MultiEncoding_ErrorInInnerEncoding(t *testing.T) {
// Construct a body that can be successfully zlib-decoded, but is not valid gzip,
// so that the second decoding step fails.
var zlibBuf bytes.Buffer
zlibWriter := zlib.NewWriter(&zlibBuf)
_, err := zlibWriter.Write([]byte("not-gzip-data"))
if err != nil {
t.Fatalf("failed to zlib-compress data: %v", err)
}
if err := zlibWriter.Close(); err != nil {
t.Fatalf("failed to close zlib writer: %v", err)
}
resp := &http.Response{
Header: http.Header{
// readDecodedResponseBody should decode in reverse order:
// first "deflate" (zlib), then "gzip". The second step should fail.
"Content-Encoding": []string{"gzip", "deflate"},
},
Body: io.NopCloser(bytes.NewReader(zlibBuf.Bytes())),
}
_, err = readDecodedResponseBody(resp)
assert.Error(t, err)
}
```
These tests assume that `readDecodedResponseBody` treats `"deflate"` as zlib-wrapped data and that it iterates the encodings slice in reverse order, as suggested by your comment. If the implementation instead:
1. Uses `compress/flate` directly for `"deflate"`, or
2. Parses a single header value like `"gzip, deflate"` rather than `[]string{"gzip", "deflate"}`,
you should adjust:
- The encoder (`encodeWithGzipThenZlib`) to match the actual `"deflate"` implementation (e.g., using `flate.NewWriter` instead of `zlib.NewWriter`), and/or
- The `Content-Encoding` header construction to mirror how your code reads/normalizes multiple encodings (e.g., `Header: http.Header{"Content-Encoding": []string{"gzip, deflate"}}`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+68
to
+77
| case "zstd": | ||
| zstdReader, err := zstd.NewReader(reader) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create zstd reader: %w", err) | ||
| } | ||
| reader = zstdReader | ||
| closeFn = func() error { | ||
| zstdReader.Close() | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Preserve and surface errors from zstdReader.Close instead of discarding them
In the zstd branch, zstdReader.Close() is wrapped in a closeFn that always returns nil, so any close error is silently dropped while closeErr is still being checked by callers. To align with the other codecs and preserve potential failures, you can wire the close error through:
case "zstd":
zstdReader, err := zstd.NewReader(reader)
if err != nil {
return nil, fmt.Errorf("failed to create zstd reader: %w", err)
}
reader = zstdReader
closeFn = func() error {
return zstdReader.Close()
}This keeps error handling consistent and ensures zstd close errors are observable to callers.
Suggested change
| case "zstd": | |
| zstdReader, err := zstd.NewReader(reader) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create zstd reader: %w", err) | |
| } | |
| reader = zstdReader | |
| closeFn = func() error { | |
| zstdReader.Close() | |
| return nil | |
| } | |
| case "zstd": | |
| zstdReader, err := zstd.NewReader(reader) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create zstd reader: %w", err) | |
| } | |
| reader = zstdReader | |
| closeFn = func() error { | |
| return zstdReader.Close() | |
| } |
Comment on lines
+444
to
+447
| // Restore decoded response body for further processing. | ||
| resp.Body = io.NopCloser(bytes.NewBuffer(respBodyBytes)) | ||
| resp.Header.Del("Content-Encoding") | ||
| resp.Header.Del("Content-Length") |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider updating resp.ContentLength alongside the Content-Length header after decoding
Right now the resp.ContentLength field still reflects the original (encoded) length, so it can disagree with the decoded body you’ve set on resp.Body. Any caller using resp.ContentLength will see the wrong value. After decoding, also update the field, e.g.:
resp.ContentLength = int64(len(respBodyBytes)) // or -1 if you want it treated as unknown
Suggested change
| // Restore decoded response body for further processing. | |
| resp.Body = io.NopCloser(bytes.NewBuffer(respBodyBytes)) | |
| resp.Header.Del("Content-Encoding") | |
| resp.Header.Del("Content-Length") | |
| // Restore decoded response body for further processing. | |
| resp.Body = io.NopCloser(bytes.NewBuffer(respBodyBytes)) | |
| resp.ContentLength = int64(len(respBodyBytes)) | |
| resp.Header.Del("Content-Encoding") | |
| resp.Header.Del("Content-Length") |
| @@ -74,3 +81,166 @@ func TestExecuteHTTPTool_ForwardHeadersAndRequestError(t *testing.T) { | |||
| assert.Error(t, err) | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for multiple Content-Encoding values and encoding order in readDecodedResponseBody
Since readDecodedResponseBody handles multiple encodings by iterating them in reverse, please add at least one test with a chained Content-Encoding (e.g. gzip, br and/or br, gzip) to verify that: (1) decoding happens in the correct order, and (2) errors from any step in the chain are surfaced. This will validate the reverse-iteration logic and protect against regressions for multi-encoding responses.
Suggested implementation:
func TestReadDecodedResponseBody_GzipInvalidData(t *testing.T) {
resp := &http.Response{
Header: http.Header{"Content-Encoding": []string{"gzip"}},
Body: io.NopCloser(bytes.NewBufferString("not-gzip-data")),
}
_, err := readDecodedResponseBody(resp)
assert.Error(t, err)
}
func encodeWithGzipThenZlib(t *testing.T, data []byte) []byte {
var gzipBuf bytes.Buffer
gzipWriter := gzip.NewWriter(&gzipBuf)
_, err := gzipWriter.Write(data)
if err != nil {
t.Fatalf("failed to gzip-compress data: %v", err)
}
if err := gzipWriter.Close(); err != nil {
t.Fatalf("failed to close gzip writer: %v", err)
}
var zlibBuf bytes.Buffer
zlibWriter := zlib.NewWriter(&zlibBuf)
_, err = zlibWriter.Write(gzipBuf.Bytes())
if err != nil {
t.Fatalf("failed to zlib-compress data: %v", err)
}
if err := zlibWriter.Close(); err != nil {
t.Fatalf("failed to close zlib writer: %v", err)
}
return zlibBuf.Bytes()
}
func TestReadDecodedResponseBody_MultiEncoding_GzipThenDeflate_Success(t *testing.T) {
original := []byte(`{"hello":"multi"}`)
encoded := encodeWithGzipThenZlib(t, original)
resp := &http.Response{
Header: http.Header{
// Encodings are listed in the order they were applied.
// Body is zlib(deflate(gzip(original))).
"Content-Encoding": []string{"gzip", "deflate"},
},
Body: io.NopCloser(bytes.NewReader(encoded)),
}
body, err := readDecodedResponseBody(resp)
if assert.NoError(t, err) {
assert.Equal(t, original, body)
}
}
func TestReadDecodedResponseBody_MultiEncoding_ErrorInInnerEncoding(t *testing.T) {
// Construct a body that can be successfully zlib-decoded, but is not valid gzip,
// so that the second decoding step fails.
var zlibBuf bytes.Buffer
zlibWriter := zlib.NewWriter(&zlibBuf)
_, err := zlibWriter.Write([]byte("not-gzip-data"))
if err != nil {
t.Fatalf("failed to zlib-compress data: %v", err)
}
if err := zlibWriter.Close(); err != nil {
t.Fatalf("failed to close zlib writer: %v", err)
}
resp := &http.Response{
Header: http.Header{
// readDecodedResponseBody should decode in reverse order:
// first "deflate" (zlib), then "gzip". The second step should fail.
"Content-Encoding": []string{"gzip", "deflate"},
},
Body: io.NopCloser(bytes.NewReader(zlibBuf.Bytes())),
}
_, err = readDecodedResponseBody(resp)
assert.Error(t, err)
}These tests assume that readDecodedResponseBody treats "deflate" as zlib-wrapped data and that it iterates the encodings slice in reverse order, as suggested by your comment. If the implementation instead:
- Uses
compress/flatedirectly for"deflate", or - Parses a single header value like
"gzip, deflate"rather than[]string{"gzip", "deflate"},
you should adjust:
- The encoder (
encodeWithGzipThenZlib) to match the actual"deflate"implementation (e.g., usingflate.NewWriterinstead ofzlib.NewWriter), and/or - The
Content-Encodingheader construction to mirror how your code reads/normalizes multiple encodings (e.g.,Header: http.Header{"Content-Encoding": []string{"gzip, deflate"}}).
Author
|
@iFurySt @LeoLiuYan 作者你好，麻烦有空审批一下此PR，我看微信交流群有不少人遇到这个bug了，谢谢！ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
修复说明
Fixes #234
修复逻辑
测试验证
测试用例验证 执行 go test -v ./internal/core/... 已通过测试
全量测试验证 执行 make test 已通过
流程验证 基于修改后的代码重新打包一个新的mcp-gateway镜像，在对应web ui的页面上的配置有压缩方式的http接口，不需要手动配置“Accept-Encoding: identity” 响应头也可以成功解析有压缩格式的响应
Summary by Sourcery
Handle HTTP tool responses that use compressed encodings and ensure downstream consumers always see decoded bodies.
Bug Fixes:
Enhancements:
Build:
Tests: