e0e9ee06 - fix(auth): scope the login sign message per environment - #4482
Conversation
A wallet signature proves ownership of the exact text that was signed. That text was identical in every environment, so a signature stored in a lower environment verified on PRD as well. Non-PRD environments now prefix the sign message with their environment marker, which changes the signed payload and therefore the resulting signature. PRD keeps the historical text byte-for-byte, so existing PRD signatures stay valid. An unset or unknown ENVIRONMENT is deliberately treated as non-PRD: it must never accidentally yield a PRD-valid signature. Covered by two specs: one pinning the PRD text byte-for-byte and the fail-closed behaviour, one proving cryptographically that a signature made over the PRD text does not verify under the DEV text and vice versa.
…gerprint The stored user.signature values on DEV were produced with the previous environment-independent sign message and would therefore still verify on PRD. This clears them. CONTRIBUTING.md requires an overwritten value to stay recoverable from the database. That cannot be satisfied literally here, and the conflict is inherent: the values are login credentials, so retaining them recoverably would preserve exactly what this rotation removes. Instead the migration writes an audit row to log before the update, holding the affected user id and an md5 fingerprint of the previous signature rather than its plaintext, and couples the update fail-closed to that insert via EXISTS (SELECT 1 FROM "audit") - following the precedent in 1784600000011-FixFiatOutputValutaDateSerials.js. A known candidate value can be checked against the fingerprint; the credential itself cannot be recovered. md5 acts purely as an audit fingerprint here. down() deliberately restores nothing: the prior value is not reconstructible by design.
The migration spec only matched substrings against a mocked query runner, so an invalid CTE or a broken audit coupling would have passed unnoticed. Adds a MIGRATION_TEST_PG-gated Postgres block, following the pattern already used by most migration specs in this repo. Four scenarios: the rotation writes exactly one audit row whose fingerprints match independently computed md5 values; an empty candidate set writes no audit row and changes nothing; a failing audit insert (log table dropped) rejects and leaves the signature intact, which is the fail-closed property; and the environment gate holds against a real database. Also switches both sign-message specs to absolute imports as required by CONTRIBUTING.md, and states the reason for the require-import lint exception.
…uarantee The previous commit claimed its dropped-log-table test proved the fail-closed property. It did not: because the audit insert and the update are one statement, Postgres rolls the statement back on any insert error, so that test passes even without the EXISTS coupling. Verified by mutation - removing the coupling left it green. Its title now says what it actually shows, statement atomicity. The coupling is now isolated by a test where the insert SUCCEEDS but yields no row: a BEFORE INSERT trigger on log discards it without raising. up() then completes, no audit row exists, and the signature must stay intact. Removing the coupling turns exactly this test red. Also pins the property the audit exception rests on - no plaintext signature reaches the log, asserted negatively against both fixtures and against a before field - narrows the env-scope titles to the text difference they actually assert, since the cryptographic proof lives in the replay spec, and covers an unknown ENVIRONMENT value.
The mock was created but never passed to down(), which takes no query runner, so the assertion observed an object the call could not reach. It would have stayed green through any regression. The unit test now asserts the actual property - down() takes zero parameters and resolves - and the behavioural guarantee is pinned against a live database instead: after up() followed by down(), the signature stays NULL and the single audit row remains. Passing the mock in would have meant giving the migration a parameter it does not need, changing production code to satisfy a test.
Two titles claimed coverage for any non-dev ENVIRONMENT while their bodies set only 'prd'. A regression that executed on loc, on an unset value or on an unknown one would have left both green. Both are now table-driven over prd, loc, staging and unset - at the unit level and against a live database - so the fail-closed guarantee of the strict !== 'dev' check is pinned across the whole value space instead of being inferred from a single case.
|
Seven review passes were needed to reach zero findings, across two independent lenses — conformance against Two of them corrected claims this PR had made about itself:
Verification on the final commit: full suite against a real Postgres ( |
|
Verified that this PR cannot touch PRD data: the migration returns before its only statement unless |
What changed
src/config/config.ts— the wallet-login sign message now carries an environment marker on every non-PRD environment. PRD keeps the historical text byte-for-byte.migration/1785500000000-ClearDevUserSignatures.js— DEV-only credential rotation that clears the storeduser.signaturevalues, which were produced with the previous environment-independent text.Why
A wallet signature proves ownership of the exact text that was signed. That text was identical across all environments, so a signature stored in one environment was equally valid in every other one, PRD included. Scoping the text per environment removes that property for everything signed from now on: a signature created outside PRD no longer verifies against the PRD message. The migration disposes of the values created before this change.
Impact
signMessagePrefixis''for PRD and the message literals are untouched, so every existing PRD signature keeps verifying. A test pins the PRD text byte-for-byte.ENVIRONMENTis deliberately treated as non-PRD (fail-closed) — it must never accidentally yield a PRD-valid signature.Deliberate exception to "Auditable mutations — no destructive overwrites (CRITICAL)"
CONTRIBUTING.md:565-590requires an overwritten value to stay recoverable from the database, and asks reviewers to reject changes where the previous value cannot be reconstructed. This migration cannot satisfy that literally, and the conflict is inherent rather than an oversight: the values being cleared are login credentials, so keeping them in recoverable form would preserve precisely what the rotation exists to remove.What it does instead:
logbefore the update, holding the affecteduser.idand anmd5fingerprint of the previous signature rather than its plaintext. A known candidate value can be checked against the fingerprint; the credential itself is not recoverable.md5serves purely as an audit fingerprint here, not as a security primitive.EXISTS (SELECT 1 FROM "audit"), following the merged precedentmigration/1784600000011-FixFiatOutputValutaDateSerials.js. If the audit insert fails, no row is modified.logrow'screated) and to which (null). Only from which is reduced to a fingerprint.Raising this explicitly instead of working around it — it needs a conscious reviewer decision. If the team wants the case codified, a
CONTRIBUTING.mdamendment covering credential rotation is the natural follow-up; deliberately not bundled into a security fix.Known consequence
Existing DEV accounts on the comparison-only login paths cannot register a replacement signature through the normal API flow:
authenticateroutes known addresses to sign-in (auth.service.ts:129-131), andsignatureis only ever set on user creation (user.service.ts:329-339). Two wallet classes are affected:auth.service.ts:554-556) — the stored signature is compared verbatim.auth.service.ts:546-551) — the stored value acts as a shared secret; after the rotation!!dbSignatureis false, so sign-in fails.Accepted on DEV: these are test accounts, and a locked-out address can be recovered by a direct DB write or by using a fresh address. Establishing a new value automatically on sign-in is deliberately not done — those paths perform no cryptographic check, so it would be an account-takeover path.
The migration intentionally does not filter by wallet type. The values it clears are valid PRD credentials regardless of how they were produced, which is exactly what should not sit in a lower environment's database. Distinguishing wallet classes in SQL would add failure modes without removing the exposure.
Not affected: the custody master-key path (
auth.service.ts:245-251) skips signature verification whencustodyProvider.masterKeymatches, so it does not depend onuser.signature.One operational note: the statement clears every non-null
signature, without distinguishing old from already-rotated values. That is correct for the single run this migration performs, but a manual re-run at a later point would also clear signatures created after the rollout.Accepted residual risk
A signature copied out of a lower environment before this change stays valid on PRD. Revoking it would require changing the PRD text as well, forcing every user to sign in again. Accepted deliberately and out of scope here.
Out of scope
The Alby and LNURL paths store values that do not derive from signing this text (
auth-alby.service.ts:91,auth-lnurl.service.ts:129), so the message change does not affect them. Deliberately not addressed.Checks
npm run format:check,npm run lint,npm run type-check— all cleanMIGRATION_TEST_PGset, serial run) — 330 suites, 5946 tests, 0 failuresmd5values; an empty candidate set writes no audit row and changes nothing; the environment gate holds; and no plaintext signature reaches the log (asserted negatively against both fixtures and against abeforefield). WithoutMIGRATION_TEST_PGthe block skips cleanly, matching the existing migration specs.EXISTScoupling is isolated by its own test. Two distinct properties are covered separately, because conflating them overstates what is proven: dropping thelogtable shows only statement atomicity — Postgres rolls the whole statement back on an insert error, which happens with or without the coupling. The coupling itself is pinned by a test where the insert succeeds but yields no row (aBEFORE INSERTtrigger discards it without raising):up()completes, no audit row exists, and the signature must remain intact. RemovingAND EXISTS (SELECT 1 FROM "audit")turns exactly that test red and leaves the atomicity test green — verified by mutation.signMessagePrefixforced to'', 5 of the 6 relevant tests turn red — the PRD case correctly stays green — and all pass again once restored. The green run is not vacuous.1785500000000is above the highest migration on develop (1785400000000)Note on running the suite locally: the pre-existing migration specs are not safe to run in parallel against a shared Postgres instance — concurrent Jest workers collide on table names in
public. A serial run (or CI's per-shard service) is green. The block added here is schema-isolated and creates/drops its own schema, so it does not contribute to that collision.