Skip to content

fix(webhook): enforce body-size limit when Content-Length is absent - #56829

Closed
Tamsi wants to merge 1 commit into
NousResearch:mainfrom
Tamsi:fix/webhook-body-size-bypass
Closed

fix(webhook): enforce body-size limit when Content-Length is absent#56829
Tamsi wants to merge 1 commit into
NousResearch:mainfrom
Tamsi:fix/webhook-body-size-bypass

Conversation

@Tamsi

@Tamsi Tamsi commented Jul 2, 2026

Copy link
Copy Markdown

Summary

The generic webhook adapter (gateway/platforms/webhook.py) enforced its per-route max_body_bytes limit only via the Content-Length header:

content_length = request.content_length or 0
if content_length > self._max_body_bytes:
    return web.json_response({"error": "Payload too large"}, status=413)
raw_body = await request.read()   # reads the whole body, no re-check

A request using Transfer-Encoding: chunked (or a spoofed small Content-Length) makes request.content_length None, so None or 0 == 0 and the check is skipped entirely. The full body is then buffered into memory regardless of the route's configured max_body_bytes.

This is reachable by an unauthenticated caller — the body must be read before HMAC signature verification — so it bypasses the documented size guard and is a DoS vector on an internet-exposed webhook endpoint. The module docstring even claims "Body size limits checked before reading payload", which chunked requests defeat.

The sibling adapters Feishu (plugins/platforms/feishu/adapter.py) and WeCom already guard this exact case with a post-read length check and have regression tests for content_length=None — the generic adapter was the odd one out.

Fix

  • Re-check the limit against the bytes actually read (len(raw_body) > self._max_body_bytes → 413), keeping the Content-Length fast path.
  • Update the module docstring to reflect the real guarantee.
  • Add a regression test driving a chunked-style request (Content-Length None) with an over-limit body, asserting 413.

Test plan

  • tests/gateway/test_webhook_adapter.py — 69 passed (68 existing + 1 new)
  • Full webhook suite (test_webhook_adapter, test_webhook_signature_rate_limit, test_webhook_integration, test_webhook_dynamic_routes, test_webhook_deliver_only) — 103 passed
  • Confirmed the new test fails without the fix (returns 202 instead of 413) and passes with it

The generic webhook adapter only checked ``request.content_length`` before
reading the body, so a request using chunked transfer-encoding (or a spoofed
small Content-Length) reported ``content_length is None`` and skipped the
guard entirely. The full body was then buffered regardless of the route's
``max_body_bytes`` — a size-limit bypass reachable by an unauthenticated
caller (the body must be read before HMAC verification), and a DoS vector on
an internet-exposed endpoint.

Re-check the limit against the bytes actually read, mirroring the Feishu and
WeCom webhook adapters (which already guard the missing-Content-Length case
and have regression tests for it). The module docstring is updated to match
the real guarantee.

Adds a regression test that drives a chunked-style request (Content-Length
None) with an over-limit body and asserts 413.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery platform/webhook Webhook / API server sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P2 Medium — degraded but workaround exists labels Jul 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Competing-PR cluster for the same generic-webhook chunked-body bypass: this PR (post-read len(raw_body) re-check), open #3955 (earliest/canonical — aiohttp client_max_size + post-read check, different mechanism), and open #13336 (streams via iter_chunked(), 413 once the accumulated body exceeds the limit). Same goal, different mechanisms → related, not duplicate. #3955 is the earliest/canonical entry; a maintainer should pick one. Per-adapter siblings that fix the same class elsewhere: #25296 (MS Graph), #54935 (Feishu), #54930 (LINE).

@teknium1

teknium1 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Thanks @Tamsi — reviewed against current origin/main: this was fixed by PR #58536 (commit ec29590, merged 2026-07-04), which enforces the cap on the actual bytes read plus sets client_max_size on the aiohttp Application, covering both chunked and spoofed-Content-Length requests with regression tests. Closing as already fixed on main. Thanks for the correct diagnosis — if you see any remaining bypass on current main, a fresh report is welcome.

@teknium1 teknium1 closed this Jul 5, 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:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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.

3 participants