🛡️ Sentinel: [HIGH] Fix DoS vulnerability in request sanitization#6300
🛡️ Sentinel: [HIGH] Fix DoS vulnerability in request sanitization#6300EffortlessSteven wants to merge 1 commit into
Conversation
The request sanitization middleware relied solely on the `content-length` header to limit payload sizes. This left the server vulnerable to unbounded payload DoS attacks if a request used `transfer-encoding: chunked` without a defined content length. This commit explicitly rejects requests that specify `transfer-encoding: chunked` if they lack a `content-length` header.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a security check to prevent unbounded payload DoS attacks by rejecting chunked transfer encoding requests that lack a content-length header, and documents this in .jules/sentinel.md. However, feedback points out that according to RFC 9112, standard-compliant chunked requests must not contain a Content-Length header, meaning the current implementation would reject all valid chunked requests. It is recommended to either reject all chunked requests outright or enforce a maximum body size limit on the request stream itself.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if !request.headers().contains_key("content-length") | ||
| && let Some(te) = request.headers().get("transfer-encoding") | ||
| && let Ok(te_str) = te.to_str() | ||
| && te_str.to_lowercase().contains("chunked") | ||
| { | ||
| warn!("Rejected chunked request without content-length"); | ||
| return Err(StatusCode::LENGTH_REQUIRED); | ||
| } |
There was a problem hiding this comment.
According to the HTTP/1.1 specification (RFC 9112, Section 6.2), a sender must not send a Content-Length header in any message that contains a Transfer-Encoding header.
By rejecting chunked requests that lack a Content-Length header, this middleware will reject all standard-compliant chunked requests, effectively breaking chunked transfer encoding support entirely.
If the goal is to prevent unbounded payloads:
- If chunked transfer encoding is not supported/allowed: Reject all chunked requests outright, regardless of the presence of a
Content-Lengthheader. - If chunked transfer encoding is supported: Enforce a maximum body size limit on the request stream itself (e.g., using
tower_http::limit::RequestBodyLimitLayeror Axum'sDefaultBodyLimit), rather than relying on headers which can be omitted or spoofed.
If you wish to reject all chunked requests to prevent DoS, you should remove the content-length check.
if let Some(te) = request.headers().get("transfer-encoding")
&& let Ok(te_str) = te.to_str()
&& te_str.to_lowercase().contains("chunked")
{
warn!("Rejected chunked request: chunked transfer encoding is not supported");
return Err(StatusCode::BAD_REQUEST);
}
There was a problem hiding this comment.
Code Review
This pull request documents a vulnerability regarding unbounded payload DoS via chunked transfer encoding and implements a mitigation in the request sanitization middleware by rejecting chunked requests that lack a content-length header. Feedback points out that requiring content-length alongside transfer-encoding: chunked violates the HTTP/1.1 specification and introduces HTTP Request Smuggling risks. It is recommended to either reject all chunked requests outright or enforce a limit on the request body stream itself.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if !request.headers().contains_key("content-length") | ||
| && let Some(te) = request.headers().get("transfer-encoding") | ||
| && let Ok(te_str) = te.to_str() | ||
| && te_str.to_lowercase().contains("chunked") | ||
| { | ||
| warn!("Rejected chunked request without content-length"); | ||
| return Err(StatusCode::LENGTH_REQUIRED); | ||
| } |
There was a problem hiding this comment.
Requiring a content-length header for requests with transfer-encoding: chunked violates the HTTP/1.1 specification (RFC 9112, Section 6.2), which states that a sender MUST NOT send a Content-Length header if Transfer-Encoding is present. Standard-compliant clients sending chunked requests will be broken by this change.
Additionally, allowing chunked requests only when content-length is present introduces a risk of HTTP Request Smuggling, as different proxies/servers handle the conflict between these two headers differently.
Recommended Approaches:
- If chunked requests are not supported: Reject all chunked requests outright without checking for
content-length. - If chunked requests are supported: Enforce a limit on the request body stream itself (e.g., using Axum's
DefaultBodyLimitor wrapping the body in a size-limiting stream likehttp_body_util::Limited) rather than relying on header validation in this middleware.
if let Some(te) = request.headers().get("transfer-encoding")
&& let Ok(te_str) = te.to_str()
&& te_str.to_lowercase().contains("chunked")
{
warn!("Rejected chunked request to prevent unbounded payload DoS");
return Err(StatusCode::BAD_REQUEST);
}1 participant
🚨 Severity: HIGH
💡 Vulnerability: The request sanitization middleware relied on the
content-lengthheader to limit payload sizes but failed to handle requests withtransfer-encoding: chunkedmissing a content length, leaving the server vulnerable to unbounded payload DoS attacks.🎯 Impact: Attackers could send unbounded chunked payloads to exhaust server resources (memory/CPU) and cause a Denial of Service.
🔧 Fix: Explicitly reject requests with
transfer-encoding: chunkedif they lack a defined content length in therequest_sanitization_middleware.✅ Verification: Ran unit tests and validated that the application correctly returns 411 Length Required for such requests.
PR created automatically by Jules for task 9885799808337788363 started by @EffortlessSteven