Skip to content

Commit 7133bdf

Browse files
pranavjain97claude
pranavjain97
and
claude
committed
fix: scope keychain validation to wallet type (WCN-84)
Only validate commonKeychain for TSS wallets and pubkey for multisig wallets. Previously, sending an irrelevant commonKeychain param for a multisig wallet would trigger a misleading mismatch error instead of being ignored. - Reuse isTss variable to avoid calling multisigType() twice - Add tests for scoped validation and ignoring irrelevant params - Update existing TSS tests to send commonKeychain instead of pubkey Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent b774030 commit 7133bdf

3 files changed

Lines changed: 145 additions & 38 deletions

File tree

‎src/__tests__/api/master/sendMany.test.ts‎

Lines changed: 113 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ describe('POST /api/v1/:coin/advancedwallet/:walletId/sendMany', () => {
290290
},
291291
],
292292
source: 'user',
293-
pubkey: 'xpub_user',
293+
commonKeychain: 'test-common-keychain',
294294
});
295295

296296
response.status.should.equal(200);
@@ -379,7 +379,7 @@ describe('POST /api/v1/:coin/advancedwallet/:walletId/sendMany', () => {
379379
},
380380
],
381381
source: 'user',
382-
pubkey: 'xpub_user',
382+
commonKeychain: 'test-common-keychain',
383383
});
384384

385385
response.status.should.equal(200);
@@ -453,7 +453,7 @@ describe('POST /api/v1/:coin/advancedwallet/:walletId/sendMany', () => {
453453
type: 'fillNonce',
454454
nonce: '2',
455455
source: 'user',
456-
pubkey: 'xpub_user',
456+
commonKeychain: 'test-common-keychain',
457457
});
458458

459459
response.status.should.equal(200);
@@ -516,7 +516,7 @@ describe('POST /api/v1/:coin/advancedwallet/:walletId/sendMany', () => {
516516
},
517517
],
518518
source: 'backup',
519-
pubkey: 'xpub_backup',
519+
commonKeychain: 'test-common-keychain',
520520
});
521521

522522
response.status.should.equal(400);
@@ -775,17 +775,17 @@ describe('POST /api/v1/:coin/advancedwallet/:walletId/sendMany', () => {
775775
signNock.done();
776776
});
777777

778-
it('should throw an error when neither the pubkey nor the commonKeychain is provided', async () => {
778+
it('should throw error when pubkey is missing for multisig wallet', async () => {
779779
const walletGetNock = nock(bitgoApiUrl)
780780
.get(`/api/v2/${coin}/wallet/${walletId}`)
781781
.matchHeader('any', () => true)
782782
.reply(200, {
783783
id: walletId,
784784
type: 'advanced',
785785
keys: ['user-key-id', 'backup-key-id', 'bitgo-key-id'],
786+
multisigType: 'onchain',
786787
});
787788

788-
// Mock keychain get request
789789
const keychainGetNock = nock(bitgoApiUrl)
790790
.get(`/api/v2/${coin}/key/user-key-id`)
791791
.matchHeader('any', () => true)
@@ -798,24 +798,121 @@ describe('POST /api/v1/:coin/advancedwallet/:walletId/sendMany', () => {
798798
.post(`/api/v1/${coin}/advancedwallet/${walletId}/sendMany`)
799799
.set('Authorization', `Bearer ${accessToken}`)
800800
.send({
801-
recipients: [
802-
{
803-
address: 'tb1qtest1',
804-
amount: '100000',
805-
},
806-
],
801+
recipients: [{ address: 'tb1qtest1', amount: '100000' }],
802+
source: 'user',
803+
});
804+
805+
response.status.should.equal(400);
806+
response.body.should.have.property('error');
807+
response.body.error.should.equal('BadRequestError');
808+
response.body.details.should.equal('pubkey must be provided for multisig user signing');
809+
810+
walletGetNock.done();
811+
keychainGetNock.done();
812+
});
813+
814+
it('should throw error when commonKeychain is missing for TSS wallet', async () => {
815+
const walletGetNock = nock(bitgoApiUrl)
816+
.get(`/api/v2/${coin}/wallet/${walletId}`)
817+
.matchHeader('any', () => true)
818+
.reply(200, {
819+
id: walletId,
820+
type: 'advanced',
821+
keys: ['user-key-id', 'backup-key-id', 'bitgo-key-id'],
822+
multisigType: 'tss',
823+
});
824+
825+
const keychainGetNock = nock(bitgoApiUrl)
826+
.get(`/api/v2/${coin}/key/user-key-id`)
827+
.matchHeader('any', () => true)
828+
.reply(200, {
829+
id: 'user-key-id',
830+
pub: 'xpub_user',
831+
commonKeychain: 'test-common-keychain',
832+
});
833+
834+
const multisigTypeStub = sinon.stub(Wallet.prototype, 'multisigType').returns('tss');
835+
836+
const response = await agent
837+
.post(`/api/v1/${coin}/advancedwallet/${walletId}/sendMany`)
838+
.set('Authorization', `Bearer ${accessToken}`)
839+
.send({
840+
recipients: [{ address: 'tb1qtest1', amount: '100000' }],
807841
source: 'user',
808842
});
809843

810-
console.log(response.body);
811844
response.status.should.equal(400);
812845
response.body.should.have.property('error');
813846
response.body.error.should.equal('BadRequestError');
814-
response.body.details.should.equal(
815-
'Either pubkey or commonKeychain must be provided for user signing',
816-
);
847+
response.body.details.should.equal('commonKeychain must be provided for TSS user signing');
817848

818849
walletGetNock.done();
819850
keychainGetNock.done();
851+
sinon.assert.calledOnce(multisigTypeStub);
852+
});
853+
854+
it('should ignore commonKeychain param for multisig wallet', async () => {
855+
const walletGetNock = nock(bitgoApiUrl)
856+
.get(`/api/v2/${coin}/wallet/${walletId}`)
857+
.matchHeader('any', () => true)
858+
.reply(200, {
859+
id: walletId,
860+
type: 'advanced',
861+
keys: ['user-key-id', 'backup-key-id', 'bitgo-key-id'],
862+
multisigType: 'onchain',
863+
});
864+
865+
const keychainGetNock = nock(bitgoApiUrl)
866+
.get(`/api/v2/${coin}/key/user-key-id`)
867+
.matchHeader('any', () => true)
868+
.reply(200, {
869+
id: 'user-key-id',
870+
pub: 'xpub_user',
871+
});
872+
873+
const prebuildStub = sinon.stub(Wallet.prototype, 'prebuildTransaction').resolves({
874+
txHex: 'prebuilt-tx-hex',
875+
txInfo: { nP2SHInputs: 1, nSegwitInputs: 0, nOutputs: 2 },
876+
walletId,
877+
});
878+
879+
const verifyStub = sinon.stub(Tbtc.prototype, 'verifyTransaction').resolves(true);
880+
881+
const signNock = nock(advancedWalletManagerUrl)
882+
.post(`/api/${coin}/multisig/sign`)
883+
.reply(200, {
884+
halfSigned: {
885+
txHex: 'signed-tx-hex',
886+
txInfo: { nP2SHInputs: 1, nSegwitInputs: 0, nOutputs: 2 },
887+
},
888+
walletId: 'test-wallet-id',
889+
source: 'user',
890+
pub: 'xpub_user',
891+
});
892+
893+
const submitNock = nock(bitgoApiUrl)
894+
.post(`/api/v2/${coin}/wallet/${walletId}/tx/send`)
895+
.matchHeader('any', () => true)
896+
.reply(200, { txid: 'test-tx-id', status: 'signed' });
897+
898+
const response = await agent
899+
.post(`/api/v1/${coin}/advancedwallet/${walletId}/sendMany`)
900+
.set('Authorization', `Bearer ${accessToken}`)
901+
.send({
902+
recipients: [{ address: 'tb1qtest1', amount: '100000' }],
903+
source: 'user',
904+
pubkey: 'xpub_user',
905+
commonKeychain: 'some-irrelevant-value',
906+
});
907+
908+
response.status.should.equal(200);
909+
response.body.should.have.property('txid', 'test-tx-id');
910+
911+
walletGetNock.done();
912+
keychainGetNock.done();
913+
sinon.assert.calledOnce(prebuildStub);
914+
sinon.assert.calledOnce(verifyStub);
915+
signNock.done();
916+
submitNock.done();
820917
});
821918
});

‎src/masterBitgoExpress/handlers/handleSendMany.ts‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -103,25 +103,31 @@ export async function handleSendMany(req: MasterApiSpecRouteRequest<'v1.wallet.s
103103
if (!signingKeychain) {
104104
throw new NotFoundError(`Signing keychain for ${params.source} not found`);
105105
}
106-
if (!params.pubkey && !params.commonKeychain) {
107-
throw new BadRequestError(
108-
`Either pubkey or commonKeychain must be provided for ${params.source} signing`,
109-
);
110-
}
111-
if (params.pubkey && signingKeychain.pub !== params.pubkey) {
112-
throw new BadRequestError(
113-
`Pub provided does not match the keychain on wallet for ${params.source}`,
114-
);
115-
}
116-
if (params.commonKeychain && signingKeychain.commonKeychain !== params.commonKeychain) {
117-
throw new BadRequestError(
118-
`Common keychain provided does not match the keychain on wallet for ${params.source}`,
119-
);
106+
const isTss = wallet.multisigType() === 'tss';
107+
108+
if (isTss) {
109+
if (!params.commonKeychain) {
110+
throw new BadRequestError(`commonKeychain must be provided for TSS ${params.source} signing`);
111+
}
112+
if (signingKeychain.commonKeychain !== params.commonKeychain) {
113+
throw new BadRequestError(
114+
`Common keychain provided does not match the keychain on wallet for ${params.source}`,
115+
);
116+
}
117+
} else {
118+
if (!params.pubkey) {
119+
throw new BadRequestError(`pubkey must be provided for multisig ${params.source} signing`);
120+
}
121+
if (signingKeychain.pub !== params.pubkey) {
122+
throw new BadRequestError(
123+
`Pub provided does not match the keychain on wallet for ${params.source}`,
124+
);
125+
}
120126
}
121127

122128
try {
123129
// Create MPC send parameters with custom signing functions
124-
if (wallet.multisigType() === 'tss') {
130+
if (isTss) {
125131
if (signingKeychain.source === 'backup') {
126132
throw new BadRequestError('Backup MPC signing not supported for sendMany');
127133
}

‎src/masterBitgoExpress/handlers/utils/utils.ts‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,18 @@ export async function getWalletAndSigningKeychain({
4141
throw new Error(`Signing keychain for ${params.source} not found`);
4242
}
4343

44-
if (params.pubkey && params.pubkey !== signingKeychain.pub) {
45-
throw new Error(`Pub provided does not match the keychain on wallet for ${params.source}`);
46-
}
44+
const isTss = wallet.multisigType() === 'tss';
4745

48-
if (params.commonKeychain && signingKeychain.commonKeychain !== params.commonKeychain) {
49-
throw new Error(
50-
`Common keychain provided does not match the keychain on wallet for ${params.source}`,
51-
);
46+
if (isTss) {
47+
if (params.commonKeychain && signingKeychain.commonKeychain !== params.commonKeychain) {
48+
throw new Error(
49+
`Common keychain provided does not match the keychain on wallet for ${params.source}`,
50+
);
51+
}
52+
} else {
53+
if (params.pubkey && params.pubkey !== signingKeychain.pub) {
54+
throw new Error(`Pub provided does not match the keychain on wallet for ${params.source}`);
55+
}
5256
}
5357

5458
return { baseCoin, wallet, signingKeychain };

0 commit comments

Comments
 (0)