Skip to content

fix(ingest): bound decompressed request size - #1662

Merged
Makisuo merged 1 commit into
mainfrom
sec/ingest-decoded-limit
Oct 22, 2026
Merged

Makisuo merged 1 commit into
mainfrom
sec/ingest-decoded-limit

Conversation

@Makisuo

@Makisuo Makisuo commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

What changed

  • Gzip bodies are decoded through a byte limit (gunzip_bounded). Past the limit the request gets a 413 (payload_too_large), tagged the same way as an oversized compressed body.
  • New INGEST_MAX_DECODED_BODY_BYTES, default 8x INGEST_MAX_REQUEST_BODY_BYTES (160 MB). Boot fails if it is set below the body limit. Passed through in alchemy.run.ts like the other ingest knobs.
  • Applied to every decode_payload caller (OTLP HTTP, Cloudflare Logpush) and to the inline session replay path. Invalid UTF-8 in a replay chunk keeps the same rejection message as before.
  • gRPC: unchanged. No accept_compressed is configured and tonic keeps its default 4 MB decoding limit.

Verification

  • New unit tests in main.rs: small gzip decodes at exactly the limit, a 4 MiB inflating body is refused with 413 under a 1 MiB limit and accepted when the limit allows it, replay text decoding is bounded and keeps the invalid-data error.
  • Not compiled or run locally; ingest-rust-tests.yml runs them.

Not changed

  • The blob-store replay path (decompressed_len) streams into a sink and does not allocate, so it is left as is.
  • The container still runs as root: the WAL is a host bind mount (/mnt/wal, created root-owned by the EC2 user data) holding root-owned segments, so a non-root user needs a coordinated host-side ownership change first.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

Gzip request bodies are now read through a byte limit. A body that
decompresses past INGEST_MAX_DECODED_BODY_BYTES (default 8x
INGEST_MAX_REQUEST_BODY_BYTES, 160 MB) is refused with a 413, on the
OTLP HTTP, Cloudflare Logpush and inline session replay paths.
@maple-review-bot

maple-review-bot Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Maple review

🟢 Confidence 9/10 · safe to merge
Contained gzip-limit change with correct Take bounding; only the handler-level 413 reason mapping is untested.
1 note · tests partial −1 · risk medium · 2/2 new units observable

Bounds gunzip output behind a new INGEST_MAX_DECODED_BODY_BYTES ceiling (default 8x the body limit) and turns an over-limit body into a 413 payload_too_large on the OTLP, Logpush and inline replay paths. The bounding logic is correct and safe to merge.

  • gunzip_bounded caps decompression with a Take(max+1) and reports TooLarge
  • decode_payload and the inline replay path take the decoded ceiling
  • Over-limit decodes map to 413 payload_too_large instead of a 400 decode
  • Boot fails when the decoded ceiling is set below INGEST_MAX_REQUEST_BODY_BYTES

Before merge

Findings

🔵 Note · F1 · Handler-level 413 reason mapping for over-limit gzip is untested

tests · apps/ingest/src/main.rs:5116-5122

Only decode_payload is tested for the new over-limit case; no test drives an over-limit gzip through handle_signal_inner (and the same mapping in handle_cloudflare_logpush_inner at main.rs:5356), so the branch that keeps this rejection out of Error status and org_data_loss is unverified. Add a case to the existing gzip handler test at main.rs:12080 with a body inflating past the 8 MiB test ceiling, asserting error.status == PAYLOAD_TOO_LARGE and error_kind == "payload_too_large".

Extend `otlp_gate_precedes_decode_and_admits_an_allowed_org` with a gzip body over `config.max_decoded_body_bytes` and assert the 413 and `payload_too_large` reason.
🤖 Prompt to fix this finding with an AI agent
Findings from an automated review of commit 4b22cbc154f7b4da6fc6910e059a1604a2617228. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F1 · Note · tests · apps/ingest/src/main.rs:5116-5122
Handler-level 413 reason mapping for over-limit gzip is untested
Only `decode_payload` is tested for the new over-limit case; no test drives an over-limit gzip through `handle_signal_inner` (and the same mapping in `handle_cloudflare_logpush_inner` at `main.rs:5356`), so the branch that keeps this rejection out of `Error` status and `org_data_loss` is unverified. Add a case to the existing gzip handler test at `main.rs:12080` with a body inflating past the 8 MiB test ceiling, asserting `error.status == PAYLOAD_TOO_LARGE` and `error_kind == "payload_too_large"`.
Suggested fix: Extend `otlp_gate_precedes_decode_and_admits_an_allowed_org` with a gzip body over `config.max_decoded_body_bytes` and assert the 413 and `payload_too_large` reason.
What was checked
  • Bounded decode pinned by the new tests at exactly the limit and at 4 MiB over it (main.rs:10812, main.rs:10827)
  • Uncompressed bodies stay bounded by max_request_body_bytes before decode (main.rs:5060, main.rs:5292)
  • Invalid-UTF-8 replay chunks keep the InvalidData message the old read_to_string raised (main.rs:4440); blob-store path still streams to a sink, so no allocation
Observability coverage: 2 of 2 changes observable
Change Kind Observable Evidence
OTLP HTTP / Cloudflare Logpush gzip decode bound (413 payload_too_large) request error branch yes Stage span records payload_too_large (main.rs:5117-5121); metrics::request_completed carries the returned error_kind label
Inline session-replay chunk gunzip bound request error branch yes 413 surfaces through the replay handler's record_rejection_reason (main.rs:4502)

4b22cbc · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.

@coderabbitai

coderabbitai Bot commented Oct 22, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 764a1d0e-4a79-418f-996e-53ffb269bb9c
📥 Commits

Reviewing files that changed from the base of the PR and between 58be9ff and 4b22cbc.

📒 Files selected for processing: 2
  • apps/ingest/alchemy.run.ts
  • apps/ingest/src/main.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

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

@Makisuo
Makisuo merged commit c98e2e4 into main Oct 22, 2026
22 checks passed
@Makisuo
Makisuo deleted the sec/ingest-decoded-limit branch October 22, 2026 00:56
@maple-review-bot

Copy link
Copy Markdown

Merged with 2 steps from "Before merge" still open. If they are done, tick them on the review comment; if not, now is the time.

  • Env var · Set INGEST_MAX_DECODED_BODY_BYTES in every environment, or confirm its default
  • Infra · Check every environment has what this config now expects

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.

1 participant