From 80f1412e5d888846d879f949f3d8756f92af2548 Mon Sep 17 00:00:00 2001 From: Marcelo Ceccon Date: Sun, 10 May 2026 00:06:47 +0000 Subject: [PATCH] Security: post-review fixes for merchant-sdk Hardenings flowing from the security review of the v0.5.0 additions (estimateGas, telemetry breadcrumb, headless hooks + Web Components, EIP-2612 permit signing, ConfirmationPolicy): - src/evm/permit.ts: * cap permit deadlines at 60 minutes from now (rejects MAX_SAFE_INTEGER and other no-expiry bearer windows); * cross-validate input.chainId against walletClient.getChainId() before signing so a stale config can't produce a payload replayable on the wallet's actual chain; * run validatePermitSignature() on the wallet's reply before returning, catching corrupt signatures earlier than the token contract would. - src/core/telemetry.ts: redactErrorMessage now strips POSIX/Windows filesystem paths, file:// URLs, and long bare hex blobs (>=64 chars, private-key shaped) in addition to addresses, tx hashes, UUIDs, and base58 pubkeys. Stops integrators from leaking stack-trace paths and raw secrets via 3rd-party analytics pipelines. - src/solana/estimateGas.ts: throw when BOTH simulateTransaction and getRecentPrioritizationFees fail rather than silently returning the static 5000-lamport signature fee (which would render as "~$0.001" even on a congested cluster). The single-failure tolerant paths stay in place. - package.json: declare the wc subpath entrypoint as having side effects so tree-shaking-aware bundlers don't drop the customElements.define call. This is the documented contract; the manifest now matches. - src/index.ts: re-export MAX_PERMIT_DEADLINE_WINDOW_SECONDS so callers can introspect the cap (e.g. to render their own deadline-picker UI without hard-coding the value). Tests: 171 pass (149 baseline + 22 new for permit hardenings, telemetry path/hex redaction, and the Solana both-fail throw). --- package.json | 6 +- src/__tests__/permit.test.ts | 240 +++++++++++++++++++++++ src/__tests__/solana-estimateGas.test.ts | 16 ++ src/__tests__/telemetry.test.ts | 31 +++ src/core/telemetry.ts | 29 ++- src/evm/permit.ts | 68 ++++++- src/index.ts | 1 + src/solana/estimateGas.ts | 18 ++ 8 files changed, 403 insertions(+), 6 deletions(-) create mode 100644 src/__tests__/permit.test.ts diff --git a/package.json b/package.json index e9bdefe..1a45194 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,11 @@ "module": "./dist/index.js", "types": "./dist/index.d.ts", "sideEffects": [ - "**/*.css" + "**/*.css", + "./dist/wc.js", + "./dist/wc.cjs", + "./src/wc/index.ts", + "./src/wc/pay-button.ts" ], "exports": { ".": { diff --git a/src/__tests__/permit.test.ts b/src/__tests__/permit.test.ts new file mode 100644 index 0000000..0332afd --- /dev/null +++ b/src/__tests__/permit.test.ts @@ -0,0 +1,240 @@ +import { describe, it, expect, vi } from 'vitest'; +import type { WalletClient } from 'viem'; +import { + assertDeadlineFresh, + buildPermitTypedData, + signPermit, + validatePermitSignature, + MAX_PERMIT_DEADLINE_WINDOW_SECONDS, +} from '../evm/permit'; + +const OWNER = '0xA0b86991C6218b36c1d19D4a2e9Eb0cE3606eB48' as const; +const SPENDER = '0x1111111111111111111111111111111111111111' as const; +const TOKEN = '0x2222222222222222222222222222222222222222' as const; + +// 65-byte signature (132 hex chars) shaped like a real EIP-712 reply. r and s +// are non-zero, s is in the low half, v is 27. Used as the wallet's mock reply +// in the signPermit happy-path tests. +const VALID_SIG = ('0x' + + '11'.repeat(32) + // r + '22'.repeat(32) + // s + '1b' // v = 27 +) as `0x${string}`; + +function fakeWallet(opts: { + account?: `0x${string}`; + chainId?: number; + signature?: `0x${string}`; + signError?: string; +} = {}) { + const account = opts.account ?? OWNER; + const chainId = opts.chainId ?? 1; + const signature = opts.signature ?? VALID_SIG; + return { + getAddresses: vi.fn().mockResolvedValue([account]), + getChainId: vi.fn().mockResolvedValue(chainId), + signTypedData: vi.fn().mockImplementation(() => { + if (opts.signError) return Promise.reject(new Error(opts.signError)); + return Promise.resolve(signature); + }), + } as unknown as WalletClient; +} + +describe('assertDeadlineFresh', () => { + it('accepts a deadline within the SDK cap window', () => { + const now = Math.floor(Date.now() / 1000); + const deadline = BigInt(now + 30 * 60); // 30 minutes + expect(() => assertDeadlineFresh(deadline)).not.toThrow(); + }); + + it('rejects a deadline already in the past', () => { + const now = Math.floor(Date.now() / 1000); + expect(() => assertDeadlineFresh(BigInt(now - 1))).toThrow(/not in the future/); + }); + + it('rejects an unbounded deadline (e.g. MAX_SAFE_INTEGER)', () => { + // Acts as a no-expiry bearer permit — defeats the EIP-2612 deadline + // mechanism. Must be rejected so a bug in the caller can't sign one. + expect(() => assertDeadlineFresh(BigInt(Number.MAX_SAFE_INTEGER))).toThrow(/exceeds the SDK cap/); + }); + + it('rejects a deadline more than the cap into the future', () => { + const now = Math.floor(Date.now() / 1000); + const tooFar = BigInt(now + MAX_PERMIT_DEADLINE_WINDOW_SECONDS + 60); + expect(() => assertDeadlineFresh(tooFar)).toThrow(/exceeds the SDK cap/); + }); + + it('honors a caller-provided larger window when explicitly set', () => { + // The cap is a default; advanced callers can opt out by supplying a wider + // bound. This keeps the function flexible while making the safe path + // automatic. + const now = Math.floor(Date.now() / 1000); + const twoHours = 60 * 60 * 2; + expect(() => assertDeadlineFresh(BigInt(now + twoHours), now, twoHours + 1)).not.toThrow(); + }); +}); + +describe('buildPermitTypedData', () => { + it('packs the EIP-2612 domain with chainId, name, version, verifyingContract', () => { + const td = buildPermitTypedData({ + chainId: 137, + tokenAddress: TOKEN, + tokenName: 'USD Coin', + tokenVersion: '2', + owner: OWNER, + spender: SPENDER, + value: 1_000_000n, + nonce: 5n, + deadline: 9_999_999_999n, + }); + expect(td.domain).toEqual({ + name: 'USD Coin', + version: '2', + chainId: 137, + verifyingContract: TOKEN, + }); + expect(td.primaryType).toBe('Permit'); + // Permit struct must match EIP-2612 exactly (owner, spender, value, nonce, deadline). + expect(td.types.Permit).toEqual([ + { name: 'owner', type: 'address' }, + { name: 'spender', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint256' }, + ]); + }); + + it('defaults version to "1" when omitted', () => { + const td = buildPermitTypedData({ + chainId: 1, + tokenAddress: TOKEN, + tokenName: 'DAI', + owner: OWNER, + spender: SPENDER, + value: 1n, + nonce: 0n, + deadline: 9_999_999_999n, + }); + expect(td.domain.version).toBe('1'); + }); +}); + +describe('validatePermitSignature', () => { + it('accepts a well-formed signature with low-s and v=27', () => { + expect(validatePermitSignature(VALID_SIG)).toEqual({ valid: true }); + }); + + it('rejects a wrong-length string', () => { + const short = '0xdeadbeef' as `0x${string}`; + const out = validatePermitSignature(short); + expect(out.valid).toBe(false); + expect(out.reason).toMatch(/Expected 132 hex chars/); + }); + + it('rejects an r=0 signature', () => { + const sig = ('0x' + '00'.repeat(32) + '22'.repeat(32) + '1b') as `0x${string}`; + expect(validatePermitSignature(sig).valid).toBe(false); + }); + + it('rejects a high-s signature (EIP-2 malleability)', () => { + // s = secp256k1 N - 1 → high half of the curve order. + const highS = 'fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140'; + const sig = ('0x' + '11'.repeat(32) + highS + '1b') as `0x${string}`; + expect(validatePermitSignature(sig).valid).toBe(false); + }); + + it('rejects an out-of-range v', () => { + const sig = ('0x' + '11'.repeat(32) + '22'.repeat(32) + 'ff') as `0x${string}`; + expect(validatePermitSignature(sig).valid).toBe(false); + }); +}); + +describe('signPermit', () => { + function freshDeadline(): bigint { + return BigInt(Math.floor(Date.now() / 1000) + 5 * 60); + } + + it('signs and returns split v/r/s for a valid input', async () => { + const wallet = fakeWallet(); + const out = await signPermit({ + walletClient: wallet, + chainId: 1, + tokenAddress: TOKEN, + tokenName: 'USD Coin', + tokenVersion: '2', + owner: OWNER, + spender: SPENDER, + value: 1_000_000n, + nonce: 0n, + deadline: freshDeadline(), + }); + expect(out.signature).toBe(VALID_SIG); + expect([27, 28]).toContain(out.v); + expect(out.r).toMatch(/^0x[0-9a-f]{64}$/i); + expect(out.s).toMatch(/^0x[0-9a-f]{64}$/i); + }); + + it('refuses to sign when the wallet account differs from the permit owner', async () => { + const wallet = fakeWallet({ account: '0x9999999999999999999999999999999999999999' }); + await expect(signPermit({ + walletClient: wallet, + chainId: 1, + tokenAddress: TOKEN, + tokenName: 'USD Coin', + owner: OWNER, + spender: SPENDER, + value: 1n, + nonce: 0n, + deadline: freshDeadline(), + })).rejects.toThrow(/does not match permit owner/); + }); + + it('refuses to sign when the wallet chainId disagrees with the permit chainId', async () => { + // The wallet is on chain 137 but the caller is asking us to sign for chain 1. + // The signed message would be replayable on the wallet's actual chain. + const wallet = fakeWallet({ chainId: 137 }); + await expect(signPermit({ + walletClient: wallet, + chainId: 1, + tokenAddress: TOKEN, + tokenName: 'USD Coin', + owner: OWNER, + spender: SPENDER, + value: 1n, + nonce: 0n, + deadline: freshDeadline(), + })).rejects.toThrow(/Wallet chainId 137 does not match permit chainId 1/); + }); + + it('refuses to sign with a MAX_SAFE_INTEGER deadline (no expiry)', async () => { + const wallet = fakeWallet(); + await expect(signPermit({ + walletClient: wallet, + chainId: 1, + tokenAddress: TOKEN, + tokenName: 'USD Coin', + owner: OWNER, + spender: SPENDER, + value: 1n, + nonce: 0n, + deadline: BigInt(Number.MAX_SAFE_INTEGER), + })).rejects.toThrow(/exceeds the SDK cap/); + }); + + it('throws when the wallet returns a malformed signature', async () => { + // r=0 → fails validatePermitSignature inside signPermit before returning. + const badSig = ('0x' + '00'.repeat(32) + '22'.repeat(32) + '1b') as `0x${string}`; + const wallet = fakeWallet({ signature: badSig }); + await expect(signPermit({ + walletClient: wallet, + chainId: 1, + tokenAddress: TOKEN, + tokenName: 'USD Coin', + owner: OWNER, + spender: SPENDER, + value: 1n, + nonce: 0n, + deadline: freshDeadline(), + })).rejects.toThrow(/invalid permit signature/); + }); +}); diff --git a/src/__tests__/solana-estimateGas.test.ts b/src/__tests__/solana-estimateGas.test.ts index 17a438e..990cbf0 100644 --- a/src/__tests__/solana-estimateGas.test.ts +++ b/src/__tests__/solana-estimateGas.test.ts @@ -128,6 +128,22 @@ describe('estimateSolanaGas (native)', () => { expect(out.native).toBe(LAMPORTS_PER_SIGNATURE); }); + it('throws when BOTH simulate and getRecentPrioritizationFees fail', async () => { + // When both dynamic sources are unavailable the only thing left is the + // 5000-lamport signature fee, which would render "≈ $0.001" and be + // misleading on a congested cluster. The estimator should refuse rather + // than silently produce a fake-looking number. + const conn = mockConnection({ simulateError: true, feesError: true }); + await expect(estimateSolanaGas({ + connection: conn, + sender: SENDER, + programId: PROGRAM_ID, + merchantId: MERCHANT_ID, + token: NATIVE_TOKEN_SENTINEL, + amount: 1n, + })).rejects.toThrow(/Solana fee estimate unavailable/); + }); + it('converts lamports to USD when a priceUsd oracle is supplied', async () => { const conn = mockConnection({ unitsConsumed: 50_000, prioritization: [] }); const out = await estimateSolanaGas( diff --git a/src/__tests__/telemetry.test.ts b/src/__tests__/telemetry.test.ts index 987aaa5..14774b5 100644 --- a/src/__tests__/telemetry.test.ts +++ b/src/__tests__/telemetry.test.ts @@ -67,6 +67,37 @@ describe('redactErrorMessage', () => { expect(out.length).toBeLessThanOrEqual(240); expect(out.endsWith('...')).toBe(true); }); + + it('redacts POSIX absolute paths from stack-trace fragments', () => { + const msg = 'TypeError at /Users/alice/src/wallet/index.ts:42'; + const out = redactErrorMessage(msg) ?? ''; + expect(out).not.toContain('/Users/alice'); + expect(out).toContain(''); + }); + + it('redacts Windows absolute paths', () => { + const msg = 'Failed loading C:\\Users\\bob\\AppData\\Local\\app\\index.js'; + const out = redactErrorMessage(msg) ?? ''; + expect(out).not.toMatch(/C:\\Users\\bob/); + expect(out).toContain(''); + }); + + it('redacts file:// URLs', () => { + const msg = 'thrown at file:///home/vscode/workspace/x/y.ts:10:5'; + const out = redactErrorMessage(msg) ?? ''; + expect(out).not.toContain('file:///home'); + expect(out).toContain(''); + }); + + it('redacts long unbroken hex blobs (private-key shaped)', () => { + // A 64+ char hex run is consistent with a raw private key, raw signature, + // or session token. Strip rather than risk leaking via 3rd-party + // analytics. + const msg = 'leaked secret 0x' + 'ab'.repeat(40); // 80 hex chars, way past the floor + const out = redactErrorMessage(msg) ?? ''; + expect(out).not.toMatch(/ab{20,}/i); + expect(out).toContain('<'); + }); }); describe('hashWalletAddress', () => { diff --git a/src/core/telemetry.ts b/src/core/telemetry.ts index 7b21080..cda363a 100644 --- a/src/core/telemetry.ts +++ b/src/core/telemetry.ts @@ -139,8 +139,21 @@ export async function hashWalletAddress(address: string | null | undefined): Pro } /** - * Strip values that look like wallet addresses, pubkeys, or UUIDs from a - * developer error message. Keeps the message under 240 chars. + * Strip values that look like wallet addresses, pubkeys, UUIDs, filesystem + * paths, or long secrets from a developer error message. Keeps the message + * under 240 chars. + * + * The redaction list reflects what we've seen leak through `Error.message` / + * `Error.stack` in browser & Node SDKs: + * - EVM addresses + tx hashes (caller's wallet, our contract). + * - UUIDs (session ids). + * - Solana / TRON base58 (caller's wallet). + * - Absolute filesystem paths from stack traces (`/Users/`, `/home/`, + * `C:\`, `file://`) — these reveal username and source-file layout when + * the integrator pipes the message into a 3rd-party analytics pipeline. + * - Long unbroken hex blobs (≥64 chars) — catches private keys, raw + * signatures, or session tokens that occasionally surface in nested + * wallet errors. */ export function redactErrorMessage(message: string | undefined): string | undefined { if (!message) return undefined; @@ -151,6 +164,18 @@ export function redactErrorMessage(message: string | undefined): string | undefi .replace(/0x[a-fA-F0-9]{64}/g, '0x') // UUID-shaped strings .replace(/[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}/g, '') + // POSIX absolute paths (`/Users/alice/...`, `/home/bob/...`, `/private/tmp/...`). + // Catches stack-trace fragments that leak the integrator's username. + .replace(/\/(?:Users|home|root|private|var|opt|srv|tmp)\/[^\s)"']+/gi, '') + // Windows-style absolute paths (`C:\Users\...`). + .replace(/[A-Za-z]:\\[^\s)"']+/g, '') + // file:// URLs from JS stack traces. + .replace(/file:\/\/[^\s)"']+/g, '') + // Long bare hex blobs (≥64 chars). Catches private keys, raw 65-byte + // signatures, and session-token-shaped strings nested inside wallet + // error messages. The 64-char floor avoids false positives on shorter + // identifiers. + .replace(/(?:0x)?[a-fA-F0-9]{64,}/g, '') // Solana / TRON base58 (32–44 chars, no 0/O/I/l) — be conservative, only // redact when the substring is a standalone token (whitespace bounded). .replace(/(^|\s)[1-9A-HJ-NP-Za-km-z]{32,44}(?=\s|[,.;:]|$)/g, '$1'); diff --git a/src/evm/permit.ts b/src/evm/permit.ts index ab1d11d..0700dd6 100644 --- a/src/evm/permit.ts +++ b/src/evm/permit.ts @@ -66,6 +66,20 @@ const EIP2612_ABI = [ /** Default DAI/USDC/USDT permit version when the token doesn't expose `version()`. */ const DEFAULT_PERMIT_VERSION = '1'; +/** + * Maximum permit deadline window the SDK will sign — 60 minutes from now. + * + * Defence-in-depth against a caller passing `Number.MAX_SAFE_INTEGER` (or any + * sufficiently large value) as a deadline, which would effectively be a "no + * expiry" permit. EIP-2612 permits are bearer instruments: a leaked signature + * with a far-future deadline is replayable on-chain until the nonce + * increments, so we cap the window aggressively. Most flows want ≤30 minutes; + * the 60-minute cap leaves headroom for mobile-wallet round-trips while still + * limiting blast radius if the signed payload escapes (e.g. logged in the + * user's wallet history). + */ +export const MAX_PERMIT_DEADLINE_WINDOW_SECONDS = 60 * 60; + export interface PermitSupport { /** Whether the token responds to all four EIP-2612 view calls. */ supported: boolean; @@ -204,21 +218,47 @@ export function buildPermitTypedData(input: Omit maxDeadline) { + throw new Error( + `Permit deadline ${deadline.toString()} exceeds the SDK cap of ` + + `${maxWindowSeconds}s from now (max=${maxDeadline.toString()}). ` + + `Permit signatures are bearer instruments — keep deadlines short.`, + ); + } return deadline; } /** * Ask the wallet to sign the EIP-2612 permit. Splits the resulting 65-byte * signature into v/r/s the caller can pass to the token's `permit(...)` tx. + * + * Hardenings beyond the EIP-712 spec basics: + * - rejects deadlines in the past or beyond {@link MAX_PERMIT_DEADLINE_WINDOW_SECONDS}; + * - cross-validates `input.chainId` against the wallet's live `getChainId()` + * so a stale config can't end up signing a payload that's then replayable + * on a different chain; + * - re-runs {@link validatePermitSignature} on the wallet's reply before + * returning (catches corrupt signatures earlier than the token contract + * would). */ export async function signPermit(input: SignPermitInput): Promise { assertDeadlineFresh(input.deadline); @@ -233,6 +273,20 @@ export async function signPermit(input: SignPermitInput): Promise undefined); + if (typeof walletChainId === 'number' && walletChainId !== input.chainId) { + throw new Error( + `Wallet chainId ${walletChainId} does not match permit chainId ${input.chainId} — ` + + `refusing to sign. The caller likely needs to switch the wallet to chain ${input.chainId} first.`, + ); + } + // viem's signTypedData returns a 0x-prefixed 65-byte hex string. const signature = await input.walletClient.signTypedData({ account, @@ -245,6 +299,14 @@ export async function signPermit(input: SignPermitInput): Promise 0) { computeUnits = consumed; } + simulationOk = true; } catch { // Keep DEFAULT_COMPUTE_UNITS — surface a conservative estimate. } // 2. Median priority fee across recent blocks. Returns micro-lamports per CU. let microLamportsPerCu = 0; + let priorityOk = false; try { const recent = await input.connection.getRecentPrioritizationFees(); if (Array.isArray(recent) && recent.length > 0) { @@ -137,10 +140,25 @@ export async function estimateSolanaGas( microLamportsPerCu = sorted[Math.floor(sorted.length / 2)] ?? 0; } } + // The RPC succeeded even when it returned an empty array (a healthy + // cluster with no congestion). Treat that as "priority data confirmed + // zero" rather than "RPC down". + priorityOk = true; } catch { // Networks that don't support the RPC just skip priority — base fee still applies. } + // If BOTH dynamic sources failed, the only thing we'd be returning is the + // static 5000-lamport signature fee — i.e. "≈ $0.001" — which is misleading + // when the cluster might actually be congested. Throw so the modal can fall + // back to a "fee unavailable" UI rather than render a fake-looking number. + if (!simulationOk && !priorityOk) { + throw new Error( + 'Solana fee estimate unavailable: both simulateTransaction and ' + + 'getRecentPrioritizationFees failed. The caller should surface "fee unavailable".', + ); + } + // 3. Total lamports = base + (priority µLAM/CU × CU) / 1e6. const priorityLamports = Math.ceil((microLamportsPerCu * computeUnits) / 1_000_000); const totalLamports = LAMPORTS_PER_SIGNATURE + priorityLamports;