mirror of
https://github.com/Memo-2023/mana-monorepo.git
synced 2026-05-14 20:21:09 +02:00
fix(mana-auth): account lockout was structurally dead + add failure-path tests
While adding negative-path integration tests for the auth flow I
discovered that *neither* of the lockout primitives in
services/mana-auth/src/services/security.ts has actually been
working in production. Two independent silent failures that combined
into a "the lockout never triggers, ever" outcome:
1. recordAttempt() inserted into auth.login_attempts with explicit
`id = gen_random_uuid()`, but auth.login_attempts.id is a
`serial integer` column with `nextval('auth.login_attempts_id_seq')`
as default. The UUID-into-integer cast threw a type error every
single time, the bare `catch {}` swallowed it as "non-critical",
and not a single login attempt was ever persisted. Lockout's "5
failures in 15 min" check was running against an empty table.
2. checkLockout() built `attempted_at > ${new Date(...)}` via the
drizzle sql template, but postgres-js cannot bind a JS Date object
directly — it tries to byteLength() the parameter and crashes with
`Received an instance of Date`. Same anti-pattern: bare `catch`,
returns `{locked: false}` (fail-open), no log, completely invisible.
Both are "silent broken since the encryption-vault series of changes"
class — caught only because the integration test for the lockout flow
expected the 6th login attempt to return 429 and got 200 instead.
Fixes:
- recordAttempt(): drop the bogus `id` column from the INSERT (let the
sequence default assign it), default ipAddress to null instead of
letting `${undefined}` collapse the parameter slot, and surface
errors in the catch instead of swallowing them silently.
- checkLockout(): pass `windowStart.toISOString()` instead of the Date
object so postgres-js can serialize it. Same catch upgrade — log the
cause when failing open.
Failure-path test additions (tests/integration/auth-failures.test.ts):
- wrong password: assert 401, no JWT, +1 LOGIN_FAILURE in security_events,
+1 row in auth.login_attempts
- account lockout: 5 failed attempts then 6th returns 429 with
remainingSeconds, even with the correct password
- unverified email login: 403 with code = EMAIL_NOT_VERIFIED
- validate with garbage token: valid !== true
- resend verification: second mail arrives in mailpit
Plus the run-integration-tests.sh helper now runs both .test.ts files
and tests/integration/package.json's `test` script does the same.
Negative-control: reverted the recordAttempt fix (re-added the bogus
gen_random_uuid id), the wrong-password test failed at the
login_attempts assertion. Reverted the checkLockout fix, the lockout
test failed at the 429 assertion. Both fixes verified to be load-bearing.
6 tests, 45 expects, ~1.3s on a warm cache.
This commit is contained in:
parent
a9178ec2fb
commit
d941ff2231
4 changed files with 258 additions and 9 deletions
|
|
@ -86,7 +86,10 @@ export class AccountLockoutService {
|
|||
|
||||
async checkLockout(email: string): Promise<{ locked: boolean; remainingSeconds?: number }> {
|
||||
try {
|
||||
const windowStart = new Date(Date.now() - WINDOW_MINUTES * 60 * 1000);
|
||||
// postgres-js can't bind a JS Date directly via the drizzle sql
|
||||
// template — it tries to byteLength() the parameter and crashes
|
||||
// with `Received an instance of Date`. Pass an ISO string instead.
|
||||
const windowStart = new Date(Date.now() - WINDOW_MINUTES * 60 * 1000).toISOString();
|
||||
const result = await this.db.execute(
|
||||
sql`SELECT COUNT(*) as count, MAX(attempted_at) as last_attempt
|
||||
FROM auth.login_attempts
|
||||
|
|
@ -104,19 +107,38 @@ export class AccountLockoutService {
|
|||
locked: true,
|
||||
remainingSeconds: Math.ceil((lockoutEnd.getTime() - Date.now()) / 1000),
|
||||
};
|
||||
} catch {
|
||||
} catch (error) {
|
||||
// Fail open on lockout-check errors (we'd rather let a legit
|
||||
// user log in than block them on a transient DB hiccup), but
|
||||
// surface the cause so the next bug doesn't take 4 hours to
|
||||
// find like this one did.
|
||||
console.warn(
|
||||
'checkLockout failed (fail-open):',
|
||||
email,
|
||||
error instanceof Error ? error.message : error
|
||||
);
|
||||
return { locked: false };
|
||||
}
|
||||
}
|
||||
|
||||
async recordAttempt(email: string, successful: boolean, ipAddress?: string) {
|
||||
try {
|
||||
// Don't INSERT id — auth.login_attempts.id is a serial integer
|
||||
// (`nextval('auth.login_attempts_id_seq')` default), not a UUID.
|
||||
// The previous code passed `gen_random_uuid()` into it and the
|
||||
// resulting type-cast error was silently eaten by the catch
|
||||
// below — meaning lockout's "5 failures in 15 min" check ran on
|
||||
// an empty table forever and the lockout never actually triggered.
|
||||
await this.db.execute(
|
||||
sql`INSERT INTO auth.login_attempts (id, email, successful, ip_address, attempted_at)
|
||||
VALUES (gen_random_uuid(), ${email}, ${successful}, ${ipAddress}, NOW())`
|
||||
sql`INSERT INTO auth.login_attempts (email, successful, ip_address, attempted_at)
|
||||
VALUES (${email}, ${successful}, ${ipAddress ?? null}, NOW())`
|
||||
);
|
||||
} catch (error) {
|
||||
console.warn(
|
||||
'Failed to record login attempt (non-critical):',
|
||||
email,
|
||||
error instanceof Error ? error.message : error
|
||||
);
|
||||
} catch {
|
||||
// Non-critical
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue