From 8c26f993c0f11217472db07a4c3b455b9ffab127 Mon Sep 17 00:00:00 2001 From: Michael Jay Date: Thu, 20 Aug 2026 23:05:01 -0400 Subject: [PATCH 1/3] implement wallet-id gated GET txp --- .../bitcore-wallet-service/src/lib/server.ts | 7 ++++++- .../bitcore-wallet-service/src/lib/storage.ts | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/bitcore-wallet-service/src/lib/server.ts b/packages/bitcore-wallet-service/src/lib/server.ts index 264fbd7169..20903bc569 100644 --- a/packages/bitcore-wallet-service/src/lib/server.ts +++ b/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); diff --git a/packages/bitcore-wallet-service/src/lib/storage.ts b/packages/bitcore-wallet-service/src/lib/storage.ts index 92a547d7cd..ee3f81b93c 100644 --- a/packages/bitcore-wallet-service/src/lib/storage.ts +++ b/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) From 4a41d35161a7f9bce8bd4a2afa20637db8d3c11b Mon Sep 17 00:00:00 2001 From: Michael Jay Date: Thu, 20 Aug 2026 23:09:47 -0400 Subject: [PATCH 2/3] implement wallet-id gated GET txp tests --- .../test/integration/server.test.ts | 214 +++++++++++++++++- .../test/storage.test.ts | 91 ++++++++ 2 files changed, 304 insertions(+), 1 deletion(-) diff --git a/packages/bitcore-wallet-service/test/integration/server.test.ts b/packages/bitcore-wallet-service/test/integration/server.test.ts index 7cd974da4c..5727d62f4e 100644 --- a/packages/bitcore-wallet-service/test/integration/server.test.ts +++ b/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,219 @@ 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() { + // Two unrelated wallets (the multi-wallet `{ offset: 1 }` pattern used + // elsewhere in this file, e.g. 'should delete a wallet, and only that + // wallet') so wallet B has no legitimate relationship to wallet A's + // TxProposal. + 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); + // Sign to accepted status, which is when TxProposal#sign populates + // `txid`/`raw` (Phase 1 finding); no broadcast is required. signTx's + // callback returns the updated txp, since the local `txp` reference + // above predates signing. + 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) { + // Also covers the note: getTx's sibling reader attaches the caller's + // own note to the response, and that attachment is unconditional on + // ownership (it always used this.walletId) -- confirm the fix didn't + // disturb it. + 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 (matching helpers.getAuthServer's + // established pattern) -- copayer/wallet lookup, ownership scoping, and + // JSON serialization all run for real, against the same real storage + // instance the rest of this file uses. + 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 })); + + // Bypass only the crypto signature check, same as helpers.getAuthServer; + // the real ExpressApp/WalletService/Storage stack does everything else, + // including the actual walletId scoping under test. + 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; diff --git a/packages/bitcore-wallet-service/test/storage.test.ts b/packages/bitcore-wallet-service/test/storage.test.ts index a87b811e4a..948d38b0ac 100644 --- a/packages/bitcore-wallet-service/test/storage.test.ts +++ b/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); From 19a04645226b39508be1231adf4ef2aa83d764ad Mon Sep 17 00:00:00 2001 From: Michael Jay Date: Thu, 20 Aug 2026 23:10:06 -0400 Subject: [PATCH 3/3] clean up comments --- .../test/integration/server.test.ts | 21 ++----------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/packages/bitcore-wallet-service/test/integration/server.test.ts b/packages/bitcore-wallet-service/test/integration/server.test.ts index 5727d62f4e..c77b5d8f5b 100644 --- a/packages/bitcore-wallet-service/test/integration/server.test.ts +++ b/packages/bitcore-wallet-service/test/integration/server.test.ts @@ -9038,10 +9038,6 @@ describe('Wallet service', function() { }); describe('#getTxByHash', function() { - // Two unrelated wallets (the multi-wallet `{ offset: 1 }` pattern used - // elsewhere in this file, e.g. 'should delete a wallet, and only that - // wallet') so wallet B has no legitimate relationship to wallet A's - // TxProposal. let serverA: WalletService; let walletA: Model.Wallet; let serverB: WalletService; @@ -9060,10 +9056,7 @@ describe('Wallet service', function() { }; txp = await helpers.createAndPublishTx(serverA, txOpts, TestData.copayers[0].privKey_1H_0); should.exist(txp); - // Sign to accepted status, which is when TxProposal#sign populates - // `txid`/`raw` (Phase 1 finding); no broadcast is required. signTx's - // callback returns the updated txp, since the local `txp` reference - // above predates signing. + const signatures = helpers.clientSign(txp, TestData.copayers[0].xPrivKey_44H_0H_0H); txp = await util.promisify(serverA.signTx).call(serverA, { txProposalId: txp.id, @@ -9091,10 +9084,6 @@ describe('Wallet service', function() { }); it('should get own transaction proposal by hash, unchanged from before the fix', function(done) { - // Also covers the note: getTx's sibling reader attaches the caller's - // own note to the response, and that attachment is unconditional on - // ownership (it always used this.walletId) -- confirm the fix didn't - // disturb it. serverA.editTxNote({ txid: txp.txid, body: 'a note from wallet A' @@ -9142,10 +9131,7 @@ describe('Wallet service', function() { // 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 (matching helpers.getAuthServer's - // established pattern) -- copayer/wallet lookup, ownership scoping, and - // JSON serialization all run for real, against the same real storage - // instance the rest of this file uses. + // check is stubbed to return true const testPort = 3240; const testHost = 'http://127.0.0.1'; let httpServer; @@ -9177,9 +9163,6 @@ describe('Wallet service', function() { ({ server: serverB, wallet: walletB } = await helpers.createAndJoinWallet(1, 1, { offset: 1 })); - // Bypass only the crypto signature check, same as helpers.getAuthServer; - // the real ExpressApp/WalletService/Storage stack does everything else, - // including the actual walletId scoping under test. verifyStub = sinon.stub(WalletService.prototype, '_verifySignature').returns(true); const app = new ExpressApp();