fix(security): fail-closed inbound JSON Content-Length framing - #633
fix(security): fail-closed inbound JSON Content-Length framing#633cursor[bot] wants to merge 18 commits into
Conversation
…closed otherwise Chat history: message-level audio and legacy function_call are null/empty omit no-ops; non-empty fail closed with named errors (including tools passthrough). Tip substrate from #577 assistant refusal/annotations honesty. Local full unit: 940 passed.
…ed otherwise OpenAI fine-tune style message weight is not applied on this gateway. Accept null/0/1 as honest no-ops; reject other types and values with invalid_message_weight. Tip substrate from #578. Local full unit: 943 passed.
…ion role Reject unsupported message keys with named unknown_message_fields (not silent strip or tools-passthrough smuggle). Reject legacy function role with invalid_message_role migration to tool. Tip substrate from #579. Local full unit: 947 passed.
OpenAI partial-assistant prefix flag is not applied on this gateway. null/false are honest no-ops; true and non-booleans fail closed with invalid_message_prefix. Tip substrate from #580. Local full unit: 950 passed.
…therwise Named invalid_max_tool_calls on /v1/chat/completions instead of opaque unknown_fields. Aligns with Responses max_tool_calls honesty; gateway has no multi-step tool loop.
…losed otherwise Legacy /v1/completions treated max_tool_calls as unknown_fields. Accept the key for named invalid_max_tool_calls (null/empty/whitespace omit-equivalent), matching chat/Responses honesty so SDKs get a clear migration path.
SDK clients often send include_usage/include_obfuscation as JSON null. Drop null flag values before validation so null (and null+false mixes) match omit / all-false no-ops on chat, Completions, and Responses. True flags remain fail-closed with invalid_stream_options.
…age honesty Null include_usage/include_obfuscation stay omit-equivalent, but unknown stream_options keys no longer become no-ops just because their value is null. Weight, prefix, refusal, annotations, developer role, empty user/system content, and participant name now use the same named errors on the tools passthrough path as on orchestration. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Hoist stream, required model, stream_options, and temperature/top_p range checks before proxy_completion so a tools or response_format body cannot return a billed JSON completion when the SDK asked for SSE, or silently pick a pool model when model is omitted. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Call the orchestration message, max_tokens, attribution, and routing validators before proxy_completion so a tools or response_format body cannot bill a completion with no prompt, crash on a non-object entry, or silently drop unknown spend/routing keys. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Passthrough has no batch job plane. Reject routing.channel=batch and latency_tolerant=true before proxy_completion so a tool-calling body cannot bill a silent sync completion. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Hoist the orchestration request-knob validators before proxy_completion so a tools or response_format body cannot bill a completion that silently drops seed, stop, n>1, logprobs, logit_bias, or out-of-range penalties. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
stream_chat defaulted to 0.2 even after the HTTP path wrote default_temperature from the request. A streamed invoice summary at temperature=0.8 no longer silently falls back to 0.2. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
… proxy SDK optional defaults serialize omitted fields as JSON null. Accepting those keys without popping them is not omit-equivalent: proxy_completion forwards the body and several providers reject a null JSON Schema object. Pop the keys in place, keep non-null wrong types on invalid_tools, and assert omit-real via mock echo on chat and Responses. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
stream_chat honored default_temperature after 18e6263 but still dropped default_top_p, presence_penalty, and frequency_penalty. A streamed invoice summary at top_p=0.1 billed a completion that ignored nucleus sampling. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Reject signed, missing, duplicate, and chunked request framing before rfile.read so Content-Length: -1 cannot become an unbounded read. Invoice HTTP cases and a Content-Length fuzz seam cover the bound. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Buyer next action
Do not merge this PR as the 155-file honesty stack. Keep #633 as the issue #119 framing landing (dcf6afd on 18537ef). On this same branch, reject non-ASCII Content-Length tokens as invalid_content_length before int(), add those cases to the invoice HTTP suite, then wait for an independent human APPROVE plus Full unit / Semgrep / Security job results. After #615/#628 land, cherry-pick the unique commit (plus the ASCII-digit follow-up). Leave stale #121 / #129 unmerged. Do not open a second inbound-framing PR.
Unique slice reviewed
dcf6afd is the right #119 fix: _parse_content_length fail-closes missing / signed / non-decimal / duplicate Content-Length and non-identity Transfer-Encoding before rfile.read, and incomplete bodies become incomplete_body. ASCII invoice cases (Content-Length: -1, 12abc, missing, 413, chunked, valid summarize invoice 4419) were run locally — python3 tests/test_request_framing_http_honesty.py printed ok.
Blocking honesty gap on this slice
str.isdigit() is not RFC 9110 §8.6 DIGIT (%x30-39). Reproduced on this tip:
| Token | isdigit() |
int() |
Today |
|---|---|---|---|
² (latin-1 0xB2, HTTP-reachable) |
True | ValueError |
do_POST maps this to 400 invalid_request with the int() literal, not invalid_content_length |
3² |
True | ValueError |
same |
١٠ / １ |
True | 10 / 1 |
accepted, so a non-ASCII decimal becomes a successful size |
exercise_content_length documents “never raises anything except RequestError”. Hypothesis st.text() can draw these tokens and fail the new property. Require text.isascii() and text.isdigit() (and a short digit-length cap, e.g. len(text) > 20) before int() so the parser stays fail-closed with the published error code.
Also add an HTTP incomplete_body case (declared length longer than the bytes sent, connection closed). That path is new and untested on the wire.
Scope / process
- vs
mainthis PR is 155 files / +25k. That is the #615 tip, not the framing slice. Cherry-pickdcf6afdafter the honesty landing; do not merge this file list. - Reviewer
seonghobaeis already requested. This automation will not self-approve or merge. - Required checks were still queued at review time (Full unit, Semgrep, Security). Queued CI is not a blocker for this comment.
- CodeRabbit CLI 0.7.3 installed here;
coderabbit auth login --agentstayedawaiting_browser_auth/not_authenticated. No CodeRabbit findings for this run.
References
Fielding, R. (Ed.), Nottingham, M. (Ed.), & Reschke, J. (Ed.). (2022). HTTP semantics (RFC 9110). RFC Editor. https://doi.org/10.17487/RFC9110
Fielding, R. (Ed.), Nottingham, M. (Ed.), & Reschke, J. (Ed.). (2022). HTTP/1.1 (RFC 9112). RFC Editor. https://doi.org/10.17487/RFC9112
Sent by Cursor Automation: Fix Issues
| if isinstance(raw, str) and "," in raw: | ||
| raise RequestError(400, "invalid_content_length", "duplicate or ambiguous Content-Length") | ||
| text = str(raw).strip() | ||
| if not text.isdigit(): |
There was a problem hiding this comment.
str.isdigit() is not RFC 9110 DIGIT. On this tip, Content-Length: ² (latin-1 0xB2, HTTP-reachable) and 3² pass isdigit() then raise ValueError from int() — do_POST turns that into 400 invalid_request with the int() literal, not invalid_content_length. Arabic-Indic ١٠ and fullwidth １ are accepted as 10 / 1.
Require text.isascii() and text.isdigit() (and reject len(text) > 20) before int(). The fuzz seam (exercise_content_length) already forbids any exception except RequestError; Hypothesis st.text() can draw these tokens and fail CI.
Next action: keep the change on this branch. Do not open a second inbound-framing PR.
|
|
||
| def test_parse_content_length_rejects_negative_and_signed_forms() -> None: | ||
| """Signed or non-decimal Content-Length must never become read(-1).""" | ||
| for raw in ("-1", "+10", "12abc", "1.5", ""): |
There was a problem hiding this comment.
These cases miss the tokens that break the published contract. Add unit + invoice HTTP cases for ², 3², ١٠, and １ — each must be invalid_content_length (400), never a successful size and never invalid_request from a leaked int() message.
Also add a wire case for the new incomplete_body path: declare Content-Length larger than the bytes sent, then close. Next action: extend this file on #633; do not open a successor framing PR.
![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 headdcf6afd28c078495f5e4a3f7e1cb69046ca0176e. -
Head SHA:
dcf6afd28c078495f5e4a3f7e1cb69046ca0176e -
Workflow run: 32162701530
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
Evidence["OpenCode evidence"] --> Review["Current PR review path"]
Review --> Verify["Required checks"]
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 evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
Evidence["OpenCode evidence"] --> Review["Current PR review path"]
Review --> Verify["Required checks"]
|
2 participants
Buyer next action
Send one unsigned decimal
Content-Lengththat matches the JSON bytes. Do not sendContent-Length: -1,+N, hex, duplicates, orTransfer-Encoding: chunked. Those now fail closed withinvalid_content_length,length_required,request_too_large, orunsupported_transfer_encodingbefore the socket read.A correctly framed invoice-lookup POST still returns
chat.completion.Why this PR exists
Issue #119:
_read_jsonusedint(Content-Length)and only rejectedbody_size > max_body_bytes.Content-Length: -1reachedrfile.read(-1)(read-until-EOF) and could stall past the configured body limit.Unique commit:
dcf6afdon the #615 tip (18537ef). Do not merge this as the honesty-stack file list. Cherry-pickdcf6afdonto the current landing vehicle after #615/#628. Prefer this unique slice over stale #121 / #129 (those target an olderserver.py).Do not open a second inbound-framing PR. Do not fold this into #621 or #628.
Evidence
python3 tests/test_request_framing_http_honesty.py(watched RED ImportError, then GREEN)summarize invoice 4419fuzz.targets.exercise_content_lengthplus Hypothesis propertytest_content_length_framing_never_returns_negativetest_self_check.pytest_paper_contracts.pytest_conventions.pytest_api_contract.pyprintedokDocs
docs/rest_api_design.md,docs/architecture.md,docs/fuzzing.md,docs/papers/README.md, CHANGELOGReferences
Independent non-author APPROVE + Full unit / Semgrep / Security job results still required. This automation will not self-approve or merge.