Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe protobuf decoder now rejects invalid offsets, truncated varints, unsafe integer values, and invalid length-delimited fields. Nested timestamp, reset-token, and response parsing uses validated bounds. Tests cover these failure cases. ChangesProtobuf decoding validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. This PR stays in draft until every box above is ticked. |
리뷰 · 우선순위 61 / 80이 PR은 잘못된 Grok 리셋 쿠폰 응답을 거절하게 바꿉니다. 쿠폰 응답은 프로토버프입니다. 칸마다 "다음은 몇 바이트다"라는 숫자가 앞에 붙습니다. 예전 읽기는 그 숫자를 검사하지 않았습니다. 길이가 남은 바이트보다 크면 잘린 조각만 쿠폰으로 읽었고, 숫자가 버퍼 끝에서 끊기면 덜 읽은 값을 맞는 값처럼 넘겼습니다. 자바스크립트 비트 이동은 32비트만 옮깁니다. 예전 코드는 그 한계에 닿으면 읽기를 멈추고, 남은 바이트를 다음 칸으로 오해할 수 있었습니다.
테스트는 너무 긴 길이와, 한 바이트에서 끊긴 숫자가 예외인지만 봅니다. 본문은 이 파일 테스트 7개와 타입 검사가 통과했다고 적습니다. 베이스는 라인 tests/providers/xai/grok-reset-coupons.test.ts - 테스트 이름은 "파서 진행을 잃지 않는다"입니다. 확인하는 것은 예외뿐입니다. 예외가 나면 그 응답은 통째로 버립니다. 다음 칸으로 이어 가는 동작은 없고, 테스트도 그걸 보지 않습니다. 본문이 말한 "안전한 정수 범위를 넘으면 던진다"도 여기 없습니다. 긴 길이 예시는 약 21억이라, 범위 초과가 아니라 "남은 바이트보다 길다"에만 걸립니다. 라인 src/grok/reset-coupons.ts - 와이어 타입 0과 2는 잘못되면 던집니다. 타입 1(8바이트 고정)과 타입 5(4바이트 고정)는 여전히 반복을 끊고, 그때까지 모은 쿠폰만 돌려줍니다. 그런 칸이 가운데 있으면 뒤 쿠폰이 조용히 빠집니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
추가 리뷰 · 우선순위 58 / 80지난 리뷰 뒤에 테스트만 바뀌었습니다. 커밋은 하나이고, 쿠폰을 읽는 코드는 그대로입니다. 테스트 이름에서 "진행을 잃지 않는다"를 뺐습니다. 확인하는 것은 예외뿐입니다. 이름과 내용이 맞습니다. 담을 수 있는 가장 큰 정수는 통과합니다. 그보다 1 큰 숫자는 라인 src/grok/reset-coupons.ts - 와이어 타입 1과 5는 손대지 않았습니다. 그런 칸이 가운데 있으면 읽기를 멈추고, 그때까지 모은 쿠폰만 돌려줍니다. 본문에는 이 말이 없습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
033d8ad to
ed8a93f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The reset-coupon decoders trusted provider-controlled varints and length prefixes. decodeVarint accumulated with a 32-bit bitwise shift, so a six-byte varint could set the sign bit and return a NEGATIVE length; the length-delimited branches then did offset += bytesRead + len and moved the cursor BACKWARDS, which is a non-terminating loop on a hostile or corrupt gRPC-web body rather than merely a wrong value. A truncated varint returned its partial accumulation, and a declared length past the end silently produced a short subarray. decodeVarint now validates the offset, accumulates by multiplication so the value cannot wrap, throws once a part leaves the safe-integer range, and throws on a varint with no terminator instead of returning a partial value. It also enforces the ten-byte protobuf varint limit explicitly, because the safe-integer guard cannot stand in for a length bound: a continuation byte with no payload bits contributes a part of zero, which is a safe integer, so an arbitrarily long run of 0x80 decoded as a valid zero and an overlong zero length normalized a malformed body into an empty coupon list. The new decodeLength bounds every wire-type-2 field inside its enclosing message, and all three branches route through it. This turns a malformed response into a thrown error where it previously returned partially decoded coupons. Both callers in src/server/management/grok-coupon-routes.ts already wrap getGrokRemainingResets in try/catch and answer 502, and a new case asserts the throw at that boundary rather than only at the decoder, so the endpoint reports the upstream failure instead of acting on a tokenId recovered from garbage. Wire types 1 and 5 still stop iteration rather than throwing; that pre-existing silent drop is unchanged. Carried from #5150. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…rdered freeform wrapper gap (#5203) * fix(responses): avoid spreading stripped tool indices into Math.min preferConfiguredHostedTools collected the index of every stripped additional_tools container into a Set and spread it into Math.min to find the first one. The set size is request-controlled, so a body carrying enough additional_tools containers exceeds the engine argument-count limit and throws RangeError, aborting request normalization before dispatch. Indices arrive in increasing order, so the first stripped index is already the minimum, and a scalar captured during the same map pass replaces the reduction. The regression lives in its own file because tests/responses/openai-responses-passthrough.test.ts sits at its file-size ratchet cap. It covers the Math.min argument count and, added while carrying, the restoration target the original case could not separate: with a non-carrier at index 0 and an unstripped container at index 1, the hosted declaration must land in the container at index 2 and appear exactly once. Carried from #5132. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(grok): reject unsafe protobuf response lengths The reset-coupon decoders trusted provider-controlled varints and length prefixes. decodeVarint accumulated with a 32-bit bitwise shift, so a six-byte varint could set the sign bit and return a NEGATIVE length; the length-delimited branches then did offset += bytesRead + len and moved the cursor BACKWARDS, which is a non-terminating loop on a hostile or corrupt gRPC-web body rather than merely a wrong value. A truncated varint returned its partial accumulation, and a declared length past the end silently produced a short subarray. decodeVarint now validates the offset, accumulates by multiplication so the value cannot wrap, throws once a part leaves the safe-integer range, and throws on a varint with no terminator instead of returning a partial value. It also enforces the ten-byte protobuf varint limit explicitly, because the safe-integer guard cannot stand in for a length bound: a continuation byte with no payload bits contributes a part of zero, which is a safe integer, so an arbitrarily long run of 0x80 decoded as a valid zero and an overlong zero length normalized a malformed body into an empty coupon list. The new decodeLength bounds every wire-type-2 field inside its enclosing message, and all three branches route through it. This turns a malformed response into a thrown error where it previously returned partially decoded coupons. Both callers in src/server/management/grok-coupon-routes.ts already wrap getGrokRemainingResets in try/catch and answer 502, and a new case asserts the throw at that boundary rather than only at the decoder, so the endpoint reports the upstream failure instead of acting on a tokenId recovered from garbage. Wire types 1 and 5 still stop iteration rather than throwing; that pre-existing silent drop is unchanged. Carried from #5150. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(devin): preserve reasoning signature association assistantThinking concatenated every thinking block and independently picked the last available signature, so one block text rode ChatMessagePrompt field 11 paired with a different block signature at field 12. Cognition validates field 12 against the thinking it attests, so that pairing is an invalid replay. The blocks that reach this point are separately signed by construction: src/responses/parser.ts merges CONSECUTIVE unsigned reasoning parts into one, so more than one surviving text-bearing block means more than one real attestation. The pair is now only formed when it is real. Every block with text is still replayed at field 11, and field 12 is attached only when the text replayed IS the text that signature attests, which is the single-block case. Several independently signed blocks send the joined chain unsigned. Keeping only the final block instead would trade an invalid pairing for silently discarding reasoning the turn produced, which the history replay this function exists for cannot afford. A signature-only block, which the parser emits for an encrypted-only reasoning item, contributes neither text nor signature; the turn-dropping guard in mapOneMessage already keyed on reasoning.thinking, so no assistant turn changes its drop decision. The parser also parks a JSON.stringify of the whole reasoning item in the signature field of an UNSIGNED thinking part so the opaque item survives a same-provider round trip. That dump is provider state, not an attestation, and it was reaching field 12 verbatim. isProviderIssuedThinkingSignature now denies exactly that shape, and it lives in src/responses/reasoning-envelope.ts beside the representation it describes rather than as a private copy in one adapter. It stays a deny-list: field 12 is opaque, so an allow-list modelled on the base64 spelling of an Anthropic signature would drop a JWT-shaped or JSON-shaped token the service really issued. A counter-case test pins that those survive. Carried from #5140. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(responses): recognize reordered and escaped freeform wrapper keys The progressive decoder located a wrapper by comparing the buffer against the literal opening for each key it knew. unwrapFreeformToolInput decides the COMPLETED input with JSON.parse, which cares about neither property order nor how a name is spelled, so two spellings of the same wrapper matched nothing at all: {"metadata":1,"input":"cmd"} and a canonical key written with a \\u0069 escape both streamed the raw object as deltas and then completed as cmd. The routed path hid this behind its own hold for unrecognized objects; the direct Responses bridge published the wrapper syntax that completion then removed. This is the third spelling of the disagreement #5047 and #5129 closed for compact and whitespace wrappers. The prefix is now scanned as JSON instead of matched as text, in a new src/responses/freeform-wrapper-scan.ts. It answers only which wrapper the completed text will unwrap to: an own input with a string value streams progressively, because completion gives it precedence over everything else in the object whatever its position; an input with a non-string value, a text that is not an object, and an object JSON.parse can no longer accept publish their own bytes, because that is what completion returns for them; every other object holds until it parses, because a key that has not arrived can still change the answer. Property names are decoded with JSON.parse rather than by hand, since a second decoder beside it is how this defect arose. That last rule narrows the direct bridge and the narrowing is deliberate. A parseable object that is not a wrapper now arrives in one delta when it closes, where it used to stream as it was generated. No prefix of it can be published safely, because input can still follow any property, and routed restoration has held exactly these bodies since #5047 — this is the two paths agreeing rather than a new restriction on one. Bodies that are not objects, which is what an exec program or a patch envelope looks like, are unaffected. structure/transports/responses.md states the cost rather than repeating the old claim that raw input is always progressive, and the bridge test comment that asserted the old timing is corrected. Holding every undecided object subsumes the fallback keys, which only unwrap as the single string field and so are decidable by no prefix. freeformFallbackKeys existed solely to let the streaming side hold them and is removed with its last caller. Containers are walked with an explicit stack, not recursion, because the value being skipped is provider-controlled. Classification is clamped to MAX_FREEFORM_WRAPPER_SCAN_CHARS, and the release parse runs only where the scan actually SAW the object close. Every other hold stays held: an incomplete object has nothing to parse, and re-reading a budget-exhausted buffer on every delta whose last character happens to be a brace is quadratic work for a delta that would arrive in the same instant as the authoritative completion behind it. The previous code parsed the whole buffer on every delta once a fallback wrapper was committed. Duplicate input keys and wrappers that turn invalid after a valid prefix was published remain the same bounded exceptions, now asserted through a reordered wrapper as well. The regression records the deltas a caller would receive and also the values the decoder PROPOSED that do not extend what was already published: both callers drop those, so a test that only mirrored the callers would stay green while the decoder proposed a retraction. It asserts liveness too, so satisfying agreement by holding everything fails, and it pins that an oversized value full of braces publishes nothing. Closes #5151. --------- Co-authored-by: lidge-jun <lidge-jun@users.noreply.github.com> Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
The fix from this PR landed on Adversarial review found that the varint length bound was not actually bounding anything, and the finding is worth passing back. A continuation byte whose payload bits are zero contributes zero, so it never trips the safe-integer guard: twenty The real hazard your change addressed is still the more serious one, and it is now recorded in the commit: the previous 32-bit shift could produce a negative length that moved the cursor backwards, which is a non-terminating loop. Closing this one because its content is on |
2 participants
Motivation
The reset-coupon protobuf decoders trusted provider-controlled varints and length prefixes: a malformed or hostile response could push offsets past the buffer, silently truncate a varint, or overflow a shift into garbage values instead of failing.
Description
decodeVarintvalidates the offset, detects overflow beyond JavaScript''''''''s safe integer range, and throws on a truncated varint rather than returning a partial value.New
decodeLengthbounds every length-delimited field: the declared byte count must remain inside the enclosing message.All three wire-type-2 branches (timestamp, reset token, response) route through the checked helper.
Out of scope: wire types 1 (64-bit) and 5 (32-bit) still stop iteration on malformed fields, so coupons after such a field can be silently dropped.
Tests
bun test tests/providers/xai/grok-reset-coupons.test.ts— 7 pass, including a new case asserting oversized length prefixes and truncated varints throw.bun x tsc --noEmit— clean.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Tests