Skip to content

Add HTTP parser boundary review gates#1281

Open
bozicovichsantiago20-oss wants to merge 1 commit into
UnitOneAI:mainfrom
bozicovichsantiago20-oss:codex/secure-code-parser-boundary-1174
Open

Add HTTP parser boundary review gates#1281
bozicovichsantiago20-oss wants to merge 1 commit into
UnitOneAI:mainfrom
bozicovichsantiago20-oss:codex/secure-code-parser-boundary-1174

Conversation

@bozicovichsantiago20-oss

Copy link
Copy Markdown

Skill Improvement ($50-150 Bounty)

Skill Modified

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

What Was Wrong

The skill covered common injection, auth, file, SSRF, and deserialization review paths, but it did not give reviewers a concrete gate for HTTP request smuggling / parser-boundary issues. That left two failure modes from #1174 uncovered:

  • missed desync variants involving frontend/backend parser disagreement, CL/TE conflicts, duplicate headers, HTTP/2 downgrade, serverless adapters, and raw-body middleware;
  • false positives where benign header size/timeouts or safe raw-body handling could be labeled as request smuggling without parser mismatch evidence.

Addresses #1174.

What This PR Fixes

  • Bumps secure-code-review to v1.0.1.
  • Adds a focused HTTP parser boundary and request smuggling review section.
  • Adds an evidence table for frontend/backend parser inventory, protocol conversion, boundary header policy, raw-body paths, conflict handling, and route impact.
  • Adds vulnerable/benign examples for Nginx-to-Node transfer framing, JavaScript raw-body middleware, and benign Go HTTP server bounds.
  • Adds a checklist for CL/TE conflicts, duplicate headers, hop-by-hop header stripping, HTTP/2 downgrade, raw-body scoping, and integration tests through the deployed proxy path.
  • Adds Boundary Evidence to finding output and CWE-444 supplemental coverage.
  • Adds primary references for OWASP WSTG request smuggling, CWE-444, RFC 9110, and Node HTTP duplicate header handling.

Evidence

Before (skill misses this / false positive on this):

proxy_http_version 1.1;
proxy_set_header Transfer-Encoding $http_transfer_encoding;
proxy_pass http://node_backend;
srv := &http.Server{
    Addr:              :8443,
    Handler:           app,
    ReadHeaderTimeout: 5 * time.Second,
    MaxHeaderBytes:    1 << 20,
}

The first pattern needs parser-boundary review; the second should not be reported as CWE-444 by itself.

After (now correctly handled):

The skill now requires evidence for frontend component, backend parser, protocol translation, boundary header policy, raw-body path, conflict handling, and route impact before classifying CWE-444.

Test Cases Added/Updated

  • Added vulnerable test cases (tests/vulnerable/)
  • Added benign test cases (tests/benign/)
  • Existing tests still pass

Inline vulnerable and benign examples were added in the skill file. This skill currently has no tests/ directory, so validation was performed against the Markdown skill content and references.

Bounty Tier

  • Minor ($50) - Doc update, small logic tweak, typo fix
  • Moderate ($100) - New edge case coverage, FP reduction with evidence
  • Substantial ($150) - Rewritten detection logic, major coverage expansion

Bounty Info

  • I have read and agree to the CONTRIBUTING.md bounty terms
  • Preferred payment method: PayPal; details can be provided privately after maintainer acceptance.

Validation

  • git diff --check
  • Markdown fence balance: 42 fences, balanced
  • ASCII scan for changed file: OK
  • Required markers present: CWE-444, Boundary Evidence, Content-Length, Transfer-Encoding, HTTP/2-to-HTTP/1, raw-body
  • Prohibited prompt-injection pattern scan per SECURITY.md: OK
  • Reference URL checks returned HTTP 200 for OWASP WSTG, CWE-444, RFC 9110, and Node.js HTTP docs

Tested with: Codex GPT-5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant