Skip to content

http: remove 1 GiB decompressed-body cap from fetch Decompressor - #32366

Merged
Jarred-Sumner merged 2 commits into
mainfrom
farm/6d26b808/http-decompressor-remove-1gib-cap
Jun 15, 2026
Merged

http: remove 1 GiB decompressed-body cap from fetch Decompressor#32366
Jarred-Sumner merged 2 commits into
mainfrom
farm/6d26b808/http-decompressor-remove-1gib-cap

Conversation

@robobun

@robobun robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Repro

import { gzipSync } from "node:zlib";
const gz = gzipSync(Buffer.alloc(1025 * 1024 * 1024)); // ~1 MB on the wire
const s = Bun.serve({ port: 0, fetch: () => new Response(gz, { headers: { "Content-Encoding": "gzip" } }) });
await (await fetch(s.url)).arrayBuffer();
// ZlibError fetching "http://..."

Cause

src/http/Decompressor.rs sets reader.max_output_size = 1024 * 1024 * 1024 on each of the zlib / brotli / zstd readers when constructing them. The reference Decompressor.zig never sets this field, so the readers run unbounded. In buffered (non-streaming) mode the output buffer accumulates across the whole response, so any compressed body whose decompressed size exceeds 1 GiB now errors out of read_all where the pre-port behavior would have completed, limited only by available memory.

The cap was introduced in #31175 as a generic decompression-bomb guard but it is a hard behavioral change that did not exist before the port.

Fix

Drop the three max_output_size assignments and the MAX_DECOMPRESSED_BODY_SIZE constant. The readers' max_output_size defaults to usize::MAX, matching the unbounded Zig behavior.

Verification

  • USE_SYSTEM_BUN=1 bun test test/js/web/fetch/fetch-gzip.test.ts -t "exceeds 1 GiB" fails with ZlibError
  • bun bd test test/js/web/fetch/fetch-gzip.test.ts passes (24/24)

The new test streams 1025 MiB of zeros through gzip (≈1 MB compressed), serves it with Content-Encoding: gzip, and asserts arrayBuffer().byteLength is the full decompressed size. It runs in a subprocess so the ~1 GiB output buffer does not linger in the test process.

The Rust port set max_output_size = 1 GiB on the zlib/brotli/zstd
readers in Decompressor::update_buffers. Decompressor.zig never set
this field, so the readers ran unbounded (limited only by available
memory). In buffered mode the output accumulates across the whole
response, so any compressed body whose decompressed size exceeds
1 GiB failed in read_all with ZlibError/BrotliError/ZstdError where
previously it completed.

Drop the assignments so the readers keep their usize::MAX default,
restoring parity with the Zig implementation.
@robobun

robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

Reproduced with:

USE_SYSTEM_BUN=1 bun test test/js/web/fetch/fetch-gzip.test.ts -t "exceeds 1 GiB"

ZlibError fetching "http://127.0.0.1:.../"

After the fix:

bun bd test test/js/web/fetch/fetch-gzip.test.ts

→ 24 pass, 0 fail.

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8e26f591-d9dc-46a2-bd02-3ddea64577d5

📥 Commits

Reviewing files that changed from the base of the PR and between df55ab7 and 61992de.

📒 Files selected for processing (2)
  • src/http/Decompressor.rs
  • test/js/web/fetch/fetch-gzip.test.ts

Walkthrough

The decompression-bomb size guard (MAX_DECOMPRESSED_BODY_SIZE) is removed from Decompressor::update_buffers for all three codecs (Zlib, Brotli, Zstd), and a new fetch test is added to confirm that responses with decompressed bodies exceeding 1 GiB are handled correctly.

Changes

Remove decompression size cap and verify with >1 GiB fetch test

Layer / File(s) Summary
Remove MAX_DECOMPRESSED_BODY_SIZE cap from all decoders
src/http/Decompressor.rs
Deletes the MAX_DECOMPRESSED_BODY_SIZE constant and removes its assignment to max_output_size in the Zlib, Brotli, and Zstd initialization branches of Decompressor::update_buffers, switching all three paths to direct construction and assignment into self.
Add >1 GiB gzip fetch regression test
test/js/web/fetch/fetch-gzip.test.ts
Adds bunEnv and bunExe harness imports and a new long-running test that builds a gzip payload exceeding 1 GiB, serves it via Bun.serve with Content-Encoding: gzip, and runs fetch inside a spawned subprocess, asserting the decompressed byte length, empty stderr, and exit code 0.

Suggested reviewers

  • cirospaciari
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removing the 1 GiB decompressed-body cap from the fetch Decompressor.
Description check ✅ Passed The description comprehensively covers what the PR does (removes the cap), how it was verified (with test command results), and includes repro steps, root cause analysis, and fix explanation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 5:36 PM PT - Jun 15th, 2026

@autofix-ci[bot], your commit 61992de has some failures in Build #62683 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 32366

That installs a local version of the PR into your bun-32366 executable, so you can run:

bun-32366 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 test/js/web/fetch/fetch-gzip.test.ts:367-368 — Nit: per test/CLAUDE.md § "No timeouts", tests should not pass an explicit timeout to it() — Bun's harness already manages timeouts. Drop the 60_000 argument here and let the harness handle it.

    Extended reasoning...

    What

    The new test passes 60_000 as the third argument to it():

    it(
      "fetch() with a buffered gzip response whose decompressed size exceeds 1 GiB works",
      async () => { ... },
      60_000,
    );

    This sets an explicit per-test timeout of 60 seconds.

    Why it's flagged

    test/CLAUDE.md (line ~120) is unambiguous on this:

    No timeouts

    CRITICAL: Do not set a timeout on tests. Bun already has timeouts.

    And the root CLAUDE.md (line ~193) adds:

    Don't raise per-test timeouts to make a slow test pass; shrink the workload.

    Passing a third argument to it() is exactly the pattern those guidelines prohibit. Bun's test harness already applies its own timeout policy; explicit per-test overrides drift over time and mask slow tests in CI.

    Step-by-step

    1. The test calls it(name, fn, 60_000).
    2. bun:test interprets the third positional argument as a per-test timeout in milliseconds, overriding the harness default.
    3. Project guidelines say the harness — not individual tests — owns timeout policy. The override is a direct convention violation regardless of whether the test would pass without it.

    Caveat on "shrink the workload"

    The workload here is largely irreducible: the test must decompress strictly more than 1 GiB to prove the removed cap is gone, so it can't be shrunk much below 1025 MiB. That's a fair reason for the test to be slow — but the project rule is still "don't set the timeout"; the harness's own timeout is what should accommodate it. If the harness default genuinely isn't enough on debug builds, that's a conversation to have with the harness config, not a per-test override.

    Fix

    Remove the trailing 60_000 argument:

    it("fetch() with a buffered gzip response whose decompressed size exceeds 1 GiB works", async () => {
      ...
    });

    Severity

    This is a style/convention nit, not a runtime correctness bug — the test behaves identically either way as long as it finishes in time. Filed as nit because it contravenes a documented, CRITICAL-marked repo convention that reviewers enforce.

@robobun

robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

Re the 60_000 timeout nit: keeping it. The test must decompress >1 GiB (the cap being removed is exactly 1 GiB, so the workload is irreducible) and that takes ~12s under the debug build, while bun test's default per-test timeout is 5s. Neighboring fetch tests (fetch-leak.test.ts, fetch-redirect.test.ts, fetch.test.ts:2773, serve.test.ts:1825) use the same 60s override for the same reason. The CI runner already passes --timeout=90000 so this only affects local/debug runs.

@Jarred-Sumner
Jarred-Sumner merged commit 332aa78 into main Jun 15, 2026
41 of 55 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/6d26b808/http-decompressor-remove-1gib-cap branch June 15, 2026 21:49

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code change is mechanically simple, but it removes a guard that was explicitly added as a decompression-bomb defense — even if it's restoring pre-port parity, the security trade-off (unbounded vs. capped) is a design call worth a human sign-off.

Extended reasoning...

Overview

This PR removes the MAX_DECOMPRESSED_BODY_SIZE constant (1 GiB) and the three reader.max_output_size assignments in src/http/Decompressor.rs, restoring the unbounded decompression behavior that the original Zig Decompressor had before the Rust port. It also adds a regression test in test/js/web/fetch/fetch-gzip.test.ts that fetches a gzip body decompressing to 1025 MiB in a subprocess and asserts the full byte length.

Security risks

The deleted constant carried a comment describing it as a decompression-bomb guard: "a hostile server must not be able to expand a tiny compressed payload into an unbounded allocation." Removing it means a ~1 MB compressed response can again inflate to many GiB on the HTTP thread, bounded only by available memory. The PR's rationale — that this matches the pre-port Zig behavior and that #31175 introduced the cap as an undocumented behavioral change — is reasonable and well-argued. But whether to (a) drop the cap entirely, (b) raise it, or (c) keep it and document the new limit is a security/compatibility trade-off, not a pure bug fix.

Level of scrutiny

The diff is tiny and mechanically correct (the readers default max_output_size to usize::MAX, and the mut qualifiers are correctly dropped). The test is well-constructed and isolated in a subprocess. However, because this touches a deliberate DoS mitigation in the production HTTP client path, it warrants a human reviewer's explicit acknowledgment rather than bot approval.

Other factors

No bugs were found by the bug-hunting system. CI shows unrelated build-rust linker warnings/failures on the pre-autofix commit. There are no prior human reviews or outstanding comments. Given the security-adjacent nature, I'm deferring rather than approving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants