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

test(e2e): make nine red specs assert the contracts the code actually implements

Browse Source 
The e2e suite had never been run during this audit. It failed 9 of 256; seven of those
predated the audit's changes, established by building a stack from a clean HEAD worktree
and running the same specs against it rather than guessing.

Most were stale assertions rather than product defects:

- quota.spec solved for a target limit using the observed uploader count, but the divisor is
  max(active, estimated_guest_count, 1) and that config seeds at 100 — so every limit it
  aimed for came out 100x small and every "within quota" upload 413'd.
- rate-limit-shared-nat destructured `ticket` from a 429 body and fetched with
  `ticket=undefined`, turning the 429 under test into an unrelated 401. It also faked a
  release with no archive on disk, so the mint's pre-check 404'd and the per-day limiter was
  never reached; it now does a real release and asserts 200 rather than "not 429".
- ddos allowed only [200,429] from ten concurrent streams, so it failed on the very defence
  it exercises: four tickets per session survive and the rest correctly 401. Now asserts
  exactly four, which a tightened cap or an inverted eviction order would catch.
- auth-tampering asserted a throttled IP is refused EVEN with the correct password. That
  contract was deliberately removed — it let any phone on the venue NAT lock the operator
  out of their own admin panel, with a circular escape hatch. Inverted, plus a new check
  that a success does not refill an attacker's bucket.
- moderation-ui assumed a ban leaves a comment "stuck on screen"; `list_for_upload` filters
  banned authors, so it is hidden from everyone including the host. Now pins the pair that
  matters — the ban hides it, and the host's permanent removal survives an unban — and the
  UI leg it used to own is restored as a separate test on a reachable comment.

The export specs mint with `?kind=` now that a download ticket is bound to one archive, and
four of them assert the mint's 404 rather than the download's: with the kind always known,
the pre-check refuses up front instead of after charging a daily download for an archive
that cannot be served.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
fabi
2026-08-11 22:44:48 +02:00
parent a53729a704
commit 32dfe6874a
19 changed files with 351 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files

114
e2e/specs/01-auth/rate-limit-shared-nat.spec.ts
View File

@@ -13,6 +13,7 @@
*/
import { test, expect } from '../../fixtures/test';
import { BASE } from '../../helpers/env';
import { seedUpload } from '../../helpers/seed';
test.describe('Rate limits — guests behind a shared NAT', () => {
test('a dozen guests can all join from one IP, and 429s carry Retry-After', async ({
@@ -100,46 +101,139 @@ test.describe('Rate limits — guests behind a shared NAT', () => {
expect((await read(b.jwt)).status, 'B must not inherit A’s exhausted bucket').toBe(200);
});
test('one guest sweeping /recover cannot lock the venue — or the host — out of PIN recovery', async ({
api,
adminToken,
guest,
}) => {
// The sharpest version of this file's whole premise. `/recover` has a cross-name failure
// budget keyed on IP, meant to catch someone sweeping the public name list. Behind the venue
// NAT that budget is SHARED BY THE ENTIRE PARTY, and it used to be checked before the account
// was even looked up — so it refused a correct PIN.
//
// That is the host's problem specifically: hosts are promoted guests whose only credential is
// a 4-digit PIN, so /recover is their only way back in after losing a session. A guest posting
// invented names could deny it to everyone, indefinitely, for the price of ~2 requests/minute.
await api.patchConfig(adminToken, {
rate_limits_enabled: 'true',
recover_rate_enabled: 'true',
// Raise the per-IP VOLUME ceiling out of the way. It defaults to 30/min, and the
// cross-name FAILURE budget under test is also 30 — so the sweep below would trip the
// volume limiter first and this test would pass for the wrong reason (a 429 that proves
// nothing about whether a correct PIN survives a spent failure budget).
recover_ip_rate_per_min: '500',
});
const victim = await guest('RecoverVictim');
// Burn the shared per-IP budget with names that do not exist — the cheapest sweep, and the
// one that needs no knowledge of the guest list at all.
for (let i = 0; i < 35; i++) {
await fetch(`${BASE}/api/v1/recover`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ display_name: `Ghost${i}-${Date.now()}`, pin: '0000' }),
});
}
// A real guest, on that same IP, with their REAL PIN, must still get in.
const res = await fetch(`${BASE}/api/v1/recover`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ display_name: 'RecoverVictim', pin: victim.pin }),
});
expect(
res.status,
'a correct PIN must survive a spent cross-name budget — otherwise any guest can lock the ' +
'host out of the only login path they have'
).toBe(200);
// ...and the sweep is still answered as a sweep: a WRONG pin gets 429, not a bare 401, so the
// budget still does its job on the traffic it was built for.
const wrong = await fetch(`${BASE}/api/v1/recover`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ display_name: 'RecoverVictim', pin: '0001' }),
});
expect(wrong.status, 'wrong PINs from an exhausted IP are still throttled').toBe(429);
});
test('the export limit is per-user — one guest cannot spend the whole venue’s quota', async ({
api,
adminToken,
guest,
host,
db,
}) => {
// The sharpest case: 3 downloads per DAY on an IP key meant the 4th guest to fetch
// their keepsake was locked out until tomorrow.
await db.setExportReleased('e2e-test-event', true);
//
// A REAL release, not `setExportReleased`. `/export/ticket` pre-validates that the archive is
// actually servable and answers 404 without charging the limiter — deliberately, so a guest
// never spends one of their three daily downloads on an archive that cannot be served. With
// only the released FLAG set and no archive on disk, every mint here 404'd and the per-day
// limiter under test was never reached at all.
await seedUpload(host.jwt, { caption: 'for the keepsake' });
expect(
(
await fetch(`${BASE}/api/v1/host/gallery/release`, {
method: 'POST',
headers: { Authorization: `Bearer ${host.jwt}` },
})
).status
).toBe(204);
await expect
.poll(
async () => {
const s = await (
await fetch(`${BASE}/api/v1/export/status`, {
headers: { Authorization: `Bearer ${host.jwt}` },
})
).json();
return s.released === true && s.zip?.status === 'done';
},
{ timeout: 90_000, intervals: [500] }
)
.toBe(true);
await api.patchConfig(adminToken, {
rate_limits_enabled: 'true',
export_rate_enabled: 'true',
export_rate_per_day: '1',
});
// The per-day export limit is charged at the MINT, not at the download: the ticket endpoint is
// the authenticated chokepoint, while `/export/zip` authenticates by ticket alone so a resumed
// transfer doesn't spend another of the guest's daily allowance. So a throttled guest is
// refused with 429 at `/export/ticket` and never reaches the archive.
//
// This helper used to destructure `ticket` from that 429 body regardless, then fetch with
// `ticket=undefined` — turning the 429 under test into an unrelated 401 from the download
// endpoint. Surface the mint's refusal instead; that IS the throttle.
const mintAndFetch = async (jwt: string) => {
const res = await fetch(`${BASE}/api/v1/export/ticket`, {
const minted = await fetch(`${BASE}/api/v1/export/ticket?kind=zip`, {
method: 'POST',
headers: { Authorization: `Bearer ${jwt}` },
});
const { ticket } = await res.json();
if (!minted.ok) return minted;
const { ticket } = await minted.json();
return fetch(`${BASE}/api/v1/export/zip?ticket=${encodeURIComponent(ticket)}`);
};
const a = await guest('ExportFirst');
const b = await guest('ExportSecond');
// A spends their single daily allowance. The archive itself may not exist (404) —
// what matters is that the limiter admitted the request rather than 429ing it.
expect((await mintAndFetch(a.jwt)).status).not.toBe(429);
// A spends their single daily allowance — on a real archive, so this is a genuine 200 rather
// than merely "not 429", which would have been satisfied by any error at all.
expect((await mintAndFetch(a.jwt)).status, 'A’s first download must succeed').toBe(200);
expect((await mintAndFetch(a.jwt)).status, 'A’s second download is throttled').toBe(429);
// B shares A's IP and must still get their keepsake.
expect((await mintAndFetch(b.jwt)).status, 'B must not be locked out by A’s download').not.toBe(
429
expect((await mintAndFetch(b.jwt)).status, 'B must not be locked out by A’s download').toBe(
200
);
// And the host too, for good measure.
expect((await mintAndFetch(host.jwt)).status).not.toBe(429);
expect((await mintAndFetch(host.jwt)).status).toBe(200);
});
});