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'); + }); +});