From 9b278165d82b1e85df252334bf0bc7e378dabca0 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Wed, 19 Aug 2026 15:46:47 +0700 Subject: [PATCH] fix(transport): redact rpcUrl credentials before logging them 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. --- .../services/transport/CloudHttpTransport.ts | 10 +- .../services/transport/LanHttpTransport.ts | 3 +- .../services/transport/TransportManager.ts | 5 +- .../services/transport/logRedaction.test.ts | 93 +++++++++++++++++++ 4 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 app/src/services/transport/logRedaction.test.ts diff --git a/app/src/services/transport/CloudHttpTransport.ts b/app/src/services/transport/CloudHttpTransport.ts index 6637ffdcc0..5ed18713cc 100644 --- a/app/src/services/transport/CloudHttpTransport.ts +++ b/app/src/services/transport/CloudHttpTransport.ts @@ -6,6 +6,7 @@ */ import debug from 'debug'; +import { redactRpcUrlForLog } from '../../utils/redactRpcUrlForLog'; import type { CoreTransport } from './CoreTransport'; const log = debug('transport:cloud'); @@ -35,7 +36,14 @@ export class CloudHttpTransport implements CoreTransport { private readonly bearerToken: string | null = null, private readonly timeoutMs: number = 30_000 ) { - log('[transport:cloud] created rpcUrl=%s token=%s', rpcUrl, bearerToken ? 'set' : 'none'); + // The bearer token is already reported by presence only; the URL needs the same + // care, because a profile rpcUrl may carry `user:pass@` or `?token=` (see the + // redactRpcUrlForLog test) and this line ran before any of it was stripped. + log( + '[transport:cloud] created rpcUrl=%s token=%s', + redactRpcUrlForLog(rpcUrl), + bearerToken ? 'set' : 'none' + ); } async call(method: string, params: unknown, opts?: { signal?: AbortSignal }): Promise { diff --git a/app/src/services/transport/LanHttpTransport.ts b/app/src/services/transport/LanHttpTransport.ts index 83d2f7633c..a88bee55f1 100644 --- a/app/src/services/transport/LanHttpTransport.ts +++ b/app/src/services/transport/LanHttpTransport.ts @@ -6,6 +6,7 @@ */ import debug from 'debug'; +import { redactRpcUrlForLog } from '../../utils/redactRpcUrlForLog'; import type { CoreTransport } from './CoreTransport'; const log = debug('transport:lan'); @@ -34,7 +35,7 @@ export class LanHttpTransport implements CoreTransport { private readonly rpcUrl: string, private readonly timeoutMs: number = 10_000 ) { - log('[transport:lan] created rpcUrl=%s', rpcUrl); + log('[transport:lan] created rpcUrl=%s', redactRpcUrlForLog(rpcUrl)); } async call(method: string, params: unknown, opts?: { signal?: AbortSignal }): Promise { diff --git a/app/src/services/transport/TransportManager.ts b/app/src/services/transport/TransportManager.ts index d2d8617365..6dd36d2270 100644 --- a/app/src/services/transport/TransportManager.ts +++ b/app/src/services/transport/TransportManager.ts @@ -9,6 +9,7 @@ */ import debug from 'debug'; +import { redactRpcUrlForLog } from '../../utils/redactRpcUrlForLog'; import { CloudHttpTransport } from './CloudHttpTransport'; import type { CoreTransport } from './CoreTransport'; import { LanHttpTransport } from './LanHttpTransport'; @@ -80,7 +81,7 @@ export class TransportManager { throw new Error('[transport:manager] cloud profile missing rpcUrl'); } const t = new CloudHttpTransport(rpcUrl, sessionToken ?? null); - log('[transport:manager] → CloudHttpTransport rpcUrl=%s', rpcUrl); + log('[transport:manager] → CloudHttpTransport rpcUrl=%s', redactRpcUrlForLog(rpcUrl)); return t; } @@ -90,7 +91,7 @@ export class TransportManager { throw new Error('[transport:manager] lan profile missing rpcUrl'); } const t = new LanHttpTransport(rpcUrl); - log('[transport:manager] → LanHttpTransport rpcUrl=%s', rpcUrl); + log('[transport:manager] → LanHttpTransport rpcUrl=%s', redactRpcUrlForLog(rpcUrl)); return t; } diff --git a/app/src/services/transport/logRedaction.test.ts b/app/src/services/transport/logRedaction.test.ts new file mode 100644 index 0000000000..500efa8ece --- /dev/null +++ b/app/src/services/transport/logRedaction.test.ts @@ -0,0 +1,93 @@ +import debug from 'debug'; +import { describe, expect, it } from 'vitest'; + +import { CloudHttpTransport } from './CloudHttpTransport'; +import { LanHttpTransport } from './LanHttpTransport'; +import type { ConnectionProfile } from './profileStore'; +import { createTransportManager } from './TransportManager'; + +/** + * A connection profile's `rpcUrl` is stored verbatim — `normalizeRpcUrl` keeps + * userinfo, query and hash — so it can carry `user:pass@` or `?token=`. That is + * exactly what `redactRpcUrlForLog` exists for, and what its own test pins. + * + * These transports logged the raw URL at construction time. `transport:cloud` + * was already careful with the bearer token (it reports presence, not value), + * which is what makes the URL beside it the odd one out. + */ + +const SECRETS = ['HUNTER2', 'SUPERSECRET'] as const; +const SECRET_URL = 'https://svc:HUNTER2@core.example.com/rpc?token=SUPERSECRET#/tok'; +const SAFE_PART = 'https://core.example.com/rpc'; + +/** 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'); +} + +function expectNoSecrets(output: string) { + for (const secret of SECRETS) { + expect(output).not.toContain(secret); + } +} + +function profile(kind: 'cloud' | 'lan'): ConnectionProfile { + return { + id: `p-${kind}`, + kind, + rpcUrl: SECRET_URL, + sessionToken: 'session-value', + } as unknown as ConnectionProfile; +} + +describe('transport construction logging', () => { + it('CloudHttpTransport does not log rpcUrl credentials', () => { + const output = captureDebug(() => { + new CloudHttpTransport(SECRET_URL, 'bearer-value'); + }); + expectNoSecrets(output); + expect(output).toContain(SAFE_PART); + // The bearer value was never logged and must stay that way. + expect(output).not.toContain('bearer-value'); + }); + + it('LanHttpTransport does not log rpcUrl credentials', () => { + const output = captureDebug(() => { + new LanHttpTransport(SECRET_URL); + }); + expectNoSecrets(output); + expect(output).toContain(SAFE_PART); + }); + + it('TransportManager does not log rpcUrl credentials when selecting cloud', async () => { + let selected: Promise | undefined; + const output = captureDebug(() => { + selected = createTransportManager(profile('cloud')).getTransport(); + }); + await selected; + expectNoSecrets(output); + }); + + it('TransportManager does not log rpcUrl credentials when selecting lan', async () => { + let selected: Promise | undefined; + const output = captureDebug(() => { + selected = createTransportManager(profile('lan')).getTransport(); + }); + await selected; + expectNoSecrets(output); + }); +});