mirror of
https://github.com/HeyPuter/puter.git
synced 2026-10-12 23:00:06 +00:00
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:
1 parent
91a0f3bc42
commit
df265ca699
2 files changed
+42
-19
No files matched your search
@@ -303,18 +303,41 @@ describe('userProtected — verifyIdentity (step 3)', () => {
|
||||
expect(refused.legacyCode).toBe('too_many_requests');
|
||||
});
|
||||
|
||||
it('spends the budget on wrong answers only', async () => {
|
||||
it('bounds attempts rather than rounds, so a burst cannot outrun it', async () => {
|
||||
const user = await makeUserWithPassword('correct-horse');
|
||||
const [, , verifyIdentity] = buildGate();
|
||||
const right = () =>
|
||||
const guess = () =>
|
||||
run(
|
||||
verifyIdentity,
|
||||
withUser(user, { body: { password: 'correct-horse' } }),
|
||||
withUser(user, { body: { password: 'wrong-guess' } }),
|
||||
);
|
||||
|
||||
// Well past the limit, and none of it is charged.
|
||||
for (let i = 0; i < 20; i++) expect(await right()).toBeUndefined();
|
||||
expect(await right()).toBeUndefined();
|
||||
// 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 () => {
|
||||
|
||||
@@ -21,7 +21,7 @@ import type { Request, RequestHandler, Response, NextFunction } from 'express';
|
||||
import bcrypt from 'bcrypt';
|
||||
import { isPlainUserActor } from '../../actor';
|
||||
import { HttpError } from '../HttpError';
|
||||
import { checkRateLimit, peekRateLimit } from './rateLimit.js';
|
||||
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';
|
||||
@@ -49,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';
|
||||
@@ -139,7 +141,7 @@ async function buildRevalidateFields(
|
||||
};
|
||||
}
|
||||
|
||||
/** Wrong passwords per account per hour, matching the sibling routes. */
|
||||
/** 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';
|
||||
@@ -225,10 +227,14 @@ export const createUserProtectedGate = (
|
||||
fields,
|
||||
});
|
||||
}
|
||||
// Charged by the outcome, so a right answer spends nothing.
|
||||
const budgetKey = `${PASSWORD_ATTEMPT_SCOPE}:${user.id}`;
|
||||
// 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 peekRateLimit(
|
||||
!(await checkRateLimit(
|
||||
budgetKey,
|
||||
PASSWORD_ATTEMPT_LIMIT,
|
||||
PASSWORD_ATTEMPT_WINDOW_MS,
|
||||
@@ -247,16 +253,10 @@ export const createUserProtectedGate = (
|
||||
} catch {
|
||||
match = false;
|
||||
}
|
||||
if (!match) {
|
||||
await checkRateLimit(
|
||||
budgetKey,
|
||||
PASSWORD_ATTEMPT_LIMIT,
|
||||
PASSWORD_ATTEMPT_WINDOW_MS,
|
||||
);
|
||||
if (!match)
|
||||
throw new HttpError(400, 'Password mismatch', {
|
||||
legacyCode: 'password_mismatch',
|
||||
});
|
||||
}
|
||||
return next();
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user