feat(wallets): add approval signature validation per signer type#1848
feat(wallets): add approval signature validation per signer type#1848daniil-dovgal wants to merge 2 commits into
Conversation
|
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/wallets/src/utils/signature-validation.ts:4
**Missing export: `InvalidSignatureForApprovalError` does not exist in `errors.ts`**
Both `signature-validation.ts` and `signature-validation.test.ts` import `InvalidSignatureForApprovalError` from `./errors`, but that class is not defined or exported anywhere in `packages/wallets/src/utils/errors.ts`. The PR description mentions adding it to `errors.ts`, but the actual change is absent from this diff — confirmed by running a two-dot diff between base and head, which shows only these two new files. This will produce a TypeScript compilation error, breaking the build entirely. The `WalletError` union type in `errors.ts` also needs to be updated to include the new error class.
### Issue 2 of 3
packages/wallets/src/utils/signature-validation.ts:130-161
**`assertApprovalSignatureFormat` is never called — the validation is unreachable**
`wallet.ts` is listed in the PR description as modified ("integration at `approveTransactionInternal` and `approveSignatureInternal`"), but the two-dot diff confirms `wallet.ts` was not changed. Neither `approveTransactionInternal` nor `approveSignatureInternal` import or call `assertApprovalSignatureFormat`. As a result, clients can still pass ERC-6492-wrapped or otherwise invalid signatures through `wallet.approve()` and the error will surface on-chain (AA23) rather than being caught early client-side. The entire purpose of this PR is not achieved.
### Issue 3 of 3
packages/wallets/src/utils/signature-validation.ts:131-133
**`registerSignatureValidator` allows overriding built-in validators silently**
`Map.set()` will silently replace any existing entry, meaning third-party code (or test code with a stale registry from a previous run) could call `registerSignatureValidator("external-wallet", noOpValidator)` and completely disable ECDSA validation. Guarding against overwrites of built-in types would prevent accidental or malicious bypass.
```suggestion
const BUILTIN_SIGNER_TYPES = new Set(["external-wallet", "server", "email", "phone", "passkey", "device"]);
export function registerSignatureValidator(signerType: string, validator: SignatureValidator): void {
if (BUILTIN_SIGNER_TYPES.has(signerType)) {
console.warn(`[registerSignatureValidator] Overriding built-in validator for signer type "${signerType}"`);
}
validatorRegistry.set(signerType, validator);
}
```
Reviews (1): Last reviewed commit: "feat(wallets): add approval signature va..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import type { Approval } from "@/wallets/types"; | ||
| import type { BaseSignResult, DeviceSignResult, PasskeySignResult, SignerLocator } from "@/signers/types"; | ||
| import { parseSignerLocator } from "./signer-locator"; | ||
| import { InvalidSignatureForApprovalError } from "./errors"; |
There was a problem hiding this comment.
InvalidSignatureForApprovalError does not exist in errors.ts
Both signature-validation.ts and signature-validation.test.ts import InvalidSignatureForApprovalError from ./errors, but that class is not defined or exported anywhere in packages/wallets/src/utils/errors.ts. The PR description mentions adding it to errors.ts, but the actual change is absent from this diff — confirmed by running a two-dot diff between base and head, which shows only these two new files. This will produce a TypeScript compilation error, breaking the build entirely. The WalletError union type in errors.ts also needs to be updated to include the new error class.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/utils/signature-validation.ts
Line: 4
Comment:
**Missing export: `InvalidSignatureForApprovalError` does not exist in `errors.ts`**
Both `signature-validation.ts` and `signature-validation.test.ts` import `InvalidSignatureForApprovalError` from `./errors`, but that class is not defined or exported anywhere in `packages/wallets/src/utils/errors.ts`. The PR description mentions adding it to `errors.ts`, but the actual change is absent from this diff — confirmed by running a two-dot diff between base and head, which shows only these two new files. This will produce a TypeScript compilation error, breaking the build entirely. The `WalletError` union type in `errors.ts` also needs to be updated to include the new error class.
How can I resolve this? If you propose a fix, please make it concise.| */ | ||
| export function registerSignatureValidator(signerType: string, validator: SignatureValidator): void { | ||
| validatorRegistry.set(signerType, validator); | ||
| } | ||
|
|
||
| /** | ||
| * Validates that the externally-provided approval signature matches the | ||
| * expected format for the given signer type. | ||
| * | ||
| * Call this in the `options.approval` branch of `approveTransactionInternal` | ||
| * and `approveSignatureInternal` **before** submitting to the API. | ||
| * | ||
| * @throws {InvalidSignatureForApprovalError} when the signature is malformed, | ||
| * ERC-6492-wrapped, or otherwise incompatible with the signer type. | ||
| */ | ||
| export function assertApprovalSignatureFormat(approval: Approval): void { | ||
|
|
||
| const { type: signerType } = parseSignerLocator(approval.signer as SignerLocator); | ||
|
|
||
| if (signerType === "api-key") { | ||
| return; | ||
| } | ||
|
|
||
| const validator = validatorRegistry.get(signerType); | ||
| if (validator == null) { | ||
| console.warn(`[assertApprovalSignatureFormat] No validator for signer type "${signerType}" — skipping validation`); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| validator.validate(approval); | ||
| } No newline at end of file |
There was a problem hiding this comment.
assertApprovalSignatureFormat is never called — the validation is unreachable
wallet.ts is listed in the PR description as modified ("integration at approveTransactionInternal and approveSignatureInternal"), but the two-dot diff confirms wallet.ts was not changed. Neither approveTransactionInternal nor approveSignatureInternal import or call assertApprovalSignatureFormat. As a result, clients can still pass ERC-6492-wrapped or otherwise invalid signatures through wallet.approve() and the error will surface on-chain (AA23) rather than being caught early client-side. The entire purpose of this PR is not achieved.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/utils/signature-validation.ts
Line: 130-161
Comment:
**`assertApprovalSignatureFormat` is never called — the validation is unreachable**
`wallet.ts` is listed in the PR description as modified ("integration at `approveTransactionInternal` and `approveSignatureInternal`"), but the two-dot diff confirms `wallet.ts` was not changed. Neither `approveTransactionInternal` nor `approveSignatureInternal` import or call `assertApprovalSignatureFormat`. As a result, clients can still pass ERC-6492-wrapped or otherwise invalid signatures through `wallet.approve()` and the error will surface on-chain (AA23) rather than being caught early client-side. The entire purpose of this PR is not achieved.
How can I resolve this? If you propose a fix, please make it concise.| export function registerSignatureValidator(signerType: string, validator: SignatureValidator): void { | ||
| validatorRegistry.set(signerType, validator); | ||
| } |
There was a problem hiding this comment.
registerSignatureValidator allows overriding built-in validators silently
Map.set() will silently replace any existing entry, meaning third-party code (or test code with a stale registry from a previous run) could call registerSignatureValidator("external-wallet", noOpValidator) and completely disable ECDSA validation. Guarding against overwrites of built-in types would prevent accidental or malicious bypass.
| export function registerSignatureValidator(signerType: string, validator: SignatureValidator): void { | |
| validatorRegistry.set(signerType, validator); | |
| } | |
| const BUILTIN_SIGNER_TYPES = new Set(["external-wallet", "server", "email", "phone", "passkey", "device"]); | |
| export function registerSignatureValidator(signerType: string, validator: SignatureValidator): void { | |
| if (BUILTIN_SIGNER_TYPES.has(signerType)) { | |
| console.warn(`[registerSignatureValidator] Overriding built-in validator for signer type "${signerType}"`); | |
| } | |
| validatorRegistry.set(signerType, validator); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/utils/signature-validation.ts
Line: 131-133
Comment:
**`registerSignatureValidator` allows overriding built-in validators silently**
`Map.set()` will silently replace any existing entry, meaning third-party code (or test code with a stale registry from a previous run) could call `registerSignatureValidator("external-wallet", noOpValidator)` and completely disable ECDSA validation. Guarding against overwrites of built-in types would prevent accidental or malicious bypass.
```suggestion
const BUILTIN_SIGNER_TYPES = new Set(["external-wallet", "server", "email", "phone", "passkey", "device"]);
export function registerSignatureValidator(signerType: string, validator: SignatureValidator): void {
if (BUILTIN_SIGNER_TYPES.has(signerType)) {
console.warn(`[registerSignatureValidator] Overriding built-in validator for signer type "${signerType}"`);
}
validatorRegistry.set(signerType, validator);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/wallets/src/utils/signature-validation.ts:44-57
**ECDSA validator rejects valid Solana/Stellar approval signatures**
`external-wallet` and `server` signer types on Solana produce base58-encoded Ed25519 signatures (`SolanaExternalWalletSigner.signTransaction` returns `{ signature: base58.encode(signatureBytes) }`), and Stellar signers produce base64 (`StellarServerSigner.signMessage` returns `{ signature: Buffer.from(signatureBytes).toString("base64") }`). When a user calls `wallet.approve({ approval: { signer: "external-wallet:SolanaAddress", signature: "<base58sig>" } })`, `isHex(signature)` returns `false` and the validator throws "expected a hex string", rejecting a perfectly valid Solana approval.
The cross-chain compatibility tests do not catch this because they pass a hex ECDSA signature for a Solana address — the actual production signature format for Solana is base58, not hex. The ECDSA validator should not be applied uniformly to signer types that can produce non-hex signatures across chains. Consider either threading the wallet's chain context into `assertApprovalSignatureFormat` so the correct format check is applied per chain, or applying ECDSA format validation only when the wallet is confirmed to be an EVM wallet.
Reviews (2): Last reviewed commit: "feat: errors, assertApprovalSignatureFor..." | Re-trigger Greptile |
| received !== undefined ? `Received: ${JSON.stringify(received)}` : undefined | ||
| ); | ||
| } | ||
|
|
||
| const ecdsaValidator: SignatureValidator<EcdsaApproval> = { | ||
| validate(approval): void { | ||
| const { signature, signer } = approval; | ||
|
|
||
| if (!isHex(signature)) { | ||
| invalidSignature(signer, "expected a hex string", signature); | ||
| } | ||
|
|
||
| if (signature.endsWith(ERC_6492_MAGIC_SUFFIX)) { | ||
| invalidSignature(signer, "ERC-6492 wrapped signatures are not supported — provide a raw ECDSA signature"); |
There was a problem hiding this comment.
external-wallet and server signer types on Solana produce base58-encoded Ed25519 signatures (SolanaExternalWalletSigner.signTransaction returns { signature: base58.encode(signatureBytes) }), and Stellar signers produce base64 (StellarServerSigner.signMessage returns { signature: Buffer.from(signatureBytes).toString("base64") }). When a user calls wallet.approve({ approval: { signer: "external-wallet:SolanaAddress", signature: "<base58sig>" } }), isHex(signature) returns false and the validator throws "expected a hex string", rejecting a perfectly valid Solana approval.
The cross-chain compatibility tests do not catch this because they pass a hex ECDSA signature for a Solana address — the actual production signature format for Solana is base58, not hex. The ECDSA validator should not be applied uniformly to signer types that can produce non-hex signatures across chains. Consider either threading the wallet's chain context into assertApprovalSignatureFormat so the correct format check is applied per chain, or applying ECDSA format validation only when the wallet is confirmed to be an EVM wallet.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/utils/signature-validation.ts
Line: 44-57
Comment:
**ECDSA validator rejects valid Solana/Stellar approval signatures**
`external-wallet` and `server` signer types on Solana produce base58-encoded Ed25519 signatures (`SolanaExternalWalletSigner.signTransaction` returns `{ signature: base58.encode(signatureBytes) }`), and Stellar signers produce base64 (`StellarServerSigner.signMessage` returns `{ signature: Buffer.from(signatureBytes).toString("base64") }`). When a user calls `wallet.approve({ approval: { signer: "external-wallet:SolanaAddress", signature: "<base58sig>" } })`, `isHex(signature)` returns `false` and the validator throws "expected a hex string", rejecting a perfectly valid Solana approval.
The cross-chain compatibility tests do not catch this because they pass a hex ECDSA signature for a Solana address — the actual production signature format for Solana is base58, not hex. The ECDSA validator should not be applied uniformly to signer types that can produce non-hex signatures across chains. Consider either threading the wallet's chain context into `assertApprovalSignatureFormat` so the correct format check is applied per chain, or applying ECDSA format validation only when the wallet is confirmed to be an EVM wallet.
How can I resolve this? If you propose a fix, please make it concise.
🔥 Smoke Test Results❌ Status: Failed Statistics
Test DetailsThis is a non-blocking smoke test. Full regression tests run separately. |
| * Register a custom validator for a new signer type. | ||
| * Existing validators can be extended without modifying this module. | ||
| */ | ||
| export function registerSignatureValidator(signerType: string, validator: SignatureValidator): void { |
There was a problem hiding this comment.
where are we calling this? cant we auto call this on setup?
2 participants
Description
A third-party client used
EVMWallet.signMessage()to sign theuserOperationHashfor a transaction approval. For undeployed smart wallets,signMessage()routes through the Signature API, which wraps the result with ERC-6492 (~580 bytes). The on-chain ECDSA validator expects a 65-byte raw ECDSA signature — it cannot parse the ERC-6492 envelope and reverts withInvalidSignature(), which surfaces as AA23.The client assumption that
signMessage(userOpHash)produces a valid transaction approval signature is incorrect — the SDK exposeswallet.approve()/wallet.send()specifically for this purpose.This PR adds SDK-side validation inside
wallet.approve()before calling the API, catching invalid signatures early with clear error messages:external-wallet,server,email,phone): validates hex format viaisHex(), rejects ERC-6492-wrapped signatures, checks 64/65-byte length viasize(), and structurally parses 65-byte sigs viaparseSignature()device): validatesrandscomponents satisfy1 ≤ value < P256_ORDERpasskey): delegates to P256 validation, then checks metadata presenceapi-key: bypasses validation (auto-approved)registerSignatureValidator())Files changed:
packages/wallets/src/utils/signature-validation.ts— new validation modulepackages/wallets/src/utils/signature-validation.test.ts— 45 test casespackages/wallets/src/utils/errors.ts—InvalidSignatureForApprovalErrorpackages/wallets/src/wallets/wallet.ts— integration atapproveTransactionInternalandapproveSignatureInternalTest plan
{r,s}, just below curve order, r=0, s=0, r≥ORDER, s≥ORDER{r,s}registerSignatureValidatorPackage updates
@crossmint/wallets-sdk(patch): SDK-side approval signature validation