feat: add user-managed totp security settings#1122
Closed
NieiR wants to merge 7 commits into
Closed
Conversation
github-actions
Bot
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough新增基于 TOTP 的二次验正功能：数据库表与 Drizzle 模式、TOTP 生成/验证与版本化加密、仓库读写、账户安全 API（GET/POST/DELETE）、登录流程 OTP 支持、安全设置页面与客户端组件、国际化文案扩展与大量测试覆盖。 Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1564d4b85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements TOTP-based two-factor authentication, including database schema updates, translation files for multiple locales, and a new security settings interface. The login process now supports a two-step verification flow. Feedback recommends encrypting TOTP secrets at rest to mitigate security risks, requiring verification codes for sensitive actions like disabling or re-binding 2FA, and optimizing the base32 decoding logic for better performance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
src/lib/security/totp.ts (1)
93-106: TOTP URI 标签分隔符编码优化建议当前代码使用
encodeURIComponent(label)对整个标签进行编码，将 issuer 与 accountName 之间的冒号编码为%3A，生成otpauth://totp/Claude%20Code%20Hub%3AWeb%20UI?...的形式。根据 Google Authenticator 官方格式文档，虽然
%3A作为分隔符在技术上是有效的，但官方推荐对 issuer 与 accountName 分别进行 URL 编码，然后使用字面冒号:连接（如otpauth://totp/Claude%20Code%20Hub:Web%20UI?...）。这样可以提升与第三方验证器（如 FreeOTP 等）的兼容性，确保所有 TOTP 应用都能正确解析。♻️ 建议改写
- const label = `${issuer}:${accountName}`; + const encodedLabel = `${encodeURIComponent(issuer)}:${encodeURIComponent(accountName)}`; const params = new URLSearchParams({ secret: options.secret, issuer, algorithm: "SHA1", digits: "6", period: "30", }); - return `otpauth://totp/${encodeURIComponent(label)}?${params.toString()}`; + return `otpauth://totp/${encodedLabel}?${params.toString()}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/security/totp.ts` around lines 93 - 106, The TOTP label currently uses encodeURIComponent(label) which encodes the colon separator; update buildTotpAuthUri so issuer and accountName are URL-encoded separately and then joined with a literal ':' to form the label (e.g., const encodedIssuer = encodeURIComponent(issuer); const encodedAccount = encodeURIComponent(accountName); const label = `${encodedIssuer}:${encodedAccount}`), keeping the existing issuer/accountName trimming/fallback logic and leaving the params generation unchanged; use this new label when constructing the otpauth URI in buildTotpAuthUri.src/app/[locale]/settings/_lib/nav-items.ts (1)
42-47: 图标与「Sensitive Words」重复，建议替换以避免侧边栏视觉冲突。第 56-60 行的
Sensitive Words项已经使用iconName: "shield-alert"。新增的Security项使用同一图标后，侧边栏中会出现两个紧邻的菜单项使用完全相同的图标，影响可识别性。建议为安全设置选用一个更具语义、不与现有项冲突的图标（例如lock之类）。注意SettingsNavIconName是显式的字面量联合，需要同时在第 3-18 行扩展该类型并在渲染层添加对应图标映射。建议改动示意
{ href: "/settings/security", labelKey: "nav.security", label: "Security", - iconName: "shield-alert", + iconName: "lock", },并相应在
SettingsNavIconName联合类型与图标渲染处补充"lock"（或所选的新图标名）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/_lib/nav-items.ts around lines 42 - 47, The Security nav item currently reuses iconName "shield-alert" which duplicates the Sensitive Words icon; change the Security item's iconName (the object with href "/settings/security") to a distinct semantic name like "lock", then add that new literal to the SettingsNavIconName union (lines defining the type) and finally add the corresponding icon mapping in the icon-rendering code (the component/function that reads iconName and returns an icon) so "lock" is rendered correctly; ensure the new literal name is used consistently across the nav item, the SettingsNavIconName type, and the render mapping.src/app/api/auth/login/route.ts (1)
276-293: TOTP 校验缺少重放保护，建议跟踪已使用的 code/counter。
verifyTotp仅做时间窗匹配，相同 OTP 在 ~90s（含相邻窗口）内可被重复使用。若攻击者通过中间人/肩窥获取了一次性密码，在窗口内可多次重放完成登录。建议在user_security_settings表上记录最近一次成功使用的 counter（或 OTP 哈希），在verifyTotp通过后比较并拒绝相同/更小的 counter，并在登录成功路径上持久化。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/auth/login/route.ts` around lines 276 - 293, The TOTP check using verifyTotp lacks replay protection; after verifyTotp returns true you must compare the TOTP counter (or a hash of the OTP) against the stored last_used value in the user's user_security_settings and reject the login if the incoming counter is equal or <= stored counter; on successful login persist the new counter (or OTP hash) back to user_security_settings. Locate the OTP verification block around verifyTotp in the login route handler, extract the counter (or derive a stable OTP hash) from the verification result, check it against the user_security_settings last_used_counter/otp_hash, return the same 401 OTP_INVALID response when replay is detected, and update the stored last_used value only after a completed successful authentication flow.src/app/[locale]/dashboard/_components/user-menu.tsx (1)
67-78: 单项子菜单可以扁平化。
DropdownMenuSub下目前只有一个Security Settings入口，多嵌一层 hover 菜单仅为一个目的地，交互成本偏高。除非短期内会再加入更多设置入口，建议直接用平铺的DropdownMenuItem：建议改动
- <DropdownMenuSub> - <DropdownMenuSubTrigger> - <Settings className="h-4 w-4" /> - <span>{t("settings")}</span> - </DropdownMenuSubTrigger> - <DropdownMenuSubContent className="w-44"> - <DropdownMenuItem onSelect={() => router.push("/settings/security")}> - <ShieldCheck className="h-4 w-4" /> - <span>{t("securitySettings")}</span> - </DropdownMenuItem> - </DropdownMenuSubContent> - </DropdownMenuSub> + <DropdownMenuItem onSelect={() => router.push("/settings/security")}> + <ShieldCheck className="h-4 w-4" /> + <span>{t("securitySettings")}</span> + </DropdownMenuItem>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/user-menu.tsx around lines 67 - 78, The submenu under DropdownMenuSub only contains one item so the nested hover structure is unnecessary; replace the DropdownMenuSub/DropdownMenuSubTrigger/DropdownMenuSubContent trio with a single top-level DropdownMenuItem that navigates to "/settings/security" (the existing onSelect that calls router.push("/settings/security")) and preserves the icon (ShieldCheck) and label (t("securitySettings")), removing the extra DropdownMenuSub elements (DropdownMenuSubTrigger and DropdownMenuSubContent) to flatten the menu.src/app/[locale]/settings/security/_components/security-settings-client.tsx (2)
118-133: 禁用 MFA 使用window.confirm，与项目其余 shadcn 风格不一致。原生
window.confirm不可主题化、无法本地化按钮文本，且对辅助技术支持较差。建议复用项目已有的AlertDialog（shadcn）来收集二次确认，与设置页其它流程保持一致并便于补充"风险说明"等附加文案。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/security/_components/security-settings-client.tsx around lines 118 - 133, The disableTotp function currently uses window.confirm; replace this with the project's AlertDialog (shadcn) pattern so the confirmation UI is themeable/localizable and accessible: change the flow so clicking the "disable" button opens AlertDialog (with localized text via t and optional risk copy), and only after the user confirms invoke the existing async disable handler (the logic inside disableTotp that calls fetch "/api/account/security/totp" and uses setSaving, setStatus, toast). Keep the error handling and finally setSaving(false) behavior, and ensure cancellation from the dialog simply closes it without calling fetch.
106-110: 乐观写入boundAt可能与服务端实际时间不一致。
saveTotpEnabled在仓储层使用new Date()写库；这里再new Date().toISOString()作为本地展示，二者可能产生几十毫秒至几秒的偏差，下次刷新页面又会重新对齐。如果希望严格一致，可在enable接口的成功响应中返回boundAt，前端直接使用服务端时间。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/security/_components/security-settings-client.tsx around lines 106 - 110, The optimistic write uses local time via new Date().toISOString() when calling setStatus — this can diverge from the server-side timestamp written by saveTotpEnabled; instead have the enable TOTP API return the authoritative boundAt timestamp and update the client state from that response (e.g. call the enable/saveTotpEnabled endpoint, read response.boundAt, then call setStatus({ enabled: true, boundAt: response.boundAt }) and only after that setSetupOpen(false), setSetup(null), setOtpCode(""), toast.success(t("enabled"))). Ensure saveTotpEnabled (or the controller it calls) returns the server-side boundAt so the front end uses the canonical time.src/app/api/account/security/totp/route.ts (1)
38-103: 建议为 TOTP 启用 / 禁用 / 失败动作补充审计日志。启用、禁用、密钥重绑这类涉及账户安全态的事件目前没有任何
logger调用。补充结构化日志（包含subjectId、动作、IP、UA、errorCode）有助于事后排障与可疑行为告警，且不影响GET等只读路径。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/account/security/totp/route.ts` around lines 38 - 103, Add structured audit logging for all TOTP state-changing flows in POST and DELETE: in POST inside the "setup" branch (where generateBase32Secret/buildTotpAuthUri are called) emit a "totp.setup" log with subjectId, action:"setup", client IP and User-Agent from request headers and the generated secret metadata (but avoid logging full secret if policy prohibits); in POST "enable" branch (where verifyTotp and saveTotpEnabled are used) log both failure cases (invalid OTP → include errorCode:"OTP_INVALID") and success (after saveTotpEnabled → action:"enable", enabled:true); in DELETE (where disableTotp is called) log action:"disable" with subjectId and result; attach csrf check context where relevant (csrfResult) and include errorCode:"CSRF_REJECTED" or "UNAUTHORIZED" when returning those responses; use the existing request, context.subjectId, and functions saveTotpEnabled/disableTotp/verifyTotp to locate spots for the new logger calls.src/repository/user-security-settings.ts (1)
13-15: 将 admin sentinel 值-1提取为常量
session.user.id === -1用于标识管理员会话，这个约定在@/lib/auth中定义。将-1散落在各处（仓储层、测试中）会导致修改时需要同步多个位置。建议从 auth 模块导出一个ADMIN_USER_ID常量，在此处和其他地方统一引用，避免魔法数字。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/user-security-settings.ts` around lines 13 - 15, Extract the admin sentinel value -1 into a shared constant and use it here: import the exported ADMIN_USER_ID from the auth module and replace the literal check in getSecuritySubjectId (function getSecuritySubjectId in user-security-settings.ts) so it reads session.user.id === ADMIN_USER_ID ? "admin-token" : `user:${session.user.id}`; update any related tests/uses to reference ADMIN_USER_ID as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@messages/ru/dashboard.json`:
- Line 790: 键 "securitySettings" 的值仍为英文 "Security Settings"，导致俄语界面出现混合语言；请将
messages/ru/dashboard.json 中该键 "securitySettings" 的值替换为俄语本地化文本（例如 "Настройки
безопасности" 或项目约定的俄语翻译），确保所有相同上下文的导航文案在俄语文件中一致。
In `@messages/ru/settings/security.json`:
- Around line 1-31: The ru locale file messages/ru/settings/security.json
currently contains English strings; replace every top-level key value and nested
values (e.g., "title", "description", "loadFailed", "setupFailed",
"enableFailed", "disableFailed", "invalidCode", "enabled", "disabled",
"disableConfirm", the "totp" object keys "title","description","toggle", the
"status" keys "enabled","disabled","boundAt", and the "setup" object keys
"title","description","manualSecret","codeLabel","notice","cancel","confirm")
with proper Russian translations so the file provides full Russian i18n parity
with other locales; ensure placeholders like {date} are preserved exactly and
keep string intent consistent with messages/en and messages/zh-CN.
In `@src/app/`[locale]/settings/security/_components/security-settings-client.tsx:
- Around line 45-56: The GET to "/api/account/security/totp" currently calls
response.json() unconditionally so 4xx/5xx bodies are treated as valid data and
make setStatus({ enabled: Boolean(data.enabled), boundAt: data.boundAt ?? null
}) wrongly show "disabled"; update the fetch flow in the block that calls
fetch(...).then(response => ...) so you first check response.ok and if not ok
throw an Error (or call response.json() to get server error and throw it) to
drive the .catch branch, only call response.json() when response.ok, and ensure
that in the catch you call toast.error(t("loadFailed")) and do not call
setStatus; keep the finally behavior that sets setLoading(false) when active.
Target the promise chain around fetch(...) / TotpStatus handling and the
setStatus/setLoading/toast.error usages.
In `@src/app/api/account/security/totp/route.ts`:
- Around line 76-85: When handling body.action === "enable", prevent silent
replacement of an existing TOTP secret by first loading the user's current
settings (check settings.totpEnabled and settings.totpSecret for
context.subjectId) and—if settings.totpEnabled is true—require body.oldOtpCode
and validate it with verifyTotp({ secret: settings.totpSecret, code: oldOtpCode
}) before calling saveTotpEnabled; if the old code verification fails return the
same OTP_INVALID (or a distinct error) response. Ensure you reference
verifyTotp, settings.totpEnabled, settings.totpSecret, saveTotpEnabled and
body.oldOtpCode when making this change.
- Around line 90-103: The DELETE handler for disabling TOTP (function DELETE)
currently only checks CSRF and session and then calls
disableTotp(context.subjectId), allowing MFA to be turned off without
re-authentication; require a second factor by accepting an otpCode in the
request body, verify it against the user’s TOTP before calling disableTotp
(e.g., parse otpCode from request, call your existing TOTP verification routine
such as verifyTotp/validateTotp with context.subjectId and otpCode, return
401/403 if verification fails), and update the client-side disableTotp() call in
security-settings-client.tsx to open a confirmation prompt and include otpCode
in the DELETE body.
In `@src/drizzle/schema.ts`:
- Line 159: The schema currently persists totp_secret in plaintext (symbol:
totpSecret), which is insecure; change the column to store ciphertext (e.g.,
binary/text for encrypted data) and add a key-version column (e.g.,
totpKeyVersion) to support rotation, then update application code to encrypt via
a secure AEAD routine (e.g., encryptTotpSecret) before insert/update and decrypt
(decryptTotpSecret) on read; also add a migration to re-encrypt existing
plaintext values and ensure secrets are never written or logged in plain text in
functions that interact with totpSecret/totpKeyVersion.
In `@src/lib/security/totp.ts`:
- Around line 126-135: The verifyTotp function currently derives `digits` from
the user input length (code.length) which conflicts with buildTotpAuthUri that
hardcodes `digits=6`, widening accepted inputs; fix by enforcing a consistent
digit count: change verifyTotp to use a fixed digits value (6) or read the
configured digits from the same account/storage source used by buildTotpAuthUri
instead of using `code.length`, and update the validation regex and candidate
generation in verifyTotp accordingly (refer to verifyTotp, buildTotpAuthUri, and
the `digits` variable).
---
Nitpick comments:
In `@src/app/`[locale]/dashboard/_components/user-menu.tsx:
- Around line 67-78: The submenu under DropdownMenuSub only contains one item so
the nested hover structure is unnecessary; replace the
DropdownMenuSub/DropdownMenuSubTrigger/DropdownMenuSubContent trio with a single
top-level DropdownMenuItem that navigates to "/settings/security" (the existing
onSelect that calls router.push("/settings/security")) and preserves the icon
(ShieldCheck) and label (t("securitySettings")), removing the extra
DropdownMenuSub elements (DropdownMenuSubTrigger and DropdownMenuSubContent) to
flatten the menu.
In `@src/app/`[locale]/settings/_lib/nav-items.ts:
- Around line 42-47: The Security nav item currently reuses iconName
"shield-alert" which duplicates the Sensitive Words icon; change the Security
item's iconName (the object with href "/settings/security") to a distinct
semantic name like "lock", then add that new literal to the SettingsNavIconName
union (lines defining the type) and finally add the corresponding icon mapping
in the icon-rendering code (the component/function that reads iconName and
returns an icon) so "lock" is rendered correctly; ensure the new literal name is
used consistently across the nav item, the SettingsNavIconName type, and the
render mapping.
In `@src/app/`[locale]/settings/security/_components/security-settings-client.tsx:
- Around line 118-133: The disableTotp function currently uses window.confirm;
replace this with the project's AlertDialog (shadcn) pattern so the confirmation
UI is themeable/localizable and accessible: change the flow so clicking the
"disable" button opens AlertDialog (with localized text via t and optional risk
copy), and only after the user confirms invoke the existing async disable
handler (the logic inside disableTotp that calls fetch
"/api/account/security/totp" and uses setSaving, setStatus, toast). Keep the
error handling and finally setSaving(false) behavior, and ensure cancellation
from the dialog simply closes it without calling fetch.
- Around line 106-110: The optimistic write uses local time via new
Date().toISOString() when calling setStatus — this can diverge from the
server-side timestamp written by saveTotpEnabled; instead have the enable TOTP
API return the authoritative boundAt timestamp and update the client state from
that response (e.g. call the enable/saveTotpEnabled endpoint, read
response.boundAt, then call setStatus({ enabled: true, boundAt: response.boundAt
}) and only after that setSetupOpen(false), setSetup(null), setOtpCode(""),
toast.success(t("enabled"))). Ensure saveTotpEnabled (or the controller it
calls) returns the server-side boundAt so the front end uses the canonical time.
In `@src/app/api/account/security/totp/route.ts`:
- Around line 38-103: Add structured audit logging for all TOTP state-changing
flows in POST and DELETE: in POST inside the "setup" branch (where
generateBase32Secret/buildTotpAuthUri are called) emit a "totp.setup" log with
subjectId, action:"setup", client IP and User-Agent from request headers and the
generated secret metadata (but avoid logging full secret if policy prohibits);
in POST "enable" branch (where verifyTotp and saveTotpEnabled are used) log both
failure cases (invalid OTP → include errorCode:"OTP_INVALID") and success (after
saveTotpEnabled → action:"enable", enabled:true); in DELETE (where disableTotp
is called) log action:"disable" with subjectId and result; attach csrf check
context where relevant (csrfResult) and include errorCode:"CSRF_REJECTED" or
"UNAUTHORIZED" when returning those responses; use the existing request,
context.subjectId, and functions saveTotpEnabled/disableTotp/verifyTotp to
locate spots for the new logger calls.
In `@src/app/api/auth/login/route.ts`:
- Around line 276-293: The TOTP check using verifyTotp lacks replay protection;
after verifyTotp returns true you must compare the TOTP counter (or a hash of
the OTP) against the stored last_used value in the user's user_security_settings
and reject the login if the incoming counter is equal or <= stored counter; on
successful login persist the new counter (or OTP hash) back to
user_security_settings. Locate the OTP verification block around verifyTotp in
the login route handler, extract the counter (or derive a stable OTP hash) from
the verification result, check it against the user_security_settings
last_used_counter/otp_hash, return the same 401 OTP_INVALID response when replay
is detected, and update the stored last_used value only after a completed
successful authentication flow.
In `@src/lib/security/totp.ts`:
- Around line 93-106: The TOTP label currently uses encodeURIComponent(label)
which encodes the colon separator; update buildTotpAuthUri so issuer and
accountName are URL-encoded separately and then joined with a literal ':' to
form the label (e.g., const encodedIssuer = encodeURIComponent(issuer); const
encodedAccount = encodeURIComponent(accountName); const label =
`${encodedIssuer}:${encodedAccount}`), keeping the existing issuer/accountName
trimming/fallback logic and leaving the params generation unchanged; use this
new label when constructing the otpauth URI in buildTotpAuthUri.
In `@src/repository/user-security-settings.ts`:
- Around line 13-15: Extract the admin sentinel value -1 into a shared constant
and use it here: import the exported ADMIN_USER_ID from the auth module and
replace the literal check in getSecuritySubjectId (function getSecuritySubjectId
in user-security-settings.ts) so it reads session.user.id === ADMIN_USER_ID ?
"admin-token" : `user:${session.user.id}`; update any related tests/uses to
reference ADMIN_USER_ID as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13f72e16-e621-49bc-8669-2f84b8827efc
📒 Files selected for processing (44)
drizzle/0099_user_security_settings.sqldrizzle/meta/_journal.jsonmessages/en/auth.jsonmessages/en/dashboard.jsonmessages/en/settings/index.tsmessages/en/settings/nav.jsonmessages/en/settings/security.jsonmessages/ja/auth.jsonmessages/ja/dashboard.jsonmessages/ja/settings/index.tsmessages/ja/settings/nav.jsonmessages/ja/settings/security.jsonmessages/ru/auth.jsonmessages/ru/dashboard.jsonmessages/ru/settings/index.tsmessages/ru/settings/nav.jsonmessages/ru/settings/security.jsonmessages/zh-CN/auth.jsonmessages/zh-CN/dashboard.jsonmessages/zh-CN/settings/index.tsmessages/zh-CN/settings/nav.jsonmessages/zh-CN/settings/security.jsonmessages/zh-TW/auth.jsonmessages/zh-TW/dashboard.jsonmessages/zh-TW/settings/index.tsmessages/zh-TW/settings/nav.jsonmessages/zh-TW/settings/security.jsonsrc/app/[locale]/dashboard/_components/user-menu.tsxsrc/app/[locale]/login/page.tsxsrc/app/[locale]/settings/_lib/nav-items.tssrc/app/[locale]/settings/security/_components/security-settings-client.tsxsrc/app/[locale]/settings/security/page.tsxsrc/app/api/account/security/totp/route.tssrc/app/api/auth/login/route.tssrc/drizzle/schema.tssrc/lib/security/totp.tssrc/repository/user-security-settings.tstests/security/session-login-integration.test.tstests/security/totp.test.tstests/unit/api/account-security-totp.test.tstests/unit/api/auth-login-route.test.tstests/unit/api/auth-login-totp.test.tstests/unit/auth/login-page-totp.test.tsxtests/unit/security/user-security-settings-subject.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR adds user-managed TOTP MFA with a solid security foundation: CSRF protection on mutations, constant-time code comparison, no secret exposure on status reads, and correct placement of the TOTP check after key validation but before cookie setting. The core implementation is well-structured with focused test coverage. The only issues found are missing Russian translations in two i18n files.
PR Size: XL
- Lines changed: 1,629 (1,594 additions + 35 deletions)
- Files changed: 44
Split suggestions (recommended for future PRs of this scope):
- Database layer - schema, migration, repository (
user-security-settings.ts) - TOTP core library -
totp.ts+tests/security/totp.test.ts - Login TOTP integration - login route changes + login page OTP UI + auth tests
- Security settings UI & API - settings page, client component, TOTP API endpoint + tests
- i18n - all message files across 5 locales
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Standards (i18n) | 0 | 0 | 2 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues (Should Fix)
-
messages/ru/settings/security.json- Entire file is in English instead of Russian. All other locales (zh-CN, zh-TW, ja) have proper translations. Violates CLAUDE.md: "All user-facing strings must use i18n (5 languages supported)." -
messages/ru/dashboard.jsonline 790 -"securitySettings": "Security Settings"is untranslated. The adjacent"settings"key is correctly translated as"Настройки".
Review Coverage
- Logic and correctness - TOTP check correctly placed after key validation, before cookie set
- Security (OWASP Top 10) - CSRF guard on mutations, constant-time comparison, no secret on GET, no-store headers
- Error handling - All catch blocks surface errors via toast or error responses; no silent failures
- Type safety - Clean interfaces, no
anyusage in new code, properPick<>typing - Documentation accuracy - No misleading comments
- Test coverage - TOTP RFC vectors, login OTP flow, API CRUD, CSRF rejection, UI flow
- Code clarity - Well-structured modules with clear separation of concerns
Automated review by Claude AI
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/security/totp-secret-encryption.test.ts (1)
1-36:⚠️ Potential issue | 🟡 Minor单元测试文件位置不符合目录规范
该文件是单元测试，但当前放在
tests/security/。建议迁移到tests/unit/下对应目录，避免测试分层继续漂移。As per coding guidelines "Unit tests should be located in tests/unit/, integration tests in tests/integration/, and source-adjacent tests in src/**/*.test.ts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/totp-secret-encryption.test.ts` around lines 1 - 36, This test file belongs in the unit-tests tree: move the "TOTP secret encryption" test into the project's unit test directory (tests/unit) under the corresponding security subfolder, updating imports if needed so the referenced functions decryptTotpSecret and encryptTotpSecret still resolve; ensure existing vi.mock hoisting and beforeEach setup remain unchanged and adjust any test runner/config globs so this file is treated as a unit test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/account/security/totp/route.ts`:
- Around line 52-83: recordTotpAudit currently calls createAuditLogAsync with
void (fire-and-forget) while callers await recordTotpAudit, causing mismatched
semantics and unhandled rejection risk; change recordTotpAudit to await
createAuditLogAsync(...) inside the function and wrap that await in a try/catch
so any errors are caught and logged (but not rethrown) so callers’ awaits
reflect completion without propagating audit failures, and ensure the function
signature returns Promise<void> (symbols: recordTotpAudit, createAuditLogAsync,
resolveClientIp).
- Around line 165-170: The code currently only verifies old OTP when both
settings.totpEnabled and settings.totpSecret are truthy, which lets a state of
totpEnabled=true and totpSecret=null bypass validation; update the
enable/replace branch in route.ts to fail-closed: if settings.totpEnabled is
true but settings.totpSecret is missing, call recordTotpAudit(request, context,
"totp.enable", false, "MISSING_TOTP_SECRET") and return the same error response
used for DELETE (or a consistent error) instead of proceeding, otherwise
continue to call verifyTotp({ secret: settings.totpSecret, code: oldOtpCode })
and handle failures with otpInvalidResponse().
In `@src/app/api/auth/login/route.ts`:
- Around line 267-295: Replace the hardcoded English messages in the OTP error
responses with i18n keys only: remove the literal "Verification code required"
and "Invalid verification code" and instead rely solely on the translation
fallback chain (e.g. use t?.("otpRequired") ?? t?.("loginFailed") or another
generic translation key like t?.("unknownError") as the final fallback). Update
the two error assignments where error is set (the branch returning OTP_REQUIRED
and the branch returning OTP_INVALID) so they no longer include literal strings,
and ensure the translation keys ("otpRequired", "otpInvalid", "loginFailed" or
"unknownError") exist in the i18n resources for the supported locales.
In `@src/lib/security/totp-secret-encryption.ts`:
- Around line 61-69: The decryption block using createDecipheriv/getAesKey with
AUTH_TAG_LENGTH and decipher.setAuthTag can throw on corrupted ciphertext and
currently bubbles up; wrap the decrypt logic (createDecipheriv, setAuthTag,
update, final, Buffer.concat and toString) in a try/catch and on any error
return null (or the controlled value used by callers) so a bad "v1" payload does
not cause a 500; ensure the catch is narrow and does not swallow unrelated
errors and adjust callers if they expect a non-null value.
In `@tests/unit/auth/login-page-totp.test.tsx`:
- Line 8: The import in the test uses a relative path; update it to use the
project path alias so it follows the repo convention: replace the relative
import of LoginPage (import LoginPage from
"../../../src/app/[locale]/login/page") with the alias form using "@/": import
LoginPage from "@/app/[locale]/login/page" so tests reference the same module
via the `@/` -> ./src/ mapping.
---
Outside diff comments:
In `@tests/security/totp-secret-encryption.test.ts`:
- Around line 1-36: This test file belongs in the unit-tests tree: move the
"TOTP secret encryption" test into the project's unit test directory
(tests/unit) under the corresponding security subfolder, updating imports if
needed so the referenced functions decryptTotpSecret and encryptTotpSecret still
resolve; ensure existing vi.mock hoisting and beforeEach setup remain unchanged
and adjust any test runner/config globs so this file is treated as a unit test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d90be5d-a9f7-4469-ac22-4cc10ecdee73
📒 Files selected for processing (42)
.env.exampledeploy/.env.exampledrizzle/0099_user_security_settings.sqlmessages/en/auth.jsonmessages/en/settings/security.jsonmessages/ja/auth.jsonmessages/ja/settings/security.jsonmessages/ru/auth.jsonmessages/ru/dashboard.jsonmessages/ru/settings/security.jsonmessages/zh-CN/auth.jsonmessages/zh-CN/settings/security.jsonmessages/zh-TW/auth.jsonmessages/zh-TW/settings/security.jsonpostcss.config.mjssrc/app/[locale]/dashboard/_components/user-menu.tsxsrc/app/[locale]/login/page.tsxsrc/app/[locale]/settings/_components/settings-nav.tsxsrc/app/[locale]/settings/_lib/nav-items.tssrc/app/[locale]/settings/security/_components/security-settings-client.tsxsrc/app/api/account/security/totp/route.tssrc/app/api/auth/login/route.tssrc/drizzle/schema.tssrc/lib/auth.tssrc/lib/config/env.schema.tssrc/lib/security/totp-secret-encryption.tssrc/lib/security/totp.tssrc/repository/user-security-settings.tstests/security/auth-bruteforce-integration.test.tstests/security/auth-csrf-route-integration.test.tstests/security/security-headers-integration.test.tstests/security/session-cookie-hardening.test.tstests/security/session-login-integration.test.tstests/security/totp-secret-encryption.test.tstests/security/totp.test.tstests/unit/api/account-security-totp.test.tstests/unit/api/auth-login-route.test.tstests/unit/api/auth-login-totp.test.tstests/unit/auth/login-page-totp.test.tsxtests/unit/dashboard/dashboard-home-layout.test.tsxtests/unit/login/login-regression-matrix.test.tsxtests/unit/security/user-security-settings-subject.test.ts
✅ Files skipped from review due to trivial changes (15)
- src/app/[locale]/settings/_components/settings-nav.tsx
- .env.example
- deploy/.env.example
- messages/zh-TW/settings/security.json
- messages/zh-CN/settings/security.json
- messages/ru/settings/security.json
- src/lib/auth.ts
- messages/en/settings/security.json
- messages/ru/dashboard.json
- tests/unit/security/user-security-settings-subject.test.ts
- messages/en/auth.json
- drizzle/0099_user_security_settings.sql
- tests/security/totp.test.ts
- messages/zh-CN/auth.json
- src/app/[locale]/settings/security/_components/security-settings-client.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- messages/ja/settings/security.json
- tests/security/session-login-integration.test.ts
- src/drizzle/schema.ts
- src/lib/security/totp.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 674d940cf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2855382e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/app/api/auth/login/route.ts (1)
261-306:⚠️ Potential issue | 🟡 MinorOTP 错误分支里不要再回退到英文文案。
"Internal server error"、"Verification code required"、"Invalid verification code"这几个 literal 仍可能直接返回给用户，导致新增的 TOTP 登录路径出现英文直出。这里建议只走翻译键回退链。可参考的收敛方式
- const serverError = t?.("serverError") ?? "Internal server error"; + const serverError = t?.("serverError") ?? ""; - const error = t?.("otpRequired") ?? t?.("loginFailed") ?? "Verification code required"; + const error = t?.("otpRequired") ?? t?.("loginFailed") ?? t?.("serverError") ?? ""; - const error = t?.("otpInvalid") ?? t?.("loginFailed") ?? "Invalid verification code"; + const error = t?.("otpInvalid") ?? t?.("loginFailed") ?? t?.("serverError") ?? "";As per coding guidelines "All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/auth/login/route.ts` around lines 261 - 306, Several branches return hardcoded English literals ("Internal server error", "Verification code required", "Invalid verification code"); replace those fallbacks with i18n-only fallbacks so no literal English is returned. In the totpSecret check (serverError), the otpCode check (error), and the otpCounterAccepted failure branch (error) remove the string literal fallbacks and use only translation lookups (e.g. t("serverError") ?? t("loginFailed") and t("otpRequired") ?? t("loginFailed") and t("otpInvalid") ?? t("loginFailed")) so the responses produced by NextResponse.json (via withAuthResponseHeaders) never contain hardcoded text; keep the same response shapes, status codes, and calls to verifyTotpAndGetCounter, saveTotpLastUsedCounter, loginPolicy.recordFailure, and createAuditLogAsync.src/app/api/account/security/totp/route.ts (1)
62-93:⚠️ Potential issue | 🟠 Major
recordTotpAudit()里不要继续 fire-and-forget。调用方都在
await recordTotpAudit(...)，但这里的void createAuditLogAsync(...)会让await提前结束；一旦审计写入失败，也只会变成未处理 rejection / 丢日志。建议在函数内真正await审计写入，并吞掉异常后记录错误。可参考的修正方式
async function recordTotpAudit( request: NextRequest, context: NonNullable<Awaited<ReturnType<typeof getSecurityContext>>>, actionType: string, success: boolean, errorMessage?: string ) { let operatorIp = "unknown"; try { operatorIp = await resolveClientIp(request); } catch (error) { logger.warn("TOTP audit IP resolution failed", { subjectId: context.subjectId, error: error instanceof Error ? error.message : String(error), }); } - void createAuditLogAsync({ - actionCategory: "auth", - actionType, - targetType: "security_settings", - targetId: context.subjectId, - operatorUserId: context.session.user.id, - operatorUserName: context.session.user.name, - operatorKeyId: context.session.key?.id ?? null, - operatorKeyName: context.session.key?.name ?? null, - operatorIp, - userAgent: request.headers.get("user-agent"), - success, - errorMessage, - }); + try { + await createAuditLogAsync({ + actionCategory: "auth", + actionType, + targetType: "security_settings", + targetId: context.subjectId, + operatorUserId: context.session.user.id, + operatorUserName: context.session.user.name, + operatorKeyId: context.session.key?.id ?? null, + operatorKeyName: context.session.key?.name ?? null, + operatorIp, + userAgent: request.headers.get("user-agent"), + success, + errorMessage, + }); + } catch (error) { + logger.error("TOTP audit write failed", { + subjectId: context.subjectId, + actionType, + error: error instanceof Error ? error.message : String(error), + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/account/security/totp/route.ts` around lines 62 - 93, The function recordTotpAudit currently fire-and-forgets createAuditLogAsync (using void) which makes callers' await return early and loses failures; change it to await createAuditLogAsync(...) inside the function and wrap that await in a try/catch so any errors are caught, logged (use logger.error with context like actionType/subjectId/operatorUserId) and swallowed (do not rethrow) so the caller still awaits completion but the audit failure doesn't crash the flow; keep the existing IP resolution try/catch as-is and reference createAuditLogAsync and recordTotpAudit to locate the change.
🧹 Nitpick comments (1)
tests/security/totp.test.ts (1)
1-50: 这个 helper suite 建议放到tests/unit/（或源文件旁）。这里测的是
src/lib/security/totp.ts的纯函数行为，继续放在tests/security/会把测试目录约定再扩成第三种形态。更适合迁到tests/unit/，或者直接放到src/lib/security/totp.test.ts。As per coding guidelines "Unit tests should be located in tests/unit/, integration tests in tests/integration/, and source-adjacent tests in src/**/*.test.ts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/totp.test.ts` around lines 1 - 50, Move this pure unit test file tests/security/totp.test.ts into the unit-tests area (either tests/unit/ or next to the implementation as src/lib/security/totp.test.ts) so it follows the repo convention; update any import paths if you place it next to the source to keep imports of generateTotp, verifyTotp, and verifyTotpAndGetCounter correct, and remove the now-empty tests/security directory if no other tests remain there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/account/security/totp/route.ts`:
- Around line 182-218: The current flow verifies the new secret with
verifyTotp(...) then calls saveTotpEnabled(...) which resets totpLastUsedCounter
to null, allowing immediate replay; replace the verifyTotp(...) call with a new
verifyTotpAndGetCounter(...) that returns the matched counter, then pass that
counter into saveTotpEnabled(subjectId, secret, initialCounter) so the
repository persists totpLastUsedCounter = initialCounter (instead of null); also
update the storage/repo signature for saveTotpEnabled to accept initialCounter
and stop writing null, and keep the existing audit/log calls unchanged.
In `@tests/unit/login/login-regression-matrix.test.tsx`:
- Around line 110-119: The beforeEach currently fixes
mockGetUserSecuritySettings to totpEnabled: false which prevents any OTP
branches from being exercised; change the test matrix to parameterize
totpEnabled (true/false) and adjust mockGetUserSecuritySettings to return values
based on the current test params (e.g., read from the matrix entry or a local
variable set in each it/describe block) so cases cover
OTP_REQUIRED/OTP_INVALID/OTP_NOT_CONFIGURED and replay-counter logic; update any
helpers that rely on mockGetSecuritySubjectId if needed to produce distinct
subjectIds for OTP-true scenarios and add at least one matrix entry with
totpEnabled: true and appropriate totpSecret/totpLastUsedCounter values.
- Line 110: 当前在 tests/unit/login/login-regression-matrix.test.tsx 中直接用
mockGetSecuritySubjectId.mockReturnValue("user:1") 固定返回会掩盖 admin-token 与
user:<id> 的映射回归；请改为使用 mockGetSecuritySubjectId.mockImplementation((session) =>
...) 根据传入的 session（或 token）动态返回不同的 subjectId（例如为 admin-token 返回 "admin:<id>"、为普通
session 返回 "user:<id>"），并在关键用例（admin/readonly/dashboard）中增加断言检查
mockGetSecuritySubjectId 的调用参数以确保每个用例传入正确的 session/token。
---
Duplicate comments:
In `@src/app/api/account/security/totp/route.ts`:
- Around line 62-93: The function recordTotpAudit currently fire-and-forgets
createAuditLogAsync (using void) which makes callers' await return early and
loses failures; change it to await createAuditLogAsync(...) inside the function
and wrap that await in a try/catch so any errors are caught, logged (use
logger.error with context like actionType/subjectId/operatorUserId) and
swallowed (do not rethrow) so the caller still awaits completion but the audit
failure doesn't crash the flow; keep the existing IP resolution try/catch as-is
and reference createAuditLogAsync and recordTotpAudit to locate the change.
In `@src/app/api/auth/login/route.ts`:
- Around line 261-306: Several branches return hardcoded English literals
("Internal server error", "Verification code required", "Invalid verification
code"); replace those fallbacks with i18n-only fallbacks so no literal English
is returned. In the totpSecret check (serverError), the otpCode check (error),
and the otpCounterAccepted failure branch (error) remove the string literal
fallbacks and use only translation lookups (e.g. t("serverError") ??
t("loginFailed") and t("otpRequired") ?? t("loginFailed") and t("otpInvalid") ??
t("loginFailed")) so the responses produced by NextResponse.json (via
withAuthResponseHeaders) never contain hardcoded text; keep the same response
shapes, status codes, and calls to verifyTotpAndGetCounter,
saveTotpLastUsedCounter, loginPolicy.recordFailure, and createAuditLogAsync.
---
Nitpick comments:
In `@tests/security/totp.test.ts`:
- Around line 1-50: Move this pure unit test file tests/security/totp.test.ts
into the unit-tests area (either tests/unit/ or next to the implementation as
src/lib/security/totp.test.ts) so it follows the repo convention; update any
import paths if you place it next to the source to keep imports of generateTotp,
verifyTotp, and verifyTotpAndGetCounter correct, and remove the now-empty
tests/security directory if no other tests remain there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1af79bec-f890-4d32-b27a-95b9f84d6774
📒 Files selected for processing (20)
.env.exampledeploy/.env.exampledrizzle/0099_user_security_settings.sqlsrc/app/api/account/security/totp/route.tssrc/app/api/auth/login/route.tssrc/drizzle/schema.tssrc/lib/security/totp-secret-encryption.tssrc/lib/security/totp.tssrc/repository/user-security-settings.tstests/security/auth-bruteforce-integration.test.tstests/security/auth-csrf-route-integration.test.tstests/security/security-headers-integration.test.tstests/security/session-cookie-hardening.test.tstests/security/session-login-integration.test.tstests/security/totp-secret-encryption.test.tstests/security/totp.test.tstests/unit/api/account-security-totp.test.tstests/unit/api/auth-login-route.test.tstests/unit/api/auth-login-totp.test.tstests/unit/login/login-regression-matrix.test.tsx
✅ Files skipped from review due to trivial changes (5)
- deploy/.env.example
- tests/unit/api/auth-login-route.test.ts
- tests/security/session-login-integration.test.ts
- drizzle/0099_user_security_settings.sql
- src/drizzle/schema.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- .env.example
- tests/security/session-cookie-hardening.test.ts
- tests/security/security-headers-integration.test.ts
- tests/security/auth-bruteforce-integration.test.ts
- tests/security/totp-secret-encryption.test.ts
- src/lib/security/totp-secret-encryption.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/app/api/account/security/totp/route.ts (1)
205-222:⚠️ Potential issue | 🟠 Major启用时应把这次绑定成功的 counter 一并持久化。
这里仍然先用
verifyTotp(...)校验 pending secret，再直接saveTotpEnabled(...)。这样用于绑定的这枚 6 位码在同一个 30 秒窗口内还能立刻被复用到后续登录或敏感操作，首次绑定没有真正纳入回放保护。Proposed fix
- if (!verifyTotp({ secret: settings.totpPendingSecret, code: otpCode })) { + const pendingOtpResult = verifyTotpAndGetCounter({ + secret: settings.totpPendingSecret, + code: otpCode, + }); + if (!pendingOtpResult) { await recordTotpAudit(request, context, "totp.enable", false, "OTP_INVALID"); return otpInvalidResponse(); } @@ - const boundAt = await saveTotpEnabled(context.subjectId, settings.totpPendingSecret); + const boundAt = await saveTotpEnabled( + context.subjectId, + settings.totpPendingSecret, + pendingOtpResult.counter, + );
saveTotpEnabled的仓储签名也要同步接收initialCounter，不要再把totpLastUsedCounter重置为null。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/account/security/totp/route.ts` around lines 205 - 222, When enabling TOTP, persist the one-time-code's counter alongside the pending secret so the first successful binding is protected from replay: modify the flow around verifyTotp/verifyCurrentTotpForMutation so that verifyTotp returns (or you compute) the matched counter for settings.totpPendingSecret and otpCode, then pass that initialCounter into saveTotpEnabled(context.subjectId, settings.totpPendingSecret, initialCounter) instead of leaving totpLastUsedCounter null; update the saveTotpEnabled repository signature to accept and store initialCounter (and ensure recordTotpAudit and related callers remain unchanged).src/app/api/auth/login/route.ts (1)
261-266:⚠️ Potential issue | 🟡 Minor移除这里新增的英文兜底。
OTP_NOT_CONFIGURED分支又回退到了"Internal server error"，这仍然会把英文直接返回给用户。建议和下面两个 OTP 分支保持一致，只走翻译键回退链。As per coding guidelines "All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/auth/login/route.ts` around lines 261 - 266, The branch handling missing TOTP currently falls back to a hardcoded English string ("Internal server error"); remove that literal and use the i18n translation chain like the other OTP branches. Specifically, in the block that checks totpSecret and returns withAuthResponseHeaders/NextResponse.json for errorCode "OTP_NOT_CONFIGURED", replace the assignment const serverError = t?.("serverError") ?? "Internal server error" with a translation-only fallback (e.g., const serverError = t?.("serverError") or the same OTP-specific translation chain used in the other OTP branches) so no hardcoded English text is returned.
🧹 Nitpick comments (1)
tests/unit/auth/login-page-totp.test.tsx (1)
41-45: 让 mock Response 带上真实的 HTTP 状态码。现在的 mock 只有
ok/json，没有status。真实登录接口里 OTP_REQUIRED 是401、成功是200；如果页面后续改成按response.status分支，这个测试会继续通过，但已经和真实契约脱节了。建议修改
-function mockJsonResponse(payload: unknown, ok = true): Response { +function mockJsonResponse(payload: unknown, status = 200): Response { return { - ok, + ok: status >= 200 && status < 300, + status, json: async () => payload, } as Response; } @@ return Promise.resolve( mockJsonResponse( { errorCode: "OTP_REQUIRED", requiresOtp: true, otp: { method: "totp" } }, - false + 401 ) ); } return Promise.resolve( - mockJsonResponse({ - ok: true, - redirectTo: "/dashboard", - loginType: "dashboard_user", - }) + mockJsonResponse( + { + ok: true, + redirectTo: "/dashboard", + loginType: "dashboard_user", + }, + 200 + ) );Also applies to: 95-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth/login-page-totp.test.tsx` around lines 41 - 45, The mockJsonResponse helper currently returns a Response with only ok and json, which disconnects tests from real HTTP semantics; update mockJsonResponse to include a status property (e.g. accept an optional status param or derive status from the ok flag: status = ok ? 200 : 401) and ensure all uses (including the other mocks around lines 95-111) supply appropriate status codes for OTP_REQUIRED (401) and success (200) so tests reflect real response.status branching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/account/security/totp/route.ts`:
- Around line 190-218: Add short-window failure throttling for TOTP verification
in the "enable" (and similarly the "disable" branch around the verify code at
the other location) by tracking attempts keyed by subjectId + operatorIp; before
calling verifyTotp or verifyCurrentTotpForMutation check a rate-limit store
(in-memory/redis) for recent failures and, if over threshold,
recordTotpAudit(request, context, "totp.enable"/"totp.disable", false,
"OTP_RATE_LIMITED") and return an appropriate throttled response; on every
failed verifyTotp or failed verifyCurrentTotpForMutation increment the failure
counter with an expiry (short window) and log the event via recordTotpAudit so
repeated failed attempts are blocked briefly and audited.
In `@src/app/api/auth/login/route.ts`:
- Line 186: 在处理请求体解析处（即 route.ts 中执行 const { key, otpCode } = await
request.json();）单独捕获 JSON 解析错误并返回 400，而不是让它落到外层通用 catch 导致 500。具体做法：在读取
request.json() 周围添加一个局部 try/catch，捕获 SyntaxError（或校验到返回值不是对象/缺少必需字段）的情况并立即返回一个
400 响应（说明“Invalid JSON”或“Malformed request body”），其余逻辑仍在外层正常执行以处理业务错误。确保引用到
request.json() 的位置和对 key/otpCode 的解构校验都包含在该局部处理逻辑中。
---
Duplicate comments:
In `@src/app/api/account/security/totp/route.ts`:
- Around line 205-222: When enabling TOTP, persist the one-time-code's counter
alongside the pending secret so the first successful binding is protected from
replay: modify the flow around verifyTotp/verifyCurrentTotpForMutation so that
verifyTotp returns (or you compute) the matched counter for
settings.totpPendingSecret and otpCode, then pass that initialCounter into
saveTotpEnabled(context.subjectId, settings.totpPendingSecret, initialCounter)
instead of leaving totpLastUsedCounter null; update the saveTotpEnabled
repository signature to accept and store initialCounter (and ensure
recordTotpAudit and related callers remain unchanged).
In `@src/app/api/auth/login/route.ts`:
- Around line 261-266: The branch handling missing TOTP currently falls back to
a hardcoded English string ("Internal server error"); remove that literal and
use the i18n translation chain like the other OTP branches. Specifically, in the
block that checks totpSecret and returns
withAuthResponseHeaders/NextResponse.json for errorCode "OTP_NOT_CONFIGURED",
replace the assignment const serverError = t?.("serverError") ?? "Internal
server error" with a translation-only fallback (e.g., const serverError =
t?.("serverError") or the same OTP-specific translation chain used in the other
OTP branches) so no hardcoded English text is returned.
---
Nitpick comments:
In `@tests/unit/auth/login-page-totp.test.tsx`:
- Around line 41-45: The mockJsonResponse helper currently returns a Response
with only ok and json, which disconnects tests from real HTTP semantics; update
mockJsonResponse to include a status property (e.g. accept an optional status
param or derive status from the ok flag: status = ok ? 200 : 401) and ensure all
uses (including the other mocks around lines 95-111) supply appropriate status
codes for OTP_REQUIRED (401) and success (200) so tests reflect real
response.status branching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d55d195-2c66-43a4-8173-e2aba1caa70e
📒 Files selected for processing (3)
src/app/api/account/security/totp/route.tssrc/app/api/auth/login/route.tstests/unit/auth/login-page-totp.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/login/login-regression-matrix.test.tsx (1)
200-221: 建议增强 OTP_REQUIRED 用例的协议与调用链断言。当前已覆盖核心状态码和错误码；建议再断言
otp.method以及 subject/settings 调用参数，进一步防止 OTP 分支静默回归。建议修改
it("TOTP enabled user without OTP: 401 + OTP_REQUIRED", async () => { mockValidateKey.mockResolvedValue(dashboardUserSession); mockGetUserSecuritySettings.mockResolvedValueOnce({ subjectId: "user:1", totpEnabled: true, totpSecret: "JBSWY3DPEHPK3PXP", totpLastUsedCounter: null, totpPendingSecret: null, totpPendingExpiresAt: null, totpBoundAt: new Date("2026-04-27T00:00:00.000Z"), }); const res = await POST(makeRequest({ key: "dashboard-user-key" })); expect(res.status).toBe(401); - expect(await res.json()).toMatchObject({ + const body = await res.json(); + expect(body).toMatchObject({ error: "translated:otpRequired", errorCode: "OTP_REQUIRED", requiresOtp: true, + otp: { method: "totp" }, }); + expect(mockGetSecuritySubjectId).toHaveBeenCalledWith(dashboardUserSession); + expect(mockGetUserSecuritySettings).toHaveBeenCalledWith("user:1"); expect(mockSetAuthCookie).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/login/login-regression-matrix.test.tsx` around lines 200 - 221, Add assertions that the OTP branch returned the expected OTP payload and that the security/settings call chain received correct identifiers: after calling POST(makeRequest(...)) assert the parsed JSON includes otp.method: "totp" (or whatever canonical method string your code returns), and add expect calls verifying mockGetUserSecuritySettings was called with the correct subjectId (e.g., dashboardUserSession.subjectId or "user:1") and that mockValidateKey was invoked with the incoming key; keep the existing expect(mockSetAuthCookie).not.toHaveBeenCalled() check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/login/login-regression-matrix.test.tsx`:
- Around line 200-221: Add assertions that the OTP branch returned the expected
OTP payload and that the security/settings call chain received correct
identifiers: after calling POST(makeRequest(...)) assert the parsed JSON
includes otp.method: "totp" (or whatever canonical method string your code
returns), and add expect calls verifying mockGetUserSecuritySettings was called
with the correct subjectId (e.g., dashboardUserSession.subjectId or "user:1")
and that mockValidateKey was invoked with the incoming key; keep the existing
expect(mockSetAuthCookie).not.toHaveBeenCalled() check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd136006-ae85-46c1-9df5-06133510e8d4
📒 Files selected for processing (8)
.env.exampledeploy/.env.examplesrc/app/api/account/security/totp/route.tssrc/lib/security/totp-secret-encryption.tssrc/repository/user-security-settings.tstests/security/totp-secret-encryption.test.tstests/unit/api/account-security-totp.test.tstests/unit/login/login-regression-matrix.test.tsx
✅ Files skipped from review due to trivial changes (2)
- deploy/.env.example
- src/repository/user-security-settings.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .env.example
- tests/security/totp-secret-encryption.test.ts
- src/lib/security/totp-secret-encryption.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5565c8b67f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
|
您好，这一功能由于实现较复杂，暂不在 Nodejs 版本中考虑。如有安全疑虑，建议使用强口令配合必要的限流措施，并定期轮换密钥。 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
Users authenticate to the web UI with a single API key. If that key is compromised, an attacker gets full dashboard access with no second factor. This PR adds opt-in TOTP two-factor authentication so each user can independently strengthen their login security, building on the session and auth hardening introduced in PR #806.
Related: Follow-up to #806 (security auth overhaul and provider batch operations)
Solution
src/lib/security/totp.ts) implementing RFC 6238 with HMAC-SHA1, 30-second time steps, and a +/- 1 step verification window{ requiresOtp: true }if TOTP is enabled; the frontend prompts for a 6-digit code and resubmitsuser_security_settingstable, keyed by a stable subject identifier (admin-tokenoruser:<id>)QRCodecomponent; the secret is persisted only after the user proves possession with a valid codeChanges
Core
src/lib/security/totp.ts— TOTP generation, verification, base32 encoding, andotpauth://URI buildersrc/app/api/account/security/totp/route.ts— GET (status), POST (setup/enable), DELETE (disable) with CSRF guardsrc/app/api/auth/login/route.ts— OTP challenge step after key validation, before cookie issuancesrc/repository/user-security-settings.ts— Drizzle data access for security settingssrc/drizzle/schema.ts+drizzle/0099_user_security_settings.sql— Newuser_security_settingstableUI
src/app/[locale]/settings/security/— New Security Settings page with TOTP toggle and QR setup dialogsrc/app/[locale]/login/page.tsx— OTP input field, two-phase submit, auto-focus managementsrc/app/[locale]/dashboard/_components/user-menu.tsx— Upgraded to dropdown menu with Settings > Security entrysrc/app/[locale]/settings/_lib/nav-items.ts— Security nav item addedi18n (all 5 languages: en, ja, ru, zh-CN, zh-TW)
messages/*/auth.json— OTP field labels, placeholder, button text, error messagemessages/*/dashboard.json— Security settings menu labelsmessages/*/settings/security.json— Full security settings page translationsmessages/*/settings/nav.json— Security nav labelmessages/*/settings/index.ts— Security namespace importTests
tests/security/totp.test.ts— RFC 6238 test vectors, time-step window, malformed input rejectiontests/unit/api/account-security-totp.test.ts— API endpoint tests (status, setup, enable, disable, CSRF rejection)tests/unit/api/auth-login-totp.test.ts— Login flow with TOTP enabled (challenge, valid/invalid OTP, bypass when disabled)tests/unit/auth/login-page-totp.test.tsx— UI integration test for two-phase OTP submittests/unit/security/user-security-settings-subject.test.ts— Subject ID mappingTesting
bun run lintbun run typechecknode scripts/validate-migrations.js./node_modules/.bin/vitest run tests/unit/api/account-security-totp.test.ts tests/unit/api/auth-login-totp.test.ts tests/unit/api/auth-login-route.test.ts tests/unit/api/auth-login-failure-taxonomy.test.ts tests/security/session-login-integration.test.ts tests/unit/security/user-security-settings-subject.test.ts tests/security/totp.test.ts tests/unit/i18n/auth-login-keys.test.ts tests/unit/auth/login-page-totp.test.tsx --reporter=verboseNotes
Description enhanced by Claude AI
Greptile Summary
This PR adds opt-in TOTP MFA for all users via a custom RFC 6238 implementation, a two-phase login flow, per-user
user_security_settingsstorage with AES-256-GCM encrypted secrets, and a full settings UI. The critical security issues flagged in the previous round (replay protection, server-side pending secret, OTP oracle, disable requiring verification, decryption error handling) have all been addressed in this revision.Confidence Score: 4/5
Safe to merge after addressing the counterToBuffer bitwise overflow and the decryptTotpSecret plaintext fallback; no P0 issues present.
All previous P0/P1 findings have been fixed. Remaining findings are P2: a bitwise coercion in counterToBuffer that is safe today but will silently corrupt TOTP generation if the counter ever exceeds Int32 max (~year 2038), and a silent plaintext fallback in decryptTotpSecret for non-v1 format values.
src/lib/security/totp.ts (counterToBuffer bitwise coercion), src/lib/security/totp-secret-encryption.ts (plaintext fallback path in decryptTotpSecret)
Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User (Browser) participant L as Login API participant S as TOTP Settings API participant DB as user_security_settings note over U,DB: Normal Login (TOTP disabled) U->>L: POST /api/auth/login { key } L->>DB: getUserSecuritySettings(subjectId) DB-->>L: { totpEnabled: false } L-->>U: 200 Set-Cookie (session) note over U,DB: Login with TOTP enabled U->>L: POST /api/auth/login { key } L->>DB: getUserSecuritySettings(subjectId) DB-->>L: { totpEnabled: true, totpSecret } L-->>U: 401 { errorCode: OTP_REQUIRED, requiresOtp: true } U->>L: POST /api/auth/login { key, otpCode } L->>DB: saveTotpLastUsedCounter (atomic WHERE counter < new) DB-->>L: updated (replay prevented) L-->>U: 200 Set-Cookie (session) note over U,DB: TOTP Setup Flow U->>S: POST { action: setup } S->>DB: saveTotpSetupPending(encrypted secret, expiresAt) S-->>U: { secret plaintext, otpauthUri, expiresAt } U->>U: Scan QR in authenticator app U->>S: POST { action: enable, otpCode } S->>DB: getUserSecuritySettings → pendingSecret (decrypted) S->>S: verifyTotpAndGetCounter(pendingSecret, otpCode) S->>DB: saveTotpEnabled(pendingSecret, initialCounter) S-->>U: 200 { enabled: true, boundAt } note over U,DB: TOTP Disable Flow U->>S: DELETE { otpCode } S->>DB: getUserSecuritySettings → totpSecret (decrypted) S->>S: verifyCurrentTotpForMutation (checks replay counter) S->>DB: disableTotp (clears all TOTP fields) S-->>U: 200 { enabled: false }Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "fix: resync totp setup status" | Re-trigger Greptile