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