fix: give every route behind the user-protected gate a password budget (#4150)

* fix: give every route behind the user-protected gate a password budget

Five of the six routes behind the gate declare a per-user hourly bucket
for the password check. The sixth, delete-own-user, declared none.

The bucket moves into the gate itself, so it covers every route behind
it and a new one cannot be added without one.

It is charged by the outcome rather than by the request: a wrong answer
spends one and a right answer spends nothing, so a caller who knows the
password never meets the limit.

* fix: the review's findings on the password budget

Reading the budget and spending it on the outcome bounded rounds rather
than attempts: everything already in flight read the same budget and was
answered before any of it was spent. The charge lands before the
comparison now, so a burst is bounded by the same number as a sequence.

The key gained a route component. One bucket across all six meant ten
mistyped passwords on one route closed the other five for the hour,
including the one an account owing its team's 2FA has to reach.
This commit is contained in:
Juan Fernando Castro authored and GitHub committed 2026-10-14 09:52:23 -04:00
1 parent f6a1e54642
commit 84aaf005ec
2 files changed
+106 -15

No files matched your search

@@ -100,12 +100,14 @@ const makeUserWithPassword = async (
}> = {},
) => {
const hash = await bcrypt.hash(plainPassword, 4);
const username = extra.username ?? `up-${Math.random().toString(36).slice(2, 10)}`;
const username =
extra.username ?? `up-${Math.random().toString(36).slice(2, 10)}`;
const created = await userStore.create({
username,
uuid: uuidv4(),
password: hash,
email: extra.email !== undefined ? extra.email : `${username}@test.local`,
email:
extra.email !== undefined ? extra.email : `${username}@test.local`,
free_storage: 100 * 1024 * 1024,
requires_email_confirmation: false,
} as Parameters<UserStore['create']>[0]);
@@ -144,7 +146,7 @@ describe('userProtected — requireSessionCookie (step 1)', () => {
expect(arg).toBeUndefined();
});
it("passes through when the cookie is present and req.token is undefined (no probe-attached token)", async () => {
it('passes through when the cookie is present and req.token is undefined (no probe-attached token)', async () => {
// This covers test-only / bypass paths where the cookie is the
// only credential. The guard only fires when `req.token` is set
// AND differs from the cookie.
@@ -177,7 +179,7 @@ describe('userProtected — requireSessionCookie (step 1)', () => {
expect((arg as HttpError).statusCode).toBe(401);
});
it("honors a custom config.cookie_name", async () => {
it('honors a custom config.cookie_name', async () => {
const gates = createUserProtectedGate({
config: {
cookie_name: 'custom_session',
@@ -210,7 +212,9 @@ describe('userProtected — refreshUser (step 2)', () => {
);
const [, refreshUser] = buildGate();
const req: Partial<Request> = { actor: { user: { id: user.id, uuid: user.uuid } } };
const req: Partial<Request> = {
actor: { user: { id: user.id, uuid: user.uuid } },
};
const arg = await run(refreshUser, req);
expect(isHttpError(arg)).toBe(true);
expect((arg as HttpError).statusCode).toBe(403);
@@ -220,13 +224,15 @@ describe('userProtected — refreshUser (step 2)', () => {
it('passes through and stashes the fresh row on req.userProtected', async () => {
const user = await makeUserWithPassword('hunter2');
const [, refreshUser] = buildGate();
const req: Partial<Request> = { actor: { user: { id: user.id, uuid: user.uuid } } };
const req: Partial<Request> = {
actor: { user: { id: user.id, uuid: user.uuid } },
};
const arg = await run(refreshUser, req);
expect(arg).toBeUndefined();
expect((req as Request).userProtected?.user.uuid).toBe(user.uuid);
});
it("throws 401 when the actor lacks a user id (defensive — earlier gates should catch this)", async () => {
it('throws 401 when the actor lacks a user id (defensive — earlier gates should catch this)', async () => {
const [, refreshUser] = buildGate();
const arg = await run(refreshUser, { actor: undefined });
expect(isHttpError(arg)).toBe(true);
@@ -266,7 +272,7 @@ describe('userProtected — verifyIdentity (step 3)', () => {
expect(arg).toBeUndefined();
});
it("returns 400 password_mismatch when bcrypt says no", async () => {
it('returns 400 password_mismatch when bcrypt says no', async () => {
const user = await makeUserWithPassword('correct-horse');
const [, , verifyIdentity] = buildGate();
const arg = await run(
@@ -278,7 +284,63 @@ describe('userProtected — verifyIdentity (step 3)', () => {
expect((arg as HttpError).legacyCode).toBe('password_mismatch');
});
it("returns 403 password_required when password account submits no credentials", async () => {
it('stops guessing after ten wrong passwords on the same account', async () => {
const user = await makeUserWithPassword('correct-horse');
const [, , verifyIdentity] = buildGate();
const guess = () =>
run(
verifyIdentity,
withUser(user, { body: { password: 'wrong-guess' } }),
);
for (let i = 0; i < 10; i++) {
expect((((await guess()) as HttpError) ?? {}).legacyCode).toBe(
'password_mismatch',
);
}
const refused = (await guess()) as HttpError;
expect(refused.statusCode).toBe(429);
expect(refused.legacyCode).toBe('too_many_requests');
});
it('bounds attempts rather than rounds, so a burst cannot outrun it', async () => {
const user = await makeUserWithPassword('correct-horse');
const [, , verifyIdentity] = buildGate();
const guess = () =>
run(
verifyIdentity,
withUser(user, { body: { password: 'wrong-guess' } }),
);
// All at once: the budget is spent before any comparison runs, so
// the same ten get through whether they arrive in step or together.
const answers = (await Promise.all(
Array.from({ length: 30 }, guess),
)) as HttpError[];
const compared = answers.filter(
(a) => a.legacyCode === 'password_mismatch',
);
expect(compared).toHaveLength(10);
});
it('keeps an exhausted budget on one route away from the others', async () => {
const user = await makeUserWithPassword('correct-horse');
const [, , verifyIdentity] = buildGate();
const on = (path: string, password: string) =>
run(verifyIdentity, {
...withUser(user, { body: { password } }),
route: { path },
} as Partial<Request>);
for (let i = 0; i < 10; i++) await on('/a', 'wrong-guess');
expect(((await on('/a', 'wrong-guess')) as HttpError).statusCode).toBe(
429,
);
// A different route behind the same gate still answers.
expect(await on('/b', 'correct-horse')).toBeUndefined();
});
it('returns 403 password_required when password account submits no credentials', async () => {
// No password in body, no revalidation cookie → reject.
const user = await makeUserWithPassword('hunter2');
const [, , verifyIdentity] = buildGate();
@@ -288,7 +350,7 @@ describe('userProtected — verifyIdentity (step 3)', () => {
expect((arg as HttpError).legacyCode).toBe('password_required');
});
it("accepts a valid puter_revalidation cookie in lieu of a password", async () => {
it('accepts a valid puter_revalidation cookie in lieu of a password', async () => {
const user = await makeUserWithPassword('hunter2');
// Sign a real revalidation token via the real TokenService.
const cookieValue = tokenService.sign('oidc-state', {
@@ -347,7 +409,7 @@ describe('userProtected — verifyIdentity (step 3)', () => {
expect((arg as HttpError).legacyCode).toBe('password_required');
});
it("falls through silently when the revalidation cookie is unparseable (bad signature)", async () => {
it('falls through silently when the revalidation cookie is unparseable (bad signature)', async () => {
const user = await makeUserWithPassword('pwd');
const [, , verifyIdentity] = buildGate();
const arg = await run(
@@ -415,10 +477,12 @@ describe('userProtected — OIDC-only accounts', () => {
});
expect(isHttpError(arg)).toBe(true);
expect((arg as HttpError).statusCode).toBe(403);
expect((arg as HttpError).legacyCode).toBe('oidc_revalidation_required');
expect((arg as HttpError).legacyCode).toBe(
'oidc_revalidation_required',
);
});
it("returns oidc_revalidation_required when a password-less user submits no credentials", async () => {
it('returns oidc_revalidation_required when a password-less user submits no credentials', async () => {
const user = await makeUserWithPassword('seed-then-null');
await server.clients.db.write(
'UPDATE user SET password = NULL WHERE id = ?',
@@ -434,6 +498,8 @@ describe('userProtected — OIDC-only accounts', () => {
cookies: {},
});
expect(isHttpError(arg)).toBe(true);
expect((arg as HttpError).legacyCode).toBe('oidc_revalidation_required');
expect((arg as HttpError).legacyCode).toBe(
'oidc_revalidation_required',
);
});
});
@@ -21,6 +21,7 @@ import type { Request, RequestHandler, Response, NextFunction } from 'express';
import bcrypt from 'bcrypt';
import { isPlainUserActor } from '../../actor';
import { HttpError } from '../HttpError';
import { checkRateLimit } from './rateLimit.js';
import type { IConfig } from '../../../types';
import type { UserStore, UserRow } from '../../../stores/user/UserStore';
import type { OIDCService } from '../../../services/auth/OIDCService';
@@ -48,7 +49,9 @@ import type { TokenService } from '../../../services/auth/TokenService';
* cookie (signed via `services.token.sign('oidc-state')`) is required.
* OIDC-only accounts (no password) MUST use the revalidation cookie —
* password path returns `oidc_revalidation_required` with a `revalidate_url`
* so the GUI can open the OIDC popup.
* so the GUI can open the OIDC popup. Each password check is charged to a
* per-account, per-route hourly budget, so a route behind this gate is
* bounded whether or not it declares a limit of its own.
*/
const REVALIDATION_COOKIE_NAME = 'puter_revalidation';
@@ -138,6 +141,11 @@ async function buildRevalidateFields(
};
}
/** Password checks per account per route per hour, as the siblings bound. */
const PASSWORD_ATTEMPT_LIMIT = 10;
const PASSWORD_ATTEMPT_WINDOW_MS = 60 * 60_000;
const PASSWORD_ATTEMPT_SCOPE = 'user-protected-password';
export const createUserProtectedGate = (
deps: UserProtectedGateDeps,
options: UserProtectedGateOptions = {},
@@ -219,6 +227,23 @@ export const createUserProtectedGate = (
fields,
});
}
// Charged before the comparison, or everything already in
// flight is answered before any of it is spent. Per route, so
// one exhausted budget cannot close the other five.
const budgetKey =
`${PASSWORD_ATTEMPT_SCOPE}:${req.route?.path ?? req.path}` +
`:${user.id}`;
if (
!(await checkRateLimit(
budgetKey,
PASSWORD_ATTEMPT_LIMIT,
PASSWORD_ATTEMPT_WINDOW_MS,
))
) {
throw new HttpError(429, 'Too many requests.', {
legacyCode: 'too_many_requests',
});
}
let match = false;
try {
match = await bcrypt.compare(