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.
This commit is contained in:
@@ -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[],
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user