From 11114c80d6b8aaa34b7f690be0a852b0b02df1c8 Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Sat, 31 Oct 2026 00:26:56 -0700 Subject: [PATCH] fix(auth): fail closed on otp-login token reuse check (#4421) /login/otp and /login/recovery-code marked the otp-login token used through a rate-limit check that admits on a store error, so while the store was unavailable a captured token could complete another sign-in. The check now fails closed. Also drops the check-translations scripts and the AGENTS.md step that runs them; the script they call was removed in #2873. --- AGENTS.md | 2 +- package.json | 1 - .../controllers/auth/AuthController.test.ts | 38 +++++++++++++++++++ .../controllers/auth/AuthController.ts | 8 +++- src/gui/package.json | 1 - 5 files changed, 46 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c16bb19cd..e7740afc7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -201,6 +201,6 @@ Recipes are tested, copy-pasteable patterns for a task, one layer above the per- - **Reuse existing UI primitives before writing new ones.** Windows and dialogs are `UIWindow*` functions in [src/gui/src/UI/](src/gui/src/UI/); generic pieces already exist (`UIAlert`, `UIPrompt`, `UIContextMenu`, `UINotification`, `UIPopover`, widgets in [UI/Components/](src/gui/src/UI/Components/)). A new window should read like its neighbors: an async function taking an options object, composing an HTML string, wiring behavior with jQuery, delegating to `UIWindow(...)`. - **Match the visual language.** Use existing CSS classes (`button`, `button-primary`, window chrome, form styles) and copy the layout patterns of neighboring windows; new styles go in [src/gui/src/css/](src/gui/src/css/) following existing conventions. Verify anything positional (menus, overlays, z-index) on both desktop and mobile viewports. -- **i18n every user-facing string.** No hardcoded UI text — use `i18n('key')` and add the key to [src/gui/src/i18n/translations/en.js](src/gui/src/i18n/translations/en.js). Run `npm run check-translations` before opening the PR. +- **i18n every user-facing string.** No hardcoded UI text — use `i18n('key')` and add the key to [src/gui/src/i18n/translations/en.js](src/gui/src/i18n/translations/en.js). - **Shared logic goes in helpers/services.** Reusable non-UI logic belongs in [src/gui/src/helpers/](src/gui/src/helpers/) or [src/gui/src/services/](src/gui/src/services/), not copy-pasted between windows. - **Tests.** Vitest is wired for the GUI (`src/**/*.test.js`; see [appOrder.test.js](src/gui/src/UI/Dashboard/appOrder.test.js) for the shape). Extract pure logic into functions and test those. Desktop behavior driven through puter.js (`puter.ui.*`) is covered by the Playwright harness in [src/puter-js/tests/e2e/](src/puter-js/tests/e2e/) — add a spec there when you change how the desktop renders SDK-driven UI. diff --git a/package.json b/package.json index 3ff7c8d2f..6630e6212 100644 --- a/package.json +++ b/package.json @@ -54,7 +54,6 @@ "dev": "npm start", "build": "npm run setupExtensions && npm run build:ts && cd src/gui && node ./build.js && cd ../puter-js && npm run build", "build:workerLib": "cd src/puter-js && npm run build && cd ../worker && npm run build", - "check-translations": "node tools/check-translations.js", "prepare": "husky", "build:ts": "tsc -p tsconfig.build.json && node ./tools/write-dist-package-json.mjs", "check:puterjs:types": "node tools/checkPuterjsTypes.mjs", diff --git a/src/backend/controllers/auth/AuthController.test.ts b/src/backend/controllers/auth/AuthController.test.ts index 50f4eb287..9c8bff097 100644 --- a/src/backend/controllers/auth/AuthController.test.ts +++ b/src/backend/controllers/auth/AuthController.test.ts @@ -42,6 +42,10 @@ import { adminOnlyGate, requireUserActorGate, } from '../../core/http/middleware/gates.js'; +import { + configureRateLimit, + listConfiguredRateLimitBackends, +} from '../../core/http/middleware/rateLimit.js'; import { ROUTES_METADATA_KEY, type CollectedRoute, @@ -1753,6 +1757,40 @@ describe('AuthController.handleLoginOtp + handleLoginRecoveryCode', () => { ).rejects.toMatchObject({ statusCode: 400 }); }); + it('refuses the sign-in while the used-token store is down', async () => { + const { user, secret, otpToken } = await startOtpLogin(); + const token = await otpToken(); + + const { default: backend } = listConfiguredRateLimitBackends(); + configureRateLimit({ + default: 'redis', + redis: { + multi: () => { + throw new Error('store down'); + }, + }, + }); + const logged = vi.spyOn(console, 'error').mockImplementation(() => {}); + const res = makeRes(); + try { + await expect( + controller.handleLoginOtp( + makeReq({ token, code: liveTotp(user.username, secret) }), + res, + ), + ).rejects.toThrow('store down'); + } finally { + logged.mockRestore(); + configureRateLimit({ + default: backend, + redis: server.clients.redis, + kv: server.stores.kv, + metering: server.services.metering, + }); + } + expect(res.body).toBeUndefined(); + }); + it('a wrong code leaves the token usable for a retry', async () => { const { user, secret, otpToken } = await startOtpLogin(); const token = await otpToken(); diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index 02114a4bb..0f0548a05 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -4637,13 +4637,19 @@ export class AuthController extends PuterController { await checkRateLimit(`${scope}:${userUid}`, limit, window); } - /** An `otp-login` JWT completes one sign-in; a second redemption is refused. */ + /** + * An `otp-login` JWT completes one sign-in; a second redemption is refused. + * Fails closed: while the store can't say whether the token was used, the + * sign-in is refused rather than letting a captured token replay. + */ async #consumeOtpLoginToken(token: string): Promise { const digest = crypto.createHash('sha256').update(token).digest('hex'); const firstUse = await checkRateLimit( `login-otp-token-used:${digest}`, 1, OTP_LOGIN_TOKEN_USED_WINDOW_MS, + undefined, + { failOpen: false }, ); if (!firstUse) { throw new HttpError(400, 'Invalid token.', { diff --git a/src/gui/package.json b/src/gui/package.json index fafc43dff..a4ec8c358 100644 --- a/src/gui/package.json +++ b/src/gui/package.json @@ -27,7 +27,6 @@ "test": "vitest run --config vitest.config.js", "start:gui": "nodemon --exec \"node dev-server.js\" ", "build": "node ./build.js", - "check-translations": "node tools/check-translations.js", "start-webpack": "webpack --watch --devtool source-map --stats=errors-only" }, "workspaces": [