Tolerate lone-surrogate escapes in JSON strings#52
Conversation
…pe diagnostics JavaScript's JSON.parse accepts lone surrogate escapes (JS strings are arbitrary UTF-16), but System.Text.Json throws InvalidOperationException when materializing such strings — so payloads the Node service handled fine were turning into 500s (dozens/day in production logs). Route all string extraction through JsVal.TryGetStringLenient, which falls back to unescaping the raw JSON text with JS semantics; Stringify already re-emits lone surrogates as escapes, matching well-formed JSON.stringify (new V8-generated golden vector proves the full hash-parity path). Also include the request path in pipe() diagnostics so the headers-already-sent warnings are attributable to endpoints.
Greptile SummaryThis PR makes JSON string handling tolerate lone-surrogate escapes like JavaScript does. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix: guard the lenient string extraction..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| headers[kv.Key] = kv.Value switch | ||
| { | ||
| JsonValue v when v.TryGetValue<string>(out var s) => s, | ||
| JsonValue v when JsVal.TryGetStringLenient(v, out var s) => s, |
There was a problem hiding this comment.
When an auth config value contains a lone surrogate escape, this branch now decodes it into a raw .NET string and forwards it as an HTTP header value. Header values cannot safely carry isolated UTF-16 surrogates, so the private API request can fail later with malformed or rejected headers instead of rejecting the bad auth configuration at this boundary.
There was a problem hiding this comment.
This value comes from PRIVATE_API_AUTH — operator-controlled deploy configuration (a base64-encoded JSON header map we set ourselves), not client input, so a lone surrogate here means broken config rather than a reachable attack surface. And the failure mode is identical either way: a header value containing a surrogate (or any non-ASCII) makes the HTTP client throw at send time in both runtimes (SocketsHttpHandler rejects non-ASCII header values; Node http throws ERR_INVALID_CHAR), which pipe() turns into the same 500. Rejecting earlier at this boundary would add validation for a config state that fails loudly on the first request regardless.
There was a problem hiding this comment.
That's a fair point. The failure boundary argument doesn't hold here — SocketsHttpHandler throws on send regardless, and since PRIVATE_API_AUTH is operator config, a surrogate there is broken config that surfaces immediately on the first request. Early rejection at this boundary adds no safety net that doesn't already exist. The comment stands withdrawn.
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds lenient UTF-16 JSON string extraction for lone-surrogate escapes, updates JSON and API conversion paths to use it, expands validation/hash vectors, and adds regression coverage. Upstream error logs now include request paths. ChangesLenient JSON string handling
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dotnet/EcencyApi/Handlers/AuthApi.cs (1)
62-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM on the migration — return value is checked and fallbacks are preserved.
JsToStringandJsTemplateare identical to their counterparts inSearchApi.cs(lines 45–63 and 42–43). Consider extracting both to a shared utility (e.g.,JsCoercionsor similar) to eliminate the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dotnet/EcencyApi/Handlers/AuthApi.cs` around lines 62 - 65, Extract the duplicated JsToString and JsTemplate implementations from AuthApi.cs and SearchApi.cs into a shared utility such as JsCoercions. Update both handlers to call the shared methods, preserving the existing string, boolean, JSON fallback, and template coercion behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dotnet/EcencyApi/Infrastructure/Upstream.cs`:
- Around line 269-270: Update the response-writing logic around
JsVal.TryGetStringLenient to check its boolean result before calling
ctx.Response.WriteAsync. Write bodyStr only when conversion succeeds, and handle
the false case using the same established pattern as the other call sites in
this PR, avoiding WriteAsync with a null value.
---
Nitpick comments:
In `@dotnet/EcencyApi/Handlers/AuthApi.cs`:
- Around line 62-65: Extract the duplicated JsToString and JsTemplate
implementations from AuthApi.cs and SearchApi.cs into a shared utility such as
JsCoercions. Update both handlers to call the shared methods, preserving the
existing string, boolean, JSON fallback, and template coercion behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca3fb1ce-439a-4713-922d-0d591d25ba72
📒 Files selected for processing (19)
dotnet/EcencyApi.Tests/JsValTests.csdotnet/EcencyApi.Tests/fixtures/crypto-vectors.jsondotnet/EcencyApi/Handlers/AuthApi.csdotnet/EcencyApi/Handlers/HiveExplorer.csdotnet/EcencyApi/Handlers/PrivateApi.Accounts.csdotnet/EcencyApi/Handlers/PrivateApi.Chain.csdotnet/EcencyApi/Handlers/PrivateApi.Core.csdotnet/EcencyApi/Handlers/PrivateApi.Feeds.csdotnet/EcencyApi/Handlers/PrivateApi.Misc.csdotnet/EcencyApi/Handlers/PrivateApi.Payments.csdotnet/EcencyApi/Handlers/PrivateApi.UserData1.csdotnet/EcencyApi/Handlers/PrivateApi.UserData2.csdotnet/EcencyApi/Handlers/SearchApi.csdotnet/EcencyApi/Infrastructure/ApiClient.csdotnet/EcencyApi/Infrastructure/HttpContextExtensions.csdotnet/EcencyApi/Infrastructure/JsJson.csdotnet/EcencyApi/Infrastructure/JsVal.csdotnet/EcencyApi/Infrastructure/Upstream.csdotnet/tools/gen-vectors.js
1 participant
Day-after production monitoring caught dozens of unhandled
InvalidOperationException: Cannot read incomplete UTF-16 JSON text as string with missing low surrogateper day. Root cause: JavaScript'sJSON.parseaccepts lone surrogate escapes (JS strings are arbitrary UTF-16), but System.Text.Json throws when materializing such strings — so requests the Node service handled fine (typically bot traffic with malformed unicode) were returning 500.JsVal.TryGetStringLenient, which falls back to unescaping the raw JSON text with JS semantics (\uXXXXdecodes to the code unit, paired or not).JsJson.Stringifyalready re-emits lone surrogates as escapes (well-formedJSON.stringifymode), so the signature-hashing path stays byte-exact — proven by a new V8-generated golden vector that runs through the full validateCode re-serialization parity test.pipe()diagnostics now include the request path, making the headers-already-sent warnings attributable to specific endpoints.Verified: 43 tests green; live smoke with lone-surrogate payloads returns the same statuses Node did (401 invalid-token / 200 token-create) with zero unhandled exceptions.
Summary by CodeRabbit
Bug Fixes
Tests