Fixes cross-wallet txp view

packages/bitcore-wallet-service/src/lib/server.ts

@@ -3139,7 +3139,12 @@ export class WalletService implements IWalletService {
    * @returns {Object} txProposal
    */
   getTxByHash(opts, cb) {
-    this.storage.fetchTxByHash(opts.txid, (err, txp) => {
+    // Scoped to this.walletId: the global storage.fetchTxByHash
+    // would return any wallet's TxProposal for a known txid, disclosing a
+    // foreign wallet's proposal data to any authenticated copayer. The
+    // internal global consumers (BlockchainMonitor, getWalletFromIdentifier)
+    // are unaffected; they keep calling storage.fetchTxByHash directly.
+    this.storage.fetchTxByHashForWallet(this.walletId, opts.txid, (err, txp) => {
       if (err) return cb(err);
       if (!txp) return cb(Errors.TX_NOT_FOUND);
 

packages/bitcore-wallet-service/src/lib/storage.ts

@@ -360,6 +360,25 @@ export class Storage {
     );
   }
 
+  // Wallet-scoped counterpart to fetchTxByHash, used by the authenticated
+  // by-hash API read.
+  fetchTxByHashForWallet(walletId: string, hash, cb: (err?: any, tx?: TxProposal) => void) {
+    if (!this.db) return cb();
+
+    this.db.collection(collections.TXS).findOne(
+      {
+        walletId,
+        txid: hash
+      },
+      (err, result) => {
+        if (err) return cb(err);
+        if (!result) return cb();
+
+        return this._completeTxData(walletId, TxProposal.fromObj(result), cb);
+      }
+    );
+  }
+
   fetchLastTxs(walletId, creatorId, limit, cb) {
     this.db
       .collection(collections.TXS)

packages/bitcore-wallet-service/test/integration/server.test.ts

@@ -4,6 +4,8 @@ import * as chai from 'chai';
 import 'chai/register-should';
 import util from 'util';
 import sinon from 'sinon';
+import http from 'http';
+import request from 'request';
 import * as CWC from '@bitpay-labs/crypto-wallet-core';
 import { ChainService } from '../../src/lib/chain/index';
 import config from '../../src/config';
@@ -15,6 +17,7 @@ import { BCHAddressTranslator } from '../../src/lib/bchaddresstranslator';
 import * as TestData from '../testdata';
 import helpers from './helpers';
 import { ClientError } from '../../src/lib/errors/clienterror';
+import { ExpressApp } from '../../src/lib/expressapp';
 
 const should = chai.should();
 config.moralis = config.moralis ?? {
@@ -9030,10 +9033,202 @@ describe('Wallet service', function() {
     });
 
     it.skip('should get accepted/rejected transaction proposal', function(done) { });
-    
+
     it.skip('should get broadcasted transaction proposal', function(done) { });
   });
 
+  describe('#getTxByHash', function() {
+    let serverA: WalletService;
+    let walletA: Model.Wallet;
+    let serverB: WalletService;
+    let txp;
+
+    beforeEach(async function() {
+      ({ server: serverA, wallet: walletA } = await helpers.createAndJoinWallet(1, 1));
+      await helpers.stubUtxos(serverA, walletA, 1);
+      const txOpts = {
+        outputs: [{
+          toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
+          amount: 0.5e8
+        }],
+        feePerKb: 100e2,
+        message: 'some message',
+      };
+      txp = await helpers.createAndPublishTx(serverA, txOpts, TestData.copayers[0].privKey_1H_0);
+      should.exist(txp);
+
+      const signatures = helpers.clientSign(txp, TestData.copayers[0].xPrivKey_44H_0H_0H);
+      txp = await util.promisify(serverA.signTx).call(serverA, {
+        txProposalId: txp.id,
+        signatures,
+      });
+      should.exist(txp.txid);
+
+      ({ server: serverB } = await helpers.createAndJoinWallet(1, 1, { offset: 1 }));
+    });
+
+    it('should not disclose another wallet\'s transaction proposal by hash', function(done) {
+      serverB.getTxByHash({
+        txid: txp.txid
+      }, function(err, res) {
+        should.exist(err);
+        should.not.exist(res);
+        err.should.be.instanceof(ClientError);
+        err.code.should.equal('TX_NOT_FOUND');
+        err.message.should.equal('Transaction proposal not found');
+        // Belt-and-suspenders: confirm the error itself carries none of
+        // wallet A's data (no walletId/creatorId/raw leaking via the error).
+        JSON.stringify(err).should.not.include(walletA.id);
+        done();
+      });
+    });
+
+    it('should get own transaction proposal by hash, unchanged from before the fix', function(done) {
+      serverA.editTxNote({
+        txid: txp.txid,
+        body: 'a note from wallet A'
+      }, function(err) {
+        should.not.exist(err);
+        serverA.getTxByHash({
+          txid: txp.txid
+        }, function(err, res) {
+          should.not.exist(err);
+          should.exist(res);
+          res.id.should.equal(txp.id);
+          res.walletId.should.equal(walletA.id);
+          res.txid.should.equal(txp.txid);
+          should.exist(res.raw);
+          should.exist(res.note);
+          res.note.body.should.equal('a note from wallet A');
+          done();
+        });
+      });
+    });
+
+    it('should return the identical not-found error for a foreign txid and an unknown txid', function(done) {
+      serverB.getTxByHash({
+        txid: txp.txid
+      }, function(errForeign, resForeign) {
+        should.exist(errForeign);
+        should.not.exist(resForeign);
+        serverB.getTxByHash({
+          txid: helpers.randomTXID()
+        }, function(errUnknown, resUnknown) {
+          should.exist(errUnknown);
+          should.not.exist(resUnknown);
+          errForeign.code.should.equal(errUnknown.code);
+          errForeign.message.should.equal(errUnknown.message);
+          errForeign.code.should.equal('TX_NOT_FOUND');
+          done();
+        });
+      });
+    });
+  });
+
+  describe('GET /v1/txproposalsbyhash/:id/ (HTTP route)', function() {
+    // Exercises the real Express route (registerTransactionRoutes ->
+    // getServerWithAuth -> WalletService.getInstanceWithAuth), not just the
+    // service method, so route wiring, request-auth plumbing, and HTTP
+    // status/body serialization are covered, not only the underlying
+    // service logic already tested above. Only the cryptographic signature
+    // check is stubbed to return true
+    const testPort = 3240;
+    const testHost = 'http://127.0.0.1';
+    let httpServer;
+    let verifyStub;
+    let walletA: Model.Wallet;
+    let walletB: Model.Wallet;
+    let txp;
+
+    beforeEach(async function() {
+      let serverA: WalletService;
+      let serverB: WalletService;
+      ({ server: serverA, wallet: walletA } = await helpers.createAndJoinWallet(1, 1));
+      await helpers.stubUtxos(serverA, walletA, 1);
+      const txOpts = {
+        outputs: [{
+          toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
+          amount: 0.5e8
+        }],
+        feePerKb: 100e2,
+        message: 'some message',
+      };
+      txp = await helpers.createAndPublishTx(serverA, txOpts, TestData.copayers[0].privKey_1H_0);
+      const signatures = helpers.clientSign(txp, TestData.copayers[0].xPrivKey_44H_0H_0H);
+      txp = await util.promisify(serverA.signTx).call(serverA, {
+        txProposalId: txp.id,
+        signatures,
+      });
+      should.exist(txp.txid);
+
+      ({ server: serverB, wallet: walletB } = await helpers.createAndJoinWallet(1, 1, { offset: 1 }));
+
+      verifyStub = sinon.stub(WalletService.prototype, '_verifySignature').returns(true);
+
+      const app = new ExpressApp();
+      httpServer = new http.Server(app.app);
+      await util.promisify(app.start).call(app, {
+        storage: helpers.getStorage(),
+        blockchainExplorer: helpers.getBlockchainExplorer(),
+        request: sinon.stub(),
+        disableLogs: true,
+        basePath: config.basePath
+      });
+      httpServer.listen(testPort);
+    });
+
+    afterEach(function() {
+      verifyStub.restore();
+      httpServer.close();
+    });
+
+    function getByHash(copayerId, txid, cb) {
+      request({
+        method: 'GET',
+        url: testHost + ':' + testPort + config.basePath + '/v1/txproposalsbyhash/' + txid + '/',
+        headers: {
+          'x-identity': copayerId,
+          'x-signature': 'stubbed'
+        },
+        json: true
+      }, (err, res, body) => cb(err, res, body));
+    }
+
+    it('foreign wallet gets HTTP 400 TX_NOT_FOUND with no owner data in the body', function(done) {
+      getByHash(walletB.copayers[0].id, txp.txid, (err, res, body) => {
+        should.not.exist(err);
+        res.statusCode.should.equal(400);
+        body.code.should.equal('TX_NOT_FOUND');
+        JSON.stringify(body).should.not.include(walletA.id);
+        done();
+      });
+    });
+
+    it('owner wallet gets HTTP 200 with the full proposal, unchanged', function(done) {
+      getByHash(walletA.copayers[0].id, txp.txid, (err, res, body) => {
+        should.not.exist(err);
+        res.statusCode.should.equal(200);
+        body.walletId.should.equal(walletA.id);
+        body.txid.should.equal(txp.txid);
+        should.exist(body.raw);
+        done();
+      });
+    });
+
+    it('foreign and unknown txids produce an identical HTTP 400 TX_NOT_FOUND body', function(done) {
+      getByHash(walletB.copayers[0].id, txp.txid, (err, resForeign, bodyForeign) => {
+        should.not.exist(err);
+        getByHash(walletB.copayers[0].id, helpers.randomTXID(), (err, resUnknown, bodyUnknown) => {
+          should.not.exist(err);
+          resForeign.statusCode.should.equal(resUnknown.statusCode);
+          bodyForeign.code.should.equal(bodyUnknown.code);
+          bodyForeign.code.should.equal('TX_NOT_FOUND');
+          done();
+        });
+      });
+    });
+  });
+
   describe('#getTxs', function() {
     let server: WalletService;
     let wallet: Model.Wallet;

packages/bitcore-wallet-service/test/storage.test.ts

@@ -200,6 +200,97 @@ describe('Storage', function() {
       });
     });
 
+    it('should fetch tx by hash scoped to wallet, disambiguating duplicate txids across wallets', async function() {
+      // A second, unrelated wallet with its own proposal reusing 'txid0'. The
+      // txid index is non-unique, so this can happen in production; the
+      // scoped method must still resolve deterministically per walletId
+      // while the legacy global fetchTxByHash contract stays unchanged.
+      const otherWallet = Model.Wallet.create({
+        id: '456',
+        name: 'other wallet',
+        m: 1,
+        n: 1,
+        coin: 'btc',
+        chain: 'btc',
+        network: 'livenet',
+        pubKey: '',
+        singleAddress: false,
+        derivationStrategy: 'BIP45',
+        addressType: 'P2SH',
+      });
+      const otherCopayer = Model.Copayer.create({
+        coin: 'btc',
+        name: 'other copayer',
+        xPubKey: 'other xPubKey',
+        requestPubKey: 'other requestPubKey',
+        signature: 'other signature',
+      });
+      otherWallet.addCopayer(otherCopayer);
+      await util.promisify(storage.storeWalletAndUpdateCopayersLookup).call(storage, otherWallet);
+
+      const otherTx = Model.TxProposal.create({
+        walletId: '456',
+        coin: 'btc',
+        network: 'livenet',
+        outputs: [{
+          toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
+          amount: 999,
+        }],
+        feePerKb: 100e2,
+        creatorId: otherWallet.copayers[0].id,
+      });
+      otherTx.status = 'pending';
+      otherTx.txid = 'txid0';
+      await util.promisify(storage.storeTx).call(storage, '456', otherTx);
+
+      // Global lookup is unchanged: it still finds *a* proposal with this
+      // txid (which one is not guaranteed with duplicates), proving the
+      // existing internal-caller contract was not altered.
+      const globalTx = await util.promisify(storage.fetchTxByHash).call(storage, 'txid0');
+      should.exist(globalTx);
+      globalTx.txid.should.equal('txid0');
+
+      // The scoped method resolves the correct wallet's proposal regardless
+      // of the duplicate.
+      const txForWalletA = await util.promisify(storage.fetchTxByHashForWallet).call(storage, '123', 'txid0');
+      should.exist(txForWalletA);
+      txForWalletA.walletId.should.equal('123');
+      txForWalletA.id.should.equal(proposals[0].id);
+
+      const txForWalletB = await util.promisify(storage.fetchTxByHashForWallet).call(storage, '456', 'txid0');
+      should.exist(txForWalletB);
+      txForWalletB.walletId.should.equal('456');
+      txForWalletB.id.should.equal(otherTx.id);
+
+      // A wallet with no proposal for this txid gets nothing (not the other
+      // wallet's data) -- proving the query is scoped, not just filtered
+      // client-side.
+      const otherWallet2 = Model.Wallet.create({
+        id: '789',
+        name: 'third wallet',
+        m: 1,
+        n: 1,
+        coin: 'btc',
+        chain: 'btc',
+        network: 'livenet',
+        pubKey: '',
+        singleAddress: false,
+        derivationStrategy: 'BIP45',
+        addressType: 'P2SH',
+      });
+      const thirdCopayer = Model.Copayer.create({
+        coin: 'btc',
+        name: 'third copayer',
+        xPubKey: 'third xPubKey',
+        requestPubKey: 'third requestPubKey',
+        signature: 'third signature',
+      });
+      otherWallet2.addCopayer(thirdCopayer);
+      await util.promisify(storage.storeWalletAndUpdateCopayersLookup).call(storage, otherWallet2);
+      const noTx = await util.promisify(storage.fetchTxByHashForWallet).call(storage, '789', 'txid0');
+      should.not.exist(noTx);
+    });
+
     it('should fetch all pending txs', function(done) {
       storage.fetchPendingTxs('123', function(err, txs) {
         should.not.exist(err);