Skip to content

fix: apply MaxBodySize to the decompressed body, not the compressed stream - #926

Open
haoku123 wants to merge 1 commit into
gocolly:masterfrom
haoku123:fix/max-body-size-after-decompression
Open

fix: apply MaxBodySize to the decompressed body, not the compressed stream#926
haoku123 wants to merge 1 commit into
gocolly:masterfrom
haoku123:fix/max-body-size-after-decompression

Conversation

@haoku123

Copy link
Copy Markdown
Contributor

Description

Fixes #923: MaxBodySize is applied to the compressed response stream before gzip decompression, so a small gzip payload can expand far beyond the limit (CWE-409 decompression bomb).

Root Cause

In http_backend.go, io.LimitReader wraps res.Body before gzip detection. When colly's own gzip branch runs (e.g. .xml.gz responses without Content-Encoding, or a client with compression disabled), io.ReadAll consumes the decompressed stream with no size limit.

Reproduced locally: 2 KiB compressed → 2 MiB decompressed body passed to callbacks with MaxBodySize(1MiB), matching the issue's repro (2072 → 2097152 bytes).

Changes

Keep the existing limit on the raw stream (first line of defense against malformed compression layers) and add a second LimitReader after the gzip branch, so the final body never exceeds MaxBodySize:

if bodySize > 0 {
    bodyReader = io.LimitReader(bodyReader, int64(bodySize))
}
body, err := io.ReadAll(bodyReader)

Verification

  • New test TestMaxBodySizeAfterDecompression serves gzip data on a .xml.gz path (no Content-Encoding header, so Go's Transport does not auto-decompress and colly's gzip branch runs):
    • without the fix: response body = 2097152 bytes, exceeds MaxBodySize 1048576 (FAIL)
    • with the fix: PASS, body truncated to the limit
  • go test ./...: all packages PASS
  • go vet .: clean

@asciimoo

Copy link
Copy Markdown
Member

This PR includes all of PR #925.

Also, the original LimitReader remains before decompression (

colly/http_backend.go

Lines 234 to 268 in 83c334f

var bodyReader io.Reader = res.Body
if bodySize > 0 {
bodyReader = io.LimitReader(bodyReader, int64(bodySize))
}
contentEncoding := strings.ToLower(res.Header.Get("Content-Encoding"))
if !res.Uncompressed && (strings.Contains(contentEncoding, "gzip") || (contentEncoding == "" && strings.Contains(strings.ToLower(res.Header.Get("Content-Type")), "gzip")) || (strings.HasSuffix(strings.ToLower(finalRequest.URL.Path), ".xml.gz") && res.StatusCode >= 200 && res.StatusCode < 300)) {
// Even if URL contains .xml.gz, it doesn't mean that we get gzip
// compressed data back. We might get 404 error page instead,
// for example. So check gzip magic bytes.
bufReader := bufio.NewReader(bodyReader)
bodyReader = bufReader
magic, err := bufReader.Peek(2)
switch err {
case io.EOF:
// less than 2 bytes, do nothing
case nil:
// gzip magic, as specified in RFC 1952
if magic[0] == 0x1f && magic[1] == 0x8b {
bodyReader, err = gzip.NewReader(bufReader)
if err != nil {
return nil, err
}
defer bodyReader.(*gzip.Reader).Close()
}
default:
return nil, err
}
}
if bodySize > 0 {
// Limit the decompressed stream as well: MaxBodySize must apply
// to the final response body, not to the compressed bytes. A
// small gzip payload could otherwise expand past the limit
// after decompression (CWE-409).
bodyReader = io.LimitReader(bodyReader, int64(bodySize))
}
), while
another is added afterward. Consequently, legitimate compressed responses can fail even when their decompressed body is below the configured limit.

@haoku123
haoku123 force-pushed the fix/max-body-size-after-decompression branch from 83c334f to 4c64816 Compare August 14, 2026 10:16
@haoku123

Copy link
Copy Markdown
Contributor Author

Thanks for catching both issues!

  1. Rebased off master — this PR no longer includes the fix: escape multipart field names in PostMultipart to prevent header injection #925 changes (my branch had been created off the multipart branch by mistake).
  2. Removed the pre-decompression LimitReader — MaxBodySize is now applied only after gzip decompression, so it caps the final body without truncating legitimate compressed responses whose decompressed body is under the limit.

Added TestMaxBodySizeAllowsCompressedResponseWithinLimit to cover the regression: an 8-byte body with a 10-byte limit (gzip overhead pushes the wire size above the limit) decompresses correctly. Without the fix this fails with unexpected EOF.

…tream

MaxBodySize was wrapped around res.Body before gzip detection, so the
limit applied to compressed bytes while io.ReadAll consumed the
decompressed stream. This failed to cap decompression bombs (CWE-409)
and truncated legitimate compressed responses whose decompressed body
was under the limit.

Apply a single LimitReader after gzip decompression so MaxBodySize caps
the final body handed to callbacks, never the wire bytes.

Fixes gocolly#923
@haoku123
haoku123 force-pushed the fix/max-body-size-after-decompression branch from 4c64816 to b8fb62f Compare August 14, 2026 14:38
@haoku123

Copy link
Copy Markdown
Contributor Author

Rebased onto latest master after #925 landed — the multipart tests are no longer duplicated in this branch, and only the MaxBodySize changes remain. Local full test suite passes.

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.

【BUG】MaxBodySize is applied before gzip decompression

2 participants