fix(api): truncate primary-RPC error before logging it [BUG-007] - #216
fix(api): truncate primary-RPC error before logging it [BUG-007]#216Morenikeoa wants to merge 1 commit into
Conversation
withRpcFallback logged primaryErr.message raw via logger.warn — the one call site in this codebase that didn't wrap an error message with truncateErrorMessage() before logging it (every other site already does: health.ts, markets.ts, etc.). Solana RPC connection errors can embed the full endpoint URL in .message, and per .env.example, paid RPC providers (Helius/Alchemy) embed an API key directly in that URL. truncateErrorMessage only bounds length — it doesn't redact secrets — so this doesn't fully eliminate the exposure (a short enough message could still leak a key even truncated), but it brings this site in line with the same mitigation already applied everywhere else in the codebase instead of being the one outlier with a strictly larger exposure window. Added tests covering the no-fallback-configured passthrough, the normal fallback path, and the truncation itself. Verified the truncation test fails against the pre-fix code (273-char raw message logged) and passes against the fix (bounded to 120 chars + "..."). Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@Princessdada is attempting to deploy a commit to the Khubair Nasir's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
More reviews will be available in 36 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
|
Independent verification — not an approval (QA/Security own that). The fix works and the test binds. But the PR's own comment names a leak that the same package already has a helper for. VerificationBranch is current with main (0 behind), tests pass (3), and neutralising the truncation: error: truncateErrorMessage(msg, 120) → error: msgBinds cleanly to the change. The bit worth acting onCredit for the comment — it's unusually honest about its own limits:
That's exactly right. And /**
* Mask API keys in URLs and connection strings before logging
* Handles patterns like:
* - https://devnet.helius-rpc.com/?api-key=YOUR_KEY
* - http://localhost:8899?api_key=SECRET
* - rpc_url=secret_key_here
*/
export declare function maskApiKeys(input: string): string;I ran it against a realistic Helius failure rather than assuming: The message is 89 chars — under the 120 limit — so truncation does nothing at all to the case the comment is worried about. The key ships to the logs intact. Suggested one-line change: error: truncateErrorMessage(
maskApiKeys(primaryErr instanceof Error ? primaryErr.message : String(primaryErr)),
120,
),Order matters: mask first, then truncate. Truncating first can cut mid-key and leave a partial secret in the log. Scope call — yours, not mineBUG-007 as filed is about log bloat, and this PR closes that. Adding The truncation itself: no changes requested. |
Problem
withRpcFallback(src/utils/rpc-fallback.ts) logsprimaryErr.messageraw vialogger.warnwhen falling back to a secondary RPC connection. Every other error-log call site in this codebase (health.ts,markets.ts, etc.) already wraps the logged message withtruncateErrorMessage()— this was the one outlier.Impact
Solana RPC connection errors can embed the full endpoint URL in
.message. Per.env.example, this API's productionRPC_URL/FALLBACK_RPC_URLare expected to be paid providers (Helius/Alchemy) whose URLs embed an API key directly in the URL itself. A primary-RPC failure here could put a key-bearing URL into centralized logs in full, untruncated form, depending on who has log access.To be precise about what this fix actually achieves:
truncateErrorMessageonly bounds length, it doesn't redact secrets — so a short-enough error message could still leak a key even after truncation. This fix brings the call site in line with the same (imperfect but consistent) mitigation already applied everywhere else in the codebase, rather than fully eliminating the exposure.Fix
Wrap the logged message with
truncateErrorMessage(..., 120), matching the existing convention.Proof of Fix
New test file covers the no-fallback-configured passthrough, the normal fallback-success path, and the truncation behavior itself (a 273-char error message gets bounded to ≤123 chars in the logged output).
Verified the truncation test is a genuine regression test: reverted just the source change and reran — failed with the full 273-char raw message logged. Restored the fix and it passes.
tsc --noEmitclean (no separate lint script in this repo).Test Output
Full suite: 297/298 passed (294 baseline + 3 new). The 1 failure (
tests/sdk-smoke.test.ts) is pre-existing and unrelated — it asserts on an exact@percolatorct/sdkerror-message string that has drifted from the locally-resolved SDK version in this environment.Related
Found during a broader API audit; no existing open issue/PR covers this.