diff --git a/modules/express/src/clientRoutes.ts b/modules/express/src/clientRoutes.ts index 0dd314fbcbc..0d8b0780622 100755 --- a/modules/express/src/clientRoutes.ts +++ b/modules/express/src/clientRoutes.ts @@ -1163,6 +1163,10 @@ async function handleV2AccelerateTransaction(req: ExpressApiRouteRequest<'expres return wallet.accelerateTransaction(createSendParams(req)); } +function cloneRequestBody(req: express.Request) { + return req.body !== null && typeof req.body === 'object' && !Array.isArray(req.body) ? { ...req.body } : req.body; +} + function createSendParams(req: express.Request) { if (req.config?.externalSignerUrl !== undefined) { return { @@ -1170,7 +1174,7 @@ function createSendParams(req: express.Request) { customSigningFunction: createCustomSigningFunction(req.config.externalSignerUrl), }; } else { - return req.body; + return cloneRequestBody(req); } } @@ -1241,7 +1245,7 @@ function createTSSSendParams(req: express.Request, wallet: Wallet) { throw new Error(`MPC Algorithm ${coin.getMPCAlgorithm()} is not supported.`); } } else { - return req.body; + return cloneRequestBody(req); } } @@ -1846,36 +1850,59 @@ interface RequestHandler extends express.RequestHandler; } -function collectSensitiveRequestValues(value: unknown, values = new Set()): Set { +function isTraversableObject(value: unknown): value is Record | unknown[] { + return value !== null && typeof value === 'object'; +} + +function collectSensitiveRequestValues( + value: unknown, + values = new Set(), + seen = new WeakSet | unknown[]>() +): Set { + if (!isTraversableObject(value) || seen.has(value)) { + return values; + } + seen.add(value); if (Array.isArray(value)) { - value.forEach((item) => collectSensitiveRequestValues(item, values)); - } else if (value !== null && typeof value === 'object') { + value.forEach((item) => collectSensitiveRequestValues(item, values, seen)); + } else { Object.entries(value).forEach(([key, item]) => { if (SENSITIVE_REQUEST_KEYS.has(key.toLowerCase()) && typeof item === 'string' && item.length > 0) { values.add(item); } - collectSensitiveRequestValues(item, values); + collectSensitiveRequestValues(item, values, seen); }); } return values; } -function redactSensitiveValues(value: unknown, sensitiveValues: Set): unknown { +function redactSensitiveValues( + value: unknown, + sensitiveValues: Set, + ancestors = new WeakSet | unknown[]>() +): unknown { if (typeof value === 'string') { return [...sensitiveValues].reduce((redacted, secret) => redacted.split(secret).join('[REDACTED]'), value); } - if (Array.isArray(value)) { - return value.map((item) => redactSensitiveValues(item, sensitiveValues)); - } - if (value !== null && typeof value === 'object') { - return Object.fromEntries( - Object.entries(value).map(([key, item]) => [ - key, - SENSITIVE_REQUEST_KEYS.has(key.toLowerCase()) ? '[REDACTED]' : redactSensitiveValues(item, sensitiveValues), - ]) - ); - } - return value; + if (!isTraversableObject(value)) { + return value; + } + if (ancestors.has(value)) { + return '[Circular]'; + } + ancestors.add(value); + const redacted = Array.isArray(value) + ? value.map((item) => redactSensitiveValues(item, sensitiveValues, ancestors)) + : Object.fromEntries( + Object.entries(value).map(([key, item]) => [ + key, + SENSITIVE_REQUEST_KEYS.has(key.toLowerCase()) + ? '[REDACTED]' + : redactSensitiveValues(item, sensitiveValues, ancestors), + ]) + ); + ancestors.delete(value); + return redacted; } function handleRequestHandlerError(res: express.Response, error: unknown, requestBody?: unknown) { diff --git a/modules/express/test/unit/clientRoutes/index.ts b/modules/express/test/unit/clientRoutes/index.ts index 5c8abff7319..8b6603e8b96 100644 --- a/modules/express/test/unit/clientRoutes/index.ts +++ b/modules/express/test/unit/clientRoutes/index.ts @@ -212,5 +212,29 @@ describe('common methods', () => { consoleLog.args.flat().join(' ').should.not.containEql(privateKey); consoleLog.args.flat().join(' ').should.not.containEql(walletPassphrase); }); + it('should handle circular request bodies while redacting errors', async () => { + const passphrase = 'circular-passphrase'; + const requestBody: { credentials: { passphrase: string }; self?: unknown } = { + credentials: { passphrase }, + }; + requestBody.self = requestBody; + const errorResult: { secret: string; self?: unknown } = { secret: passphrase }; + errorResult.self = errorResult; + const handler = sandbox + .stub() + .rejects(Object.assign(new Error(`failed with ${passphrase}`), { result: errorResult })); + const req = { body: requestBody } as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + + const next: express.NextFunction = () => undefined; + await promiseWrapper(handler)(req, res, next); + + status.calledWith(500).should.be.true(); + send.firstCall.args[0].message.should.equal('failed with [REDACTED]'); + send.firstCall.args[0].secret.should.equal('[REDACTED]'); + send.firstCall.args[0].self.should.equal('[Circular]'); + }); }); }); diff --git a/modules/express/test/unit/typedRoutes/sendmany.ts b/modules/express/test/unit/typedRoutes/sendmany.ts index 2525329941a..bf8699eb3e1 100644 --- a/modules/express/test/unit/typedRoutes/sendmany.ts +++ b/modules/express/test/unit/typedRoutes/sendmany.ts @@ -365,6 +365,50 @@ describe('SendMany V2 codec tests', function () { result.body.should.have.property('error'); }); + it('should redact secrets when sendMany mutates the cloned params and then fails', async function () { + const walletPassphrase = 'test_passphrase_12345'; + const requestBody = { + recipients: [ + { address: 'mzKTJw3XJNb7VfkFP77mzPJJz4Dkp4M1T6', amount: 1000000 }, + { address: 'mvQewFHmFjJVr5G7K9TJWNQxB7cLGhJpJV', amount: 500000 }, + ], + walletPassphrase, + }; + + // Mimic the SDK: attach a self-referencing wallet to the params (as Wallet.prebuildTransaction + // does with Object.assign(params, { wallet: this })), drop the passphrase from them, then fail + // with an error that leaks the passphrase. + let sdkParams: Record | undefined; + const sendMany = sinon.stub().callsFake(async (params: Record) => { + sdkParams = params; + const circularWallet: Record = { id: walletId }; + circularWallet.self = circularWallet; + Object.assign(params, { wallet: circularWallet }); + delete params.walletPassphrase; + throw new Error(`failed to sign with ${walletPassphrase}`); + }); + const mockWallet = { sendMany, _wallet: { multisigType: 'onchain' } }; + const mockCoin = { wallets: sinon.stub().returns({ get: sinon.stub().resolves(mockWallet) }) }; + sinon.stub(BitGo.prototype, 'coin').returns(mockCoin as unknown as ReturnType); + const consoleLog = sinon.stub(console, 'log'); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/sendmany`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send(requestBody); + consoleLog.restore(); + + assert.strictEqual(result.status, 400); + assert.strictEqual(result.body.message, 'failed to sign with [REDACTED]'); + assert.ok(!JSON.stringify(result.body).includes(walletPassphrase)); + assert.ok(!consoleLog.args.flat().join(' ').includes(walletPassphrase)); + // The SDK received a copy of the request body, not the body itself + assert.ok(sdkParams); + assert.ok(sdkParams.wallet); + assert.strictEqual(sdkParams.walletPassphrase, undefined); + }); + it('should support token transfer parameters', async function () { const requestBody = { recipients: [