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.
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.