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.
This commit is contained in:
FilipBl4gojevic
2026-07-28 12:30:14 +02:00
parent 65a3c00994
commit 9bbd26b04c
2 changed files with 265 additions and 3 deletions

View File

@@ -24,6 +24,16 @@ function ada(lovelace: bigint): string {
return (Number(lovelace) / 1_000_000).toFixed(6); 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. * Net outflow = every output that does NOT return to us, plus the fee.
* *
@@ -35,7 +45,7 @@ export function computeOutflow(
tx: DecodedTx, tx: DecodedTx,
ownAddresses: string[] ownAddresses: string[]
): { netOutflowLovelace: bigint; assetMovements: AssetMovement[] } { ): { netOutflowLovelace: bigint; assetMovements: AssetMovement[] } {
const own = new Set(ownAddresses); const own = new Set(knownAddresses(ownAddresses));
let leaving = 0n; let leaving = 0n;
const assetMovements: AssetMovement[] = []; const assetMovements: AssetMovement[] = [];
@@ -59,8 +69,10 @@ export function evaluate(
const { netOutflowLovelace, assetMovements } = computeOutflow(tx, ownAddresses); const { netOutflowLovelace, assetMovements } = computeOutflow(tx, ownAddresses);
// Fail closed: with no own address, every output looks foreign and change // Fail closed: with no own address, every output looks foreign and change
// cannot be identified, so any decision would be meaningless. // cannot be identified, so any decision would be meaningless. A list
if (ownAddresses.length === 0) { // containing only blank entries counts as no own address too — see
// knownAddresses().
if (knownAddresses(ownAddresses).length === 0) {
return { return {
allowed: false, allowed: false,
reason: 'No own address available, so change cannot be distinguished from outflow. Refusing.', reason: 'No own address available, so change cannot be distinguished from outflow. Refusing.',

View File

@@ -6,6 +6,7 @@ import type { DecodedTx } from '../src/decode.ts';
const OWN = 'addr1own'; const OWN = 'addr1own';
const OTHER_OWN = 'addr1own_second_index'; const OTHER_OWN = 'addr1own_second_index';
const FOREIGN = 'addr1foreign'; const FOREIGN = 'addr1foreign';
const FOREIGN2 = 'addr1foreign_second';
const LIMITS: SpendLimits = { perTxLovelace: 100_000_000n, dailyLovelace: 500_000_000n }; 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); 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');
});
});