feat: Validate signature expiration ledger before submitting auth ent…#202
Conversation
📝 WalkthroughWalkthroughIntroduces signature expiration validation to SmartWalletService by checking Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/wallet/src/smart-wallet.service.ts (3)
487-519:⚠️ Potential issue | 🟠 MajorRe-read
currentLedgerafter simulation in these three flows.The value passed into
validateSignatureExpiration()here was fetched before the RPC simulation and is also being used to fake the source-account sequence. If the ledger advances during that round-trip, auth entries that are already inside the configured buffer can still pass validation here and then fail on-chain. Keep the pre-simulationsequenceforTransactionBuilder, but fetchgetLatestLedger()again aftersimulateTransaction()succeeds and validate against that fresh ledger.🛠️ Suggested change
const simResult = await this.server.simulateTransaction(invokeTx); if (Api.isSimulationError(simResult)) { throw new Error(`...`); } if (!simResult.result?.auth?.length) { throw new Error('...'); } const authEntry: xdr.SorobanAuthorizationEntry = simResult.result.auth[0]; -this.validateSignatureExpiration(authEntry, sequence); +const { sequence: currentLedger } = await this.server.getLatestLedger(); +this.validateSignatureExpiration(authEntry, currentLedger);Apply the same post-simulation refresh in
addSigner(),addSessionSigner(), andremoveSigner().Also applies to: 603-638, 744-777
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 487 - 519, The pre-simulation ledger sequence is used to build the fake source account but must be refreshed after simulation before calling validateSignatureExpiration to avoid race conditions; keep using the original sequence for TransactionBuilder/Transaction construction, but immediately after simulateTransaction() succeeds (and before accessing auth/result entries) call this.server.getLatestLedger() again and pass that fresh ledger.sequence into validateSignatureExpiration(authEntry, freshSequence); apply this change in the three flows/functions addSigner, addSessionSigner, and removeSigner, keeping TransactionBuilder usage unchanged and only swapping the sequence value used for validateSignatureExpiration.
439-455:⚠️ Potential issue | 🟠 MajorDon't downgrade
SignatureExpiredExceptionto a plainErrorinaddSigner().When the legacy session path delegates to
addSessionSigner(), this catch wraps everyErrorin a newError. That stripsinstanceof SignatureExpiredExceptionand dropsexpirationLedger/currentLedger, so callers on theaddSigner()path can't observe the new typed failure. Preserve the original object and only rewrite the message text if needed.🛠️ Suggested change
} catch (error) { if (error instanceof Error) { - throw new Error( - error.message.replace(/addSessionSigner/g, 'addSigner') - ); + error.message = error.message.replace(/addSessionSigner/g, 'addSigner'); } throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 439 - 455, The catch currently wraps every Error in a new Error which loses typed exceptions like SignatureExpiredException and their properties; update the catch in the isLegacySessionPath branch so it preserves the original error object: if error is an instance of SignatureExpiredException rethrow it as-is; otherwise if error is an Error mutate its message in-place (e.g. error.message = error.message.replace(/addSessionSigner/g, 'addSigner')) and then throw the original error; for non-Error throwables keep throwing them unchanged; this preserves SignatureExpiredException, its instanceof check and fields while still normalizing the message for callers of addSigner().
371-384:⚠️ Potential issue | 🟡 MinorValidate
expirationBufferLedgersbefore storing it.This is a new public runtime option, but
NaN, negative values, or stringly JS inputs change thecurrentLedger + expirationBufferLedgerscomparison in surprising ways. That can silently disable the safeguard or make it reject everything. A small constructor guard would keep the feature safe for JS/env-configured callers.🛠️ Suggested change
) { + if ( + !Number.isInteger(expirationBufferLedgers) || + expirationBufferLedgers < 0 + ) { + throw new Error( + 'expirationBufferLedgers must be a non-negative integer' + ); + } this.server = new Server(rpcUrl); this.credentialBackend = credentialBackend;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/wallet/src/smart-wallet.service.ts` around lines 371 - 384, Validate and sanitize the constructor parameter expirationBufferLedgers before storing it: in the SmartWalletService constructor, coerce the incoming expirationBufferLedgers (from the parameter list) to a safe integer (e.g., Number(...) then Math.floor) and ensure it is a finite non-negative number; if it is NaN, non-finite, negative, or not a number, fallback to the default value (10) or throw a clear error, and then assign the sanitized value to this.expirationBufferLedgers so downstream comparisons (e.g., currentLedger + expirationBufferLedgers) behave predictably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/wallet/src/smart-wallet.service.ts`:
- Around line 487-519: The pre-simulation ledger sequence is used to build the
fake source account but must be refreshed after simulation before calling
validateSignatureExpiration to avoid race conditions; keep using the original
sequence for TransactionBuilder/Transaction construction, but immediately after
simulateTransaction() succeeds (and before accessing auth/result entries) call
this.server.getLatestLedger() again and pass that fresh ledger.sequence into
validateSignatureExpiration(authEntry, freshSequence); apply this change in the
three flows/functions addSigner, addSessionSigner, and removeSigner, keeping
TransactionBuilder usage unchanged and only swapping the sequence value used for
validateSignatureExpiration.
- Around line 439-455: The catch currently wraps every Error in a new Error
which loses typed exceptions like SignatureExpiredException and their
properties; update the catch in the isLegacySessionPath branch so it preserves
the original error object: if error is an instance of SignatureExpiredException
rethrow it as-is; otherwise if error is an Error mutate its message in-place
(e.g. error.message = error.message.replace(/addSessionSigner/g, 'addSigner'))
and then throw the original error; for non-Error throwables keep throwing them
unchanged; this preserves SignatureExpiredException, its instanceof check and
fields while still normalizing the message for callers of addSigner().
- Around line 371-384: Validate and sanitize the constructor parameter
expirationBufferLedgers before storing it: in the SmartWalletService
constructor, coerce the incoming expirationBufferLedgers (from the parameter
list) to a safe integer (e.g., Number(...) then Math.floor) and ensure it is a
finite non-negative number; if it is NaN, non-finite, negative, or not a number,
fallback to the default value (10) or throw a clear error, and then assign the
sanitized value to this.expirationBufferLedgers so downstream comparisons (e.g.,
currentLedger + expirationBufferLedgers) behave predictably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31d86f99-e5ff-4daf-ba98-2b61828c4c36
📒 Files selected for processing (3)
packages/core/wallet/src/smart-wallet.service.tspackages/core/wallet/src/tests/smart-wallet.service.test.tspackages/core/wallet/src/types/smart-wallet.types.ts
2 participants
Pull Request
📋 Description
Validates
signatureExpirationLedgeron Soroban auth entries before assembling and returning signed transaction XDR. A newSignatureExpiredExceptionis thrown early (at signing time) when a signature has already expired or falls within a configurable ledger buffer, giving developers a clear, actionable error instead of a cryptic on-chain rejection.Changes:
SignatureExpiredExceptionadded to the types module withexpirationLedgerandcurrentLedgerfieldsvalidateSignatureExpiration()helper inSmartWalletServicesign(),signWithSessionKey(),addSigner(),addSessionSigner(),removeSigner()expirationBufferLedgers(default:10) for configurable buffer🔗 Related Issues
Closes #188
🧪 Testing
9 new unit tests added covering:
SignatureExpiredExceptionSignatureExpiredExceptionexpirationLedger+currentLedgeron thrown instance)expirationBufferLedgersconstructor value respectedAll 76 tests pass.
📚 Documentation Updates (Required)
docs/AI.mdwith new patterns/examplesDocumentation Checklist by Component:
If you modified
packages/core/invisible-wallet/:packages/core/invisible-wallet/README.mddocs/SECURITY.mddocs/ARCHITECTURE.md🤖 AI-Friendly Documentation
New Files Created
Key Functions/Classes Added
Patterns Used
signatureExpirationLedgeris checked before the WebAuthn or Ed25519 signing step. This prevents users from completing a biometric gesture only to have the transaction fail on-chain.credentials().switch().nameis checked before accessing.address(), so source-account credentials (which carry no expiry) are silently skipped — matching the same pattern used byvalidateDeFiAuthorization().getLatestLedger()(addSigner,addSessionSigner,removeSigner) reuse thesequencevalue to avoid an extra RPC round-trip. Methods that did not (sign,signWithSessionKey) now call it once after simulation.expirationBufferLedgersis an optional last constructor parameter (default10, ~50 seconds), preserving full backward compatibility for existing callers.📸 Screenshots (if applicable)
N/A — server-side SDK change, no UI.
expirationBufferLedgersis an optional 6th constructor parameter with a default of10. All existing call sites compile and behave identically unless a signature is expiring within 10 ledgers, in which case they now receiveSignatureExpiredExceptioninstead of a silent on-chain failure.🔄 Deployment Notes
No deployment changes required. This is a pure SDK-level validation; no contract changes, no new RPC endpoints, no configuration migrations.
✅ Final Checklist
By submitting this PR, I confirm that:
Summary by CodeRabbit
Release Notes
New Features
Tests