Skip to content

fix(webhook): cap chunked request bodies - #13336

Closed
sgaofen wants to merge 1 commit into
NousResearch:mainfrom
sgaofen:codex/fix-12543-webhook-chunk-cap
Closed

fix(webhook): cap chunked request bodies#13336
sgaofen wants to merge 1 commit into
NousResearch:mainfrom
sgaofen:codex/fix-12543-webhook-chunk-cap

Conversation

@sgaofen

@sgaofen sgaofen commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #12543.

Root cause:
The webhook adapter only enforced max_body_bytes from request.content_length. Chunked requests without Content-Length were treated as length zero, then request.read() pulled the full body into memory before validation.

Fix summary:

  • Add limited body reading for webhook requests.
  • Keep the existing early Content-Length rejection for known oversized bodies.
  • Stream no-length/chunked bodies with request.content.iter_chunked() and return 413 as soon as the accumulated body exceeds max_body_bytes.
  • Keep a fallback post-read length check for request objects without a stream interface.
  • Add a regression proving an oversized no-length chunked request returns 413 without calling request.read() or dispatching the webhook.

Tests:

  • uv run --frozen --python 3.11 --extra dev pytest -o addopts= tests/gateway/test_webhook_adapter.py -q -> 37 passed
  • git diff --check -- gateway/platforms/webhook.py tests/gateway/test_webhook_adapter.py

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery platform/webhook Webhook / API server labels Apr 22, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to PR #3955 — same webhook chunked body limit fix. Maintainers should pick one.

@sgaofen

sgaofen commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for pointing to #3955. I compared the two patches: #3955 relies on aiohttp client_max_size plus a post-read length check, while this PR enforces max_body_bytes while streaming chunked/no-length bodies and includes a regression that verifies the fallback request.read() path is not used after overflow. If maintainers prefer the older PR I’m fine deferring, but this one is rebased and targets the chunked-body bypass directly.

@egilewski

Copy link
Copy Markdown
Contributor

stale base

This PR has not incorporated the base branch in 60 days. Please rebase or merge if it's still relevant.

Signed: GPT-5.5-low in Codex

@alt-glitch alt-glitch added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jun 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Competing with the earlier open #3955 for the same #12543 chunked-body bypass — #3955 uses aiohttp client_max_size, this PR streams with iter_chunked() and 413s early. Same goal, different mechanism: marked related_to, not a duplicate, for a maintainer to pick.

@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists and removed P1 High — major feature broken, no workaround labels Jun 26, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused chunked-body protection work. This is already implemented on current main by the related salvage.

  • Automated hermes-sweeper review verified gateway/platforms/webhook.py:216 configures aiohttp with client_max_size=self._max_body_bytes, covering chunked requests without Content-Length.
  • gateway/platforms/webhook.py:503 maps aiohttp's overflow exception to HTTP 413, with a post-read byte-length check at gateway/platforms/webhook.py:513.
  • tests/gateway/test_webhook_adapter.py:1164 already exercises an oversized chunked request through TestClient and asserts no webhook dispatch.
  • This shipped in ec29590a0f193590e24012ff6c165de5ba2930d6 via merged PR fix(gateway): enforce body-size limits on chunked requests (salvage #3955 + #3949) #58536 (f23026f9792640fbc7eb0f8656f256bc85d3bc19), which salvaged related PR fix(webhook): enforce chunked body limits #3955.

Closing as implemented on main.

@teknium1 teknium1 closed this Jul 12, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/webhook Webhook / API server sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

webhook: max_body_bytes check is bypassed for chunked requests without Content-Length

4 participants