[REVIEW] secure-code-review: add HTTP request desynchronization evidence gates

[vumgg]

Skill Being Reviewed

Skill name: secure-code-review
Skill path: skills/appsec/secure-code-review/

False Positive Analysis

Benign code/config that should not be flagged:

# Edge and origin reject ambiguous framing and use one canonical request path.
proxy_http_version 1.1;
proxy_set_header Connection "";
proxy_set_header Transfer-Encoding "";
proxy_request_buffering on;

Application evidence:

request_framing:
  duplicate_content_length: reject
  content_length_with_transfer_encoding: reject
  invalid_chunked_encoding: reject
  h2_to_h1_downgrade_tests: passed
  frontend_backend_parser_matrix: verified

Why this is a false positive:

The presence of a reverse proxy, HTTP/1.1 keep-alive, or HTTP/2 downgrade is not itself a vulnerability. A finding should require evidence of ambiguous message framing or inconsistent parsing across hops. If every hop rejects conflicting Content-Length / Transfer-Encoding, malformed chunks, duplicate framing headers, and unsafe downgrade translations, the architecture should not be flagged merely because multiple HTTP components exist.

Coverage Gaps

Missed variant 1: CL.TE / TE.CL disagreement across proxy and backend

POST /api HTTP/1.1
Host: app.example
Content-Length: 6
Transfer-Encoding: chunked

0

G

Why it should be caught:

The current skill reviews generic input validation and injection sinks, but it does not require reviewers to compare how the edge proxy, WAF, load balancer, application server, and framework delimit one request. If one hop trusts Content-Length and another trusts Transfer-Encoding, attacker-controlled bytes can be reinterpreted as a second request, enabling authorization bypass, cache poisoning, or cross-user response confusion.

Missed variant 2: HTTP/2 downgrade and header normalization drift

frontend:
  protocol: h2
  accepts_duplicate_content_length: false
  forwards_to_origin_as: http/1.1
  preserves_unvalidated_transfer_encoding: true
origin:
  protocol: http/1.1
  parser: legacy
  transfer_encoding_precedence: true

Why it should be caught:

An HTTP/2 request may be safe at the client-facing hop but become ambiguous when translated to HTTP/1.1. The skill currently has no gate for downgrade behavior, pseudo-header normalization, duplicate header folding, chunk extensions, or front-end/back-end parser equivalence.

Edge Cases

• Duplicate Content-Length values that are identical may be accepted by one intermediary and rejected or merged by another.
• Obfuscated Transfer-Encoding tokens, whitespace, casing, comma joining, and chunk extensions can trigger parser differences without a literal CL.TE signature.
• 0.CL, client-side desync, pause-based desync, and connection reuse can exist even when a classic two-header payload is rejected.
• A CDN or WAF may normalize a request differently from the load balancer behind it; reviewing only application code produces a false sense of safety.
• Serverless/API-gateway deployments still need evidence for edge-to-runtime translation and connection reuse, even though the developer does not manage the proxy directly.
• WebSocket upgrades, CONNECT, trailers, and request-body streaming need separate treatment so defensive buffering does not break legitimate streaming routes.

Remediation Quality

• Fix resolves the vulnerability
• Fix doesn't introduce new security issues
• Fix doesn't break functionality when streaming/upgrade routes are explicitly tested
• Issues found: The current skill has no remediation guidance for parser disagreement. Add gates that require ambiguous framing to be rejected at the first trusted hop, verify equivalent parsing across every hop, disable unsafe HTTP/2-to-HTTP/1 downgrade behavior, isolate streaming/upgrade exceptions, and regression-test connection reuse with a second victim request.

Suggested evidence fields:

Field	Required evidence
Frontend parser	Product/version and framing rejection policy
Backend parser	Product/version and framing precedence
Protocol translation	H2/H3 to H1 downgrade behavior
Ambiguity tests	CL.TE, TE.CL, duplicate CL, malformed chunks, header normalization
Connection reuse	Proof that ambiguous requests cannot poison the next request
Exception routes	Streaming, WebSocket, upload, and proxy bypass paths

Comparison to Other Tools

Tool	Catches this?	Notes
Semgrep	No	Static source patterns cannot prove how multiple HTTP parsers delimit the same byte stream.
CodeQL	Partial	It may find unsafe header construction or CRLF injection, but not end-to-end parser disagreement across deployed hops.
AWS http-desync-guardian	Partial	It analyzes HTTP requests for desync risk, but the skill still needs architecture/config evidence across non-AWS proxies, CDNs, gateways, and origins.

Overall Assessment

Strengths:

The skill is strong on common code-level input validation, authentication, authorization, cryptography, error handling, dependencies, and secrets. It provides actionable ASVS/CWE mappings and language-specific examples.

Needs improvement:

It treats the application parser as the main interpretation boundary. Modern deployments commonly have several parsers before application code, and HTTP request desynchronization is specifically caused by disagreement between those parsers. A code review can therefore report “input validated” while a proxy/origin pair still permits request smuggling.

Priority recommendations:

1. Add an HTTP message-framing and parser-equivalence gate mapped to CWE-444 and RFC 9112 request-framing requirements.
2. Require deployment evidence for every HTTP hop and protocol translation, not only application framework code.
3. Add vulnerable and benign fixtures covering CL.TE, TE.CL, duplicate Content-Length, HTTP/2 downgrade, and a correctly rejecting multi-hop stack.

References

• CWE-444: Inconsistent Interpretation of HTTP Requests
• RFC 9112, Sections 6.1-6.3: HTTP/1.1 message framing and message body length
• AWS http-desync-guardian: defensive request analysis for HTTP desync risks
• PortSwigger Web Security Academy: HTTP request smuggling and HTTP/2 downgrade classes

Bounty Info

• I have read and agree to the CONTRIBUTING.md bounty terms
• Preferred payment method: PayPal