[FIX] support OPRF replacements in request URLs too#75
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds byte-level replacement logic for TOPRF parameters across client and server contexts. When TOPRF parameters are rewritten, the code now mutates the underlying byte sequences in client request packets to ensure consistency. This includes a new utility for in-place byte sequence replacement, client-side packet mutation, server-side TOPRF nullifier extraction, and HTTP provider URL reconciliation with TOPRF data. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant HTTPProvider
Client->>Client: User initiates request with TOPRF parameters
Client->>Client: OPRF parameter rewriting triggered
Client->>Client: Replace params in request string
Client->>Client: Mutate client packet bytes<br/>(replaceByteSequence)
Client->>Server: Send encrypted request packet<br/>with mutated bytes
Server->>Server: Decrypt transcript
Server->>Server: Extract TOPRF nullifiers<br/>from ZK reveals
Server->>Server: Store nullifiers in<br/>decrypted transcript
Server->>HTTPProvider: Validate request via<br/>assertValidProviderReceipt
HTTPProvider->>HTTPProvider: matchRedactedStrings fails
HTTPProvider->>HTTPProvider: reconcileToprfInUrl:<br/>compare expectedPath<br/>vs actualUrl
HTTPProvider->>HTTPProvider: Extract original substrings<br/>from nullifiers
HTTPProvider->>HTTPProvider: Mutate receipt packets<br/>(replaceByteSequence)
HTTPProvider->>HTTPProvider: Re-validate with<br/>updated packets
HTTPProvider-->>Server: URL validation result
Server-->>Client: Request accepted/rejected
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/server/utils/assert-valid-claim-request.ts (2)
26-37:⚠️ Potential issue | 🟡 MinorFix ESLint import sort to unblock CI.
Pipeline
GitHub Actions: Testfails withsimple-import-sort/imports.SIGNATURESshould come beforestrToUint8Arrayunder case-insensitive natural ordering.🧹 Proposed fix
import { AttestorError, binaryHashToStr, canonicalStringify, decryptDirect, extractApplicationDataFromTranscript, hashProviderParams, replaceByteSequence, - strToUint8Array, SIGNATURES, + strToUint8Array, verifyZkPacket } from '#src/utils/index.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/utils/assert-valid-claim-request.ts` around lines 26 - 37, Reorder the named imports so they follow simple-import-sort's case-insensitive natural ordering: move SIGNATURES to appear before strToUint8Array in the import list (i.e., ensure SIGNATURES comes earlier than strToUint8Array among the imported symbols such as SIGNATURES, strToUint8Array, replaceByteSequence, etc. in the import from '#src/utils/index.ts') so ESLint rule simple-import-sort/imports passes.
144-167:⚠️ Potential issue | 🟡 MinorSame over-broad-replacement caveat as
reconcileToprfInUrl.
replaceByteSequence(pkt.message, ogBytes, hashBytes)is applied to the entire clientapplDatapacket. IforiginalTextalso appears outside the URL (headers, body, cookies that happen to echo the value), those bytes will also be rewritten and silently alter what downstreamprovider.assertValidProviderReceiptsees. For OPRF-raw (server-revealed plaintext) this is often a real value visible on the wire, so collisions are plausible. Consider scoping to the request line, or requiring the substring to be unique within the client packet before replacing. See the related comment onsrc/providers/http/index.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/utils/assert-valid-claim-request.ts` around lines 144 - 167, The current loop uses replaceByteSequence(pkt.message, ogBytes, hashBytes) across all client applData packets which may accidentally rewrite occurrences of originalText outside the request URL (headers/body/cookies); modify the logic in this block to restrict replacements to the HTTP request-line or only when originalText is uniquely found in pkt.message (e.g., verify the substring appears exactly once and within the request-line region) before calling replaceByteSequence, and ensure info.parameters and params are still updated as done now; reference the variables/functions oprfRawReplacements, originalText/nullifierText, applData, pkt.message, replaceByteSequence and reconcileToprfInUrl when implementing the change.src/client/create-claim.ts (1)
533-550:⚠️ Potential issue | 🟡 MinorVerify byte-length invariant before calling
replaceByteSequence.
ogBytesis produced by round-trippingtoprf.plaintextthroughuint8ArrayToStr→strToUint8Array. Sincetoprf.plaintextis raw TLS message data (binary), it may contain invalid UTF-8 sequences. WhenTextDecoderencounters these, it replaces them with U+FFFD (encoded as 3 bytes in UTF-8), soogBytes.lengthcan differ from bothtoprf.plaintext.lengthandhashBytes.length(which equalstoprf.dataLocation.length). BecausereplaceByteSequencedoes not enforce equal lengths, a mismatch silently corruptspkt.message.Safer approaches: slice the plaintext directly using
toprf.dataLocationbounds to avoid string encoding loss, or explicitly assert thatogBytes.length === hashBytes.lengthbefore callingreplaceByteSequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/create-claim.ts` around lines 533 - 550, The code converts binary TLS data (toprf.plaintext) through uint8ArrayToStr and strToUint8Array which can change byte lengths due to UTF-8 decoding, then calls replaceByteSequence with ogBytes and hashBytes; instead, avoid the string round-trip and operate on the original bytes or validate lengths before replacement: use toprf.plaintext.slice(toprf.dataLocation.start, toprf.dataLocation.end) (or equivalent) to produce ogBytes that match toprf.dataLocation.length, compute hashBytes from binaryHashToStr/profile length, then for each pkt in revealedPackets where pkt.sender === 'client' call replaceByteSequence with the validated same-length byte arrays (or assert ogBytes.length === hashBytes.length and handle mismatch explicitly) to prevent silent corruption of pkt.message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/http/index.ts`:
- Around line 285-293: reconcileToprfInUrl mutates receipt even when it
ultimately fails, so capture the original bytes before calling it and restore or
log them on failure; specifically, before calling
reconcileToprfInUrl(expectedPath, req.url, ctx, receipt) clone the current
receipt (e.g., Buffer.from(receipt) or appropriate copy) and if
reconcileToprfInUrl returns false, either restore receipt from the clone so
downstream code sees the original bytes or include the cloned original in the
error/log (e.g., when logging params.url or throwing the Error) so the
transcript reflects what was actually received; update the code around
matchRedactedStrings, reconcileToprfInUrl, and the error path that now throws to
ensure assertValidProviderReceipt and logTranscript consumers are not given the
mutated receipt.
- Around line 462-504: In reconcileToprfInUrl: the code calls
replaceByteSequence on every client pkt.message using original derived with JS
substring (UTF-16) and strToUint8Array (UTF-8), which can corrupt non-ASCII URLs
and unintentionally replace occurrences outside the request-line; fix by (1)
deriving the original as a byte-slice (use the transcript/packet bytes or a
byte-aware helper instead of actualUrl.substring) so ogBytes length matches
hashBytes, (2) assert ogBytes.length === hashBytes.length (or restrict to ASCII)
before calling replaceByteSequence, and (3) restrict replacements to the
request-line only (e.g., find the first client packet and limit replacement to
bytes up to the first "\r\n") or only apply if the original is unique within
that packet; use identifiers reconcileToprfInUrl, replaceByteSequence,
strToUint8Array, getHttpRequestDataFromTranscript, matchRedactedStrings, and
pkt.message to locate and implement these checks.
In `@src/utils/generics.ts`:
- Around line 82-101: The replaceByteSequence function must enforce the
documented invariant that search and replace are the same length; add a
fail-fast guard at the start of replaceByteSequence that checks if search.length
!== replace.length and throws a descriptive Error (including the mismatched
lengths and the names search/replace) before running the loop so callers cannot
silently corrupt buf when lengths differ.
---
Outside diff comments:
In `@src/client/create-claim.ts`:
- Around line 533-550: The code converts binary TLS data (toprf.plaintext)
through uint8ArrayToStr and strToUint8Array which can change byte lengths due to
UTF-8 decoding, then calls replaceByteSequence with ogBytes and hashBytes;
instead, avoid the string round-trip and operate on the original bytes or
validate lengths before replacement: use
toprf.plaintext.slice(toprf.dataLocation.start, toprf.dataLocation.end) (or
equivalent) to produce ogBytes that match toprf.dataLocation.length, compute
hashBytes from binaryHashToStr/profile length, then for each pkt in
revealedPackets where pkt.sender === 'client' call replaceByteSequence with the
validated same-length byte arrays (or assert ogBytes.length === hashBytes.length
and handle mismatch explicitly) to prevent silent corruption of pkt.message.
In `@src/server/utils/assert-valid-claim-request.ts`:
- Around line 26-37: Reorder the named imports so they follow
simple-import-sort's case-insensitive natural ordering: move SIGNATURES to
appear before strToUint8Array in the import list (i.e., ensure SIGNATURES comes
earlier than strToUint8Array among the imported symbols such as SIGNATURES,
strToUint8Array, replaceByteSequence, etc. in the import from
'#src/utils/index.ts') so ESLint rule simple-import-sort/imports passes.
- Around line 144-167: The current loop uses replaceByteSequence(pkt.message,
ogBytes, hashBytes) across all client applData packets which may accidentally
rewrite occurrences of originalText outside the request URL
(headers/body/cookies); modify the logic in this block to restrict replacements
to the HTTP request-line or only when originalText is uniquely found in
pkt.message (e.g., verify the substring appears exactly once and within the
request-line region) before calling replaceByteSequence, and ensure
info.parameters and params are still updated as done now; reference the
variables/functions oprfRawReplacements, originalText/nullifierText, applData,
pkt.message, replaceByteSequence and reconcileToprfInUrl when implementing the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec9454d5-4003-4d4b-9b2d-8c85cee4a58b
📒 Files selected for processing (6)
src/client/create-claim.tssrc/providers/http/index.tssrc/server/utils/assert-valid-claim-request.tssrc/types/general.tssrc/types/providers.tssrc/utils/generics.ts
1 participant
Summary by CodeRabbit
Release Notes