From 46e814b7ae3d1077c1cb8982cadaa9cf447ae3a9 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:05:25 +0200 Subject: [PATCH 01/27] feat(signer): scaffold the package and enforce the no-network boundary The signer's security claim is that it cannot reach the network, not that it chooses not to. test/boundary.test.ts enforces that mechanically over both the import graph and the manifest. --- package-lock.json | 20 +++++++++ package.json | 4 +- packages/signer/package.json | 21 ++++++++++ packages/signer/src/index.ts | 3 ++ packages/signer/test/boundary.test.ts | 58 +++++++++++++++++++++++++++ packages/signer/tsconfig.json | 14 +++++++ packages/signer/tsup.config.ts | 11 +++++ 7 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 packages/signer/package.json create mode 100644 packages/signer/src/index.ts create mode 100644 packages/signer/test/boundary.test.ts create mode 100644 packages/signer/tsconfig.json create mode 100644 packages/signer/tsup.config.ts diff --git a/package-lock.json b/package-lock.json index fa49ee6..7ce802c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,6 +42,10 @@ "resolved": "packages/shared", "link": true }, + "node_modules/@apexfusion/vector-mcp-signer": { + "resolved": "packages/signer", + "link": true + }, "node_modules/@aws-sdk/checksums": { "version": "3.1000.20", "resolved": "https://registry.npmjs.org/@aws-sdk/checksums/-/checksums-3.1000.20.tgz", @@ -7893,6 +7897,22 @@ "@noble/hashes": "^1.7.1", "cross-fetch": "^4.0.0" } + }, + "packages/signer": { + "name": "@apexfusion/vector-mcp-signer", + "version": "0.1.0", + "dependencies": { + "@anastasia-labs/cardano-multiplatform-lib-nodejs": "6.0.2-3", + "@lucid-evolution/wallet": "^0.1.72", + "@modelcontextprotocol/sdk": "^1.4.1", + "zod": "^3.24.1" + }, + "bin": { + "vector-mcp-signer": "build/index.js" + }, + "devDependencies": { + "@apexfusion/vector-mcp-shared": "*" + } } } } diff --git a/package.json b/package.json index 821206d..f8fcd80 100644 --- a/package.json +++ b/package.json @@ -7,8 +7,8 @@ "scripts": { "postinstall": "ln -sf ../../../libsodium-sumo/dist/modules-sumo-esm/libsodium-sumo.mjs node_modules/libsodium-wrappers-sumo/dist/modules-sumo-esm/libsodium-sumo.mjs 2>/dev/null || true", "clean": "rm -rf packages/*/build", - "build": "npm run build --workspace=@apexfusion/vector-mcp-builder", - "typecheck": "tsc --noEmit -p packages/shared && tsc --noEmit -p packages/builder", + "build": "npm run build --workspace=@apexfusion/vector-mcp-builder && npm run build --workspace=@apexfusion/vector-mcp-signer", + "typecheck": "tsc --noEmit -p packages/shared && tsc --noEmit -p packages/builder && tsc --noEmit -p packages/signer", "start": "node packages/builder/build/index.js", "test": "npm run test:unit", "test:unit": "node --import tsx --test packages/*/test/*.test.ts", diff --git a/packages/signer/package.json b/packages/signer/package.json new file mode 100644 index 0000000..712a47c --- /dev/null +++ b/packages/signer/package.json @@ -0,0 +1,21 @@ +{ + "name": "@apexfusion/vector-mcp-signer", + "version": "0.1.0", + "private": true, + "type": "module", + "description": "Local signing companion for the Vector MCP server. Holds the key; no network.", + "bin": { "vector-mcp-signer": "./build/index.js" }, + "scripts": { + "build": "tsup && chmod +x build/index.js", + "start": "node build/index.js" + }, + "dependencies": { + "@anastasia-labs/cardano-multiplatform-lib-nodejs": "6.0.2-3", + "@lucid-evolution/wallet": "^0.1.72", + "@modelcontextprotocol/sdk": "^1.4.1", + "zod": "^3.24.1" + }, + "devDependencies": { + "@apexfusion/vector-mcp-shared": "*" + } +} diff --git a/packages/signer/src/index.ts b/packages/signer/src/index.ts new file mode 100644 index 0000000..862da72 --- /dev/null +++ b/packages/signer/src/index.ts @@ -0,0 +1,3 @@ +// Local signing companion for the Vector MCP server. +// Speaks stdio only: no HTTP listener, no outbound network. See test/boundary.test.ts. +console.error('vector-mcp-signer: not yet implemented'); diff --git a/packages/signer/test/boundary.test.ts b/packages/signer/test/boundary.test.ts new file mode 100644 index 0000000..df55f4f --- /dev/null +++ b/packages/signer/test/boundary.test.ts @@ -0,0 +1,58 @@ +import { describe, test } from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync, readdirSync } from 'node:fs'; +import { join } from 'node:path'; + +// The signer's security claim is that it CANNOT reach the network — not that it +// chooses not to. These assertions are that claim, mechanically enforced. +// Weakening them defeats the purpose of the package. + +const PKG_DIR = new URL('..', import.meta.url).pathname.replace(/^\/([A-Za-z]:)/, '$1'); +const SRC_DIR = join(PKG_DIR, 'src'); + +const FORBIDDEN_IMPORTS = [ + '@apexfusion/vector-mcp-shared/provider', + '@lucid-evolution/lucid', + 'cross-fetch', + 'node:http', + 'node:https', + 'node:net', + 'node:dgram', + 'node:tls', +]; + +function sourceFiles(dir: string): string[] { + return readdirSync(dir, { withFileTypes: true }).flatMap((e) => + e.isDirectory() ? sourceFiles(join(dir, e.name)) : e.name.endsWith('.ts') ? [join(dir, e.name)] : [] + ); +} + +describe('signer has no network capability', () => { + test('no source file imports a network-capable module', () => { + const offenders: string[] = []; + for (const file of sourceFiles(SRC_DIR)) { + const text = readFileSync(file, 'utf8'); + for (const bad of FORBIDDEN_IMPORTS) { + if (text.includes(`'${bad}'`) || text.includes(`"${bad}"`)) { + offenders.push(`${file} imports ${bad}`); + } + } + } + assert.deepStrictEqual(offenders, [], `network-capable imports found:\n ${offenders.join('\n ')}`); + }); + + test('package.json declares no network-capable dependency', () => { + const pkg = JSON.parse(readFileSync(join(PKG_DIR, 'package.json'), 'utf8')); + const declared = Object.keys({ ...pkg.dependencies, ...pkg.devDependencies }); + const bad = declared.filter((d) => d === '@lucid-evolution/lucid' || d === 'cross-fetch'); + assert.deepStrictEqual(bad, [], `network-capable dependencies declared: ${bad.join(', ')}`); + }); + + test('does not depend on the shared provider subpath', () => { + const pkg = JSON.parse(readFileSync(join(PKG_DIR, 'package.json'), 'utf8')); + const shared = { ...pkg.dependencies, ...pkg.devDependencies }['@apexfusion/vector-mcp-shared']; + // Depending on the package is fine — /tx is pure. Reaching /provider is not, + // and the shared package's export map has no root export to smuggle it through. + assert.ok(shared === undefined || typeof shared === 'string'); + }); +}); diff --git a/packages/signer/tsconfig.json b/packages/signer/tsconfig.json new file mode 100644 index 0000000..66ea60f --- /dev/null +++ b/packages/signer/tsconfig.json @@ -0,0 +1,14 @@ +{ + "compilerOptions": { + "target": "es2020", + "module": "esnext", + "moduleResolution": "bundler", + "allowSyntheticDefaultImports": true, + "esModuleInterop": true, + "strict": true, + "skipLibCheck": true, + "noEmit": true, + "resolveJsonModule": true + }, + "include": ["src/**/*", "test/**/*"] +} diff --git a/packages/signer/tsup.config.ts b/packages/signer/tsup.config.ts new file mode 100644 index 0000000..7ced406 --- /dev/null +++ b/packages/signer/tsup.config.ts @@ -0,0 +1,11 @@ +import { defineConfig } from "tsup"; + +export default defineConfig({ + entry: ["src/index.ts"], + format: ["esm"], + target: "es2020", + outDir: "build", + clean: true, + splitting: false, + banner: { js: "#!/usr/bin/env node" }, +}); From 34b2d7fa18b18553d721d0a49951f10efa9247d6 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:13:15 +0200 Subject: [PATCH 02/27] test(signer): make the no-network boundary test able to actually fail Two holes: the shared-dependency assertion was a tautology that held for any manifest, and ambient globals (fetch, WebSocket) need no import so the import scan was blind to them. Both now enforced, and both verified to fail when a violation is injected. --- packages/signer/test/boundary.test.ts | 53 ++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/packages/signer/test/boundary.test.ts b/packages/signer/test/boundary.test.ts index df55f4f..13b4300 100644 --- a/packages/signer/test/boundary.test.ts +++ b/packages/signer/test/boundary.test.ts @@ -21,6 +21,27 @@ const FORBIDDEN_IMPORTS = [ 'node:tls', ]; +const SHARED_PKG = '@apexfusion/vector-mcp-shared'; +// /tx and /types are pure. /provider is network-capable. /config is deliberately +// excluded too: it carries network endpoint URLs the signer has no use for and +// should not be reaching into. A bare root import (no subpath at all) is excluded +// as well. This is an allowlist, not a denylist — anything not named here fails, +// including forms nobody has thought to forbid yet. +const ALLOWED_SHARED_SUBPATHS = [`${SHARED_PKG}/tx`, `${SHARED_PKG}/types`]; + +// fetch, WebSocket, EventSource, XMLHttpRequest, and navigator.sendBeacon are +// globals in Node 22 — no import statement exists for them, so the import-graph +// scan above is structurally blind to their use. These patterns are matched with +// word boundaries so they don't false-positive on legitimate identifiers like a +// variable named `prefetchCount`. +const AMBIENT_NETWORK_GLOBALS: { label: string; pattern: RegExp }[] = [ + { label: 'fetch(...)', pattern: /\bfetch\s*\(/ }, + { label: 'new WebSocket', pattern: /\bnew\s+WebSocket\b/ }, + { label: 'new EventSource', pattern: /\bnew\s+EventSource\b/ }, + { label: 'new XMLHttpRequest', pattern: /\bnew\s+XMLHttpRequest\b/ }, + { label: 'navigator.sendBeacon', pattern: /\bnavigator\s*\.\s*sendBeacon\b/ }, +]; + function sourceFiles(dir: string): string[] { return readdirSync(dir, { withFileTypes: true }).flatMap((e) => e.isDirectory() ? sourceFiles(join(dir, e.name)) : e.name.endsWith('.ts') ? [join(dir, e.name)] : [] @@ -48,11 +69,31 @@ describe('signer has no network capability', () => { assert.deepStrictEqual(bad, [], `network-capable dependencies declared: ${bad.join(', ')}`); }); - test('does not depend on the shared provider subpath', () => { - const pkg = JSON.parse(readFileSync(join(PKG_DIR, 'package.json'), 'utf8')); - const shared = { ...pkg.dependencies, ...pkg.devDependencies }['@apexfusion/vector-mcp-shared']; - // Depending on the package is fine — /tx is pure. Reaching /provider is not, - // and the shared package's export map has no root export to smuggle it through. - assert.ok(shared === undefined || typeof shared === 'string'); + test('only imports the shared package via an allowed pure subpath', () => { + const specifierPattern = new RegExp(`['"](${SHARED_PKG}(?:/[^'"]*)?)['"]`, 'g'); + const offenders: string[] = []; + for (const file of sourceFiles(SRC_DIR)) { + const text = readFileSync(file, 'utf8'); + for (const match of text.matchAll(specifierPattern)) { + const specifier = match[1]; + if (!ALLOWED_SHARED_SUBPATHS.includes(specifier)) { + offenders.push(`${file} imports '${specifier}'`); + } + } + } + assert.deepStrictEqual(offenders, [], `disallowed shared-package specifier found:\n ${offenders.join('\n ')}`); + }); + + test('no source file uses an ambient network global', () => { + const offenders: string[] = []; + for (const file of sourceFiles(SRC_DIR)) { + const text = readFileSync(file, 'utf8'); + for (const { label, pattern } of AMBIENT_NETWORK_GLOBALS) { + if (pattern.test(text)) { + offenders.push(`${file} uses ${label}`); + } + } + } + assert.deepStrictEqual(offenders, [], `ambient network globals found:\n ${offenders.join('\n ')}`); }); }); From de6ae72db9c94f3384b9b966b584e8953dcdffcd Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:30:10 +0200 Subject: [PATCH 03/27] test(signer): close the shell-out bypass and three evasions in the boundary test Review found node:child_process had zero coverage across all four assertions: execSync('curl ...') would have passed every one. Forbidden now, and verified to fail when injected. Also closed: backtick-quoted specifiers evaded the substring and regex scans; sourceFiles() only walked .ts so a stray .js was invisible; the dependency check ignored optional/peer deps and named only two packages. --- packages/signer/test/boundary.test.ts | 48 ++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/packages/signer/test/boundary.test.ts b/packages/signer/test/boundary.test.ts index 13b4300..91f7c03 100644 --- a/packages/signer/test/boundary.test.ts +++ b/packages/signer/test/boundary.test.ts @@ -19,6 +19,12 @@ const FORBIDDEN_IMPORTS = [ 'node:net', 'node:dgram', 'node:tls', + // Reaching the network by shelling out (execSync('curl ...')) bypasses every + // other assertion here, so the process-spawning modules are forbidden too. + 'node:child_process', + 'child_process', + // Opens a debugger port. + 'node:inspector', ]; const SHARED_PKG = '@apexfusion/vector-mcp-shared'; @@ -42,19 +48,33 @@ const AMBIENT_NETWORK_GLOBALS: { label: string; pattern: RegExp }[] = [ { label: 'navigator.sendBeacon', pattern: /\bnavigator\s*\.\s*sendBeacon\b/ }, ]; +// Any module format that Node can execute, not just .ts — a stray .js or .mjs +// under src/ would otherwise be invisible to every scan below. +const SOURCE_EXTENSIONS = ['.ts', '.mts', '.cts', '.js', '.mjs', '.cjs']; + function sourceFiles(dir: string): string[] { return readdirSync(dir, { withFileTypes: true }).flatMap((e) => - e.isDirectory() ? sourceFiles(join(dir, e.name)) : e.name.endsWith('.ts') ? [join(dir, e.name)] : [] + e.isDirectory() + ? sourceFiles(join(dir, e.name)) + : SOURCE_EXTENSIONS.some((ext) => e.name.endsWith(ext)) + ? [join(dir, e.name)] + : [] ); } +// Module specifiers can be single-, double-, or backtick-quoted. Matching only +// the first two left a trivial evasion. +function quotedForms(specifier: string): string[] { + return [`'${specifier}'`, `"${specifier}"`, `\`${specifier}\``]; +} + describe('signer has no network capability', () => { test('no source file imports a network-capable module', () => { const offenders: string[] = []; for (const file of sourceFiles(SRC_DIR)) { const text = readFileSync(file, 'utf8'); for (const bad of FORBIDDEN_IMPORTS) { - if (text.includes(`'${bad}'`) || text.includes(`"${bad}"`)) { + if (quotedForms(bad).some((form) => text.includes(form))) { offenders.push(`${file} imports ${bad}`); } } @@ -64,8 +84,28 @@ describe('signer has no network capability', () => { test('package.json declares no network-capable dependency', () => { const pkg = JSON.parse(readFileSync(join(PKG_DIR, 'package.json'), 'utf8')); - const declared = Object.keys({ ...pkg.dependencies, ...pkg.devDependencies }); - const bad = declared.filter((d) => d === '@lucid-evolution/lucid' || d === 'cross-fetch'); + // All four dependency kinds — optional and peer deps install too. + const declared = Object.keys({ + ...pkg.dependencies, + ...pkg.devDependencies, + ...pkg.optionalDependencies, + ...pkg.peerDependencies, + }); + const NETWORK_PACKAGES = [ + '@lucid-evolution/lucid', + 'cross-fetch', + 'axios', + 'node-fetch', + 'undici', + 'got', + 'ky', + 'superagent', + 'request', + 'ws', + 'socket.io-client', + 'eventsource', + ]; + const bad = declared.filter((d) => NETWORK_PACKAGES.includes(d)); assert.deepStrictEqual(bad, [], `network-capable dependencies declared: ${bad.join(', ')}`); }); From b2eb998e416167541815345ca23e4894a80b1ceb Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:38:59 +0200 Subject: [PATCH 04/27] feat(signer): decode a transaction body into plain data Pure function over CBOR. Everything policy needs lives in the body, which is why the signer never resolves inputs and therefore needs no chain access. Fails closed on anything it cannot fully parse. readAssets uses AssetName.to_hex() (raw bytes, no CBOR envelope) rather than to_cbor_hex().slice(2), which corrupts asset names of 24-32 bytes because the CBOR byte-string header grows from 1 to 2 bytes at length 24. Verified directly against CML at lengths 0-32; the golden fixture carries no native assets so the test suite alone can't catch this. --- packages/signer/src/decode.ts | 96 +++++++++++++++++++++++++++++ packages/signer/test/decode.test.ts | 59 ++++++++++++++++++ 2 files changed, 155 insertions(+) create mode 100644 packages/signer/src/decode.ts create mode 100644 packages/signer/test/decode.test.ts diff --git a/packages/signer/src/decode.ts b/packages/signer/src/decode.ts new file mode 100644 index 0000000..43822db --- /dev/null +++ b/packages/signer/src/decode.ts @@ -0,0 +1,96 @@ +// Decode a transaction into plain data. Pure: no key, no filesystem, no network. +// Everything policy needs is in the transaction body, which is why the signer +// never has to resolve inputs against the chain. +import * as CML from '@anastasia-labs/cardano-multiplatform-lib-nodejs'; + +export interface DecodedOutput { + address: string; + lovelace: bigint; + assets: Array<{ unit: string; quantity: bigint }>; +} + +export interface DecodedTx { + txHashHex: string; + fee: bigint; + outputs: DecodedOutput[]; + inputCount: number; + mintedAssetCount: number; +} + +function readAssets(value: CML.Value): Array<{ unit: string; quantity: bigint }> { + const out: Array<{ unit: string; quantity: bigint }> = []; + const multiasset = value.multi_asset(); + if (!multiasset) return out; + const policies = multiasset.keys(); + for (let p = 0; p < policies.len(); p++) { + const policy = policies.get(p); + const assets = multiasset.get_assets(policy); + if (!assets) continue; + const names = assets.keys(); + for (let n = 0; n < names.len(); n++) { + const name = names.get(n); + const qty = assets.get(name); + if (qty === undefined) continue; + out.push({ + // name.to_hex() returns the asset name's raw bytes as hex, with no CBOR + // envelope. name.to_cbor_hex() would need its CBOR byte-string header + // stripped, and that header is 1 byte only for names up to 23 bytes long; + // Cardano asset names run up to 32 bytes, where the header grows to 2 + // bytes, so a fixed slice(2) silently corrupts the unit for 24-32 byte + // names. to_hex() sidesteps CBOR entirely and is correct at every length + // (verified against CML for lengths 0, 1, 8, 22, 23, 24, 25, 27, 31, 32). + unit: `${policy.to_hex()}${name.to_hex()}`, + quantity: BigInt(qty.toString()), + }); + } + } + return out; +} + +export function decodeTransaction(cborHex: string): DecodedTx { + let tx: CML.Transaction; + try { + tx = CML.Transaction.from_cbor_hex(cborHex); + } catch (err) { + // Fail closed: refuse rather than return something partially understood. + throw new Error( + `Transaction could not be decoded: ${err instanceof Error ? err.message : String(err)}` + ); + } + + const body = tx.body(); + const outputs: DecodedOutput[] = []; + const cmlOutputs = body.outputs(); + for (let i = 0; i < cmlOutputs.len(); i++) { + const o = cmlOutputs.get(i); + let address: string; + try { + address = o.address().to_bech32(); + } catch (err) { + throw new Error(`Transaction could not be decoded: output ${i} has an unreadable address`); + } + outputs.push({ + address, + lovelace: BigInt(o.amount().coin().toString()), + assets: readAssets(o.amount()), + }); + } + + const mint = body.mint(); + let mintedAssetCount = 0; + if (mint) { + const policies = mint.keys(); + for (let p = 0; p < policies.len(); p++) { + const assets = mint.get_assets(policies.get(p)); + if (assets) mintedAssetCount += assets.keys().len(); + } + } + + return { + txHashHex: CML.hash_transaction(body).to_hex(), + fee: BigInt(body.fee().toString()), + outputs, + inputCount: body.inputs().len(), + mintedAssetCount, + }; +} diff --git a/packages/signer/test/decode.test.ts b/packages/signer/test/decode.test.ts new file mode 100644 index 0000000..eac60ec --- /dev/null +++ b/packages/signer/test/decode.test.ts @@ -0,0 +1,59 @@ +import { describe, test } from 'node:test'; +import assert from 'node:assert/strict'; +import { decodeTransaction } from '../src/decode.ts'; + +// Real unsigned transaction from Vector testnet. 1 input, 2 outputs +// (one foreign, one change back to OWN), fee 168223. +const FIXTURE_CBOR = + '84a300d9010281825820e17a2ebab8eae3850f959290041b3bef3f3597584f8cf4a728e14777dddb8c3200018282581d71ab1aad52c4774e5da9f2c0fa1a4d07220a0bdd57ee3dce9be860dac61a002dc6c0825839013fd872a48d71872cf76528bb41dd2a5775e2c532c21662a47c6cebf44b6be5f33456f28523ed04d1aaad158541176425378d8ed8fce53f881a3b6a7221021a0002911fa0f5f6'; + +const OWN_ADDRESS = 'addr1qylasu4y34ccwt8hv55tkswa9fthtck9xtppvc4y03kwhaztd0jlxdzk72zj8mgy6x4269v9gytkgffh3k8d3l8987yqu5c35z'; +const FOREIGN_ADDRESS = 'addr1wx434t2jc3m5uhdf7tq05xjdqu3q5z7a2lhrmn5mapsd43srh7ll8'; + +describe('decodeTransaction', () => { + test('extracts the fee', () => { + assert.equal(decodeTransaction(FIXTURE_CBOR).fee, 168223n); + }); + + test('extracts both outputs with bech32 addresses and lovelace', () => { + const { outputs } = decodeTransaction(FIXTURE_CBOR); + assert.equal(outputs.length, 2); + assert.equal(outputs[0].address, FOREIGN_ADDRESS); + assert.equal(outputs[0].lovelace, 3_000_000n); + assert.equal(outputs[1].address, OWN_ADDRESS); + assert.equal(outputs[1].lovelace, 996_831_777n); + }); + + test('reports the input count', () => { + assert.equal(decodeTransaction(FIXTURE_CBOR).inputCount, 1); + }); + + test('reports no minted assets for a plain payment', () => { + assert.equal(decodeTransaction(FIXTURE_CBOR).mintedAssetCount, 0); + }); + + test('reports no native assets on either output', () => { + const { outputs } = decodeTransaction(FIXTURE_CBOR); + assert.deepStrictEqual(outputs[0].assets, []); + assert.deepStrictEqual(outputs[1].assets, []); + }); + + test('computes a 64-hex transaction hash', () => { + const { txHashHex } = decodeTransaction(FIXTURE_CBOR); + assert.match(txHashHex, /^[0-9a-f]{64}$/); + }); + + // Fail closed: anything unparseable must throw, never yield a partial result + // that policy might then evaluate against incomplete data. + test('rejects non-hex input', () => { + assert.throws(() => decodeTransaction('not-hex'), /could not be decoded/i); + }); + + test('rejects truncated CBOR', () => { + assert.throws(() => decodeTransaction(FIXTURE_CBOR.slice(0, 40)), /could not be decoded/i); + }); + + test('rejects an empty string', () => { + assert.throws(() => decodeTransaction(''), /could not be decoded/i); + }); +}); From 85d718dc14f660a804b9a66ad439a5182d06773f Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:41:31 +0200 Subject: [PATCH 05/27] fix(signer): allow .ts extension imports so tests are typechecked The signer's tsconfig includes test files, and they import source with an explicit .ts extension (which tsx resolves at runtime). Without this flag tsc exits 2, which fails the blocking typecheck gate in CI. Safe because noEmit is set; the alternative - excluding tests - would leave them unchecked. --- packages/signer/tsconfig.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/signer/tsconfig.json b/packages/signer/tsconfig.json index 66ea60f..9277517 100644 --- a/packages/signer/tsconfig.json +++ b/packages/signer/tsconfig.json @@ -8,7 +8,11 @@ "strict": true, "skipLibCheck": true, "noEmit": true, - "resolveJsonModule": true + "resolveJsonModule": true, + // Tests import source with an explicit .ts extension (node --import tsx + // resolves it at runtime). Safe here because noEmit is set, and it lets the + // tests be typechecked rather than excluded from the gate. + "allowImportingTsExtensions": true }, "include": ["src/**/*", "test/**/*"] } From 5da4447835f0b8e832a3038122b83281a12bd52f Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:02:17 +0200 Subject: [PATCH 06/27] fix(signer): reject transactions with unconsumed trailing bytes from_cbor_hex only requires a valid prefix, so extra bytes were silently dropped and decode returned a confident, fully-populated result - the exact fail-open the module exists to prevent, and policy trusts this output. --- packages/signer/src/decode.ts | 22 ++++++++++++++++ packages/signer/test/decode.test.ts | 40 +++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/packages/signer/src/decode.ts b/packages/signer/src/decode.ts index 43822db..afdafff 100644 --- a/packages/signer/src/decode.ts +++ b/packages/signer/src/decode.ts @@ -58,6 +58,28 @@ export function decodeTransaction(cborHex: string): DecodedTx { ); } + // from_cbor_hex only requires that a *prefix* of cborHex parse as a valid + // transaction; it does not check that parsing consumed the whole string, so + // trailing bytes would otherwise be silently discarded and produce a + // confident, fully-populated result over less data than was actually + // supplied. CML preserves the original encoding on parse-then-reserialize + // (only objects built from scratch via `.new()` are canonicalised), so + // re-serialising and comparing against the input detects any unconsumed + // suffix. Compared case-insensitively: valid uppercase-hex input round-trips + // through CML as lowercase, which would otherwise read as a mismatch. + // Verified empirically (not just by this module's own tests, which lack an + // asset-bearing or multi-shape fixture) across: the clean fixture, the + // fixture with trailing bytes appended, a fixture-derived transaction with a + // real vkey witness attached (the shape sign.ts will produce), and a + // fixture-derived transaction with an added output (a structural change) — + // strict equality held for every legitimate case and failed only, in the + // "input longer than reserialised" direction, for the trailing-bytes case. + if (tx.to_cbor_hex().toLowerCase() !== cborHex.toLowerCase()) { + throw new Error( + 'Transaction could not be decoded: input was not fully consumed (unrecognised trailing data)' + ); + } + const body = tx.body(); const outputs: DecodedOutput[] = []; const cmlOutputs = body.outputs(); diff --git a/packages/signer/test/decode.test.ts b/packages/signer/test/decode.test.ts index eac60ec..f36a54d 100644 --- a/packages/signer/test/decode.test.ts +++ b/packages/signer/test/decode.test.ts @@ -1,5 +1,6 @@ import { describe, test } from 'node:test'; import assert from 'node:assert/strict'; +import * as CML from '@anastasia-labs/cardano-multiplatform-lib-nodejs'; import { decodeTransaction } from '../src/decode.ts'; // Real unsigned transaction from Vector testnet. 1 input, 2 outputs @@ -56,4 +57,43 @@ describe('decodeTransaction', () => { test('rejects an empty string', () => { assert.throws(() => decodeTransaction(''), /could not be decoded/i); }); + + // CML's from_cbor_hex only requires that a *prefix* of the input parse as a + // valid transaction — it does not check that parsing consumed the whole + // string. Left unchecked, appending arbitrary bytes to a real transaction + // would silently decode as if those bytes did not exist: a confident, + // fully-populated result over less data than was actually supplied. That is + // the opposite failure mode from the tests above (which all fail to parse + // at all) and is exactly the kind of partial understanding this module must + // never hand to policy. + describe('unconsumed input (trailing bytes)', () => { + test('rejects a transaction with trailing bytes appended', () => { + assert.throws(() => decodeTransaction(FIXTURE_CBOR + 'deadbeef'), /could not be decoded/i); + }); + + test('still accepts the clean fixture (no false positive on legitimate input)', () => { + assert.doesNotThrow(() => decodeTransaction(FIXTURE_CBOR)); + }); + + test('still accepts a signed transaction (fixture + a real vkey witness)', () => { + const unsigned = CML.Transaction.from_cbor_hex(FIXTURE_CBOR); + const body = unsigned.body(); + const hash = CML.hash_transaction(body); + const privateKey = CML.PrivateKey.generate_ed25519(); + const signature = privateKey.sign(hash.to_raw_bytes()); + + const vkeywitnesses = CML.VkeywitnessList.new(); + vkeywitnesses.add(CML.Vkeywitness.new(privateKey.to_public(), signature)); + const witnessSet = CML.TransactionWitnessSet.new(); + witnessSet.set_vkeywitnesses(vkeywitnesses); + + // The same shape sign.ts will produce: original body, a witness set, no + // auxiliary data. + const signed = CML.Transaction.new(body, witnessSet, true, undefined); + + const decoded = decodeTransaction(signed.to_cbor_hex()); + assert.equal(decoded.fee, 168223n); + assert.equal(decoded.outputs.length, 2); + }); + }); }); From 2a8623d22a762fbd7a71b84078b9b307cac0c144 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:14:39 +0200 Subject: [PATCH 07/27] docs(signer): keep the durable rationale, drop the dated verification list The mechanism explanation is worth keeping; the snapshot of which fixtures were tested on which day is commit-message content that goes stale silently. --- packages/signer/src/decode.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/signer/src/decode.ts b/packages/signer/src/decode.ts index afdafff..88c2a32 100644 --- a/packages/signer/src/decode.ts +++ b/packages/signer/src/decode.ts @@ -67,13 +67,12 @@ export function decodeTransaction(cborHex: string): DecodedTx { // re-serialising and comparing against the input detects any unconsumed // suffix. Compared case-insensitively: valid uppercase-hex input round-trips // through CML as lowercase, which would otherwise read as a mismatch. - // Verified empirically (not just by this module's own tests, which lack an - // asset-bearing or multi-shape fixture) across: the clean fixture, the - // fixture with trailing bytes appended, a fixture-derived transaction with a - // real vkey witness attached (the shape sign.ts will produce), and a - // fixture-derived transaction with an added output (a structural change) — - // strict equality held for every legitimate case and failed only, in the - // "input longer than reserialised" direction, for the trailing-bytes case. + // The risk of comparing strictly is a false positive: if CML re-encoded any + // legitimate transaction differently, the signer would refuse to sign it at + // all. That was checked against the encodings this ecosystem actually uses — + // including the indefinite-length CBOR the agent registry hand-rolls (see + // shared/tx.ts), metadata, Plutus scripts and datums, and non-canonical + // integers — and the encoding survived every round trip. if (tx.to_cbor_hex().toLowerCase() !== cborHex.toLowerCase()) { throw new Error( 'Transaction could not be decoded: input was not fully consumed (unrecognised trailing data)' From 65a3c00994cd8b3eba3a7dfe24d4042f44e6583f Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:23:12 +0200 Subject: [PATCH 08/27] feat(signer): spend policy over true net outflow Net outflow is every output that does not return to us, plus the fee - more accurate than summing declared amounts, and computable without chain access. Refuses when no own address is known, since change cannot then be identified. --- packages/signer/src/policy.ts | 92 ++++++++++++++++++++ packages/signer/test/policy.test.ts | 128 ++++++++++++++++++++++++++++ 2 files changed, 220 insertions(+) create mode 100644 packages/signer/src/policy.ts create mode 100644 packages/signer/test/policy.test.ts diff --git a/packages/signer/src/policy.ts b/packages/signer/src/policy.ts new file mode 100644 index 0000000..837e8f1 --- /dev/null +++ b/packages/signer/src/policy.ts @@ -0,0 +1,92 @@ +// Spend policy. Pure function of its arguments — the caller supplies today's +// spend from the audit log, so this module needs no I/O and is fully testable. +import type { DecodedTx } from './decode.js'; + +export interface AssetMovement { + unit: string; + quantity: bigint; + toAddress: string; +} + +export interface PolicyDecision { + allowed: boolean; + reason?: string; + netOutflowLovelace: bigint; + assetMovements: AssetMovement[]; +} + +export interface SpendLimits { + perTxLovelace: bigint; + dailyLovelace: bigint; +} + +function ada(lovelace: bigint): string { + return (Number(lovelace) / 1_000_000).toFixed(6); +} + +/** + * Net outflow = every output that does NOT return to us, plus the fee. + * + * Computable from the body alone, so no chain access is needed. An output to a + * wallet address at a different derivation index counts as outflow — that + * over-counts, which refuses too much rather than too little. + */ +export function computeOutflow( + tx: DecodedTx, + ownAddresses: string[] +): { netOutflowLovelace: bigint; assetMovements: AssetMovement[] } { + const own = new Set(ownAddresses); + let leaving = 0n; + const assetMovements: AssetMovement[] = []; + + for (const output of tx.outputs) { + if (own.has(output.address)) continue; + leaving += output.lovelace; + for (const asset of output.assets) { + assetMovements.push({ unit: asset.unit, quantity: asset.quantity, toAddress: output.address }); + } + } + + return { netOutflowLovelace: leaving + tx.fee, assetMovements }; +} + +export function evaluate( + tx: DecodedTx, + ownAddresses: string[], + limits: SpendLimits, + alreadySpentTodayLovelace: bigint +): PolicyDecision { + const { netOutflowLovelace, assetMovements } = computeOutflow(tx, ownAddresses); + + // Fail closed: with no own address, every output looks foreign and change + // cannot be identified, so any decision would be meaningless. + if (ownAddresses.length === 0) { + return { + allowed: false, + reason: 'No own address available, so change cannot be distinguished from outflow. Refusing.', + netOutflowLovelace, + assetMovements, + }; + } + + if (netOutflowLovelace > limits.perTxLovelace) { + return { + allowed: false, + reason: `Net outflow ${ada(netOutflowLovelace)} AP3X exceeds the per-transaction limit of ${ada(limits.perTxLovelace)} AP3X.`, + netOutflowLovelace, + assetMovements, + }; + } + + if (alreadySpentTodayLovelace + netOutflowLovelace > limits.dailyLovelace) { + const remaining = limits.dailyLovelace - alreadySpentTodayLovelace; + return { + allowed: false, + reason: `Net outflow ${ada(netOutflowLovelace)} AP3X exceeds the remaining daily budget of ${ada(remaining > 0n ? remaining : 0n)} AP3X (limit ${ada(limits.dailyLovelace)} AP3X, already committed ${ada(alreadySpentTodayLovelace)} AP3X).`, + netOutflowLovelace, + assetMovements, + }; + } + + return { allowed: true, netOutflowLovelace, assetMovements }; +} diff --git a/packages/signer/test/policy.test.ts b/packages/signer/test/policy.test.ts new file mode 100644 index 0000000..16caa0d --- /dev/null +++ b/packages/signer/test/policy.test.ts @@ -0,0 +1,128 @@ +import { describe, test } from 'node:test'; +import assert from 'node:assert/strict'; +import { computeOutflow, evaluate, type SpendLimits } from '../src/policy.ts'; +import type { DecodedTx } from '../src/decode.ts'; + +const OWN = 'addr1own'; +const OTHER_OWN = 'addr1own_second_index'; +const FOREIGN = 'addr1foreign'; + +const LIMITS: SpendLimits = { perTxLovelace: 100_000_000n, dailyLovelace: 500_000_000n }; + +function tx(partial: Partial): DecodedTx { + return { + txHashHex: 'a'.repeat(64), + fee: 200_000n, + outputs: [], + inputCount: 1, + mintedAssetCount: 0, + ...partial, + }; +} + +describe('computeOutflow', () => { + test('counts a foreign output plus the fee', () => { + const r = computeOutflow(tx({ outputs: [{ address: FOREIGN, lovelace: 3_000_000n, assets: [] }] }), [OWN]); + assert.equal(r.netOutflowLovelace, 3_200_000n); + }); + + test('excludes change returning to our own address', () => { + const r = computeOutflow( + tx({ outputs: [ + { address: FOREIGN, lovelace: 3_000_000n, assets: [] }, + { address: OWN, lovelace: 996_000_000n, assets: [] }, + ] }), + [OWN] + ); + assert.equal(r.netOutflowLovelace, 3_200_000n, 'change must not count as outflow'); + }); + + test('counts the fee even when every output returns to us', () => { + const r = computeOutflow(tx({ outputs: [{ address: OWN, lovelace: 5_000_000n, assets: [] }] }), [OWN]); + assert.equal(r.netOutflowLovelace, 200_000n); + }); + + test('honours multiple own addresses', () => { + const r = computeOutflow( + tx({ outputs: [{ address: OTHER_OWN, lovelace: 7_000_000n, assets: [] }] }), + [OWN, OTHER_OWN] + ); + assert.equal(r.netOutflowLovelace, 200_000n); + }); + + test('records assets leaving to a foreign address', () => { + const r = computeOutflow( + tx({ outputs: [{ address: FOREIGN, lovelace: 2_000_000n, assets: [{ unit: 'policy1nft', quantity: 1n }] }] }), + [OWN] + ); + assert.deepStrictEqual(r.assetMovements, [{ unit: 'policy1nft', quantity: 1n, toAddress: FOREIGN }]); + }); + + test('does not record assets returning to us', () => { + const r = computeOutflow( + tx({ outputs: [{ address: OWN, lovelace: 2_000_000n, assets: [{ unit: 'policy1nft', quantity: 1n }] }] }), + [OWN] + ); + assert.deepStrictEqual(r.assetMovements, []); + }); +}); + +describe('evaluate — adversarial', () => { + test('refuses a drain that exceeds the per-transaction limit', () => { + const d = evaluate( + tx({ outputs: [{ address: FOREIGN, lovelace: 250_000_000n, assets: [] }] }), + [OWN], LIMITS, 0n + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /per-transaction limit/i); + }); + + test('allows a transaction inside both limits', () => { + const d = evaluate( + tx({ outputs: [{ address: FOREIGN, lovelace: 3_000_000n, assets: [] }] }), + [OWN], LIMITS, 0n + ); + assert.equal(d.allowed, true); + assert.equal(d.reason, undefined); + }); + + test('refuses when the daily budget is already exhausted', () => { + const d = evaluate( + tx({ outputs: [{ address: FOREIGN, lovelace: 10_000_000n, assets: [] }] }), + [OWN], LIMITS, 495_000_000n + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /daily/i); + }); + + test('a large transaction is allowed if it fits exactly', () => { + const d = evaluate( + tx({ fee: 0n, outputs: [{ address: FOREIGN, lovelace: 100_000_000n, assets: [] }] }), + [OWN], LIMITS, 0n + ); + assert.equal(d.allowed, true); + }); + + test('a transaction with no outputs still charges the fee', () => { + const d = evaluate(tx({ outputs: [] }), [OWN], LIMITS, 0n); + assert.equal(d.netOutflowLovelace, 200_000n); + assert.equal(d.allowed, true); + }); + + test('refuses when ownAddresses is empty — nothing can be classified as change', () => { + const d = evaluate( + tx({ outputs: [{ address: OWN, lovelace: 5_000_000n, assets: [] }] }), + [], LIMITS, 0n + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /own address/i); + }); + + test('the reported outflow is always present, even on refusal', () => { + const d = evaluate( + tx({ outputs: [{ address: FOREIGN, lovelace: 250_000_000n, assets: [] }] }), + [OWN], LIMITS, 0n + ); + assert.equal(d.netOutflowLovelace, 250_200_000n); + }); +}); From 9bbd26b04cc1f9cc983126f2938cadcd31d6e894 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:30:14 +0200 Subject: [PATCH 09/27] fix(signer): treat blank-only ownAddresses as no known own address Adversarial pass on the spend policy found that ownAddresses.length === 0 does not catch a non-empty array containing only blank entries - a realistic outcome of ''.split(',') on an empty config/env value, which yields [''] not []. A blank entry never matches a real output address, so it degrades to "every output looks foreign" rather than permitting anything, but it bypassed the explicit fail-closed path and its message, and if the resulting over-counted outflow still happened to land under the configured limits the transaction would be signed without any real own address ever having been known - exactly the condition that check exists to refuse. knownAddresses() filters blank entries before both the Set used for classification and the fail-closed emptiness check, so an all-blank list now refuses the same way a literally empty one does. A blank entry alongside a real address is unaffected (regression-tested). Also adds the rest of the adversarial pass: exact-limit boundaries one lovelace either side, many-small-outputs summation, aggregation correctness (duplicate own addresses, repeated foreign addresses, multi-asset and multi-output asset movements), check-ordering pins, zero-limit and already-over-budget edge cases, and a pinned note that SpendLimits governs lovelace only - native asset movements are tracked but not limited. Every core comparison was verified to actually fail on injection (inverted own/foreign check, dropped fee, dropped daily history, off-by-one on both limit comparisons) before being reverted; see the task report. --- packages/signer/src/policy.ts | 18 +- packages/signer/test/policy.test.ts | 250 ++++++++++++++++++++++++++++ 2 files changed, 265 insertions(+), 3 deletions(-) diff --git a/packages/signer/src/policy.ts b/packages/signer/src/policy.ts index 837e8f1..948f2ef 100644 --- a/packages/signer/src/policy.ts +++ b/packages/signer/src/policy.ts @@ -24,6 +24,16 @@ function ada(lovelace: bigint): string { return (Number(lovelace) / 1_000_000).toFixed(6); } +// A blank entry is not a usable address. `''.split(',')` on an empty string +// yields `['']`, not `[]` — a realistic way an upstream config/env-parsing bug +// could hand this module a non-empty array that names no real address. Left +// unfiltered, `ownAddresses.length === 0` would miss that case (the array +// isn't empty), the blank entry would never match a real output, and the +// module would proceed as if it knew an own address when it does not. +function knownAddresses(ownAddresses: string[]): string[] { + return ownAddresses.filter((address) => address.trim().length > 0); +} + /** * Net outflow = every output that does NOT return to us, plus the fee. * @@ -35,7 +45,7 @@ export function computeOutflow( tx: DecodedTx, ownAddresses: string[] ): { netOutflowLovelace: bigint; assetMovements: AssetMovement[] } { - const own = new Set(ownAddresses); + const own = new Set(knownAddresses(ownAddresses)); let leaving = 0n; const assetMovements: AssetMovement[] = []; @@ -59,8 +69,10 @@ export function evaluate( const { netOutflowLovelace, assetMovements } = computeOutflow(tx, ownAddresses); // Fail closed: with no own address, every output looks foreign and change - // cannot be identified, so any decision would be meaningless. - if (ownAddresses.length === 0) { + // cannot be identified, so any decision would be meaningless. A list + // containing only blank entries counts as no own address too — see + // knownAddresses(). + if (knownAddresses(ownAddresses).length === 0) { return { allowed: false, reason: 'No own address available, so change cannot be distinguished from outflow. Refusing.', diff --git a/packages/signer/test/policy.test.ts b/packages/signer/test/policy.test.ts index 16caa0d..f246945 100644 --- a/packages/signer/test/policy.test.ts +++ b/packages/signer/test/policy.test.ts @@ -6,6 +6,7 @@ import type { DecodedTx } from '../src/decode.ts'; const OWN = 'addr1own'; const OTHER_OWN = 'addr1own_second_index'; const FOREIGN = 'addr1foreign'; +const FOREIGN2 = 'addr1foreign_second'; const LIMITS: SpendLimits = { perTxLovelace: 100_000_000n, dailyLovelace: 500_000_000n }; @@ -126,3 +127,252 @@ describe('evaluate — adversarial', () => { assert.equal(d.netOutflowLovelace, 250_200_000n); }); }); + +// --- Adversarial pass (beyond the brief) ----------------------------------- +// +// The brief's 13 tests are a floor. Everything below was added after they +// passed, specifically hunting for the failure shape this module is uniquely +// exposed to: a wrong comparison or a missed exclusion that does not throw, +// it just quietly returns `allowed: true` when it should not. + +describe('evaluate — ownAddresses containing only blank entries', () => { + // A realistic real-world source of this: `''.split(',')` in JS yields `['']`, + // not `[]`. If whatever populates ownAddresses from config/env ever hits that + // empty-string case, the array is non-empty but contains no usable address. + // `ownAddresses.length === 0` alone does not catch this: the array has + // length 1. Every real output then fails to match the blank entry, so it is + // over-counted as outflow rather than silently permitted — the safe + // direction — but the explicit, intentional fail-closed path (and its + // message) is bypassed, and if the (over-counted) outflow happens to still + // land under the configured limits, the transaction is signed even though + // no real own address was ever known. That is the same condition the + // empty-array check exists to refuse; it should refuse here too. + test('a single blank entry is treated as no known own address (fails closed)', () => { + const d = evaluate( + tx({ outputs: [{ address: OWN, lovelace: 5_000_000n, assets: [] }] }), + [''], LIMITS, 0n + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /own address/i); + }); + + test('a blank entry alongside a real own address does not weaken matching', () => { + // Regression guard for the fix above: filtering blanks must not touch a + // list that also contains a genuine address. + const r = computeOutflow( + tx({ outputs: [{ address: OWN, lovelace: 5_000_000n, assets: [] }] }), + ['', OWN] + ); + assert.equal(r.netOutflowLovelace, 200_000n, 'OWN must still be recognised as change'); + }); +}); + +describe('evaluate — exact limit boundaries, one lovelace either side', () => { + test('one lovelace under the per-transaction limit is allowed', () => { + const d = evaluate( + tx({ fee: 0n, outputs: [{ address: FOREIGN, lovelace: 99_999_999n, assets: [] }] }), + [OWN], LIMITS, 0n + ); + assert.equal(d.allowed, true); + }); + + test('one lovelace over the per-transaction limit is refused', () => { + const d = evaluate( + tx({ fee: 0n, outputs: [{ address: FOREIGN, lovelace: 100_000_001n, assets: [] }] }), + [OWN], LIMITS, 0n + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /per-transaction limit/i); + }); + + test('a daily total landing exactly on the limit is allowed', () => { + const d = evaluate( + tx({ fee: 0n, outputs: [{ address: FOREIGN, lovelace: 50_000_000n, assets: [] }] }), + [OWN], LIMITS, 450_000_000n // 450M + 50M == 500M dailyLovelace, exactly + ); + assert.equal(d.allowed, true); + }); + + test('one lovelace over the daily limit is refused', () => { + const d = evaluate( + tx({ fee: 0n, outputs: [{ address: FOREIGN, lovelace: 50_000_000n, assets: [] }] }), + [OWN], LIMITS, 450_000_001n // sum == dailyLovelace + 1 + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /daily/i); + }); + + test('one lovelace under the daily limit is allowed', () => { + const d = evaluate( + tx({ fee: 0n, outputs: [{ address: FOREIGN, lovelace: 50_000_000n, assets: [] }] }), + [OWN], LIMITS, 449_999_999n // sum == dailyLovelace - 1 + ); + assert.equal(d.allowed, true); + }); +}); + +describe('evaluate — many small foreign outputs must sum, not spot-check', () => { + test('34 outputs of 3 AP3X each breach a 100 AP3X per-transaction limit on their sum', () => { + // No single output is anywhere near the limit (3_000_000n each); only the + // total is. A check that inspected outputs individually rather than + // summing would wrongly allow this. + const outputs = Array.from({ length: 34 }, () => ({ address: FOREIGN, lovelace: 3_000_000n, assets: [] })); + const d = evaluate(tx({ outputs }), [OWN], LIMITS, 0n); + assert.equal(d.netOutflowLovelace, 102_200_000n); // 34 * 3,000,000 + 200,000 fee + assert.equal(d.allowed, false); + assert.match(d.reason!, /per-transaction limit/i); + }); +}); + +describe('computeOutflow — aggregation correctness', () => { + test('sums repeated outputs to the same foreign address rather than deduplicating', () => { + const r = computeOutflow( + tx({ outputs: [ + { address: FOREIGN, lovelace: 1_000_000n, assets: [] }, + { address: FOREIGN, lovelace: 2_000_000n, assets: [] }, // same address again + { address: OWN, lovelace: 50_000_000n, assets: [] }, // change, excluded + { address: FOREIGN2, lovelace: 3_000_000n, assets: [] }, // a different foreign address + ] }), + [OWN] + ); + assert.equal(r.netOutflowLovelace, 6_200_000n); // 1M + 2M + 3M + 200K fee; 50M change excluded + }); + + test('duplicate entries in ownAddresses do not change classification', () => { + const r = computeOutflow( + tx({ outputs: [ + { address: OWN, lovelace: 5_000_000n, assets: [] }, + { address: OTHER_OWN, lovelace: 3_000_000n, assets: [] }, + { address: FOREIGN, lovelace: 2_000_000n, assets: [] }, + ] }), + [OWN, OWN, OTHER_OWN, OTHER_OWN] + ); + assert.equal(r.netOutflowLovelace, 2_200_000n); // only the foreign output + fee + }); + + test('an own address supplied in a different case does not match — over-counts as outflow, the safe direction', () => { + // Bech32 addresses are canonically lowercase and decode.ts only ever emits + // that canonical form, so this should not arise in practice. Pinning it + // anyway: a mismatch here fails toward refusing too much, never toward + // missing real outflow, which is the direction that matters. + const r = computeOutflow( + tx({ outputs: [{ address: OWN, lovelace: 5_000_000n, assets: [] }] }), + ['ADDR1OWN'] + ); + assert.equal(r.netOutflowLovelace, 5_200_000n, 'differently-cased own address must not be recognised as change'); + }); + + test('records every distinct asset on a single foreign output, not just the first', () => { + const r = computeOutflow( + tx({ outputs: [{ + address: FOREIGN, + lovelace: 2_000_000n, + assets: [ + { unit: 'policy1nft', quantity: 1n }, + { unit: 'policy2token', quantity: 500n }, + ], + }] }), + [OWN] + ); + assert.deepStrictEqual(r.assetMovements, [ + { unit: 'policy1nft', quantity: 1n, toAddress: FOREIGN }, + { unit: 'policy2token', quantity: 500n, toAddress: FOREIGN }, + ]); + }); + + test('aggregates assets across multiple foreign outputs rather than keeping only the last', () => { + const r = computeOutflow( + tx({ outputs: [ + { address: FOREIGN, lovelace: 2_000_000n, assets: [{ unit: 'nftA', quantity: 1n }] }, + { address: FOREIGN2, lovelace: 3_000_000n, assets: [{ unit: 'nftB', quantity: 1n }] }, + ] }), + [OWN] + ); + assert.deepStrictEqual(r.assetMovements, [ + { unit: 'nftA', quantity: 1n, toAddress: FOREIGN }, + { unit: 'nftB', quantity: 1n, toAddress: FOREIGN2 }, + ]); + }); +}); + +describe('evaluate — check ordering is pinned, not incidental', () => { + test('a transaction breaching both limits at once reports the per-transaction reason first', () => { + const d = evaluate( + tx({ fee: 0n, outputs: [{ address: FOREIGN, lovelace: 250_000_000n, assets: [] }] }), + [OWN], LIMITS, 400_000_000n // also breaches the daily limit (250M + 400M > 500M) + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /per-transaction limit/i); + assert.doesNotMatch(d.reason!, /daily/i); + }); + + test('an empty ownAddresses list reports the own-address reason before any limit is even considered', () => { + const d = evaluate( + tx({ fee: 0n, outputs: [{ address: FOREIGN, lovelace: 250_000_000n, assets: [] }] }), + [], LIMITS, 400_000_000n // this transaction would also fail both limit checks + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /own address/i); + }); +}); + +describe('evaluate — zero limits and an already-over-budget day', () => { + test('zero limits still allow a genuinely zero-value transaction', () => { + const zeroLimits: SpendLimits = { perTxLovelace: 0n, dailyLovelace: 0n }; + const d = evaluate(tx({ fee: 0n, outputs: [] }), [OWN], zeroLimits, 0n); + assert.equal(d.netOutflowLovelace, 0n); + assert.equal(d.allowed, true); + }); + + test('zero limits refuse even the smallest positive outflow', () => { + const zeroLimits: SpendLimits = { perTxLovelace: 0n, dailyLovelace: 0n }; + const d = evaluate(tx({ fee: 1n, outputs: [] }), [OWN], zeroLimits, 0n); + assert.equal(d.allowed, false); + assert.match(d.reason!, /per-transaction limit/i); + }); + + test('an already over-budget day clamps the reported remaining budget to zero, not negative', () => { + // alreadySpentTodayLovelace (600M) already exceeds dailyLovelace (500M) — + // should not happen if the audit log is accounted correctly, but the + // module must still behave sanely (and keep refusing) if it ever does. + const d = evaluate(tx({ outputs: [] }), [OWN], LIMITS, 600_000_000n); + assert.equal(d.allowed, false); + assert.match(d.reason!, /daily/i); + assert.match(d.reason!, /remaining daily budget of 0\.000000 AP3X/); + }); +}); + +describe('policy — known gap: SpendLimits governs lovelace only, not native asset value', () => { + test('a transaction moving a native asset to a foreign address is allowed if lovelace stays under both limits', () => { + // computeOutflow tracks assetMovements, but evaluate() only compares + // netOutflowLovelace against limits — it never inspects assetMovements. + // A transaction that sends an NFT or a large token balance out while + // keeping the lovelace side minimal (e.g. just the ~2 AP3X min-UTxO + // deposit) passes both limit checks untouched. SpendLimits has no + // asset-value dimension to check against, so this is not a coding + // mistake relative to the given interface — it is a scope boundary of + // this policy, pinned here so it is visible rather than silently assumed. + const d = evaluate( + tx({ outputs: [{ address: FOREIGN, lovelace: 2_000_000n, assets: [{ unit: 'preciousNFT', quantity: 1n }] }] }), + [OWN], LIMITS, 0n + ); + assert.equal(d.allowed, true); + assert.deepStrictEqual(d.assetMovements, [{ unit: 'preciousNFT', quantity: 1n, toAddress: FOREIGN }]); + }); +}); + +describe('evaluate — bigint precision at magnitudes beyond float safety', () => { + test('the bigint outflow figure stays exact past Number.MAX_SAFE_INTEGER, even though the human-readable message may round', () => { + // ada() formats via Number() for the reason string only; the allow/deny + // decision itself is decided entirely in bigint arithmetic and must never + // lose precision, no matter how large the values get. Not reachable at + // this system's real scale (limits and balances run to hundreds of AP3X, + // far below 2^53 lovelace) — tested anyway because the decision-critical + // field must hold regardless of realistic bounds. + const HUGE_FEE = 9_007_199_254_740_993n; // Number.MAX_SAFE_INTEGER + 2 + const zeroLimits: SpendLimits = { perTxLovelace: 0n, dailyLovelace: 0n }; + const d = evaluate(tx({ fee: HUGE_FEE, outputs: [] }), [OWN], zeroLimits, 0n); + assert.equal(d.allowed, false); + assert.equal(d.netOutflowLovelace, HUGE_FEE, 'the bigint decision figure must never lose precision'); + }); +}); From 97987427ce040b6b2f4ca1c247c3e17a0813d230 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:57:45 +0200 Subject: [PATCH 10/27] fix(signer): close two review-found gaps in ownAddresses validation Fix 1 (Important, live bypass): the blank-address fix from the prior commit used .trim().length > 0, which only strips the ECMAScript WhiteSpace set (NBSP U+00A0, BOM U+FEFF, etc). U+200B ZERO WIDTH SPACE and U+2060 WORD JOINER are Unicode category Cf (format), not Zs, so trim() leaves them untouched - ownAddresses: ['\u200B'] passed the filter, bypassed the fail-closed guard, and reproduced allowed:true against the shipped code (verified directly, not just against a mutant). Replaced the blank-only filter with a positive check: an entry only counts as a known own address if it starts with 'addr' after trimming. A denylist of invisible codepoints is unwinnable; this rules out anything built from them without enumerating any. Applies at both call sites (evaluate's guard and computeOutflow's Set) via the shared knownAddresses() helper, as it already did. Fix 2 (Important, surviving mutant): added a regression test that exercises computeOutflow's own Set-construction filtering directly, independent of evaluate's separate guard - a blank/invisible output address paired with a matching blank/invisible ownAddresses entry must still count as outflow, not be matched as change. Nothing previously exercised this combination: every prior blank-entry test either went through evaluate (whose guard would refuse first) or paired a blank ownAddresses entry with a real output address. Confirmed this test now fails if computeOutflow's Set reverts to unfiltered while evaluate's guard stays filtered (re-verified in this session; see the report addendum for output). Fix 3: added a doc comment on evaluate() stating plainly that it bounds lovelace only - assetMovements is reported for visibility and never gates allowed. No interface change; this is documentation of the existing, deliberate v1 scope boundary already flagged in the task report, not new behavior. Re-ran the full mutation set after all three fixes: inverted classification, removed fail-closed guard entirely, guard reverted to raw ownAddresses.length === 0, and computeOutflow's Set left unfiltered are all caught. Full before/after evidence in the report addendum. --- packages/signer/src/policy.ts | 43 +++++++++++--- packages/signer/test/policy.test.ts | 88 +++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 7 deletions(-) diff --git a/packages/signer/src/policy.ts b/packages/signer/src/policy.ts index 948f2ef..8dff899 100644 --- a/packages/signer/src/policy.ts +++ b/packages/signer/src/policy.ts @@ -24,14 +24,27 @@ function ada(lovelace: bigint): string { return (Number(lovelace) / 1_000_000).toFixed(6); } -// A blank entry is not a usable address. `''.split(',')` on an empty string -// yields `['']`, not `[]` — a realistic way an upstream config/env-parsing bug -// could hand this module a non-empty array that names no real address. Left -// unfiltered, `ownAddresses.length === 0` would miss that case (the array -// isn't empty), the blank entry would never match a real output, and the -// module would proceed as if it knew an own address when it does not. +// A blank entry is not a usable address — and neither is one built entirely +// from characters that merely look blank. `''.split(',')` on an empty string +// yields `['']`, not `[]`, a realistic way an upstream config/env-parsing bug +// could hand this module a non-empty array that names no real address; a +// literal '' is the obvious case, but `.trim()` alone does not generalise +// past it, since it only strips the ECMAScript WhiteSpace set (which +// includes NBSP U+00A0 and BOM U+FEFF). Plenty of other invisible characters +// survive it: U+200B ZERO WIDTH SPACE and U+2060 WORD JOINER are Unicode +// category Cf (format), not Zs (space separator), so `.trim()` leaves them +// untouched. A denylist of "invisible" codepoints is not a fix — there are +// dozens of Cf/Cc codepoints and more get added — so this checks positively +// instead: an entry only counts as a known own address if, after trimming, +// it starts with the 'addr' prefix every Cardano/Vector payment address +// actually uses. Nothing built purely from blank or invisible characters can +// satisfy that, without needing to enumerate any of them. +function looksLikeAddress(address: string): boolean { + return address.trim().startsWith('addr'); +} + function knownAddresses(ownAddresses: string[]): string[] { - return ownAddresses.filter((address) => address.trim().length > 0); + return ownAddresses.filter(looksLikeAddress); } /** @@ -60,6 +73,22 @@ export function computeOutflow( return { netOutflowLovelace: leaving + tx.fee, assetMovements }; } +/** + * Decides whether to sign `tx`, bounding lovelace outflow only. + * + * `SpendLimits` has no asset-value dimension: `assetMovements` (native + * assets leaving to a foreign address, e.g. NFTs or token balances) is + * computed and reported on the returned `PolicyDecision` for visibility, but + * it never gates `allowed`. A transaction can move an entire native-asset + * balance out of the wallet and still receive `allowed: true` as long as the + * lovelace side of that same transaction stays under both limits — e.g. an + * NFT sent with only the minimum UTxO deposit attached. This is a deliberate + * v1 scope boundary, not an oversight: adding asset-value limits is future + * work, and would need `SpendLimits` itself to grow a corresponding field. + * A caller must not read `allowed: true` from this function as approval of + * whatever `assetMovements` contains — only as approval of the lovelace + * amount in `netOutflowLovelace`. + */ export function evaluate( tx: DecodedTx, ownAddresses: string[], diff --git a/packages/signer/test/policy.test.ts b/packages/signer/test/policy.test.ts index f246945..bfb4d69 100644 --- a/packages/signer/test/policy.test.ts +++ b/packages/signer/test/policy.test.ts @@ -167,6 +167,94 @@ describe('evaluate — ownAddresses containing only blank entries', () => { }); }); +describe('evaluate — ownAddresses entries built from invisible Unicode characters', () => { + // `.trim()` alone is not enough to detect "not a real address": it strips + // the ECMAScript WhiteSpace set (which includes NBSP U+00A0 and BOM + // U+FEFF) but not every invisible character. U+200B ZERO WIDTH SPACE and + // U+2060 WORD JOINER are Unicode category Cf (format), not Zs (space + // separator) or part of ECMAScript's WhiteSpace production, so trimming + // either one leaves a non-empty string — a trim-then-check-length filter + // alone would let either slip past exactly the way '' used to. (U+00A0 and + // U+FEFF, by contrast, ARE stripped by .trim() and were never actually a + // gap here — verified directly before writing these tests: only U+200B and + // U+2060 reproduce it.) A denylist of "invisible" codepoints is not the + // fix: there are dozens of Cf/Cc codepoints and more get added, so this is + // a positive check instead — an entry only counts as a known own address + // if, after trimming, it starts with the 'addr' prefix every + // Cardano/Vector payment address actually uses. Nothing built purely from + // invisible characters can satisfy that. + // Built via fromCharCode rather than a literal escape so the codepoint is + // unambiguous in source and cannot be misrendered by an editor or diff tool. + const ZERO_WIDTH_SPACE = String.fromCharCode(0x200b); + const WORD_JOINER = String.fromCharCode(0x2060); + + test('a single ZERO WIDTH SPACE (U+200B) entry is treated as no known own address', () => { + const d = evaluate( + tx({ outputs: [{ address: FOREIGN, lovelace: 5_000_000n, assets: [] }] }), + [ZERO_WIDTH_SPACE], LIMITS, 0n + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /own address/i); + }); + + test('a single WORD JOINER (U+2060) entry is treated as no known own address', () => { + const d = evaluate( + tx({ outputs: [{ address: FOREIGN, lovelace: 5_000_000n, assets: [] }] }), + [WORD_JOINER], LIMITS, 0n + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /own address/i); + }); + + test('a genuine addr1... address is still accepted (the positive check does not over-reject)', () => { + const r = computeOutflow( + tx({ outputs: [{ address: OWN, lovelace: 5_000_000n, assets: [] }] }), + [OWN] + ); + assert.equal(r.netOutflowLovelace, 200_000n, 'a real own address must still be recognised as change'); + }); +}); + +describe('computeOutflow — the filter applies at the Set-construction site itself, not only in evaluate()', () => { + // evaluate()'s fail-closed guard and computeOutflow()'s own Set both call + // knownAddresses(), but nothing above exercises them independently: every + // prior blank/invisible-entry test either goes through evaluate() (whose + // guard would refuse before computeOutflow's classification could matter) + // or pairs a blank ownAddresses entry with a *real* output address (which + // would pass either way, filtered or not). That leaves a real gap: if a + // future edit filtered ownAddresses in evaluate()'s guard but left + // computeOutflow's `new Set(ownAddresses)` unfiltered, nothing here would + // catch it, because a blank entry in an otherwise-unfiltered Set only + // matters when something can actually match it — i.e. when an output + // address is *also* blank. decode.ts can never produce that: every output + // address comes from `o.address().to_bech32()`, which throws before an + // empty or blank address could ever reach a DecodedOutput (see decode.ts's + // per-output try/catch). So this is unreachable through the real pipeline + // — but computeOutflow is an exported pure function with its own contract, + // and that contract must hold regardless of what the current pipeline + // happens to be able to construct. + test('a blank output address is counted as outflow, not matched as change via a blank ownAddresses entry', () => { + const r = computeOutflow( + tx({ outputs: [{ address: '', lovelace: 5_000_000n, assets: [] }] }), + [''] + ); + assert.equal( + r.netOutflowLovelace, + 5_200_000n, + 'a blank output address must never be classified as change, even against a blank ownAddresses entry' + ); + }); + + test('an output address built from an invisible character is counted as outflow, not matched as change', () => { + const ZERO_WIDTH_SPACE = String.fromCharCode(0x200b); + const r = computeOutflow( + tx({ outputs: [{ address: ZERO_WIDTH_SPACE, lovelace: 5_000_000n, assets: [] }] }), + [ZERO_WIDTH_SPACE] + ); + assert.equal(r.netOutflowLovelace, 5_200_000n); + }); +}); + describe('evaluate — exact limit boundaries, one lovelace either side', () => { test('one lovelace under the per-transaction limit is allowed', () => { const d = evaluate( From ea3a23e5f9c25ef02bf8058c7ca3c86298929992 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:18:58 +0200 Subject: [PATCH 11/27] fix(signer): match own addresses after trimming, not before knownAddresses validated the trimmed value but stored the original, so an own address configured with an incidental leading or trailing space passed the fail-closed gate yet could never match its own outputs - every change output was then counted as outflow and normal transactions were refused. Also pins prefix-match, trim-before-check, and case-sensitivity, none of which any test previously required. --- packages/signer/src/policy.ts | 22 ++++++-- packages/signer/test/policy.test.ts | 81 +++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 5 deletions(-) diff --git a/packages/signer/src/policy.ts b/packages/signer/src/policy.ts index 8dff899..6693aa5 100644 --- a/packages/signer/src/policy.ts +++ b/packages/signer/src/policy.ts @@ -39,12 +39,24 @@ function ada(lovelace: bigint): string { // it starts with the 'addr' prefix every Cardano/Vector payment address // actually uses. Nothing built purely from blank or invisible characters can // satisfy that, without needing to enumerate any of them. -function looksLikeAddress(address: string): boolean { - return address.trim().startsWith('addr'); -} - +// +// The trimmed value is what gets kept, not the original: this array feeds +// straight into a `Set` that real output addresses (always clean, never +// padded) are matched against by exact string equality. An entry that only +// passes the `startsWith('addr')` check after trimming but is stored with +// its padding intact would pass this gate yet never match anything +// downstream — "known" but functionally invisible, silently misclassifying +// every one of its own change outputs as outflow. `.map` before `.filter` +// keeps the check and the stored value in agreement. +// +// Residual, deliberately not chased further: a *trailing* invisible +// character (e.g. U+200B) is not stripped by `.trim()`, so an address padded +// that way still will not match its own outputs. Same safe direction as +// everything else here — it over-refuses, never under-refuses — and closing +// it would mean reaching for the denylist-of-codepoints approach this +// function exists to avoid. function knownAddresses(ownAddresses: string[]): string[] { - return ownAddresses.filter(looksLikeAddress); + return ownAddresses.map((address) => address.trim()).filter((address) => address.startsWith('addr')); } /** diff --git a/packages/signer/test/policy.test.ts b/packages/signer/test/policy.test.ts index bfb4d69..e5e3652 100644 --- a/packages/signer/test/policy.test.ts +++ b/packages/signer/test/policy.test.ts @@ -215,6 +215,87 @@ describe('evaluate — ownAddresses entries built from invisible Unicode charact }); }); +describe('evaluate — knownAddresses must store the trimmed value, not just check it', () => { + // Real bug, found reviewing the invisible-character fix, present since that + // fix's very first version: the filter checked `address.trim().startsWith(...)` + // but Array.prototype.filter always returns the ORIGINAL element on a match, + // never a transformed one. So an entry that only qualified after trimming + // was kept *with its padding intact*, then compared by exact string + // equality against real output addresses (always clean, never padded) — + // which can never match. The address passes the fail-closed gate (it + // "looks like" a known address) but is then functionally invisible to + // computeOutflow's classification, so every one of its own change outputs + // gets counted as outflow instead of being excluded. Since change is + // usually the dominant value in a UTxO transaction, a single incidental + // leading or trailing space on a configured own address would refuse + // nearly every real transaction at a wildly inflated outflow figure. + test('a leading space on a real own address no longer hides its change output (regression test for the fix)', () => { + const d = evaluate( + tx({ outputs: [ + { address: FOREIGN, lovelace: 3_000_000n, assets: [] }, + { address: OWN, lovelace: 996_000_000n, assets: [] }, + ] }), + [' ' + OWN], LIMITS, 0n + ); + assert.equal(d.allowed, true); + assert.equal(d.netOutflowLovelace, 3_200_000n, 'only the genuine foreign spend + fee, not the misclassified change'); + }); + + test('a trailing space on a real own address no longer hides its change output', () => { + const d = evaluate( + tx({ outputs: [ + { address: FOREIGN, lovelace: 3_000_000n, assets: [] }, + { address: OWN, lovelace: 996_000_000n, assets: [] }, + ] }), + [OWN + ' '], LIMITS, 0n + ); + assert.equal(d.allowed, true); + assert.equal(d.netOutflowLovelace, 3_200_000n); + }); + + test('residual: a trailing invisible character still hides the change output, since .trim() does not strip it — same safe direction', () => { + // Documented, deliberate residual (see the comment on knownAddresses in + // policy.ts): fixing this specific case would mean reaching for the + // denylist-of-invisible-codepoints approach the positive check exists to + // avoid. Pinned here so it stays a known, bounded gap rather than an + // implicit assumption — and because it fails in the safe direction + // (refuses a legitimate transaction) rather than the dangerous one. + const ZERO_WIDTH_SPACE = String.fromCharCode(0x200b); + const d = evaluate( + tx({ outputs: [ + { address: FOREIGN, lovelace: 3_000_000n, assets: [] }, + { address: OWN, lovelace: 996_000_000n, assets: [] }, + ] }), + [OWN + ZERO_WIDTH_SPACE], LIMITS, 0n + ); + assert.equal(d.allowed, false); + assert.equal(d.netOutflowLovelace, 999_200_000n, 'the change is still misclassified as outflow — known, bounded, safe-direction residual'); + }); + + test('an entry merely containing "addr" without starting with it is not a known own address (prefix, not substring)', () => { + const d = evaluate( + tx({ outputs: [{ address: FOREIGN, lovelace: 5_000_000n, assets: [] }] }), + ['xxxaddrxxx'], LIMITS, 0n + ); + assert.equal(d.allowed, false); + assert.match(d.reason!, /own address/i); + }); + + test('a case-variant entry does not match a real own address (pins the existing case-sensitivity decision)', () => { + const CASE_VARIANT = 'ADDR1own'; // OWN is 'addr1own' + const d = evaluate( + tx({ outputs: [{ address: OWN, lovelace: 5_000_000n, assets: [] }] }), + [CASE_VARIANT], LIMITS, 0n + ); + // knownAddresses([CASE_VARIANT]) is empty (case-sensitive prefix check + // fails), so this hits the fail-closed "no own address" path rather than + // the over-counting path — either way, OWN must not be recognised as + // change, which is the property this test pins. + assert.equal(d.allowed, false); + assert.match(d.reason!, /own address/i); + }); +}); + describe('computeOutflow — the filter applies at the Set-construction site itself, not only in evaluate()', () => { // evaluate()'s fail-closed guard and computeOutflow()'s own Set both call // knownAddresses(), but nothing above exercises them independently: every From 74ddf873d6200fb53c9c462189a38c38db13f3a3 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:38:29 +0200 Subject: [PATCH 12/27] feat(signer): local append-only audit log Records intent at signing time; the signer has no network so it cannot observe submission. Refusals are recorded but never consume daily budget. Hardened beyond the brief after an edge-case pass found two genuine under-report vectors in committedTodayLovelace() (the function the daily limit's alreadySpentTodayLovelace comes from): a negative netOutflowLovelace string parses as a valid negative bigint and would silently subtract from the total, and a null element in the on-disk array crashes the summation outright rather than being skipped. Both are neutralized. Also: load() now reports a non-array log file on stderr instead of resetting silently, recent(0) no longer returns the whole log (Array.prototype.slice(-0) is slice(0), not "no entries" - a plain JS gotcha), and append() now throws on a write failure instead of swallowing it, since a swallowed failure is a normal-looking return while the entry silently exists only in memory until the next restart's reload finds it missing. Co-Authored-By: Claude Opus 5 --- packages/signer/src/audit.ts | 161 +++++++++++++ packages/signer/test/audit.test.ts | 362 +++++++++++++++++++++++++++++ 2 files changed, 523 insertions(+) create mode 100644 packages/signer/src/audit.ts create mode 100644 packages/signer/test/audit.test.ts diff --git a/packages/signer/src/audit.ts b/packages/signer/src/audit.ts new file mode 100644 index 0000000..c40acbd --- /dev/null +++ b/packages/signer/src/audit.ts @@ -0,0 +1,161 @@ +// Local, per-user audit log. Records intent at signing time, not confirmed +// submission — the signer has no network, so it cannot know whether a submit +// later succeeded. That over-counts on a failed submit, which is conservative. +// +// The log file is untrusted input every time it is read: it can be hand-edited, +// truncated by a process that died mid-write, or produced by a future version +// of this module with a different entry shape. Every method below treats it +// accordingly. Two different failure shapes get two different responses, on +// purpose: +// - The whole file is unreadable or not a JSON array: nothing in it can be +// trusted, so start clean. Loud on stderr, never silent (see load()). +// - The file is a JSON array but one element in it is garbage (not an +// object — most dangerously `null`, see below): drop only that element +// and keep the rest. A single bad entry must not erase an otherwise-good +// day's history, since committedTodayLovelace() is exactly what the +// daily spend limit depends on. +// Nothing here ever throws out of committedTodayLovelace() or recent(): a +// thrown exception from a "how much have I already spent" query is the kind +// of bug most likely to be handled upstream by assuming zero — which would +// silently defeat the daily limit. append() is the one exception: see its +// own comment for why a write failure there throws instead of swallowing. +import { readFileSync, writeFileSync, existsSync } from 'node:fs'; + +export interface AuditEntry { + timestamp: string; + txHash: string; + decision: 'signed' | 'refused'; + netOutflowLovelace: string; + reason?: string; + assetMovements: number; +} + +export class AuditLog { + private entries: AuditEntry[] = []; + + constructor(private readonly filePath: string) { + this.load(); + } + + private load(): void { + if (!existsSync(this.filePath)) return; + try { + const parsed: unknown = JSON.parse(readFileSync(this.filePath, 'utf8')); + if (!Array.isArray(parsed)) { + throw new Error('log file did not contain a JSON array'); + } + + // JSON.parse can hand back array elements of any shape. Most of those + // are harmless here even though they are not real AuditEntry objects: + // a stray string, number, or boolean silently returns `undefined` for + // `.decision`, which just fails the `!== 'signed'` check like any other + // unrecognised value. `null` is the one exception — property access on + // null throws — so committedTodayLovelace() would crash on the entry + // AFTER it, and every entry after that, not just skip the bad one. + // Filtering to object-shaped, non-array elements up front means every + // other method can assume its entries are at least objects, even if + // individual fields on them still turn out to be garbage (handled + // separately, per field, in committedTodayLovelace()). + const usable = parsed.filter( + (e): e is AuditEntry => e !== null && typeof e === 'object' && !Array.isArray(e) + ); + if (usable.length !== parsed.length) { + console.error( + `[audit] ${this.filePath}: dropped ${parsed.length - usable.length} of ${parsed.length} log entries that were not objects (e.g. null); keeping the rest` + ); + } + this.entries = usable; + } catch (err) { + // A corrupt log must not stop the signer from working, and must not be + // silently trusted either. Start clean; the file is overwritten on the + // next append. This branch also catches a valid-JSON-but-wrong-shape + // file (an object, `null`, a string, ...) via the throw above — that is + // just as untrustworthy as a syntax error and gets the same fresh-start + // treatment and the same stderr line. + console.error( + `[audit] ${this.filePath} was unreadable (${err instanceof Error ? err.message : String(err)}); starting a fresh log` + ); + this.entries = []; + } + } + + append(entry: AuditEntry): void { + // Pushed before the write is attempted: if the write below fails, this + // process's own in-memory state (committedTodayLovelace(), recent()) + // must still reflect the decision that was actually made. Losing it here + // would under-count even within this same process, which would be worse + // than the cross-restart gap discussed below. + this.entries.push(entry); + try { + writeFileSync(this.filePath, JSON.stringify(this.entries, null, 2) + '\n'); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + console.error(`[audit] could not write ${this.filePath}: ${message}`); + // Deliberately rethrown rather than swallowed. Swallowing would make + // append() return normally — a plausible-looking success — while the + // entry silently exists only in this process's memory. That is fine + // until the next restart: a fresh AuditLog reloads from disk, does not + // find this entry, and committedTodayLovelace() under-reports by + // exactly its amount. For a 'signed' entry that is precisely the + // failure this module exists to prevent — it raises the effective + // daily cap with no visible symptom until someone goes looking. Making + // the write failure loud and synchronous, at the moment it happens, + // is the fail-closed choice: the caller finds out now, instead of a + // future process finding out by silently allowing more spend than + // intended. This does mean a write failure can surface as a thrown + // error at the call site (e.g. the tool call that just signed a + // transaction); the caller is expected to treat that as "the signature + // was produced but its audit record was not durably saved" and act + // accordingly, not to retry blindly. + throw new Error(`Audit log write failed (entry kept in memory only, not persisted): ${message}`); + } + } + + /** Lovelace committed today (UTC) by signed transactions. Refusals do not count. */ + committedTodayLovelace(): bigint { + const startOfDayUtc = new Date(); + startOfDayUtc.setUTCHours(0, 0, 0, 0); + const cutoff = startOfDayUtc.getTime(); + + let total = 0n; + for (const e of this.entries) { + if (e.decision !== 'signed') continue; + const t = new Date(e.timestamp).getTime(); + if (Number.isNaN(t) || t < cutoff) continue; + + let amount: bigint; + try { + amount = BigInt(e.netOutflowLovelace); + } catch { + // Not parseable at all (non-numeric text, a decimal, ...). Ignore + // rather than crash the signer's accounting. + continue; + } + // BigInt string parsing accepts more than plain positive integers. + // BigInt('') is 0n — a harmless no-op, not a crash and not skipped, + // just zero. BigInt('-5000000') is a valid negative bigint, and that + // one is NOT harmless: adding a negative amount can only ever reduce + // the running total, i.e. under-report — the one direction this + // function must never move in. Nothing in the real pipeline can + // produce a negative netOutflowLovelace (policy.ts sums non-negative + // output lovelace plus a non-negative fee), so a negative value here + // can only come from a corrupted or hand-edited log entry. Treated the + // same as any other unparseable amount: skipped, not subtracted. + if (amount < 0n) continue; + total += amount; + } + return total; + } + + recent(limit: number): AuditEntry[] { + // Array.prototype.slice(-0) behaves as slice(0): -0 is not < 0, so the + // "count back from the end" branch never triggers, and the call returns + // the ENTIRE array instead of none. limit <= 0 is handled explicitly so + // recent(0) means "no entries" rather than "all of them" — and the same + // guard makes a negative limit equally harmless instead of doing + // whatever slice() would otherwise do with it (drop entries off the + // front, which is not a sensible reading of "give me N recent entries"). + if (limit <= 0) return []; + return this.entries.slice(-limit).reverse(); + } +} diff --git a/packages/signer/test/audit.test.ts b/packages/signer/test/audit.test.ts new file mode 100644 index 0000000..40e12a2 --- /dev/null +++ b/packages/signer/test/audit.test.ts @@ -0,0 +1,362 @@ +import { describe, test, beforeEach, afterEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, rmSync, existsSync, readFileSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { AuditLog, type AuditEntry } from '../src/audit.ts'; + +let dir: string; +let logPath: string; + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'signer-audit-')); + logPath = join(dir, 'audit.json'); +}); +afterEach(() => rmSync(dir, { recursive: true, force: true })); + +function entry(over: Partial = {}): AuditEntry { + return { + timestamp: new Date().toISOString(), + txHash: 'b'.repeat(64), + decision: 'signed', + netOutflowLovelace: '3000000', + assetMovements: 0, + ...over, + }; +} + +describe('AuditLog', () => { + test('starts empty when the file does not exist', () => { + const log = new AuditLog(logPath); + assert.deepStrictEqual(log.recent(10), []); + assert.equal(log.committedTodayLovelace(), 0n); + }); + + test('appends and persists to disk', () => { + new AuditLog(logPath).append(entry()); + assert.ok(existsSync(logPath)); + assert.equal(JSON.parse(readFileSync(logPath, 'utf8')).length, 1); + }); + + test('reloads entries written by a previous instance', () => { + new AuditLog(logPath).append(entry()); + assert.equal(new AuditLog(logPath).recent(10).length, 1); + }); + + test('sums only signed entries into today total', () => { + const log = new AuditLog(logPath); + log.append(entry({ netOutflowLovelace: '3000000' })); + log.append(entry({ netOutflowLovelace: '5000000' })); + assert.equal(log.committedTodayLovelace(), 8_000_000n); + }); + + test('excludes refused entries from today total', () => { + const log = new AuditLog(logPath); + log.append(entry({ netOutflowLovelace: '3000000' })); + log.append(entry({ decision: 'refused', netOutflowLovelace: '900000000', reason: 'over limit' })); + assert.equal(log.committedTodayLovelace(), 3_000_000n, 'a refusal must not consume budget'); + }); + + test('excludes entries from previous days', () => { + const log = new AuditLog(logPath); + log.append(entry({ timestamp: '2020-01-01T00:00:00.000Z', netOutflowLovelace: '400000000' })); + log.append(entry({ netOutflowLovelace: '3000000' })); + assert.equal(log.committedTodayLovelace(), 3_000_000n); + }); + + test('recent() returns newest first and respects the limit', () => { + const log = new AuditLog(logPath); + log.append(entry({ txHash: 'c'.repeat(64) })); + log.append(entry({ txHash: 'd'.repeat(64) })); + const r = log.recent(1); + assert.equal(r.length, 1); + assert.equal(r[0].txHash, 'd'.repeat(64)); + }); + + test('survives a corrupt log file rather than crashing', () => { + // The brief's own version of this test called `require('node:fs')` here, + // which throws `ReferenceError: require is not defined` in this package: + // packages/signer/package.json sets "type": "module", so this file is + // real ESM (not CJS-via-tsx-in-a-directory-with-no-package.json, which is + // what made an ad-hoc isolated check of the same snippet misleadingly + // pass). Using the top-level import instead is the only change from the + // brief's given test code needed to make it run at all in this package. + writeFileSync(logPath, 'not json at all'); + const log = new AuditLog(logPath); + assert.deepStrictEqual(log.recent(10), []); + log.append(entry()); + assert.equal(log.recent(10).length, 1); + }); +}); + +// --- Edge-case pass (beyond the brief) -------------------------------------- +// +// The brief's 8 tests are a floor. Everything below was added after they +// passed, hunting specifically for the failure shape this module is exposed +// to: committedTodayLovelace() quietly returning a number that is too LOW +// (which loosens the daily limit) rather than throwing or visibly failing. +// Some of these were verified against plain Node semantics before writing an +// assertion, precisely because a couple of them are not what intuition +// suggests (see the BigInt('') and slice(-0) cases below). + +/** Captures console.error output for the duration of `fn`, then restores it. */ +function captureStderr(fn: () => void): string[] { + const messages: string[] = []; + const original = console.error; + console.error = (...args: unknown[]) => { + messages.push(args.map(String).join(' ')); + }; + try { + fn(); + } finally { + console.error = original; + } + return messages; +} + +describe('committedTodayLovelace — netOutflowLovelace parsing', () => { + test('an unparseable amount (non-numeric text) does not crash the total and contributes nothing', () => { + const log = new AuditLog(logPath); + log.append(entry({ netOutflowLovelace: '3000000' })); + log.append(entry({ netOutflowLovelace: 'not-a-number' })); + assert.equal(log.committedTodayLovelace(), 3_000_000n); + }); + + test('a decimal amount is rejected by BigInt (no fractional lovelace) and contributes nothing', () => { + const log = new AuditLog(logPath); + log.append(entry({ netOutflowLovelace: '3000000' })); + log.append(entry({ netOutflowLovelace: '3000000.5' })); + assert.equal(log.committedTodayLovelace(), 3_000_000n); + }); + + test('an empty-string amount parses as BigInt("") === 0n — verified directly, it does NOT throw — so it contributes zero rather than crashing or getting "skipped"', () => { + // BigInt("") is a genuine JS gotcha (mirrors Number("") === 0): the catch + // block never fires for this input, so this is a different code path + // from the non-numeric-text case above even though the externally + // observable result (no change to the total) looks the same. + const log = new AuditLog(logPath); + log.append(entry({ netOutflowLovelace: '3000000' })); + log.append(entry({ netOutflowLovelace: '' })); + assert.equal(log.committedTodayLovelace(), 3_000_000n); + }); + + test('a negative amount parses successfully as a valid negative bigint but must not reduce the total', () => { + // The dangerous case: BigInt('-5000000') does not throw, so without an + // explicit sign check this entry would SUBTRACT from an already-correct + // total — an actual under-report, not just a missed contribution. Not + // reachable through the real decode.ts -> policy.ts pipeline (outputs + // and fees are always non-negative), so this can only arise from a + // corrupted or hand-edited log file, which is exactly the input this + // module must not trust. + const log = new AuditLog(logPath); + log.append(entry({ netOutflowLovelace: '5000000' })); + log.append(entry({ netOutflowLovelace: '-5000000' })); + assert.equal( + log.committedTodayLovelace(), + 5_000_000n, + 'a negative entry must never subtract from the committed total' + ); + }); +}); + +describe('committedTodayLovelace — timestamp handling', () => { + test('a missing timestamp field does not crash the total and is excluded', () => { + const raw = [ + { txHash: 'e'.repeat(64), decision: 'signed', netOutflowLovelace: '3000000', assetMovements: 0 }, + // no `timestamp` key at all — simulates an older or hand-edited entry. + ]; + writeFileSync(logPath, JSON.stringify(raw)); + const log = new AuditLog(logPath); + assert.equal(log.committedTodayLovelace(), 0n); + assert.equal(log.recent(10).length, 1, 'the entry itself is still visible for forensic purposes'); + }); + + test('an unparseable timestamp string does not crash the total and is excluded', () => { + const log = new AuditLog(logPath); + log.append(entry({ netOutflowLovelace: '3000000' })); + log.append({ ...entry({ netOutflowLovelace: '9000000' }), timestamp: 'not-a-date' }); + assert.equal(log.committedTodayLovelace(), 3_000_000n); + }); + + test('a JSON null timestamp resolves to the 1970 epoch (verified: does not throw) and is excluded as not-today', () => { + // new Date(null) is new Date(+null) = new Date(0) = the epoch — a VALID + // date, not NaN. It still gets excluded, but via the "not today" branch + // rather than the NaN branch; worth pinning so that distinction stays + // intentional rather than incidental. + const raw = [ + { + txHash: 'e'.repeat(64), + decision: 'signed', + netOutflowLovelace: '3000000', + assetMovements: 0, + timestamp: null, + }, + ]; + writeFileSync(logPath, JSON.stringify(raw)); + const log = new AuditLog(logPath); + assert.equal(log.committedTodayLovelace(), 0n); + }); +}); + +describe('committedTodayLovelace — the UTC day boundary', () => { + test('23:59 UTC yesterday is excluded and 00:01 UTC today is included', () => { + const now = new Date(); + const startOfTodayUtc = Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate()); + const yesterday2359 = new Date(startOfTodayUtc - 60_000).toISOString(); // 23:59:00 UTC yesterday + const today0001 = new Date(startOfTodayUtc + 60_000).toISOString(); // 00:01:00 UTC today + + const log = new AuditLog(logPath); + log.append(entry({ timestamp: yesterday2359, netOutflowLovelace: '400000000' })); + log.append(entry({ timestamp: today0001, netOutflowLovelace: '3000000' })); + assert.equal(log.committedTodayLovelace(), 3_000_000n, 'only the entry from today (UTC) should count'); + }); + + test('an entry timestamped at exactly UTC midnight counts as today (inclusive boundary)', () => { + const now = new Date(); + const startOfTodayUtc = new Date( + Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate()) + ).toISOString(); + + const log = new AuditLog(logPath); + log.append(entry({ timestamp: startOfTodayUtc, netOutflowLovelace: '3000000' })); + assert.equal(log.committedTodayLovelace(), 3_000_000n); + }); +}); + +describe('load() — malformed on-disk shapes', () => { + test('a JSON object at the top level (not an array) starts clean rather than crashing', () => { + writeFileSync(logPath, JSON.stringify({ oops: 'not an array' })); + const log = new AuditLog(logPath); + assert.deepStrictEqual(log.recent(10), []); + assert.equal(log.committedTodayLovelace(), 0n); + }); + + test('a bare JSON null at the top level starts clean rather than crashing', () => { + writeFileSync(logPath, 'null'); + const log = new AuditLog(logPath); + assert.deepStrictEqual(log.recent(10), []); + assert.equal(log.committedTodayLovelace(), 0n); + }); + + test('a non-array top-level shape is reported on stderr, not silently accepted', () => { + const messages = captureStderr(() => { + writeFileSync(logPath, JSON.stringify({ oops: 'not an array' })); + new AuditLog(logPath); + }); + assert.equal(messages.length, 1); + assert.match(messages[0], /\[audit\]/); + }); + + test('a null element inside an otherwise-valid array crashed committedTodayLovelace() before the fix; now it is dropped and the rest of the log still counts', () => { + // Verified directly against plain JS before writing this test: accessing + // `.decision` on `null` throws (unlike a stray string or number element, + // which just yields `undefined` and is safely skipped). One bad element + // anywhere in the array — from a hand edit, a future producer bug, or a + // torn write that happens to still leave valid-but-odd JSON — must not + // take down accounting for every entry in the file. + const good = entry({ netOutflowLovelace: '5000000' }); + writeFileSync(logPath, JSON.stringify([null, good])); + const log = new AuditLog(logPath); + assert.equal(log.committedTodayLovelace(), 5_000_000n); + assert.equal(log.recent(10).length, 1, 'only the one genuine entry survives; null is dropped, not kept as a phantom entry'); + }); + + test('dropping a null element is reported on stderr, not silently accepted', () => { + const good = entry({ netOutflowLovelace: '5000000' }); + const messages = captureStderr(() => { + writeFileSync(logPath, JSON.stringify([null, good])); + new AuditLog(logPath); + }); + assert.equal(messages.length, 1); + assert.match(messages[0], /\[audit\]/); + assert.match(messages[0], /dropped 1 of 2/); + }); +}); + +describe('recent() — limit handling', () => { + test('recent(0) returns no entries, not the entire log (slice(-0) === slice(0) is a JS gotcha — verified directly)', () => { + // [1,2,3].slice(-0) returns [1,2,3], the whole array, because -0 < 0 is + // false in JS — the "count back from the end" branch never triggers. + // Without an explicit limit <= 0 guard, recent(0) would silently return + // every entry ever recorded instead of none. + const log = new AuditLog(logPath); + log.append(entry()); + log.append(entry()); + assert.deepStrictEqual(log.recent(0), []); + }); + + test('a negative limit is treated the same as zero rather than slicing from the front', () => { + const log = new AuditLog(logPath); + log.append(entry()); + assert.deepStrictEqual(log.recent(-1), []); + }); + + test('recent(n) where n exceeds the entry count returns everything available, newest first', () => { + const log = new AuditLog(logPath); + log.append(entry({ txHash: 'c'.repeat(64) })); + log.append(entry({ txHash: 'd'.repeat(64) })); + const r = log.recent(1000); + assert.equal(r.length, 2); + assert.equal(r[0].txHash, 'd'.repeat(64)); + assert.equal(r[1].txHash, 'c'.repeat(64)); + }); +}); + +describe('append() — write failure', () => { + test('a write to an unwritable path throws, but the entry is kept in this process\'s memory rather than lost', () => { + const unwritablePath = join(dir, 'no-such-subdirectory', 'audit.json'); + const log = new AuditLog(unwritablePath); + assert.throws(() => log.append(entry({ netOutflowLovelace: '3000000' })), /audit log write failed/i); + // The write failed, but the decision it recorded must still be visible + // to this same process — see the comment on append() in audit.ts for why + // silently losing it here (or silently swallowing the write failure) + // would be worse than throwing. + assert.equal(log.recent(10).length, 1); + assert.equal(log.committedTodayLovelace(), 3_000_000n); + }); + + test('a write failure is reported on stderr in addition to being thrown', () => { + const unwritablePath = join(dir, 'no-such-subdirectory', 'audit.json'); + const log = new AuditLog(unwritablePath); + const messages = captureStderr(() => { + assert.throws(() => log.append(entry())); + }); + assert.equal(messages.length, 1); + assert.match(messages[0], /\[audit\]/); + }); + + test('a refused entry\'s write failure throws exactly like a signed entry\'s — append() does not special-case by decision', () => { + const unwritablePath = join(dir, 'no-such-subdirectory', 'audit.json'); + const log = new AuditLog(unwritablePath); + assert.throws( + () => log.append(entry({ decision: 'refused', reason: 'over limit', netOutflowLovelace: '900000000' })), + /audit log write failed/i + ); + assert.equal(log.recent(10).length, 1); + }); +}); + +describe('committedTodayLovelace — decision field integrity', () => { + test('recent() returns entries newest-first even when some are refused, and refused entries carry their reason', () => { + const log = new AuditLog(logPath); + log.append(entry({ netOutflowLovelace: '3000000' })); + log.append(entry({ decision: 'refused', netOutflowLovelace: '900000000', reason: 'over limit' })); + const r = log.recent(10); + assert.equal(r.length, 2); + assert.equal(r[0].decision, 'refused'); + assert.equal(r[0].reason, 'over limit'); + assert.equal(r[1].decision, 'signed'); + }); + + test('many interleaved signed and refused entries: only the signed amounts sum, regardless of order', () => { + const log = new AuditLog(logPath); + log.append(entry({ decision: 'refused', netOutflowLovelace: '999000000', reason: 'too big' })); + log.append(entry({ netOutflowLovelace: '1000000' })); + log.append(entry({ decision: 'refused', netOutflowLovelace: '888000000', reason: 'too big' })); + log.append(entry({ netOutflowLovelace: '2000000' })); + log.append(entry({ decision: 'refused', netOutflowLovelace: '777000000', reason: 'too big' })); + log.append(entry({ netOutflowLovelace: '4000000' })); + assert.equal(log.committedTodayLovelace(), 7_000_000n); + assert.equal(log.recent(100).length, 6); + }); +}); From 87d1bcf1a7cdbb987c8be825bfff9e6f28be8bf4 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:01:53 +0200 Subject: [PATCH 13/27] fix(signer): survive a torn write, and pin the skip-don't-abort behaviour A malformed amount followed by valid entries silently dropped every later entry when the loop aborted instead of skipping - 30/30 tests passed against that mutation because every bad-amount case put the bad entry last. Persistence now writes to a temp file and renames, so a kill mid-write costs at most the in-flight entry rather than the whole day's history. Co-Authored-By: Claude Opus 5 --- packages/signer/src/audit.ts | 68 ++++++++++++++++++++++-------- packages/signer/test/audit.test.ts | 51 ++++++++++++++++++++++ 2 files changed, 101 insertions(+), 18 deletions(-) diff --git a/packages/signer/src/audit.ts b/packages/signer/src/audit.ts index c40acbd..4d2196c 100644 --- a/packages/signer/src/audit.ts +++ b/packages/signer/src/audit.ts @@ -19,7 +19,17 @@ // of bug most likely to be handled upstream by assuming zero — which would // silently defeat the daily limit. append() is the one exception: see its // own comment for why a write failure there throws instead of swallowing. -import { readFileSync, writeFileSync, existsSync } from 'node:fs'; +// +// CONTRACT for callers of append(): it can throw. If you wrap it in +// try/catch — a reasonable instinct, since an uncaught throw from an audit +// module makes for an ugly tool-call failure — catching that throw must NOT +// be followed by still returning a signature you already produced. append() +// is normally called immediately after a transaction is signed; catching its +// throw and returning the signature anyway reproduces exactly the failure +// this module exists to prevent, just one call site later: a real signature +// reaches the caller while no durable record of it exists on disk, so the +// next process's committedTodayLovelace() will never count it. +import { readFileSync, writeFileSync, existsSync, renameSync } from 'node:fs'; export interface AuditEntry { timestamp: string; @@ -79,6 +89,11 @@ export class AuditLog { } } + /** + * Persists `entry`. Can throw if the log cannot be durably written — see + * the CONTRACT note at the top of this file before wrapping this in + * try/catch. + */ append(entry: AuditEntry): void { // Pushed before the write is attempted: if the write below fails, this // process's own in-memory state (committedTodayLovelace(), recent()) @@ -86,27 +101,44 @@ export class AuditLog { // would under-count even within this same process, which would be worse // than the cross-restart gap discussed below. this.entries.push(entry); + + // Written to a temp file and renamed into place, rather than written + // directly to this.filePath, to survive a process kill mid-write (power + // loss, OOM kill — not a normal error path, but a real one for a + // long-running signer). Writing this.filePath directly has two problems + // a temp file avoids: writeFileSync truncates its target the moment it + // opens it, before a single byte is written, so a kill right after that + // truncation would destroy the entire existing log, not just fail to add + // the new entry; and even a completed write can leave the file + // half-written if the kill lands mid-syscall. Neither can happen to + // this.filePath here, because this.filePath is never opened for writing + // at all — only tmpPath is. The rename that publishes tmpPath over + // this.filePath is a single directory-entry update on the same + // filesystem, which POSIX and Windows both perform atomically: a kill + // during the rename itself cannot leave this.filePath half-written + // either, it is simply still the old complete file or already the new + // complete one. The only entry a kill can cost is the one in flight — + // shrinking the worst case from "lose the whole day's history" to "lose + // at most this one entry", which is the same entry append() already + // cannot guarantee past a hard kill regardless of persistence strategy. + const tmpPath = `${this.filePath}.tmp`; try { - writeFileSync(this.filePath, JSON.stringify(this.entries, null, 2) + '\n'); + writeFileSync(tmpPath, JSON.stringify(this.entries, null, 2) + '\n'); + renameSync(tmpPath, this.filePath); } catch (err) { const message = err instanceof Error ? err.message : String(err); console.error(`[audit] could not write ${this.filePath}: ${message}`); - // Deliberately rethrown rather than swallowed. Swallowing would make - // append() return normally — a plausible-looking success — while the - // entry silently exists only in this process's memory. That is fine - // until the next restart: a fresh AuditLog reloads from disk, does not - // find this entry, and committedTodayLovelace() under-reports by - // exactly its amount. For a 'signed' entry that is precisely the - // failure this module exists to prevent — it raises the effective - // daily cap with no visible symptom until someone goes looking. Making - // the write failure loud and synchronous, at the moment it happens, - // is the fail-closed choice: the caller finds out now, instead of a - // future process finding out by silently allowing more spend than - // intended. This does mean a write failure can surface as a thrown - // error at the call site (e.g. the tool call that just signed a - // transaction); the caller is expected to treat that as "the signature - // was produced but its audit record was not durably saved" and act - // accordingly, not to retry blindly. + // Deliberately rethrown rather than swallowed — see the CONTRACT note + // at the top of this file for what a catching caller must not do. + // Swallowing would make append() return normally — a plausible- + // looking success — while the entry silently exists only in this + // process's memory. That is fine until the next restart: a fresh + // AuditLog reloads from disk, does not find this entry, and + // committedTodayLovelace() under-reports by exactly its amount. For a + // 'signed' entry that is precisely the failure this module exists to + // prevent — it raises the effective daily cap with no visible symptom + // until someone goes looking. Making the write failure loud and + // synchronous, at the moment it happens, is the fail-closed choice. throw new Error(`Audit log write failed (entry kept in memory only, not persisted): ${message}`); } } diff --git a/packages/signer/test/audit.test.ts b/packages/signer/test/audit.test.ts index 40e12a2..5eea5d6 100644 --- a/packages/signer/test/audit.test.ts +++ b/packages/signer/test/audit.test.ts @@ -157,6 +157,29 @@ describe('committedTodayLovelace — netOutflowLovelace parsing', () => { 'a negative entry must never subtract from the committed total' ); }); + + test('a malformed amount does not abort processing of later signed entries (mutation guard: continue vs break)', () => { + // Every other bad-amount test above appends the malformed entry LAST, so + // it cannot tell a `continue` (skip this entry, keep summing) from a + // `break` (stop summing entirely) — nothing follows the bad entry in + // those tests to be lost either way. Reviewed and confirmed as a real + // coverage gap: mutating the `continue` inside the BigInt catch block + // (committedTodayLovelace()'s parse-failure branch) to a `break` still + // passed all 30 tests. Putting the bad entry FIRST, followed by entries + // that must still count, is what actually distinguishes the two: a + // `break` here would silently stop at the first entry and report 0 + // instead of 12,000,000 — an under-report of everything after the bad + // entry, not just the bad entry itself. + const log = new AuditLog(logPath); + log.append(entry({ netOutflowLovelace: 'not-a-number' })); + log.append(entry({ netOutflowLovelace: '5000000' })); + log.append(entry({ netOutflowLovelace: '7000000' })); + assert.equal( + log.committedTodayLovelace(), + 12_000_000n, + 'entries after a malformed one must still be summed, not silently dropped' + ); + }); }); describe('committedTodayLovelace — timestamp handling', () => { @@ -336,6 +359,34 @@ describe('append() — write failure', () => { }); }); +describe('append() — durable writes (temp file + rename)', () => { + // append() writes to `${filePath}.tmp` and renames it over filePath rather + // than writing filePath directly, so that a process kill mid-write costs + // at most the one in-flight entry instead of truncating the whole day's + // history (writeFileSync truncates its target the instant it opens it, + // before any byte is written — see the comment on append() in audit.ts). + // This only helps if the temp file is actually transient; if it were left + // behind after every successful append, load() would still never read it + // (load() only ever reads filePath), but it would be visible litter next + // to the real log and would eventually be mistaken for something meaningful. + test('a completed append() leaves no stray .tmp file behind', () => { + const log = new AuditLog(logPath); + log.append(entry()); + assert.equal(existsSync(logPath), true); + assert.equal(existsSync(`${logPath}.tmp`), false); + }); + + test('no stray .tmp file accumulates across repeated appends, and the log still reloads correctly', () => { + const log = new AuditLog(logPath); + log.append(entry({ txHash: 'c'.repeat(64) })); + log.append(entry({ txHash: 'd'.repeat(64) })); + log.append(entry({ txHash: 'e'.repeat(64) })); + assert.equal(existsSync(`${logPath}.tmp`), false); + assert.equal(JSON.parse(readFileSync(logPath, 'utf8')).length, 3); + assert.equal(new AuditLog(logPath).recent(10).length, 3); + }); +}); + describe('committedTodayLovelace — decision field integrity', () => { test('recent() returns entries newest-first even when some are refused, and refused entries carry their reason', () => { const log = new AuditLog(logPath); From 60f001921bbdc6646ff20176ebf47e8e3e9cd55b Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:27:24 +0200 Subject: [PATCH 14/27] feat(signer): pluggable key sources (mnemonic and raw private key) walletFromSeed is pure derivation and needs no Provider, keeping this module network-free. MnemonicKeySource is the familiar default; PrivateKeyKeySource is strictly safer since it controls one account rather than the whole HD tree. describe() is asserted never to leak key material. Verified rather than assumed: walletFromSeed's private key prefix is ed25519e_sk1 and its address is addr1... (Base, Mainnet default) - the brief's regex already matched. The brief's test mnemonic passes BIP39 checksum validation. Hardened beyond the brief after an edge-case pass: - Mnemonic case: bip39's wordlist lookup is case-sensitive, so an uppercase or mixed-case mnemonic threw outright. Casing carries no BIP39 meaning, so lowercasing is safe - now applied, and verified to derive the identical address rather than merely "some" address. - PrivateKeyKeySource validated the trimmed key/address but stored the original untrimmed value - the same knownAddresses() bug shape as policy.ts (validate trimmed, store padded, silently fail every later exact-string comparison). Now stores what was validated. - Empty mnemonic input reported "got 1" word (''.split(' ') === ['']), not "got 0" - message accuracy only, both branches already rejected. Documented, not fixed: PrivateKeyKeySource cannot verify that `address` is actually controlled by `privateKeyBech32`. That needs CML, and this module deliberately imports only @lucid-evolution/wallet, whose one raw-private-key function requires a live Provider. The failure is bounded if misused - signing needs no address, so a mismatch surfaces as an on-chain rejection, and policy.ts's own-address matching only ever over-refuses on a wrong address, never under-refuses. 29 tests beyond the brief's 10. 5 of the fixes above were mutation-tested (each reverted and confirmed to fail red before restoring the fix). Co-Authored-By: Claude Opus 5 --- packages/signer/src/keysource.ts | 186 +++++++++++++ packages/signer/test/keysource.test.ts | 371 +++++++++++++++++++++++++ 2 files changed, 557 insertions(+) create mode 100644 packages/signer/src/keysource.ts create mode 100644 packages/signer/test/keysource.test.ts diff --git a/packages/signer/src/keysource.ts b/packages/signer/src/keysource.ts new file mode 100644 index 0000000..bdf4b39 --- /dev/null +++ b/packages/signer/src/keysource.ts @@ -0,0 +1,186 @@ +// Key material sources. Pure derivation only: walletFromSeed needs no Provider, +// so nothing here can reach the network. +// +// A raw private key is strictly safer than a mnemonic: it controls one account +// rather than the entire HD tree, so a leak is bounded. +// +// SECURITY: this module handles the most sensitive data in the system. Key +// material (mnemonics, private keys) must never reach describe(), a thrown +// error, or a log line -- only the *source* of a failure ("mnemonic could not +// be derived", "invalid private key"), never the value. Every catch block +// below is commented with why the upstream message it surfaces is safe to +// include verbatim. +import { walletFromSeed } from '@lucid-evolution/wallet'; + +export interface SigningIdentity { + privateKeyBech32: string; + address: string; +} + +export interface KeySource { + /** Human-readable description for logs. MUST NOT include key material. */ + describe(): string; + load(): SigningIdentity; +} + +const VALID_WORD_COUNTS = [12, 15, 18, 21, 24]; + +export class MnemonicKeySource implements KeySource { + private readonly mnemonic: string; + + constructor(mnemonic: string, private readonly accountIndex: number = 0) { + // Whitespace normalisation: collapse any run of whitespace (including + // tabs/newlines and Unicode space separators like NBSP, which JS `\s` + // already covers) to a single space, and trim the ends. This is the + // ONLY normalisation that can be done safely with a denylist-free, + // positive approach for whitespace -- see the residual note below for + // what it deliberately does not catch. + // + // Case normalisation: BIP39 wordlist lookup is case-sensitive + // (`wordlist.indexOf(word)`), but casing carries no BIP39 semantic + // weight -- a word is identified purely by which of the 2048 fixed + // wordlist entries it matches, never by how it happens to be + // capitalised. There is no such thing as two different valid mnemonics + // that differ only in case, so lowercasing here cannot turn one legitimate + // mnemonic into a different, also-legitimate one -- it can only turn an + // otherwise-correct mnemonic that a client capitalised (autocapitalize, + // pasted from a sentence, typed by hand) from a hard, unhelpful throw + // into the working derivation the caller actually intended. Verified + // empirically before relying on this: lowercasing an uppercase or + // mixed-case rendering of a valid mnemonic reproduces the exact + // canonical lowercase string byte-for-byte, and therefore derives the + // exact same address -- never a different, also-plausible one. + this.mnemonic = mnemonic.trim().split(/\s+/).join(' ').toLowerCase(); + + // `''.split(' ')` yields `['']` (length 1), not `[]` (length 0) -- the + // same empty-string-split gotcha documented at length in policy.ts's + // knownAddresses(). Handled explicitly so a blank/whitespace-only + // mnemonic is reported as "got 0 words", not the misleading "got 1". + // Either way this throws and derives nothing; this only fixes what the + // message says. + const words = this.mnemonic.length === 0 ? [] : this.mnemonic.split(' '); + if (!VALID_WORD_COUNTS.includes(words.length)) { + throw new Error( + `Invalid mnemonic: expected 12, 15, 18, 21 or 24 words, got ${words.length}.` + ); + } + + // Residual, deliberately not chased further (same shape as policy.ts's + // own documented residual): a character that is invisible but is NOT + // matched by `\s` -- e.g. U+200B ZERO WIDTH SPACE or U+2060 WORD + // JOINER, neither of which are in ECMAScript's WhiteSpace production -- + // survives `.split(/\s+/)` glued onto whichever word it sits next to. + // Unlike policy.ts's address matching (an exact-equality check against + // caller-supplied strings, where a corrupted entry can silently fail to + // match and be misclassified), this does not create a silent-wrong- + // derivation risk: BIP39 word lookup is an exact match against a fixed, + // closed 2048-word list, so a corrupted word can (for all practical, + // non-adversarially-constructed input) never happen to equal a + // different real wordlist word. The corrupted word simply fails to + // match anything, and load() throws "Invalid mnemonic" -- confirmed + // empirically, see keysource.test.ts. The failure is safe (loud, no + // derivation), just not always caught at construction time the way a + // word-count mismatch is, because the corrupted string still counts as + // one "word" by count alone. + } + + describe(): string { + return `mnemonic (account index ${this.accountIndex})`; + } + + load(): SigningIdentity { + // Vector runs with the --mainnet flag, so addresses are addr1... + let w; + try { + w = walletFromSeed(this.mnemonic, { network: 'Mainnet', accountIndex: this.accountIndex }); + } catch (err) { + // Never interpolate `this.mnemonic` here. Read bip39's source + // (mnemonicToEntropy, which walletFromSeed calls directly) to confirm + // this before relying on it: every throw site in that function uses + // one of four FIXED constant strings -- 'Invalid mnemonic' (wrong + // word count, or a word absent from the wordlist), 'Invalid mnemonic + // checksum' (right words, wrong combination), 'Invalid entropy', or a + // wordlist-config error -- and none of them ever interpolates the + // input. err.message is therefore safe to surface verbatim. This + // also confirms the safety property the constructor's comment + // depends on: a bad mnemonic throws here rather than silently + // resolving to different, plausible-looking entropy. + const message = err instanceof Error ? err.message : String(err); + throw new Error(`Mnemonic could not be derived into a wallet: ${message}`); + } + return { privateKeyBech32: w.paymentKey, address: w.address }; + } +} + +export class PrivateKeyKeySource implements KeySource { + private readonly privateKeyBech32: string; + private readonly address: string; + + // NOTE on a gap this constructor does NOT close: it validates that each + // argument is independently well-formed, but never checks that `address` + // is actually the address controlled by `privateKeyBech32`. A caller that + // supplies a syntactically valid key and a syntactically valid but + // UNRELATED address constructs and load()s successfully. + // + // This is a structural limit, not an oversight. Confirming the + // correspondence would need to derive a public key (and, for the Base + // addresses walletFromSeed produces, a stake credential too -- a Base + // address is not a function of the payment key alone) from + // privateKeyBech32 and compare it against the address's embedded + // credential, which needs the Cardano Multiplatform Library. This module + // deliberately imports ONLY `@lucid-evolution/wallet` (see + // boundary.test.ts and the package-level import allowlist), and that + // package's only Provider-free export is walletFromSeed, which takes a + // MNEMONIC, not a raw key. Its one function that accepts a raw private + // key, makeWalletFromPrivateKey, requires a live Provider -- exactly the + // network capability this module exists to not have. There is no way to + // perform this check without either an out-of-scope import or a network + // round-trip, so it is left undone, deliberately, rather than partially + // done via an import this module should not have. + // + // What this gap does NOT do: it does not let a mismatch pass unnoticed + // forever, and it fails in a bounded direction. Signing itself does not + // need the address (raw Ed25519 signing over a transaction body needs + // only the private key), so a mismatch does not corrupt the signature -- + // it produces a witness for a DIFFERENT credential than the one the + // transaction's inputs actually require, which is exactly the shape of + // failure a UTxO chain is built to reject at submission. Separately, and + // verified by reading the already-implemented policy.ts: `address` is + // also the value fed into policy.ts's own-address change detection, which + // only ever uses it to decide what does NOT count as outflow. A wrong + // `address` there cannot make policy.ts treat a foreign payment as safe + // change -- at worst it fails to recognise real change as change, which + // policy.ts's own documentation already establishes as its deliberate + // bias (over-refuse, never under-refuse). Both consequences are pinned as + // tests below rather than left as assumptions. + constructor(privateKeyBech32: string, address: string) { + // Trimmed for the same reason MnemonicKeySource tolerates surrounding + // whitespace: a bech32 string has no meaningful leading/trailing + // whitespace, so trimming is pure UX (copy-paste noise) with no + // ambiguity risk. Validate AND store the trimmed value, not the + // original -- storing the untrimmed original after validating the + // trimmed one is precisely the bug policy.ts's knownAddresses() had + // (validated the trimmed form, kept the padded one, which then never + // matched anything downstream by exact string equality). Both fields + // here feed later exact-string comparisons too (this address flows into + // policy.ts's ownAddresses), so the same failure shape applies. + const trimmedKey = privateKeyBech32.trim(); + const trimmedAddress = address.trim(); + if (!/^ed25519e?_sk1[0-9a-z]+$/.test(trimmedKey)) { + throw new Error('Invalid private key: expected a bech32 ed25519_sk / ed25519e_sk value.'); + } + if (!/^addr1[0-9a-z]+$/.test(trimmedAddress)) { + throw new Error('Invalid address: expected a bech32 addr1 value.'); + } + this.privateKeyBech32 = trimmedKey; + this.address = trimmedAddress; + } + + describe(): string { + return 'raw private key'; + } + + load(): SigningIdentity { + return { privateKeyBech32: this.privateKeyBech32, address: this.address }; + } +} diff --git a/packages/signer/test/keysource.test.ts b/packages/signer/test/keysource.test.ts new file mode 100644 index 0000000..3529883 --- /dev/null +++ b/packages/signer/test/keysource.test.ts @@ -0,0 +1,371 @@ +import { describe, test } from 'node:test'; +import assert from 'node:assert/strict'; +import { MnemonicKeySource, PrivateKeyKeySource } from '../src/keysource.ts'; + +// Throwaway test mnemonic. Controls no funds. Never use outside tests. +const TEST_MNEMONIC = + 'test walk nut penalty hip pave soap entry language right filter choice'; + +// A second throwaway fixture at the OTHER end of BIP39's valid word-count +// range (24, vs TEST_MNEMONIC's 12). Every other test in this file only ever +// exercises a 12-word mnemonic, which leaves VALID_WORD_COUNTS's other four +// entries (15, 18, 21, 24) completely unexercised as *accepted* input — a +// mutation dropping all of them but 12 would pass every other test in this +// file. Deterministically generated from fixed, obviously-fake entropy +// (bytes 0x00..0x1f) rather than randomly, so it is reproducible from that +// description alone. Controls no funds. Never use outside tests. +const TEST_MNEMONIC_24 = + 'abandon amount liar amount expire adjust cage candy arch gather drum bullet absurd math era live bid rhythm alien crouch range attend journey unaware'; + +describe('MnemonicKeySource', () => { + test('derives a bech32 private key and an addr1 address', () => { + const id = new MnemonicKeySource(TEST_MNEMONIC).load(); + assert.match(id.privateKeyBech32, /^ed25519e?_sk1/); + assert.match(id.address, /^addr1/); + }); + + test('derivation is deterministic', () => { + const a = new MnemonicKeySource(TEST_MNEMONIC).load(); + const b = new MnemonicKeySource(TEST_MNEMONIC).load(); + assert.equal(a.address, b.address); + assert.equal(a.privateKeyBech32, b.privateKeyBech32); + }); + + test('a different account index yields a different address', () => { + const a = new MnemonicKeySource(TEST_MNEMONIC, 0).load(); + const b = new MnemonicKeySource(TEST_MNEMONIC, 1).load(); + assert.notEqual(a.address, b.address); + }); + + test('tolerates surrounding whitespace and irregular spacing', () => { + const messy = ` ${TEST_MNEMONIC.split(' ').join(' ')} \n`; + assert.equal(new MnemonicKeySource(messy).load().address, new MnemonicKeySource(TEST_MNEMONIC).load().address); + }); + + test('rejects a wrong word count', () => { + assert.throws(() => new MnemonicKeySource('one two three'), /12, 15, 18, 21 or 24 words/); + }); + + test('describe() never reveals the mnemonic', () => { + const d = new MnemonicKeySource(TEST_MNEMONIC).describe(); + assert.ok(!d.includes('walk'), 'describe() must not leak key material'); + assert.match(d, /mnemonic/i); + }); +}); + +describe('PrivateKeyKeySource', () => { + test('returns the key and address it was constructed with', () => { + const derived = new MnemonicKeySource(TEST_MNEMONIC).load(); + const id = new PrivateKeyKeySource(derived.privateKeyBech32, derived.address).load(); + assert.equal(id.privateKeyBech32, derived.privateKeyBech32); + assert.equal(id.address, derived.address); + }); + + test('rejects a key that is not bech32 ed25519', () => { + assert.throws(() => new PrivateKeyKeySource('nonsense', 'addr1abc'), /private key/i); + }); + + test('rejects an address that is not addr1', () => { + const derived = new MnemonicKeySource(TEST_MNEMONIC).load(); + assert.throws(() => new PrivateKeyKeySource(derived.privateKeyBech32, 'nope'), /address/i); + }); + + test('describe() never reveals the key', () => { + const derived = new MnemonicKeySource(TEST_MNEMONIC).load(); + const d = new PrivateKeyKeySource(derived.privateKeyBech32, derived.address).describe(); + assert.ok(!d.includes(derived.privateKeyBech32), 'describe() must not leak key material'); + }); +}); + +// --- Edge-case pass (beyond the brief) -------------------------------------- +// +// The brief's 10 tests are a floor. Everything below hunts specifically for +// the failure shape the last three tasks each shipped once: a defect that +// produces a plausible-looking success instead of an error. For this module +// that means: does any input ever derive a WRONG BUT VALID-LOOKING address or +// key, rather than either the correct one or a loud failure? + +describe('MnemonicKeySource — case is not part of BIP39, so normalising it must be safe', () => { + // bip39's wordlist lookup (`wordlist.indexOf(word)`) is exact and + // case-sensitive — confirmed directly against this workspace's installed + // bip39 before writing this fix: an uppercase or mixed-case rendering of + // TEST_MNEMONIC throws 'Invalid mnemonic' with NO normalisation applied. + // That is a safe failure (loud, no derivation) but an unhelpful one, since + // casing carries no BIP39 meaning — a word is identified purely by which of + // the 2048 fixed wordlist entries it matches, never by how it happens to be + // capitalised, so no two distinct valid mnemonics differ only by case. + // Lowercasing before derivation therefore cannot make this module derive a + // DIFFERENT wallet than the one the correctly-cased mnemonic would — pinned + // below as deriving the SAME address, not merely "some" address. + const CANONICAL = new MnemonicKeySource(TEST_MNEMONIC).load(); + + test('an uppercase mnemonic derives the identical address and key as the canonical lowercase form', () => { + const upper = new MnemonicKeySource(TEST_MNEMONIC.toUpperCase()).load(); + assert.equal(upper.address, CANONICAL.address); + assert.equal(upper.privateKeyBech32, CANONICAL.privateKeyBech32); + }); + + test('a mixed-case mnemonic derives the identical address and key as the canonical lowercase form', () => { + const mixedWords = TEST_MNEMONIC.split(' ').map((w, i) => (i % 2 === 0 ? w.toUpperCase() : w)); + const mixed = new MnemonicKeySource(mixedWords.join(' ')).load(); + assert.equal(mixed.address, CANONICAL.address); + assert.equal(mixed.privateKeyBech32, CANONICAL.privateKeyBech32); + }); +}); + +describe('MnemonicKeySource — a wrong-but-plausible mnemonic', () => { + // Right word count (12), every word individually present in the BIP39 + // wordlist, but the checksum encoded in the final word's spare bits does + // not match the entropy in the first eleven words. Constructed by + // replacing TEST_MNEMONIC's FIRST word only: the checksum bits live in the + // last word, so tampering with any of the other eleven changes the entropy + // while (for a non-adversarially-chosen replacement) almost certainly + // failing the checksum — confirmed directly: bip39's own validateMnemonic() + // rejects this exact string. Throwaway, controls nothing; it does not even + // pass standard BIP39 validation, so no compliant wallet would derive from + // it at all. + const BAD_CHECKSUM_MNEMONIC = + 'bulk walk nut penalty hip pave soap entry language right filter choice'; + + test('a right-word-count, invalid-checksum mnemonic does not throw at construction time', () => { + // Construction only checks word COUNT. Documents that the checksum + // problem is invisible until load() is actually called. + assert.doesNotThrow(() => new MnemonicKeySource(BAD_CHECKSUM_MNEMONIC)); + }); + + test('...but throws at load() time, with a checksum-specific message', () => { + const source = new MnemonicKeySource(BAD_CHECKSUM_MNEMONIC); + assert.throws(() => source.load(), /checksum/i); + }); + + test('the checksum failure message reveals no word from the mnemonic', () => { + const source = new MnemonicKeySource(BAD_CHECKSUM_MNEMONIC); + try { + source.load(); + assert.fail('expected load() to throw'); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + for (const word of BAD_CHECKSUM_MNEMONIC.split(' ')) { + assert.ok(!message.includes(word), `error message must not contain the word "${word}"`); + } + } + }); + + test('a mnemonic of real English words not all found in the BIP39 wordlist also throws, not derives', () => { + // Distinct failure path from a checksum mismatch: this fails the + // wordlist lookup itself (some of these words are not among the 2048 + // BIP39 entries), which is arguably the single most likely real mistake + // — fat-fingering or misremembering a word — a user actually makes. + const notAllInWordlist = 'apple banana cherry dragon eagle falcon guitar hammer island jungle kitten lantern'; + assert.throws(() => new MnemonicKeySource(notAllInWordlist).load(), /invalid mnemonic/i); + }); +}); + +describe('MnemonicKeySource — invisible Unicode characters between words', () => { + // This exact class of bug hit policy.ts twice: .trim() and split(/\s+/) do + // not treat every invisible character alike. NBSP (U+00A0) IS part of JS's + // \s and IS covered by ECMAScript's WhiteSpace production, so it is already + // handled correctly by .trim().split(/\s+/).join(' ') with no special + // casing — confirmed below, not assumed. ZERO WIDTH SPACE (U+200B) is + // Unicode category Cf (format), not Zs/WhiteSpace, so it is NOT matched by + // \s and survives normalisation glued onto whichever word it sits next to. + const NBSP = String.fromCharCode(0xa0); + const ZERO_WIDTH_SPACE = String.fromCharCode(0x200b); + const CANONICAL = new MnemonicKeySource(TEST_MNEMONIC).load(); + + test('a non-breaking space between every word normalises exactly like a regular space', () => { + const withNbsp = TEST_MNEMONIC.split(' ').join(NBSP); + const derived = new MnemonicKeySource(withNbsp).load(); + assert.equal(derived.address, CANONICAL.address); + assert.equal(derived.privateKeyBech32, CANONICAL.privateKeyBech32); + }); + + test('a ZERO WIDTH SPACE glued onto a word is not silently ignored — it fails the wordlist match and throws', () => { + // Unlike policy.ts's address Set (an exact-equality lookup against + // caller-controlled strings, where a corrupted entry can silently miss a + // match and get misclassified instead of rejected), BIP39 wordlist + // lookup is an exact match against a small, fixed 2048-word list that + // contains no invisible characters. A corrupted word cannot coincidentally + // equal a different real word, so this fails loud instead of deriving a + // different, plausible-looking wallet — confirmed here, not assumed. + const words = TEST_MNEMONIC.split(' '); + const corrupted = [words[0] + ZERO_WIDTH_SPACE, ...words.slice(1)].join(' '); + const source = new MnemonicKeySource(corrupted); + assert.throws(() => source.load(), /invalid mnemonic/i); + }); + + test('a ZERO WIDTH SPACE used as the ONLY separator between two words fuses them, changing the word count and throwing at construction', () => { + const words = TEST_MNEMONIC.split(' '); + // Replace the space between word[0] and word[1] with a ZWSP: \s+ cannot + // split on it, so the two words fuse into a single non-whitespace token, + // dropping the apparent count from 12 to 11. + const fused = [words[0] + ZERO_WIDTH_SPACE + words[1], ...words.slice(2)].join(' '); + assert.throws(() => new MnemonicKeySource(fused), /got 11\./); + }); +}); + +describe('MnemonicKeySource — the empty-string-split gotcha (same shape as policy.ts\'s knownAddresses bug)', () => { + // ''.split(' ') yields [''] (length 1), not [] (length 0). Both before and + // after accounting for this, empty/blank input is correctly REJECTED + // either way — this only pins that the reported count is accurate (0, not + // the misleading 1), not a change in whether it fails. + test('an empty string is reported as 0 words, not the misleading 1 a naive split would report', () => { + assert.throws(() => new MnemonicKeySource(''), /got 0\./); + }); + + test('a whitespace-only string is also reported as 0 words', () => { + assert.throws(() => new MnemonicKeySource(' \n\t '), /got 0\./); + }); +}); + +describe('MnemonicKeySource — word-count breadth beyond 12', () => { + // Every test above this one exercises only a 12-word mnemonic as VALID + // input (12 also appears as an invalid COUNT case via 3/11/0-word inputs, + // but never as one of the other three accepted lengths). A mutation that + // narrowed VALID_WORD_COUNTS to just [12] would pass every test above this + // one. TEST_MNEMONIC_24 closes that gap at the opposite end of the range. + test('a valid 24-word mnemonic derives a bech32 private key and an addr1 address', () => { + const id = new MnemonicKeySource(TEST_MNEMONIC_24).load(); + assert.match(id.privateKeyBech32, /^ed25519e?_sk1/); + assert.match(id.address, /^addr1/); + }); + + test('the 24-word mnemonic is deterministic, same as the 12-word case', () => { + const a = new MnemonicKeySource(TEST_MNEMONIC_24).load(); + const b = new MnemonicKeySource(TEST_MNEMONIC_24).load(); + assert.equal(a.address, b.address); + assert.equal(a.privateKeyBech32, b.privateKeyBech32); + }); +}); + +describe('load() is idempotent — calling it twice on the SAME instance returns consistent results', () => { + test('MnemonicKeySource.load() called twice on one instance agrees with itself', () => { + const source = new MnemonicKeySource(TEST_MNEMONIC, 0); + const first = source.load(); + const second = source.load(); + assert.deepEqual(first, second); + }); + + test('PrivateKeyKeySource.load() called twice on one instance agrees with itself', () => { + const derived = new MnemonicKeySource(TEST_MNEMONIC).load(); + const source = new PrivateKeyKeySource(derived.privateKeyBech32, derived.address); + const first = source.load(); + const second = source.load(); + assert.deepEqual(first, second); + }); +}); + +describe('PrivateKeyKeySource — whitespace tolerance mirrors MnemonicKeySource, without weakening validation', () => { + const derived = new MnemonicKeySource(TEST_MNEMONIC).load(); + + test('tolerates surrounding whitespace on both the key and the address, and stores the trimmed form', () => { + // Regression guard for the exact shape of policy.ts's knownAddresses() + // bug: validating the trimmed value but STORING the original padded one + // would pass this constructor yet never match anything downstream by + // exact string equality. Asserting the loaded value equals the clean, + // untrimmed-source derivation (not just "does not throw") is what would + // catch that. + const padded = new PrivateKeyKeySource(` ${derived.privateKeyBech32} `, `\t${derived.address}\n`).load(); + assert.equal(padded.privateKeyBech32, derived.privateKeyBech32); + assert.equal(padded.address, derived.address); + }); + + test('a trailing ZERO WIDTH SPACE on the address is rejected, not silently accepted — trim() cannot strip it, and the regex is anchored', () => { + const ZERO_WIDTH_SPACE = String.fromCharCode(0x200b); + assert.throws( + () => new PrivateKeyKeySource(derived.privateKeyBech32, derived.address + ZERO_WIDTH_SPACE), + /address/i + ); + }); + + test('a trailing ZERO WIDTH SPACE on the private key is rejected, not silently accepted', () => { + const ZERO_WIDTH_SPACE = String.fromCharCode(0x200b); + assert.throws( + () => new PrivateKeyKeySource(derived.privateKeyBech32 + ZERO_WIDTH_SPACE, derived.address), + /private key/i + ); + }); +}); + +describe('PrivateKeyKeySource — validation boundaries a loose regex mutation could silently pass', () => { + const derived = new MnemonicKeySource(TEST_MNEMONIC).load(); + + test('accepts the non-extended ed25519_sk1 form, not just ed25519e_sk1', () => { + // walletFromSeed only ever produces the EXTENDED form (ed25519e_sk1...), + // so nothing above this test ever exercises the regex's `e?` — every + // prior key came from walletFromSeed. A power user importing a raw key + // generated by e.g. `cardano-cli address key-gen` gets a NON-extended + // key (ed25519_sk1...) instead. Constructed here by stripping the 'e' + // from a real derived key: this only needs to be shaped like a + // non-extended bech32 key for the regex under test, not a + // cryptographically valid one, since the constructor performs no + // cryptographic check (see the documented address/key gap above) — same + // "fake but correctly shaped" approach the brief itself uses for + // 'addr1abc'. + const nonExtended = derived.privateKeyBech32.replace(/^ed25519e_sk1/, 'ed25519_sk1'); + assert.doesNotThrow(() => new PrivateKeyKeySource(nonExtended, derived.address)); + }); + + test('rejects a testnet-shaped address (addr_test1...) even though it starts with "addr"', () => { + // Vector runs mainnet-only (see MnemonicKeySource, which hardcodes + // network: 'Mainnet'), so an address for any other network is wrong + // for this system, not merely differently formatted. A regex loosened + // to `/^addr/` (matching only the four literal letters, not `addr1` + // specifically) would let this through — a realistic operator mistake + // (pasting a testnet address into a mainnet-only signer config), not + // just a contrived string. + const testnetShaped = 'addr_test1qpu5vlrf4xkxv2qpwngf6cjhtw542ayty80v8dyr49rf5ewvxwdrt70qlcpeeagscasafhffqsxy36t90ldv06wqrk2qum8x5w'; + assert.throws(() => new PrivateKeyKeySource(derived.privateKeyBech32, testnetShaped), /address/i); + }); +}); + +describe('PrivateKeyKeySource — address/key correspondence is NOT verified (documented, deliberate gap)', () => { + // See the long comment on PrivateKeyKeySource in keysource.ts for the full + // reasoning. Short version: verifying this would need to derive a public + // key (and, for a Base address, a stake credential too — a Base address is + // not a function of the payment key alone) from the private key and + // compare it against the address's embedded credential, which needs the + // Cardano Multiplatform Library. This module deliberately imports ONLY + // @lucid-evolution/wallet, and that package's one function taking a raw + // private key (makeWalletFromPrivateKey) requires a live Provider — the + // network capability this module exists to not have. Pinned here so the + // gap is a visible, intentional decision, not a silent assumption. + test('a syntactically valid key and a syntactically valid but unrelated address construct and load() without error', () => { + const accountA = new MnemonicKeySource(TEST_MNEMONIC, 0).load(); + const accountB = new MnemonicKeySource(TEST_MNEMONIC, 1).load(); + // accountA's key paired with accountB's address: independently valid, + // mutually unrelated. Nothing in this module can detect that mismatch. + const mismatched = new PrivateKeyKeySource(accountA.privateKeyBech32, accountB.address).load(); + assert.equal(mismatched.privateKeyBech32, accountA.privateKeyBech32); + assert.equal(mismatched.address, accountB.address); + }); +}); + +describe('error messages never echo the rejected input, even when the input itself is invalid', () => { + // Distinct from the describe()-leak tests above: this pins that the + // CONSTRUCTOR's own validation errors never interpolate the value that was + // rejected, on the principle that a string purporting to be a private key + // should never be echoed back, valid or not. + test('the rejected private key value itself never appears in the constructor error message', () => { + const rejected = 'this-string-must-never-appear-in-the-thrown-message'; + try { + new PrivateKeyKeySource(rejected, 'addr1abc'); + assert.fail('expected constructor to throw'); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + assert.ok(!message.includes(rejected)); + } + }); + + test('the rejected address value itself never appears in the constructor error message', () => { + const derived = new MnemonicKeySource(TEST_MNEMONIC).load(); + const rejected = 'this-address-string-must-never-appear-in-the-thrown-message'; + try { + new PrivateKeyKeySource(derived.privateKeyBech32, rejected); + assert.fail('expected constructor to throw'); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + assert.ok(!message.includes(rejected)); + } + }); +}); From a3c8c69b54420e1e6ddb3f3384de325d92481a28 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:59:43 +0200 Subject: [PATCH 15/27] fix(signer): make key material unreachable at runtime, not just to the compiler TypeScript `private` is compile-time only, so JSON.stringify or console.log of a KeySource printed the mnemonic or signing key in full - contradicting this module's own "never via a log line" invariant, and one natural mistake away in a package that already logs diagnostics to stderr. Native #private fields plus toJSON/inspect overrides. Also pins the default account index, which no test previously required to be 0. Co-Authored-By: Claude Opus 5 --- packages/signer/src/keysource.ts | 112 ++++++++++++++++++------ packages/signer/test/keysource.test.ts | 114 +++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 25 deletions(-) diff --git a/packages/signer/src/keysource.ts b/packages/signer/src/keysource.ts index bdf4b39..365f54b 100644 --- a/packages/signer/src/keysource.ts +++ b/packages/signer/src/keysource.ts @@ -10,6 +10,22 @@ // be derived", "invalid private key"), never the value. Every catch block // below is commented with why the upstream message it surfaces is safe to // include verbatim. +// +// TypeScript `private` is a COMPILE-TIME check only -- at runtime it is an +// ordinary enumerable own property, fully visible to JSON.stringify, +// util.inspect, console.log (which goes through util.inspect), Object.keys, +// Object.getOwnPropertyNames, spread, and for...in. Verified by execution, +// not assumed: `JSON.stringify(new MnemonicKeySource(m))` printed the +// mnemonic in full under TS `private`. This package already writes +// diagnostics to stderr elsewhere (audit.ts, index.ts) and describe()'s own +// docstring below invites "for logs" usage -- one `console.log(keySource)` +// instead of `console.log(keySource.describe())` away from a real leak. Key +// material fields are therefore declared with native `#private` syntax +// (invisible to all of the above, confirmed by execution) rather than TS +// `private`, and both classes additionally override toJSON() and Node's +// util.inspect.custom symbol as defence in depth, so that even an +// accidental serialisation attempt produces describe()'s safe string +// instead of either a leak or an uninformative `{}`. import { walletFromSeed } from '@lucid-evolution/wallet'; export interface SigningIdentity { @@ -23,10 +39,19 @@ export interface KeySource { load(): SigningIdentity; } +// The well-known symbol Node's util.inspect (and therefore console.log, which +// calls it) looks for. Built via Symbol.for on its registered name rather +// than `import { inspect } from 'node:util'` so this module's import surface +// stays exactly `@lucid-evolution/wallet` -- confirmed by execution that +// `Symbol.for('nodejs.util.inspect.custom') === util.inspect.custom`. +const NODE_INSPECT_CUSTOM = Symbol.for('nodejs.util.inspect.custom'); + const VALID_WORD_COUNTS = [12, 15, 18, 21, 24]; export class MnemonicKeySource implements KeySource { - private readonly mnemonic: string; + // Native private field, not TS `private` -- see the module header. The + // mnemonic is the single most sensitive value in this whole package. + #mnemonic: string; constructor(mnemonic: string, private readonly accountIndex: number = 0) { // Whitespace normalisation: collapse any run of whitespace (including @@ -50,7 +75,7 @@ export class MnemonicKeySource implements KeySource { // mixed-case rendering of a valid mnemonic reproduces the exact // canonical lowercase string byte-for-byte, and therefore derives the // exact same address -- never a different, also-plausible one. - this.mnemonic = mnemonic.trim().split(/\s+/).join(' ').toLowerCase(); + this.#mnemonic = mnemonic.trim().split(/\s+/).join(' ').toLowerCase(); // `''.split(' ')` yields `['']` (length 1), not `[]` (length 0) -- the // same empty-string-split gotcha documented at length in policy.ts's @@ -58,7 +83,7 @@ export class MnemonicKeySource implements KeySource { // mnemonic is reported as "got 0 words", not the misleading "got 1". // Either way this throws and derives nothing; this only fixes what the // message says. - const words = this.mnemonic.length === 0 ? [] : this.mnemonic.split(' '); + const words = this.#mnemonic.length === 0 ? [] : this.#mnemonic.split(' '); if (!VALID_WORD_COUNTS.includes(words.length)) { throw new Error( `Invalid mnemonic: expected 12, 15, 18, 21 or 24 words, got ${words.length}.` @@ -88,13 +113,26 @@ export class MnemonicKeySource implements KeySource { return `mnemonic (account index ${this.accountIndex})`; } + // Defence in depth beyond the #private field above: even if some future + // caller (or library) reaches for JSON.stringify(keySource) or + // console.log(keySource) instead of keySource.describe()'s intended path, + // both now resolve to the same safe string rather than either leaking or + // silently producing an uninformative `{}`. + toJSON(): string { + return this.describe(); + } + + [NODE_INSPECT_CUSTOM](): string { + return this.describe(); + } + load(): SigningIdentity { // Vector runs with the --mainnet flag, so addresses are addr1... let w; try { - w = walletFromSeed(this.mnemonic, { network: 'Mainnet', accountIndex: this.accountIndex }); + w = walletFromSeed(this.#mnemonic, { network: 'Mainnet', accountIndex: this.accountIndex }); } catch (err) { - // Never interpolate `this.mnemonic` here. Read bip39's source + // Never interpolate `this.#mnemonic` here. Read bip39's source // (mnemonicToEntropy, which walletFromSeed calls directly) to confirm // this before relying on it: every throw site in that function uses // one of four FIXED constant strings -- 'Invalid mnemonic' (wrong @@ -113,8 +151,9 @@ export class MnemonicKeySource implements KeySource { } export class PrivateKeyKeySource implements KeySource { - private readonly privateKeyBech32: string; - private readonly address: string; + // Native private fields, not TS `private` -- see the module header. + #privateKeyBech32: string; + #address: string; // NOTE on a gap this constructor does NOT close: it validates that each // argument is independently well-formed, but never checks that `address` @@ -122,21 +161,34 @@ export class PrivateKeyKeySource implements KeySource { // supplies a syntactically valid key and a syntactically valid but // UNRELATED address constructs and load()s successfully. // - // This is a structural limit, not an oversight. Confirming the - // correspondence would need to derive a public key (and, for the Base - // addresses walletFromSeed produces, a stake credential too -- a Base - // address is not a function of the payment key alone) from - // privateKeyBech32 and compare it against the address's embedded - // credential, which needs the Cardano Multiplatform Library. This module - // deliberately imports ONLY `@lucid-evolution/wallet` (see - // boundary.test.ts and the package-level import allowlist), and that - // package's only Provider-free export is walletFromSeed, which takes a - // MNEMONIC, not a raw key. Its one function that accepts a raw private - // key, makeWalletFromPrivateKey, requires a live Provider -- exactly the - // network capability this module exists to not have. There is no way to - // perform this check without either an out-of-scope import or a network - // round-trip, so it is left undone, deliberately, rather than partially - // done via an import this module should not have. + // This is a deliberate scope boundary for THIS module, not a technical + // impossibility -- worth being precise about, since "structural limit" + // language here previously read as the latter and that is not accurate. + // The check itself is possible with no network I/O: the Cardano + // Multiplatform Library can derive a public key from a raw private key + // and hash it into a credential entirely offline. CML is not some + // unavailable dependency -- it is already a DIRECT dependency of this + // exact package (`@anastasia-labs/cardano-multiplatform-lib-nodejs` in + // package.json), it is not on boundary.test.ts's forbidden-import list, + // and decode.ts already imports it elsewhere in this same package. So CML + // is not the obstacle. The actual constraint is narrower and self- + // imposed: this module's task brief calls for importing ONLY + // `@lucid-evolution/wallet`, to keep this one file's dependency surface + // minimal and its intent legible at a glance -- and that package's only + // function accepting a raw private key, makeWalletFromPrivateKey, + // requires a live Provider (a genuine network capability, unlike CML), + // which really is off-limits here. Closing this gap is therefore in + // reach for a future, deliberate change (a CML import, with + // boundary.test.ts re-verified against it) -- it is left open FOR NOW + // because it is out of THIS task's scope, not because it cannot be done. + // + // For the Base addresses walletFromSeed produces specifically, closing + // this gap fully would also need the stake credential, not just the + // payment one -- a Base address is not a function of the payment key + // alone, and this constructor never receives a stake key at all. A CML- + // based check could still verify the payment-credential half, which + // would already catch the realistic mistake (an unrelated key/address + // pair) even without verifying the stake half. // // What this gap does NOT do: it does not let a mismatch pass unnoticed // forever, and it fails in a bounded direction. Signing itself does not @@ -172,15 +224,25 @@ export class PrivateKeyKeySource implements KeySource { if (!/^addr1[0-9a-z]+$/.test(trimmedAddress)) { throw new Error('Invalid address: expected a bech32 addr1 value.'); } - this.privateKeyBech32 = trimmedKey; - this.address = trimmedAddress; + this.#privateKeyBech32 = trimmedKey; + this.#address = trimmedAddress; } describe(): string { return 'raw private key'; } + // Defence in depth beyond the #private fields above -- see the matching + // comment on MnemonicKeySource. + toJSON(): string { + return this.describe(); + } + + [NODE_INSPECT_CUSTOM](): string { + return this.describe(); + } + load(): SigningIdentity { - return { privateKeyBech32: this.privateKeyBech32, address: this.address }; + return { privateKeyBech32: this.#privateKeyBech32, address: this.#address }; } } diff --git a/packages/signer/test/keysource.test.ts b/packages/signer/test/keysource.test.ts index 3529883..c6d0da0 100644 --- a/packages/signer/test/keysource.test.ts +++ b/packages/signer/test/keysource.test.ts @@ -1,5 +1,6 @@ import { describe, test } from 'node:test'; import assert from 'node:assert/strict'; +import util from 'node:util'; import { MnemonicKeySource, PrivateKeyKeySource } from '../src/keysource.ts'; // Throwaway test mnemonic. Controls no funds. Never use outside tests. @@ -369,3 +370,116 @@ describe('error messages never echo the rejected input, even when the input itse } }); }); + +// --- Coordinator review pass ------------------------------------------------ +// +// Independent review found one real leak the tests above did not cover, and +// one unpinned default. Both reproduced first against the pre-fix code +// (confirmed RED — see signer-task-5-report.md's addendum for the transcript) +// before the fix landed. + +describe('key material is unreachable at RUNTIME, not just hidden from the compiler', () => { + // TypeScript `private` erases at compile time only. At runtime an instance + // built with plain `private readonly mnemonic: string` is an ordinary + // object with an ordinary enumerable `mnemonic` property — JSON.stringify, + // util.inspect (and therefore console.log, which calls it internally), + // Object.keys, Object.getOwnPropertyNames, spread, and for...in all see it. + // Reproduced directly before writing this fix: JSON.stringify() on the + // pre-fix class printed the mnemonic in full. This package already writes + // diagnostics to stderr elsewhere (audit.ts, index.ts), so this is one + // `console.log(keySource)` away from a real leak, not a theoretical one. + const mnemonicSource = new MnemonicKeySource(TEST_MNEMONIC, 2); + const derived = new MnemonicKeySource(TEST_MNEMONIC).load(); + const privateKeySource = new PrivateKeyKeySource(derived.privateKeyBech32, derived.address); + + // TEST_MNEMONIC's words, checked individually: a substring check for the + // whole phrase would miss a leak that reformats or partially reproduces it + // (e.g. an inspect call that line-wraps between words). + const MNEMONIC_WORDS = TEST_MNEMONIC.split(' '); + + function assertNoLeak(rendered: string, label: string) { + for (const word of MNEMONIC_WORDS) { + assert.ok(!rendered.includes(word), `${label} must not contain the mnemonic word "${word}"`); + } + assert.ok(!rendered.includes(derived.privateKeyBech32), `${label} must not contain the private key`); + } + + test('JSON.stringify(MnemonicKeySource instance) leaks nothing', () => { + assertNoLeak(JSON.stringify(mnemonicSource), 'JSON.stringify'); + }); + + test('util.inspect(MnemonicKeySource instance) leaks nothing (covers console.log too, which calls util.inspect internally)', () => { + assertNoLeak(util.inspect(mnemonicSource), 'util.inspect'); + }); + + test('Object.keys(MnemonicKeySource instance) no longer exposes the "mnemonic" property name, and accountIndex (not secret) remains', () => { + // Object.keys returns property NAMES, which for the secret field are + // never themselves derived from the mnemonic's VALUE — the meaningful + // assertion is that the #private field is not enumerable at all. + // accountIndex is explicitly not secret and is allowed to stay visible. + assert.deepEqual(Object.keys(mnemonicSource), ['accountIndex']); + }); + + test('Object.values(MnemonicKeySource instance) leaks nothing (the value-exposure half Object.keys alone does not test)', () => { + assertNoLeak(JSON.stringify(Object.values(mnemonicSource)), 'Object.values'); + }); + + test('JSON.stringify(PrivateKeyKeySource instance) leaks nothing', () => { + assertNoLeak(JSON.stringify(privateKeySource), 'JSON.stringify'); + }); + + test('util.inspect(PrivateKeyKeySource instance) leaks nothing (covers console.log too)', () => { + assertNoLeak(util.inspect(privateKeySource), 'util.inspect'); + }); + + test('Object.keys(PrivateKeyKeySource instance) exposes no property names at all — both fields are secret', () => { + assert.deepEqual(Object.keys(privateKeySource), []); + }); + + test('Object.values(PrivateKeyKeySource instance) leaks nothing', () => { + assertNoLeak(JSON.stringify(Object.values(privateKeySource)), 'Object.values'); + }); + + test('JSON.stringify still produces something useful (describe()\'s string), not an uninformative {}', () => { + // Defence-in-depth check: toJSON()/inspect.custom exist so an accidental + // serialisation attempt is still legible, not merely safe. Confirms the + // override actually engages rather than JSON.stringify silently falling + // back to `{}` because every enumerable property happened to be gone. + assert.equal(JSON.stringify(mnemonicSource), JSON.stringify(mnemonicSource.describe())); + assert.equal(JSON.stringify(privateKeySource), JSON.stringify(privateKeySource.describe())); + }); +}); + +describe('MnemonicKeySource — the default account index is pinned to 0', () => { + // keysource.test.ts previously only ever compared an EXPLICIT 0 against an + // EXPLICIT 1 ("a different account index yields a different address"); + // nothing tied the OMITTED-argument default itself to 0. A silent + // regression changing the default would derive a different, validly + // formatted wallet for every caller that omits the argument — and since + // policy.ts matches change outputs by exact address, that would make + // every real transaction look like a full outflow and get refused. + test('a default-constructed instance derives the identical address as an explicit accountIndex: 0', () => { + const defaulted = new MnemonicKeySource(TEST_MNEMONIC).load(); + const explicit = new MnemonicKeySource(TEST_MNEMONIC, 0).load(); + assert.equal(defaulted.address, explicit.address); + assert.equal(defaulted.privateKeyBech32, explicit.privateKeyBech32); + }); +}); + +describe('MnemonicKeySource.load() — the error-wrap prefix is pinned', () => { + // Removing the try/catch around walletFromSeed entirely still lets the + // underlying bip39 error ("Invalid mnemonic checksum") through unwrapped, + // which satisfied every /checksum/i-style assertion above without the + // wrap actually being present. This pins the wrap itself. + const BAD_CHECKSUM_MNEMONIC = + 'bulk walk nut penalty hip pave soap entry language right filter choice'; + + test('load() failure is prefixed with the source-attributing wrap message', () => { + // assert.throws matches against error.toString() ('Error: '), + // not error.message directly, hence no leading ^ anchor here. + assert.throws( + () => new MnemonicKeySource(BAD_CHECKSUM_MNEMONIC).load(), + /Mnemonic could not be derived into a wallet: Invalid mnemonic checksum/ + ); + }); +}); From 556c504bf8a5a84bfcfe733d8808e13cd60f092d Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:20:37 +0200 Subject: [PATCH 16/27] feat(signer): sign with CML, no Lucid and no Provider Lucid's constructor requires a Provider, which would put a network object in the signer. The CML path was verified to produce an identical tx hash and byte-identical signed output. Preserves any pre-existing witnesses. --- packages/signer/src/sign.ts | 43 ++++++++++++++++++++++++ packages/signer/test/sign.test.ts | 56 +++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 packages/signer/src/sign.ts create mode 100644 packages/signer/test/sign.test.ts diff --git a/packages/signer/src/sign.ts b/packages/signer/src/sign.ts new file mode 100644 index 0000000..437290d --- /dev/null +++ b/packages/signer/src/sign.ts @@ -0,0 +1,43 @@ +// Sign with CML directly. No Lucid instance and no Provider — Lucid's +// constructor requires a Provider, which would put a network object inside the +// signer. This path was verified to produce a byte-identical result. +import * as CML from '@anastasia-labs/cardano-multiplatform-lib-nodejs'; + +export function signTransaction( + unsignedCborHex: string, + privateKeyBech32: string +): { signedCborHex: string; txHashHex: string } { + let tx: CML.Transaction; + try { + tx = CML.Transaction.from_cbor_hex(unsignedCborHex); + } catch (err) { + throw new Error( + `Transaction could not be decoded: ${err instanceof Error ? err.message : String(err)}` + ); + } + + let sk: CML.PrivateKey; + try { + sk = CML.PrivateKey.from_bech32(privateKeyBech32); + } catch { + // Never echo the key value into the message. + throw new Error('Invalid private key: could not parse the bech32 signing key.'); + } + + const body = tx.body(); + const txHash = CML.hash_transaction(body); + const signature = sk.sign(txHash.to_raw_bytes()); + + const witnessSet = tx.witness_set(); + const vkeys = CML.VkeywitnessList.new(); + const existing = witnessSet.vkeywitnesses(); + if (existing) { + for (let i = 0; i < existing.len(); i++) vkeys.add(existing.get(i)); + } + vkeys.add(CML.Vkeywitness.new(sk.to_public(), signature)); + witnessSet.set_vkeywitnesses(vkeys); + + const signed = CML.Transaction.new(body, witnessSet, true, tx.auxiliary_data()); + + return { signedCborHex: signed.to_cbor_hex(), txHashHex: txHash.to_hex() }; +} diff --git a/packages/signer/test/sign.test.ts b/packages/signer/test/sign.test.ts new file mode 100644 index 0000000..86eefb7 --- /dev/null +++ b/packages/signer/test/sign.test.ts @@ -0,0 +1,56 @@ +import { describe, test } from 'node:test'; +import assert from 'node:assert/strict'; +import * as CML from '@anastasia-labs/cardano-multiplatform-lib-nodejs'; +import { signTransaction } from '../src/sign.ts'; +import { MnemonicKeySource } from '../src/keysource.ts'; +import { decodeTransaction } from '../src/decode.ts'; + +const TEST_MNEMONIC = + 'test walk nut penalty hip pave soap entry language right filter choice'; + +const FIXTURE_CBOR = + '84a300d9010281825820e17a2ebab8eae3850f959290041b3bef3f3597584f8cf4a728e14777dddb8c3200018282581d71ab1aad52c4774e5da9f2c0fa1a4d07220a0bdd57ee3dce9be860dac61a002dc6c0825839013fd872a48d71872cf76528bb41dd2a5775e2c532c21662a47c6cebf44b6be5f33456f28523ed04d1aaad158541176425378d8ed8fce53f881a3b6a7221021a0002911fa0f5f6'; + +const KEY = new MnemonicKeySource(TEST_MNEMONIC).load().privateKeyBech32; + +describe('signTransaction', () => { + test('attaches exactly one vkey witness', () => { + const { signedCborHex } = signTransaction(FIXTURE_CBOR, KEY); + const ws = CML.Transaction.from_cbor_hex(signedCborHex).witness_set().vkeywitnesses(); + assert.ok(ws); + assert.equal(ws!.len(), 1); + }); + + test('does not alter the transaction body — hash is unchanged', () => { + const { signedCborHex, txHashHex } = signTransaction(FIXTURE_CBOR, KEY); + assert.equal(txHashHex, decodeTransaction(FIXTURE_CBOR).txHashHex, 'signing must not change the body'); + assert.equal(decodeTransaction(signedCborHex).txHashHex, txHashHex); + }); + + test('the signature verifies against the signing public key', () => { + const { signedCborHex, txHashHex } = signTransaction(FIXTURE_CBOR, KEY); + const w = CML.Transaction.from_cbor_hex(signedCborHex).witness_set().vkeywitnesses()!.get(0); + const hashBytes = CML.TransactionHash.from_hex(txHashHex).to_raw_bytes(); + assert.equal(w.vkey().verify(hashBytes, w.ed25519_signature()), true); + }); + + test('output is longer than input — witnesses were added', () => { + const { signedCborHex } = signTransaction(FIXTURE_CBOR, KEY); + assert.ok(signedCborHex.length > FIXTURE_CBOR.length); + }); + + test('signing is deterministic for the same key and transaction', () => { + assert.equal( + signTransaction(FIXTURE_CBOR, KEY).signedCborHex, + signTransaction(FIXTURE_CBOR, KEY).signedCborHex + ); + }); + + test('rejects undecodable CBOR rather than producing garbage', () => { + assert.throws(() => signTransaction('not-hex', KEY), /could not be decoded/i); + }); + + test('rejects an invalid private key', () => { + assert.throws(() => signTransaction(FIXTURE_CBOR, 'nonsense'), /private key/i); + }); +}); From c9afa965711115d4bf9cca1098dc1ea26342caca Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:21:03 +0200 Subject: [PATCH 17/27] fix(signer): reject signing transactions with unconsumed trailing bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit from_cbor_hex only requires a valid prefix, so appending bytes to a real transaction let sign.ts silently sign a shorter transaction than the caller supplied and return a confident result — a signature over different bytes than what was asked for, with nothing to reveal the discrepancy. sign.ts calls from_cbor_hex independently of decode.ts and does not inherit that module's consumption check (commit 5da4447) automatically; this closes the same gap here, with the identical case-insensitive comparison and reasoning. Also closes an untested claim: decode.ts's own suite documents but never pins that the case-insensitive comparison tolerates legitimate uppercase-hex input without a false positive. Pinned here for sign.ts. Edge-case pass beyond the brief's 7 tests, all reproduced against the unmodified implementation before any fix and mutation-verified after: - trailing bytes silently accepted (BUG, fixed above) - existing vkey witnesses are preserved and appended to, not replaced (already correct; mutation-tested by dropping the copy loop) - two different keys produce two independently-verifying witnesses, chained or independent (already correct) - determinism strengthened to compare raw signature bytes across a freshly re-derived key instance, not just the memoized one (already correct) - auxiliary data / metadata survives signing byte-for-byte, body hash unchanged (already correct) - no thrown error, on any path, ever echoes the private key value (already correct; mutation-tested by deliberately leaking it) --- packages/signer/src/sign.ts | 21 +++ packages/signer/test/sign.test.ts | 292 ++++++++++++++++++++++++++++++ 2 files changed, 313 insertions(+) diff --git a/packages/signer/src/sign.ts b/packages/signer/src/sign.ts index 437290d..68338d4 100644 --- a/packages/signer/src/sign.ts +++ b/packages/signer/src/sign.ts @@ -16,6 +16,27 @@ export function signTransaction( ); } + // from_cbor_hex only requires that a *prefix* of unsignedCborHex parse as a + // valid transaction — it does not check that parsing consumed the whole + // string. This is the same gap decode.ts closes (see its comment for the + // full reasoning) and the check here is deliberately identical. sign.ts + // calls from_cbor_hex independently and does NOT inherit decode.ts's guard + // automatically just because the two modules share a package: confirmed + // empirically that, without this check, signing FIXTURE_CBOR with + // 'deadbeef' appended did not throw — it silently signed the shorter, + // valid prefix and returned a confident, fully-formed signature, with the + // appended bytes dropped and no indication anything was wrong. That is + // worse here than in decode.ts: the caller would receive a cryptographic + // signature over DIFFERENT, FEWER bytes than it actually asked to sign, + // with nothing to reveal the discrepancy. Compared case-insensitively: + // valid uppercase-hex input round-trips through CML as lowercase, which + // would otherwise read as a false-positive mismatch. + if (tx.to_cbor_hex().toLowerCase() !== unsignedCborHex.toLowerCase()) { + throw new Error( + 'Transaction could not be decoded: input was not fully consumed (unrecognised trailing data)' + ); + } + let sk: CML.PrivateKey; try { sk = CML.PrivateKey.from_bech32(privateKeyBech32); diff --git a/packages/signer/test/sign.test.ts b/packages/signer/test/sign.test.ts index 86eefb7..00f7637 100644 --- a/packages/signer/test/sign.test.ts +++ b/packages/signer/test/sign.test.ts @@ -54,3 +54,295 @@ describe('signTransaction', () => { assert.throws(() => signTransaction(FIXTURE_CBOR, 'nonsense'), /private key/i); }); }); + +// --- Edge-case pass (beyond the brief) -------------------------------------- +// +// The brief's 7 tests are a floor. sign.ts calls CML.Transaction.from_cbor_hex +// independently of decode.ts — it does not automatically inherit decode.ts's +// trailing-bytes consumption guard just because that module happens to have +// one. Checked directly, not assumed. +describe('unconsumed input (trailing bytes) — sign.ts decodes independently of decode.ts', () => { + // CML's from_cbor_hex only requires that a *prefix* of the input parse as a + // valid transaction — decode.ts's own history (commit 5da4447) documents + // this exact gap and fixes it with a re-serialise consumption check. sign.ts + // is a SEPARATE call site that does not inherit that fix automatically. + // Reproduced empirically before writing this test (see the task report): + // against the brief's unmodified implementation, signing + // FIXTURE_CBOR + 'deadbeef' did NOT throw — it silently signed the 156-byte + // prefix and returned a confident, fully-formed result, with the appended + // 4 bytes dropped and no indication anything was wrong. That is precisely + // the failure this task warned about: the caller asked to sign one set of + // bytes and, with no error, received a signature over a DIFFERENT + // (shorter) set of bytes than it supplied. + test('rejects a transaction with trailing bytes appended, rather than silently signing a shorter prefix', () => { + assert.throws(() => signTransaction(FIXTURE_CBOR + 'deadbeef', KEY), /could not be decoded/i); + }); + + test('still accepts the clean fixture (no false positive on legitimate input)', () => { + assert.doesNotThrow(() => signTransaction(FIXTURE_CBOR, KEY)); + }); + + test('a single self-valid trailing byte is still caught, not just multi-byte garbage', () => { + // Mirrors decode.test.ts's own strongest version of this check: even one + // extra byte that would itself decode as something (rather than obvious + // noise) must not slip through. + assert.throws(() => signTransaction(FIXTURE_CBOR + '00', KEY), /could not be decoded/i); + }); + + // The consumption check compares tx.to_cbor_hex() against unsignedCborHex + // case-INSENSITIVELY, specifically because CML re-serialises hex as + // lowercase regardless of the input's casing — a strict comparison would + // reject perfectly legitimate uppercase-hex input as a false positive + // "trailing data" mismatch. Notably, decode.ts's OWN test suite documents + // this reasoning in a comment but never actually pins it with a test — this + // closes that same gap here, verified empirically first (see the task + // report) rather than assumed safe by analogy. + test('an uppercase-hex rendering of the same clean transaction is still accepted (no false positive from the case-insensitive consumption check)', () => { + const result = signTransaction(FIXTURE_CBOR.toUpperCase(), KEY); + assert.equal(result.txHashHex, decodeTransaction(FIXTURE_CBOR).txHashHex); + }); +}); + +// A second, independent signing identity — a different account index derives +// a different key pair from the same throwaway mnemonic (already proven +// distinct in keysource.test.ts: "a different account index yields a +// different address"). Used below wherever a test needs two DIFFERENT keys. +const KEY_2 = new MnemonicKeySource(TEST_MNEMONIC, 1).load().privateKeyBech32; + +/** + * Builds a variant of FIXTURE_CBOR that already carries one vkey witness from + * a throwaway, unrelated key — the same fixture shape decode.test.ts's own + * "still accepts a signed transaction" test uses. Returns the CBOR plus the + * pre-existing witness's public key (bech32) and raw signature bytes, so a + * test can confirm that EXACT witness — not merely "some" witness — survives. + */ +function fixtureWithOneWitness(): { + cborHex: string; + existingVkeyBech32: string; + existingSignatureHex: string; +} { + const unsigned = CML.Transaction.from_cbor_hex(FIXTURE_CBOR); + const body = unsigned.body(); + const hash = CML.hash_transaction(body); + const preexistingKey = CML.PrivateKey.generate_ed25519(); + const signature = preexistingKey.sign(hash.to_raw_bytes()); + + const vkeywitnesses = CML.VkeywitnessList.new(); + vkeywitnesses.add(CML.Vkeywitness.new(preexistingKey.to_public(), signature)); + const witnessSet = CML.TransactionWitnessSet.new(); + witnessSet.set_vkeywitnesses(vkeywitnesses); + + const withWitness = CML.Transaction.new(body, witnessSet, true, undefined); + return { + cborHex: withWitness.to_cbor_hex(), + existingVkeyBech32: preexistingKey.to_public().to_bech32(), + existingSignatureHex: Buffer.from(signature.to_raw_bytes()).toString('hex'), + }; +} + +describe('signing a transaction that already carries a vkey witness', () => { + // The brief's implementation copies existing witnesses into a fresh + // VkeywitnessList before appending the new one. This is the test that + // actually exercises that copy step — nothing in the brief's own 7 tests + // ever signs a transaction that already has a witness, so a version that + // dropped the copy (built the new list containing ONLY the freshly-added + // witness) would satisfy every brief test while silently discarding any + // other signer's contribution — fatal for anything requiring more than one + // signature. Verified below (mutation) that this test does in fact catch + // that exact regression. + test('the pre-existing witness survives, and the new one is appended rather than replacing it', () => { + const { cborHex, existingVkeyBech32, existingSignatureHex } = fixtureWithOneWitness(); + const { signedCborHex, txHashHex } = signTransaction(cborHex, KEY); + + const ws = CML.Transaction.from_cbor_hex(signedCborHex).witness_set().vkeywitnesses(); + assert.ok(ws); + assert.equal(ws!.len(), 2, 'expected the pre-existing witness PLUS the newly added one, not one replacing the other'); + + const witnesses = [ws!.get(0), ws!.get(1)]; + const vkeyBech32s = witnesses.map((w) => w.vkey().to_bech32()); + const signatureHexes = witnesses.map((w) => Buffer.from(w.ed25519_signature().to_raw_bytes()).toString('hex')); + + assert.ok(vkeyBech32s.includes(existingVkeyBech32), 'the pre-existing witness public key must still be present'); + assert.ok( + signatureHexes.includes(existingSignatureHex), + 'the pre-existing witness signature bytes must be byte-for-byte unchanged, not merely a witness for the same key' + ); + + const newSignerVkeyBech32 = CML.PrivateKey.from_bech32(KEY).to_public().to_bech32(); + assert.ok(vkeyBech32s.includes(newSignerVkeyBech32), 'the newly added witness must also be present'); + + // Both witnesses must independently verify against the (unchanged) tx hash. + const hashBytes = CML.TransactionHash.from_hex(txHashHex).to_raw_bytes(); + for (const w of witnesses) { + assert.equal(w.vkey().verify(hashBytes, w.ed25519_signature()), true); + } + }); + + test('signing a witnessed transaction still does not alter the body — hash matches the pre-witnessed fixture', () => { + const { cborHex } = fixtureWithOneWitness(); + const { txHashHex } = signTransaction(cborHex, KEY); + // Same underlying body as FIXTURE_CBOR (only the witness set differs), so + // the hash must match the plain fixture's hash too. + assert.equal(txHashHex, decodeTransaction(FIXTURE_CBOR).txHashHex); + }); +}); + +describe('signing with two different keys', () => { + test('produces two distinct witnesses when applied one after the other, and both verify', () => { + const firstPass = signTransaction(FIXTURE_CBOR, KEY); + const secondPass = signTransaction(firstPass.signedCborHex, KEY_2); + + const ws = CML.Transaction.from_cbor_hex(secondPass.signedCborHex).witness_set().vkeywitnesses(); + assert.ok(ws); + assert.equal(ws!.len(), 2, 'both signatures must be present — the second call must not replace the first'); + + const witnesses = [ws!.get(0), ws!.get(1)]; + const vkeyBech32s = witnesses.map((w) => w.vkey().to_bech32()); + assert.notEqual(vkeyBech32s[0], vkeyBech32s[1], 'the two witnesses must be for two DIFFERENT public keys'); + + const signatureHexes = witnesses.map((w) => Buffer.from(w.ed25519_signature().to_raw_bytes()).toString('hex')); + assert.notEqual(signatureHexes[0], signatureHexes[1], 'the two witnesses must carry two DIFFERENT signatures'); + + const hashBytes = CML.TransactionHash.from_hex(secondPass.txHashHex).to_raw_bytes(); + for (const w of witnesses) { + assert.equal(w.vkey().verify(hashBytes, w.ed25519_signature()), true, 'every witness must independently verify'); + } + + // Neither key's public key can verify the OTHER key's signature — confirms + // the two witnesses are not accidentally duplicates of one another. + assert.equal(witnesses[0].vkey().verify(hashBytes, witnesses[1].ed25519_signature()), false); + assert.equal(witnesses[1].vkey().verify(hashBytes, witnesses[0].ed25519_signature()), false); + }); + + test('signing the SAME transaction independently (not chained) with two different keys yields two different single-witness results', () => { + const signedByKey1 = signTransaction(FIXTURE_CBOR, KEY); + const signedByKey2 = signTransaction(FIXTURE_CBOR, KEY_2); + assert.notEqual(signedByKey1.signedCborHex, signedByKey2.signedCborHex); + // Both still sign the exact same, unaltered body. + assert.equal(signedByKey1.txHashHex, signedByKey2.txHashHex); + }); +}); + +describe('signing is deterministic — strengthened beyond the brief\'s single check', () => { + // The brief's own determinism test only compares the whole signedCborHex + // string across two calls using the SAME already-loaded KEY constant. This + // strengthens it two ways: (a) asserts on the witness's raw signature bytes + // specifically, the property that actually matters (ed25519 is a + // deterministic signature scheme per RFC 8032 — no random nonce, so the + // same key signing the same message must always produce the same + // signature), and (b) re-derives the key from the mnemonic a SECOND time + // (a fresh MnemonicKeySource / fresh load()) rather than reusing the one + // in-memory KEY constant, so a hypothetical bug that made determinism + // depend on JS object identity rather than the key's actual bytes would be + // caught. + test('two independently re-derived instances of the same key produce byte-identical signatures', () => { + const rederivedKey = new MnemonicKeySource(TEST_MNEMONIC).load().privateKeyBech32; + assert.equal(rederivedKey, KEY, 'sanity check: re-derivation must reproduce the same key bech32'); + + const a = signTransaction(FIXTURE_CBOR, KEY); + const b = signTransaction(FIXTURE_CBOR, rederivedKey); + + const wa = CML.Transaction.from_cbor_hex(a.signedCborHex).witness_set().vkeywitnesses()!.get(0); + const wb = CML.Transaction.from_cbor_hex(b.signedCborHex).witness_set().vkeywitnesses()!.get(0); + assert.equal( + Buffer.from(wa.ed25519_signature().to_raw_bytes()).toString('hex'), + Buffer.from(wb.ed25519_signature().to_raw_bytes()).toString('hex') + ); + assert.equal(a.signedCborHex, b.signedCborHex); + }); +}); + +describe('a transaction carrying auxiliary data / metadata', () => { + // Builds a variant of FIXTURE_CBOR with a real AuxiliaryData (metadata) + // section attached to the transaction, the same way the brief's own + // implementation passes `tx.auxiliary_data()` through into the reconstructed + // signed Transaction. Confirms that plumbing actually works end to end, + // rather than trusting the pass-through argument is wired correctly. + function fixtureWithMetadata(): { cborHex: string; auxDataCborHex: string } { + const unsigned = CML.Transaction.from_cbor_hex(FIXTURE_CBOR); + const body = unsigned.body(); + + const metadata = CML.Metadata.new(); + metadata.set(674n, CML.TransactionMetadatum.new_text('sign.ts edge-case pass')); + const auxData = CML.AuxiliaryData.new(); + auxData.add_metadata(metadata); + // Captured BEFORE auxData is passed into Transaction.new() below. + // wasm-bindgen gives Transaction.new() ownership of its AuxiliaryData + // argument (Rust move semantics leaking through the WASM boundary): + // reusing `auxData` itself after this call throws "null pointer passed + // to rust" (confirmed empirically — see the task report). This is a + // property of CML's bindings, not of sign.ts, which never reuses a + // getter-returned object after handing it to `.new()` for exactly this + // reason. A caller of THIS fixture function only ever sees the CBOR + // string, so it is unaffected either way. + const auxDataCborHex = auxData.to_cbor_hex(); + + const withAux = CML.Transaction.new(body, CML.TransactionWitnessSet.new(), true, auxData); + return { cborHex: withAux.to_cbor_hex(), auxDataCborHex }; + } + + test('auxiliary data survives signing byte-for-byte', () => { + const { cborHex, auxDataCborHex } = fixtureWithMetadata(); + const { signedCborHex } = signTransaction(cborHex, KEY); + + const signedTx = CML.Transaction.from_cbor_hex(signedCborHex); + const survivingAuxData = signedTx.auxiliary_data(); + assert.ok(survivingAuxData, 'auxiliary data must not be dropped when a witness is added'); + assert.equal(survivingAuxData!.to_cbor_hex(), auxDataCborHex); + + // Content-level check too, not just byte-equality of the CBOR blob. + const label = survivingAuxData!.metadata()!.get(674n); + assert.ok(label); + assert.equal(label!.as_text(), 'sign.ts edge-case pass'); + }); + + test('the body hash is unchanged by the presence of auxiliary data', () => { + const { cborHex } = fixtureWithMetadata(); + const { txHashHex } = signTransaction(cborHex, KEY); + // Same body as FIXTURE_CBOR — aux data lives alongside the body, not + // inside it, so the hash must be identical to the plain fixture's. + assert.equal(txHashHex, decodeTransaction(FIXTURE_CBOR).txHashHex); + }); + + test('the witness itself still verifies against the unchanged hash when auxiliary data is present', () => { + const { cborHex } = fixtureWithMetadata(); + const { signedCborHex, txHashHex } = signTransaction(cborHex, KEY); + const w = CML.Transaction.from_cbor_hex(signedCborHex).witness_set().vkeywitnesses()!.get(0); + const hashBytes = CML.TransactionHash.from_hex(txHashHex).to_raw_bytes(); + assert.equal(w.vkey().verify(hashBytes, w.ed25519_signature()), true); + }); +}); + +describe('key material is never emitted', () => { + // Task 5's keysource.ts had to fix an actual leak (TypeScript `private` is + // compile-time only). sign.ts never holds a KeySource instance and never + // constructs a class at all — privateKeyBech32 is a plain function + // parameter — but the discipline from that task (verify by execution, not + // by reading the code and assuming) is applied here too: every throw site + // in sign.ts is checked directly against the actual rejected key value. + test('an invalid private key value never appears in the thrown error message', () => { + const rejected = 'this-key-value-must-never-appear-in-the-thrown-message'; + try { + signTransaction(FIXTURE_CBOR, rejected); + assert.fail('expected signTransaction to throw'); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + assert.ok(!message.includes(rejected)); + } + }); + + test('a VALID private key never appears in any thrown error message either (decode failure path)', () => { + // Distinct code path from the test above: here the KEY is well-formed + // and never even reaches PrivateKey.from_bech32, because the CBOR is + // rejected first. Confirms the decode-failure branch's error message + // (built from CML's own err.message, not from any local variable) cannot + // somehow embed the key argument either. + try { + signTransaction('not-hex', KEY); + assert.fail('expected signTransaction to throw'); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + assert.ok(!message.includes(KEY)); + } + }); +}); From 79004b90f3462c33cb308f6842b8aa25a949c773 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:59:03 +0200 Subject: [PATCH 18/27] fix(signer): preserve the transaction's is_valid flag instead of forcing true is_valid lives on Transaction rather than TransactionBody, so hardcoding it was invisible to every hash-equality test - flipping it passed all 21. For a deliberately-invalid script transaction that silently rewrites the claim a node re-checks in phase 2. Also pins the indefinite-length CBOR round trip that the trailing-bytes guard depends on, which until now was asserted only in a comment. --- packages/signer/src/sign.ts | 8 +- packages/signer/test/sign.test.ts | 175 ++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 1 deletion(-) diff --git a/packages/signer/src/sign.ts b/packages/signer/src/sign.ts index 68338d4..aa5f5b8 100644 --- a/packages/signer/src/sign.ts +++ b/packages/signer/src/sign.ts @@ -58,7 +58,13 @@ export function signTransaction( vkeys.add(CML.Vkeywitness.new(sk.to_public(), signature)); witnessSet.set_vkeywitnesses(vkeys); - const signed = CML.Transaction.new(body, witnessSet, true, tx.auxiliary_data()); + // Threaded through from the input transaction, not hardcoded: is_valid is + // Cardano's collateral-forfeit flag (false marks a script transaction + // included specifically to consume collateral after a phase-2 validation + // failure). It lives on Transaction, not TransactionBody, so hardcoding it + // would be invisible to hash_transaction(body) and to every hash-equality + // check — the signer must not silently rewrite a claim a node re-checks. + const signed = CML.Transaction.new(body, witnessSet, tx.is_valid(), tx.auxiliary_data()); return { signedCborHex: signed.to_cbor_hex(), txHashHex: txHash.to_hex() }; } diff --git a/packages/signer/test/sign.test.ts b/packages/signer/test/sign.test.ts index 00f7637..b89a741 100644 --- a/packages/signer/test/sign.test.ts +++ b/packages/signer/test/sign.test.ts @@ -4,6 +4,14 @@ import * as CML from '@anastasia-labs/cardano-multiplatform-lib-nodejs'; import { signTransaction } from '../src/sign.ts'; import { MnemonicKeySource } from '../src/keysource.ts'; import { decodeTransaction } from '../src/decode.ts'; +// /tx is an explicitly allowed pure subpath (boundary.test.ts's own +// ALLOWLIST — /tx and /types are pure, /provider is network-capable). Used +// below to build the exact same indefinite-length CBOR constructor shape +// deriveNftAssetName() builds, via the same helper functions, rather than a +// hand-rolled reimplementation that could subtly diverge from the real +// encoding this ecosystem actually produces. Test-only import — sign.ts +// itself imports nothing beyond CML. +import { cborBytes, cborUint } from '@apexfusion/vector-mcp-shared/tx'; const TEST_MNEMONIC = 'test walk nut penalty hip pave soap entry language right filter choice'; @@ -103,6 +111,124 @@ describe('unconsumed input (trailing bytes) — sign.ts decodes independently of }); }); +describe('indefinite-length CBOR — the agent registry\'s D8 79 9F … FF constructor encoding', () => { + // packages/shared/src/tx.ts's deriveNftAssetName() documents that the + // agent registry's Conway-era Aiken validators require INDEFINITE-length + // CBOR for a Plutus constructor's field array (D8 79 9F … FF) — Lucid's + // Data.to instead emits definite-length arrays (D8 79 82 …), which those + // validators reject. Both sign.ts's consumption check (above) and + // decode.ts's equivalent assert in a comment that CML's parse-then- + // reserialise round trip preserves this exact encoding rather than + // silently canonicalising it to definite-length — which matters because if + // it did NOT, the consumption check would misread that harmless + // re-encoding as "unrecognised trailing data" and refuse to sign every + // transaction using this ecosystem's own real encoding. That claim was + // asserted only in prose until now; this fixture makes it an executable, + // permanent check. (decode.ts's own test suite has the exact same untested + // gap — out of scope for this task's file boundary; flagged in the report + // for a later task to close.) + // + // Built using the SAME helper functions (cborBytes, cborUint) that + // packages/shared/src/tx.ts's deriveNftAssetName() itself uses to build its + // OutputReference constructor, for maximum fidelity to the real encoding. + function indefiniteLengthConstructor(): Buffer { + // Same shape as deriveNftAssetName's outRefCbor: tag 121 (Plutus + // constructor index 0), indefinite-length array open, one bytestring + // field (reusing FIXTURE_CBOR's own input tx hash, so this is a + // realistic 32-byte field rather than arbitrary filler), one small uint + // field, indefinite-length array close. + const someHash32 = 'e17a2ebab8eae3850f959290041b3bef3f3597584f8cf4a728e14777dddb8c3'; + return Buffer.concat([ + Buffer.from([0xd8, 0x79, 0x9f]), + cborBytes(someHash32), + cborUint(0n), + Buffer.from([0xff]), + ]); + } + + test('sanity: the hand-rolled bytes are actually the D8 79 9F … FF shape under test', () => { + const hex = indefiniteLengthConstructor().toString('hex'); + assert.ok(hex.startsWith('d8799f'), 'must open with tag 121 + indefinite-length array'); + assert.ok(hex.endsWith('ff'), 'must close with the indefinite-length-array break byte'); + }); + + describe('embedded in the transaction BODY (an inline datum on an output)', () => { + // Body-embedded specifically because this is the placement where the + // encoding actually participates in the body hash: if CML's round trip + // ever DID mangle this encoding, this is the fixture that would show up + // as a changed hash, not just a rejected consumption check. + function fixtureWithBodyEmbeddedDatum(): string { + const unsigned = CML.Transaction.from_cbor_hex(FIXTURE_CBOR); + const body = unsigned.body(); + const datum = CML.PlutusData.from_cbor_hex(indefiniteLengthConstructor().toString('hex')); + const existingOutput = body.outputs().get(0); + const outputWithDatum = CML.TransactionOutput.new( + existingOutput.address(), + existingOutput.amount(), + CML.DatumOption.new_datum(datum), + undefined + ); + const outputs = CML.TransactionOutputList.new(); + outputs.add(outputWithDatum); + const newBody = CML.TransactionBody.new(body.inputs(), outputs, body.fee()); + const tx = CML.Transaction.new(newBody, CML.TransactionWitnessSet.new(), true, undefined); + return tx.to_cbor_hex(); + } + + test('signTransaction accepts it — no false-positive "unrecognised trailing data" rejection', () => { + const cborHex = fixtureWithBodyEmbeddedDatum(); + assert.doesNotThrow(() => signTransaction(cborHex, KEY)); + }); + + test('the body hash is unchanged by signing', () => { + const cborHex = fixtureWithBodyEmbeddedDatum(); + const { txHashHex } = signTransaction(cborHex, KEY); + const independentHash = CML.hash_transaction(CML.Transaction.from_cbor_hex(cborHex).body()).to_hex(); + assert.equal(txHashHex, independentHash); + }); + + test('the indefinite-length datum itself survives signing byte-for-byte, not just the surrounding transaction', () => { + const cborHex = fixtureWithBodyEmbeddedDatum(); + const { signedCborHex } = signTransaction(cborHex, KEY); + const survivingDatum = CML.Transaction.from_cbor_hex(signedCborHex).body().outputs().get(0).datum()!.as_datum()!; + assert.equal(survivingDatum.to_cbor_hex(), indefiniteLengthConstructor().toString('hex')); + }); + }); + + describe('embedded in the WITNESS SET (a plutus_datums entry)', () => { + // The other placement the coordinator's reviewer independently checked. + // Unlike the body-embedded case, the body hash can never be affected + // here (plutus_datums lives on TransactionWitnessSet, not + // TransactionBody) — this fixture instead exercises the OTHER risk: the + // consumption check compares the WHOLE transaction's re-serialised CBOR, + // witness set included, so a mangled round trip here would still cause a + // false-positive rejection even though it could never show up as a + // changed hash. + function fixtureWithWitnessEmbeddedDatum(): string { + const unsigned = CML.Transaction.from_cbor_hex(FIXTURE_CBOR); + const body = unsigned.body(); + const datum = CML.PlutusData.from_cbor_hex(indefiniteLengthConstructor().toString('hex')); + const witnessSet = CML.TransactionWitnessSet.new(); + const datums = CML.PlutusDataList.new(); + datums.add(datum); + witnessSet.set_plutus_datums(datums); + const tx = CML.Transaction.new(body, witnessSet, true, undefined); + return tx.to_cbor_hex(); + } + + test('signTransaction accepts it — no false-positive "unrecognised trailing data" rejection', () => { + const cborHex = fixtureWithWitnessEmbeddedDatum(); + assert.doesNotThrow(() => signTransaction(cborHex, KEY)); + }); + + test('the body hash is unchanged (same body as FIXTURE_CBOR — only the witness set differs)', () => { + const cborHex = fixtureWithWitnessEmbeddedDatum(); + const { txHashHex } = signTransaction(cborHex, KEY); + assert.equal(txHashHex, decodeTransaction(FIXTURE_CBOR).txHashHex); + }); + }); +}); + // A second, independent signing identity — a different account index derives // a different key pair from the same throwaway mnemonic (already proven // distinct in keysource.test.ts: "a different account index yields a @@ -313,6 +439,55 @@ describe('a transaction carrying auxiliary data / metadata', () => { }); }); +describe('the is_valid flag is threaded through, not hardcoded', () => { + // is_valid lives on CML.Transaction itself, not on TransactionBody — so it + // is INVISIBLE to every hash-equality check elsewhere in this file + // (hash_transaction only ever hashes the body). A version of sign.ts that + // hardcodes Transaction.new(body, witnessSet, true, ...) regardless of the + // INPUT transaction's own is_valid() would satisfy every other test in + // this file, including "does not alter the transaction body — hash is + // unchanged", because FIXTURE_CBOR's own is_valid happens to already be + // true. Confirmed by mutation (see the task report): flipping the + // hardcoded literal from true to false still passed all 21 pre-existing + // tests, with zero failures. + // + // is_valid: false is Cardano's collateral-forfeit encoding: a node sets it + // when a script transaction is included specifically to consume collateral + // after a phase-2 validation failure. If the signer silently rewrote this + // to true, a node re-executing phase 2 would find the claimed validity + // mismatched and reject the transaction outright — the signer would have + // silently rewritten a field of the transaction it was asked to sign. + // + // Confirmed directly (throwaway probe, see the task report) that CML can + // construct, serialise, and correctly round-trip an is_valid: false + // Transaction before relying on that here. + function fixtureWithIsValid(isValid: boolean): string { + const unsigned = CML.Transaction.from_cbor_hex(FIXTURE_CBOR); + const tx = CML.Transaction.new(unsigned.body(), unsigned.witness_set(), isValid, undefined); + return tx.to_cbor_hex(); + } + + test('an is_valid: false transaction is signed with is_valid still false, not silently flipped to true', () => { + const cborHex = fixtureWithIsValid(false); + const { signedCborHex } = signTransaction(cborHex, KEY); + const signedTx = CML.Transaction.from_cbor_hex(signedCborHex); + assert.equal(signedTx.is_valid(), false); + }); + + test('an is_valid: true transaction is signed with is_valid still true (pinned explicitly, not just incidentally true because the default fixture happens to be)', () => { + const cborHex = fixtureWithIsValid(true); + const { signedCborHex } = signTransaction(cborHex, KEY); + const signedTx = CML.Transaction.from_cbor_hex(signedCborHex); + assert.equal(signedTx.is_valid(), true); + }); + + test('is_valid: false does not affect the body hash — is_valid lives on Transaction, not TransactionBody', () => { + const cborHex = fixtureWithIsValid(false); + const { txHashHex } = signTransaction(cborHex, KEY); + assert.equal(txHashHex, decodeTransaction(FIXTURE_CBOR).txHashHex); + }); +}); + describe('key material is never emitted', () => { // Task 5's keysource.ts had to fix an actual leak (TypeScript `private` is // compile-time only). sign.ts never holds a KeySource instance and never From b0faac8c122e74f58ccec6ac774bcd2c7a9b8657 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:22:19 +0200 Subject: [PATCH 19/27] feat(signer): MCP server with get_address, decode, sign, get_spend_limits stdio transport only - no HTTP listener, no Provider, no outbound network. Wires together decode.ts/policy.ts/sign.ts/audit.ts/keysource.ts (Tasks 2-6) behind four tools: vector_signer_get_address, vector_signer_decode_transaction, vector_signer_sign, vector_signer_get_spend_limits. Two corrections needed beyond the brief's given code to reach its own stated "pass 9, fail 0" outcome, both verified necessary by reproducing the failure first: - packages/signer/tsup.config.ts: target bumped es2020 -> es2022. index.ts's top-level `await server.connect(...)` is ES2022 syntax; esbuild hard-errors ("Top-level await is not available in the configured target environment") under the package's original es2020 target rather than attempting an unsafe downlevel transform. Scoped to the signer package only - the builder never uses top-level await and its own tsup.config.ts is untouched. - tools.ts's decode_transaction response text: "(not signed)" and "would be signed" both contain the substring "signed", which collides with this same smoke test's own `!/signed/i.test(text)` check on every call, including the refused case. Reworded to "(read-only)" / "would be ALLOWED" - same meaning, no collision. Fixed the response text rather than loosening the test, which stays exactly as given. Brief's own smoke test (9 tests) passes verbatim. Workspace-wide: typecheck 0, test:unit 180/180 (no regressions), builder test:smoke still 26/23/0. The audit-append-failure handling required by the task (a produced signature must never be released if its audit record fails to write) is not yet in this commit - see the follow-up commit. --- .github/workflows/ci.yml | 5 + package.json | 1 + packages/signer/src/config.ts | 63 ++++++++ packages/signer/src/index.ts | 20 ++- packages/signer/src/tools.ts | 139 ++++++++++++++++++ .../signer/test/smoke/signer-tools.test.ts | 114 ++++++++++++++ packages/signer/tsup.config.ts | 11 +- 7 files changed, 350 insertions(+), 3 deletions(-) create mode 100644 packages/signer/src/config.ts create mode 100644 packages/signer/src/tools.ts create mode 100644 packages/signer/test/smoke/signer-tools.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8253182..d99c68b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,6 +43,11 @@ jobs: env: NODE_OPTIONS: --max-old-space-size=12288 + - name: Signer smoke test + run: npm run test:smoke:signer + env: + NODE_OPTIONS: --max-old-space-size=12288 + typecheck: runs-on: ubuntu-latest timeout-minutes: 15 diff --git a/package.json b/package.json index f8fcd80..0a1643f 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,7 @@ "test": "npm run test:unit", "test:unit": "node --import tsx --test packages/*/test/*.test.ts", "test:smoke": "npm run build && node --import tsx --test packages/builder/test/smoke/*.test.ts", + "test:smoke:signer": "npm run build && node --import tsx --test packages/signer/test/smoke/*.test.ts", "test:integration": "npm run build && node --import tsx --test packages/builder/test/integration/*.test.ts" }, "devDependencies": { diff --git a/packages/signer/src/config.ts b/packages/signer/src/config.ts new file mode 100644 index 0000000..b0974dd --- /dev/null +++ b/packages/signer/src/config.ts @@ -0,0 +1,63 @@ +// Signer configuration, entirely from the environment. No network settings +// exist here by design — the signer has no endpoints to configure. +import { readFileSync } from 'node:fs'; +import { resolve, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { MnemonicKeySource, PrivateKeyKeySource, type KeySource } from './keysource.js'; +import type { SpendLimits } from './policy.js'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); + +export interface SignerConfig { + keySource: KeySource; + limits: SpendLimits; + auditLogPath: string; +} + +function repoRootDefault(): string { + // From packages/signer/build/index.js, three levels up is the repo root. + return resolve(__dirname, '../../..'); +} + +export function loadConfig(env: NodeJS.ProcessEnv = process.env): SignerConfig { + const keySource = resolveKeySource(env); + + return { + keySource, + limits: { + perTxLovelace: BigInt(env.VECTOR_SIGNER_SPEND_LIMIT_PER_TX ?? '100000000'), + dailyLovelace: BigInt(env.VECTOR_SIGNER_SPEND_LIMIT_DAILY ?? '500000000'), + }, + auditLogPath: env.VECTOR_SIGNER_AUDIT_LOG_PATH + ? resolve(env.VECTOR_SIGNER_AUDIT_LOG_PATH) + : resolve(repoRootDefault(), 'vector-signer-audit-log.json'), + }; +} + +function resolveKeySource(env: NodeJS.ProcessEnv): KeySource { + const accountIndex = Number(env.VECTOR_SIGNER_ACCOUNT_INDEX ?? '0'); + + if (env.VECTOR_SIGNER_PRIVATE_KEY && env.VECTOR_SIGNER_ADDRESS) { + return new PrivateKeyKeySource(env.VECTOR_SIGNER_PRIVATE_KEY, env.VECTOR_SIGNER_ADDRESS); + } + if (env.VECTOR_SIGNER_MNEMONIC) { + return new MnemonicKeySource(env.VECTOR_SIGNER_MNEMONIC, accountIndex); + } + if (env.VECTOR_SIGNER_MNEMONIC_FILE) { + let contents: string; + try { + contents = readFileSync(env.VECTOR_SIGNER_MNEMONIC_FILE, 'utf8'); + } catch (err) { + // Reference the source, never the contents. + throw new Error( + `Could not read VECTOR_SIGNER_MNEMONIC_FILE (${env.VECTOR_SIGNER_MNEMONIC_FILE}): ${err instanceof Error ? err.message : String(err)}` + ); + } + return new MnemonicKeySource(contents, accountIndex); + } + + throw new Error( + 'No key material configured. Set one of: VECTOR_SIGNER_MNEMONIC_FILE (recommended), ' + + 'VECTOR_SIGNER_MNEMONIC, or VECTOR_SIGNER_PRIVATE_KEY together with VECTOR_SIGNER_ADDRESS.' + ); +} diff --git a/packages/signer/src/index.ts b/packages/signer/src/index.ts index 862da72..1840a75 100644 --- a/packages/signer/src/index.ts +++ b/packages/signer/src/index.ts @@ -1,3 +1,19 @@ // Local signing companion for the Vector MCP server. -// Speaks stdio only: no HTTP listener, no outbound network. See test/boundary.test.ts. -console.error('vector-mcp-signer: not yet implemented'); +// +// Speaks stdio only: no HTTP listener, no outbound network, no Provider. +// The signing key never leaves this process. See test/boundary.test.ts. +import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; +import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; +import { loadConfig } from './config.js'; +import { registerSignerTools } from './tools.js'; + +const config = loadConfig(); + +const server = new McpServer({ name: 'vector-mcp-signer', version: '0.1.0' }); +registerSignerTools(server, config); + +// stdout is the MCP channel — every diagnostic must go to stderr. +console.error(`vector-mcp-signer ready. Key source: ${config.keySource.describe()}`); +console.error(`Audit log: ${config.auditLogPath}`); + +await server.connect(new StdioServerTransport()); diff --git a/packages/signer/src/tools.ts b/packages/signer/src/tools.ts new file mode 100644 index 0000000..b9a5a84 --- /dev/null +++ b/packages/signer/src/tools.ts @@ -0,0 +1,139 @@ +import { z } from 'zod/v4'; +import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; +import { lovelaceToAda } from '@apexfusion/vector-mcp-shared/tx'; +import { decodeTransaction } from './decode.js'; +import { evaluate } from './policy.js'; +import { signTransaction } from './sign.js'; +import { AuditLog } from './audit.js'; +import type { SignerConfig } from './config.js'; + +const txCborShape = { txCbor: z.string().describe('Hex-encoded CBOR of an unsigned transaction') }; + +function text(body: string) { + return { content: [{ type: 'text' as const, text: body }] }; +} + +export function registerSignerTools(server: McpServer, config: SignerConfig): void { + const identity = config.keySource.load(); + const audit = new AuditLog(config.auditLogPath); + const ownAddresses = [identity.address]; + + server.tool( + 'vector_signer_get_address', + 'Get this signer\'s wallet address. Pass it to the builder as changeAddress when building a transaction.', + {}, + async () => text(`Signer address: ${identity.address}\nKey source: ${config.keySource.describe()}`) + ); + + server.tool( + 'vector_signer_decode_transaction', + 'Decode an unsigned transaction and report what it would move, without signing it. Use this to show a human what is about to be signed.', + txCborShape, + async ({ txCbor }) => { + try { + const tx = decodeTransaction(txCbor); + const decision = evaluate(tx, ownAddresses, config.limits, audit.committedTodayLovelace()); + const rows = tx.outputs + .map((o, i) => { + const mine = o.address === identity.address; + const assets = o.assets.length ? ` + ${o.assets.length} asset(s)` : ''; + return ` ${i}. ${mine ? '[change] ' : '[outgoing]'} ${lovelaceToAda(o.lovelace)} AP3X${assets}\n ${o.address}`; + }) + .join('\n'); + return text( + // "(read-only)", not "(not signed)": the latter still contains the + // substring "signed", which collides with this smoke test's own + // `!/signed/i.test(text)` check (verified empirically — see the + // task report). "would be ALLOWED" below is the same fix applied + // to the policy line, which otherwise would have read "would be + // signed" whenever a decoded transaction happened to be in-policy. + `# Transaction preview (read-only)\n\n` + + `Hash: ${tx.txHashHex}\nInputs: ${tx.inputCount}\nFee: ${lovelaceToAda(tx.fee)} AP3X\n` + + `Minted assets: ${tx.mintedAssetCount}\n\nOutputs:\n${rows || ' (none)'}\n\n` + + `**Net outflow: ${lovelaceToAda(decision.netOutflowLovelace)} AP3X** (outgoing outputs + fee; change excluded)\n` + + `Assets leaving: ${decision.assetMovements.length}\n\n` + + `Policy: ${decision.allowed ? 'would be ALLOWED' : `would be REFUSED - ${decision.reason}`}` + ); + } catch (err) { + return text(`Could not decode the transaction: ${err instanceof Error ? err.message : String(err)}`); + } + } + ); + + server.tool( + 'vector_signer_sign', + 'Decode, policy-check, and sign an unsigned transaction. Returns signed CBOR to submit via the builder. Refuses if the transaction violates spend policy.', + txCborShape, + async ({ txCbor }) => { + let tx; + try { + tx = decodeTransaction(txCbor); + } catch (err) { + // Fail closed: never sign bytes that were not fully understood. + return text(`REFUSED. ${err instanceof Error ? err.message : String(err)}`); + } + + const decision = evaluate(tx, ownAddresses, config.limits, audit.committedTodayLovelace()); + + if (!decision.allowed) { + audit.append({ + timestamp: new Date().toISOString(), + txHash: tx.txHashHex, + decision: 'refused', + netOutflowLovelace: decision.netOutflowLovelace.toString(), + reason: decision.reason, + assetMovements: decision.assetMovements.length, + }); + return text(`REFUSED. ${decision.reason}`); + } + + let signed; + try { + signed = signTransaction(txCbor, identity.privateKeyBech32); + } catch (err) { + return text(`REFUSED. Signing failed: ${err instanceof Error ? err.message : String(err)}`); + } + + audit.append({ + timestamp: new Date().toISOString(), + txHash: signed.txHashHex, + decision: 'signed', + netOutflowLovelace: decision.netOutflowLovelace.toString(), + assetMovements: decision.assetMovements.length, + }); + + return text( + `# Signed\n\nHash: ${signed.txHashHex}\n` + + `Net outflow: ${lovelaceToAda(decision.netOutflowLovelace)} AP3X\n\n` + + `Signed CBOR:\n\`\`\`\n${signed.signedCborHex}\n\`\`\`\n\n` + + `Submit it with the builder's vector_submit_transaction. Nothing has been submitted yet.` + ); + } + ); + + server.tool( + 'vector_signer_get_spend_limits', + 'Report this signer\'s spend limits, how much of today\'s budget is committed, and recent signing decisions.', + {}, + async () => { + const committed = audit.committedTodayLovelace(); + const remaining = config.limits.dailyLovelace - committed; + const recent = audit + .recent(5) + .map((e) => ` ${e.timestamp} ${e.decision.toUpperCase()} ${lovelaceToAda(BigInt(e.netOutflowLovelace))} AP3X ${e.txHash.slice(0, 16)}...${e.reason ? ` (${e.reason})` : ''}`) + .join('\n'); + return text( + `# Signer spend limits\n\n` + + `Per transaction: ${lovelaceToAda(config.limits.perTxLovelace)} AP3X\n` + + `Daily: ${lovelaceToAda(config.limits.dailyLovelace)} AP3X\n` + + `Committed today: ${lovelaceToAda(committed)} AP3X\n` + + `Remaining today: ${lovelaceToAda(remaining > 0n ? remaining : 0n)} AP3X\n\n` + + `Audit log: ${config.auditLogPath}\n\n` + + (recent ? `Recent decisions:\n${recent}` : 'No decisions recorded yet.') + + `\n\nNote: the daily total counts signed transactions at signing time. The signer has no ` + + `network access and cannot observe whether a submit later succeeded, so a failed submit ` + + `still counts against the budget.` + ); + } + ); +} diff --git a/packages/signer/test/smoke/signer-tools.test.ts b/packages/signer/test/smoke/signer-tools.test.ts new file mode 100644 index 0000000..42aabc7 --- /dev/null +++ b/packages/signer/test/smoke/signer-tools.test.ts @@ -0,0 +1,114 @@ +import { describe, test, before, after } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; + +const TEST_MNEMONIC = + 'test walk nut penalty hip pave soap entry language right filter choice'; + +const FIXTURE_CBOR = + '84a300d9010281825820e17a2ebab8eae3850f959290041b3bef3f3597584f8cf4a728e14777dddb8c3200018282581d71ab1aad52c4774e5da9f2c0fa1a4d07220a0bdd57ee3dce9be860dac61a002dc6c0825839013fd872a48d71872cf76528bb41dd2a5775e2c532c21662a47c6cebf44b6be5f33456f28523ed04d1aaad158541176425378d8ed8fce53f881a3b6a7221021a0002911fa0f5f6'; + +const EXPECTED_TOOLS = [ + 'vector_signer_decode_transaction', + 'vector_signer_get_address', + 'vector_signer_get_spend_limits', + 'vector_signer_sign', +]; + +let client: Client; +let dir: string; + +async function callText(name: string, args: Record = {}): Promise { + const r = await client.callTool({ name, arguments: args }); + const c = r.content as Array<{ type: string; text?: string }>; + return c?.[0]?.text ?? ''; +} + +before(async () => { + dir = mkdtempSync(join(tmpdir(), 'signer-smoke-')); + const mnemonicFile = join(dir, 'mnemonic.txt'); + writeFileSync(mnemonicFile, TEST_MNEMONIC, { mode: 0o600 }); + + const transport = new StdioClientTransport({ + command: 'node', + args: ['packages/signer/build/index.js'], + env: { + ...process.env, + VECTOR_SIGNER_MNEMONIC_FILE: mnemonicFile, + VECTOR_SIGNER_AUDIT_LOG_PATH: join(dir, 'audit.json'), + VECTOR_SIGNER_SPEND_LIMIT_PER_TX: '100000000', + VECTOR_SIGNER_SPEND_LIMIT_DAILY: '500000000', + }, + }); + client = new Client({ name: 'signer-smoke', version: '1.0.0' }); + await client.connect(transport); +}); + +after(async () => { + try { await client.close(); } catch {} + rmSync(dir, { recursive: true, force: true }); +}); + +describe('signer MCP server over stdio', () => { + test('exposes exactly the four expected tools', async () => { + const { tools } = await client.listTools(); + assert.deepStrictEqual(tools.map((t) => t.name).sort(), EXPECTED_TOOLS); + }); + + test('no tool accepts a mnemonic or private key parameter', async () => { + const { tools } = await client.listTools(); + for (const t of tools) { + const props = Object.keys((t.inputSchema as any).properties ?? {}); + for (const p of props) { + assert.ok(!/mnemonic|private_?key|seed/i.test(p), `${t.name} exposes key material as parameter "${p}"`); + } + } + }); + + test('get_address returns the derived addr1 address', async () => { + assert.match(await callText('vector_signer_get_address'), /addr1[0-9a-z]+/); + }); + + test('decode_transaction reports the net outflow without signing', async () => { + const text = await callText('vector_signer_decode_transaction', { txCbor: FIXTURE_CBOR }); + assert.match(text, /Net outflow/i); + assert.ok(!/signed/i.test(text), 'decode must not sign'); + }); + + // The fixture was built for a DIFFERENT wallet, so for this test key both of + // its outputs are foreign: net outflow is ~1000 AP3X, far over the 100 AP3X + // per-transaction limit. Refusal is the correct outcome and the security- + // relevant path, so assert it precisely rather than settling for "responded". + test('sign refuses a transaction whose net outflow exceeds the per-transaction limit', async () => { + const text = await callText('vector_signer_sign', { txCbor: FIXTURE_CBOR }); + assert.match(text, /REFUSED/); + assert.match(text, /per-transaction limit/i); + }); + + test('a refusal returns no signed CBOR', async () => { + const text = await callText('vector_signer_sign', { txCbor: FIXTURE_CBOR }); + assert.ok(!/Signed CBOR/i.test(text), 'a refused transaction must not return signed bytes'); + }); + + test('a refusal is recorded in the audit log but consumes no daily budget', async () => { + await callText('vector_signer_sign', { txCbor: FIXTURE_CBOR }); + const limits = await callText('vector_signer_get_spend_limits'); + assert.match(limits, /REFUSED/, 'the refusal should appear in recent decisions'); + assert.match(limits, /Committed today:\s*0\.000000 AP3X/, 'a refusal must not consume budget'); + }); + + test('get_spend_limits reports the configured limits', async () => { + const text = await callText('vector_signer_get_spend_limits'); + assert.match(text, /100\.000000/); + assert.match(text, /500\.000000/); + }); + + test('sign rejects malformed CBOR', async () => { + const text = await callText('vector_signer_sign', { txCbor: 'not-hex' }); + assert.match(text, /could not be decoded|refus/i); + }); +}); diff --git a/packages/signer/tsup.config.ts b/packages/signer/tsup.config.ts index 7ced406..5e7f5dd 100644 --- a/packages/signer/tsup.config.ts +++ b/packages/signer/tsup.config.ts @@ -3,7 +3,16 @@ import { defineConfig } from "tsup"; export default defineConfig({ entry: ["src/index.ts"], format: ["esm"], - target: "es2020", + // es2022, not es2020 (Task 1's original value): index.ts uses a top-level + // `await server.connect(...)` (brief's own given code, Task 7), and + // top-level await is an ES2022 language feature — esbuild hard-errors + // ("Top-level await is not available in the configured target environment") + // rather than attempting an unsafe downlevel transform, unlike most other + // modern syntax. Node 22 (this package's engines.node floor) natively + // supports it. Scoped to the signer package only; the builder's own + // tsup.config.ts is untouched because vector.ts/index.ts never uses + // top-level await and has no need for the bump. + target: "es2022", outDir: "build", clean: true, splitting: false, From ece9ec9ab8f46f3cac4a390450df5fe5bef01ed5 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:36:58 +0200 Subject: [PATCH 20/27] fix(signer): withhold a signature when its audit record fails to write Hard requirement for this task: audit.ts's append() throws by design on a write failure (see its own CONTRACT note). The trap is catching that throw and still returning the signature already produced - that reproduces exactly the failure the throw exists to prevent, one call site later: a real signature reaches the caller with no durable record of it, so a restarted process's committedTodayLovelace() silently under-counts and the daily limit stops binding. vector_signer_sign now wraps both audit.append() calls explicitly: - Signed path: if the append throws, the signature is withheld. The function returns before ever reading signed.signedCborHex - the only `return` that reaches it is the one after a successful append. The response tells the caller a valid signature WAS produced but could not be durably recorded, so it is being withheld rather than released. - Refused path (lower stakes - no funds move either way): if the append throws, the refusal itself is still delivered with its concrete policy reason (never silently replaced by a generic disk-error message), plus a note that this specific refusal could not be recorded, so the caller does not mistake an incomplete audit trail for a normal one. Mutation-tested against the brief's own unguarded version (see the task report for the full transcript): reverting to unwrapped `audit.append()` calls does NOT leak signed CBOR either way (the throw prevents the function from ever reaching its return statement) - but it does replace "REFUSED. " with the SDK's generic isError conversion of audit.ts's own message on BOTH paths, so a refusal's own policy reason becomes invisible to the caller exactly when the audit log is unhealthy. Exactly 2 of 21 tests failed under that mutation, precisely the two targeting this gap, no collateral. Edge-case pass added to the smoke test (12 new tests, each on its own dedicated server instance): - loadConfig() / the built server's behaviour with no key material configured at all (clear throw; process exits non-zero and promptly, never hangs, never writes anything to stdout). - No mnemonic word or the derived private key appears in the startup banner or in any tool response across a sweep that includes a real successful sign. - decode_transaction never writes an audit entry, even for an in-policy transaction. - An undecodable transaction passed to sign() never reaches evaluate(), so there is nothing to audit yet - by design, pinned rather than assumed. - get_spend_limits surfaces the concrete refusal reason, not just the word REFUSED. - The hard requirement itself: a control test proving the in-policy fixture genuinely signs under a healthy audit log, then the same transaction against an unwritable audit log withholding the signature, then a pin of the (intentional, conservative) consequence that the withheld signature still counts toward this process's own committed- today total even though nothing was ever released. - The refused-path decision under the same unwritable-audit-log condition. Also reworded decode_transaction's response text ("(read-only)" / "would be ALLOWED" instead of "(not signed)" / "would be signed") so it never contains the substring "signed" even when a decoded transaction happens to be in-policy - matches this same smoke test's own `!/signed/i.test(text)` check on the brief's decode test, which otherwise fails against the brief's own unmodified wording on every call. typecheck 0, test:unit 180/180, builder test:smoke 26/23/0, boundary 4/4, signer test:smoke:signer 21/21 (9 brief + 12 edge-case pass). --- packages/signer/src/tools.ts | 73 +++- .../signer/test/smoke/signer-tools.test.ts | 412 ++++++++++++++++++ 2 files changed, 470 insertions(+), 15 deletions(-) diff --git a/packages/signer/src/tools.ts b/packages/signer/src/tools.ts index b9a5a84..4759875 100644 --- a/packages/signer/src/tools.ts +++ b/packages/signer/src/tools.ts @@ -76,14 +76,36 @@ export function registerSignerTools(server: McpServer, config: SignerConfig): vo const decision = evaluate(tx, ownAddresses, config.limits, audit.committedTodayLovelace()); if (!decision.allowed) { - audit.append({ - timestamp: new Date().toISOString(), - txHash: tx.txHashHex, - decision: 'refused', - netOutflowLovelace: decision.netOutflowLovelace.toString(), - reason: decision.reason, - assetMovements: decision.assetMovements.length, - }); + // CONTRACT (see audit.ts's own header comment): append() throws if the + // write to disk fails. No funds move on a refusal either way, so this + // is lower stakes than the signed branch below — but a refusal whose + // audit record silently failed to write would still hand back a + // clean-looking "REFUSED. " with nothing to indicate that the + // refusal itself is now missing from the durable log. Caught here (not + // left to propagate as a generic isError) specifically so the refusal + // text and reason are ALWAYS delivered to the caller regardless of + // audit health, with an appended note when the record could not be + // written — never silence about it, never a bare unexplained error in + // its place. + try { + audit.append({ + timestamp: new Date().toISOString(), + txHash: tx.txHashHex, + decision: 'refused', + netOutflowLovelace: decision.netOutflowLovelace.toString(), + reason: decision.reason, + assetMovements: decision.assetMovements.length, + }); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + console.error(`[sign] audit append failed for a REFUSED tx ${tx.txHashHex}: ${message}`); + return text( + `REFUSED. ${decision.reason}\n\n` + + `Note: this refusal could NOT be recorded in the local audit log (${message}). ` + + `No funds moved and nothing was signed, but the audit trail for this refusal is incomplete ` + + `until the log is writable again.` + ); + } return text(`REFUSED. ${decision.reason}`); } @@ -94,13 +116,34 @@ export function registerSignerTools(server: McpServer, config: SignerConfig): vo return text(`REFUSED. Signing failed: ${err instanceof Error ? err.message : String(err)}`); } - audit.append({ - timestamp: new Date().toISOString(), - txHash: signed.txHashHex, - decision: 'signed', - netOutflowLovelace: decision.netOutflowLovelace.toString(), - assetMovements: decision.assetMovements.length, - }); + // HARD REQUIREMENT (audit.ts's own CONTRACT note; see also the task + // report): append() throws on a write failure, by design. The reflex to + // avoid here is `catch { warn(); return signed anyway; }` — that + // reproduces exactly the failure the throw exists to prevent, just one + // call site later: a real signature reaches the caller while no durable + // record of it exists on disk, so the next process's + // committedTodayLovelace() silently under-counts and the daily limit + // stops binding. `signed.signedCborHex` is therefore not read anywhere + // below this catch block — the only `return` that can reach it is the + // one after a successful append. + try { + audit.append({ + timestamp: new Date().toISOString(), + txHash: signed.txHashHex, + decision: 'signed', + netOutflowLovelace: decision.netOutflowLovelace.toString(), + assetMovements: decision.assetMovements.length, + }); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + console.error(`[sign] withholding a produced signature for tx ${signed.txHashHex}: audit append failed: ${message}`); + return text( + `REFUSED. A valid signature for this transaction was produced, but it could not be durably ` + + `recorded in the local audit log, so it is being withheld rather than returned to you. ` + + `No signed transaction has been released to any caller. Detail: ${message}\n\n` + + `Check that the audit log path is writable, then retry.` + ); + } return text( `# Signed\n\nHash: ${signed.txHashHex}\n` + diff --git a/packages/signer/test/smoke/signer-tools.test.ts b/packages/signer/test/smoke/signer-tools.test.ts index 42aabc7..8da54d8 100644 --- a/packages/signer/test/smoke/signer-tools.test.ts +++ b/packages/signer/test/smoke/signer-tools.test.ts @@ -3,8 +3,12 @@ import assert from 'node:assert/strict'; import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; +import { spawn, type ChildProcess } from 'node:child_process'; +import * as CML from '@anastasia-labs/cardano-multiplatform-lib-nodejs'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import { MnemonicKeySource } from '../../src/keysource.ts'; +import { loadConfig } from '../../src/config.ts'; const TEST_MNEMONIC = 'test walk nut penalty hip pave soap entry language right filter choice'; @@ -112,3 +116,411 @@ describe('signer MCP server over stdio', () => { assert.match(text, /could not be decoded|refus/i); }); }); + +// --- Edge-case pass (beyond the brief) -------------------------------------- +// +// The brief's 9 tests above are a floor. Every previous task in this signer +// package shipped at least one defect that produced a plausible-looking +// success rather than an error (see .superpowers/sdd/signer-task-*-report.md) +// — that same suspicion is applied here, especially to the audit-then-return +// ordering in the sign tool, which is THE thing this task's brief calls out +// as mattering most: audit.ts's append() throws by design on a write +// failure (see the CONTRACT note at the top of audit.ts), and a signature +// must never be released to a caller when its own audit record failed to +// land durably on disk. +// +// Each test below spins up its OWN dedicated server (own temp dir, own +// mnemonic file, own audit log path) rather than sharing the brief's module- +// level `client`/`dir` — the brief's own 9 tests are order-dependent on a +// single accumulating audit log, and coupling more tests onto that same +// shared state would make failures harder to isolate for no benefit, since +// nothing below needs to observe the brief's own tests' history. + +const TEST_IDENTITY = new MnemonicKeySource(TEST_MNEMONIC).load(); + +interface EdgeServer { + client: Client; + dir: string; + stderrChunks: string[]; +} + +async function spawnSigner( + opts: { + auditLogPath?: (dir: string) => string; + perTx?: string; + daily?: string; + } = {} +): Promise { + const dir = mkdtempSync(join(tmpdir(), 'signer-smoke-edge-')); + const mnemonicFile = join(dir, 'mnemonic.txt'); + writeFileSync(mnemonicFile, TEST_MNEMONIC, { mode: 0o600 }); + const auditLogPath = opts.auditLogPath ? opts.auditLogPath(dir) : join(dir, 'audit.json'); + + const transport = new StdioClientTransport({ + command: 'node', + args: ['packages/signer/build/index.js'], + env: { + ...process.env, + VECTOR_SIGNER_MNEMONIC_FILE: mnemonicFile, + VECTOR_SIGNER_AUDIT_LOG_PATH: auditLogPath, + VECTOR_SIGNER_SPEND_LIMIT_PER_TX: opts.perTx ?? '100000000', + VECTOR_SIGNER_SPEND_LIMIT_DAILY: opts.daily ?? '500000000', + }, + // Piped (not the brief's default 'inherit') specifically so the startup + // banner can be captured and inspected for leaked key material below. + // Per the SDK's own doc comment on StdioClientTransport#stderr, the + // PassThrough stream exists immediately after construction, so the + // listener below is attached before connect() (which spawns the child) + // to avoid losing early output. + stderr: 'pipe', + }); + const stderrChunks: string[] = []; + transport.stderr?.on('data', (chunk: Buffer) => stderrChunks.push(chunk.toString('utf8'))); + + const client = new Client({ name: 'signer-smoke-edge', version: '1.0.0' }); + await client.connect(transport); + // client.connect() only waits for the MCP initialize handshake on stdout; + // stderr is a separate, independently-buffered pipe with no ordering + // guarantee relative to stdout at the Node event-loop level, even though + // the server writes its banner before it ever starts that handshake. A + // short grace period avoids a race between "handshake done" and "banner + // chunk delivered" when a test reads stderrChunks immediately afterward. + await new Promise((r) => setTimeout(r, 75)); + return { client, dir, stderrChunks }; +} + +async function stopSigner(server: EdgeServer): Promise { + try { + await server.client.close(); + } catch { + /* already closed */ + } + rmSync(server.dir, { recursive: true, force: true }); +} + +async function callToolText(client: Client, name: string, args: Record = {}): Promise { + const r = await client.callTool({ name, arguments: args }); + const c = r.content as Array<{ type: string; text?: string }>; + return c?.[0]?.text ?? ''; +} + +// Builds a synthetic transaction whose SOLE output pays `payToAddress`, with +// a fee well inside the smoke test's 100/500 AP3X limits. The brief's own +// FIXTURE_CBOR is deliberately refused for TEST_MNEMONIC (built for a +// different, funded wallet — see the note above the "sign refuses..." test), +// so a genuinely in-policy transaction has to be constructed by hand to +// exercise the "signed" branch of vector_signer_sign at all. +// +// Verified against a throwaway probe before use in any test (see the task +// report): decodes cleanly with decode.ts, policy.evaluate() returns +// allowed:true with netOutflowLovelace equal to feeLovelace alone (the +// output returns to its own address, so it counts as change, not outflow), +// and signTransaction() accepts it. The referenced input UTXO does not need +// to exist on-chain — decode.ts/policy.ts/sign.ts never resolve inputs +// against the chain, by design (that is exactly why this signer needs no +// network access at all). +function buildInPolicyTxCbor(payToAddress: string, feeLovelace = 200_000n, outputLovelace = 5_000_000n): string { + const inputTxHash = CML.TransactionHash.from_hex( + 'e17a2ebab8eae3850f959290041b3bef3f3597584f8cf4a728e14777dddb8c32' + ); + const inputs = CML.TransactionInputList.new(); + inputs.add(CML.TransactionInput.new(inputTxHash, 0n)); + + const outputs = CML.TransactionOutputList.new(); + outputs.add( + CML.TransactionOutput.new(CML.Address.from_bech32(payToAddress), CML.Value.from_coin(outputLovelace)) + ); + + const body = CML.TransactionBody.new(inputs, outputs, feeLovelace); + const tx = CML.Transaction.new(body, CML.TransactionWitnessSet.new(), true, undefined); + return tx.to_cbor_hex(); +} + +describe('loadConfig() — no key material configured', () => { + test('throws a clear, specific error naming what is missing, not a vague one', () => { + // No VECTOR_SIGNER_MNEMONIC / _MNEMONIC_FILE / _PRIVATE_KEY+_ADDRESS at + // all — the "nobody configured a key yet" case a fresh deployment or a + // misconfigured launcher would actually hit. + const bareEnv = {} as NodeJS.ProcessEnv; + assert.throws(() => loadConfig(bareEnv), /No key material configured/); + }); + + test('the built server exits promptly and loudly on stderr rather than hanging or crashing unhelpfully', async () => { + const strippedEnv: NodeJS.ProcessEnv = { ...process.env }; + delete strippedEnv.VECTOR_SIGNER_MNEMONIC; + delete strippedEnv.VECTOR_SIGNER_MNEMONIC_FILE; + delete strippedEnv.VECTOR_SIGNER_PRIVATE_KEY; + delete strippedEnv.VECTOR_SIGNER_ADDRESS; + + const child: ChildProcess = spawn('node', ['packages/signer/build/index.js'], { + env: strippedEnv, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let stdout = ''; + let stderr = ''; + child.stdout!.on('data', (c: Buffer) => { + stdout += c.toString('utf8'); + }); + child.stderr!.on('data', (c: Buffer) => { + stderr += c.toString('utf8'); + }); + + const exitCode = await new Promise((resolvePromise, reject) => { + const timeout = setTimeout( + () => reject(new Error(`process did not exit within 5s — looks like a hang. stderr so far: ${stderr}`)), + 5000 + ); + child.on('exit', (code) => { + clearTimeout(timeout); + resolvePromise(code); + }); + child.on('error', (err) => { + clearTimeout(timeout); + reject(err); + }); + }); + + assert.notEqual(exitCode, 0, 'must exit non-zero when it cannot start at all'); + assert.match(stderr, /No key material configured/, 'the real, specific reason must reach stderr'); + assert.equal(stdout, '', 'nothing MCP-protocol-shaped should ever reach stdout on a startup failure'); + }); +}); + +describe('key material is never emitted', () => { + test('the startup banner (stderr) never contains a mnemonic word or the derived private key', async () => { + const server = await spawnSigner(); + try { + const banner = server.stderrChunks.join(''); + assert.ok(banner.length > 0, 'sanity: something was actually captured on stderr'); + assert.match(banner, /ready/i); + for (const word of TEST_MNEMONIC.split(' ')) { + assert.ok(!banner.includes(word), `startup banner must not contain mnemonic word "${word}": ${banner}`); + } + assert.ok( + !banner.includes(TEST_IDENTITY.privateKeyBech32), + 'startup banner must not contain the derived private key' + ); + } finally { + await stopSigner(server); + } + }); + + test('no tool response across a sweep of calls (including a real successful sign) contains a mnemonic word or the derived private key', async () => { + const server = await spawnSigner(); + try { + const inPolicyTx = buildInPolicyTxCbor(TEST_IDENTITY.address); + + const responses: string[] = []; + responses.push(await callToolText(server.client, 'vector_signer_get_address')); + responses.push(await callToolText(server.client, 'vector_signer_decode_transaction', { txCbor: FIXTURE_CBOR })); + responses.push( + await callToolText(server.client, 'vector_signer_decode_transaction', { txCbor: inPolicyTx }) + ); + responses.push(await callToolText(server.client, 'vector_signer_sign', { txCbor: FIXTURE_CBOR })); // refused + responses.push(await callToolText(server.client, 'vector_signer_sign', { txCbor: 'not-hex' })); // malformed + responses.push(await callToolText(server.client, 'vector_signer_sign', { txCbor: inPolicyTx })); // actually signed + responses.push(await callToolText(server.client, 'vector_signer_get_spend_limits')); + + // Sanity: the sweep must actually exercise the real success path, or + // this test would vacuously pass by never looking at a response that + // could plausibly contain key material in the first place. + assert.ok(responses.some((r) => /Signed CBOR/i.test(r)), 'sanity: the sweep must include a real signed response'); + + for (const [i, resp] of responses.entries()) { + for (const word of TEST_MNEMONIC.split(' ')) { + assert.ok(!resp.includes(word), `response #${i} contains mnemonic word "${word}": ${resp.slice(0, 300)}`); + } + assert.ok(!resp.includes(TEST_IDENTITY.privateKeyBech32), `response #${i} contains the raw private key`); + } + } finally { + await stopSigner(server); + } + }); +}); + +describe('decode_transaction is strictly read-only', () => { + test('never appears in get_spend_limits recent decisions and never changes committed-today, even for an in-policy transaction', async () => { + const server = await spawnSigner(); + try { + const before = await callToolText(server.client, 'vector_signer_get_spend_limits'); + assert.match(before, /No decisions recorded yet\./, 'sanity: starts with an empty audit log'); + + await callToolText(server.client, 'vector_signer_decode_transaction', { txCbor: FIXTURE_CBOR }); + await callToolText(server.client, 'vector_signer_decode_transaction', { txCbor: FIXTURE_CBOR }); + await callToolText(server.client, 'vector_signer_decode_transaction', { + txCbor: buildInPolicyTxCbor(TEST_IDENTITY.address), + }); + + const after = await callToolText(server.client, 'vector_signer_get_spend_limits'); + assert.match( + after, + /No decisions recorded yet\./, + 'decode_transaction must never write an audit entry, even when previewing a transaction that would be ALLOWED' + ); + assert.match(after, /Committed today:\s*0\.000000 AP3X/); + } finally { + await stopSigner(server); + } + }); +}); + +describe('a decode failure inside sign() is refused before any audit entry is even attempted', () => { + test('malformed CBOR does not appear in recent decisions and does not change committed-today', async () => { + const server = await spawnSigner(); + try { + await callToolText(server.client, 'vector_signer_sign', { txCbor: 'not-hex' }); + const after = await callToolText(server.client, 'vector_signer_get_spend_limits'); + assert.match( + after, + /No decisions recorded yet\./, + 'undecodable input never reaches evaluate(), so there is no PolicyDecision to audit yet — by design, not a gap' + ); + assert.match(after, /Committed today:\s*0\.000000 AP3X/); + } finally { + await stopSigner(server); + } + }); +}); + +describe('get_spend_limits surfaces the concrete refusal reason, not just the word REFUSED', () => { + test('the per-transaction-limit reason text appears in recent decisions', async () => { + const server = await spawnSigner(); + try { + await callToolText(server.client, 'vector_signer_sign', { txCbor: FIXTURE_CBOR }); + const limits = await callToolText(server.client, 'vector_signer_get_spend_limits'); + assert.match(limits, /REFUSED/); + assert.match( + limits, + /per-transaction limit/i, + 'the recent-decisions line should carry the actual policy reason, not just the decision label' + ); + assert.match(limits, /Committed today:\s*0\.000000 AP3X/); + } finally { + await stopSigner(server); + } + }); +}); + +describe('THE hard requirement: an audit append failure on the SIGNED path withholds the signature', () => { + // audit.ts's own CONTRACT note (top of the file) is explicit: append() + // throws if the durable write fails, and a caller that catches the throw + // and still returns the signature it already produced reproduces exactly + // the failure the throw exists to prevent — a real signature reaching a + // caller with no durable record of it, so a restarted process's + // committedTodayLovelace() silently under-counts and the daily limit + // stops binding. This describe block is the test that proves tools.ts + // does not fall into that trap. + + test('control: the SAME in-policy transaction against a HEALTHY audit log genuinely signs (proves the fixture below is not simply broken)', async () => { + const server = await spawnSigner(); + try { + const tx = buildInPolicyTxCbor(TEST_IDENTITY.address); + const result = await callToolText(server.client, 'vector_signer_sign', { txCbor: tx }); + assert.match(result, /^# Signed/); + assert.match(result, /Signed CBOR/i); + } finally { + await stopSigner(server); + } + }); + + test('an unwritable audit log withholds a genuinely-produced signature: no signed CBOR anywhere in the response', async () => { + const server = await spawnSigner({ + // Mirrors audit.test.ts's own proven approach: a parent directory that + // does not exist makes writeFileSync fail with ENOENT, deterministically + // and cross-platform, without needing OS-specific permission tricks. + auditLogPath: (dir) => join(dir, 'no-such-subdirectory', 'audit.json'), + }); + try { + const tx = buildInPolicyTxCbor(TEST_IDENTITY.address); + const result = await callToolText(server.client, 'vector_signer_sign', { txCbor: tx }); + + assert.ok(!/Signed CBOR/i.test(result), 'a signature must never be returned when its audit record failed to write'); + assert.ok(!/```/.test(result), 'no fenced code block — where signed CBOR would appear — should be present at all'); + assert.match(result, /REFUSED/, 'the caller must get an unambiguous, non-signed response'); + assert.match(result, /produced/i, 'the response should be honest that a real signature WAS produced'); + assert.match( + result, + /(withheld|not been released)/i, + 'the response should say the signature is being withheld, not silently dropped' + ); + } finally { + await stopSigner(server); + } + }); + + test('after a withheld signature, this process still counts it toward committed-today (conservative over-count, never an under-count)', async () => { + // AuditLog.append() pushes the entry into its own in-memory array BEFORE + // attempting the write (audit.ts's own comment explains why: so THIS + // process's accounting does not under-count even when the write fails). + // tools.ts has no way to "unpush" that entry, and should not try to - + // over-counting a withheld signature against this process's own + // remaining daily budget is the safe direction, matching every other + // fail-closed choice in this package. Pinned here as a deliberate, + // understood consequence rather than an accidental surprise. + const server = await spawnSigner({ + auditLogPath: (dir) => join(dir, 'no-such-subdirectory', 'audit.json'), + }); + try { + const tx = buildInPolicyTxCbor(TEST_IDENTITY.address, 200_000n, 5_000_000n); + await callToolText(server.client, 'vector_signer_sign', { txCbor: tx }); + + const limits = await callToolText(server.client, 'vector_signer_get_spend_limits'); + assert.match( + limits, + /Committed today:\s*0\.200000 AP3X/, + 'the fee from the withheld signature is still counted in this process memory, even though it was never released' + ); + } finally { + await stopSigner(server); + } + }); +}); + +describe('a lower-stakes decision: an audit append failure on the REFUSED path', () => { + // Funds do not move on a refusal either way, so this is lower stakes than + // the signed case above — but a refusal whose own audit record silently + // failed to write must not look like a normal, cleanly-recorded refusal to + // the caller. Decision made here: still refuse (obviously — nothing about + // an audit failure should ever cause a REFUSED decision to become + // ALLOWED), still deliver the concrete policy reason, and additionally + // disclose that the audit trail for this specific refusal is incomplete. + + test('still refuses with the concrete reason, and additionally flags that the refusal itself could not be recorded', async () => { + const server = await spawnSigner({ + auditLogPath: (dir) => join(dir, 'no-such-subdirectory', 'audit.json'), + }); + try { + // FIXTURE_CBOR is refused for TEST_MNEMONIC unconditionally (~1000 AP3X + // net outflow against a 100 AP3X per-transaction limit — see the note + // above the brief's own "sign refuses..." test). No funds are at risk + // either way; this test is about whether the CALLER is told the truth + // about the state of the audit trail. + const result = await callToolText(server.client, 'vector_signer_sign', { txCbor: FIXTURE_CBOR }); + + assert.match(result, /REFUSED/); + assert.match( + result, + /per-transaction limit/i, + 'the concrete policy reason must still reach the caller even when the audit write fails' + ); + assert.ok(!/Signed CBOR/i.test(result)); + assert.match(result, /audit log/i); + assert.match(result, /(could not|NOT).*record/i, 'must disclose that this specific refusal was not durably recorded'); + } finally { + await stopSigner(server); + } + }); + + test('committed-today is unaffected by a refusal whether or not the audit write succeeds', async () => { + const server = await spawnSigner({ + auditLogPath: (dir) => join(dir, 'no-such-subdirectory', 'audit.json'), + }); + try { + await callToolText(server.client, 'vector_signer_sign', { txCbor: FIXTURE_CBOR }); + const limits = await callToolText(server.client, 'vector_signer_get_spend_limits'); + assert.match(limits, /Committed today:\s*0\.000000 AP3X/); + } finally { + await stopSigner(server); + } + }); +}); From d4a1a217a6a8818b6c1b24aac199d68bce139dc5 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:28:25 +0200 Subject: [PATCH 21/27] test(signer): pin decode's read-only contract and stdout purity directly A decode_transaction that signs only when policy-allowed, worded to avoid "sign", passed all 21 tests - a preview tool returning real signed bytes with no audit entry and no budget consumed. And the stdout-purity constraint was only enforced incidentally by an unrelated stderr sanity check, while the MCP client silently swallows unparseable stdout lines. Gap 1 (decode-can-sign) is closed structurally, not just by test: tools.ts splits into registerReadOnlyTools() (get_address, decode_transaction, get_spend_limits - receives only signerAddress/limits/auditLogPath/audit, never config or keySource) and registerSignTool() (the only function in the module whose closure ever holds a loaded SigningIdentity). Reproducing the review's exact mutation against the new code is now a compile error (TS2304: Cannot find name 'identity') rather than a passing test suite. Defense-in-depth on top of that: a new content-based test independently computes what signing the previewed transaction would actually produce and asserts decode's response never contains those bytes, under any wording - this is what catches a more determined mutation that deliberately re-widens the read-only context to reach signing capability again. Gap 2 (stdout purity) is closed with a purpose-built minimal Transport (RawCapturingStdioTransport) that captures every raw stdout byte independently of the SDK's own lenient line parsing, while still driving the real Client class for a correct initialize handshake. Asserts every line written to stdout, across a healthy startup and a normal tool call, parses as JSON. Both fixes reproduced-then-verified: applying the review's mutations first confirmed each gap was real (all tests green under the vulnerable code), then confirmed each new check catches it, then confirmed restoring the fix returns to 23/23 with no collateral. Also re-ran the original hard-requirement mutations (signed- and refused-path unguarded audit.append, the exact catch{warn();return signature} anti-pattern, and a missing return after the refused branch that lets a refused transaction fall through to real signing) against the restructured tools.ts to confirm the refactor weakened nothing - none did; the missing-return mutation alone is caught by 4 tests, including 3 of the brief's own original 9. typecheck 0, test:unit 180/180, builder test:smoke 26/23/0, boundary 4/4, signer test:smoke:signer 23/23 (9 brief + 12 original edge-case pass + 2 new). --- packages/signer/src/tools.ts | 139 +++++++++---- .../signer/test/smoke/signer-tools.test.ts | 192 ++++++++++++++++++ 2 files changed, 296 insertions(+), 35 deletions(-) diff --git a/packages/signer/src/tools.ts b/packages/signer/src/tools.ts index 4759875..9e345bd 100644 --- a/packages/signer/src/tools.ts +++ b/packages/signer/src/tools.ts @@ -2,10 +2,11 @@ import { z } from 'zod/v4'; import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { lovelaceToAda } from '@apexfusion/vector-mcp-shared/tx'; import { decodeTransaction } from './decode.js'; -import { evaluate } from './policy.js'; +import { evaluate, type SpendLimits } from './policy.js'; import { signTransaction } from './sign.js'; import { AuditLog } from './audit.js'; import type { SignerConfig } from './config.js'; +import type { KeySource } from './keysource.js'; const txCborShape = { txCbor: z.string().describe('Hex-encoded CBOR of an unsigned transaction') }; @@ -13,16 +14,48 @@ function text(body: string) { return { content: [{ type: 'text' as const, text: body }] }; } -export function registerSignerTools(server: McpServer, config: SignerConfig): void { - const identity = config.keySource.load(); - const audit = new AuditLog(config.auditLogPath); - const ownAddresses = [identity.address]; +// Structural guard (review finding, Task 7 follow-up): registerSignerTools() +// used to load the signing identity once and hold it in ONE closure shared +// by all four tool handlers - including decode_transaction and +// get_spend_limits, which never needed it. That is not just untidy: it means +// `identity.privateKeyBech32` sat pre-loaded and one line away inside the +// decode handler, and a mutation that added +// `signTransaction(txCbor, identity.privateKeyBech32)` there, gated behind +// `decision.allowed` and worded to avoid the substring "sign", passed every +// existing test (see the task report for the full mutation transcript) - a +// preview tool that can be made to return genuine signed bytes with no audit +// entry and no budget consumed. +// +// registerReadOnlyTools() below is the fix: its parameter list is exactly +// the non-secret pieces get_address/decode_transaction/get_spend_limits +// need. It never receives `config`, `config.keySource`, or an `identity` +// object, so none of the handlers defined inside it can reach the private +// key - not via a pre-loaded variable, and not via a fresh +// `keySource.load()` call either, because neither `config` nor `keySource` +// is a name that resolves in that function's scope at all. This is +// structural, not a matter of discipline: reintroducing signing capability +// into that scope requires visibly changing this function's signature, not +// a one-line addition inside a handler body. +// +// registerSignTool() is the only function in this module whose closure ever +// contains a loaded SigningIdentity. It registers exactly one tool. +interface ReadOnlyToolsContext { + signerAddress: string; + keySourceDescription: string; + limits: SpendLimits; + auditLogPath: string; + audit: AuditLog; +} + +function registerReadOnlyTools(server: McpServer, ctx: ReadOnlyToolsContext): void { + const { signerAddress, keySourceDescription, limits, auditLogPath, audit } = ctx; + const ownAddresses = [signerAddress]; server.tool( 'vector_signer_get_address', 'Get this signer\'s wallet address. Pass it to the builder as changeAddress when building a transaction.', {}, - async () => text(`Signer address: ${identity.address}\nKey source: ${config.keySource.describe()}`) + async () => text(`Signer address: ${signerAddress}\nKey source: ${keySourceDescription}`) ); server.tool( @@ -32,10 +65,10 @@ export function registerSignerTools(server: McpServer, config: SignerConfig): vo async ({ txCbor }) => { try { const tx = decodeTransaction(txCbor); - const decision = evaluate(tx, ownAddresses, config.limits, audit.committedTodayLovelace()); + const decision = evaluate(tx, ownAddresses, limits, audit.committedTodayLovelace()); const rows = tx.outputs .map((o, i) => { - const mine = o.address === identity.address; + const mine = o.address === signerAddress; const assets = o.assets.length ? ` + ${o.assets.length} asset(s)` : ''; return ` ${i}. ${mine ? '[change] ' : '[outgoing]'} ${lovelaceToAda(o.lovelace)} AP3X${assets}\n ${o.address}`; }) @@ -60,6 +93,44 @@ export function registerSignerTools(server: McpServer, config: SignerConfig): vo } ); + server.tool( + 'vector_signer_get_spend_limits', + 'Report this signer\'s spend limits, how much of today\'s budget is committed, and recent signing decisions.', + {}, + async () => { + const committed = audit.committedTodayLovelace(); + const remaining = limits.dailyLovelace - committed; + const recent = audit + .recent(5) + .map((e) => ` ${e.timestamp} ${e.decision.toUpperCase()} ${lovelaceToAda(BigInt(e.netOutflowLovelace))} AP3X ${e.txHash.slice(0, 16)}...${e.reason ? ` (${e.reason})` : ''}`) + .join('\n'); + return text( + `# Signer spend limits\n\n` + + `Per transaction: ${lovelaceToAda(limits.perTxLovelace)} AP3X\n` + + `Daily: ${lovelaceToAda(limits.dailyLovelace)} AP3X\n` + + `Committed today: ${lovelaceToAda(committed)} AP3X\n` + + `Remaining today: ${lovelaceToAda(remaining > 0n ? remaining : 0n)} AP3X\n\n` + + `Audit log: ${auditLogPath}\n\n` + + (recent ? `Recent decisions:\n${recent}` : 'No decisions recorded yet.') + + `\n\nNote: the daily total counts signed transactions at signing time. The signer has no ` + + `network access and cannot observe whether a submit later succeeded, so a failed submit ` + + `still counts against the budget.` + ); + } + ); +} + +// The ONLY function in this module whose closure ever contains a loaded +// SigningIdentity (and therefore the raw private key). Its parameter list +// receives `keySource` directly, not the whole `SignerConfig` - a smaller +// surface than strictly required, kept deliberately narrow so this +// function's own signature stays an honest, visible marker of "this is +// where signing capability lives" for anyone reading the module top to +// bottom. +function registerSignTool(server: McpServer, keySource: KeySource, limits: SpendLimits, audit: AuditLog): void { + const identity = keySource.load(); + const ownAddresses = [identity.address]; + server.tool( 'vector_signer_sign', 'Decode, policy-check, and sign an unsigned transaction. Returns signed CBOR to submit via the builder. Refuses if the transaction violates spend policy.', @@ -73,7 +144,7 @@ export function registerSignerTools(server: McpServer, config: SignerConfig): vo return text(`REFUSED. ${err instanceof Error ? err.message : String(err)}`); } - const decision = evaluate(tx, ownAddresses, config.limits, audit.committedTodayLovelace()); + const decision = evaluate(tx, ownAddresses, limits, audit.committedTodayLovelace()); if (!decision.allowed) { // CONTRACT (see audit.ts's own header comment): append() throws if the @@ -106,6 +177,11 @@ export function registerSignerTools(server: McpServer, config: SignerConfig): vo `until the log is writable again.` ); } + // This `return` is load-bearing on its own, not just the branch + // above it: without it, a policy-refused transaction would fall + // through into the signing code below and be signed for real. Pinned + // by a dedicated mutation test in the smoke suite (see the task + // report) that removes exactly this statement. return text(`REFUSED. ${decision.reason}`); } @@ -153,30 +229,23 @@ export function registerSignerTools(server: McpServer, config: SignerConfig): vo ); } ); - - server.tool( - 'vector_signer_get_spend_limits', - 'Report this signer\'s spend limits, how much of today\'s budget is committed, and recent signing decisions.', - {}, - async () => { - const committed = audit.committedTodayLovelace(); - const remaining = config.limits.dailyLovelace - committed; - const recent = audit - .recent(5) - .map((e) => ` ${e.timestamp} ${e.decision.toUpperCase()} ${lovelaceToAda(BigInt(e.netOutflowLovelace))} AP3X ${e.txHash.slice(0, 16)}...${e.reason ? ` (${e.reason})` : ''}`) - .join('\n'); - return text( - `# Signer spend limits\n\n` + - `Per transaction: ${lovelaceToAda(config.limits.perTxLovelace)} AP3X\n` + - `Daily: ${lovelaceToAda(config.limits.dailyLovelace)} AP3X\n` + - `Committed today: ${lovelaceToAda(committed)} AP3X\n` + - `Remaining today: ${lovelaceToAda(remaining > 0n ? remaining : 0n)} AP3X\n\n` + - `Audit log: ${config.auditLogPath}\n\n` + - (recent ? `Recent decisions:\n${recent}` : 'No decisions recorded yet.') + - `\n\nNote: the daily total counts signed transactions at signing time. The signer has no ` + - `network access and cannot observe whether a submit later succeeded, so a failed submit ` + - `still counts against the budget.` - ); - } - ); +} + +export function registerSignerTools(server: McpServer, config: SignerConfig): void { + // Only the address survives past this line into registerReadOnlyTools()'s + // context - `config.keySource` itself (the only path to the private key) + // is threaded exclusively into registerSignTool() below. See the comment + // above registerReadOnlyTools() for why this split exists. + const { address: signerAddress } = config.keySource.load(); + const audit = new AuditLog(config.auditLogPath); + + registerReadOnlyTools(server, { + signerAddress, + keySourceDescription: config.keySource.describe(), + limits: config.limits, + auditLogPath: config.auditLogPath, + audit, + }); + + registerSignTool(server, config.keySource, config.limits, audit); } diff --git a/packages/signer/test/smoke/signer-tools.test.ts b/packages/signer/test/smoke/signer-tools.test.ts index 8da54d8..68099df 100644 --- a/packages/signer/test/smoke/signer-tools.test.ts +++ b/packages/signer/test/smoke/signer-tools.test.ts @@ -7,8 +7,11 @@ import { spawn, type ChildProcess } from 'node:child_process'; import * as CML from '@anastasia-labs/cardano-multiplatform-lib-nodejs'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; +import type { JSONRPCMessage } from '@modelcontextprotocol/sdk/types.js'; import { MnemonicKeySource } from '../../src/keysource.ts'; import { loadConfig } from '../../src/config.ts'; +import { signTransaction } from '../../src/sign.ts'; const TEST_MNEMONIC = 'test walk nut penalty hip pave soap entry language right filter choice'; @@ -236,6 +239,155 @@ function buildInPolicyTxCbor(payToAddress: string, feeLovelace = 200_000n, outpu return tx.to_cbor_hex(); } +// A minimal hand-rolled Transport, used ONLY by the stdout-purity test below. +// StdioClientTransport (used everywhere else in this file) does not expose +// the child process's raw stdout stream through any public API — only +// `.stderr` and `.pid` are getters on it, by design, since a normal caller +// should only ever see fully parsed messages. That is exactly the wrong tool +// for this test: verified directly against the SDK's own source +// (node_modules/@modelcontextprotocol/sdk/dist/esm/shared/stdio.js and +// client/stdio.js) that `ReadBuffer.readMessage()` advances its internal +// buffer PAST a line before attempting to parse it, so a line that fails +// `JSON.parse` is silently skipped — the read loop moves on to the next +// line and the connection keeps working. A stray non-JSON line on stdout +// (e.g. index.ts's banner lines written via console.log instead of +// console.error) would therefore corrupt the protocol stream without +// failing the handshake, without failing any tool call, and without +// tripping any assertion that only checks "did the connection work" — +// which is all every other test in this file does. This class exists so a +// test can instead read every raw byte written to stdout directly and +// assert on it, independent of the SDK's own leniency. +// +// Reimplements the wire format directly (JSON.stringify(message) + '\n' to +// send; split-on-'\n', JSON.parse each line to receive) rather than +// importing the SDK's own ReadBuffer/serializeMessage — both are internal, +// unexported implementation details of shared/stdio.js, not part of the +// package's public export surface (unlike client/index.js and +// client/stdio.js, which this file already imports). The format itself is +// simple enough (confirmed by reading the same source above) that +// reproducing it directly here is more honest than reaching into another +// module's internals. +class RawCapturingStdioTransport implements Transport { + private child?: ChildProcess; + private lineBuffer = ''; + readonly rawStdoutChunks: string[] = []; + onclose?: () => void; + onerror?: (error: Error) => void; + onmessage?: (message: JSONRPCMessage) => void; + + constructor( + private readonly command: string, + private readonly args: string[], + private readonly env: NodeJS.ProcessEnv + ) {} + + async start(): Promise { + this.child = spawn(this.command, this.args, { + env: this.env, + stdio: ['pipe', 'pipe', 'ignore'], + }); + this.child.stdout!.on('data', (chunk: Buffer) => { + const chunkText = chunk.toString('utf8'); + // Captured BEFORE any line-splitting/parsing below — this is the raw + // ground truth the test asserts against, deliberately independent of + // whatever the parsing loop below does with it. + this.rawStdoutChunks.push(chunkText); + + this.lineBuffer += chunkText; + let index: number; + while ((index = this.lineBuffer.indexOf('\n')) !== -1) { + const line = this.lineBuffer.slice(0, index).replace(/\r$/, ''); + this.lineBuffer = this.lineBuffer.slice(index + 1); + if (line.length === 0) continue; + try { + this.onmessage?.(JSON.parse(line) as JSONRPCMessage); + } catch (err) { + this.onerror?.(err instanceof Error ? err : new Error(String(err))); + } + } + }); + this.child.on('close', () => this.onclose?.()); + this.child.on('error', (err) => this.onerror?.(err)); + await new Promise((resolvePromise, reject) => { + this.child!.once('spawn', () => resolvePromise()); + this.child!.once('error', reject); + }); + } + + async send(message: JSONRPCMessage): Promise { + const stdin = this.child?.stdin; + if (!stdin) throw new Error('RawCapturingStdioTransport: not connected'); + const json = JSON.stringify(message) + '\n'; + await new Promise((resolvePromise) => { + if (stdin.write(json)) resolvePromise(); + else stdin.once('drain', resolvePromise); + }); + } + + async close(): Promise { + this.child?.kill(); + } +} + +describe('stdout carries ONLY the MCP protocol — direct assertion, not an incidental one', () => { + // Review finding (Task 7 follow-up): swapping index.ts's two startup- + // banner console.error calls for console.log was previously caught only + // incidentally, by the "startup banner (stderr)" test's + // `banner.length > 0` sanity check happening to fail once the banner + // stopped going to stderr at all — a coincidence of a DIFFERENT test's + // setup, not a real check of stdout. Confirmed directly against the SDK's + // own source (see the comment on RawCapturingStdioTransport above) that + // there is no implicit tripwire either: a corrupted line is silently + // skipped by the read loop, so the connection and every tool call in this + // suite would keep working normally even with genuinely corrupted stdout. + test('every line written to stdout, across a healthy startup and a normal tool call, is valid JSON', async () => { + const dir = mkdtempSync(join(tmpdir(), 'signer-smoke-stdout-')); + const mnemonicFile = join(dir, 'mnemonic.txt'); + writeFileSync(mnemonicFile, TEST_MNEMONIC, { mode: 0o600 }); + + const transport = new RawCapturingStdioTransport('node', ['packages/signer/build/index.js'], { + ...process.env, + VECTOR_SIGNER_MNEMONIC_FILE: mnemonicFile, + VECTOR_SIGNER_AUDIT_LOG_PATH: join(dir, 'audit.json'), + VECTOR_SIGNER_SPEND_LIMIT_PER_TX: '100000000', + VECTOR_SIGNER_SPEND_LIMIT_DAILY: '500000000', + }); + const client = new Client({ name: 'signer-smoke-stdout-purity', version: '1.0.0' }); + try { + // The real handshake (initialize -> notifications/initialized), driven + // by the actual Client class for protocol correctness — only the + // transport underneath it is custom. See Client.connect() in + // node_modules/@modelcontextprotocol/sdk/dist/esm/client/index.js: it + // calls transport.start(), sends 'initialize', then + // 'notifications/initialized' once the response validates. + await client.connect(transport); + // One normal tool call, matching "a normal tool call" from the review. + await callToolText(client, 'vector_signer_get_address'); + + const rawOutput = transport.rawStdoutChunks.join(''); + const lines = rawOutput + .split('\n') + .map((l) => l.replace(/\r$/, '')) + .filter((l) => l.length > 0); + + assert.ok(lines.length >= 2, `sanity: expected at least an initialize response and a tool-call response, got ${lines.length} line(s)`); + for (const [i, line] of lines.entries()) { + assert.doesNotThrow( + () => JSON.parse(line), + `stdout line #${i} is not valid JSON — the MCP protocol stream is corrupted by a stray write: ${line.slice(0, 200)}` + ); + } + } finally { + try { + await client.close(); + } catch { + /* already closed */ + } + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + describe('loadConfig() — no key material configured', () => { test('throws a clear, specific error naming what is missing, not a vague one', () => { // No VECTOR_SIGNER_MNEMONIC / _MNEMONIC_FILE / _PRIVATE_KEY+_ADDRESS at @@ -362,6 +514,46 @@ describe('decode_transaction is strictly read-only', () => { await stopSigner(server); } }); + + // Review finding (Task 7 follow-up): the audit-log check above does NOT + // catch a decode_transaction that has been made to genuinely sign a + // transaction whenever policy would have allowed it, worded to avoid the + // substring "sign" (e.g. "Witnessed bytes:") - that mutation writes no + // audit entry at all, so "No decisions recorded yet." stays true right up + // until real signed bytes are already sitting in the response text. This + // test targets that different failure mode directly: it does not depend on + // wording, and it does not depend on FIXTURE_CBOR happening to be refused + // (the mutation is only reachable when a transaction is genuinely allowed). + // + // registerReadOnlyTools() in tools.ts (see its own comment) now makes the + // underlying mutation impossible to write as a one-line addition in the + // first place - decode_transaction's handler has no `identity` or + // `keySource` in scope at all, structurally, not by convention. This test + // is the defense-in-depth layer underneath that structural guard: it pins + // the CURRENT behaviour directly against what real signing would actually + // produce, so it would still catch a future regression that re-threads + // signing capability into that scope some other way. + test('the response never contains the transaction\'s actual signed bytes, computed independently, for a transaction that would be ALLOWED', async () => { + const server = await spawnSigner(); + try { + const tx = buildInPolicyTxCbor(TEST_IDENTITY.address); + // Computed independently, in the test, via the same signTransaction() + // the real vector_signer_sign tool uses - this is exactly what a + // "helpfully pre-computed" decode_transaction would produce and return, + // whatever label it gave those bytes. + const { signedCborHex } = signTransaction(tx, TEST_IDENTITY.privateKeyBech32); + + const result = await callToolText(server.client, 'vector_signer_decode_transaction', { txCbor: tx }); + + assert.match(result, /would be ALLOWED/i, 'sanity: this fixture must actually reach the allowed branch decode_transaction previews, or the check below is vacuous'); + assert.ok( + !result.includes(signedCborHex), + 'decode_transaction must never return the transaction\'s actual signed bytes, under any wording or label' + ); + } finally { + await stopSigner(server); + } + }); }); describe('a decode failure inside sign() is refused before any audit entry is even attempted', () => { From b4038aa1f1b449a8c38f10f8faf79a06a6c7dc13 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:44:36 +0200 Subject: [PATCH 22/27] docs(signer): document configuration, semantics, and known limitations Includes an opt-in testnet round-trip test (keyless build -> decode -> policy -> sign, never submits), excluded from CI because it needs a funded wallet. Run against the real funded testnet wallet: pass 3, fail 0, nothing submitted. Verified every README claim against the code as implemented, not just the brief text, and corrected several: - VECTOR_SIGNER_AUDIT_LOG_PATH's default resolves against the package's own install location (repo root), not the process's cwd as originally stated -- and that default filename is not yet covered by .gitignore. - An audit-log write failure blocks ALL signing, not just accounting: audit.append() throws by design and vector_signer_sign withholds an already-produced signature rather than release one with no durable record. - Documented the #private-field defense against accidental key leakage via JSON.stringify/console.log, and the read-only/sign tool split that keeps the signing key out of scope for get_address/decode/get_spend_limits. - Documented that pairing this signer with the CURRENT hosted builder does not yet remove the custody exposure it's designed to fix: no builder tool is keyless yet, and there is no submit_transaction tool to hand a produced signature to. That's the next stage of the migration, not shipped here. Env-var cross-check (brief's Step 6, both directions) passes clean. --- README.md | 33 +++ package.json | 3 +- packages/signer/README.md | 196 ++++++++++++++++++ .../signer/test/integration/roundtrip.test.ts | 79 +++++++ 4 files changed, 310 insertions(+), 1 deletion(-) create mode 100644 packages/signer/README.md create mode 100644 packages/signer/test/integration/roundtrip.test.ts diff --git a/README.md b/README.md index 57d018f..91b612e 100644 --- a/README.md +++ b/README.md @@ -30,6 +30,21 @@ claude mcp add --transport sse vector-mcp https://mcp.vector.mainnet.apexfusion. Self-hosting instructions are below. +## Local signer (non-custodial path) + +`packages/signer` is a local MCP server that holds your key and signs transactions locally +instead of handing your mnemonic to a shared host. It has **no network access at all** — +stdio transport, no Provider, no egress — so a key given to it never reaches a shared server +or your model provider. + +See [`packages/signer/README.md`](packages/signer/README.md) for configuration, its tools, and +known limitations — including the gap that matters most right now: the hosted builder's tools +above still require your mnemonic as a call parameter, so pairing the signer with *this* +hosted builder does not yet remove the custody exposure described above. Closing that gap is +the next stage of the migration described in +[`docs/architecture/non-custodial-split.md`](docs/architecture/non-custodial-split.md); until +it ships, the security notice above still applies. + ## Features - **Wallet management** - derive addresses from mnemonic, query balances and UTxOs @@ -194,6 +209,24 @@ npm run test:integration Requires `mnemonic.txt` in `packages/builder/` containing a **funded Vector testnet** mnemonic. Covers the core tools end-to-end against Vector testnet, including the full agent lifecycle: register, discover, profile, update, transfer, message, and deregister. Never runs in CI. +```bash +npm run test:smoke:signer +``` + +Builds the local signer (`packages/signer`), boots it over stdio, and exercises all four of +its tools, including the fail-closed audit-write-failure path. No wallet, no network. CI runs +this on every PR alongside the two smoke tests above. + +```bash +export VECTOR_SIGNER_MNEMONIC_FILE=/absolute/path/to/testnet-mnemonic.txt +npm run test:integration:signer +``` + +Requires a **funded Vector testnet** wallet. Builds a real unsigned transaction the way the +hosted builder does (no key), then has the local signer decode, policy-check, and sign it +against live chain data. **Never submits.** Never runs in CI. See +[`packages/signer/README.md`](packages/signer/README.md#testing). + ## Architecture ``` diff --git a/package.json b/package.json index 0a1643f..a126701 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,8 @@ "test:unit": "node --import tsx --test packages/*/test/*.test.ts", "test:smoke": "npm run build && node --import tsx --test packages/builder/test/smoke/*.test.ts", "test:smoke:signer": "npm run build && node --import tsx --test packages/signer/test/smoke/*.test.ts", - "test:integration": "npm run build && node --import tsx --test packages/builder/test/integration/*.test.ts" + "test:integration": "npm run build && node --import tsx --test packages/builder/test/integration/*.test.ts", + "test:integration:signer": "node --import tsx --test packages/signer/test/integration/*.test.ts" }, "devDependencies": { "@types/node": "^22.13.1", diff --git a/packages/signer/README.md b/packages/signer/README.md new file mode 100644 index 0000000..2121fc7 --- /dev/null +++ b/packages/signer/README.md @@ -0,0 +1,196 @@ +# Vector MCP Signer + +Local signing companion for the Vector MCP server. **Holds your key; has no network access.** + +The hosted builder is keyless: it constructs unsigned transactions but cannot sign. This +package signs them locally, so your key never reaches a shared server or your model +provider. + +## Why it has no network + +- **stdio transport only** — no port, no HTTP listener, nothing to connect to +- **no Provider and no Lucid instance** — signing uses CML directly +- **submission lives on the builder**, so the signer needs no egress either + +`test/boundary.test.ts` enforces this over both the import graph and the manifest — it walks +every file under `src/`, checks `package.json` for network-capable dependencies, and greps for +ambient network globals (`fetch`, `WebSocket`, ...). It is the package's central security +guarantee. + +`test/integration/roundtrip.test.ts` imports `@lucid-evolution/lucid` and the shared package's +`/provider` — that does **not** weaken the guarantee above. That file lives under `test/`, +which `boundary.test.ts` deliberately scans only as far as `src/`, and which the build never +bundles (`tsup`'s only entry point is `src/index.ts`). Those imports exist so the test can +build a real transaction the way the hosted *builder* does (no key) before handing it to the +signer's actual decode/policy/sign code path — the network calls belong to the test's +stand-in builder, never to code that ships. + +## Defense in depth + +Beyond "no network," a few structural choices narrow what a bug or an accidental leak can +expose: + +- **The read-only tools cannot reach the key, even in principle.** `vector_signer_get_address`, + `vector_signer_decode_transaction`, and `vector_signer_get_spend_limits` are all registered + by one function whose parameter list never includes the key source. That's structural, not a + matter of discipline: reintroducing signing capability there means visibly changing that + function's signature, not adding a line inside a handler. +- **Key material resists accidental logging.** TypeScript `private` is compile-time only — at + runtime the field is an ordinary enumerable property, fully visible to `JSON.stringify`, + `console.log`, and `util.inspect`. Mnemonics and derived private keys are therefore held in + native JavaScript `#private` fields instead, and both `KeySource` classes override + `toJSON()` and Node's inspect symbol so that even a stray `console.log(keySource)` prints a + safe description instead of the secret. This defends against *accidental* disclosure (a + debugging log line, a crash reporter serializing local state) — it is not a defense against + a compromised process, which can still read the key out of memory directly. +- **The audit log is crash-safe.** Every entry is written to a temp file and renamed into + place, so a process kill mid-write costs at most the one entry in flight, never the rest of + the day's history. See the mandatory-audit limitation below for the cost of this design. + +## Configuration + +Pick one key source: + +| Variable | Notes | +|---|---| +| `VECTOR_SIGNER_MNEMONIC_FILE` | Path to a file containing your mnemonic. Use `chmod 600`. Recommended. | +| `VECTOR_SIGNER_MNEMONIC` | The mnemonic inline. Convenient, but ends up in your shell history and process environment. | +| `VECTOR_SIGNER_PRIVATE_KEY` + `VECTOR_SIGNER_ADDRESS` | A derived bech32 signing key. **Safest in terms of blast radius** — it controls one account rather than your whole HD tree, so a leak is bounded. See the mismatch caveat under Known limitations. | + +If more than one is set, `config.ts` checks them in this order: `VECTOR_SIGNER_PRIVATE_KEY` + +`VECTOR_SIGNER_ADDRESS`, then `VECTOR_SIGNER_MNEMONIC`, then `VECTOR_SIGNER_MNEMONIC_FILE`. +Unset the ones you are not using — a leftover `VECTOR_SIGNER_MNEMONIC` in your shell profile +silently wins over a `VECTOR_SIGNER_MNEMONIC_FILE` you just configured. + +Optional: + +| Variable | Default | Notes | +|---|---|---| +| `VECTOR_SIGNER_ACCOUNT_INDEX` | `0` | HD account index, for mnemonic sources | +| `VECTOR_SIGNER_SPEND_LIMIT_PER_TX` | `100000000` (100 AP3X) | Refuses any transaction whose net outflow exceeds this | +| `VECTOR_SIGNER_SPEND_LIMIT_DAILY` | `500000000` (500 AP3X) | Refuses once today's signed total would exceed this | +| `VECTOR_SIGNER_AUDIT_LOG_PATH` | *(see note below)* | Local record of every decision | + +**`VECTOR_SIGNER_AUDIT_LOG_PATH`, precisely:** if unset, the log is written to +`vector-signer-audit-log.json` resolved against the *package's own install location* (three +directories up from `build/index.js` — the monorepo root, in this repo), **not** against +whatever directory the process happened to be started from. That makes the location +predictable regardless of your MCP client's working directory, but it also means the file is +easy to forget about. In particular: unlike the builder's `vector-audit-log.json`, this +filename is **not currently listed in this repo's `.gitignore`**. If you run the signer from a +checkout of this repo without setting `VECTOR_SIGNER_AUDIT_LOG_PATH` explicitly, check before +you `git add` — or just set the variable and point it somewhere outside the repo. + +## Tools + +| Tool | Purpose | +|---|---| +| `vector_signer_get_address` | Your address. Pass it to the builder as `changeAddress`. | +| `vector_signer_decode_transaction` | Show what a transaction would move, **without signing**. | +| `vector_signer_sign` | Policy-check and sign. Returns signed CBOR. | +| `vector_signer_get_spend_limits` | Limits, today's committed total, recent decisions. | + +No tool accepts key material as a parameter — that is asserted by the smoke test. + +## What "net outflow" means + +Spend limits apply to **net outflow**: every output that does *not* return to your address, +plus the fee. Change is excluded, so a 1000 AP3X wallet sending 3 AP3X is measured as ~3.2 +AP3X, not 1000. + +An output to your own wallet at a *different* derivation index counts as outflow. That +over-counts, which refuses too much rather than too little. + +## Known limitations + +- **Native assets are recorded and reported, but not limited.** Spend limits apply to + lovelace. A transaction moving ~2 AP3X plus a valuable NFT passes the lovelace check. + Asset movements appear in `decode_transaction` output and the audit log so they are + visible, but they are not yet a policy input. +- **The audit log records intent, not confirmation.** The signer has no network and cannot + observe whether a submit succeeded, so a failed submit still counts against the daily + budget. Conservative by design. +- **A durable audit record is mandatory for signing — which means an unwritable audit log + stops all signing, not just accounting.** `AuditLog.append()` throws if the write fails, and + `vector_signer_sign` withholds a signature it already produced rather than return one with + no durable record of it — the alternative would let a restarted process silently under-count + today's spend, which quietly raises the effective daily limit. The practical effect: if the + audit log's directory disappears, the disk fills, or the path loses write permission, every + `vector_signer_sign` call starts returning `REFUSED` — including transactions well inside + policy — until it is writable again. That refusal names the audit failure explicitly rather + than reading like an ordinary policy refusal, but it is still a full stop on signing, not a + warning. +- **The key is plaintext at rest** in v1 (env var or file). The win is that it is *local* + and never transits your model provider or a shared host — not that it is in an HSM. + `KeySource` exists so keychain and hardware backends can be added without touching the + signing path. +- **`VECTOR_SIGNER_PRIVATE_KEY` and `VECTOR_SIGNER_ADDRESS` are not cross-checked against each + other.** Each is validated as well-formed independently, but nothing confirms the address is + actually the one the private key controls — that check is possible entirely offline (this + package already depends on the CML library that could do it) but is not wired up yet. A + mismatched pair still constructs and signs without error. The failure surfaces later: either + as a chain-level rejection at submission, or as the policy engine failing to recognise your + own change as change and over-counting it as outflow — the safe direction, but confusing if + you hit it without knowing why. +- **Prompt injection is bounded, not eliminated.** Policy refuses out-of-limit + transactions, but an injected agent can still cause any transaction *within* policy. + Recipient allowlisting is the next lever. +- **Pairing this signer with the hosted builder does not yet remove the custody exposure it + is designed to fix.** The signer's own four tools are complete and independently verified + end to end against live chain data (`test/integration/roundtrip.test.ts`). What is not yet + true is a wired-up keyless round trip through the hosted *builder's* actual tools: today + every builder tool that builds or sends a transaction (`vector_send_apex`, + `vector_build_transaction`, the registry and governance tools, ...) still requires your + mnemonic as a call parameter, and no `submit_transaction` tool exists yet to submit a + transaction this signer produced. That migration is tracked separately in + [`docs/architecture/non-custodial-split.md`](../../docs/architecture/non-custodial-split.md) + (PRs 6-8) and has not shipped. Until it does, the root README's custody notice still + describes the hosted builder accurately. + +## Running it + +```bash +npm run build +VECTOR_SIGNER_MNEMONIC_FILE=/path/to/mnemonic.txt node packages/signer/build/index.js +``` + +Or registered directly in an MCP client (Claude Desktop's `claude_desktop_config.json`, for +example): + +```json +{ + "mcpServers": { + "vector-signer": { + "command": "node", + "args": ["/path/to/mcp-server/packages/signer/build/index.js"], + "env": { + "VECTOR_SIGNER_MNEMONIC_FILE": "/path/to/mnemonic.txt" + } + } + } +} +``` + +Register it alongside the hosted builder so an agent has both sets of tools available. See the +last bullet under Known limitations above before expecting that pairing to keep your mnemonic +off the hosted server today — the signer half is ready; the builder half is not yet. + +## Testing + +```bash +npm run test:unit # pure logic, no wallet, no network (from the repo root) +npm run test:smoke:signer # builds the signer, boots it over stdio, exercises all 4 tools +``` + +Both run in CI on every PR. + +```bash +export VECTOR_SIGNER_MNEMONIC_FILE=/absolute/path/to/testnet-mnemonic.txt +export NODE_OPTIONS=--max-old-space-size=12288 +npm run test:integration:signer +``` + +Requires a **funded Vector testnet** wallet. Builds a real unsigned transaction the way the +hosted builder does (no key), then decodes, policy-checks, and signs it with the real signer +code path against live chain data. **Never submits.** Not run in CI — see +`test/integration/roundtrip.test.ts`. diff --git a/packages/signer/test/integration/roundtrip.test.ts b/packages/signer/test/integration/roundtrip.test.ts new file mode 100644 index 0000000..8d8fa73 --- /dev/null +++ b/packages/signer/test/integration/roundtrip.test.ts @@ -0,0 +1,79 @@ +// Opt-in. Requires a funded Vector testnet wallet at the path in +// VECTOR_SIGNER_MNEMONIC_FILE. Builds a real unsigned transaction keylessly, +// has the signer decode and sign it, and verifies the witness. Never submits. +// +// Excluded from CI (no test:integration:signer step there) because it needs a +// funded wallet. Run explicitly: see README "Testing" section. +// +// This file lives under test/, not src/ — boundary.test.ts only scans src/ +// (see its own SRC_DIR constant), so the network-capable imports below +// (@lucid-evolution/lucid, the shared package's /provider subpath) do not +// trip the signer's no-network guarantee. Those imports exist ONLY to +// reproduce, from the test's own side, exactly what the hosted *builder* +// does when it constructs a transaction with no key — everything from +// MnemonicKeySource onward exercises the real signer code path. +import { describe, test, before } from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { Lucid } from '@lucid-evolution/lucid'; +import { OgmiosProvider } from '@apexfusion/vector-mcp-shared/provider'; +import * as CML from '@anastasia-labs/cardano-multiplatform-lib-nodejs'; +import { MnemonicKeySource } from '../../src/keysource.ts'; +import { decodeTransaction } from '../../src/decode.ts'; +import { evaluate } from '../../src/policy.ts'; +import { signTransaction } from '../../src/sign.ts'; + +const FOREIGN = 'addr1wx434t2jc3m5uhdf7tq05xjdqu3q5z7a2lhrmn5mapsd43srh7ll8'; + +let unsignedCbor: string; +let identity: { address: string; privateKeyBech32: string }; + +before(async () => { + const path = process.env.VECTOR_SIGNER_MNEMONIC_FILE; + assert.ok(path, 'set VECTOR_SIGNER_MNEMONIC_FILE to a funded testnet wallet'); + identity = new MnemonicKeySource(readFileSync(path!, 'utf8')).load(); + + const lucid = await Lucid( + new OgmiosProvider({ + ogmiosUrl: 'https://ogmios.vector.testnet.apexfusion.org', + submitUrl: 'https://submit.vector.testnet.apexfusion.org/api/submit/tx', + koiosUrl: 'https://v2.koios.vector.testnet.apexfusion.org/', + }), + 'Mainnet' + ); + const utxos = await lucid.utxosAt(identity.address); + assert.ok(utxos.length > 0, `wallet ${identity.address} has no UTxOs - fund it`); + + // Build with NO key, exactly as the hosted builder will. + lucid.selectWallet.fromAddress(identity.address, utxos); + unsignedCbor = (await lucid.newTx().pay.ToAddress(FOREIGN, { lovelace: 3_000_000n }).complete()).toCBOR(); +}); + +describe('signer round trip against Vector testnet', () => { + test('the keyless build produced a decodable transaction', () => { + assert.ok(decodeTransaction(unsignedCbor).outputs.length >= 1); + }); + + test('net outflow is the payment plus the fee, with change excluded', () => { + const tx = decodeTransaction(unsignedCbor); + const { netOutflowLovelace } = evaluate( + tx, [identity.address], + { perTxLovelace: 100_000_000n, dailyLovelace: 500_000_000n }, 0n + ); + assert.equal(netOutflowLovelace, 3_000_000n + tx.fee); + }); + + test('policy allows it and signing attaches a verifying witness', () => { + const tx = decodeTransaction(unsignedCbor); + const decision = evaluate( + tx, [identity.address], + { perTxLovelace: 100_000_000n, dailyLovelace: 500_000_000n }, 0n + ); + assert.equal(decision.allowed, true, decision.reason); + + const { signedCborHex, txHashHex } = signTransaction(unsignedCbor, identity.privateKeyBech32); + const w = CML.Transaction.from_cbor_hex(signedCborHex).witness_set().vkeywitnesses()!.get(0); + assert.equal(w.vkey().verify(CML.TransactionHash.from_hex(txHashHex).to_raw_bytes(), w.ed25519_signature()), true); + assert.equal(txHashHex, tx.txHashHex, 'signing must not alter the body'); + }); +}); From 20dba15d63fb714374ef42f39557064296286594 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:30:43 +0200 Subject: [PATCH 23/27] fix(signer): gitignore the signer's audit log, matching the builder's vector-signer-audit-log.json resolves inside the repo checkout by default, and the README self-disclosed that it was NOT in .gitignore (unlike the builder's vector-audit-log.json). A documented footgun is worse than no footgun when the fix is one line. Adds the entry and updates the README paragraph that described the gap to describe the fix instead. Final whole-branch review, Fix 1 of 4. Co-Authored-By: Claude Opus 5 --- .gitignore | 1 + packages/signer/README.md | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 8b089dc..39c2b9e 100644 --- a/.gitignore +++ b/.gitignore @@ -38,6 +38,7 @@ mnemonic.txt # Runtime state (contains transaction history) vector-audit-log.json +vector-signer-audit-log.json # Incremental typecheck cache (npm run typecheck uses --incremental) *.tsbuildinfo diff --git a/packages/signer/README.md b/packages/signer/README.md index 2121fc7..47c7f52 100644 --- a/packages/signer/README.md +++ b/packages/signer/README.md @@ -76,10 +76,11 @@ Optional: directories up from `build/index.js` — the monorepo root, in this repo), **not** against whatever directory the process happened to be started from. That makes the location predictable regardless of your MCP client's working directory, but it also means the file is -easy to forget about. In particular: unlike the builder's `vector-audit-log.json`, this -filename is **not currently listed in this repo's `.gitignore`**. If you run the signer from a -checkout of this repo without setting `VECTOR_SIGNER_AUDIT_LOG_PATH` explicitly, check before -you `git add` — or just set the variable and point it somewhere outside the repo. +easy to forget about. Like the builder's `vector-audit-log.json`, this exact filename is listed +in this repo's `.gitignore`, so a default-configured signer run from a checkout will not get its +audit log accidentally staged. That entry only matches the default filename, though — if you +point `VECTOR_SIGNER_AUDIT_LOG_PATH` at some other path inside the repo, check before you +`git add`, or just set the variable to somewhere outside the repo entirely. ## Tools From bec758302aca954d2bff860921fb083c319830f5 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:30:58 +0200 Subject: [PATCH 24/27] fix(signer): reject a non-integer or negative VECTOR_SIGNER_ACCOUNT_INDEX Number(env.VECTOR_SIGNER_ACCOUNT_INDEX ?? '0') was never validated. Verified against the real walletFromSeed: a NaN-producing value, a negative integer, and a fractional value each derived a DIFFERENT, syntactically valid addr1... wallet with no error anywhere in the chain, and describe() would echo the nonsense value back as if nothing were wrong ("mnemonic (account index NaN)"). Spend limits three lines away already fail loud on the same shape of mistake (BigInt(...) throws on garbage) -- this applies that same instinct here: parseAccountIndex() rejects anything that is not a non-negative integer, naming the variable and the offending value. Only called from the two MnemonicKeySource-constructing branches, not unconditionally -- PrivateKeyKeySource never reads an account index, so a leftover garbage VECTOR_SIGNER_ACCOUNT_INDEX must not block a private-key-configured signer that never uses it. Pinned by its own test. config.test.ts covers: NaN-producing input, a negative integer, a fractional value, a valid value (cross-checked against a directly-constructed MnemonicKeySource for the same wallet), the unset default, and the PrivateKeyKeySource non-interference case. Verified by mutation (temporarily disabling the guard) that exactly the three invalid-input tests fail and the other three are unaffected -- see the task report. Final whole-branch review, Fix 2 of 4. Co-Authored-By: Claude Opus 5 --- packages/signer/src/config.ts | 35 +++++++++++-- packages/signer/test/config.test.ts | 77 +++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 packages/signer/test/config.test.ts diff --git a/packages/signer/src/config.ts b/packages/signer/src/config.ts index b0974dd..616fa13 100644 --- a/packages/signer/src/config.ts +++ b/packages/signer/src/config.ts @@ -35,13 +35,11 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): SignerConfig { } function resolveKeySource(env: NodeJS.ProcessEnv): KeySource { - const accountIndex = Number(env.VECTOR_SIGNER_ACCOUNT_INDEX ?? '0'); - if (env.VECTOR_SIGNER_PRIVATE_KEY && env.VECTOR_SIGNER_ADDRESS) { return new PrivateKeyKeySource(env.VECTOR_SIGNER_PRIVATE_KEY, env.VECTOR_SIGNER_ADDRESS); } if (env.VECTOR_SIGNER_MNEMONIC) { - return new MnemonicKeySource(env.VECTOR_SIGNER_MNEMONIC, accountIndex); + return new MnemonicKeySource(env.VECTOR_SIGNER_MNEMONIC, parseAccountIndex(env.VECTOR_SIGNER_ACCOUNT_INDEX)); } if (env.VECTOR_SIGNER_MNEMONIC_FILE) { let contents: string; @@ -53,7 +51,7 @@ function resolveKeySource(env: NodeJS.ProcessEnv): KeySource { `Could not read VECTOR_SIGNER_MNEMONIC_FILE (${env.VECTOR_SIGNER_MNEMONIC_FILE}): ${err instanceof Error ? err.message : String(err)}` ); } - return new MnemonicKeySource(contents, accountIndex); + return new MnemonicKeySource(contents, parseAccountIndex(env.VECTOR_SIGNER_ACCOUNT_INDEX)); } throw new Error( @@ -61,3 +59,32 @@ function resolveKeySource(env: NodeJS.ProcessEnv): KeySource { 'VECTOR_SIGNER_MNEMONIC, or VECTOR_SIGNER_PRIVATE_KEY together with VECTOR_SIGNER_ADDRESS.' ); } + +// Number(x) returns NaN for garbage input rather than throwing, and nothing +// downstream re-validates it: MnemonicKeySource's constructor takes +// accountIndex on faith (its own validation is about the mnemonic, not this +// argument), and walletFromSeed's HD derivation does not throw on a NaN, +// negative, or fractional index either -- it just walks a different (but +// still syntactically valid-looking) branch of the tree. A single env-var +// typo would otherwise silently start the signer against a different +// wallet, with no error anywhere in the chain and describe() printing the +// nonsense value back out as if nothing were wrong (e.g. "mnemonic (account +// index NaN)"). This file's own spend limits, a few lines up, already fail +// loud on the same shape of mistake (BigInt(...) throws on non-numeric +// input) -- this applies that same instinct here. +// +// Only called from the two MnemonicKeySource-constructing branches above, +// not unconditionally: PrivateKeyKeySource never uses an account index (see +// its own doc comment in keysource.ts), so a leftover/garbage +// VECTOR_SIGNER_ACCOUNT_INDEX must not block startup for a config that never +// reads it. +function parseAccountIndex(raw: string | undefined): number { + const source = raw ?? '0'; + const accountIndex = Number(source); + if (!Number.isInteger(accountIndex) || accountIndex < 0) { + throw new Error( + `Invalid VECTOR_SIGNER_ACCOUNT_INDEX: expected a non-negative integer, got "${source}".` + ); + } + return accountIndex; +} diff --git a/packages/signer/test/config.test.ts b/packages/signer/test/config.test.ts new file mode 100644 index 0000000..7da503f --- /dev/null +++ b/packages/signer/test/config.test.ts @@ -0,0 +1,77 @@ +import { describe, test } from 'node:test'; +import assert from 'node:assert/strict'; +import { loadConfig } from '../src/config.ts'; +import { MnemonicKeySource } from '../src/keysource.ts'; + +// Throwaway test mnemonic, shared with keysource.test.ts and sign.test.ts. +// Controls no funds. Never use outside tests. +const TEST_MNEMONIC = + 'test walk nut penalty hip pave soap entry language right filter choice'; + +function envWithMnemonic(accountIndex?: string): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { VECTOR_SIGNER_MNEMONIC: TEST_MNEMONIC }; + if (accountIndex !== undefined) env.VECTOR_SIGNER_ACCOUNT_INDEX = accountIndex; + return env; +} + +// VECTOR_SIGNER_ACCOUNT_INDEX used to go straight into `Number(...)` with no +// validation anywhere downstream. Number() returns NaN for garbage input +// rather than throwing; MnemonicKeySource's constructor takes accountIndex +// on faith (its own validation is about the mnemonic, not this argument); +// and CML's HD derivation (walletFromSeed) does not throw on a NaN, +// negative, or fractional index either — it just walks a different branch +// of the tree. Verified directly against the real walletFromSeed before +// this fix (see the task report): a NaN-producing string, a negative +// integer, and a fractional value each derived a DIFFERENT, syntactically +// valid addr1... address, with no error anywhere in the chain — the +// "confident wrong answer" shape, not a crash. describe() would even echo +// the nonsense value back as if nothing were wrong (e.g. "mnemonic (account +// index NaN)"). These tests pin the fix: anything that is not a +// non-negative integer must throw, loudly, naming both the variable and the +// offending value. +describe('loadConfig — VECTOR_SIGNER_ACCOUNT_INDEX validation', () => { + test('a non-numeric value (Number(...) produces NaN) is rejected, not silently treated as account 0', () => { + assert.throws( + () => loadConfig(envWithMnemonic('not-a-number')), + /VECTOR_SIGNER_ACCOUNT_INDEX.*not-a-number/ + ); + }); + + test('a negative integer is rejected', () => { + assert.throws(() => loadConfig(envWithMnemonic('-1')), /VECTOR_SIGNER_ACCOUNT_INDEX.*-1/); + }); + + test('a fractional value is rejected', () => { + assert.throws(() => loadConfig(envWithMnemonic('1.5')), /VECTOR_SIGNER_ACCOUNT_INDEX.*1\.5/); + }); + + test('a valid non-negative integer is accepted and derives the SAME wallet as constructing MnemonicKeySource directly', () => { + // Not just "does not throw" — confirms the validated value actually + // reaches derivation unchanged, by cross-checking against an + // independently constructed MnemonicKeySource for the same index. + const viaConfig = loadConfig(envWithMnemonic('2')).keySource.load(); + const direct = new MnemonicKeySource(TEST_MNEMONIC, 2).load(); + assert.equal(viaConfig.address, direct.address); + assert.equal(viaConfig.privateKeyBech32, direct.privateKeyBech32); + }); + + test('an unset VECTOR_SIGNER_ACCOUNT_INDEX still defaults to account 0, unaffected by the new validation', () => { + const viaConfig = loadConfig(envWithMnemonic()).keySource.load(); + const direct = new MnemonicKeySource(TEST_MNEMONIC, 0).load(); + assert.equal(viaConfig.address, direct.address); + }); + + test('a garbage VECTOR_SIGNER_ACCOUNT_INDEX does not block a PRIVATE_KEY-configured signer, since that path never reads it', () => { + // parseAccountIndex() is only called from the two MnemonicKeySource + // branches, not unconditionally at the top of resolveKeySource — a + // leftover/mistyped VECTOR_SIGNER_ACCOUNT_INDEX in the environment must + // not newly block startup for a config that never uses it. + const derived = new MnemonicKeySource(TEST_MNEMONIC).load(); + const env: NodeJS.ProcessEnv = { + VECTOR_SIGNER_PRIVATE_KEY: derived.privateKeyBech32, + VECTOR_SIGNER_ADDRESS: derived.address, + VECTOR_SIGNER_ACCOUNT_INDEX: 'not-a-number', + }; + assert.doesNotThrow(() => loadConfig(env)); + }); +}); From c464b5b0c5f6160a3a47b08ed11e3b9a3d4c09f8 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:31:50 +0200 Subject: [PATCH 25/27] test(signer): pin decode.ts's asset-name unit construction at the 24-byte CBOR-header boundary decode.ts's readAssets() builds each asset's `unit` as `policy.to_hex() + name.to_hex()`, deliberately not `name.to_cbor_hex().slice(2)` -- the CBOR byte-string header is 1 byte for names up to 23 bytes but grows to 2 bytes at 24 (Cardano asset names run up to 32 bytes), so a fixed slice(2) leaves the extra header byte glued onto the front of the unit for every 24-32 byte name. That fix shipped with this module's first commit but was only ever verified by a throwaway probe -- nothing in the suite constructed a real CML multi-asset output, across 8 tasks and 22 commits. Builds a real TransactionOutput carrying a MultiAsset value with three deterministic asset names (23 bytes, exactly 24, and the 32-byte max) under one policy, re-serialised through Transaction.new(...) so the fixture exercises decodeTransaction's full parse path, not just readAssets() in isolation. Asserts the decoded unit is exactly policyHex + assetNameHex. Verified by mutation (reverting to name.to_cbor_hex().slice(2)): the 24-byte and 32-byte tests fail with the corrupted unit carrying a stray leading header byte (18.../20... prefix); the 23-byte test is correctly unaffected, since both forms agree below the boundary -- see the task report. Final whole-branch review, Fix 3a of 4. Co-Authored-By: Claude Opus 5 --- packages/signer/test/decode.test.ts | 87 +++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/packages/signer/test/decode.test.ts b/packages/signer/test/decode.test.ts index f36a54d..d62a30d 100644 --- a/packages/signer/test/decode.test.ts +++ b/packages/signer/test/decode.test.ts @@ -97,3 +97,90 @@ describe('decodeTransaction', () => { }); }); }); + +// Fix, verified once by a throwaway probe and never pinned by a permanent +// test until now (see the task report): readAssets() builds each asset's +// `unit` string as `policy.to_hex() + name.to_hex()`, specifically NOT +// `name.to_cbor_hex().slice(2)`. name.to_cbor_hex() carries a CBOR +// byte-string header that is 1 byte for names up to 23 bytes long but grows +// to 2 bytes at 24 (Cardano asset names run up to 32 bytes) — a fixed +// slice(2) strips only the first form correctly and leaves the second +// header byte glued onto the front of the "unit" for every 24-32 byte name, +// silently corrupting it. to_hex() returns the raw bytes with no CBOR +// envelope at all, so it is correct at every length. Nothing in the rest of +// this suite constructs a real multi-asset output, so this was previously +// completely unexercised. +describe('native asset units — asset names at and beyond the 24-byte CBOR-header boundary', () => { + // Deterministic, not random: sequential bytes offset by a per-fixture seed + // so the three names never collide and a failure is reproducible from + // this description alone. 28 bytes for the policy ID because ScriptHash + // (a Cardano policy ID) is always exactly 28 bytes, never variable. + function hexOfLength(byteLen: number, seed: number): string { + return Array.from({ length: byteLen }, (_, i) => ((i + seed) % 256).toString(16).padStart(2, '0')).join(''); + } + + const POLICY_HEX = hexOfLength(28, 0); + // Just under the boundary (1-byte CBOR header either way) — confirms the + // fix is not merely coincidentally right only at 24+ while accidentally + // still being right below it for some other reason. + const NAME_23 = hexOfLength(23, 1); + // Exactly the length at which CBOR's byte-string header grows from 1 byte + // to 2 — the precise boundary the reverted `to_cbor_hex().slice(2)` form + // corrupts. + const NAME_24 = hexOfLength(24, 50); + // The maximum asset name length Cardano allows. + const NAME_32 = hexOfLength(32, 100); + + // Real CML MultiAsset construction, not a hand-rolled CBOR fixture: builds + // one output on top of FIXTURE_CBOR's own body/inputs/fee carrying all + // three asset names under one policy, then re-serialises through + // Transaction.new(...).to_cbor_hex() so this exercises decodeTransaction's + // full parse path, including its trailing-bytes consumption check, not + // just readAssets() in isolation. + function fixtureWithAssets(): string { + const unsigned = CML.Transaction.from_cbor_hex(FIXTURE_CBOR); + const body = unsigned.body(); + const existingOutput = body.outputs().get(0); + + const policy = CML.ScriptHash.from_hex(POLICY_HEX); + const assets = CML.MapAssetNameToCoin.new(); + assets.insert(CML.AssetName.from_hex(NAME_23), 1_000n); + assets.insert(CML.AssetName.from_hex(NAME_24), 2_000n); + assets.insert(CML.AssetName.from_hex(NAME_32), 3_000n); + const multiasset = CML.MultiAsset.new(); + multiasset.insert_assets(policy, assets); + const value = CML.Value.new(3_000_000n, multiasset); + + const outputWithAssets = CML.TransactionOutput.new(existingOutput.address(), value, undefined, undefined); + const outputs = CML.TransactionOutputList.new(); + outputs.add(outputWithAssets); + const newBody = CML.TransactionBody.new(body.inputs(), outputs, body.fee()); + const tx = CML.Transaction.new(newBody, CML.TransactionWitnessSet.new(), true, undefined); + return tx.to_cbor_hex(); + } + + test('a 23-byte name (just under the boundary) decodes to policyHex + assetNameHex exactly', () => { + const { outputs } = decodeTransaction(fixtureWithAssets()); + const unit = outputs[0].assets.find((a) => a.quantity === 1_000n)?.unit; + assert.equal(unit, `${POLICY_HEX}${NAME_23}`); + }); + + test('a 24-byte name (exactly at the boundary) decodes to policyHex + assetNameHex exactly, not with a stray leading CBOR header byte', () => { + const { outputs } = decodeTransaction(fixtureWithAssets()); + const unit = outputs[0].assets.find((a) => a.quantity === 2_000n)?.unit; + assert.equal(unit, `${POLICY_HEX}${NAME_24}`); + }); + + test('a 32-byte name (the maximum Cardano allows) decodes to policyHex + assetNameHex exactly', () => { + const { outputs } = decodeTransaction(fixtureWithAssets()); + const unit = outputs[0].assets.find((a) => a.quantity === 3_000n)?.unit; + assert.equal(unit, `${POLICY_HEX}${NAME_32}`); + }); + + test('all three assets are reported on the one output that carries them, with their quantities intact', () => { + const { outputs } = decodeTransaction(fixtureWithAssets()); + assert.equal(outputs[0].assets.length, 3); + const quantities = outputs[0].assets.map((a) => a.quantity).sort(); + assert.deepEqual(quantities, [1_000n, 2_000n, 3_000n]); + }); +}); From 77d63f15ef884a7809934fbf3aa2980f72bde302 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:32:42 +0200 Subject: [PATCH 26/27] test(signer): port sign.test.ts's indefinite-length CBOR fixture to decode.test.ts decode.ts's trailing-bytes consumption check re-serialises a parsed transaction and compares it against the input, refusing on any mismatch. That check relies on CML's parse-then-reserialise round trip preserving the agent registry's indefinite-length CBOR constructor encoding (D8 79 9F ... FF, required by the Conway-era Aiken validators; Lucid's Data.to emits the definite-length form instead, which those validators reject) rather than silently canonicalising it -- if it did not, the consumption check would misread that harmless re-encoding as unrecognised trailing data and refuse every transaction using this ecosystem's own real encoding. sign.test.ts already pins this exact claim for sign.ts's own, independent from_cbor_hex call, and its own header comment flagged decode.ts as having "the exact same untested gap" -- out of that task's file scope, left for a later task. This is that closure: the same fixture (built with the same cborBytes/cborUint helpers, embedded both as a body datum and a witness-set datum) ported to exercise decodeTransaction directly. Verified by mutation (swapping the comparison to tx.to_canonical_cbor_hex()): probed directly first and confirmed canonical re-serialisation really does rewrite this fixture's bytes, then confirmed exactly the 4 tests that call decodeTransaction() on it fail, while the sanity test and the byte-survival test (neither of which call decodeTransaction) are unaffected -- see the task report. Final whole-branch review, Fix 3b of 4. Co-Authored-By: Claude Opus 5 --- packages/signer/test/decode.test.ts | 135 ++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/packages/signer/test/decode.test.ts b/packages/signer/test/decode.test.ts index d62a30d..56a0b2d 100644 --- a/packages/signer/test/decode.test.ts +++ b/packages/signer/test/decode.test.ts @@ -2,6 +2,15 @@ import { describe, test } from 'node:test'; import assert from 'node:assert/strict'; import * as CML from '@anastasia-labs/cardano-multiplatform-lib-nodejs'; import { decodeTransaction } from '../src/decode.ts'; +// /tx is an explicitly allowed pure subpath (boundary.test.ts's own +// ALLOWLIST — /tx and /types are pure, /provider is network-capable). Used +// below to build the exact same indefinite-length CBOR constructor shape +// deriveNftAssetName() builds, via the same helper functions sign.test.ts +// already uses for the identical fixture, rather than a hand-rolled +// reimplementation that could subtly diverge from the real encoding this +// ecosystem actually produces. Test-only import — decode.ts itself imports +// nothing beyond CML. +import { cborBytes, cborUint } from '@apexfusion/vector-mcp-shared/tx'; // Real unsigned transaction from Vector testnet. 1 input, 2 outputs // (one foreign, one change back to OWN), fee 168223. @@ -184,3 +193,129 @@ describe('native asset units — asset names at and beyond the 24-byte CBOR-head assert.deepEqual(quantities, [1_000n, 2_000n, 3_000n]); }); }); + +// Ported from sign.test.ts, which covers this exact shape for sign.ts's own +// (separate) from_cbor_hex call. decode.ts runs the identical +// re-serialise-and-compare consumption check over the same encodings — see +// the long comment above decodeTransaction() in decode.ts — but had no test +// of its own exercising it against indefinite-length CBOR; sign.test.ts's +// own header comment flagged this as an out-of-scope gap for a later task to +// close. This is that closure. +describe('indefinite-length CBOR — the agent registry\'s D8 79 9F … FF constructor encoding', () => { + // packages/shared/src/tx.ts's deriveNftAssetName() documents that the + // agent registry's Conway-era Aiken validators require INDEFINITE-length + // CBOR for a Plutus constructor's field array (D8 79 9F … FF) — Lucid's + // Data.to instead emits definite-length arrays (D8 79 82 …), which those + // validators reject. decode.ts's consumption check asserts in a comment + // that CML's parse-then-reserialise round trip preserves this exact + // encoding rather than silently canonicalising it to definite-length — + // which matters because if it did NOT, the consumption check would misread + // that harmless re-encoding as "unrecognised trailing data" and refuse to + // decode every transaction using this ecosystem's own real encoding. That + // claim was asserted only in prose until now; this fixture makes it an + // executable, permanent check for decode.ts specifically (sign.test.ts + // already covers the equivalent claim for sign.ts's own, independent + // from_cbor_hex call). + // + // Built using the SAME helper functions (cborBytes, cborUint) that + // packages/shared/src/tx.ts's deriveNftAssetName() itself uses to build its + // OutputReference constructor, for maximum fidelity to the real encoding. + function indefiniteLengthConstructor(): Buffer { + // Same shape as deriveNftAssetName's outRefCbor: tag 121 (Plutus + // constructor index 0), indefinite-length array open, one bytestring + // field (reusing FIXTURE_CBOR's own input tx hash, so this is a + // realistic 32-byte field rather than arbitrary filler), one small uint + // field, indefinite-length array close. + const someHash32 = 'e17a2ebab8eae3850f959290041b3bef3f3597584f8cf4a728e14777dddb8c3'; + return Buffer.concat([ + Buffer.from([0xd8, 0x79, 0x9f]), + cborBytes(someHash32), + cborUint(0n), + Buffer.from([0xff]), + ]); + } + + test('sanity: the hand-rolled bytes are actually the D8 79 9F … FF shape under test', () => { + const hex = indefiniteLengthConstructor().toString('hex'); + assert.ok(hex.startsWith('d8799f'), 'must open with tag 121 + indefinite-length array'); + assert.ok(hex.endsWith('ff'), 'must close with the indefinite-length-array break byte'); + }); + + describe('embedded in the transaction BODY (an inline datum on an output)', () => { + // Body-embedded specifically because this is the placement where the + // encoding actually participates in the body hash: if CML's round trip + // ever DID mangle this encoding, this is the fixture that would show up + // as a changed hash, not just a rejected consumption check. + function fixtureWithBodyEmbeddedDatum(): string { + const unsigned = CML.Transaction.from_cbor_hex(FIXTURE_CBOR); + const body = unsigned.body(); + const datum = CML.PlutusData.from_cbor_hex(indefiniteLengthConstructor().toString('hex')); + const existingOutput = body.outputs().get(0); + const outputWithDatum = CML.TransactionOutput.new( + existingOutput.address(), + existingOutput.amount(), + CML.DatumOption.new_datum(datum), + undefined + ); + const outputs = CML.TransactionOutputList.new(); + outputs.add(outputWithDatum); + const newBody = CML.TransactionBody.new(body.inputs(), outputs, body.fee()); + const tx = CML.Transaction.new(newBody, CML.TransactionWitnessSet.new(), true, undefined); + return tx.to_cbor_hex(); + } + + test('decodeTransaction accepts it — no false-positive "unrecognised trailing data" rejection', () => { + const cborHex = fixtureWithBodyEmbeddedDatum(); + assert.doesNotThrow(() => decodeTransaction(cborHex)); + }); + + test('the reported hash matches an independent hash_transaction call over the same body', () => { + const cborHex = fixtureWithBodyEmbeddedDatum(); + const { txHashHex } = decodeTransaction(cborHex); + const independentHash = CML.hash_transaction(CML.Transaction.from_cbor_hex(cborHex).body()).to_hex(); + assert.equal(txHashHex, independentHash); + }); + + test('the indefinite-length datum itself survives decode-then-reserialise byte-for-byte', () => { + const cborHex = fixtureWithBodyEmbeddedDatum(); + // decodeTransaction() only returns plain data, not CBOR — this checks + // the same property sign.test.ts pins for sign.ts's output by going + // through CML directly on decode.ts's own accepted input, confirming + // the encoding decode.ts just validated really is byte-identical to + // what was supplied, not merely "close enough to parse". + const survivingDatum = CML.Transaction.from_cbor_hex(cborHex).body().outputs().get(0).datum()!.as_datum()!; + assert.equal(survivingDatum.to_cbor_hex(), indefiniteLengthConstructor().toString('hex')); + }); + }); + + describe('embedded in the WITNESS SET (a plutus_datums entry)', () => { + // Unlike the body-embedded case, this can never affect the body hash + // (plutus_datums lives on TransactionWitnessSet, not TransactionBody) — + // it instead exercises the OTHER risk: the consumption check compares + // the WHOLE transaction's re-serialised CBOR, witness set included, so a + // mangled round trip here would still cause a false-positive rejection + // even though it could never show up as a changed hash. + function fixtureWithWitnessEmbeddedDatum(): string { + const unsigned = CML.Transaction.from_cbor_hex(FIXTURE_CBOR); + const body = unsigned.body(); + const datum = CML.PlutusData.from_cbor_hex(indefiniteLengthConstructor().toString('hex')); + const witnessSet = CML.TransactionWitnessSet.new(); + const datums = CML.PlutusDataList.new(); + datums.add(datum); + witnessSet.set_plutus_datums(datums); + const tx = CML.Transaction.new(body, witnessSet, true, undefined); + return tx.to_cbor_hex(); + } + + test('decodeTransaction accepts it — no false-positive "unrecognised trailing data" rejection', () => { + const cborHex = fixtureWithWitnessEmbeddedDatum(); + assert.doesNotThrow(() => decodeTransaction(cborHex)); + }); + + test('the body hash is unchanged (same body as FIXTURE_CBOR — only the witness set differs)', () => { + const cborHex = fixtureWithWitnessEmbeddedDatum(); + const { txHashHex } = decodeTransaction(cborHex); + assert.equal(txHashHex, decodeTransaction(FIXTURE_CBOR).txHashHex); + }); + }); +}); From a56e70deae99000bae1ac327cf3e7d87b69cebc0 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:33:14 +0200 Subject: [PATCH 27/27] fix(signer): harden the boundary check's globalThis blind spot, and stop overstating it The review verified six obfuscations that bypass boundary.test.ts's ambient- global and forbidden-import checks: globalThis['fetch'](...), an alias (const f = fetch; f(...)), an aliased bracket reference, the comma-operator trick ((0, globalThis.fetch)(...)), and two dynamic import()s with a concatenated specifier (import('node:' + 'dns'), import('child_' + 'process')). README.md called the check "the package's central security guarantee" and boundary.test.ts's header claimed the signer "CANNOT reach the network -- not that it chooses not to." Neither line was true of what a text scan can actually promise. Hardened cheaply, within reach of a text scan: - FORBIDDEN_IMPORTS gains node:dns, node:dns/promises, node:http2, node:worker_threads, node:vm, each with a one-line rationale. - AMBIENT_NETWORK_GLOBALS gains globalThisQualifiedAccess(name), matching a global reached via globalThis in either dot or bracket ("computed member access") notation, plus navigator['sendBeacon'] bracket notation. Verified empirically against the real hardened check (temporary probe files under src/, one per evasion, deleted immediately after -- see the task report for the full per-file breakdown): globalThis['fetch'](...), the aliased-bracket-reference variant, and the comma-operator globalThis.fetch trick are now all caught. The plain-identifier alias and both concatenated dynamic imports still pass -- those need real data-flow tracking or evaluating an arbitrary runtime expression, neither of which a text scan can do, and no attempt was made to chase them. Reworded both the README section and boundary.test.ts's header comment to say what the check actually verifies: today's shipped src/ has no accidental or careless network capability. Deliberate obfuscation is named as a code-review responsibility, not something this file promises to catch. Final whole-branch review, Fix 4 of 4. Co-Authored-By: Claude Opus 5 --- packages/signer/README.md | 18 +++++--- packages/signer/test/boundary.test.ts | 65 +++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/packages/signer/README.md b/packages/signer/README.md index 47c7f52..7dd30d5 100644 --- a/packages/signer/README.md +++ b/packages/signer/README.md @@ -12,13 +12,21 @@ provider. - **no Provider and no Lucid instance** — signing uses CML directly - **submission lives on the builder**, so the signer needs no egress either -`test/boundary.test.ts` enforces this over both the import graph and the manifest — it walks -every file under `src/`, checks `package.json` for network-capable dependencies, and greps for -ambient network globals (`fetch`, `WebSocket`, ...). It is the package's central security -guarantee. +`test/boundary.test.ts` checks this mechanically — it walks every file under `src/` for a +network-capable import or an ambient network global (`fetch`, `WebSocket`, ...), and checks +`package.json` for a network-capable dependency. That check is a text scan, not a sandbox, and +it is scoped accordingly: it reliably catches *accidental* regressions and *casual* misuse — an +added dependency, a forgotten forbidden import, an unthinking `fetch(...)` call, including one +routed around the bare identifier via `globalThis['fetch']` or `globalThis.fetch`. It does not +catch *deliberate* obfuscation. Verified directly against the hardened check: an ambient global +reached through a plain alias (`const f = fetch; f(...)`) or a forbidden module specifier +assembled at runtime from concatenated strings (`import('node:' + 'dns')`) both still pass. +Closing gaps like those is a code-review responsibility, not something a fixed set of text +patterns can guarantee — what this check actually verifies is that today's shipped `src/` has no +*accidental or careless* network capability, not a proof that none could ever be smuggled in. `test/integration/roundtrip.test.ts` imports `@lucid-evolution/lucid` and the shared package's -`/provider` — that does **not** weaken the guarantee above. That file lives under `test/`, +`/provider` — that does **not** weaken the check above. That file lives under `test/`, which `boundary.test.ts` deliberately scans only as far as `src/`, and which the build never bundles (`tsup`'s only entry point is `src/index.ts`). Those imports exist so the test can build a real transaction the way the hosted *builder* does (no key) before handing it to the diff --git a/packages/signer/test/boundary.test.ts b/packages/signer/test/boundary.test.ts index 91f7c03..39d545d 100644 --- a/packages/signer/test/boundary.test.ts +++ b/packages/signer/test/boundary.test.ts @@ -3,9 +3,23 @@ import assert from 'node:assert/strict'; import { readFileSync, readdirSync } from 'node:fs'; import { join } from 'node:path'; -// The signer's security claim is that it CANNOT reach the network — not that it -// chooses not to. These assertions are that claim, mechanically enforced. -// Weakening them defeats the purpose of the package. +// This is a text scan over src/'s import graph, package.json's declared +// dependencies, and a fixed list of ambient-global patterns — not a sandbox, +// and not a proof that no sequence of JavaScript could ever reach the +// network from this package. What it reliably catches: an accidentally +// added network-capable dependency, a forgotten forbidden import, an +// unthinking `fetch(...)` call — the shape of mistake code review can miss +// but a mechanical check cannot. What it does not catch: deliberate +// obfuscation. A reference to an ambient global reached through an alias +// (`const f = fetch; f(...)`) or a module specifier assembled at runtime +// from concatenated strings (`import('node:' + 'dns')`) defeats a text scan +// by construction — closing every such route is not a fixed-pattern problem, +// so it is not this file's job. Catching deliberate evasion is a code-review +// responsibility; this file's job is to make the accidental and the casual +// loud and immediate. See the README for the specific evasions verified +// against this file. Within that narrower, accurate scope, weakening these +// assertions — narrowing FORBIDDEN_IMPORTS, loosening a pattern, deleting a +// check — still defeats the purpose they do serve. const PKG_DIR = new URL('..', import.meta.url).pathname.replace(/^\/([A-Za-z]:)/, '$1'); const SRC_DIR = join(PKG_DIR, 'src'); @@ -16,13 +30,28 @@ const FORBIDDEN_IMPORTS = [ 'cross-fetch', 'node:http', 'node:https', + // Same network-transport class as http/https above, different protocol + // version. + 'node:http2', 'node:net', 'node:dgram', 'node:tls', + // A DNS lookup is network I/O in its own right, not merely a step before + // some other request — and a well-known covert exfiltration channel (DNS + // tunneling) even with no HTTP client available at all. + 'node:dns', + 'node:dns/promises', // Reaching the network by shelling out (execSync('curl ...')) bypasses every // other assertion here, so the process-spawning modules are forbidden too. 'node:child_process', 'child_process', + // Same reasoning as child_process just above: both are general + // code-execution primitives that can run a separate script (a worker's + // entry file, or a string handed to vm.Script/vm.runInNewContext) outside + // this file's own source text — precisely what an import-graph scan + // cannot see into. + 'node:worker_threads', + 'node:vm', // Opens a debugger port. 'node:inspector', ]; @@ -40,12 +69,42 @@ const ALLOWED_SHARED_SUBPATHS = [`${SHARED_PKG}/tx`, `${SHARED_PKG}/types`]; // scan above is structurally blind to their use. These patterns are matched with // word boundaries so they don't false-positive on legitimate identifiers like a // variable named `prefetchCount`. +// +// Every pattern below this comment requires the global's BARE identifier +// (`fetch`, `WebSocket`, ...) to appear, and most require it to be called or +// constructed immediately. Both are one-line evasions: `globalThis['fetch']` +// never spells the bare word `fetch` followed by `(`, and neither does +// `(0, globalThis.fetch)(...)` — the comma-operator trick (used to strip +// `this` binding so a borrowed method doesn't throw on an unexpected +// receiver) puts a `)` between `fetch` and the call parens that actually +// invoke it. Both still name `globalThis` and the target property, though — +// just via a different route to the same runtime object — so that route is +// checked for directly below: dot notation and bracket notation (the latter +// is literally "computed member access", the formal name for what +// `obj[expr]` is), covering both quoting styles quotedForms() above exists +// to handle. This still only recognises the literal text `globalThis` — a +// further level of indirection (aliasing globalThis to another name first, +// or building the property name from concatenated substrings the way +// FORBIDDEN_IMPORTS's own dynamic-`import()` gap allows, see the README) is +// the kind of arbitrary obfuscation this file does not attempt to defeat. +function globalThisQualifiedAccess(name: string): RegExp { + return new RegExp(`\\bglobalThis\\s*(?:\\.\\s*${name}\\b|\\[\\s*['"\`]${name}['"\`]\\s*\\])`); +} + const AMBIENT_NETWORK_GLOBALS: { label: string; pattern: RegExp }[] = [ { label: 'fetch(...)', pattern: /\bfetch\s*\(/ }, { label: 'new WebSocket', pattern: /\bnew\s+WebSocket\b/ }, { label: 'new EventSource', pattern: /\bnew\s+EventSource\b/ }, { label: 'new XMLHttpRequest', pattern: /\bnew\s+XMLHttpRequest\b/ }, { label: 'navigator.sendBeacon', pattern: /\bnavigator\s*\.\s*sendBeacon\b/ }, + { label: `navigator['sendBeacon']`, pattern: /\bnavigator\s*\[\s*['"`]sendBeacon['"`]\s*\]/ }, + { label: `globalThis.fetch / globalThis['fetch']`, pattern: globalThisQualifiedAccess('fetch') }, + { label: `globalThis.WebSocket / globalThis['WebSocket']`, pattern: globalThisQualifiedAccess('WebSocket') }, + { label: `globalThis.EventSource / globalThis['EventSource']`, pattern: globalThisQualifiedAccess('EventSource') }, + { + label: `globalThis.XMLHttpRequest / globalThis['XMLHttpRequest']`, + pattern: globalThisQualifiedAccess('XMLHttpRequest'), + }, ]; // Any module format that Node can execute, not just .ts — a stray .js or .mjs