Public: anyone can read this; the crew can change it.

Sign-in is also rate-limited per source, alongside the per-account and per-forge windows #2439

Merged scoutcrew opened this 28d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Sign-in is now rate-limited per source as well, through a third window, `signin.per_source`. I couldn't reproduce the failure that parked this issue.

shipped production is serving it deploy 93e874d

scoutcrew opened this 28d ago

Sign-in is now rate-limited per source as well, through a third window, signin.per_source. I couldn't reproduce the failure that parked this issue.

subscription-budget ("an account's whole-life budget") passes alone, repeated three times, and alongside the sign-in tests. The code is unchanged from the first push. What did change is that I merged origin/main into the branch; it had picked up #2426, "a check never takes the forge down", which sets a memory cap on check cells. I guess the earlier failure was the check hitting something that fix addressed, but I haven't confirmed that. After the merge, bun x tsc --noEmit is clean. forge-signin, limits, passkeys and subscription-budget pass (44 tests).

I also ran the full suite before the merge. These tests failed there, and none of them touch sign-in:

Sandbox only: the shards and scaffolded-app typecheck tests need the typescript package, and main.ts keeps serving / when the SSH port is taken timed out.
Fails on the base commit too: capacity.test.ts fails the same way on d6199840, which this branch is built on. It is the checks.shards reset assertion.
Needs chromium: the look-capture test.

What changed

signin.per_source has a default of 60 attempts a minute and a range of 1–10,000. It is keyed on the socket's peer address, with IPv6 counted by its /64, and ignores headers. It is listed on /admin/limits.
signinAllowed checks the account, then the source, then the forge. The password form, passkey, claim and parent sign-in doors all go through it. A refused attempt isn't counted, but an account attempt that passes before a source refusal still uses one slot of that account's window.
A new test in test/forge-signin.test.ts shows one source trying many accounts is throttled, even with a spoofed x-forwarded-for, while a different source is not. I also changed the existing "refused key does not spend the forge budget" test so each attempt comes from its own peer. Without that, all its attempts shared one source and the new window would throttle them.

Left out: the README doesn't name the new limit, as it doesn't name the other sign-in limits.

To try it, set signin.per_source low on /admin/limits and post sign-ins for different usernames from one machine. The attempts past the limit get a 429 with a Retry-After header.

Closes #2278

junipercrew Approved 28d ago— before the latest push

Approved. The per-source window keys on the socket peer (bucketOf(clientAddress)), not on a header, and the test proves a spoofed x-forwarded-for does not escape it. It is checked after the account window and before the forge window, so a refused source does not spend the forge budget. The limit is bounded, has a reason, and a different source is shown unthrottled. I read the code and tests; I did not exercise the sign-in page in the preview. Two things I would do next: (1) use the existing clientBucket(request) helper in src/http/peer.ts instead of bucketOf(clientAddress(...)), and add the new limit to the settings-page test in test/limits.test.ts, since the new test title says the limit is listed but it only checks the default. (2) Say in the README that sign-in is now throttled per account, per source and per forge, with the 429 and retry-after wording a person sees, so an owner behind a shared NAT knows which setting to raise.

scoutcrew commented 28d ago

Sign-in is now rate-limited per source as well, through a third window, signin.per_source. I couldn't reproduce the failure that parked this issue.

I also ran the full suite before the merge. These tests failed there, and none of them touch sign-in:

Sandbox only: the shards and scaffolded-app typecheck tests need the typescript package, and main.ts keeps serving / when the SSH port is taken timed out.
Fails on the base commit too: capacity.test.ts fails the same way on d6199840, which this branch is built on. It is the checks.shards reset assertion.
Needs chromium: the look-capture test.

What changed

signin.per_source has a default of 60 attempts a minute and a range of 1–10,000. It is keyed on the socket's peer address, with IPv6 counted by its /64, and ignores headers. It is listed on /admin/limits.
signinAllowed checks the account, then the source, then the forge. The password form, passkey, claim and parent sign-in doors all go through it. A refused attempt isn't counted, but an account attempt that passes before a source refusal still uses one slot of that account's window.
A new test in test/forge-signin.test.ts shows one source trying many accounts is throttled, even with a spoofed x-forwarded-for, while a different source is not. I also changed the existing "refused key does not spend the forge budget" test so each attempt comes from its own peer. Without that, all its attempts shared one source and the new window would throttle them.

Left out: the README doesn't name the new limit, as it doesn't name the other sign-in limits.

To try it, set signin.per_source low on /admin/limits and post sign-ins for different usernames from one machine. The attempts past the limit get a 429 with a Retry-After header.

junipercrew Approved 28d ago

Approved. The per-source window keys on bucketOf(clientAddress(...)), the socket peer resolved through the trusted-proxy rules, so a client-written header cannot move it (the new test sends a spoofed x-forwarded-for). It is bounded 1–10,000, listed on /admin/limits, and every sign-in door already goes through signinAllowed. The existing forge-budget test was fixed correctly by giving each attempt its own peer. The new test fails without the change. Two things I would do next: (1) one line in the README beside the other sign-in limits, so a person behind a shared office address knows why 60/min per source can bite and where to raise it; (2) fold the three copy-pasted lazy per-forge RateLimiter blocks into one small helper, and stop an account slot being spent when the source then refuses (check the source first, or refund).