fix(transport): redact rpcUrl credentials before logging them - #5586
Conversation
A connection profile's rpcUrl is stored verbatim -- normalizeRpcUrl keeps
userinfo, query and hash -- so it can carry `user:pass@` or `?token=`.
That is what redactRpcUrlForLog exists for, and its own test pins exactly
that shape:
redactRpcUrlForLog('https://user:pass@host.example/rpc?token=secret#/token')
=== 'https://host.example/rpc'
Four construction-time log lines passed the raw URL instead:
CloudHttpTransport, LanHttpTransport, and both TransportManager
selection branches. With DEBUG=transport:* the credential lands in the
log verbatim.
transport:cloud makes the gap plain: it already reports the bearer token
by presence only ('set' / 'none'), then printed the URL beside it in
full.
Four tests capture what the debug namespaces actually emit and assert
the secrets are absent while the origin+path survives. All four are red
without the src change and green with it.
241 tests pass across services/transport, configPersistence and
coreRpcClient. prettier, eslint and tsc all exit 0.
How this change flows1 changed behaviour across 7 relationships. 4 surrounding behaviours are shown (60 graph nodes walked). 45 further behaviours left out to keep the diagram readable. flowchart LR
n0["TransportManager<br/>changed"]:::changed
n1["getTransport"]:::impacted
n2["CoreTransport"]:::impacted
n3["manager"]:::impacted
n4["manager"]:::impacted
n0 -->|uses| n2
n1 -->|uses| n2
n3 -->|calls| n0
n3 -->|uses| n0
n3 -->|tests| n0
n4 -->|calls| n0
n4 -->|tests| n0
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinysweeper
Bot
added
the
priority: p3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughTransport constructors and selection logs now redact credentials and tokens from RPC URLs. New tests verify safe URL logging and secret removal for cloud, LAN, and manager-based transport creation. ChangesTransport log URL redaction
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change redacts credentials from transport URLs while preserving useful origin and path information; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0231 · 64,501 in / 13,105 out · 14,152 cached (22%) · openrouter/openai/text-embedding-3-small, deepseek/deepseek-v4-flash, z-ai/glm-5.2 · 234 embedded
critique: $0.0017 · 23,139 in / 136 out · 0 cached (0%) · deepseek/deepseek-v4-flash
security: $0.0017 · 23,097 in / 130 out · 0 cached (0%) · deepseek/deepseek-v4-flash
tests: $0.0186 · 12,801 in / 12,626 out · 9,975 cached (78%) · z-ai/glm-5.2
description: $0.0011 · 5,464 in / 213 out · 4,177 cached (76%) · z-ai/glm-5.2
| /** Collect everything the `debug` namespaces emit while `fn` runs. */ | ||
| function captureDebug(fn: () => void): string { | ||
| const lines: string[] = []; | ||
| const previous = debug.disable(); | ||
| const previousLog = debug.log; | ||
| debug.enable('transport:*'); | ||
| debug.log = (...args: unknown[]) => { | ||
| lines.push(args.map(String).join(' ')); | ||
| }; | ||
| try { | ||
| fn(); | ||
| } finally { | ||
| debug.log = previousLog; | ||
| debug.disable(); | ||
| if (previous) debug.enable(previous); | ||
| } | ||
| return lines.join('\n'); | ||
| } |
There was a problem hiding this comment.
Await async callbacks in captureDebug so async logs are captured
captureDebug calls fn() synchronously and restores debug.log in a finally block. getTransport() is async — it awaits rpcUrl/sessionToken resolution before logging — so the TransportManager log line fires in a microtask after captureDebug has restored debug.log and disabled the namespace. The two TransportManager tests therefore capture empty output, and expectNoSecrets(output) passes trivially. They would still pass if the redaction in TransportManager.ts were reverted, because the secret-leaking log line is never captured.
Making captureDebug async and awaiting fn() keeps the capture window open. All four callers must then await captureDebug(...), and the two TransportManager tests must await getTransport() inside the callback, e.g.:
const output = await captureDebug(async () => {
await createTransportManager(profile('cloud')).getTransport();
});The CloudHttpTransport and LanHttpTransport callers only need a leading await added.
| /** Collect everything the `debug` namespaces emit while `fn` runs. */ | |
| function captureDebug(fn: () => void): string { | |
| const lines: string[] = []; | |
| const previous = debug.disable(); | |
| const previousLog = debug.log; | |
| debug.enable('transport:*'); | |
| debug.log = (...args: unknown[]) => { | |
| lines.push(args.map(String).join(' ')); | |
| }; | |
| try { | |
| fn(); | |
| } finally { | |
| debug.log = previousLog; | |
| debug.disable(); | |
| if (previous) debug.enable(previous); | |
| } | |
| return lines.join('\n'); | |
| } | |
| /** Collect everything the `debug` namespaces emit while `fn` runs. */ | |
| async function captureDebug(fn: () => void | Promise<void>): Promise<string> { | |
| const lines: string[] = []; | |
| const previous = debug.disable(); | |
| const previousLog = debug.log; | |
| debug.enable('transport:*'); | |
| debug.log = (...args: unknown[]) => { | |
| lines.push(args.map(String).join(' ')); | |
| }; | |
| try { | |
| await fn(); | |
| } finally { | |
| debug.log = previousLog; | |
| debug.disable(); | |
| if (previous) debug.enable(previous); | |
| } | |
| return lines.join('\n'); | |
| } |
[RULE] test-asserts-nothing ·
tinysweeper
Bot
added
priority: p2
|
Maintainer review (merge-readiness sweep). Disclosure first: earlier in this sweep I ran GitHub's "Update branch" on this PR, which pushed a merge commit — Assessment: soundChecked against current
Small and correct; the 93 lines of test for 14 lines of production change are well spent given this is a credential-leak path. State
Nothing needed from you. |
…-rpcurl-redaction\n\nfix(transport): redact rpcUrl credentials before logging them\n
3 participants
Summary
A connection profile's
rpcUrlis stored verbatim —normalizeRpcUrldeliberately keeps query and hash "byte-for-byte intact" and does not touch userinfo — so it can carryuser:pass@or?token=. That is precisely whatredactRpcUrlForLogexists for, and what its own test already pins:Four construction-time log lines passed the raw URL instead of the redacted one:
services/transport/CloudHttpTransport.tsservices/transport/LanHttpTransport.tsservices/transport/TransportManager.tscoreRpcClient.tsandconfigPersistence.tsalready route through the helper; these four were the ones left out.What makes it clear-cut
transport:cloudwas already careful with the other secret on the same line — it reports the bearer token by presence, never by value:So the token is protected and the URL beside it is not.
Reproduction
Captured from the real
debugnamespaces on this branch's parent, withrpcUrl = https://svc:HUNTER2@core.example.com/rpc?token=SUPERSECRET#/tok:Both the password and the query token are in the log line. Anyone with
DEBUG=transport:*— or anyone reading a log a user pastes into an issue — gets them.After the change the same line reads
https://core.example.com/rpc.Verification
app/src/services/transport/logRedaction.test.ts— four tests that capture what the namespaces actually emit (by swappingdebug.log) rather than asserting on the source. They cover both transports and bothTransportManagerselection branches, assert the secrets are absent, and assert the origin+path survives so the log stays useful.Reverting only the three
srcfiles, keeping the test:All four are bug proofs; none of them pass on the old code.
Wider run, using the repo's own config:
prettier --check .,eslint srcandtsc --noEmiteach exit 0.Scope
Deliberately narrow: only the redaction of an already-logged value changes. No log line is added or removed, no behaviour outside logging is touched, and
redactRpcUrlForLogitself is unchanged.Summary by CodeRabbit
Bug Fixes
Tests