From 32a0eec68cdebb4a1e167174c6afea64ce44eba9 Mon Sep 17 00:00:00 2001 From: Sagar Patil Date: Tue, 17 Mar 2026 13:31:15 -0700 Subject: [PATCH 1/3] Fix mutable tx handle and memo ID validation gap Return a defensive XDR copy from TransactionBase#tx getter to prevent mutation of the internal XDR object after construction, closing a split-brain where displayed fields could diverge from signed bytes. Internal methods now use this._tx directly for signing/serialization. Add regex validation in Memo._validateIdValue to reject non-plain-decimal strings (e.g. "1e18", "1.0") that pass BigNumber checks but crash BigInt() at XDR serialization time. Fixes #160 Co-Authored-By: Claude Opus 4.6 --- src/fee_bump_transaction.js | 6 +++--- src/memo.js | 27 +++++++-------------------- src/transaction.js | 8 ++++---- src/transaction_base.js | 6 +++++- 4 files changed, 19 insertions(+), 28 deletions(-) diff --git a/src/fee_bump_transaction.js b/src/fee_bump_transaction.js index ea91d8b1f..422880721 100644 --- a/src/fee_bump_transaction.js +++ b/src/fee_bump_transaction.js @@ -46,7 +46,7 @@ export class FeeBumpTransaction extends TransactionBase { const innerTxEnvelope = xdr.TransactionEnvelope.envelopeTypeTx( tx.innerTx().v1() ); - this._feeSource = encodeMuxedAccountToAddress(this.tx.feeSource()); + this._feeSource = encodeMuxedAccountToAddress(this._tx.feeSource()); this._innerTransaction = new Transaction( innerTxEnvelope, networkPassphrase @@ -89,7 +89,7 @@ export class FeeBumpTransaction extends TransactionBase { signatureBase() { const taggedTransaction = new xdr.TransactionSignaturePayloadTaggedTransaction.envelopeTypeTxFeeBump( - this.tx + this._tx ); const txSignature = new xdr.TransactionSignaturePayload({ @@ -106,7 +106,7 @@ export class FeeBumpTransaction extends TransactionBase { */ toEnvelope() { const envelope = new xdr.FeeBumpTransactionEnvelope({ - tx: xdr.FeeBumpTransaction.fromXDR(this.tx.toXDR()), // make a copy of the tx + tx: xdr.FeeBumpTransaction.fromXDR(this._tx.toXDR()), // make a copy of the tx signatures: this.signatures.slice() // make a copy of the signatures }); diff --git a/src/memo.js b/src/memo.js index d9d0bfa55..84150072c 100644 --- a/src/memo.js +++ b/src/memo.js @@ -102,6 +102,13 @@ export class Memo { throw error; } + // Reject anything that isn't plain decimal digits (no scientific notation, + // no decimals, no signs). BigNumber accepts formats like "1e18" and "1.0" + // which pass numeric checks but crash BigInt()/UnsignedHyper.fromString(). + if (!/^\d+$/.test(value)) { + throw error; + } + let number; try { number = new BigNumber(value); @@ -109,26 +116,6 @@ export class Memo { throw error; } - // Infinity - if (!number.isFinite()) { - throw error; - } - - // NaN - if (number.isNaN()) { - throw error; - } - - // Negative - if (number.isNegative()) { - throw error; - } - - // Decimal - if (!number.isInteger()) { - throw error; - } - // Exceeds uint64 max (2^64 - 1) if (number.isGreaterThan('18446744073709551615')) { throw error; diff --git a/src/transaction.js b/src/transaction.js index 47d63255b..4abec66a9 100644 --- a/src/transaction.js +++ b/src/transaction.js @@ -62,11 +62,11 @@ export class Transaction extends TransactionBase { switch (this._envelopeType) { case xdr.EnvelopeType.envelopeTypeTxV0(): this._source = StrKey.encodeEd25519PublicKey( - this.tx.sourceAccountEd25519() + this._tx.sourceAccountEd25519() ); break; default: - this._source = encodeMuxedAccountToAddress(this.tx.sourceAccount()); + this._source = encodeMuxedAccountToAddress(this._tx.sourceAccount()); break; } @@ -256,7 +256,7 @@ export class Transaction extends TransactionBase { * @returns {Buffer} */ signatureBase() { - let tx = this.tx; + let tx = this._tx; // Backwards Compatibility: Use ENVELOPE_TYPE_TX to sign ENVELOPE_TYPE_TX_V0 // we need a Transaction to generate the signature base @@ -288,7 +288,7 @@ export class Transaction extends TransactionBase { * @returns {xdr.TransactionEnvelope} */ toEnvelope() { - const rawTx = this.tx.toXDR(); + const rawTx = this._tx.toXDR(); const signatures = this.signatures.slice(); // make a copy of the signatures let envelope; diff --git a/src/transaction_base.js b/src/transaction_base.js index acda6af7c..f92a3608d 100644 --- a/src/transaction_base.js +++ b/src/transaction_base.js @@ -32,7 +32,11 @@ export class TransactionBase { } get tx() { - return this._tx; + // Return a defensive copy to prevent mutation of the internal XDR object + // after construction. Signing and serialization use this._tx directly, + // so exposing the live reference could let callers alter signed bytes + // without changing the cached display fields (fee, source, memo, etc.). + return this._tx.constructor.fromXDR(this._tx.toXDR()); } set tx(value) { From def13701436692759c239491257c9a1b81d0b3b0 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 17 Mar 2026 14:53:52 -0700 Subject: [PATCH 2/3] Add negative test coverage for memo ID scientific notation and trailing decimal zero (#940) * Initial plan * Add negative test cases for memo ID scientific notation and trailing decimal zero Co-authored-by: sagpatil <1414227+sagpatil@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sagpatil <1414227+sagpatil@users.noreply.github.com> --- test/unit/memo_test.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/unit/memo_test.js b/test/unit/memo_test.js index dbb69816b..549d4c927 100644 --- a/test/unit/memo_test.js +++ b/test/unit/memo_test.js @@ -134,6 +134,10 @@ describe('Memo.id()', function () { expect(() => StellarBase.Memo.id('-1')).to.throw(/Expects a uint64/); // decimal expect(() => StellarBase.Memo.id('1.5')).to.throw(/Expects a uint64/); + // trailing decimal zero (BigNumber accepts but must be rejected) + expect(() => StellarBase.Memo.id('1.0')).to.throw(/Expects a uint64/); + // scientific notation (BigNumber accepts but BigInt()/UnsignedHyper crashes) + expect(() => StellarBase.Memo.id('1e18')).to.throw(/Expects a uint64/); // overflow: 2^64 silently became 0 before this fix expect(() => StellarBase.Memo.id('18446744073709551616')).to.throw( /Expects a uint64/ From c0f37caf03666c9ac7c819f033f9e54d78e9a536 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 17 Mar 2026 14:54:07 -0700 Subject: [PATCH 3/3] Add unit test locking in `tx` getter defensive-copy immutability (#941) * Initial plan * Add unit test for tx getter defensive copy behavior Co-authored-by: sagpatil <1414227+sagpatil@users.noreply.github.com> * Rename variable t to txCopy for clarity in tx getter test Co-authored-by: sagpatil <1414227+sagpatil@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sagpatil <1414227+sagpatil@users.noreply.github.com> --- test/unit/transaction_test.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/unit/transaction_test.js b/test/unit/transaction_test.js index 8e0e7d098..3cc0fbc75 100644 --- a/test/unit/transaction_test.js +++ b/test/unit/transaction_test.js @@ -79,6 +79,13 @@ describe('Transaction', function () { const envelope = transaction.toEnvelope().value(); envelope.tx().fee(StellarBase.xdr.Int64.fromString('300')); + expect(transaction.tx.fee().toString()).to.equal('100'); + }); + it('tx getter returns a defensive copy that does not affect internal XDR', function () { + const transaction = this.transaction; + const txCopy = transaction.tx; + txCopy.fee(StellarBase.xdr.Int64.fromString('999')); + expect(transaction.tx.fee().toString()).to.equal('100'); }); });