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) diff --git a/packages/bitcore-wallet-service/test/integration/server.test.ts b/packages/bitcore-wallet-service/test/integration/server.test.ts index 7cd974da4c..c77b5d8f5b 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,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; 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);