Skip to content

fix(api): truncate primary-RPC error before logging it [BUG-007] - #216

Open
Morenikeoa wants to merge 1 commit into
dcccrypto:mainfrom
Morenikeoa:fix/rpc-fallback-log-truncation
Open

fix(api): truncate primary-RPC error before logging it [BUG-007]#216
Morenikeoa wants to merge 1 commit into
dcccrypto:mainfrom
Morenikeoa:fix/rpc-fallback-log-truncation

Conversation

@Morenikeoa

Copy link
Copy Markdown

Problem

withRpcFallback (src/utils/rpc-fallback.ts) logs primaryErr.message raw via logger.warn when 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 with truncateErrorMessage() — this was the one outlier.

Impact

Solana RPC connection errors can embed the full endpoint URL in .message. Per .env.example, this API's production RPC_URL/FALLBACK_RPC_URL are 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: truncateErrorMessage only 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.

  • All existing tests pass — output attached.
  • New tests pass against the fix, the truncation test fails against pre-fix code (verified locally).
  • tsc --noEmit clean (no separate lint script in this repo).

Test Output

✓ tests/utils/rpc-fallback.test.ts (3 tests) 29ms

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/sdk error-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.

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>
@vercel

vercel Bot commented Jun 25, 2026

Copy link
Copy Markdown

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

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Morenikeoa, we couldn't start this review because you've reached your PR review rate limit.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a62f44fb-4962-40ea-8246-3eb6dda81cbd

📥 Commits

Reviewing files that changed from the base of the PR and between b2751f4 and 77f8e20.

📒 Files selected for processing (2)
  • src/utils/rpc-fallback.ts
  • tests/utils/rpc-fallback.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dcccrypto

Copy link
Copy Markdown
Owner

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.

Verification

Branch is current with main (0 behind), tests pass (3), and neutralising the truncation:

error: truncateErrorMessage(msg, 120)      error: msg
1 FAILED
  × truncates a long primary-RPC error message before logging it,
    instead of logging it raw (BUG-007)

Binds cleanly to the change.

The bit worth acting on

Credit for the comment — it's unusually honest about its own limits:

truncateErrorMessage only bounds length (it doesn't redact secrets) … RPC connection errors can embed the full endpoint URL, and paid providers (Helius/Alchemy) embed an API key in that URL, so a short error message could still leak it even truncated.

That's exactly right. And @percolator/shared — the module this PR already imports from — exports maskApiKeys for precisely this:

/**
 * 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:

RAW                     : failed to fetch https://mainnet.helius-rpc.com/?api-key=abc123SECRETkey456 : ECONNREFUSED
TRUNCATE ONLY (this PR) : failed to fetch https://mainnet.helius-rpc.com/?api-key=abc123SECRETkey456 : ECONNREFUSED
MASK THEN TRUNCATE      : failed to fetch https://mainnet.helius-rpc.com/?api-key=*** : ECONNREFUSED

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 mine

BUG-007 as filed is about log bloat, and this PR closes that. Adding maskApiKeys is arguably a different bug. I'd lean toward doing it here because it's one call, the import is already present, and the comment shows you'd already reasoned it through — but splitting it into a follow-up is a legitimate choice. Either way the finding shouldn't get lost, since the current state leaves a real key-in-logs path that reads as handled.

The truncation itself: no changes requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants