fix(security): reject ambiguous HTTP request framing - #793
Conversation
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Exact-head local verification completed on 82a9f66: full repository suite 1442 passed in 536.74s (8:56); focused request-framing and related HTTP proof 41 passed; Ruff, compileall, and diff check passed. RFC 9112 request framing is enforced before any body read, including connection close after framing failure. Please use only this exact head for review and Checks. |
|
Current-head security repair: 3651a81. Fixed the valid Devin finding by comparing a normalized decimal Content-Length to the configured limit as strings before int conversion; arbitrary-length digit headers now return 413 through the same connection-close guard. Added a 5000-digit raw-socket regression test. Focused proof: 31 passed; Ruff, compileall, and diff check passed. Review this exact head only. |
|
Current-head verification for : full unit and contract suite passed: 1443 passed in 585.11s. The 5000-digit Content-Length guard is covered before integer conversion, raw-socket framing tests pass, and focused HTTP/security tests previously passed 31. git diff --check, Ruff, and compileall passed. No unresolved current review threads or failed check-runs were found. |
|
Correction: the verified exact HEAD is 3651a81. Full suite result above is for that SHA. |
|
@opencode-agent Review exact current HEAD 3651a81 only. Verify RFC 9112 request framing rejects conflicting or malformed Content-Length/Transfer-Encoding, bounds arbitrary-length decimal values before integer conversion, closes the connection after framing failure, and preserves valid requests. Use same-head Checks and publish a formal verdict. |
|
Exact-head local verification for
The temporary-directory override was required by the host's full data volume; it does not change repository behavior. Remote protected workflows are still queued, so this PR is not merge-ready until same-head Checks and the independent approval gate become terminal. |
|
Exact-head revalidation for PR #793 Verified
@opencode-agent Review only exact current HEAD |
Exact-head validation — PR #793
@opencode-agent please review only exact current HEAD |
Exact-head maintenance evidence
|
opencode-agent
Bot
added
area: api
There was a problem hiding this comment.
🔍 415 content-type rejection does not drain body or close connection
In _read_json the content-type check (server.py) raises 415 before _request_body_size, so on a wrong content-type the declared body is neither read nor drained and close_connection is not set. On an HTTP keep-alive connection those unread body bytes could be interpreted as the start of a subsequent request. This is pre-existing behavior (the original code also checked content-type first) and the default protocol_version is HTTP/1.0 (connections close by default unless the client opts into keep-alive), so impact is limited — but it is a residual gap not covered by this framing hardening PR.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| normalized = value.lstrip("0") or "0" | ||
| maximum = str(max_body_bytes) | ||
| if len(normalized) > len(maximum) or ( | ||
| len(normalized) == len(maximum) and normalized > maximum | ||
| ): | ||
| raise RequestError(413, "request_too_large", "request body exceeds configured limit") | ||
| body_size = int(normalized) | ||
| return body_size |
There was a problem hiding this comment.
📝 Info: Oversized-length check avoids int conversion limit
The check at server.py strips leading zeros, then compares digit-length and lexicographic order against str(max_body_bytes) before int(normalized). Any over-length value is rejected first, so int() never sees a 5000-digit string and cannot hit Python's 4300-digit conversion limit. Lexicographic comparison is valid because both operands are equal-length zero-stripped decimals.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if length_values is None: | ||
| return 0 |
There was a problem hiding this comment.
📝 Info: Missing Content-Length with a present body still returns 0
When neither Transfer-Encoding nor Content-Length is present, _request_body_size returns 0 (server.py), so any body bytes actually sent are left unread. This matches the prior behavior (int(get("content-length", "0"))) and is consistent with the documented boundary, but a body sent without Content-Length on a keep-alive connection would go unconsumed rather than being explicitly rejected.
Was this helpful? React with 👍 or 👎 to provide feedback.
| length_values = get_all("content-length") | ||
| else: # pragma: no cover - production uses email.message.Message headers | ||
| transfer_value = headers.get("transfer-encoding") | ||
| length_value = headers.get("content-length") | ||
| transfer_values = None if transfer_value is None else [transfer_value] |
There was a problem hiding this comment.
📝 Info: Fallback header branch is unreachable but silently narrower
The non-get_all fallback at server.py uses headers.get(), which for email.message.Message-style objects returns only the first occurrence of a duplicated header. If a non-Message header container were ever passed here, duplicate/comma-joined content-length detection would be weaker than the primary path. In production self.headers is an http.client.HTTPMessage (has get_all), so this branch is only exercised by hypothetical callers; not a bug today, but worth noting the divergence in defense strength.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Do not let a peer reuse a connection after an ambiguous frame. | ||
| self.close_connection = True | ||
| raise | ||
| raw = self.rfile.read(body_size) |
There was a problem hiding this comment.
📝 Info: Blocking read when declared body never arrives on a kept-open connection
self.rfile.read(body_size) at server.py will block until body_size bytes arrive or the peer closes. The short-body regression test relies on SHUT_WR to force EOF. A client that declares a large Content-Length but sends fewer bytes without closing will hold the worker thread until socket timeout (Slowloris-style). This is pre-existing behavior (the old code read content-length the same way) and not introduced by this PR, but the framing hardening does not address it.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Merge-gate evidence (2026-08-24): All required checks green on current head except |
1 participant
Buyer-visible outcome
Closes #119. The gateway rejects ambiguous or unsupported HTTP request framing
before reading a JSON body, so malformed clients receive deterministic errors
instead of desynchronizing a keep-alive connection.
Root-cause fix
Review repair
The oversized-digit Content-Length path now returns 413 and closes the
connection. A 5,000-digit raw-socket regression covers the exact desync risk.
Verification
No secret, provider, or COPILOT_GITHUB_TOKEN path changed. Merge only through
normal protected review and terminal Checks.
References: RFC 9112 sections 6.1-6.3, #119.