From 6982996d70f5d324bb33158151c7f1a647a15c86 Mon Sep 17 00:00:00 2001 From: David Ejere Date: Wed, 21 Oct 2026 08:09:14 +0100 Subject: [PATCH 1/3] fix(integration): invalidate stellar.toml cache on issuer data conflicts (#837) fetchStellarToml and getAnchorInfo both trusted a domain-keyed cache hit indefinitely within its TTL, with no check that the cached document still agrees with what the domain currently serves. A compromised or stale RPC response for a domain would be returned forever within the TTL window even if the real document had since changed its SIGNING_KEY or CURRENCIES issuer addresses, both of which control where payments get routed. Add detectStellarTomlConflict() to compare two stellar.toml documents on these security-critical fields. Both cache readers now re-verify a cache hit against a fresh fetch/resolve by default (verifyOnCacheHit, opt-out): on a mismatch, the cache is invalidated, a warning is logged with the conflict detail, and the fresh (not the stale) document is returned. If the re-verification fetch itself fails, the unverified cached value is served rather than failing the call outright. --- src/integration/anchors.test.ts | 48 +++++++++-- src/integration/anchors.ts | 46 +++++++++- src/integration/sep1Toml.test.ts | 78 ++++++++++++++++- src/integration/sep1Toml.ts | 144 ++++++++++++++++++++++++++----- 4 files changed, 285 insertions(+), 31 deletions(-) diff --git a/src/integration/anchors.test.ts b/src/integration/anchors.test.ts index c169927c..2770c9e1 100644 --- a/src/integration/anchors.test.ts +++ b/src/integration/anchors.test.ts @@ -6,7 +6,20 @@ import { fetchStellarToml } from "./sep1Toml"; describe("getAnchorInfo", () => { beforeEach(() => clearAnchorCache()); - it("reuses cached anchor information for the same domain", async () => { + it("reuses cached anchor information for the same domain when verification is disabled", async () => { + const resolve = vi.spyOn(StellarToml.Resolver, "resolve").mockResolvedValue({ + TRANSFER_SERVER: "https://anchor.example.com/sep6", + } as never); + + const first = await getAnchorInfo("example.com", { verifyOnCacheHit: false }); + const second = await getAnchorInfo("example.com", { verifyOnCacheHit: false }); + + expect(first.status).toBe("ok"); + expect(second.status).toBe("ok"); + expect(resolve).toHaveBeenCalledTimes(1); + }); + + it("by default, re-verifies a cache hit against a fresh resolve (#837)", async () => { const resolve = vi.spyOn(StellarToml.Resolver, "resolve").mockResolvedValue({ TRANSFER_SERVER: "https://anchor.example.com/sep6", } as never); @@ -16,7 +29,30 @@ describe("getAnchorInfo", () => { expect(first.status).toBe("ok"); expect(second.status).toBe("ok"); - expect(resolve).toHaveBeenCalledTimes(1); + expect(resolve).toHaveBeenCalledTimes(2); + }); + + it("invalidates the cache and warns when re-verification reveals a changed SIGNING_KEY (#837)", async () => { + const resolve = vi + .spyOn(StellarToml.Resolver, "resolve") + .mockResolvedValueOnce({ SIGNING_KEY: "GOLDKEY00000000000000000000000000000000000000000000" } as never) + .mockResolvedValueOnce({ SIGNING_KEY: "GATTACKER0000000000000000000000000000000000000000000" } as never); + const warn = vi.fn(); + + const first = await getAnchorInfo("example.com"); + if (first.status === "ok") expect(first.data.SIGNING_KEY).toContain("GOLDKEY"); + + const second = await getAnchorInfo("example.com", { + logger: { debug: vi.fn(), info: vi.fn(), warn, error: vi.fn() }, + }); + + expect(resolve).toHaveBeenCalledTimes(2); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("diverged"), + expect.objectContaining({ domain: "example.com" }), + ); + expect(second.status).toBe("ok"); + if (second.status === "ok") expect(second.data.SIGNING_KEY).toContain("GATTACKER"); }); it("fetches anchor information again after the configured TTL expires", async () => { @@ -41,9 +77,9 @@ describe("getAnchorInfo", () => { TRANSFER_SERVER: "https://anchor.example.com/sep6", } as never); - await getAnchorInfo("example.com"); + await getAnchorInfo("example.com", { verifyOnCacheHit: false }); clearAnchorCache(); - await getAnchorInfo("example.com"); + await getAnchorInfo("example.com", { verifyOnCacheHit: false }); expect(resolve).toHaveBeenCalledTimes(2); }); @@ -54,8 +90,8 @@ describe("getAnchorInfo", () => { } as never); const fetcher = vi.fn(); - await getAnchorInfo("example.com"); - const result = await fetchStellarToml("example.com", { fetch: fetcher }); + await getAnchorInfo("example.com", { verifyOnCacheHit: false }); + const result = await fetchStellarToml("example.com", { fetch: fetcher, verifyOnCacheHit: false }); expect(result.status).toBe("ok"); if (result.status === "ok") { diff --git a/src/integration/anchors.ts b/src/integration/anchors.ts index 2c6bcb2d..6f30efbc 100644 --- a/src/integration/anchors.ts +++ b/src/integration/anchors.ts @@ -6,9 +6,11 @@ import { cacheStellarToml, clearStellarTomlCache, getCachedStellarToml, + detectStellarTomlConflict, DEFAULT_STELLAR_TOML_CACHE_TTL_MS, } from "./sep1Toml"; import type { StellarToml as StellarTomlData } from "./sep1Toml"; +import type { SorokitLogger } from "../shared/logger"; export interface AnchorAsset { code: string; @@ -63,8 +65,23 @@ function asUrl(value: string): URL | null { export interface GetAnchorInfoOptions { ttlMs?: number; timeoutMs?: number; + /** See FetchStellarTomlOptions.verifyOnCacheHit in ./sep1Toml (#837). Defaults to true. */ + verifyOnCacheHit?: boolean; + /** Logger for conflict warnings when verifyOnCacheHit detects a divergence. */ + logger?: SorokitLogger; } +async function resolveAnchorToml(homeDomain: string, timeoutMs: number): Promise { + return StellarToml.Resolver.resolve(homeDomain, { timeout: timeoutMs }) as Promise; +} + +const NOOP_LOGGER: SorokitLogger = { + debug: () => undefined, + info: () => undefined, + warn: () => undefined, + error: () => undefined, +}; + export async function getAnchorInfo( homeDomain: string, options: GetAnchorInfoOptions = {}, @@ -79,10 +96,35 @@ export async function getAnchorInfo( } const cached = getCachedStellarToml(homeDomain); - if (cached) return ok(cached); + if (cached) { + const verifyOnCacheHit = options.verifyOnCacheHit ?? true; + if (!verifyOnCacheHit) return ok(cached); + + // #837: re-verify against a fresh resolve before trusting the cache, + // the same protection fetchStellarToml applies to its own cache. + let fresh: StellarTomlData; + try { + fresh = await resolveAnchorToml(homeDomain, timeoutMs); + } catch { + // Re-verification itself failed — serve the (unproven-stale) cache + // rather than failing the call outright. + return ok(cached); + } + + const conflict = detectStellarTomlConflict(cached, fresh); + if (conflict) { + (options.logger ?? NOOP_LOGGER).warn( + `Anchor stellar.toml for "${homeDomain}" diverged from cached data: ${conflict}. Invalidating cache.`, + { domain: homeDomain, conflict }, + ); + clearStellarTomlCache(homeDomain); + } + cacheStellarToml(homeDomain, fresh, ttlMs); + return ok(fresh); + } try { - const data = await StellarToml.Resolver.resolve(homeDomain, { timeout: timeoutMs }) as StellarTomlData; + const data = await resolveAnchorToml(homeDomain, timeoutMs); cacheStellarToml(homeDomain, data, ttlMs); return ok(data); } catch (cause) { diff --git a/src/integration/sep1Toml.test.ts b/src/integration/sep1Toml.test.ts index 0df5403f..7631c4a4 100644 --- a/src/integration/sep1Toml.test.ts +++ b/src/integration/sep1Toml.test.ts @@ -45,16 +45,90 @@ describe("fetchStellarToml", () => { if (result.status === "error") expect(result.error.code).toBe("INVALID_CONFIG"); }); - it("caches successful results for the configured TTL", async () => { + it("caches successful results for the configured TTL, used as a fallback if verification is unavailable", async () => { const fetcher = vi.fn().mockResolvedValue(new Response('AUTH_SERVER = "https://auth.example.com"')); - const options = { fetch: fetcher, ttlMs: 60_000 }; + const options = { fetch: fetcher, ttlMs: 60_000, verifyOnCacheHit: false }; await fetchStellarToml("https://example.com", options); await fetchStellarToml("example.com", options); + // With verification explicitly disabled, a cache hit is trusted + // immediately without a second network call. expect(fetcher).toHaveBeenCalledTimes(1); }); + it("by default, re-verifies a cache hit against a fresh fetch (#837)", async () => { + const fetcher = vi.fn().mockResolvedValue(new Response('AUTH_SERVER = "https://auth.example.com"')); + const options = { fetch: fetcher, ttlMs: 60_000 }; + + await fetchStellarToml("https://example.com", options); + const second = await fetchStellarToml("example.com", options); + + // Verification happens on every cache hit by default — the fresh data + // agrees with the cached data, so the result is still served correctly. + expect(fetcher).toHaveBeenCalledTimes(2); + expect(second.status).toBe("ok"); + if (second.status === "ok") { + expect(second.data.auth_server).toBe("https://auth.example.com"); + } + }); + + it("invalidates the cache and warns when a re-verification fetch reveals a changed SIGNING_KEY (#837)", async () => { + const fetcher = vi + .fn() + .mockResolvedValueOnce(new Response('SIGNING_KEY = "GOLDKEY00000000000000000000000000000000000000000000"')) + .mockResolvedValueOnce(new Response('SIGNING_KEY = "GATTACKER0000000000000000000000000000000000000000000"')); + const warn = vi.fn(); + const options = { + fetch: fetcher, + ttlMs: 60_000, + logger: { debug: vi.fn(), info: vi.fn(), warn, error: vi.fn() }, + }; + + const first = await fetchStellarToml("example.com", options); + expect(first.status).toBe("ok"); + if (first.status === "ok") expect(first.data.SIGNING_KEY).toContain("GOLDKEY"); + + const second = await fetchStellarToml("example.com", options); + + expect(fetcher).toHaveBeenCalledTimes(2); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("diverged"), + expect.objectContaining({ domain: "example.com" }), + ); + expect(second.status).toBe("ok"); + if (second.status === "ok") { + // Returns the fresh (correct) data, not the stale cached one. + expect(second.data.SIGNING_KEY).toContain("GATTACKER"); + } + + // A third call must see the (now updated) cache agree with another + // fresh fetch and must not warn again. + const thirdFetcher = vi.fn().mockResolvedValue( + new Response('SIGNING_KEY = "GATTACKER0000000000000000000000000000000000000000000"'), + ); + const third = await fetchStellarToml("example.com", { ...options, fetch: thirdFetcher }); + expect(third.status).toBe("ok"); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("falls back to the cached value when the re-verification fetch itself fails", async () => { + const fetcher = vi + .fn() + .mockResolvedValueOnce(new Response('AUTH_SERVER = "https://auth.example.com"')) + .mockRejectedValueOnce(new Error("network blip")); + const options = { fetch: fetcher, ttlMs: 60_000 }; + + const first = await fetchStellarToml("example.com", options); + expect(first.status).toBe("ok"); + + const second = await fetchStellarToml("example.com", options); + expect(second.status).toBe("ok"); + if (second.status === "ok") { + expect(second.data.auth_server).toBe("https://auth.example.com"); + } + }); + it("fetches again after the configured TTL expires", async () => { vi.useFakeTimers(); try { diff --git a/src/integration/sep1Toml.ts b/src/integration/sep1Toml.ts index 8b1c42c7..c71f8126 100644 --- a/src/integration/sep1Toml.ts +++ b/src/integration/sep1Toml.ts @@ -1,6 +1,7 @@ import { parse } from "smol-toml"; import { err, ok, SorokitErrorCode } from "../shared/response"; import type { SorokitResult } from "../shared/response"; +import type { SorokitLogger } from "../shared/logger"; export interface StellarToml { FEDERATION_SERVER?: string; @@ -24,12 +25,74 @@ export interface FetchStellarTomlOptions { ttlMs?: number; timeoutMs?: number; fetch?: typeof fetch; + /** + * When true (the default), a cache hit is verified against a fresh + * network fetch before being trusted. If the fresh document's + * SIGNING_KEY or any CURRENCIES issuer diverges from what's cached, the + * cache is invalidated, a warning is logged, and the fresh (not the + * stale) document is returned (#837). + * + * Set to false to skip verification and always trust a cache hit + * immediately (the old behavior) — useful for latency-sensitive callers + * that already re-verify issuer identity another way. + */ + verifyOnCacheHit?: boolean; + /** Logger for conflict warnings. Defaults to a no-op logger. */ + logger?: SorokitLogger; +} + +/** Extract issuer addresses from a parsed TOML's CURRENCIES table, if present. */ +function currencyIssuers(toml: StellarToml): Set { + const issuers = new Set(); + const currencies = (toml as Record).CURRENCIES; + if (!Array.isArray(currencies)) return issuers; + for (const entry of currencies) { + if (entry && typeof entry === "object" && typeof (entry as Record).issuer === "string") { + issuers.add((entry as Record).issuer as string); + } + } + return issuers; +} + +/** + * Detect whether two stellar.toml documents disagree on security-critical + * fields: the SEP-10 SIGNING_KEY, or any CURRENCIES issuer address (#837). + * Returns a human-readable description of the conflict, or undefined when + * the two documents agree on everything checked. + */ +export function detectStellarTomlConflict( + cached: StellarToml, + fresh: StellarToml, +): string | undefined { + if (cached.SIGNING_KEY && fresh.SIGNING_KEY && cached.SIGNING_KEY !== fresh.SIGNING_KEY) { + return `SIGNING_KEY changed from "${cached.SIGNING_KEY}" to "${fresh.SIGNING_KEY}"`; + } + const cachedIssuers = currencyIssuers(cached); + const freshIssuers = currencyIssuers(fresh); + for (const issuer of freshIssuers) { + if (cachedIssuers.size > 0 && !cachedIssuers.has(issuer)) { + return `CURRENCIES issuer set changed (new issuer "${issuer}" not present in cached document)`; + } + } + for (const issuer of cachedIssuers) { + if (!freshIssuers.has(issuer)) { + return `CURRENCIES issuer set changed (previously-cached issuer "${issuer}" is no longer present)`; + } + } + return undefined; } export const DEFAULT_STELLAR_TOML_CACHE_TTL_MS = 5 * 60 * 1000; const tomlCache = new Map(); +const NOOP_LOGGER: SorokitLogger = { + debug: () => undefined, + info: () => undefined, + warn: () => undefined, + error: () => undefined, +}; + function domainUrl(domain: string): URL | null { if (typeof domain !== "string" || !domain.trim()) return null; try { @@ -89,24 +152,12 @@ export function cacheStellarToml( }); } -/** Fetch and parse a domain's SEP-1 `/.well-known/stellar.toml` document. */ -export async function fetchStellarToml( +/** Perform the actual network fetch + parse, without touching the cache. */ +async function fetchStellarTomlFresh( domain: string, - options: FetchStellarTomlOptions = {}, + baseUrl: URL, + options: FetchStellarTomlOptions, ): Promise> { - const baseUrl = domainUrl(domain); - if (!baseUrl) { - return err(SorokitErrorCode.INVALID_ADDRESS, "A valid HTTPS domain is required."); - } - - const ttlMs = options.ttlMs ?? DEFAULT_STELLAR_TOML_CACHE_TTL_MS; - if (!Number.isFinite(ttlMs) || ttlMs < 0) { - return err(SorokitErrorCode.VALIDATION, "Cache TTL must be a non-negative finite number."); - } - - const cached = getCachedStellarToml(domain); - if (cached) return ok(cached); - const fetcher = options.fetch ?? globalThis.fetch; if (typeof fetcher !== "function") { return err(SorokitErrorCode.NETWORK_ERROR, "Fetch is unavailable in this runtime."); @@ -129,19 +180,15 @@ export async function fetchStellarToml( return err(SorokitErrorCode.NETWORK_ERROR, `stellar.toml request failed (${response.status}).`); } - let data: StellarToml; try { const parsed: unknown = parse(await response.text()); if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { throw new Error("The TOML document must contain a table."); } - data = normalizeStellarToml(parsed as Record); + return ok(normalizeStellarToml(parsed as Record)); } catch (cause) { return err(SorokitErrorCode.INVALID_CONFIG, "The domain returned invalid stellar.toml.", cause); } - - cacheStellarToml(domain, data, ttlMs); - return ok(data); } catch (cause) { return err(SorokitErrorCode.NETWORK_ERROR, "Failed to fetch stellar.toml.", cause); } finally { @@ -149,6 +196,61 @@ export async function fetchStellarToml( } } +/** Fetch and parse a domain's SEP-1 `/.well-known/stellar.toml` document. */ +export async function fetchStellarToml( + domain: string, + options: FetchStellarTomlOptions = {}, +): Promise> { + const baseUrl = domainUrl(domain); + if (!baseUrl) { + return err(SorokitErrorCode.INVALID_ADDRESS, "A valid HTTPS domain is required."); + } + + const ttlMs = options.ttlMs ?? DEFAULT_STELLAR_TOML_CACHE_TTL_MS; + if (!Number.isFinite(ttlMs) || ttlMs < 0) { + return err(SorokitErrorCode.VALIDATION, "Cache TTL must be a non-negative finite number."); + } + + const cached = getCachedStellarToml(domain); + if (cached) { + const verifyOnCacheHit = options.verifyOnCacheHit ?? true; + if (!verifyOnCacheHit) return ok(cached); + + // #837: re-verify the cached issuer data against a fresh fetch before + // trusting it, instead of returning a possibly stale/conflicting + // cached document indefinitely. + const fresh = await fetchStellarTomlFresh(domain, baseUrl, options); + if (fresh.status !== "ok") { + // The re-verification fetch itself failed (network blip, etc). Fall + // back to the cached data rather than failing the whole call — the + // cache has not been proven stale, only unverifiable right now. + return ok(cached); + } + + const conflict = detectStellarTomlConflict(cached, fresh.data); + if (conflict) { + (options.logger ?? NOOP_LOGGER).warn( + `stellar.toml for "${domain}" diverged from cached data: ${conflict}. Invalidating cache.`, + { domain, conflict }, + ); + clearStellarTomlCache(domain); + cacheStellarToml(domain, fresh.data, ttlMs); + return ok(fresh.data); + } + + // No conflict — refresh the cache entry's TTL with the confirmed-fresh + // data and return it. + cacheStellarToml(domain, fresh.data, ttlMs); + return ok(fresh.data); + } + + const fresh = await fetchStellarTomlFresh(domain, baseUrl, options); + if (fresh.status !== "ok") return fresh; + + cacheStellarToml(domain, fresh.data, ttlMs); + return ok(fresh.data); +} + /** Clear all cached stellar.toml documents, or only one domain's entry. */ export function clearStellarTomlCache(domain?: string): void { if (domain === undefined) { From dbc17c814859de24b1dfe9f93466af0d1b6fe515 Mon Sep 17 00:00:00 2001 From: David Ejere Date: Wed, 21 Oct 2026 08:09:20 +0100 Subject: [PATCH 2/3] fix(transaction): enforce size and complexity limits in validateTransactionXdr (#838) validateTransactionXdr parsed any input string with TransactionBuilder.fromXDR with no upper bound on length and no cap on the resulting operation count, making XDR decoding a CPU/memory DOS vector for an oversized or deeply structured payload. Reject XDR strings longer than maxXdrLength (default: 100_000 chars) before attempting to parse them at all, and reject a successfully-parsed transaction whose operation count exceeds maxOperationCount (default: 100, the protocol's own cap) before the per-operation validation loop runs. Both limits are configurable via ValidationRules for callers with different needs. --- .../validate-transaction-xdr-limits.test.ts | 142 ++++++++++++++++++ src/transaction/validateTransactionXdr.ts | 63 +++++++- 2 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 src/tests/validate-transaction-xdr-limits.test.ts diff --git a/src/tests/validate-transaction-xdr-limits.test.ts b/src/tests/validate-transaction-xdr-limits.test.ts new file mode 100644 index 00000000..ed1946c8 --- /dev/null +++ b/src/tests/validate-transaction-xdr-limits.test.ts @@ -0,0 +1,142 @@ +/** + * Tests for validateTransactionXdr's size and structural-complexity limits + * (#838). Kept in a standalone file (not transaction.test.ts, which mocks + * "@stellar/stellar-sdk" globally) so these tests exercise the real SDK's + * TransactionBuilder/Account for genuine XDR encode/decode round trips. + */ +import { describe, it, expect } from "vitest"; +import { + Keypair, + Account, + TransactionBuilder, + Operation, + Asset, + BASE_FEE, + Networks, +} from "@stellar/stellar-sdk"; +import { validateTransactionXdr } from "../transaction/validateTransactionXdr"; + +function buildSamplePaymentXdr(opts?: { fee?: string }): string { + const source = Keypair.random(); + const dest = Keypair.random().publicKey(); + const account = new Account(source.publicKey(), "1"); + const tx = new TransactionBuilder(account, { + fee: opts?.fee ?? BASE_FEE, + networkPassphrase: Networks.TESTNET, + }) + .addOperation( + Operation.payment({ destination: dest, asset: Asset.native(), amount: "10" }), + ) + .setTimeout(100) + .build(); + return tx.toXDR(); +} + +describe("validateTransactionXdr — size limit (#838)", () => { + it("accepts a well-formed XDR within the default size limit", () => { + const xdr = buildSamplePaymentXdr(); + const result = validateTransactionXdr(xdr, { networkPassphrase: Networks.TESTNET }); + expect(result.status).toBe("ok"); + if (result.status !== "ok") return; + expect(result.data.valid).toBe(true); + }); + + it("rejects an XDR longer than maxXdrLength without attempting to parse it", () => { + const oversized = "A".repeat(200_000); + const result = validateTransactionXdr(oversized, { + networkPassphrase: Networks.TESTNET, + }); + expect(result.status).toBe("ok"); + if (result.status !== "ok") return; + expect(result.data.valid).toBe(false); + expect(result.data.errors[0]?.code).toBe("XDR_TOO_LARGE"); + expect(result.data.operationCount).toBe(0); + }); + + it("rejects an XDR longer than a custom maxXdrLength", () => { + const xdr = buildSamplePaymentXdr(); + const result = validateTransactionXdr(xdr, { + networkPassphrase: Networks.TESTNET, + maxXdrLength: 10, + }); + expect(result.status).toBe("ok"); + if (result.status !== "ok") return; + expect(result.data.valid).toBe(false); + expect(result.data.errors[0]?.code).toBe("XDR_TOO_LARGE"); + }); + + it("accepts an XDR exactly at the configured maxXdrLength boundary", () => { + const xdr = buildSamplePaymentXdr(); + const result = validateTransactionXdr(xdr, { + networkPassphrase: Networks.TESTNET, + maxXdrLength: xdr.length, + }); + expect(result.status).toBe("ok"); + if (result.status !== "ok") return; + expect(result.data.valid).toBe(true); + }); + + it("the size check runs before XDR parsing — an oversized malformed string is reported as too-large, not XDR_INVALID", () => { + const oversizedGarbage = "not-xdr-".repeat(30_000); + const result = validateTransactionXdr(oversizedGarbage, { + networkPassphrase: Networks.TESTNET, + }); + expect(result.status).toBe("ok"); + if (result.status !== "ok") return; + expect(result.data.errors[0]?.code).toBe("XDR_TOO_LARGE"); + }); +}); + +describe("validateTransactionXdr — structural complexity limit (#838)", () => { + function buildMultiOpPaymentXdr(opCount: number): string { + const source = Keypair.random(); + const account = new Account(source.publicKey(), "1"); + const builder = new TransactionBuilder(account, { + fee: String(Number(BASE_FEE) * opCount), + networkPassphrase: Networks.TESTNET, + }); + for (let i = 0; i < opCount; i++) { + builder.addOperation( + Operation.payment({ + destination: Keypair.random().publicKey(), + asset: Asset.native(), + amount: "1", + }), + ); + } + return builder.setTimeout(100).build().toXDR(); + } + + it("accepts a transaction within the default operation-count limit", () => { + const xdr = buildMultiOpPaymentXdr(5); + const result = validateTransactionXdr(xdr, { networkPassphrase: Networks.TESTNET }); + expect(result.status).toBe("ok"); + if (result.status !== "ok") return; + expect(result.data.valid).toBe(true); + expect(result.data.operationCount).toBe(5); + }); + + it("rejects a transaction exceeding a custom maxOperationCount before running per-operation checks", () => { + const xdr = buildMultiOpPaymentXdr(10); + const result = validateTransactionXdr(xdr, { + networkPassphrase: Networks.TESTNET, + maxOperationCount: 3, + }); + expect(result.status).toBe("ok"); + if (result.status !== "ok") return; + expect(result.data.valid).toBe(false); + expect(result.data.errors.some((f) => f.code === "TOO_MANY_OPERATIONS")).toBe(true); + expect(result.data.operationCount).toBe(10); + }); + + it("accepts a transaction exactly at the configured maxOperationCount boundary", () => { + const xdr = buildMultiOpPaymentXdr(4); + const result = validateTransactionXdr(xdr, { + networkPassphrase: Networks.TESTNET, + maxOperationCount: 4, + }); + expect(result.status).toBe("ok"); + if (result.status !== "ok") return; + expect(result.data.valid).toBe(true); + }); +}); diff --git a/src/transaction/validateTransactionXdr.ts b/src/transaction/validateTransactionXdr.ts index 4650ff20..e14d345e 100644 --- a/src/transaction/validateTransactionXdr.ts +++ b/src/transaction/validateTransactionXdr.ts @@ -34,13 +34,31 @@ export interface ValidationRules { op: { type: string; [key: string]: unknown }, index: number, ) => TransactionValidationFinding[] | null; + /** + * Maximum accepted length of the base64-encoded XDR string, in + * characters. Rejecting oversized envelopes before attempting to parse + * them bounds the CPU/memory cost of a malicious or malformed input + * (#838). Default: 100_000 (base64 chars) — far beyond any legitimate + * Stellar transaction, which is bounded on-chain to 100 operations and a + * handful of KB of XDR. + */ + maxXdrLength?: number; + /** + * Maximum number of operations a transaction may contain before being + * rejected outright, bounding the structural complexity of what gets + * iterated below (#838). Default: 100 (the protocol's own operations- + * per-transaction cap). + */ + maxOperationCount?: number; } export const DEFAULT_VALIDATION_RULES: Required< - Pick + Pick > = { maxFeePerOpStroops: 1_000_000, minFeeStroops: 100, + maxXdrLength: 100_000, + maxOperationCount: 100, }; function isValidStellarAddress(addr: unknown): addr is string { @@ -74,6 +92,28 @@ export function validateTransactionXdr( const findings: TransactionValidationFinding[] = []; const maxFeePerOp = rules.maxFeePerOpStroops ?? DEFAULT_VALIDATION_RULES.maxFeePerOpStroops; const minFee = rules.minFeeStroops ?? DEFAULT_VALIDATION_RULES.minFeeStroops; + const maxXdrLength = rules.maxXdrLength ?? DEFAULT_VALIDATION_RULES.maxXdrLength; + const maxOperationCount = rules.maxOperationCount ?? DEFAULT_VALIDATION_RULES.maxOperationCount; + + // ── Size limit (#838) ─────────────────────────────────────────────────── + // Reject oversized envelopes before attempting to parse them at all, so + // an attacker cannot spend CPU/memory on XDR decoding just by sending a + // very large payload. + if (typeof xdr !== "string" || xdr.length > maxXdrLength) { + findings.push({ + severity: "error", + code: "XDR_TOO_LARGE", + message: `XDR length (${typeof xdr === "string" ? xdr.length : 0} chars) exceeds the maximum of ${maxXdrLength} chars.`, + }); + return ok({ + valid: false, + findings, + warnings: [], + errors: findings, + operationCount: 0, + fee: null, + }); + } let tx: ReturnType; try { @@ -99,6 +139,27 @@ export function validateTransactionXdr( "operations" in tx ? (tx.operations as never) : []; const fee = "fee" in tx ? String((tx as { fee: string | number }).fee) : null; + // ── Structural complexity limit (#838) ────────────────────────────────── + // Bound the number of operations iterated below. The Stellar protocol + // itself caps a transaction at 100 operations, so a successfully-parsed + // XDR with more than that is either malformed or deliberately crafted to + // maximize validation cost — reject it before the per-operation loop. + if (operations.length > maxOperationCount) { + findings.push({ + severity: "error", + code: "TOO_MANY_OPERATIONS", + message: `Transaction contains ${operations.length} operations, exceeding the maximum of ${maxOperationCount}.`, + }); + return ok({ + valid: false, + findings, + warnings: [], + errors: findings, + operationCount: operations.length, + fee, + }); + } + if (fee !== null) { const feeNum = Number(fee); if (!Number.isFinite(feeNum) || feeNum < minFee) { From 5e9f7cf8b4d14baebe1612382ed98b2516ee4535 Mon Sep 17 00:00:00 2001 From: David Ejere Date: Wed, 21 Oct 2026 08:09:27 +0100 Subject: [PATCH 3/3] fix(soroban): validate required contract parameters when building an invocation (#843) prepareContractCall only validated argument count against cachedMetadata or contractAbi when one of them was explicitly supplied by the caller. Without either, a missing required parameter was silently passed through to simulation and submission, failing on-chain with an unhelpful error after fees were already paid. When neither is supplied, auto-discover the contract's methods from its deployed Wasm via the existing getContractMethods() (already cached/deduplicated), and validate against that instead. Also improve the mismatch error message to name the specific missing parameter(s) by declared name, not just report a count mismatch. --- src/soroban/contractMetadata.ts | 11 +++ src/soroban/prepareCall.ts | 25 +++++- src/tests/soroban.test.ts | 132 +++++++++++++++++++++++++++++++- 3 files changed, 162 insertions(+), 6 deletions(-) diff --git a/src/soroban/contractMetadata.ts b/src/soroban/contractMetadata.ts index 3a4a0e8d..659ade7c 100644 --- a/src/soroban/contractMetadata.ts +++ b/src/soroban/contractMetadata.ts @@ -385,6 +385,17 @@ function validateCachedMetadata( } if (methodMetadata.inputs.length !== argCount) { + // #843: name the specific missing parameters (by declared name) when + // fewer args were supplied than the method requires, instead of only + // reporting a count mismatch, so the caller knows exactly what to add. + if (argCount < methodMetadata.inputs.length) { + const missing = methodMetadata.inputs.slice(argCount).map((input) => input.name); + return err( + SorokitErrorCode.CONTRACT_PREPARE_FAILED, + `Contract method ${method} is missing required parameter(s): ${missing.join(", ")}. ` + + `Expected ${methodMetadata.inputs.length} argument(s), received ${argCount}.`, + ); + } return err( SorokitErrorCode.CONTRACT_PREPARE_FAILED, `Contract method ${method} expects ${methodMetadata.inputs.length} argument(s), received ${argCount}.`, diff --git a/src/soroban/prepareCall.ts b/src/soroban/prepareCall.ts index cfc99a51..fd6d2e76 100644 --- a/src/soroban/prepareCall.ts +++ b/src/soroban/prepareCall.ts @@ -19,7 +19,7 @@ import { DEFAULT_SOROBAN_TX_TIMEOUT_SECONDS } from "../shared/constants"; import type { ResolvedNetworkConfig } from "../shared/types"; import type { ContractInvokeParams, PreparedContractCall } from "./types"; import { validatePublicKey } from "../shared/validation"; -import { validateContractMethodMetadata, validateContractArgs } from "./contractMetadata"; +import { validateContractMethodMetadata, validateContractArgs, getContractMethods } from "./contractMetadata"; import { validateContractAbi } from "./validateContractAbi"; import { createHorizonServer, createSorobanServer } from "../shared/serverFactory"; import { CircuitBreakerRegistry } from "../network/circuitBreaker"; @@ -88,8 +88,25 @@ export async function prepareContractCall( }); if (abiValidation.status === "error") return abiValidation; + // #843: when the caller supplies neither a cachedMetadata list nor a + // contractAbi, fall back to discovering the contract's methods from its + // deployed Wasm (already cached/deduplicated by getContractMethods) so + // missing required parameters are still caught here, at build time, + // instead of being silently passed through to simulation and failing + // on-chain with an unhelpful error. + let effectiveMetadata = params.cachedMetadata; + if (!effectiveMetadata && !params.contractAbi) { + const discovered = await getContractMethods(rpcUrl, params.contractId); + if (discovered.status === "ok") { + effectiveMetadata = discovered.data; + } + // If discovery itself fails (e.g. RPC unreachable), proceed without + // metadata rather than failing prepareContractCall over a validation + // nicety — the simulation step below will still catch real problems. + } + const metadataResult = validateContractMethodMetadata( - params.cachedMetadata, + effectiveMetadata, params.method, params.args?.length ?? 0, SorokitErrorCode.CONTRACT_PREPARE_FAILED, @@ -97,8 +114,8 @@ export async function prepareContractCall( if (metadataResult.status === "error") return metadataResult; // ─── Visibility, authorization, and argument validation ─────────────── - if (params.cachedMetadata) { - const methodMeta = params.cachedMetadata.find((m) => m.name === params.method); + if (effectiveMetadata) { + const methodMeta = effectiveMetadata.find((m) => m.name === params.method); if (methodMeta) { // Visibility: reject explicitly non-public methods diff --git a/src/tests/soroban.test.ts b/src/tests/soroban.test.ts index 2cd6164c..a857970b 100644 --- a/src/tests/soroban.test.ts +++ b/src/tests/soroban.test.ts @@ -557,7 +557,10 @@ describe("soroban contract metadata", () => { expect(result.status).toBe("error"); if (result.status === "error") { expect(result.error.code).toBe(SorokitErrorCode.CONTRACT_READ_FAILED); - expect(result.error.message).toContain("expects 1 argument"); + // #843: message now names the specific missing parameter. + expect(result.error.message).toContain("missing required parameter"); + expect(result.error.message).toContain("to"); + expect(result.error.message).toContain("Expected 1 argument"); } }); }); @@ -979,6 +982,128 @@ describe("queryContractEvents", () => { }); }); +describe("prepareContractCall — missing required parameter validation (#843)", () => { + beforeEach(() => { + mockGetLedgerEntries.mockReset(); + resetRpcSimulationMocks(); + }); + + it("rejects a call with zero args when neither cachedMetadata nor contractAbi is provided, by auto-discovering the contract's methods", async () => { + // methodSpec() declares "hello" with exactly one required input ("to"). + mockContractLedgerEntries(contractSpecWasm([methodSpec()])); + + const result = await prepareContractCall( + networkConfig.rpcUrl, + networkConfig, + networkConfig.horizonUrl, + { + contractId: contractId(), + method: "hello", + // No args, no cachedMetadata, no contractAbi — the gap #843 describes. + publicKey: Keypair.random().publicKey(), + }, + ); + + expect(result.status).toBe("error"); + if (result.status === "error") { + expect(result.error.code).toBe(SorokitErrorCode.CONTRACT_PREPARE_FAILED); + expect(result.error.message).toContain("missing required parameter"); + expect(result.error.message).toContain("to"); + } + }); + + it("names every missing parameter when more than one is omitted", async () => { + mockContractLedgerEntries( + contractSpecWasm([ + xdr.ScSpecEntry.scSpecEntryFunctionV0( + new xdr.ScSpecFunctionV0({ + doc: "", + name: "transfer", + inputs: [ + new xdr.ScSpecFunctionInputV0({ + doc: "", + name: "from", + type: xdr.ScSpecTypeDef.scSpecTypeAddress(), + }), + new xdr.ScSpecFunctionInputV0({ + doc: "", + name: "to", + type: xdr.ScSpecTypeDef.scSpecTypeAddress(), + }), + new xdr.ScSpecFunctionInputV0({ + doc: "", + name: "amount", + type: xdr.ScSpecTypeDef.scSpecTypeI128(), + }), + ], + outputs: [], + }), + ), + ]), + ); + + const result = await prepareContractCall( + networkConfig.rpcUrl, + networkConfig, + networkConfig.horizonUrl, + { + contractId: contractId(), + method: "transfer", + args: [arg], // only one of three required args supplied + publicKey: Keypair.random().publicKey(), + }, + ); + + expect(result.status).toBe("error"); + if (result.status === "error") { + expect(result.error.message).toContain("to, amount"); + } + }); + + it("succeeds when all required parameters are supplied, using auto-discovered metadata", async () => { + mockContractLedgerEntries(contractSpecWasm([methodSpec()])); + + const result = await prepareContractCall( + networkConfig.rpcUrl, + networkConfig, + networkConfig.horizonUrl, + { + contractId: contractId(), + method: "hello", + // methodSpec()'s "to" input is typed symbol, matching this arg's type. + args: [xdr.ScVal.scvSymbol("alice")], + publicKey: Keypair.random().publicKey(), + }, + ); + + expect(result.status).toBe("ok"); + }); + + it("still validates argument count when cachedMetadata is explicitly provided (pre-existing behavior, unchanged)", async () => { + const result = await prepareContractCall( + networkConfig.rpcUrl, + networkConfig, + networkConfig.horizonUrl, + { + contractId: contractId(), + method: "hello", + publicKey: Keypair.random().publicKey(), + cachedMetadata: [ + { name: "hello", inputs: [{ name: "to", type: "symbol" }], returnType: "string" }, + ], + }, + ); + + expect(result.status).toBe("error"); + if (result.status === "error") { + expect(result.error.message).toContain("missing required parameter"); + expect(result.error.message).toContain("to"); + } + // cachedMetadata was supplied, so auto-discovery must not have run. + expect(mockGetLedgerEntries).not.toHaveBeenCalled(); + }); +}); + describe.skip("soroban contract ABI validation", () => { beforeEach(() => { mockGetLedgerEntries.mockReset(); @@ -3508,7 +3633,10 @@ describe("contract method visibility and authorization (#383)", () => { expect(result.status).toBe("error"); if (result.status === "error") { expect(result.error.code).toBe(SorokitErrorCode.CONTRACT_PREPARE_FAILED); - expect(result.error.message).toContain("expects 1 argument"); + // #843: message now names the specific missing parameter. + expect(result.error.message).toContain("missing required parameter"); + expect(result.error.message).toContain("amount"); + expect(result.error.message).toContain("Expected 1 argument"); } expect(mockSimulateTransaction).not.toHaveBeenCalled(); });