Skip to content

Validate contract ID on transaction deserialization#1349

Merged
quietbits merged 5 commits intomasterfrom
fix-assembled-tx
Mar 25, 2026
Merged

Validate contract ID on transaction deserialization#1349
quietbits merged 5 commits intomasterfrom
fix-assembled-tx

Conversation

@quietbits
Copy link
Copy Markdown
Contributor

  • fromXDR() and fromJSON() in AssembledTransaction now verify that the
    deserialized transaction's contract address matches the Client's configured
    contractId, throwing a clear error on mismatch.
  • Addresses a trust boundary gap where a serialized transaction targeting a
    foreign contract could be silently signed through a Client configured for
    a different contract.
  • Updated .gitignore

Copilot AI review requested due to automatic review settings March 23, 2026 18:30
@github-project-automation github-project-automation bot moved this to Backlog (Not Ready) in DevX Mar 23, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens AssembledTransaction deserialization by ensuring a deserialized transaction’s target contract matches the Client/options contractId, preventing cross-contract transaction signing.

Changes:

  • Add contract ID validation during AssembledTransaction.fromXDR() and AssembledTransaction.fromJSON().
  • Add unit tests covering accept/reject behavior for matching vs. mismatching contract IDs.
  • Update .gitignore to exclude local tooling directories.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/contract/assembled_transaction.ts Adds contract ID checks when reconstructing assembled transactions from XDR/JSON.
test/unit/server/soroban/assembled_transaction.test.ts Adds tests validating the new deserialization checks.
.gitignore Ignores .claude/ and .copilot/ directories.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/contract/assembled_transaction.ts Outdated
Comment thread src/contract/assembled_transaction.ts Outdated
Comment thread src/contract/assembled_transaction.ts Outdated
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 23, 2026

Size Change: +89.5 kB (+0.2%)

Total Size: 45.2 MB

Filename Size Change
dist/stellar-sdk-minimal.js 5.98 MB +14.5 kB (+0.24%)
dist/stellar-sdk-minimal.min.js 5.08 MB +7.86 kB (+0.15%)
dist/stellar-sdk-no-axios.js 5.98 MB +14.5 kB (+0.24%)
dist/stellar-sdk-no-axios.min.js 5.08 MB +7.86 kB (+0.15%)
dist/stellar-sdk-no-eventsource.js 6.23 MB +14.5 kB (+0.23%)
dist/stellar-sdk-no-eventsource.min.js 5.29 MB +7.86 kB (+0.15%)
dist/stellar-sdk.js 6.23 MB +14.5 kB (+0.23%)
dist/stellar-sdk.min.js 5.29 MB +7.86 kB (+0.15%)

compressed-size-action

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/contract/assembled_transaction.ts Outdated
Comment thread src/contract/assembled_transaction.ts Outdated
Comment thread src/contract/assembled_transaction.ts Outdated
Comment thread src/contract/assembled_transaction.ts Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/contract/assembled_transaction.ts Outdated
Comment thread src/contract/assembled_transaction.ts Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@quietbits quietbits requested review from Ryang-21 and Shaptic March 23, 2026 20:22
Comment thread src/contract/assembled_transaction.ts
Comment thread src/contract/assembled_transaction.ts
Copy link
Copy Markdown
Contributor

@Ryang-21 Ryang-21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this fix should target the Client's txFromJson and txFromXdr methods. In that those are were we should be opinionated and enforce validation. I think we could create a static validate function on the AssembledTransaction class that verifies contractIds are the same and the method name is in the spec
Ignore this there is no reason to not have this validation on all AssembledTransaction parsing

@quietbits quietbits requested review from Ryang-21 and Shaptic March 24, 2026 20:28
@quietbits quietbits merged commit b8d5d66 into master Mar 25, 2026
10 checks passed
@quietbits quietbits deleted the fix-assembled-tx branch March 25, 2026 19:06
@github-project-automation github-project-automation bot moved this from Backlog (Not Ready) to Done in DevX Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants