fix(l7): reject duplicate Content-Length headers to prevent request smuggling (CWE-444)

[latenighthackathon]

Summary

• Both parse_body_length() in rest.rs and try_parse_http_request() in inference.rs silently accepted multiple Content-Length headers, overwriting with the last value
• Per RFC 7230 Section 3.3.3, differing Content-Length values must be rejected to prevent HTTP request smuggling (CWE-444)
• rest.rs: Detect duplicate CL headers with differing values and return Err before forwarding
• inference.rs: Add ParseResult::Invalid variant; detect duplicate CL headers and return Invalid
• proxy.rs: Handle ParseResult::Invalid by logging the reason server-side and sending generic HTTP 400

Additional fixes (review feedback)

• inference.rs: Reject unparseable Content-Length values instead of silently defaulting to 0 via unwrap_or(0)
• rest.rs: Reject unparseable Content-Length values so a valid+invalid duplicate pair cannot bypass the differing-values check
• rest.rs: Fix Transfer-Encoding substring match (.contains("chunked") → split(',').any(...) exact match) to align with inference.rs and prevent false positives on values like chunkedx
• proxy.rs: Return generic "Bad Request" body instead of leaking internal parsing reasons to sandboxed code (consistent with router_error_to_http pattern)

Test plan

• inference.rs: try_parse_http_request returns ParseResult::Invalid for differing CL values
• inference.rs: identical duplicate CL values are accepted
• inference.rs: non-numeric CL values are rejected
• rest.rs: parse_body_length returns Err for differing CL values
• rest.rs: identical duplicate CL values are accepted
• rest.rs: non-numeric CL values are rejected
• rest.rs: valid+invalid CL pair is rejected (bypass regression test)
• rest.rs: TE substring "chunkedx" does not match as chunked
• Run cargo test for the crate
• Send a request with a single Content-Length header — verify it passes through normally
• Send a request with two differing Content-Length headers — verify it is rejected with HTTP 400
• Existing CL+TE rejection test still passes

Closes #637

I have read the DCO document and I hereby sign the DCO.

[github-actions]

All contributors have signed the DCO ✍️ ✅
_{Posted by the DCO Assistant Lite bot.}

[latenighthackathon]

I have read the DCO document and I hereby sign the DCO.

[latenighthackathon]

recheck

[johntmyers]

Thanks for the contribution — this addresses a real smuggling vector and the approach is sound. A few things need attention before this can merge.

Must Fix

1. No tests

This PR ships zero tests for the new behavior. Issue #637 explicitly called out missing test coverage. For security-critical proxy parsing, we need at minimum:

• inference.rs: try_parse_http_request returns ParseResult::Invalid for differing CL values
• inference.rs: identical duplicate CL values are accepted
• rest.rs: parse_body_length returns Err for differing CL values
• rest.rs: identical duplicate CL values are accepted
• proxy.rs: ParseResult::Invalid produces an HTTP 400

2. unwrap_or(0) masks invalid Content-Length values in inference.rs

The existing unwrap_or(0) on the Content-Length parse (now line 137) silently converts non-numeric values like Content-Length: abc to 0. With the new duplicate detection, two such malformed headers both map to 0, pass the equality check, and proceed as if Content-Length: 0 was sent. This should reject unparseable values outright:

if name.eq_ignore_ascii_case("content-length") {
    let new_len: usize = match value.parse() {
        Ok(v) => v,
        Err(_) => return ParseResult::Invalid(
            format!("invalid Content-Length value: {value}")
        ),
    };
    if has_content_length && new_len != content_length {
        return ParseResult::Invalid(format!(
            "duplicate Content-Length headers with differing values ({content_length} vs {new_len})"
        ));
    }
    content_length = new_len;
    has_content_length = true;
}

3. rest.rs duplicate check is bypassed by unparseable second header

The new check only runs inside the let Ok(len) = val.parse::<u64>() guard. If an attacker sends Content-Length: 42\r\nContent-Length: abc\r\n, the second header fails the parse guard, the block is never entered, and cl_value remains Some(42) — duplicate never detected.

4. Don't leak parsing internals in the 400 response body

The ParseResult::Invalid handler sends reason directly in the response body:

let response = format_http_response(400, &[], reason.as_bytes());

This is inconsistent with the rest of the proxy. router_error_to_http (line 1072) is explicitly documented as returning "generic error messages instead of verbatim internal details," and the other 400s in the file use either empty or generic bodies ("Payload Too Large", "Bad Request", "Use CONNECT for HTTPS URLs"). Log the detail server-side, send a generic body:

ParseResult::Invalid(reason) => {
    tracing::warn!(reason = %reason, "rejecting malformed inference request");
    let response = format_http_response(400, &[], b"Bad Request");
    write_all(&mut tls_client, &response).await?;
    return Ok(InferenceOutcome::Denied { reason });
}

Should Fix

5. This doesn't fully close #637

Issue #637 reported three problems:

1. Duplicate Content-Length — addressed here
2. .contains("chunked") substring match on Transfer-Encoding in rest.rs — not addressed
3. Missing test coverage — not addressed

Either address items 2 and 3 in this PR, or change the PR description to not close #637 so the remaining items stay tracked. Note that inference.rs already handles TE correctly with .split(',').any(...) — the inconsistency between the two parsers is itself a potential smuggling vector since they could disagree on whether a request is chunked.

Nit

6. Consider rejecting all duplicate CL headers, not just differing ones

RFC 7230 Section 3.3.3 says a sender MUST NOT generate multiple Content-Length fields. A recipient MAY fold identical values, but a proxy SHOULD reject. Since this is security-critical default-deny proxy code, rejecting any duplicate (regardless of value equality) would be simpler, safer, and eliminate the ambiguity around parse failures mapping to 0. The PR description and test plan currently describe accepting identical duplicates as intentional, so flagging this as a design consideration rather than a requirement.

[latenighthackathon]

Thanks for that feedback @johntmyers ! Just submitted an update that should address those considerations, cheers!

[johntmyers]

Did you do mise run pre-commit? That should hopefully address the formatting issues in the branch checks.

[latenighthackathon]

@johntmyers fixed TY!