fix(api): reject ambiguous HTTP request framing - #254
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthrough세션 HTTP 구현을 별도 경계 모듈로 이동했습니다. 요청 헤더와 프레이밍을 엄격하게 검증합니다. 전체 요청 읽기에 단일 데드라인을 적용하고 관련 오류 및 응답 reason phrase를 테스트합니다. Changes세션 HTTP 경계
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to This PR strengthens session HTTP framing, but the current implementation can still process truncated or malformed requests, allow slow clients to retain processing capacity, and accept invalid Content-Length syntax. Those concrete security, correctness, and availability risks make the PR unsafe to merge until addressed. Sequence Diagram(s)sequenceDiagram
participant Client
participant TcpListener
participant accept_one_session_http
participant SessionHttpPort
Client->>TcpListener: HTTP 요청 전송
TcpListener->>accept_one_session_http: TCP 연결 수락
accept_one_session_http->>accept_one_session_http: 헤더 및 프레이밍 검증
accept_one_session_http->>SessionHttpPort: 검증된 세션 요청 전달
SessionHttpPort-->>accept_one_session_http: 세션 응답 반환
accept_one_session_http-->>Client: HTTP 응답 전송 및 연결 종료
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head2ba65562be3a36cb8202737c74edab0c3a3d1b68. -
Head SHA:
2ba65562be3a36cb8202737c74edab0c3a3d1b68 -
Workflow run: 32132929403
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: session_http_framing.rs"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: session_http_framing.rs"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (3 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (3 files)"]
R2 --> V2["targeted test run"]
|
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head2ba65562be3a36cb8202737c74edab0c3a3d1b68. -
Head SHA:
2ba65562be3a36cb8202737c74edab0c3a3d1b68 -
Workflow run: 32139440881
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: session_http_framing.rs"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: session_http_framing.rs"]
R2 --> V2["targeted test run"]
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head9bd9f3db47c8577523934d35bf5074a95cd3bfd1. -
Head SHA:
9bd9f3db47c8577523934d35bf5074a95cd3bfd1 -
Workflow run: 32213784052
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (2 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (2 files)"]
R2 --> V2["targeted test run"]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Fixed the current-head line-coverage failure at exact head
This is validation evidence, not an approval. Required remote Runtime CI/coverage/security checks must complete on the new exact head. |
|
@opencode-agent Please review only the current exact head 8ab49c7 against protected main 5544149. The dead legacy framing path was removed and current boundary coverage was added; please review only exact head 8ab49c7 against protected main 5544149, including whether the deletion is complete and the new reason-phrase integration coverage is sufficient. Do not transfer conclusions from superseded heads. |
|
Fresh exact-head validation for |
|
@opencode-agent Please review only exact head |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
Exact-head validation for |
|
@opencode-agent Please review only exact head |
|
Exact-head validation for |
|
@opencode-agent Please review only exact head |
|
@opencode-agent Correction to the review basis: protected is now exact head (it descends from the PR metadata base via merged PR #258). Please review only current PR head against current protected main ; do not rely on the stale base value in earlier comments. |
|
Verified current head |
|
Exact-head follow-up for
Please review current head |
|
@opencode-agent Please review the unchanged exact head |
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headf74c47809adbeec03b052dcd1fbe5778459ab852. -
Head SHA:
f74c47809adbeec03b052dcd1fbe5778459ab852 -
Workflow run: 32700229291
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (3 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (3 files)"]
R2 --> V2["targeted test run"]
1 participant
Why
Protected main accepts session HTTP requests with the first
Content-Lengthit sees and does not rejectTransfer-Encoding. That lets an intermediary and the runtime disagree about where a request ends, creating a request-smuggling/message-framing boundary defect in the newly shipped session transport.The same boundary also previously ignored HTTP field names with whitespace before the colon, such as
Content-Length : 2, because they no longer matched the framing-header name after splitting. A stricter intermediary can reject that syntax while another recipient normalizes it, so malformed field-name aliases must fail before application dispatch rather than being treated as unrelated headers.What
Content-Lengthheader.Transfer-Encodingbecause this listener does not implement transfer-coding/chunk decoding.Content-Length, including equal duplicates, as a deliberately narrower supported grammar.Content-Lengthrequests and GET reload behavior.TDD lineage
f65e58dc537a7c276065395dccb94a765e3aaa45adds behavior-level loopback regressions for Transfer-Encoding, TE+CL, conflicting duplicate CL, and equal duplicate CL before the guard exists.cf1380f520b1b6fc5426b9b87b9d466004510b28adds the framing guard.9a715a3a8094fa459f8e27faaddad759f0e68a47routes the public session HTTP module through that guard.c76d7ff07e94b28a91f0f6d89a2de012bddcc1c2reconciles protected main without force-push.344fcce5d021c89e5c1479cc0d05b899206ccd72adds loopback regressions for whitespace-before-colon and leading-whitespace framing aliases.82121a89291fe0c98ca260ed4ba7c31e777df14benforces the HTTP field-name token grammar before framing-header matching and adds focused helper coverage for valid, empty, whitespace-bearing, and invalid-token names.The RED commits are historical test-first evidence; no claim is made that they were locally executed outside CI. Exact-head CI remains authoritative.
Standard basis
Fielding, R., Nottingham, M., & Reschke, J. (2022). HTTP/1.1 (RFC 9112). RFC Editor. https://doi.org/10.17487/RFC9112
RFC 9112 treats conflicting or invalid HTTP/1.1 framing as a request-smuggling risk and requires request field-name/colon syntax to be parsed fail-closed. This bounded listener intentionally supports only the simpler Content-Length framing subset rather than implementing Transfer-Encoding partially.
Scope
No HTTP/2 implementation, no chunked decoder, no session-authentication redesign, no Keyverse ownership change, and no psychometric/scoring behavior change. Anonymous-session credential-to-resource authority remains on its separate current lane.
Required evidence before merge
Exact-current-head Runtime CI; exact owned statement/branch coverage; rustfmt/clippy/rustdoc; security/SAST/SBOM/provenance; zero valid unresolved findings; and qualifying independent non-author review where required by live policy.
Summary by CodeRabbit
새로운 기능
버그 수정
Content-Length, 추가 데이터, 지원하지 않는 전송 인코딩을 거부합니다.