Merge pull request #8406 from BitGo/CAAS-1186

fix(express): prevent double-stringify of string payloads in handleV2 OFCSignPayload

Author: CapnMigraine

modules/express/src/clientRoutes.ts

@@ -625,7 +625,7 @@ export async function handleV2OFCSignPayload(
 
   const walletPassphrase = bodyWalletPassphrase || getWalletPwFromEnv(wallet.id());
   const tradingAccount = wallet.toTradingAccount();
-  const stringifiedPayload = JSON.stringify(payload);
+  const stringifiedPayload = typeof payload === 'string' ? payload : JSON.stringify(payload);
   const signature = await tradingAccount.signPayload({
     payload: stringifiedPayload,
     walletPassphrase,

modules/express/test/unit/clientRoutes/signPayload.ts

@@ -87,6 +87,32 @@ describe('Sign an arbitrary payload with trading account key', function () {
       throw new Error(`Response did not match expected codec`);
     });
   });
+  it('should not double-stringify when decoded payload is already a string', async function () {
+    // When the io-ts Json codec matches a string input (left-to-right union resolution),
+    // decoded.payload is the original string. The handler must not JSON.stringify it again.
+    const expectedResponse = {
+      payload: stringifiedPayload,
+      signature,
+    };
+    const req = {
+      bitgo: bitGoStub,
+      body: {
+        payload: stringifiedPayload,
+        walletId,
+      },
+      decoded: {
+        walletId,
+        payload: stringifiedPayload,
+      },
+      query: {},
+    } as unknown as ExpressApiRouteRequest<'express.ofc.signPayload', 'post'>;
+    const result = await handleV2OFCSignPayload(req).should.be.resolvedWith(expectedResponse);
+    result.payload.should.equal(stringifiedPayload);
+    result.payload.should.not.startWith('"');
+    decodeOrElse('OfcSignPayloadResponse200', OfcSignPayloadResponse[200], result, (_) => {
+      throw new Error(`Response did not match expected codec`);
+    });
+  });
   it('should decode handler response with OfcSignPayloadResponse codec', async function () {
     const expected = {
       payload: JSON.stringify(payload),

modules/express/test/unit/typedRoutes/ofcSignPayload.ts

@@ -136,6 +136,60 @@ describe('OfcSignPayload codec tests', function () {
       assert.ok(decodedResponse);
     });
 
+    it('should not double-stringify a stringified JSON payload', async function () {
+      const originalPayload = '{"coin":"ofctbtc","recipients":[{"address":"abc123","amount":"1000"}]}';
+      const requestBody = {
+        walletId: 'ofc-wallet-id-123',
+        payload: originalPayload,
+        walletPassphrase: 'test_passphrase',
+      };
+
+      const mockTradingAccount = {
+        signPayload: sinon.stub().resolves(mockSignPayloadResponse.signature),
+      };
+
+      const mockWallet = {
+        id: () => requestBody.walletId,
+        toTradingAccount: sinon.stub().returns(mockTradingAccount),
+      };
+
+      const walletsGetStub = sinon.stub().resolves(mockWallet);
+      const mockWallets = { get: walletsGetStub };
+      const mockCoin = { wallets: sinon.stub().returns(mockWallets) };
+      sinon.stub(BitGo.prototype, 'coin').returns(mockCoin as any);
+
+      const result = await agent
+        .post('/api/v2/ofc/signPayload')
+        .set('Authorization', 'Bearer test_access_token_12345')
+        .set('Content-Type', 'application/json')
+        .send(requestBody);
+
+      assert.strictEqual(result.status, 200);
+      const decodedResponse = assertDecode(OfcSignPayloadResponse200, result.body);
+
+      // The returned payload must not be double-stringified.
+      // A double-stringified payload would start with a quote character and contain escaped quotes.
+      assert.strictEqual(
+        decodedResponse.payload.startsWith('"'),
+        false,
+        'payload was double-stringified: starts with a quote character'
+      );
+      assert.strictEqual(
+        decodedResponse.payload.includes('\\"'),
+        false,
+        'payload was double-stringified: contains escaped quotes'
+      );
+
+      // The payload passed to tradingAccount.signPayload must match the original string
+      const signCall = mockTradingAccount.signPayload.getCall(0);
+      assert.ok(signCall, 'tradingAccount.signPayload should have been called');
+      assert.strictEqual(
+        signCall.args[0].payload,
+        originalPayload,
+        'tradingAccount.signPayload received a different payload than the original'
+      );
+    });
+
     it('should successfully sign payload without walletPassphrase (uses env)', async function () {
       const requestBody = {
         walletId: 'ofc-wallet-id-123',