From 86a5cf15d545b17383fe0c89c453e7758e421cff Mon Sep 17 00:00:00 2001 From: bitgobot Date: Mon, 5 Oct 2026 18:40:17 +0000 Subject: [PATCH 1/2] fix: surface response encoding errors instead of bare 500 {} typed-express-router validates every outbound response against its io-ts codec in res.sendEncoded() and falls back to encodeErrorFormatter when the payload fails that check. The library default swallows the validation error and replies with HTTP 500 and an empty JSON body, which is what customers hit from POST /wallet/generate while the WCN-2395 permission bug left advanced-wallet creators view-only: the wallet was created on the backend, but MBE returned a bare 500 {} with nothing actionable in the response or the server logs. Add a customEncodeErrorFormatter that logs the real error (message, stack, and request context) and returns { error, details } in the response body, matching the documented 500 ErrorResponse shape, and wire it into every typed router (MBE, AWM, and the health check routers) alongside the existing decode error formatter. Ticket: WCN-2396 Session-Id: 055eae11-5e57-4df7-8a1e-f8148946b128 Task-Id: 0aaa81ad-1de6-4466-96fb-681ce47e5a15 --- .../routers/advancedWalletManagerApiSpec.ts | 6 +++- .../routers/healthCheck.ts | 6 +++- .../routers/awmExpressHealth.ts | 6 +++- src/masterBitgoExpress/routers/healthCheck.ts | 6 +++- .../routers/masterBitGoExpressApiSpec.ts | 6 +++- src/shared/errorFormatters.ts | 28 ++++++++++++++++++- 6 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/advancedWalletManager/routers/advancedWalletManagerApiSpec.ts b/src/advancedWalletManager/routers/advancedWalletManagerApiSpec.ts index 958ce036..f7659fc5 100644 --- a/src/advancedWalletManager/routers/advancedWalletManagerApiSpec.ts +++ b/src/advancedWalletManager/routers/advancedWalletManagerApiSpec.ts @@ -35,7 +35,10 @@ import { ecdsaMPCv2Finalize } from '../handlers/ecdsaMPCV2WalletGenerationFinali import { ecdsaMPCv2Recovery } from '../handlers/ecdsaMPCV2Recovery'; import { signEddsaRecoveryTransaction } from '../handlers/eddsaMPCRecovery'; import { isEddsaCoin } from '../../shared/coinUtils'; -import { customDecodeErrorFormatter } from '../../shared/errorFormatters'; +import { + customDecodeErrorFormatter, + customEncodeErrorFormatter, +} from '../../shared/errorFormatters'; // Request type for /key/independent endpoint const IndependentKeyRequest = { @@ -512,6 +515,7 @@ export function createKeyGenRouter( ): WrappedRouter { const router = createRouter(AdvancedWalletManagerApiSpec, { decodeErrorFormatter: customDecodeErrorFormatter, + encodeErrorFormatter: customEncodeErrorFormatter, }); // Add middleware router.use(express.json()); diff --git a/src/advancedWalletManager/routers/healthCheck.ts b/src/advancedWalletManager/routers/healthCheck.ts index d0462bcd..4e1666fe 100644 --- a/src/advancedWalletManager/routers/healthCheck.ts +++ b/src/advancedWalletManager/routers/healthCheck.ts @@ -4,7 +4,10 @@ import { Response } from '@api-ts/response'; import pjson from '../../../package.json'; import { responseHandler } from '../../shared/middleware'; import { PingResponseType, VersionResponseType } from '../../types/health'; -import { customDecodeErrorFormatter } from '../../shared/errorFormatters'; +import { + customDecodeErrorFormatter, + customEncodeErrorFormatter, +} from '../../shared/errorFormatters'; // API Response types const PingResponse: HttpResponse = { @@ -60,6 +63,7 @@ export const HealthCheckApiSpec = apiSpec({ export function createHealthCheckRouter(): WrappedRouter { const router = createRouter(HealthCheckApiSpec, { decodeErrorFormatter: customDecodeErrorFormatter, + encodeErrorFormatter: customEncodeErrorFormatter, }); // Ping endpoint handler router.post('advancedwallet.awm.ping', [ diff --git a/src/masterBitgoExpress/routers/awmExpressHealth.ts b/src/masterBitgoExpress/routers/awmExpressHealth.ts index 04d83e56..97ddd81f 100644 --- a/src/masterBitgoExpress/routers/awmExpressHealth.ts +++ b/src/masterBitgoExpress/routers/awmExpressHealth.ts @@ -7,7 +7,10 @@ import logger from '../../shared/logger'; import { responseHandler } from '../../shared/middleware'; import { AdvancedWalletManagerClient } from '../clients/advancedWalletManagerClient'; import { PingResponseType } from '../../types/health'; -import { customDecodeErrorFormatter } from '../../shared/errorFormatters'; +import { + customDecodeErrorFormatter, + customEncodeErrorFormatter, +} from '../../shared/errorFormatters'; // Response type for /ping/advancedWalletManager endpoint const PingAwmResponse: HttpResponse = { @@ -49,6 +52,7 @@ export function createAdvancedWalletManagerHealthRouter( ): WrappedRouter { const router = createRouter(AdvancedWalletManagerHealthSpec, { decodeErrorFormatter: customDecodeErrorFormatter, + encodeErrorFormatter: customEncodeErrorFormatter, }); // Create an instance of awmClient diff --git a/src/masterBitgoExpress/routers/healthCheck.ts b/src/masterBitgoExpress/routers/healthCheck.ts index 0dc78e2e..055624fb 100644 --- a/src/masterBitgoExpress/routers/healthCheck.ts +++ b/src/masterBitgoExpress/routers/healthCheck.ts @@ -4,7 +4,10 @@ import { Response } from '@api-ts/response'; import pjson from '../../../package.json'; import { responseHandler } from '../../shared/middleware'; import { PingResponseType, VersionResponseType } from '../../types/health'; -import { customDecodeErrorFormatter } from '../../shared/errorFormatters'; +import { + customDecodeErrorFormatter, + customEncodeErrorFormatter, +} from '../../shared/errorFormatters'; // API Response types const PingResponse: HttpResponse = { @@ -63,6 +66,7 @@ export function createHealthCheckRouter( ): WrappedRouter { const router = createRouter(HealthCheckApiSpec, { decodeErrorFormatter: customDecodeErrorFormatter, + encodeErrorFormatter: customEncodeErrorFormatter, }); // Ping endpoint handler diff --git a/src/masterBitgoExpress/routers/masterBitGoExpressApiSpec.ts b/src/masterBitgoExpress/routers/masterBitGoExpressApiSpec.ts index 2ffe05d3..0636ee26 100644 --- a/src/masterBitgoExpress/routers/masterBitGoExpressApiSpec.ts +++ b/src/masterBitgoExpress/routers/masterBitGoExpressApiSpec.ts @@ -6,7 +6,10 @@ import { type WrappedRouter, } from '@api-ts/typed-express-router'; import express from 'express'; -import { customDecodeErrorFormatter } from '../../shared/errorFormatters'; +import { + customDecodeErrorFormatter, + customEncodeErrorFormatter, +} from '../../shared/errorFormatters'; import { MasterExpressConfig } from '../../shared/types'; import * as utxolib from '@bitgo-beta/utxo-lib'; import { prepareBitGo, responseHandler } from '../../shared/middleware'; @@ -104,6 +107,7 @@ export function createMasterApiRouter( ): WrappedRouter { const router = createRouter(MasterBitGoExpressApiSpec, { decodeErrorFormatter: customDecodeErrorFormatter, + encodeErrorFormatter: customEncodeErrorFormatter, }); // Add middleware to all routes diff --git a/src/shared/errorFormatters.ts b/src/shared/errorFormatters.ts index 290cf27a..a2791542 100644 --- a/src/shared/errorFormatters.ts +++ b/src/shared/errorFormatters.ts @@ -1,8 +1,34 @@ import * as PathReporter from 'io-ts/lib/PathReporter'; -import { DecodeErrorFormatterFn } from '@api-ts/typed-express-router'; +import { DecodeErrorFormatterFn, EncodeErrorFormatterFn } from '@api-ts/typed-express-router'; +import logger from './logger'; export const customDecodeErrorFormatter: DecodeErrorFormatterFn = (errs, _req) => { const validationErrors = PathReporter.failure(errs); const validationErrorMessage = validationErrors.join('\n'); return { error: validationErrorMessage }; }; + +/** + * typed-express-router validates every outbound response against its io-ts codec in + * res.sendEncoded(). When the payload fails that check the library falls back to + * encodeErrorFormatter, whose default swallows the validation error and responds with + * a bare HTTP 500 and an empty JSON body. Log the real error (message and stack) and + * surface it in the response body so schema violations are debuggable instead of silent. + */ +export const customEncodeErrorFormatter: EncodeErrorFormatterFn = (err, req) => { + const errMessage = err instanceof Error ? err.message : String(err); + const errStack = err instanceof Error ? err.stack : undefined; + + const logLines = [ + `Failed to encode API response for ${req.method} ${req.originalUrl} (api: ${req.apiName}): ${errMessage}`, + ]; + if (errStack) { + logLines.push(errStack); + } + logger.error(logLines.join('\n')); + + return { + error: 'Internal Server Error', + details: errMessage, + }; +}; From 8071841a4d562fe07510b4b0f965aee54f973459 Mon Sep 17 00:00:00 2001 From: bitgobot Date: Mon, 5 Oct 2026 18:41:57 +0000 Subject: [PATCH 2/2] test: add encode-error regression tests Cover the new encode error formatter both end to end and at unit level: a wallet/add response that omits a required field (as produced by the WCN-2395 permission bug) must produce a 500 with a descriptive { error, details } body plus a server-side log line naming the failed route, instead of the bare 500 {} MBE previously returned. The end-to-end test reproduces the bare-500 failure against the pre-fix router wiring. Ticket: WCN-2396 Session-Id: 055eae11-5e57-4df7-8a1e-f8148946b128 Task-Id: 0aaa81ad-1de6-4466-96fb-681ce47e5a15 --- .../api/master/generateWallet.test.ts | 113 ++++++++++++++++++ src/__tests__/errorFormatters.test.ts | 49 ++++++++ 2 files changed, 162 insertions(+) create mode 100644 src/__tests__/errorFormatters.test.ts diff --git a/src/__tests__/api/master/generateWallet.test.ts b/src/__tests__/api/master/generateWallet.test.ts index 206c2a6b..c049f0e9 100644 --- a/src/__tests__/api/master/generateWallet.test.ts +++ b/src/__tests__/api/master/generateWallet.test.ts @@ -9,6 +9,7 @@ import { AppMode, MasterExpressConfig, TlsMode } from '../../../shared/types'; import { BitgoMpcGpgPubKeys, Environments } from '@bitgo-beta/sdk-core'; import { BitGoAPI } from '@bitgo-beta/sdk-api'; import * as middleware from '../../../shared/middleware'; +import logger from '../../../shared/logger'; import { BitGoRequest } from '../../../types/request'; import { BitGoAPITestHarness, DEFAULT_ASYNC_MODE_CONFIG } from './testUtils'; @@ -347,6 +348,118 @@ describe('POST /api/v1/:coin/advancedwallet/generate', () => { bitgoAddWalletNock.done(); }); + it('returns a descriptive 500 error body when the wallet/add response violates the response schema', async () => { + // WCN-2395 leaves the wallet creator at view-only, and the wallet/add response can + // come back without fields MBE declares as required. The wallet is still created on + // the backend, but the response payload fails its io-ts codec check in sendEncoded(). + // typed-express-router's default encodeErrorFormatter would swallow the validation + // error and reply with a bare HTTP 500 and an empty JSON body. + const loggerErrorSpy = sinon.spy(logger, 'error'); + + const userKeychainNock = nock(advancedWalletManagerUrl) + .post(`/api/${coin}/key/independent`, { + source: 'user', + }) + .reply(200, { + pub: validUserPub, + source: 'user', + type: 'independent', + }); + + const backupKeychainNock = nock(advancedWalletManagerUrl) + .post(`/api/${coin}/key/independent`, { + source: 'backup', + }) + .reply(200, { + pub: validBackupPub, + source: 'backup', + type: 'independent', + }); + + const bitgoAddUserKeyNock = nock(bitgoApiUrl) + .post(`/api/v2/${coin}/key`, { + pub: validUserPub, + keyType: 'independent', + source: 'user', + }) + .matchHeader('any', () => true) + .reply(200, { id: 'user-key-id', pub: validUserPub, source: 'user', type: 'independent' }); + + const bitgoAddBackupKeyNock = nock(bitgoApiUrl) + .post(`/api/v2/${coin}/key`, { + pub: validBackupPub, + keyType: 'independent', + source: 'backup', + }) + .matchHeader('any', () => true) + .reply(200, { + id: 'backup-key-id', + pub: validBackupPub, + source: 'backup', + type: 'independent', + }); + + const bitgoAddBitGoKeyNock = nock(bitgoApiUrl) + .post(`/api/v2/${coin}/key`, { + source: 'bitgo', + enterprise: 'test_enterprise', + }) + .reply(200, { + id: 'bitgo-key-id', + pub: 'xpub_bitgo', + source: 'bitgo', + type: 'independent', + isBitGo: true, + isTrust: false, + hsmType: 'institutional', + }); + + // JSON.stringify drops undefined properties, so the wallet/add response omits + // the required `users` field, violating MBE's GenerateWalletResponse schema + const schemaViolatingWallet = mockWalletResponse('new-wallet-id', coin, { + users: undefined, + }); + + const bitgoAddWalletNock = nock(bitgoApiUrl) + .post(`/api/v2/${coin}/wallet/add`, { + label: 'test_wallet', + enterprise: 'test_enterprise', + m: 2, + n: 3, + keys: ['user-key-id', 'backup-key-id', 'bitgo-key-id'], + type: 'advanced', + }) + .matchHeader('any', () => true) + .reply(200, schemaViolatingWallet); + + const response = await agent + .post(`/api/v1/${coin}/advancedwallet/generate`) + .set('Authorization', `Bearer ${accessToken}`) + .send({ + label: 'test_wallet', + enterprise: 'test_enterprise', + multisigType: 'onchain', + }); + + // The response is rejected (500) because the payload does not match the declared + // schema, but the error must be descriptive instead of an empty body. + response.status.should.equal(500); + response.body.error.should.equal('Internal Server Error'); + response.body.details.should.containEql('response does not match expected type'); + + // The real validation error is logged server-side for debugging. + assert.equal(loggerErrorSpy.callCount, 1); + loggerErrorSpy.firstCall.args[0].should.containEql('Failed to encode API response'); + loggerErrorSpy.firstCall.args[0].should.containEql(`/api/v1/${coin}/advancedwallet/generate`); + + userKeychainNock.done(); + backupKeychainNock.done(); + bitgoAddUserKeyNock.done(); + bitgoAddBackupKeyNock.done(); + bitgoAddBitGoKeyNock.done(); + bitgoAddWalletNock.done(); + }); + it('should generate a TSS MPC v1 (EdDSA) wallet with separate backup AWM', async () => { const backupAwmUrl = 'http://backup-awm.invalid'; diff --git a/src/__tests__/errorFormatters.test.ts b/src/__tests__/errorFormatters.test.ts new file mode 100644 index 00000000..f2cca6fe --- /dev/null +++ b/src/__tests__/errorFormatters.test.ts @@ -0,0 +1,49 @@ +import 'should'; +import assert from 'assert'; +import sinon from 'sinon'; +import { WrappedRequest } from '@api-ts/typed-express-router'; +import logger from '../shared/logger'; +import { customEncodeErrorFormatter } from '../shared/errorFormatters'; + +function makeFakeRequest(): WrappedRequest { + return { + method: 'POST', + originalUrl: '/api/v1/tbtc/advancedwallet/generate', + apiName: 'v1.wallet.generate', + } as unknown as WrappedRequest; +} + +describe('customEncodeErrorFormatter', () => { + afterEach(() => { + sinon.restore(); + }); + + it('logs the real validation error with request context and stack, and returns it in the response body', () => { + const loggerErrorSpy = sinon.spy(logger, 'error'); + const req = makeFakeRequest(); + const err = new Error('response does not match expected type SomeResponseCodec'); + + const body = customEncodeErrorFormatter(err, req) as Record; + + assert.equal(body.error, 'Internal Server Error'); + assert.equal(body.details, 'response does not match expected type SomeResponseCodec'); + + assert.equal(loggerErrorSpy.callCount, 1); + const logged = loggerErrorSpy.firstCall.args[0] as unknown as string; + logged.should.containEql( + 'Failed to encode API response for POST /api/v1/tbtc/advancedwallet/generate (api: v1.wallet.generate)', + ); + logged.should.containEql('response does not match expected type SomeResponseCodec'); + // The stack trace points at the codec check that failed + logged.should.containEql('Error: response does not match expected type SomeResponseCodec'); + }); + + it('falls back to String() for non-Error throwables', () => { + const req = makeFakeRequest(); + + const body = customEncodeErrorFormatter('not an Error', req) as Record; + + assert.equal(body.error, 'Internal Server Error'); + assert.equal(body.details, 'not an Error'); + }); +});