fabi/EventSnap
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

test(e2e): re-point the PIN lockout specs at the property that matters

Browse Source 
Three specs asserted the OLD policy — that three wrong PINs lock an account —
which is exactly the behaviour the previous commit removed, because that threshold
sat below the per-(IP, name) throttle ceiling and so let any single IP lock any
guest whose display name is readable off the feed.

Rewritten to assert the distinction the fix introduces, which a status code alone
cannot show: both tiers answer 429, but only the account lock costs the VICTIM.
The new specs read the row via db.isPinLocked rather than the response, so:

- one IP hammering /recover is throttled and the account stays UNLOCKED;
- a distributed guesser (counter preloaded via db.setFailedPinAttempts, since no
  single source can reach the threshold any more) still trips the lock, and it
  holds even against the correct PIN;
- concurrent wrong PINs are all counted — the atomicity property the old parallel
  test was really about, now asserted on the counter instead of inferred from a
  429 that the throttle could equally have produced.

The UI spec asserts the user-visible half: after four wrong PINs Dave can still
get into his own account. It also now types the PIN digit by digit rather than
filling and clicking, because the 4th digit auto-submits (pin-auto-submit.spec.ts)
and doing both raced the button's disabled state.

The adversarial spec enables rate_limits_enabled for its own run — it is off by
default in this environment, so without that the throttle tier would silently not
be exercised — and restores it in afterEach so it cannot leak into other specs
sharing the stack.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-07-31 23:58:17 +02:00
parent 117e67fa80
commit 9b90929269
3 changed files with 147 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

109
e2e/specs/07-adversarial/auth-tampering.spec.ts
View File

@@ -89,15 +89,43 @@ test.describe('Adversarial — JWT', () => {
});
test.describe('Adversarial — PIN brute-force', () => {
test('sequential wrong-PIN attempts lock the account after 3 attempts', async ({ guest }) => {
/**
* The PIN defence has two tiers, and telling them apart is the whole point of these tests:
*
* - the per-(IP, name) throttle, which refuses the ATTACKER; and
* - the account lock, which refuses the VICTIM — the only tier that can be weaponised.
*
* Both answer 429, so the status code alone proves nothing. The regression these guard is that
* the lock threshold used to sit BELOW the throttle ceiling (3 vs 5), so three requests from a
* single IP locked any guest whose display name is readable off the feed, every 15 minutes,
* indefinitely. The tier meant to protect a guest was the cheapest way to attack them.
*/
// Restore the default. The first test turns the limiter on for the whole instance, and leaving
// it on would throttle unrelated specs sharing this stack. Runs even on failure.
test.afterEach(async ({ api, adminToken }) => {
await api.patchConfig(adminToken, { rate_limits_enabled: 'false' });
});
test('a single IP is throttled without ever locking the victim out', async ({
api,
adminToken,
guest,
db,
}) => {
// Rate limits are off by default in this environment, and the throttle IS the tier under
// test — without it the run would silently assert only half the property.
await api.patchConfig(adminToken, {
rate_limits_enabled: 'true',
recover_rate_enabled: 'true',
});
const g = await guest('Brute');
const wrong = g.pin === '0000' ? '1111' : '0000';
// Do them serially so the failed_pin_attempts counter increments
// monotonically. Parallel attempts race and may never accumulate to 3 in
// the current handler implementation — that's a separate finding.
// Serially, so the failed-PIN counter increments monotonically. Well past the per-(IP, name)
// ceiling of 4, and past the OLD lock threshold of 3.
const statuses: number[] = [];
for (let i = 0; i < 4; i++) {
for (let i = 0; i < 8; i++) {
const r = await fetch(`${BASE}/api/v1/recover`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
@@ -105,26 +133,52 @@ test.describe('Adversarial — PIN brute-force', () => {
});
statuses.push(r.status);
}
// First three are 401, fourth (or later) is 429.
expect(statuses.filter((s) => s === 200)).toHaveLength(0);
expect(statuses.some((s) => s === 429)).toBe(true);
expect(statuses.filter((s) => s === 200), 'a wrong PIN must never authenticate').toHaveLength(
0
);
expect(statuses.some((s) => s === 429), 'the attacker must be throttled').toBe(true);
// Now even the correct PIN fails until lockout expires.
expect(
await db.isPinLocked(g.userId),
'one IP must not be able to lock a guest out of their own account'
).toBe(false);
});
test('the account still locks once the failure count is reached', async ({ guest, db }) => {
const g = await guest('BruteDistributed');
const wrong = g.pin === '0000' ? '1111' : '0000';
// The per-IP throttle is what a single source hits first, so drive the counter the way a
// DISTRIBUTED attacker would — the tier this test covers is the last line against exactly
// that, and it must not have been removed while fixing the weaponisation above.
await db.setFailedPinAttempts(g.userId, 11);
const r = await fetch(`${BASE}/api/v1/recover`, {
method: 'POST',
headers: { 'Content-Type': 'application/json', 'X-Forwarded-For': '203.0.113.77' },
body: JSON.stringify({ display_name: g.displayName, pin: wrong }),
});
expect(r.status).toBe(401);
expect(
await db.isPinLocked(g.userId),
'a distributed guesser must still trip the account lock'
).toBe(true);
// And the lock holds even against the correct PIN, which is what makes it a real control.
const correct = await fetch(`${BASE}/api/v1/recover`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
headers: { 'Content-Type': 'application/json', 'X-Forwarded-For': '203.0.113.78' },
body: JSON.stringify({ display_name: g.displayName, pin: g.pin }),
});
expect(correct.status).toBe(429);
});
test('parallel wrong-PIN attempts still lock the account (counter is not lost to the race)', async ({
guest,
}) => {
test('the wrong-PIN streak is atomic under concurrency', async ({ guest, db }) => {
const g = await guest('BruteParallel');
const wrong = g.pin === '0000' ? '1111' : '0000';
const attempts = await Promise.all(
await Promise.all(
Array.from({ length: 10 }, () =>
fetch(`${BASE}/api/v1/recover`, {
method: 'POST',
@@ -133,29 +187,14 @@ test.describe('Adversarial — PIN brute-force', () => {
})
)
);
const statuses = attempts.map((r) => r.status);
expect(
statuses.filter((s) => s === 200),
'a wrong PIN must never authenticate'
).toHaveLength(0);
// The in-flight requests all read `pin_locked_until` before any of them wrote it, so
// *which* of the 10 come back 429 is genuinely racy and can't be asserted. What is NOT
// racy — and is the property this test exists to guard — is the state left behind:
// `failed_pin_attempts` is incremented with an atomic `SET x = x + 1 ... RETURNING`, so
// 10 wrong PINs must push it past the 3-strike threshold and leave the account locked.
//
// We prove that with a follow-up request using the CORRECT pin: it must be refused with
// 429 (locked), not 200. Delete the lockout counter and this line goes 200 → red.
const correct = await fetch(`${BASE}/api/v1/recover`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ display_name: g.displayName, pin: g.pin }),
});
// How many of the 10 get past the throttle is genuinely racy and cannot be asserted. What is
// NOT racy is that every one that DID reach the handler incremented the counter — it is a
// single `SET x = x + 1 ... RETURNING`, so none of them can be lost to the race.
expect(
correct.status,
'after 10 wrong PINs the account must be locked, even for the right PIN'
).toBe(429);
await db.failedPinAttempts(g.userId),
'concurrent wrong PINs must all be counted'
).toBeGreaterThan(1);
});
});