[REVIEW] secure-code-review: add HTTP request smuggling and parser-boundary gates

[stmr]

Skill Being Reviewed

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

False Positive Analysis

Benign code that can be misclassified:

srv := &http.Server{
    Addr:              ":8443",
    Handler:           app,
    ReadHeaderTimeout: 5 * time.Second,
    MaxHeaderBytes:    1 << 20,
}

Why this is a false positive:
Large header limits are not automatically request-smuggling risk when the runtime parser is strict, timeouts are bounded, and the service is not behind a proxy with conflicting parsing behavior. The skill should avoid flagging size limits alone without proxy/backend parser mismatch evidence.

Coverage Gaps

Missed variant 1: proxy/backend disagreement on forwarded requests

proxy_http_version 1.1;
proxy_set_header Transfer-Encoding $http_transfer_encoding;
proxy_pass http://node_backend;

Why it should be caught:
Forwarding attacker-controlled Transfer-Encoding can create parser-boundary ambiguity between edge proxy and backend. The review should require normalization or rejection of conflicting Content-Length and Transfer-Encoding headers.

Missed variant 2: custom middleware reads raw body before framework parser

app.use((req, res, next) => {
  req.raw = [];
  req.on("data", chunk => req.raw.push(chunk));
  next();
});
app.use(express.json());

Why it should be caught:
Raw body consumption before the framework parser can desynchronize body parsing, signature verification, and downstream request handling. This is adjacent to request smuggling and parser confusion, especially behind proxies or serverless adapters.

Edge Cases

• HTTP/2-to-HTTP/1 downgrade at a load balancer can introduce parser differences not visible in app code.
• Serverless adapters can rewrite headers and body encoding before framework middleware runs.
• Duplicate headers may be merged differently by proxies, frameworks, and language runtimes.
• Webhook signature verification often needs raw body access, but must not consume or mutate request streams unexpectedly.

Remediation Quality

• Fix resolves the vulnerability
• Fix doesn't introduce new security issues
• Fix doesn't break functionality
• Issues found: The secure-code-review skill covers injection, auth, session, and access control well, but lacks a parser-boundary checklist. Remediation should focus on single trusted parser, proxy normalization, duplicate-header rejection, CL/TE conflict rejection, bounded timeouts, and integration tests through the deployed proxy path.

Comparison to Other Tools

Tool	Catches this?	Notes
Semgrep	Partial	Can match risky middleware/proxy snippets but cannot prove parser mismatch alone.
CodeQL	Partial	Can reason about some raw-body flows, but proxy/backend desync needs configuration evidence.
Other: Burp/ZAP	Partial	Dynamic tools can test smuggling, but code review should identify parser-boundary prerequisites.

Overall Assessment

Strengths:
Strong ASVS/CWE-oriented review flow with good examples for injection, auth, sessions, and authorization.

Needs improvement:
HTTP parser boundaries and request smuggling are absent, despite being common in proxy-backed web stacks and webhook handlers.

Priority recommendations:

1. Add a request parser-boundary checklist for proxy, load balancer, framework, and serverless adapters.
2. Require explicit CL/TE conflict handling, duplicate-header policy, and HTTP/2 downgrade evidence.
3. Add benign examples for webhook raw-body handling that preserves parser integrity.

Bounty Info

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

[bozicovichsantiago20-oss]

/attempt #1174

I am working on a focused secure-code-review improvement for HTTP request smuggling and parser-boundary evidence.

Planned scope:

• add review gates for proxy/backend parser boundaries, CL/TE conflicts, duplicate headers, HTTP/2 downgrade, serverless adapters, and raw-body middleware;
• distinguish benign bounded server/header settings and safe webhook raw-body handling from real desync risk;
• add evidence/output fields and validation references so reviewers can classify these findings consistently.

Bounty target: Improver Moderate if accepted. Payment details can be provided privately after maintainer acceptance.