feat(security): auth & session security overhaul#803
Conversation
… failure taxonomy, i18n parity
…ation, a11y overlay
…usage-doc HttpOnly alignment
…nti-bruteforce, CSRF guard, security headers
… revocation, bruteforce integration, CSRF guard
…te, quality gates, migration runbook, go-live checklist
|
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新增与重构认证与安全层：引入不透明会话（Redis）、会话模式（legacy/dual/opaque）、CSRF 原点守护、登录滥用策略、响应安全头、登录页状态/重定向安全与 provider 批量 patch 流程，并添加大量单元/集成/回归测试。 Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
ding113
changed the title
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly overhauls the application's authentication and session management system, focusing on security hardening. It introduces a robust opaque session mechanism, a phased migration strategy, and comprehensive protection against common web vulnerabilities like bruteforce attacks and CSRF. The changes also include improvements to the login user experience and a strong emphasis on security best practices through new headers and HttpOnly cookies. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
size/XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive security overhaul of the authentication and session management system. The introduction of opaque, server-side sessions backed by Redis, along with session rotation, is a major step forward in mitigating session-related vulnerabilities. The implementation of anti-bruteforce policies, CSRF protection, and standard security headers demonstrates a strong commitment to security best practices. The dual-read migration strategy is well-designed for a seamless transition with zero downtime. The extensive test suite covering the new security features is particularly commendable and provides high confidence in the changes. I have a couple of suggestions for improvement, primarily regarding the scalability of the brute-force protection and a minor code clarity improvement in the logout route. Overall, this is a very high-quality contribution that significantly hardens the application's security posture.
| }; | ||
|
|
||
| export class LoginAbusePolicy { | ||
| private attempts = new Map<string, AttemptRecord>(); |
There was a problem hiding this comment.
The current implementation of LoginAbusePolicy uses an in-memory Map to track login attempts. This is effective for a single-node deployment, but it will not work correctly in a distributed environment (e.g., multiple server instances behind a load balancer, or serverless functions). An attacker could easily bypass this brute-force protection by sending requests to different server instances, as each instance would have its own separate attempt counter.
To be effective in a scaled environment, the attempt and lockout records should be stored in a distributed data store like Redis. This would ensure that the policy is applied consistently across all instances.
| export async function POST(request: NextRequest) { | ||
| const csrfResult = csrfGuard.check(request); | ||
| if (!csrfResult.allowed) { | ||
| return withAuthResponseHeaders( | ||
| NextResponse.json({ error: "Forbidden", errorCode: "CSRF_REJECTED" }, { status: 403 }) | ||
| ); | ||
| } | ||
|
|
||
| const mode = resolveSessionTokenMode(); | ||
|
|
||
| if (mode !== "legacy") { | ||
| try { | ||
| const sessionId = await resolveAuthCookieToken(); | ||
| if (sessionId) { | ||
| const store = new RedisSessionStore(); | ||
| await store.revoke(sessionId); | ||
| } | ||
| } catch (error) { | ||
| logger.warn("[AuthLogout] Failed to revoke opaque session during logout", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| export async function POST() { | ||
| await clearAuthCookie(); | ||
| return NextResponse.json({ ok: true }); | ||
| return withAuthResponseHeaders(NextResponse.json({ ok: true })); | ||
| } |
There was a problem hiding this comment.
The helper functions resolveSessionTokenMode and resolveAuthCookieToken (defined on lines 21-30) use a confusing as unknown as cast, which is generally a code smell and makes the code harder to understand. It seems like a workaround for a testing setup, but production code should be as clear as possible.
I suggest refactoring this POST handler to call the imported functions getSessionTokenMode() and getAuthCookie() directly, and removing the resolve... wrapper functions on lines 21-30.
export async function POST(request: NextRequest) {
const csrfResult = csrfGuard.check(request);
if (!csrfResult.allowed) {
return withAuthResponseHeaders(
NextResponse.json({ error: "Forbidden", errorCode: "CSRF_REJECTED" }, { status: 403 })
);
}
const mode = getSessionTokenMode();
if (mode !== "legacy") {
try {
const sessionId = await getAuthCookie();
if (sessionId) {
const store = new RedisSessionStore();
await store.revoke(sessionId);
}
} catch (error) {
logger.warn("[AuthLogout] Failed to revoke opaque session during logout", {
error: error instanceof Error ? error.message : String(error),
});
}
}
await clearAuthCookie();
return withAuthResponseHeaders(NextResponse.json({ ok: true }));
}![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const ttl = normalizeTtlSeconds(ttlSeconds); | ||
| const createdAt = Date.now(); | ||
| const sessionData: SessionData = { | ||
| sessionId: crypto.randomUUID(), |
There was a problem hiding this comment.
Session ID missing sid_ prefix - detection logic in src/lib/auth.ts:122-127 expects opaque sessions to start with sid_ but crypto.randomUUID() generates plain UUIDs
| sessionId: crypto.randomUUID(), | |
| sessionId: `sid_${crypto.randomUUID()}`, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/auth-session-store/redis-session-store.ts
Line: 105:105
Comment:
Session ID missing `sid_` prefix - detection logic in `src/lib/auth.ts:122-127` expects opaque sessions to start with `sid_` but `crypto.randomUUID()` generates plain UUIDs
```suggestion
sessionId: `sid_${crypto.randomUUID()}`,
```
How can I resolve this? If you propose a fix, please make it concise.| rotate(oldSessionId: string): Promise<SessionData | null>; | ||
| } | ||
|
|
||
| export const DEFAULT_SESSION_TTL = 86400; |
There was a problem hiding this comment.
Session TTL mismatch - hardcoded to 86400s (1 day) but .env.example sets SESSION_TTL=300 (5 min) and env.schema.ts:97 defaults to 300. Store should read from env config.
| export const DEFAULT_SESSION_TTL = 86400; | |
| export const DEFAULT_SESSION_TTL = parseInt(process.env.SESSION_TTL || "300", 10); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/auth-session-store/index.ts
Line: 20:20
Comment:
Session TTL mismatch - hardcoded to 86400s (1 day) but `.env.example` sets `SESSION_TTL=300` (5 min) and `env.schema.ts:97` defaults to 300. Store should read from env config.
```suggestion
export const DEFAULT_SESSION_TTL = parseInt(process.env.SESSION_TTL || "300", 10);
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
tests/security/auth-csrf-route-integration.test.ts-104-108 (1)
104-108:⚠️ Potential issue | 🟡 Minor动态 import 使用相对路径，违反
@/路径别名规范第 104、107 行的动态 import 均使用了相对路径，而规范要求引用
./src/下的文件须使用@/别名。As per coding guidelines: "Use path alias
@/to reference files in./src/directory".🔧 建议修复
- const loginRoute = await import("../../src/app/api/auth/login/route"); + const loginRoute = await import("@/app/api/auth/login/route"); loginPost = loginRoute.POST; - const logoutRoute = await import("../../src/app/api/auth/logout/route"); + const logoutRoute = await import("@/app/api/auth/logout/route"); logoutPost = logoutRoute.POST;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/auth-csrf-route-integration.test.ts` around lines 104 - 108, The tests use dynamic imports with relative paths for auth routes; update the imports in tests/security/auth-csrf-route-integration.test.ts to use the project path alias (e.g., import from "@/app/api/auth/login/route" and "@/app/api/auth/logout/route") so loginRoute (and loginPost) and logoutRoute (and logoutPost) resolve via the `@/` alias per the coding guideline; ensure any test runner/tsconfig paths are already configured to support the alias before changing the import strings..env.example-72-76 (1)
72-76:⚠️ Potential issue | 🟡 Minor
legacy注释可能引发误解第 73 行将
legacy描述为 "用于紧急回滚"，但对于全新部署而言，legacy同样是初始默认状态，并非仅在回滚场景下使用。建议区分"初次部署默认值"与"回滚用途"两种语义，避免运维人员误判迁移阶段。📝 建议修改注释
-# - legacy (默认)：仅接受 legacy（Cookie 值为原始 API Key），用于紧急回滚 +# - legacy (默认)：仅接受 legacy（Cookie 值为原始 API Key）；新部署的初始模式，也可用于紧急回滚🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 72 - 76, Update the comment for SESSION_TOKEN_MODE to avoid implying "legacy" is only for rollbacks: clarify that SESSION_TOKEN_MODE accepts values legacy, dual, and opaque, that legacy is the default for new deployments (and can also be used for emergency rollbacks), dual is a migration window accepting both legacy and opaque, and opaque is the hard switch; mention that operators should choose legacy for initial deployments or dual during migration and opaque for completed migrations so intent is explicit.tests/unit/login/login-regression-matrix.test.tsx-94-95 (1)
94-95:⚠️ Potential issue | 🟡 Minor使用
@/路径别名替代相对路径，符合编码规范第 94 行使用相对路径
"../../../src/app/api/auth/login/route"导入模块，应改用@/路径别名。根据编码规范："Use path alias@/to reference files in./src/directory"，且项目其他测试文件中的动态导入均已采用@/别名。修复建议
- const mod = await import("../../../src/app/api/auth/login/route"); + const mod = await import("@/app/api/auth/login/route"); POST = mod.POST;🤖 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 94 - 95, Replace the dynamic import using the relative path "../../../src/app/api/auth/login/route" with the project path alias import "@/app/api/auth/login/route" in the test where you load the module (the line that sets const mod = await import(...)); keep the subsequent POST assignment (POST = mod.POST) unchanged so the test uses the aliased module reference consistent with other tests and the project's encoding conventions.tests/security/session-fixation-rotation.test.ts-76-82 (1)
76-82:⚠️ Potential issue | 🟡 Minor
simulatePostLoginSessionRotation测试的是 mock 本身而非生产代码
simulatePostLoginSessionRotation直接调用了mockRotate（Line 163），而非通过路由或 auth 模块的实际 rotation 流程。这意味着该测试仅验证了 mock 的返回值配置是否正确，并未测试RedisSessionStore.rotate()的实际逻辑（该逻辑已在session-store.test.ts中覆盖）。如果此测试旨在验证"登录后 rotation 产生新 session ID"的端到端行为，应通过登录路由或 auth 模块的公共 API 触发 rotation。
Also applies to: 152-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/session-fixation-rotation.test.ts` around lines 76 - 82, The test currently exercises the mock by calling simulatePostLoginSessionRotation which directly invokes the provided rotate mock (mockRotate) instead of exercising the real rotation flow; replace the direct mock invocation with an end-to-end trigger—call the public login/auth API or the routing handler that performs post-login rotation (e.g., hit the login route or invoke the auth module's public rotateSession/RedisSessionStore.rotate via the same entrypoint the app uses) so the test validates actual rotation behavior rather than mock return values; ensure simulatePostLoginSessionRotation uses the real request flow (HTTP request to the login route or the auth module's public function) and assert the new session ID is persisted and different from oldSessionId.tests/security/session-cookie-hardening.test.ts-20-26 (1)
20-26:⚠️ Potential issue | 🟡 Minor
withNoStoreHeaders测试验证的是本地副本而非生产代码
realWithNoStoreHeaders（Lines 20-26）是在测试文件中重新实现的函数，并通过vi.mock注入为@/lib/auth的 mock。Lines 80-97 的withNoStoreHeaders utility测试块测试的正是这个本地副本，而非src/lib/auth.ts中的真实实现。这意味着即使生产代码中的
withNoStoreHeaders存在 bug，这些测试仍会通过。建议将这个 describe 块移到单独的单元测试中，直接导入真实的withNoStoreHeaders进行验证。Also applies to: 80-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/session-cookie-hardening.test.ts` around lines 20 - 26, The tests are validating a local test-only reimplementation (realWithNoStoreHeaders) injected via vi.mock instead of the real production function, so move the coverage to test the real implementation: remove or stop mocking withNoStoreHeaders in the "withNoStoreHeaders utility" describe block and create a separate unit test that imports withNoStoreHeaders from the production module (src/lib/auth) and asserts headers are set on a NextResponse instance; alternatively, change the mock setup to use the actual implementation (e.g., useActual or return the real function) so that withNoStoreHeaders in your tests references the real withNoStoreHeaders rather than the hoisted realWithNoStoreHeaders test-only function.tests/unit/api/auth-login-route.test.ts-277-290 (1)
277-290:⚠️ Potential issue | 🟡 Minor
{ error: undefined }作为 API 响应可能不理想。当翻译获取全部失败时，API 返回
{ error: undefined }— 这意味着客户端收到了 401 状态码但没有任何错误信息。虽然这里测试的是实际行为，但从可靠性角度看，路由层可能应该有一个硬编码的最终回退消息（如英文 fallback），而不是返回undefined。这不是测试本身的问题，而是被测路由的潜在改进点。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/auth-login-route.test.ts` around lines 277 - 290, The route currently returns { error: undefined } when both getTranslations and its fallback fail (exercised by the test using mockGetTranslations and POST(makeRequest(...))); update the auth-login route handler to provide a final hard-coded fallback error message (e.g., an English string like "Authentication failed") whenever getTranslations (or its fallback path) throws or returns nothing, ensuring the response for 401 includes a non-undefined error string; locate the code path that calls getTranslations/getTranslationFallback in the auth-login handler and replace the undefined assignment with the constant fallback message so tests and clients always receive a meaningful error.src/lib/security/csrf-origin-guard.ts-18-20 (1)
18-20:⚠️ Potential issue | 🟡 Minor
normalizeOrigin仅做了 trim，缺少大小写归一化，导致配置不匹配风险。根据 RFC 6454 规范，浏览器发送的
Origin头中的 scheme 和 host 部分总是小写的（已在原始计算阶段被规范化）。但当前normalizeOrigin函数仅调用trim()，未调用toLowerCase()。这意味着如果配置中包含"HTTPS://Example.COM"，而浏览器发送"https://example.com"，因为 Set 的比对是大小写敏感的，配置的来源将无法匹配。建议在normalizeOrigin中添加toLowerCase()以规范化配置中可能存在的大小写不一致。建议修改
function normalizeOrigin(origin: string): string { - return origin.trim(); + return origin.trim().toLowerCase(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/security/csrf-origin-guard.ts` around lines 18 - 20, normalizeOrigin currently only trims whitespace which can leave mixed-case schemes/hosts in configured origins and cause mismatches against browser-sent Origin headers; update the normalizeOrigin function to also call toLowerCase() (e.g., apply trim().toLowerCase()) so configured origins are normalized per RFC 6454 and will match the browser Origin comparisons.src/app/api/auth/logout/route.ts-50-75 (1)
50-75:⚠️ Potential issue | 🟡 MinorLogout 流程逻辑合理，但
"Forbidden"仍为硬编码字符串。Line 54 的
error: "Forbidden"与 login 路由存在相同的 i18n 问题。此外，logout 路由无需加载翻译即可完成操作，建议至少将错误消息提取为常量，或仅依赖errorCode而非error文本。As per coding guidelines: "All user-facing strings must use i18n"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/auth/logout/route.ts` around lines 50 - 75, Replace the hard-coded "Forbidden" user-facing string in the POST handler's CSRF rejection response (the call to withAuthResponseHeaders(NextResponse.json({ error: "Forbidden", errorCode: "CSRF_REJECTED" }, { status: 403 }))) with an i18n-aware value or a shared constant; specifically update the POST function so csrfGuard.check failure returns either a localized message via your i18n helper (instead of the literal "Forbidden") or omit the human text and rely on errorCode "CSRF_REJECTED" only, ensuring the change touches the csrfGuard.check branch and the NextResponse.json payload while keeping withAuthResponseHeaders usage intact.src/app/api/auth/login/route.ts-148-152 (1)
148-152:⚠️ Potential issue | 🟡 Minor
"Forbidden"为硬编码的英文字符串，未使用 i18n。Line 150 中
error: "Forbidden"是用户可见的错误信息，应使用翻译函数。但注意此处 CSRF 检查在获取 locale/translations 之前执行，因此无法直接使用t()。可以考虑将 CSRF 检查移到获取翻译之后，或在此处仅返回errorCode而不返回error文本。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 148 - 152, The CSRF rejection response currently returns a hard-coded user-facing string ("Forbidden") in the csrfResult branch; either move the CSRF validation so it runs after you obtain locale/translations and can call the translator (so you can replace the literal with t("...")), or remove the user-facing "error" field here and only return the machine-readable errorCode ("CSRF_REJECTED") from the if (!csrfResult.allowed) block (keep withAuthResponseHeaders and NextResponse.json usage intact). Update the csrf check in route.ts to reference csrfResult, withAuthResponseHeaders, and NextResponse.json accordingly so no hard-coded display text is sent before translations are available.src/lib/auth.ts-292-302 (1)
292-302:⚠️ Potential issue | 🟡 Minor
sessionStorePromise缓存了 import promise，若首次导入失败则后续调用将永远获得 rejected promise。动态 import 的 rejected promise 被缓存后不会重试。如果
RedisSessionStore模块因瞬态原因（如临时文件系统错误）加载失败，所有后续的getSessionStore()调用都会立即失败。建议添加失败重试机制
let sessionStorePromise: Promise<SessionStoreReader> | null = null; async function getSessionStore(): Promise<SessionStoreReader> { if (!sessionStorePromise) { - sessionStorePromise = import("@/lib/auth-session-store/redis-session-store").then( + const promise = import("@/lib/auth-session-store/redis-session-store").then( ({ RedisSessionStore }) => new RedisSessionStore() ); + sessionStorePromise = promise.catch((error) => { + sessionStorePromise = null; + throw error; + }); } return sessionStorePromise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth.ts` around lines 292 - 302, sessionStorePromise currently caches the dynamic import promise so a first-time import failure permanently taints future calls; modify getSessionStore to handle import failures by clearing or replacing sessionStorePromise on rejection and retrying (with optional limited retries/backoff) so transient import errors can recover. Specifically, wrap the import("@/lib/auth-session-store/redis-session-store") call used to construct RedisSessionStore inside a try/catch (or a small retry loop), and if it rejects set sessionStorePromise back to null (or retry) before rethrowing or retrying; keep references to sessionStorePromise, getSessionStore, and RedisSessionStore to locate and update the logic.src/app/[locale]/login/page.tsx-234-234 (1)
234-234:⚠️ Potential issue | 🟡 Minor
"API Key"为硬编码的英文标签，违反 i18n 规则。项目要求支持 5 种语言（zh-CN、zh-TW、en、ja、ru），此处应使用翻译函数。
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"
建议修复
- <Label htmlFor="apiKey">API Key</Label> + <Label htmlFor="apiKey">{t("form.apiKeyLabel")}</Label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/login/page.tsx at line 234, The Label text "API Key" is hard-coded and must use the project's i18n mechanism; replace the literal string inside the Label (the element with htmlFor="apiKey" in src/app/[locale]/login/page.tsx) with a call to the repo's translation function (e.g., t('login.apiKey') or i18n.t('login.apiKey') / useTranslation().t depending on the codebase pattern) and ensure the translation key "login.apiKey" (or your chosen key) is added to all five locale files (zh-CN, zh-TW, en, ja, ru) with appropriate translations; also import or obtain the translator in this file (e.g., const { t } = useTranslation(locale) or equivalent) consistent with nearby components.
src/app/api/auth/logout/route.ts
Outdated
| function applySecurityHeaders(response: NextResponse): NextResponse { | ||
| const env = getEnvConfig(); | ||
| const headers = buildSecurityHeaders({ | ||
| enableHsts: env.ENABLE_SECURE_COOKIES, | ||
| cspMode: "report-only", | ||
| }); | ||
|
|
||
| for (const [key, value] of Object.entries(headers)) { | ||
| response.headers.set(key, value); | ||
| } | ||
|
|
||
| return response; | ||
| } | ||
|
|
||
| function withAuthResponseHeaders(response: NextResponse): NextResponse { | ||
| return applySecurityHeaders(withNoStoreHeaders(response)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
applySecurityHeaders 和 withAuthResponseHeaders 与 login/route.ts 完全重复。
src/app/api/auth/login/route.ts Lines 31-47 包含完全相同的实现。应提取到共享模块（如 @/lib/security/auth-response-headers.ts）中，避免维护两份代码。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/auth/logout/route.ts` around lines 32 - 48, Extract the
duplicated applySecurityHeaders and withAuthResponseHeaders implementations into
a shared module (e.g. create "@/lib/security/auth-response-headers.ts") that
exports applySecurityHeaders and withAuthResponseHeaders; have the new module
import getEnvConfig, buildSecurityHeaders, and withNoStoreHeaders and implement
the same logic there, then replace the local implementations in both
src/app/api/auth/logout/route.ts and src/app/api/auth/login/route.ts with
imports from the new module and remove the duplicated code from those files.
| async create( | ||
| data: Omit<SessionData, "sessionId" | "createdAt" | "expiresAt">, | ||
| ttlSeconds = this.defaultTtlSeconds | ||
| ): Promise<SessionData> { | ||
| const ttl = normalizeTtlSeconds(ttlSeconds); | ||
| const createdAt = Date.now(); | ||
| const sessionData: SessionData = { | ||
| sessionId: crypto.randomUUID(), | ||
| keyFingerprint: data.keyFingerprint, | ||
| userId: data.userId, | ||
| userRole: data.userRole, | ||
| createdAt, | ||
| expiresAt: createdAt + ttl * 1000, | ||
| }; | ||
|
|
||
| const redis = this.getReadyRedis(); | ||
| if (!redis) { | ||
| logger.warn("[AuthSessionStore] Redis not ready during create", { | ||
| sessionId: sessionData.sessionId, | ||
| }); | ||
| return sessionData; | ||
| } | ||
|
|
||
| try { | ||
| await redis.setex(buildSessionKey(sessionData.sessionId), ttl, JSON.stringify(sessionData)); | ||
| } catch (error) { | ||
| logger.error("[AuthSessionStore] Failed to create session", { | ||
| error: toLogError(error), | ||
| sessionId: sessionData.sessionId, | ||
| }); | ||
| } | ||
|
|
||
| return sessionData; | ||
| } |
There was a problem hiding this comment.
create() 在 Redis 不可用或写入失败时仍返回未持久化的 session 对象——这是静默数据丢失。
当 Redis 未就绪（Line 114-118）或 setex 抛出异常（Line 123-128）时，方法仅记录日志但仍返回 sessionData。在 login 路由的 "opaque" 模式下，调用方会将未持久化的 sessionId 写入 cookie，导致用户获得一个永远无法验证的会话令牌。
建议：在持久化失败时抛出异常，让调用方决定如何处理（login 路由的 "opaque" 分支已有外层 catch 会返回 500）。
建议修复：持久化失败时抛出异常
const redis = this.getReadyRedis();
if (!redis) {
logger.warn("[AuthSessionStore] Redis not ready during create", {
sessionId: sessionData.sessionId,
});
- return sessionData;
+ throw new Error("Redis not ready: session not persisted");
}
try {
await redis.setex(buildSessionKey(sessionData.sessionId), ttl, JSON.stringify(sessionData));
} catch (error) {
logger.error("[AuthSessionStore] Failed to create session", {
error: toLogError(error),
sessionId: sessionData.sessionId,
});
+ throw error;
}
return sessionData;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async create( | |
| data: Omit<SessionData, "sessionId" | "createdAt" | "expiresAt">, | |
| ttlSeconds = this.defaultTtlSeconds | |
| ): Promise<SessionData> { | |
| const ttl = normalizeTtlSeconds(ttlSeconds); | |
| const createdAt = Date.now(); | |
| const sessionData: SessionData = { | |
| sessionId: crypto.randomUUID(), | |
| keyFingerprint: data.keyFingerprint, | |
| userId: data.userId, | |
| userRole: data.userRole, | |
| createdAt, | |
| expiresAt: createdAt + ttl * 1000, | |
| }; | |
| const redis = this.getReadyRedis(); | |
| if (!redis) { | |
| logger.warn("[AuthSessionStore] Redis not ready during create", { | |
| sessionId: sessionData.sessionId, | |
| }); | |
| return sessionData; | |
| } | |
| try { | |
| await redis.setex(buildSessionKey(sessionData.sessionId), ttl, JSON.stringify(sessionData)); | |
| } catch (error) { | |
| logger.error("[AuthSessionStore] Failed to create session", { | |
| error: toLogError(error), | |
| sessionId: sessionData.sessionId, | |
| }); | |
| } | |
| return sessionData; | |
| } | |
| async create( | |
| data: Omit<SessionData, "sessionId" | "createdAt" | "expiresAt">, | |
| ttlSeconds = this.defaultTtlSeconds | |
| ): Promise<SessionData> { | |
| const ttl = normalizeTtlSeconds(ttlSeconds); | |
| const createdAt = Date.now(); | |
| const sessionData: SessionData = { | |
| sessionId: crypto.randomUUID(), | |
| keyFingerprint: data.keyFingerprint, | |
| userId: data.userId, | |
| userRole: data.userRole, | |
| createdAt, | |
| expiresAt: createdAt + ttl * 1000, | |
| }; | |
| const redis = this.getReadyRedis(); | |
| if (!redis) { | |
| logger.warn("[AuthSessionStore] Redis not ready during create", { | |
| sessionId: sessionData.sessionId, | |
| }); | |
| throw new Error("Redis not ready: session not persisted"); | |
| } | |
| try { | |
| await redis.setex(buildSessionKey(sessionData.sessionId), ttl, JSON.stringify(sessionData)); | |
| } catch (error) { | |
| logger.error("[AuthSessionStore] Failed to create session", { | |
| error: toLogError(error), | |
| sessionId: sessionData.sessionId, | |
| }); | |
| throw error; | |
| } | |
| return sessionData; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth-session-store/redis-session-store.ts` around lines 98 - 131, The
create() method currently returns a non-persisted SessionData when Redis is not
ready or when redis.setex fails, causing silent data loss; change it so that
when getReadyRedis() returns falsy or when the setex call throws, create()
throws an Error (including context like sessionId and underlying error) instead
of logging and returning sessionData so the caller (e.g., login opaque branch)
can handle/return a 500; update references to getReadyRedis() and the try/catch
around redis.setex(buildSessionKey(...), ttl, JSON.stringify(sessionData)) to
rethrow a wrapped error.
| describe("login route no-store headers", () => { | ||
| let POST: (request: NextRequest) => Promise<Response>; | ||
|
|
||
| beforeEach(async () => { | ||
| vi.clearAllMocks(); | ||
| const mockT = vi.fn((key: string) => `translated:${key}`); | ||
| mockGetTranslations.mockResolvedValue(mockT); | ||
| mockSetAuthCookie.mockResolvedValue(undefined); | ||
| mockGetSessionTokenMode.mockReturnValue("legacy"); | ||
| mockGetEnvConfig.mockReturnValue({ ENABLE_SECURE_COOKIES: false }); | ||
|
|
||
| const mod = await import("../../src/app/api/auth/login/route"); | ||
| POST = mod.POST; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file and examine the relevant lines
find . -type f -name "session-cookie-hardening.test.ts" 2>/dev/null | head -5Repository: ding113/claude-code-hub
Length of output: 116
🏁 Script executed:
# Also search for the CSRF guard module
find . -type f -name "*csrf*" -o -name "*csrf-origin-guard*" 2>/dev/null | head -20Repository: ding113/claude-code-hub
Length of output: 202
🏁 Script executed:
# Find the login route file
find . -type f -path "*/app/api/auth/login/route.*" 2>/dev/nullRepository: ding113/claude-code-hub
Length of output: 100
🏁 Script executed:
# Examine the test file around lines 100-113 and 175-185
wc -l ./tests/security/session-cookie-hardening.test.tsRepository: ding113/claude-code-hub
Length of output: 120
🏁 Script executed:
# Read the test file to see the test structure and mocking
head -180 ./tests/security/session-cookie-hardening.test.ts | tail -85Repository: ding113/claude-code-hub
Length of output: 3013
🏁 Script executed:
# Look at the beginning of the test file to see what's being mocked
head -100 ./tests/security/session-cookie-hardening.test.tsRepository: ding113/claude-code-hub
Length of output: 3278
🏁 Script executed:
# Check if CSRF guard is mocked in the test file
rg -n "csrf" ./tests/security/session-cookie-hardening.test.tsRepository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
# Read the login route implementation
cat -n ./src/app/api/auth/login/route.tsRepository: ding113/claude-code-hub
Length of output: 10375
🏁 Script executed:
# Examine the CSRF guard implementation
cat -n ./src/lib/security/csrf-origin-guard.tsRepository: ding113/claude-code-hub
Length of output: 2345
🏁 Script executed:
# Verify the logout route also calls CSRF guard
grep -n "csrfGuard" ./src/app/api/auth/logout/route.ts 2>/dev/null | head -5Repository: ding113/claude-code-hub
Length of output: 161
🏁 Script executed:
# Check what the test file contains around lines 175-185 (logout route tests)
sed -n '175,185p' ./tests/security/session-cookie-hardening.test.tsRepository: ding113/claude-code-hub
Length of output: 466
login 和 logout 路由测试缺少显式的 CSRF guard mock
路由处理程序（login/route.ts 第 147 行，logout/route.ts 第 51 行）调用 csrfGuard.check(request)，但测试文件未 mock @/lib/security/csrf-origin-guard 模块，实际的 guard 会被加载。测试中的 makeLoginRequest 和 makeLogoutRequest 均未设置 Origin 或 sec-fetch-site 标头。
根据 guard 实现，当缺少这些标头时，guard 会默认放行请求（只有当 sec-fetch-site 明确为 "cross-site" 且缺少 Origin 时才拒绝）。因此测试能通过，但隐含了对 guard 默认行为的依赖。
建议：显式 mock CSRF guard，或在请求中添加必要的标头（如 sec-fetch-site: same-origin 或 Origin 标头），以使测试意图更明确且不依赖 guard 的默认策略。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/security/session-cookie-hardening.test.ts` around lines 100 - 113,
Tests for login/logout rely on implicit CSRF guard behavior; explicitly mock the
CSRF guard or set request headers to avoid this hidden dependency. Update tests
in tests/security/session-cookie-hardening.test.ts to either mock the module
"@/lib/security/csrf-origin-guard" and stub csrfGuard.check to a resolved value
before importing login/route (so POST uses the mock), or modify the helper
functions makeLoginRequest and makeLogoutRequest to include explicit headers
(e.g., "sec-fetch-site": "same-origin" or an "Origin" header) so
csrfGuard.check(request) in login/route.ts and logout/route.ts is exercised
deterministically. Ensure the mock/stub is applied (vi.mock or similar) prior to
importing the route modules and that csrfGuard.check is referenced by the exact
symbol name csrfGuard.check.
| include: [ | ||
| "tests/unit/**/*.{test,spec}.tsx", | ||
| "tests/security/**/*.{test,spec}.tsx", | ||
| "tests/api/**/*.{test,spec}.tsx", | ||
| "src/**/*.{test,spec}.tsx", | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if tests/security directory exists and what files are in it
echo "=== Checking tests/security directory ==="
if [ -d "tests/security" ]; then
echo "Directory exists. Contents:"
find tests/security -type f | head -20
else
echo "tests/security directory does not exist"
fi
echo ""
echo "=== All test files by extension in tests/ ==="
fd --extension ts . tests/ | wc -l
echo "Total .ts test files: $(fd --extension ts . tests/ | wc -l)"
fd --extension tsx . tests/ | wc -l
echo "Total .tsx test files: $(fd --extension tsx . tests/ | wc -l)"Repository: ding113/claude-code-hub
Length of output: 828
🏁 Script executed:
# Check if there are any tsx or ts files specifically in tests/security
echo "=== Files in tests/security with .tsx extension ==="
fd --extension tsx . tests/security/ 2>/dev/null || echo "No .tsx files found or directory not found"
echo ""
echo "=== Files in tests/security with .ts extension ==="
fd --extension ts . tests/security/ 2>/dev/null || echo "No .ts files found or directory not found"Repository: ding113/claude-code-hub
Length of output: 756
🏁 Script executed:
# Check the actual vitest.config.ts file to verify the configuration
echo "=== vitest.config.ts lines 13-18 ==="
sed -n '13,18p' vitest.config.ts
echo ""
echo "=== vitest.config.ts lines 91-96 ==="
sed -n '91,96p' vitest.config.tsRepository: ding113/claude-code-hub
Length of output: 567
🏁 Script executed:
# Check the full projects configuration to understand the extends behavior
echo "=== Full vitest.config.ts projects section (lines 8-21) ==="
sed -n '8,21p' vitest.config.ts
echo ""
echo "=== Check if there are any Vitest docs or comments about project inheritance ==="
grep -n "extends" vitest.config.ts | head -10Repository: ding113/claude-code-hub
Length of output: 578
🌐 Web query:
Vitest projects extends true include pattern override behavior how does it work
💡 Result:
In Vitest projects, extends: true means “inherit the root config and merge it with this project config”. Vitest’s own docs call this out explicitly: with extends: true, “all options will be merged”, and with extends: false (the default) the project won’t inherit root options. [1]
What that means for test.include
Because the configs are merged (not replaced), test.include in the project config does not override the root test.include—it adds to it (so the project will match tests from both sets of globs). This is the behavior people usually run into when they expect per-project include to “replace” the root pattern, but it doesn’t when you’re extending. [1]
How to actually override which tests a project runs
Use one of these patterns:
- Don’t extend the root config for that project:
projects: [
{
extends: false,
test: { name: 'unit', include: ['**/*.unit.test.ts'] },
},
]This makes include fully project-owned. [1]
-
Move shared config into a separate “shared” config and merge it manually into each project, instead of relying on
extends, so your root file doesn’t “leak”includeinto every project. Vitest recommends this approach (viamergeConfig) because root options aren’t inherited by default. [1] -
If you do want some inheritance but different test sets, keep the root config free of
test.include, and defineincludeonly inside each project. [1]
Key mental model: with extends: true, project include is “in addition to root include”, not “instead of”.
Sources:
[1] Vitest “Test Projects” guide (extends behavior + examples): https://vitest.dev/guide/projects.html
行 15 的安全测试文件扩展名模式不正确
tests/security/ 目录中的所有 13 个安全测试文件都是 .ts 扩展名（如 csrf-origin-guard.test.ts、session-store.test.ts 等），但行 15 的项目配置中添加了 tests/security/**/*.{test,spec}.tsx 模式，该模式无法匹配任何实际文件。
虽然由于 extends: true 会合并根配置和项目配置中的 include 模式，安全测试最终仍通过行 93 的根配置 tests/security/**/*.{test,spec}.ts 被执行，但行 15 的 .tsx 模式是冗余且错误的。
建议改为 "tests/security/**/*.{test,spec}.ts"，或如果安全测试无需在 happy-dom 环境中运行，则从项目 include 中删除安全测试模式，让其仅在根配置中执行。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vitest.config.ts` around lines 13 - 18, The include array currently contains
the incorrect pattern "tests/security/**/*.{test,spec}.tsx" which doesn't match
your actual .ts security tests; update the include entry inside the include
array to "tests/security/**/*.{test,spec}.ts" or remove the security pattern
from the project's include so those tests are only matched by the root config,
ensuring the include symbol and the exact erroneous string are fixed.
| include: [ | ||
| "tests/unit/**/*.{test,spec}.ts", // 单元测试 | ||
| "tests/security/**/*.{test,spec}.ts", | ||
| "tests/api/**/*.{test,spec}.ts", // API 测试 | ||
| "src/**/*.{test,spec}.ts", // 支持源码中的测试 | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat vitest.config.tsRepository: ding113/claude-code-hub
Length of output: 4261
🏁 Script executed:
# Check for .ts test files that would be affected
echo "=== .ts test files in tests/security ==="
fd --extension ts --type f "tests/security/" 2>/dev/null | head -20
echo "=== .ts test files in tests/unit ==="
fd --extension ts --type f "tests/unit/" 2>/dev/null | head -20
echo "=== .ts test files in tests/api ==="
fd --extension ts --type f "tests/api/" 2>/dev/null | head -20
echo "=== .tsx test files (comparison) ==="
fd --extension tsx --type f "tests/" 2>/dev/null | head -20Repository: ding113/claude-code-hub
Length of output: 217
🏁 Script executed:
# Search for any test files (both .ts and .tsx extensions)
echo "=== All test/spec files in repository ==="
fd --type f "\.test\.(ts|tsx|js|jsx)$|\.spec\.(ts|tsx|js|jsx)$" | head -30
echo ""
echo "=== Count of test files by extension ==="
fd --type f "\.test\.(ts|tsx|js|jsx)$|\.spec\.(ts|tsx|js|jsx)$" | sed 's/.*\.//' | sort | uniq -cRepository: ding113/claude-code-hub
Length of output: 1936
🏁 Script executed:
# Check what's in tests/ directory
echo "=== Directory structure of tests/ ==="
tree -L 2 tests/ 2>/dev/null || find tests/ -maxdepth 2 -type d | sortRepository: ding113/claude-code-hub
Length of output: 2827
🏁 Script executed:
# Verify the actual test file types in critical directories
echo "=== tests/security/ file extensions ==="
ls -la tests/security/ | tail -20
echo ""
echo "=== Count of .ts vs .tsx in tests/security ==="
ls tests/security/*.test.* 2>/dev/null | sed 's/.*\.//' | sort | uniq -c
echo ""
echo "=== Confirm Vitest extends behavior: check if root include is used ==="
echo "Root include targets .ts but project include targets .tsx only"
echo "With extends: true, project include replaces root include (does not merge)"Repository: ding113/claude-code-hub
Length of output: 1515
🏁 Script executed:
# Check if any .tsx files exist in tests/security or tests/unit (would be matched by project)
echo "=== .tsx files in tests/security ==="
find tests/security -name "*.test.tsx" 2>/dev/null | wc -l
echo "=== .tsx files in tests/unit ==="
find tests/unit -name "*.test.tsx" 2>/dev/null | wc -l
echo "=== .ts files in tests/security (would be skipped) ==="
find tests/security -name "*.test.ts" 2>/dev/null | wc -l
echo "=== .ts files in tests/unit (would be skipped) ==="
find tests/unit -name "*.test.ts" 2>/dev/null | wc -lRepository: ding113/claude-code-hub
Length of output: 254
项目级 include 覆盖根级配置，13 个安全测试文件将被跳过
当前 Vitest 配置中，项目（project）通过 extends: true 并指定了 include: ["tests/**/*.{test,spec}.tsx", ...]，此配置会完全替代根级 include 的 .ts 匹配规则。结果是 tests/security/ 目录中的 13 个关键安全测试（CSRF 防护、暴力破解防御、会话硬化等）全部为 .ts 格式，将无法被发现和执行。同时 tests/unit/ 中的 204 个 .ts 测试也会被跳过。
建议添加一个 node 环境的项目配置来运行 .ts 测试文件：
建议修复：添加 node 环境项目以运行 .ts 测试
projects: [
{
extends: true,
test: {
environment: "happy-dom",
include: [
"tests/unit/**/*.{test,spec}.tsx",
"tests/security/**/*.{test,spec}.tsx",
"tests/api/**/*.{test,spec}.tsx",
"src/**/*.{test,spec}.tsx",
],
},
},
+ {
+ extends: true,
+ test: {
+ environment: "node",
+ include: [
+ "tests/unit/**/*.{test,spec}.ts",
+ "tests/security/**/*.{test,spec}.ts",
+ "tests/api/**/*.{test,spec}.ts",
+ "src/**/*.{test,spec}.ts",
+ ],
+ exclude: [
+ "tests/integration/**",
+ "tests/api/users-actions.test.ts",
+ "tests/api/providers-actions.test.ts",
+ "tests/api/keys-actions.test.ts",
+ "tests/api/my-usage-readonly.test.ts",
+ ],
+ },
+ },
],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vitest.config.ts` around lines 91 - 96, The project-level Vitest config is
replacing the root include patterns (via extends: true) and only matching .tsx
files, causing .ts tests under tests/security and tests/unit to be skipped;
update vitest.config.ts by adding a separate project entry with env: "node" (or
adjust the existing project) that includes the .ts patterns (e.g.,
"tests/unit/**/*.{test,spec}.ts", "tests/security/**/*.{test,spec}.ts",
"tests/api/**/*.{test,spec}.ts", "src/**/*.{test,spec}.ts") so .ts tests are
discovered, or change the project's include to merge .ts patterns rather than
overriding the root; target the include array and the project config that
currently uses extends: true to implement this fix.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| hasUpdate: Boolean(data.hasUpdate), | ||
| }); | ||
| }) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
[CRITICAL] [ERROR-SILENT] Swallowed fetch errors on login page
Location: src/app/[locale]/login/page.tsx:98 (also src/app/[locale]/login/page.tsx:126)
Why this is a problem: These requests explicitly swallow failures via .catch(() => {}). This hides network/JSON issues and makes production debugging much harder.
Suggested fix:
void fetch("/api/version")
.then((response) => response.json() as Promise<{ current?: unknown; hasUpdate?: unknown }>)
.then((data) => {
/* existing logic */
})
.catch((error) => {
console.error("[LoginPage] /api/version failed", error);
});
void fetch("/api/system-settings")
.then((response) => (response.ok ? (response.json() as Promise<{ siteTitle?: unknown }>) : null))
.then((data) => {
/* existing logic */
})
.catch((error) => {
console.error("[LoginPage] /api/system-settings failed", error);
});
.env.example
Outdated
| # Session 配置 | ||
| # Session Token 迁移模式（auth-token Cookie 载荷） | ||
| # - legacy (默认)：仅接受 legacy（Cookie 值为原始 API Key），用于紧急回滚 | ||
| # - dual：迁移窗口，同时接受 legacy 与 opaque（Cookie 值为 sid_ 前缀 sessionId） |
There was a problem hiding this comment.
[HIGH] [COMMENT-INACCURATE] SESSION_TOKEN_MODE cookie contract (sid_ prefix) is inconsistent
Location: .env.example:74
Why this is a problem: .env.example documents dual/opaque cookies as sid_-prefixed sessionIds, and src/lib/auth.ts classifies opaque tokens via const OPAQUE_SESSION_ID_PREFIX = "sid_", but src/lib/auth-session-store/redis-session-store.ts:105 generates sessionId: crypto.randomUUID() (no prefix) and src/app/api/auth/login/route.ts:239 writes that raw value into the cookie in opaque mode. This makes the migration contract ambiguous and can mislead operators/tests.
Suggested fix (pick one path and keep docs + code + tests aligned):
// src/lib/auth-session-store/redis-session-store.ts
sessionId: `sid_${crypto.randomUUID()}`,OR update .env.example and any token-kind detection/tests to match the unprefixed UUID contract.
src/app/api/auth/login/route.ts
Outdated
| } | ||
| } else { | ||
| const opaqueSession = await createOpaqueSession(key, session); | ||
| await setAuthCookie(opaqueSession.sessionId); |
There was a problem hiding this comment.
[CRITICAL] [LOGIC-BUG] Opaque session cookie breaks downstream auth (middleware + getSession)
Location: src/app/api/auth/login/route.ts:239
Why this is a problem: In opaque mode this sets the cookie to an opaque sessionId (await setAuthCookie(opaqueSession.sessionId);). But downstream auth still treats the cookie as a raw API key:
src/proxy.ts:83callsvalidateKey(authToken.value, ...)src/lib/auth.ts:285(insidegetSession()) ultimately callsvalidateKey(keyString, options)
With SESSION_TOKEN_MODE=opaque, this will immediately invalidate/clear the cookie and redirect users back to /login (login loop), making protected pages unreachable.
Suggested fix: Centralize token validation behind a single entrypoint that understands SESSION_TOKEN_MODE, then use it everywhere that authenticates cookie tokens (middleware + server routes/components).
Example direction:
// src/lib/auth.ts
export async function validateAuthToken(
token: string,
options?: { allowReadOnlyAccess?: boolean }
): Promise<AuthSession | null> {
const mode = getSessionTokenMode();
if (mode !== "legacy") {
const sessionStore = await getSessionStore();
const sessionData = await sessionStore.read(token);
if (sessionData) return convertToAuthSession(sessionData, options);
if (mode === "opaque") return null;
}
return validateKey(token, options);
}Then update src/proxy.ts to call validateAuthToken(...) (and consider making getSession() delegate to getSessionWithDualRead() to avoid missing opaque sessions in server components/routes).
There was a problem hiding this comment.
Code Review Summary
This is an XL PR with substantial auth/session behavior changes plus a large new security test suite. The main issue is an integration gap: SESSION_TOKEN_MODE=opaque writes an opaque cookie, but the rest of the app still validates the cookie as a raw API key, which will cause immediate login redirect loops when enabled.
PR Size: XL
- Lines changed: 5507
- Files changed: 48
- Split suggestion: 1) session token mode + session store + validation wiring, 2) CSRF + security headers middleware, 3) login UI/redirect safety + usage-doc auth context, 4) security regression test suite + Vitest config.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 1 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 1 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/app/api/auth/login/route.ts:239[CRITICAL] [LOGIC-BUG] Opaque cookie breaks downstream auth validation (src/proxy.ts/getSession()still usevalidateKey) → login loop whenSESSION_TOKEN_MODE=opaque. (Confidence: 100/100)src/app/[locale]/login/page.tsx:98[CRITICAL] [ERROR-SILENT]fetchfailures are swallowed via.catch(() => {})(also atsrc/app/[locale]/login/page.tsx:126). (Confidence: 95/100)
High Priority Issues (Should Fix)
.env.example:74[HIGH] [COMMENT-INACCURATE] Docs say opaque cookies aresid_-prefixed sessionIds, but the current implementation generates a bare UUID sessionId and writes it to the cookie in opaque mode. (Confidence: 90/100)
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Applied PR size label
size/XLon PR#803. - Posted 3 inline review comments (Critical:
src/app/api/auth/login/route.ts:239,src/app/[locale]/login/page.tsx:98; High:.env.example:74). - Submitted the required review summary via
gh pr review 803 --comment.
Changes session authentication from legacy API key cookies to opaque session identifiers with Redis backing. The opaque mode prevents API key exposure in cookies and enables proper session lifecycle management. Key changes: - Default SESSION_TOKEN_MODE changed from "legacy" to "opaque" - Session IDs now prefixed with "sid_" for clear identification - Unified authentication through validateAuthToken() wrapper - Redis session creation now throws on failure instead of silent fallback - Session rotation preserves new session even if old revocation fails - Extracted auth response headers to dedicated module - Fixed redirect sanitization bypass in login flow - Updated all tests to reflect opaque-first behavior BREAKING CHANGE: SESSION_TOKEN_MODE now defaults to "opaque". Existing deployments using legacy mode must explicitly set SESSION_TOKEN_MODE=legacy in environment variables to maintain current behavior. The "dual" mode remains available for gradual migration.
Vitest's `mockReset: true` config resets all mock implementations between tests, including inline `vi.fn()` created inside `vi.mock()` factories. For indirect imports (modules imported by the dynamically imported route), the factory is NOT re-executed after `vi.resetModules()`, so the reset mock returns undefined. Fix: use `vi.hoisted()` variables re-initialized in `beforeEach`, or plain arrow functions that are immune to mockReset. - auth-bruteforce: getEnvConfig uses plain function (no vi.fn()) - session-login: toKeyFingerprint + getEnvConfig use hoisted variables - redis-session-store: formatting only (biome auto-fix)
| async function convertToAuthSession( | ||
| sessionData: OpaqueSessionContract, | ||
| options?: { allowReadOnlyAccess?: boolean } | ||
| ): Promise<AuthSession | null> { | ||
| const keyList = await findKeyList(sessionData.userId); | ||
| const expectedFingerprint = normalizeKeyFingerprint(sessionData.keyFingerprint); | ||
|
|
||
| for (const key of keyList) { | ||
| const keyFingerprint = await toKeyFingerprint(key.key); | ||
| if (keyFingerprint === expectedFingerprint) { | ||
| return validateKey(key.key, options); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Linear key scan with per-key SHA-256 hashing
convertToAuthSession fetches all keys for a user via findKeyList and computes a SHA-256 hash for each one sequentially until the matching fingerprint is found. On every authenticated request in opaque/dual mode, this results in O(n) async crypto operations where n is the number of keys a user has.
Consider storing the key fingerprint → key ID mapping at session creation time (e.g., including the keyId in the OpaqueSessionContract), so that the read path can do a direct lookup instead of iterating and hashing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/auth.ts
Line: 343-358
Comment:
**Linear key scan with per-key SHA-256 hashing**
`convertToAuthSession` fetches all keys for a user via `findKeyList` and computes a SHA-256 hash for each one sequentially until the matching fingerprint is found. On every authenticated request in opaque/dual mode, this results in O(n) async crypto operations where n is the number of keys a user has.
Consider storing the key fingerprint → key ID mapping at session creation time (e.g., including the `keyId` in the `OpaqueSessionContract`), so that the read path can do a direct lookup instead of iterating and hashing.
How can I resolve this? If you propose a fix, please make it concise.| }; | ||
|
|
||
| export class LoginAbusePolicy { | ||
| private attempts = new Map<string, AttemptRecord>(); |
There was a problem hiding this comment.
In-memory rate limiting won't work across replicas
LoginAbusePolicy stores attempt records in a process-local Map. In a multi-process or multi-replica deployment (e.g., Kubernetes), each instance tracks attempts independently, so an attacker could spread requests across replicas and effectively multiply the allowed attempts by the replica count. Consider using a Redis-backed counter (Redis is already a dependency for the session store) to make rate limiting consistent across all instances.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/login-abuse-policy.ts
Line: 28
Comment:
**In-memory rate limiting won't work across replicas**
`LoginAbusePolicy` stores attempt records in a process-local `Map`. In a multi-process or multi-replica deployment (e.g., Kubernetes), each instance tracks attempts independently, so an attacker could spread requests across replicas and effectively multiply the allowed attempts by the replica count. Consider using a Redis-backed counter (Redis is already a dependency for the session store) to make rate limiting consistent across all instances.
How can I resolve this? If you propose a fix, please make it concise.| if (!origin) { | ||
| if (fetchSite === "cross-site") { | ||
| return { | ||
| allowed: false, | ||
| reason: "Cross-site request blocked: missing Origin header", | ||
| }; | ||
| } | ||
|
|
||
| return { allowed: true }; | ||
| } |
There was a problem hiding this comment.
CSRF guard allows requests with neither Origin nor Sec-Fetch-Site headers
When both Origin is absent and sec-fetch-site is neither "same-origin" nor "cross-site" (e.g., both headers are missing entirely), the guard returns { allowed: true }. This is presumably intentional to support non-browser clients (curl, Postman), but it means any HTTP client that simply omits these headers can bypass the CSRF check entirely.
If the intent is purely to block browser-based CSRF attacks, this is acceptable since modern browsers always send Sec-Fetch-Site. But it's worth documenting this assumption explicitly, since older or unusual browsers may not include these headers.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/csrf-origin-guard.ts
Line: 48-57
Comment:
**CSRF guard allows requests with neither Origin nor Sec-Fetch-Site headers**
When both `Origin` is absent and `sec-fetch-site` is neither `"same-origin"` nor `"cross-site"` (e.g., both headers are missing entirely), the guard returns `{ allowed: true }`. This is presumably intentional to support non-browser clients (curl, Postman), but it means any HTTP client that simply omits these headers can bypass the CSRF check entirely.
If the intent is purely to block browser-based CSRF attacks, this is acceptable since modern browsers always send `Sec-Fetch-Site`. But it's worth documenting this assumption explicitly, since older or unusual browsers may not include these headers.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| async function createOpaqueSession(key: string, session: AuthSession) { | ||
| const store = new RedisSessionStore(); | ||
| return store.create({ | ||
| keyFingerprint: await toKeyFingerprint(key), | ||
| userId: session.user.id, | ||
| userRole: session.user.role, | ||
| }); | ||
| } |
There was a problem hiding this comment.
New RedisSessionStore instance created per login request
createOpaqueSession instantiates a new RedisSessionStore() on every call. While the underlying Redis client is likely shared via getRedisClient(), this still creates a new object per request. Contrast this with getSessionStore() in src/lib/auth.ts:321-329 which caches the store instance in a module-level promise. Consider reusing a module-level store instance here as well for consistency.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 114-121
Comment:
**New `RedisSessionStore` instance created per login request**
`createOpaqueSession` instantiates a new `RedisSessionStore()` on every call. While the underlying Redis client is likely shared via `getRedisClient()`, this still creates a new object per request. Contrast this with `getSessionStore()` in `src/lib/auth.ts:321-329` which caches the store instance in a module-level promise. Consider reusing a module-level store instance here as well for consistency.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| } else { | ||
| const opaqueSession = await createOpaqueSession(key, session); | ||
| await setAuthCookie(opaqueSession.sessionId); | ||
| } |
There was a problem hiding this comment.
Unhandled exception from createOpaqueSession in opaque mode breaks login
In the else branch (opaque mode), if createOpaqueSession throws (e.g., Redis is down), the exception propagates to the outer catch which returns a generic 500 error. This means users cannot log in at all when Redis is unavailable in opaque mode, even though the key was already validated successfully.
In dual mode (lines 207-213), this same failure is gracefully handled with a try/catch that logs a warning and falls through. Consider applying similar resilience in opaque mode — e.g., falling back to setting the raw key as the cookie (like legacy mode) with a warning log, so that login isn't completely broken by a transient Redis outage.
| } else { | |
| const opaqueSession = await createOpaqueSession(key, session); | |
| await setAuthCookie(opaqueSession.sessionId); | |
| } | |
| try { | |
| const opaqueSession = await createOpaqueSession(key, session); | |
| await setAuthCookie(opaqueSession.sessionId); | |
| } catch (error) { | |
| logger.error("Failed to create opaque session, falling back to legacy cookie", { | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| await setAuthCookie(key); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 214-217
Comment:
**Unhandled exception from `createOpaqueSession` in opaque mode breaks login**
In the `else` branch (opaque mode), if `createOpaqueSession` throws (e.g., Redis is down), the exception propagates to the outer `catch` which returns a generic 500 error. This means users cannot log in at all when Redis is unavailable in opaque mode, even though the key was already validated successfully.
In `dual` mode (lines 207-213), this same failure is gracefully handled with a `try/catch` that logs a warning and falls through. Consider applying similar resilience in opaque mode — e.g., falling back to setting the raw key as the cookie (like legacy mode) with a warning log, so that login isn't completely broken by a transient Redis outage.
```suggestion
try {
const opaqueSession = await createOpaqueSession(key, session);
await setAuthCookie(opaqueSession.sessionId);
} catch (error) {
logger.error("Failed to create opaque session, falling back to legacy cookie", {
error: error instanceof Error ? error.message : String(error),
});
await setAuthCookie(key);
}
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
vitest.config.ts (1)
8-19: 建议将后端安全测试从happy-dom项目中分离到独立的node环境项目由于
extends: true会将根配置与项目配置的所有选项合并，当前tests/security/下所有安全测试（CSRF 防护、会话存储、Redis 操作等）都会在happy-dom环境中执行。这些都是纯服务端逻辑，不依赖 DOM，在happy-dom中运行可能导致fetch、加密 API 和 URL 处理等行为出现与真实 Node.js 环境的细微差异，使测试结果失去代表性。Vitest 官方文档的推荐做法是同时定义多个项目——分别指定
happy-dom和node的include模式——来隔离不同运行时环境的测试。♻️ 建议的修复方案：将安全测试分离到独立的 `node` 环境项目
projects: [ { extends: true, test: { environment: "happy-dom", include: [ "tests/unit/**/*.{test,spec}.tsx", - "tests/security/**/*.{test,spec}.{ts,tsx}", "tests/api/**/*.{test,spec}.tsx", "src/**/*.{test,spec}.tsx", ], }, }, + { + extends: true, + test: { + name: "security", + environment: "node", + include: [ + "tests/security/**/*.{test,spec}.ts", + ], + }, + }, ],根配置中的
include（第 93 行）仍负责将安全测试合并入该新项目，上方happy-dom项目include中移除对应条目即可。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.ts` around lines 8 - 19, 当前 projects 配置将安全测试包含在使用 extends: true 的 happy-dom 项目中，导致 tests/security/**/* 被在浏览器仿真环境运行；请为后端安全测试创建一个独立项目条目（projects 数组内的另一个对象）并将其 test.environment 设置为 "node"，将 include 指向 "tests/security/**/*.{test,spec}.{ts,tsx}"，同时从原有使用 environment "happy-dom" 的项目的 include 中移除该 security 路径；保留 extends: true 行为以继承根配置但确保 test.environment 和 include 在两个项目中各自正确指定。src/proxy.ts (1)
13-15: 注释中仍引用validateKey，但代码已迁移到validateAuthToken。第 14 行注释 "These paths bypass the canLoginWebUi check in validateKey" 应更新为
validateAuthToken，以反映当前实现。建议修改
// Paths that allow read-only access (for canLoginWebUi=false keys) -// These paths bypass the canLoginWebUi check in validateKey +// These paths bypass the canLoginWebUi check in validateAuthToken const READ_ONLY_PATH_PATTERNS = ["/my-usage"];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 13 - 15, 更新注释以反映当前实现：将第 14 行注释中提到的 validateKey 改为 validateAuthToken，以匹配现在的验证函数名称（参照常量 READ_ONLY_PATH_PATTERNS 和函数 validateAuthToken），确保注释说明这些只读路径绕过 validateAuthToken 中的 canLoginWebUi 检查。tests/security/session-contract.test.ts (1)
23-23: 应使用@/路径别名替代相对路径。文件中多处动态导入使用了
../../src/lib/auth，应统一使用@/lib/auth，与编码规范保持一致。此问题同样出现在第 35、43、80、99 行。建议修改
- const { getSessionTokenMode } = await import("../../src/lib/auth"); + const { getSessionTokenMode } = await import("@/lib/auth");As per coding guidelines: "Use path alias
@/to reference files in./src/directory"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/session-contract.test.ts` at line 23, Update the dynamic imports that reference the auth module to use the project path alias instead of relative paths: replace imports like await import("../../src/lib/auth") with await import("@/lib/auth") (affecting uses that import getSessionTokenMode and other symbols from the same module). Locate occurrences in the test file where getSessionTokenMode (and other imports from lib/auth) are dynamically imported and change them to "@/lib/auth" to conform to the coding guideline.src/lib/auth.ts (2)
360-370:getSessionWithDualRead和validateSession目前是简单透传包装。这两个函数当前仅转发到
getSession，没有附加行为。如果它们是面向未来扩展的占位接口，建议添加简短注释说明其用途和预期演进方向，便于后续维护者理解设计意图。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth.ts` around lines 360 - 370, The functions getSessionWithDualRead and validateSession are simple pass-throughs to getSession and need explicit intent comments; update the implementations by adding concise doc/comments above getSessionWithDualRead and validateSession stating they are deliberate placeholders for future dual-read or validation logic (e.g., read-only vs read-write session handling and additional validation steps), describe the expected future changes (what dual-read means and when validateSession will diverge), and note why callers should use these wrappers instead of calling getSession directly to make the design intent clear; keep the runtime behavior unchanged.
343-358:convertToAuthSession对每个 key 逐一计算 SHA-256 指纹，缺少缓存。该函数遍历用户所有 key 并逐一调用
toKeyFingerprint进行异步 SHA-256 运算。虽然单用户 key 数量通常较少，但在高频调用场景下（每次请求验证 opaque session 都会触发），可考虑在 key 记录中持久化 fingerprint 或添加内存缓存，避免重复计算。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth.ts` around lines 343 - 358, convertToAuthSession currently recomputes SHA-256 for every key by calling toKeyFingerprint inside the loop; add caching or persist fingerprint to avoid repeated async hashing: either extend the key records returned by findKeyList to include a precomputed fingerprint (compute and store fingerprint when keys are created/updated) and compare against normalized fingerprint directly, or implement an in-memory Map cache keyed by key.id or the raw key string and consult it before calling toKeyFingerprint; update convertToAuthSession to use the cached/persisted fingerprint lookup and still fall back to toKeyFingerprint only on cache miss, then call validateKey(key.key, options) when fingerprints match.src/lib/security/auth-response-headers.ts (1)
6-18:enableHsts与ENABLE_SECURE_COOKIES耦合值得关注。
enableHsts由env.ENABLE_SECURE_COOKIES驱动，语义上 HSTS（传输层安全）和安全 Cookie 虽有关联但并非同一概念。当前实现在大多数部署场景下是合理的（使用安全 Cookie 意味着 HTTPS 环境），但如果将来需要独立控制 HSTS，建议引入单独的环境变量。目前可以接受。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/security/auth-response-headers.ts` around lines 6 - 18, 当前将 enableHsts 绑定到 env.ENABLE_SECURE_COOKIES 有语义耦合风险：HSTS 与安全 cookie 是不同的设置。修改 applySecurityHeaders 中对 buildSecurityHeaders 的调用，改为读取独立的环境变量（例如 env.ENABLE_HSTS）来驱动 enableHsts，并在该变量未设置时回退到 env.ENABLE_SECURE_COOKIES 以保证向后兼容；相关符号：applySecurityHeaders, getEnvConfig, buildSecurityHeaders, env.ENABLE_SECURE_COOKIES, enableHsts。src/app/api/auth/login/route.ts (2)
22-28:loginPolicy是进程内单例，在无服务器 / 多实例环境下限流效果有限。
LoginAbusePolicy的状态存储在内存中，在 serverless 函数冷启动或多实例部署时无法跨实例共享。当前实现在单实例场景下有效，但建议在文档或注释中说明此限制，并考虑未来迁移到 Redis 支撑的方案以获得更可靠的防护。🤖 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 22 - 28, loginPolicy 是在进程内创建的单例（LoginAbusePolicy 实例），其状态保存在内存中，无法在无服务器冷启动或多实例部署下跨实例共享；请在 src/app/api/auth/login/route.ts 在 loginPolicy 定义附近添加明确注释/文档说明该限流/风控为单实例实现及其局限性，并在注释中列出后续迁移建议（例如将 LoginAbusePolicy 的状态后端改为 Redis 或其它集中式存储以支持跨实例限流），以便维护者知晓并在未来替换为基于 Redis 的实现（参考 LoginAbusePolicy 和 loginPolicy 标识符以定位代码）。
114-121: 每次调用createOpaqueSession都新建RedisSessionStore实例，与auth.ts中的单例模式不一致。
src/lib/auth.ts第 319-329 行使用sessionStorePromise实现了懒加载单例，而此处每次登录都new RedisSessionStore()。虽然构造开销不大，但建议复用auth.ts中的 session store 获取方式或提取共享工厂函数，保持一致性。🤖 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 114 - 121, createOpaqueSession currently constructs a new RedisSessionStore on every call (new RedisSessionStore()), which diverges from the lazy-singleton pattern used in auth.ts (sessionStorePromise). Update createOpaqueSession to obtain and reuse the shared session store instead of instantiating a new one—e.g., import or call the same session store factory/singleton used in auth.ts (sessionStorePromise or the exported getter) and then call store.create(...) with that instance; ensure to await the promise if the singleton is async and keep the existing payload (toKeyFingerprint(key), session.user.id, session.user.role).tests/security/auth-csrf-route-integration.test.ts (2)
112-116: 应使用@/路径别名替代相对路径。建议修改
- const loginRoute = await import("../../src/app/api/auth/login/route"); + const loginRoute = await import("@/app/api/auth/login/route"); loginPost = loginRoute.POST; - const logoutRoute = await import("../../src/app/api/auth/logout/route"); + const logoutRoute = await import("@/app/api/auth/logout/route"); logoutPost = logoutRoute.POST;As per coding guidelines: "Use path alias
@/to reference files in./src/directory"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/auth-csrf-route-integration.test.ts` around lines 112 - 116, Replace the relative imports that pull in the route modules with the project path alias so the test uses "@/..." instead of "../../src/..."; specifically update the import statements that assign loginRoute and logoutRoute (and the derived loginPost and logoutPost) to import from "@/app/api/auth/login/route" and "@/app/api/auth/logout/route" respectively, ensuring the rest of the test still references loginPost and logoutPost unchanged.
19-31: Mock 中缺少toKeyFingerprint导出。login route 从
@/lib/auth导入了toKeyFingerprint，但此处 mock 未包含该导出。当前测试因getSessionTokenMode返回"legacy"而不会触发相关代码路径，因此不会报错。但如果未来测试扩展到dual或opaque模式，将会失败。建议补全：建议修改
vi.mock("@/lib/auth", () => ({ validateKey: mockValidateKey, setAuthCookie: mockSetAuthCookie, getSessionTokenMode: mockGetSessionTokenMode, getLoginRedirectTarget: mockGetLoginRedirectTarget, clearAuthCookie: mockClearAuthCookie, getAuthCookie: mockGetAuthCookie, + toKeyFingerprint: vi.fn().mockResolvedValue("sha256:fake"), withNoStoreHeaders: <T>(res: T): T => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/auth-csrf-route-integration.test.ts` around lines 19 - 31, The mock for "@/lib/auth" is missing the toKeyFingerprint export used by the login route; update the vi.mock block to include a toKeyFingerprint export (e.g., a simple stub function or deterministic transformer) so tests that run with getSessionTokenMode returning "dual" or "opaque" won't fail; specifically add a toKeyFingerprint function alongside mockValidateKey, mockSetAuthCookie, mockGetSessionTokenMode, etc., matching the expected signature used by the login route.tests/security/session-store.test.ts (1)
67-80: 测试结构与覆盖范围整体良好各测试用例覆盖了
create、read、revoke、rotate的核心路径及异常分支（Redis 不就绪、setex 失败、del 失败），使用vi.useFakeTimers()保证时间确定性，mock 注入通过vi.hoisted()正确设置。Line 74 的 UUID 正则
/^sid_[0-9a-f-]{36}$/i能通过正确的 UUID 验证，但理论上[0-9a-f-]{36}也能匹配非标准字符串（如全连字符）。如需更严格验证可替换为/^sid_[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i，但对于当前测试场景影响不大。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/session-store.test.ts` around lines 67 - 80, The sessionId assertion uses a loose UUID pattern; update the regex used in the "create() returns session data with generated sessionId" test (the expect(created.sessionId).toMatch(...) line) to a stricter UUID v4-like pattern such as /^sid_[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i so it only matches standard hyphenated UUIDs while keeping the rest of the assertions (created.keyFingerprint, created.userId, created.userRole, created.createdAt, created.expiresAt) unchanged.src/lib/auth-session-store/redis-session-store.ts (2)
23-29:normalizeTtlSeconds中typeof value !== "number"检查冗余
Number.isFinite(x)对非数字值（包括undefined、NaN、Infinity）均返回false，因此!Number.isFinite(value)已经能覆盖所有非数字情况，typeof value !== "number"这一分支永远不会独立触发。同样的冗余模式出现在parseSessionData（Line 46-48）和resolveRotateTtlSeconds（Line 64）中。♻️ 建议简化
function normalizeTtlSeconds(value: number | undefined): number { - if (!Number.isFinite(value) || typeof value !== "number" || value <= 0) { + if (typeof value !== "number" || !Number.isFinite(value) || value <= 0) { return DEFAULT_SESSION_TTL; }或者更简洁（因为类型签名已保证
value只可能是number | undefined）：- if (!Number.isFinite(value) || typeof value !== "number" || value <= 0) { + if (!Number.isFinite(value) || value <= 0) {
parseSessionData和resolveRotateTtlSeconds中的同类写法同理修正（先typeof，再Number.isFinite，语义更清晰；或只保留Number.isFinite）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth-session-store/redis-session-store.ts` around lines 23 - 29, The checks using typeof value !== "number" are redundant because Number.isFinite already returns false for non-number values; update normalizeTtlSeconds, parseSessionData, and resolveRotateTtlSeconds to simplify the guards by removing the redundant typeof checks (or alternatively check typeof first then Number.isFinite for clearer intent), so each function should rely on a single clear validation (e.g., !Number.isFinite(value) || value <= 0) and preserve the existing fallback logic (DEFAULT_SESSION_TTL / MIN_TTL_SECONDS behavior) and return types.
202-209:rotate()中的读回验证（read-after-write）是冗余的额外 Redis 往返
create()已保证：若setex失败则抛出异常，执行到 Line 202 时 session 必然已持久化。再次调用this.read(nextSession.sessionId)仅为了"验证"写入成功，增加了一次不必要的 Redis 往返，且在高并发下还存在 TOCTOU 可能性（写入成功后 key 被外部删除会误判为写入失败）。建议直接使用
create()返回的nextSession作为结果返回。♻️ 建议简化 rotate()
} catch (error) { logger.error("[AuthSessionStore] Failed to create rotated session", { error: toLogError(error), oldSessionId, }); return null; } - const persisted = await this.read(nextSession.sessionId); - if (!persisted) { - logger.error("[AuthSessionStore] Failed to persist rotated session", { - oldSessionId, - newSessionId: nextSession.sessionId, - }); - return null; - } - const revoked = await this.revoke(oldSessionId); if (!revoked) { logger.warn( "[AuthSessionStore] Failed to revoke old session during rotate; old session will expire naturally", { oldSessionId, - newSessionId: persisted.sessionId, + newSessionId: nextSession.sessionId, } ); } - return persisted; + return nextSession;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth-session-store/redis-session-store.ts` around lines 202 - 209, The rotate() method performs an unnecessary read-after-write by calling this.read(nextSession.sessionId) after create() — since create() throws on setex failure, the session is already persisted, and the extra read adds a needless Redis round-trip and TOCTOU risk; remove the this.read(...) check and simply return the nextSession produced by create() (references: rotate(), create(), read(), nextSession, oldSessionId) so the function returns nextSession directly when create() completes successfully.
🤖 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/auth/login/route.ts`:
- Around line 106-112: getClientIp currently returns the literal "unknown" which
collapses all requests without proxy headers into one shared rate-limit bucket;
change getClientIp to return null (or undefined) instead of "unknown" when no
x-forwarded-for/x-real-ip is present, and ensure the caller (e.g.,
LoginAbusePolicy) detects a null clientIp and either skips IP-based rate
limiting or records a log entry; update getClientIp signature and call sites to
handle the nullable value and add a debug/error log when proxy headers are
missing so you don't accidentally group unrelated clients.
- Around line 126-129: The response uses a hardcoded user-facing string
"Forbidden" in withAuthResponseHeaders(NextResponse.json(...)) — move the
locale/translator initialization (the code at or after line 132 that obtains
locale/translator) to run before the CSRF check, then replace the literal with a
translated message (e.g., use the translator function t('forbidden') or a
preloaded common translation key like common.t('forbidden')) when constructing
NextResponse.json; ensure you still return withAuthResponseHeaders(...). This
keeps the CSRF branch i18n-compliant without changing the response shape.
In `@src/lib/auth.ts`:
- Around line 319-329: The code caches a rejected Promise in sessionStorePromise
causing permanent failure when Redis or dynamic import fails; update
getSessionStore so that when creating the RedisSessionStore (the
import("@/lib/auth-session-store/redis-session-store") and new
RedisSessionStore()) fails you reset sessionStorePromise back to null and
rethrow the error to allow future retries. Concretely, wrap the dynamic
import/instantiation in a try/catch (or attach a .catch) that sets
sessionStorePromise = null on any error before propagating the error; keep the
exported symbol names getSessionStore, sessionStorePromise, and
RedisSessionStore unchanged.
In `@tests/security/session-fixation-rotation.test.ts`:
- Around line 160-176: The test currently calls the helper
simulatePostLoginSessionRotation with a Vitest mock (mockRotate) and never
exercises the real login route handler, so update the test to dynamically import
the real POST handler from src/app/api/auth/login/route (similar to
loadLogoutPost) and invoke that with mocked request/context and mocked
store.rotate; ensure SESSION_TOKEN_MODE and store.rotate are mocked
appropriately, assert store.rotate was called with the old session id and that
the returned/response session id differs from the original (refer to
simulatePostLoginSessionRotation, mockRotate, loadLogoutPost,
loadLoginPost/POST, and store.rotate to locate code to change).
---
Duplicate comments:
In `@src/lib/auth-session-store/redis-session-store.ts`:
- Around line 98-129: create() was previously silently returning an unpersisted
session; ensure it instead throws when Redis is unavailable or writes fail by
keeping the explicit check and throw in getReadyRedis() usage and the try/catch
around redis.setex; specifically, in auth-session-store/redis-session-store.ts
ensure the create method (function create) continues to throw new Error("Redis
not ready: session not persisted") when this.getReadyRedis() returns falsy and
continues to log and rethrow the caught error (using logger.error with
toLogError(error) and the sessionId) inside the catch around redis.setex so
callers cannot receive an unpersisted SessionData.
- Around line 211-220: The rotate() implementation should not roll back or
delete the newly persisted session when revoke(oldSessionId) returns false;
instead it must log a warning and allow the old session to expire via Redis TTL.
Update the rotate() logic (function rotate and call to revoke) to keep the new
session (persisted.sessionId) intact on revoke failure, ensure the logger.warn
call includes oldSessionId and newSessionId for context, and remove any code
that would undo or delete the newly saved session when revoked === false.
In `@tests/unit/api/auth-login-route.test.ts`:
- Around line 105-114: Ensure the test's beforeEach calls vi.resetModules() to
reset the LoginAbusePolicy singleton between tests and then dynamically import
the route using the path alias (import("@/app/api/auth/login/route")) so the
POST handler is loaded fresh; verify mockGetTranslations, mockSetAuthCookie, and
mockGetSessionTokenMode are set before importing and that POST is assigned from
the imported module.
---
Nitpick comments:
In `@src/app/api/auth/login/route.ts`:
- Around line 22-28: loginPolicy 是在进程内创建的单例（LoginAbusePolicy
实例），其状态保存在内存中，无法在无服务器冷启动或多实例部署下跨实例共享；请在 src/app/api/auth/login/route.ts 在
loginPolicy 定义附近添加明确注释/文档说明该限流/风控为单实例实现及其局限性，并在注释中列出后续迁移建议（例如将 LoginAbusePolicy
的状态后端改为 Redis 或其它集中式存储以支持跨实例限流），以便维护者知晓并在未来替换为基于 Redis 的实现（参考 LoginAbusePolicy 和
loginPolicy 标识符以定位代码）。
- Around line 114-121: createOpaqueSession currently constructs a new
RedisSessionStore on every call (new RedisSessionStore()), which diverges from
the lazy-singleton pattern used in auth.ts (sessionStorePromise). Update
createOpaqueSession to obtain and reuse the shared session store instead of
instantiating a new one—e.g., import or call the same session store
factory/singleton used in auth.ts (sessionStorePromise or the exported getter)
and then call store.create(...) with that instance; ensure to await the promise
if the singleton is async and keep the existing payload (toKeyFingerprint(key),
session.user.id, session.user.role).
In `@src/lib/auth-session-store/redis-session-store.ts`:
- Around line 23-29: The checks using typeof value !== "number" are redundant
because Number.isFinite already returns false for non-number values; update
normalizeTtlSeconds, parseSessionData, and resolveRotateTtlSeconds to simplify
the guards by removing the redundant typeof checks (or alternatively check
typeof first then Number.isFinite for clearer intent), so each function should
rely on a single clear validation (e.g., !Number.isFinite(value) || value <= 0)
and preserve the existing fallback logic (DEFAULT_SESSION_TTL / MIN_TTL_SECONDS
behavior) and return types.
- Around line 202-209: The rotate() method performs an unnecessary
read-after-write by calling this.read(nextSession.sessionId) after create() —
since create() throws on setex failure, the session is already persisted, and
the extra read adds a needless Redis round-trip and TOCTOU risk; remove the
this.read(...) check and simply return the nextSession produced by create()
(references: rotate(), create(), read(), nextSession, oldSessionId) so the
function returns nextSession directly when create() completes successfully.
In `@src/lib/auth.ts`:
- Around line 360-370: The functions getSessionWithDualRead and validateSession
are simple pass-throughs to getSession and need explicit intent comments; update
the implementations by adding concise doc/comments above getSessionWithDualRead
and validateSession stating they are deliberate placeholders for future
dual-read or validation logic (e.g., read-only vs read-write session handling
and additional validation steps), describe the expected future changes (what
dual-read means and when validateSession will diverge), and note why callers
should use these wrappers instead of calling getSession directly to make the
design intent clear; keep the runtime behavior unchanged.
- Around line 343-358: convertToAuthSession currently recomputes SHA-256 for
every key by calling toKeyFingerprint inside the loop; add caching or persist
fingerprint to avoid repeated async hashing: either extend the key records
returned by findKeyList to include a precomputed fingerprint (compute and store
fingerprint when keys are created/updated) and compare against normalized
fingerprint directly, or implement an in-memory Map cache keyed by key.id or the
raw key string and consult it before calling toKeyFingerprint; update
convertToAuthSession to use the cached/persisted fingerprint lookup and still
fall back to toKeyFingerprint only on cache miss, then call validateKey(key.key,
options) when fingerprints match.
In `@src/lib/security/auth-response-headers.ts`:
- Around line 6-18: 当前将 enableHsts 绑定到 env.ENABLE_SECURE_COOKIES 有语义耦合风险：HSTS
与安全 cookie 是不同的设置。修改 applySecurityHeaders 中对 buildSecurityHeaders
的调用，改为读取独立的环境变量（例如 env.ENABLE_HSTS）来驱动 enableHsts，并在该变量未设置时回退到
env.ENABLE_SECURE_COOKIES 以保证向后兼容；相关符号：applySecurityHeaders, getEnvConfig,
buildSecurityHeaders, env.ENABLE_SECURE_COOKIES, enableHsts。
In `@src/proxy.ts`:
- Around line 13-15: 更新注释以反映当前实现：将第 14 行注释中提到的 validateKey 改为
validateAuthToken，以匹配现在的验证函数名称（参照常量 READ_ONLY_PATH_PATTERNS 和函数
validateAuthToken），确保注释说明这些只读路径绕过 validateAuthToken 中的 canLoginWebUi 检查。
In `@tests/security/auth-csrf-route-integration.test.ts`:
- Around line 112-116: Replace the relative imports that pull in the route
modules with the project path alias so the test uses "@/..." instead of
"../../src/..."; specifically update the import statements that assign
loginRoute and logoutRoute (and the derived loginPost and logoutPost) to import
from "@/app/api/auth/login/route" and "@/app/api/auth/logout/route"
respectively, ensuring the rest of the test still references loginPost and
logoutPost unchanged.
- Around line 19-31: The mock for "@/lib/auth" is missing the toKeyFingerprint
export used by the login route; update the vi.mock block to include a
toKeyFingerprint export (e.g., a simple stub function or deterministic
transformer) so tests that run with getSessionTokenMode returning "dual" or
"opaque" won't fail; specifically add a toKeyFingerprint function alongside
mockValidateKey, mockSetAuthCookie, mockGetSessionTokenMode, etc., matching the
expected signature used by the login route.
In `@tests/security/session-contract.test.ts`:
- Line 23: Update the dynamic imports that reference the auth module to use the
project path alias instead of relative paths: replace imports like await
import("../../src/lib/auth") with await import("@/lib/auth") (affecting uses
that import getSessionTokenMode and other symbols from the same module). Locate
occurrences in the test file where getSessionTokenMode (and other imports from
lib/auth) are dynamically imported and change them to "@/lib/auth" to conform to
the coding guideline.
In `@tests/security/session-store.test.ts`:
- Around line 67-80: The sessionId assertion uses a loose UUID pattern; update
the regex used in the "create() returns session data with generated sessionId"
test (the expect(created.sessionId).toMatch(...) line) to a stricter UUID
v4-like pattern such as
/^sid_[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i so it only
matches standard hyphenated UUIDs while keeping the rest of the assertions
(created.keyFingerprint, created.userId, created.userRole, created.createdAt,
created.expiresAt) unchanged.
In `@vitest.config.ts`:
- Around line 8-19: 当前 projects 配置将安全测试包含在使用 extends: true 的 happy-dom 项目中，导致
tests/security/**/* 被在浏览器仿真环境运行；请为后端安全测试创建一个独立项目条目（projects 数组内的另一个对象）并将其
test.environment 设置为 "node"，将 include 指向
"tests/security/**/*.{test,spec}.{ts,tsx}"，同时从原有使用 environment "happy-dom" 的项目的
include 中移除该 security 路径；保留 extends: true 行为以继承根配置但确保 test.environment 和 include
在两个项目中各自正确指定。
| function getClientIp(request: NextRequest): string { | ||
| return ( | ||
| request.headers.get("x-forwarded-for")?.split(",")[0]?.trim() || | ||
| request.headers.get("x-real-ip")?.trim() || | ||
| "unknown" | ||
| ); | ||
| } |
There was a problem hiding this comment.
getClientIp 回退到 "unknown" 会导致所有缺少代理头的请求共享同一限流桶。
当请求不经过反向代理（如本地开发或某些部署配置），所有请求的 clientIp 都是 "unknown"，在 LoginAbusePolicy 中共享同一个 IP 桶。这意味着一个用户的失败尝试会影响所有未携带转发头的请求。
建议将此场景记录到日志或考虑跳过 IP 维度的限流：
建议修改
function getClientIp(request: NextRequest): string {
return (
request.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ||
request.headers.get("x-real-ip")?.trim() ||
- "unknown"
+ request.ip ||
+ "unknown"
);
}🤖 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 106 - 112, getClientIp
currently returns the literal "unknown" which collapses all requests without
proxy headers into one shared rate-limit bucket; change getClientIp to return
null (or undefined) instead of "unknown" when no x-forwarded-for/x-real-ip is
present, and ensure the caller (e.g., LoginAbusePolicy) detects a null clientIp
and either skips IP-based rate limiting or records a log entry; update
getClientIp signature and call sites to handle the nullable value and add a
debug/error log when proxy headers are missing so you don't accidentally group
unrelated clients.
| return withAuthResponseHeaders( | ||
| NextResponse.json({ error: "Forbidden", errorCode: "CSRF_REJECTED" }, { status: 403 }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
"Forbidden" 为硬编码英文字符串，违反 i18n 规范。
CSRF 被拒绝时返回的 error: "Forbidden" 是硬编码的用户可见文本。应使用翻译函数生成此消息。但注意此处 CSRF 检查在翻译加载之前执行（第 132 行才获取 locale），需要调整执行顺序或使用预加载的通用翻译。
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 126 - 129, The response uses a
hardcoded user-facing string "Forbidden" in
withAuthResponseHeaders(NextResponse.json(...)) — move the locale/translator
initialization (the code at or after line 132 that obtains locale/translator) to
run before the CSRF check, then replace the literal with a translated message
(e.g., use the translator function t('forbidden') or a preloaded common
translation key like common.t('forbidden')) when constructing NextResponse.json;
ensure you still return withAuthResponseHeaders(...). This keeps the CSRF branch
i18n-compliant without changing the response shape.
| it("post-login rotation returns a different session id", async () => { | ||
| const oldSessionId = "sid_existing_session"; | ||
| mockRotate.mockResolvedValue({ | ||
| sessionId: "sid_rotated_session", | ||
| keyFingerprint: "fp-login", | ||
| userId: 7, | ||
| userRole: "user", | ||
| createdAt: 1_700_000_000_000, | ||
| expiresAt: 1_700_000_300_000, | ||
| }); | ||
|
|
||
| const rotatedSessionId = await simulatePostLoginSessionRotation(oldSessionId, mockRotate); | ||
|
|
||
| expect(mockRotate).toHaveBeenCalledWith(oldSessionId); | ||
| expect(rotatedSessionId).toBe("sid_rotated_session"); | ||
| expect(rotatedSessionId).not.toBe(oldSessionId); | ||
| }); |
There was a problem hiding this comment.
simulatePostLoginSessionRotation 测试未覆盖实际登录路由的轮换逻辑
该测试直接将 mockRotate（Vitest mock 函数）传给辅助函数，实际上只验证了"辅助函数能提取 sessionId"，并未触发真正的登录路由处理器（src/app/api/auth/login/route.ts）。
测试名 "post-login rotation returns a different session id" 暗示其验证了登录后的会话轮换行为，但实际上登录路由中的轮换逻辑（如判断 SESSION_TOKEN_MODE、调用 store.rotate()）完全未被测试到，存在覆盖盲区。
建议参照同文件中 loadLogoutPost() 的方式，通过动态导入真实登录路由并配合 mock 数据来验证登录后会话轮换行为：
// 示例：通过动态导入真实登录路由测试轮换
async function loadLoginPost() {
const mod = await import("../../src/app/api/auth/login/route");
return mod.POST;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/security/session-fixation-rotation.test.ts` around lines 160 - 176, The
test currently calls the helper simulatePostLoginSessionRotation with a Vitest
mock (mockRotate) and never exercises the real login route handler, so update
the test to dynamically import the real POST handler from
src/app/api/auth/login/route (similar to loadLogoutPost) and invoke that with
mocked request/context and mocked store.rotate; ensure SESSION_TOKEN_MODE and
store.rotate are mocked appropriately, assert store.rotate was called with the
old session id and that the returned/response session id differs from the
original (refer to simulatePostLoginSessionRotation, mockRotate, loadLogoutPost,
loadLoginPost/POST, and store.rotate to locate code to change).
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
src/actions/providers.ts (2)
1036-1037: 撤销窗口仅 10 秒，用户操作时间可能不够。
PROVIDER_PATCH_UNDO_TTL_MS = 10_000（10 秒）对用户交互来说非常紧张——用户需要在 10 秒内确认并发起撤销请求。建议至少放宽到 30–60 秒，或使其可配置。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/providers.ts` around lines 1036 - 1037, Increase the undo window by adjusting the PROVIDER_PATCH_UNDO_TTL_MS constant (currently 10_000) to a more user-friendly default (e.g., 30_000–60_000) or make it configurable via environment/config (read from config or process.env and fallback to a DEFAULT_UNDO_TTL_MS). Update the constant definition for PROVIDER_PATCH_UNDO_TTL_MS in this file and the code that references it so it loads the configurable value (keep the existing PROVIDER_BATCH_PREVIEW_TTL_MS untouched unless you want parity), and ensure any tests or consumers that depend on PROVIDER_PATCH_UNDO_TTL_MS are updated to use the new default or injected config.
1113-1114: 多实例部署下模块级存储的限制需要文档说明。
providerBatchPatchPreviewStore和providerPatchUndoStore使用模块级Map存储，在多实例（多 pod）部署时存在问题：若 preview 请求与 apply 请求命中不同实例，apply 会直接返回PREVIEW_EXPIRED。需注意的背景：这些操作仅限管理员使用，TTL 周期很短（预览 60 秒、撤销 10 秒），通常在单次会话内完成。不过，为了支持真正的多实例部署（如 Kubernetes），建议添加代码注释说明此限制，并在需要时迁移到 Redis。项目已有 Redis 基础设施，迁移是可行的。
建议在代码中添加注释清晰标注此限制：
建议添加注释
+// NOTE: 当前使用模块级 Map 存储仅适用于单实例部署。 +// 多实例（多 pod）部署场景下，需迁移到 Redis 等共享存储，避免跨实例请求导致预览过期。 const providerBatchPatchPreviewStore = new Map<string, ProviderBatchPatchPreviewSnapshot>(); const providerPatchUndoStore = new Map<string, ProviderPatchUndoSnapshot>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/providers.ts` around lines 1113 - 1114, providerBatchPatchPreviewStore and providerPatchUndoStore are module-level Maps which will lose consistency across multiple service instances (e.g., multiple pods) causing PREVIEW_EXPIRED when preview and apply hit different instances; update the code near the Map declarations (providerBatchPatchPreviewStore and providerPatchUndoStore) to add a clear comment/TODO stating this limitation, the short TTL behavior (preview 60s, undo 10s), and recommend migration to a shared store (Redis) for multi-instance deployments (reference existing Redis infra) or replace the Maps with Redis-backed equivalents when needed.src/lib/auth-session-store/redis-session-store.ts (1)
130-156:read()不检查 session 是否已过期（application-level expiry check）。虽然 Redis 的 TTL 机制通常会自动清理过期的 key，但在 Redis 和应用服务器之间存在时钟偏差、或 TTL 设置不精确的情况下，
read()可能返回一个expiresAt < Date.now()的 session。PR 描述中也提到这是一个已知问题（"missing application-level session expiry check in validateAuthToken"）。建议在后续迭代中增加：
if (parsed.expiresAt <= Date.now()) { logger.debug("[AuthSessionStore] Session expired", { sessionId }); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth-session-store/redis-session-store.ts` around lines 130 - 156, The read() method can return sessions whose parsed.expiresAt is already past due; after parsing the session (use parseSessionData on the value returned from redis.get(buildSessionKey(sessionId))) add an application-level expiry check that compares parsed.expiresAt to Date.now(), log with logger.debug("[AuthSessionStore] Session expired", { sessionId }) when expired, and return null; keep existing error handling intact so other failures still log via logger.error and return null.tests/security/session-contract.test.ts (1)
75-111: 测试覆盖了 dual 和 legacy 模式，但缺少 opaque 模式下拒绝 legacy token 的测试。当前测试验证了：
dual模式同时接受 legacy 和 opaque token ✓legacy模式仅接受 legacy token、拒绝 opaque token ✓建议补充
opaque模式的测试，验证仅接受 opaque token、拒绝 legacy token：建议补充的测试用例
+ it("accepts only opaque session in opaque mode", async () => { + process.env.SESSION_TOKEN_MODE = "opaque"; + + vi.resetModules(); + const { getSessionTokenMode, getSessionTokenMigrationFlags, isSessionTokenAccepted } = + await import("@/lib/auth"); + + const mode = getSessionTokenMode(); + expect(mode).toBe("opaque"); + expect(getSessionTokenMigrationFlags(mode)).toEqual({ + dualReadWindowEnabled: false, + hardCutoverEnabled: true, + emergencyRollbackEnabled: false, + }); + + expect(isSessionTokenAccepted("sk-legacy-cookie", mode)).toBe(false); + expect(isSessionTokenAccepted("sid_opaque_session_cookie", mode)).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/session-contract.test.ts` around lines 75 - 111, Add a new test case for SESSION_TOKEN_MODE = "opaque" that imports getSessionTokenMode, getSessionTokenMigrationFlags, and isSessionTokenAccepted (same pattern as existing tests) and asserts mode === "opaque", the migration flags match expected for opaque mode (dualReadWindowEnabled: false, hardCutoverEnabled: true, emergencyRollbackEnabled: false), then verify isSessionTokenAccepted("sid_opaque_session_cookie", mode) returns true and isSessionTokenAccepted("sk-legacy-cookie", mode) returns false.tests/unit/api/auth-login-route.test.ts (1)
16-27:@/lib/auth的 mock 中包含withNoStoreHeaders，但登录路由未直接使用该导出。
route.ts从@/lib/security/auth-response-headers导入withAuthResponseHeaders，不直接导入withNoStoreHeaders。如果withAuthResponseHeaders的真实实现内部调用了withNoStoreHeaders，但这里withAuthResponseHeaders已被单独 mock（Lines 41-47），因此这个 mock 不会被执行。可以移除以减少维护负担，也可以保留作为防御性 mock 以防未来导入变化。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/auth-login-route.test.ts` around lines 16 - 27, The test mock for "@/lib/auth" includes withNoStoreHeaders which the login route doesn't import; remove withNoStoreHeaders from the "@/lib/auth" mock to reduce maintenance, and ensure the actual withAuthResponseHeaders import used by route.ts remains mocked via the existing mock for "@/lib/security/auth-response-headers" (i.e., keep or adjust the withAuthResponseHeaders mock) so the route's header behavior is still covered.src/app/api/auth/login/route.ts (1)
53-93:getAuthErrorTranslations和getAuthSecurityTranslations仅命名空间不同，可提取为通用函数。两个函数的结构完全相同（尝试加载 → 失败回退到 defaultLocale → 再失败返回 null），仅
namespace参数不同。可以提取为一个通用的翻译加载器减少重复。建议提取通用翻译加载器
-async function getAuthErrorTranslations(locale: Locale) { - try { - return await getTranslations({ locale, namespace: "auth.errors" }); - } catch (error) { - logger.warn("Login route: failed to load auth.errors translations", { - locale, - error: error instanceof Error ? error.message : String(error), - }); - - try { - return await getTranslations({ locale: defaultLocale, namespace: "auth.errors" }); - } catch (fallbackError) { - logger.error("Login route: failed to load default auth.errors translations", { - locale: defaultLocale, - error: fallbackError instanceof Error ? fallbackError.message : String(fallbackError), - }); - return null; - } - } -} - -async function getAuthSecurityTranslations(locale: Locale) { - try { - return await getTranslations({ locale, namespace: "auth.security" }); - } catch (error) { - logger.warn("Login route: failed to load auth.security translations", { - locale, - error: error instanceof Error ? error.message : String(error), - }); - - try { - return await getTranslations({ locale: defaultLocale, namespace: "auth.security" }); - } catch (fallbackError) { - logger.error("Login route: failed to load default auth.security translations", { - locale: defaultLocale, - error: fallbackError instanceof Error ? fallbackError.message : String(fallbackError), - }); - return null; - } - } -} +async function loadTranslationsWithFallback(locale: Locale, namespace: string) { + try { + return await getTranslations({ locale, namespace }); + } catch (error) { + logger.warn(`Login route: failed to load ${namespace} translations`, { + locale, + error: error instanceof Error ? error.message : String(error), + }); + + try { + return await getTranslations({ locale: defaultLocale, namespace }); + } catch (fallbackError) { + logger.error(`Login route: failed to load default ${namespace} translations`, { + locale: defaultLocale, + error: fallbackError instanceof Error ? fallbackError.message : String(fallbackError), + }); + return null; + } + } +}🤖 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 53 - 93, Extract the duplicated logic in getAuthErrorTranslations and getAuthSecurityTranslations into a single reusable async function (e.g., loadTranslations or getTranslationsWithFallback) that accepts (locale: Locale, namespace: string), uses getTranslations(locale, namespace) with the same try→catch→fallback-to-defaultLocale→return-null flow, and preserves the same logger messages (use the namespace in the log text or meta). Replace both original functions to call this new loader with namespace "auth.errors" and "auth.security" respectively, reusing defaultLocale and the existing error message formatting (error instanceof Error ? error.message : String(error)).src/lib/auth.ts (2)
373-383:getSessionWithDualRead和validateSession仅为getSession的透传别名。这两个函数当前不增加任何逻辑，仅作为 API 表面的语义化入口。如果这是为了保持向后兼容或为后续差异化预留空间，建议添加简要注释说明意图，否则可考虑合并。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth.ts` around lines 373 - 383, getSessionWithDualRead and validateSession are pure pass-through aliases to getSession; either document their intent or remove them. If you want to keep them for backward compatibility or future extension, add a brief comment above getSessionWithDualRead and validateSession stating they are semantic aliases for getSession retained for API compatibility/future differentiation; otherwise delete these wrapper functions and replace all internal references to getSessionWithDualRead and validateSession with getSession to avoid dead indirection.
262-287:validateAuthToken的 token 模式分派逻辑正确，但缺少 application-level session 过期检查。在 opaque/dual 模式下，如果 session store 返回了一个
expiresAt < Date.now()的 session（如前述 Redis TTL 不精确的情况），convertToAuthSession不会检查过期。建议在调用convertToAuthSession前增加过期检查：if (sessionData && sessionData.expiresAt <= Date.now()) { logger.debug("Session expired at application level", { sessionId: token }); sessionData = null; }这与 PR 描述中提到的已知问题一致（"missing application-level session expiry check in validateAuthToken"）。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth.ts` around lines 262 - 287, In validateAuthToken, add an application-level expiry check after reading from the session store (getSessionStore/read) and before calling convertToAuthSession: if sessionData?.expiresAt exists and is <= Date.now(), log a debug message referencing the session token (e.g., "Session expired at application level") and treat sessionData as null/skip conversion so expired sessions are not returned; otherwise proceed to convertToAuthSession as before. Ensure you reference validateAuthToken, getSessionStore, sessionStore.read, and convertToAuthSession when making the change.src/app/api/auth/logout/route.ts (1)
45-56: 每次登出都创建新的RedisSessionStore实例，与 login 路由的缓存模式不一致。login 路由（
src/app/api/auth/login/route.tsLines 113-123）使用sessionStoreInstance模块级缓存来复用RedisSessionStore，而 logout 路由在每次请求时都new RedisSessionStore()。虽然底层 Redis 客户端是共享的，但建议保持一致的实例管理策略。建议复用同样的惰性缓存模式
+let sessionStoreInstance: + | import("@/lib/auth-session-store/redis-session-store").RedisSessionStore + | null = null; + +function getLogoutSessionStore() { + if (!sessionStoreInstance) { + sessionStoreInstance = new RedisSessionStore(); + } + return sessionStoreInstance; +} + if (mode !== "legacy") { try { const sessionId = await resolveAuthCookieToken(); if (sessionId) { - const store = new RedisSessionStore(); + const store = getLogoutSessionStore(); await store.revoke(sessionId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/auth/logout/route.ts` around lines 45 - 56, The logout route creates a new RedisSessionStore on every request (new RedisSessionStore()) which is inconsistent with login's module-level cached sessionStoreInstance; change logout to reuse the same lazy-cached instance (sessionStoreInstance) instead of instantiating a fresh RedisSessionStore, i.e. obtain the shared store used by login, check for the resolved sessionId via resolveAuthCookieToken(), and call revoke on that shared store while keeping the existing try/catch and logger.warn usage for errors.
🤖 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/actions/providers.ts`:
- Around line 1262-1354: applyProviderBatchPatch and undoProviderPatch are not
performing any DB writes but still return non-zero updatedCount/revertedCount;
either implement the DB operations or make the return values reflect no-op. Fix
by invoking the repository methods (e.g., call updateProvidersBatch(providerIds,
normalizedPatch.data) inside applyProviderBatchPatch and use its result to set
applyResult.updatedCount and the undo payload; add proper error handling and
persist any necessary change metadata for undo (operationId/undoToken). For
undoProviderPatch call the repository revert method (e.g.,
revertProvidersBatch(operationId or stored provider IDs)) and set revertedCount
from that operation, or if you intentionally leave DB writes unimplemented, add
a clear TODO comment in applyProviderBatchPatch and undoProviderPatch and change
updatedCount/revertedCount to 0 to avoid misleading callers.
In `@src/lib/auth-session-store/redis-session-store.ts`:
- Around line 62-69: The current resolveRotateTtlSeconds function returns
MIN_TTL_SECONDS for expired sessions (expiresAt < Date.now()), causing rotate()
to produce near-immediately-expired sessions; modify resolveRotateTtlSeconds so
that if remainingMs <= 0 it returns DEFAULT_SESSION_TTL instead of
MIN_TTL_SECONDS, and update any callers (notably rotate()) to rely on this
behavior; alternatively (or additionally) add an expiry guard in rotate() that
prevents rotating an already-expired session by checking expiresAt <= Date.now()
and handling that case (e.g., reject/cleanup) rather than creating a 1-second
session.
In `@src/lib/auth.ts`:
- Around line 346-371: convertToAuthSession currently computes SHA-256
fingerprints for every key returned by findKeyList(userId) (via
toKeyFingerprint) on each opaque session validation, causing O(n) crypto work
per request; change the flow to avoid iterating all keys by either (1) extending
the opaque session/SessionData to include a keyId and then lookup the single key
by primary id and validate it (update session creation and convertToAuthSession
to use keyId and a direct DB fetch instead of findKeyList), or (2) store/cache
key fingerprints server-side (persist fingerprint on the Key record or maintain
an in-memory/Redis cache keyed by keyId) and compare the stored fingerprint to
expectedFingerprint so convertToAuthSession only does a single fingerprint
compare and calls validateKey only for the matched key; update usages of
findKeyList, toKeyFingerprint, and validateKey accordingly.
In `@tests/unit/api/auth-login-route.test.ts`:
- Around line 102-315: Add unit tests for the POST handler to cover missing
paths: simulate CSRF rejection to assert 403 body (exercise the POST exported
handler to hit the CSRF branch), simulate rate-limit exceeded to assert 429
status and presence of Retry-After header, test session-mode variants by setting
mockGetSessionTokenMode to "dual" and "opaque" and assert setAuthCookie behavior
is called or skipped accordingly (look for setAuthCookie usage in POST), test
hasSecureCookieHttpMismatch true to assert httpMismatchGuidance appears in
response (refer to hasSecureCookieHttpMismatch handling), and add tests toggling
shouldIncludeFailureTaxonomy to true to assert errorCode is present in failure
responses; use the same mocking pattern already in the file
(mockGetTranslations, mockValidateKey, mockSetAuthCookie,
mockGetLoginRedirectTarget, mockLogger) to drive these branches and assert the
exact status, headers and JSON fields.
---
Duplicate comments:
In `@src/app/api/auth/login/route.ts`:
- Around line 105-111: getClientIp currently falls back to the literal "unknown"
so all requests without x-forwarded-for/x-real-ip share one rate-limit bucket;
change getClientIp(NextRequest) to first try x-forwarded-for then x-real-ip then
a request-unique fallback: check for an existing "x-request-id" header and
return that if present, otherwise return a newly generated per-request id (e.g.,
crypto.randomUUID()) — replace the "unknown" literal with this unique identifier
so missing-proxy requests do not collide into the same bucket.
---
Nitpick comments:
In `@src/actions/providers.ts`:
- Around line 1036-1037: Increase the undo window by adjusting the
PROVIDER_PATCH_UNDO_TTL_MS constant (currently 10_000) to a more user-friendly
default (e.g., 30_000–60_000) or make it configurable via environment/config
(read from config or process.env and fallback to a DEFAULT_UNDO_TTL_MS). Update
the constant definition for PROVIDER_PATCH_UNDO_TTL_MS in this file and the code
that references it so it loads the configurable value (keep the existing
PROVIDER_BATCH_PREVIEW_TTL_MS untouched unless you want parity), and ensure any
tests or consumers that depend on PROVIDER_PATCH_UNDO_TTL_MS are updated to use
the new default or injected config.
- Around line 1113-1114: providerBatchPatchPreviewStore and
providerPatchUndoStore are module-level Maps which will lose consistency across
multiple service instances (e.g., multiple pods) causing PREVIEW_EXPIRED when
preview and apply hit different instances; update the code near the Map
declarations (providerBatchPatchPreviewStore and providerPatchUndoStore) to add
a clear comment/TODO stating this limitation, the short TTL behavior (preview
60s, undo 10s), and recommend migration to a shared store (Redis) for
multi-instance deployments (reference existing Redis infra) or replace the Maps
with Redis-backed equivalents when needed.
In `@src/app/api/auth/login/route.ts`:
- Around line 53-93: Extract the duplicated logic in getAuthErrorTranslations
and getAuthSecurityTranslations into a single reusable async function (e.g.,
loadTranslations or getTranslationsWithFallback) that accepts (locale: Locale,
namespace: string), uses getTranslations(locale, namespace) with the same
try→catch→fallback-to-defaultLocale→return-null flow, and preserves the same
logger messages (use the namespace in the log text or meta). Replace both
original functions to call this new loader with namespace "auth.errors" and
"auth.security" respectively, reusing defaultLocale and the existing error
message formatting (error instanceof Error ? error.message : String(error)).
In `@src/app/api/auth/logout/route.ts`:
- Around line 45-56: The logout route creates a new RedisSessionStore on every
request (new RedisSessionStore()) which is inconsistent with login's
module-level cached sessionStoreInstance; change logout to reuse the same
lazy-cached instance (sessionStoreInstance) instead of instantiating a fresh
RedisSessionStore, i.e. obtain the shared store used by login, check for the
resolved sessionId via resolveAuthCookieToken(), and call revoke on that shared
store while keeping the existing try/catch and logger.warn usage for errors.
In `@src/lib/auth-session-store/redis-session-store.ts`:
- Around line 130-156: The read() method can return sessions whose
parsed.expiresAt is already past due; after parsing the session (use
parseSessionData on the value returned from
redis.get(buildSessionKey(sessionId))) add an application-level expiry check
that compares parsed.expiresAt to Date.now(), log with
logger.debug("[AuthSessionStore] Session expired", { sessionId }) when expired,
and return null; keep existing error handling intact so other failures still log
via logger.error and return null.
In `@src/lib/auth.ts`:
- Around line 373-383: getSessionWithDualRead and validateSession are pure
pass-through aliases to getSession; either document their intent or remove them.
If you want to keep them for backward compatibility or future extension, add a
brief comment above getSessionWithDualRead and validateSession stating they are
semantic aliases for getSession retained for API compatibility/future
differentiation; otherwise delete these wrapper functions and replace all
internal references to getSessionWithDualRead and validateSession with
getSession to avoid dead indirection.
- Around line 262-287: In validateAuthToken, add an application-level expiry
check after reading from the session store (getSessionStore/read) and before
calling convertToAuthSession: if sessionData?.expiresAt exists and is <=
Date.now(), log a debug message referencing the session token (e.g., "Session
expired at application level") and treat sessionData as null/skip conversion so
expired sessions are not returned; otherwise proceed to convertToAuthSession as
before. Ensure you reference validateAuthToken, getSessionStore,
sessionStore.read, and convertToAuthSession when making the change.
In `@tests/security/session-contract.test.ts`:
- Around line 75-111: Add a new test case for SESSION_TOKEN_MODE = "opaque" that
imports getSessionTokenMode, getSessionTokenMigrationFlags, and
isSessionTokenAccepted (same pattern as existing tests) and asserts mode ===
"opaque", the migration flags match expected for opaque mode
(dualReadWindowEnabled: false, hardCutoverEnabled: true,
emergencyRollbackEnabled: false), then verify
isSessionTokenAccepted("sid_opaque_session_cookie", mode) returns true and
isSessionTokenAccepted("sk-legacy-cookie", mode) returns false.
In `@tests/unit/api/auth-login-route.test.ts`:
- Around line 16-27: The test mock for "@/lib/auth" includes withNoStoreHeaders
which the login route doesn't import; remove withNoStoreHeaders from the
"@/lib/auth" mock to reduce maintenance, and ensure the actual
withAuthResponseHeaders import used by route.ts remains mocked via the existing
mock for "@/lib/security/auth-response-headers" (i.e., keep or adjust the
withAuthResponseHeaders mock) so the route's header behavior is still covered.
| export async function applyProviderBatchPatch( | ||
| input: unknown | ||
| ): Promise<ActionResult<ApplyProviderBatchPatchResult>> { | ||
| try { | ||
| const session = await getSession(); | ||
| if (!session || session.user.role !== "admin") { | ||
| return { ok: false, error: "无权限执行此操作" }; | ||
| } | ||
|
|
||
| const parsed = ApplyProviderBatchPatchSchema.safeParse(input); | ||
| if (!parsed.success) { | ||
| return buildActionValidationError(parsed.error); | ||
| } | ||
|
|
||
| const nowMs = Date.now(); | ||
| cleanupProviderPatchStores(nowMs); | ||
|
|
||
| const snapshot = providerBatchPatchPreviewStore.get(parsed.data.previewToken); | ||
| if (!snapshot || snapshot.previewExpiresAt <= nowMs) { | ||
| providerBatchPatchPreviewStore.delete(parsed.data.previewToken); | ||
| return { | ||
| ok: false, | ||
| error: "预览已过期，请重新预览", | ||
| errorCode: PROVIDER_BATCH_PATCH_ERROR_CODES.PREVIEW_EXPIRED, | ||
| }; | ||
| } | ||
|
|
||
| const normalizedPatch = normalizeProviderBatchPatchDraft(parsed.data.patch); | ||
| if (!normalizedPatch.ok) { | ||
| return { | ||
| ok: false, | ||
| error: normalizedPatch.error.message, | ||
| errorCode: PROVIDER_PATCH_ERROR_CODES.INVALID_PATCH_SHAPE, | ||
| }; | ||
| } | ||
|
|
||
| if (!hasProviderBatchPatchChanges(normalizedPatch.data)) { | ||
| return buildNoChangesError(); | ||
| } | ||
|
|
||
| const providerIds = dedupeProviderIds(parsed.data.providerIds); | ||
| const patchSerialized = JSON.stringify(normalizedPatch.data); | ||
| const isStale = | ||
| parsed.data.previewRevision !== snapshot.previewRevision || | ||
| !isSameProviderIdList(providerIds, snapshot.providerIds) || | ||
| patchSerialized !== snapshot.patchSerialized; | ||
|
|
||
| if (parsed.data.idempotencyKey) { | ||
| const existingResult = snapshot.appliedResultByIdempotencyKey.get(parsed.data.idempotencyKey); | ||
| if (existingResult) { | ||
| return { ok: true, data: existingResult }; | ||
| } | ||
| } | ||
|
|
||
| if (isStale || snapshot.applied) { | ||
| return { | ||
| ok: false, | ||
| error: "预览内容已失效，请重新预览", | ||
| errorCode: PROVIDER_BATCH_PATCH_ERROR_CODES.PREVIEW_STALE, | ||
| }; | ||
| } | ||
|
|
||
| const appliedAt = new Date(nowMs).toISOString(); | ||
| const undoToken = createProviderPatchUndoToken(); | ||
| const undoExpiresAtMs = nowMs + PROVIDER_PATCH_UNDO_TTL_MS; | ||
|
|
||
| const applyResult: ApplyProviderBatchPatchResult = { | ||
| operationId: createProviderPatchOperationId(), | ||
| appliedAt, | ||
| updatedCount: providerIds.length, | ||
| undoToken, | ||
| undoExpiresAt: new Date(undoExpiresAtMs).toISOString(), | ||
| }; | ||
|
|
||
| snapshot.applied = true; | ||
| if (parsed.data.idempotencyKey) { | ||
| snapshot.appliedResultByIdempotencyKey.set(parsed.data.idempotencyKey, applyResult); | ||
| } | ||
|
|
||
| providerPatchUndoStore.set(undoToken, { | ||
| undoToken, | ||
| undoExpiresAt: undoExpiresAtMs, | ||
| operationId: applyResult.operationId, | ||
| providerIds, | ||
| }); | ||
|
|
||
| return { ok: true, data: applyResult }; | ||
| } catch (error) { | ||
| logger.error("应用批量补丁失败:", error); | ||
| const message = error instanceof Error ? error.message : "应用批量补丁失败"; | ||
| return { ok: false, error: message }; | ||
| } | ||
| } |
There was a problem hiding this comment.
applyProviderBatchPatch 未执行实际数据库写入。
函数返回 updatedCount: providerIds.length 和 undoToken，但没有调用任何 repository 方法（如 updateProvidersBatch）来实际修改数据库中的 provider 记录。如果这是有意为之的"contract-only"阶段实现，建议：
- 添加明确的
// TODO注释标注待实现的 DB 写入逻辑 - 或者将
updatedCount设为 0 以免误导调用方
同样的问题存在于 undoProviderPatch（Line 1356-1406）——revertedCount 被返回但无实际 DB 回滚操作。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/providers.ts` around lines 1262 - 1354, applyProviderBatchPatch
and undoProviderPatch are not performing any DB writes but still return non-zero
updatedCount/revertedCount; either implement the DB operations or make the
return values reflect no-op. Fix by invoking the repository methods (e.g., call
updateProvidersBatch(providerIds, normalizedPatch.data) inside
applyProviderBatchPatch and use its result to set applyResult.updatedCount and
the undo payload; add proper error handling and persist any necessary change
metadata for undo (operationId/undoToken). For undoProviderPatch call the
repository revert method (e.g., revertProvidersBatch(operationId or stored
provider IDs)) and set revertedCount from that operation, or if you
intentionally leave DB writes unimplemented, add a clear TODO comment in
applyProviderBatchPatch and undoProviderPatch and change
updatedCount/revertedCount to 0 to avoid misleading callers.
| function resolveRotateTtlSeconds(expiresAt: number): number { | ||
| if (!Number.isFinite(expiresAt) || typeof expiresAt !== "number") { | ||
| return DEFAULT_SESSION_TTL; | ||
| } | ||
|
|
||
| const remainingMs = expiresAt - Date.now(); | ||
| return Math.max(MIN_TTL_SECONDS, Math.ceil(remainingMs / 1000)); | ||
| } |
There was a problem hiding this comment.
resolveRotateTtlSeconds 在 session 已过期时返回 MIN_TTL_SECONDS（1秒），可能导致无意义的 rotate。
当 expiresAt < Date.now() 时，remainingMs 为负值，Math.max(1, Math.ceil(negative / 1000)) 会返回 1。这意味着已过期的 session 仍会被 "rotate" 成一个仅有 1 秒 TTL 的新 session。
rotate() 的调用方拿到的新 session 几乎立即过期。建议在 rotate() 中增加过期检查，或让此函数返回 DEFAULT_SESSION_TTL 当剩余时间为负时。
建议修改
function resolveRotateTtlSeconds(expiresAt: number): number {
if (!Number.isFinite(expiresAt) || typeof expiresAt !== "number") {
return DEFAULT_SESSION_TTL;
}
const remainingMs = expiresAt - Date.now();
+ if (remainingMs <= 0) {
+ return DEFAULT_SESSION_TTL;
+ }
return Math.max(MIN_TTL_SECONDS, Math.ceil(remainingMs / 1000));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function resolveRotateTtlSeconds(expiresAt: number): number { | |
| if (!Number.isFinite(expiresAt) || typeof expiresAt !== "number") { | |
| return DEFAULT_SESSION_TTL; | |
| } | |
| const remainingMs = expiresAt - Date.now(); | |
| return Math.max(MIN_TTL_SECONDS, Math.ceil(remainingMs / 1000)); | |
| } | |
| function resolveRotateTtlSeconds(expiresAt: number): number { | |
| if (!Number.isFinite(expiresAt) || typeof expiresAt !== "number") { | |
| return DEFAULT_SESSION_TTL; | |
| } | |
| const remainingMs = expiresAt - Date.now(); | |
| if (remainingMs <= 0) { | |
| return DEFAULT_SESSION_TTL; | |
| } | |
| return Math.max(MIN_TTL_SECONDS, Math.ceil(remainingMs / 1000)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth-session-store/redis-session-store.ts` around lines 62 - 69, The
current resolveRotateTtlSeconds function returns MIN_TTL_SECONDS for expired
sessions (expiresAt < Date.now()), causing rotate() to produce
near-immediately-expired sessions; modify resolveRotateTtlSeconds so that if
remainingMs <= 0 it returns DEFAULT_SESSION_TTL instead of MIN_TTL_SECONDS, and
update any callers (notably rotate()) to rely on this behavior; alternatively
(or additionally) add an expiry guard in rotate() that prevents rotating an
already-expired session by checking expiresAt <= Date.now() and handling that
case (e.g., reject/cleanup) rather than creating a 1-second session.
| async function convertToAuthSession( | ||
| sessionData: OpaqueSessionContract, | ||
| options?: { allowReadOnlyAccess?: boolean } | ||
| ): Promise<AuthSession | null> { | ||
| const expectedFingerprint = normalizeKeyFingerprint(sessionData.keyFingerprint); | ||
|
|
||
| // Admin token uses virtual user (id=-1) which has no DB keys; | ||
| // verify fingerprint against the configured admin token directly. | ||
| if (sessionData.userId === -1) { | ||
| const adminToken = config.auth.adminToken; | ||
| if (!adminToken) return null; | ||
| const adminFingerprint = await toKeyFingerprint(adminToken); | ||
| return adminFingerprint === expectedFingerprint ? validateKey(adminToken, options) : null; | ||
| } | ||
|
|
||
| const keyList = await findKeyList(sessionData.userId); | ||
|
|
||
| for (const key of keyList) { | ||
| const keyFingerprint = await toKeyFingerprint(key.key); | ||
| if (keyFingerprint === expectedFingerprint) { | ||
| return validateKey(key.key, options); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
convertToAuthSession 对用户的所有 key 逐一计算 SHA-256 指纹，在每次 opaque session 验证时可能造成性能瓶颈。
当前流程：opaque session 验证 → findKeyList(userId) 查询用户所有 key → 逐个 toKeyFingerprint(key.key) 计算 SHA-256 → 找到匹配后再调 validateKey(key.key) 做完整校验。
这意味着每次请求需要 至少 2 次 DB 查询 + O(n) 次 SHA-256 运算（n = 用户拥有的 key 数量）。对大多数用户可能不明显，但对于拥有大量 key 的用户，延迟会显著增加。
建议考虑：
- 在
SessionData中存储keyId，通过主键直接查询匹配的 key，避免遍历 - 或缓存 key fingerprint 映射以减少重复计算
#!/bin/bash
# 检查 SessionData 和 SessionStore 接口是否有 keyId 字段的扩展空间
echo "=== SessionData interface ==="
rg -n "interface SessionData" --type=ts -A 10
echo ""
echo "=== findKeyList usage ==="
rg -n "findKeyList" --type=ts -C3
echo ""
echo "=== Average key count per user (if any hints) ==="
rg -n "keyList\|keyCount\|keys.*length" --type=ts -C2 | head -40🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth.ts` around lines 346 - 371, convertToAuthSession currently
computes SHA-256 fingerprints for every key returned by findKeyList(userId) (via
toKeyFingerprint) on each opaque session validation, causing O(n) crypto work
per request; change the flow to avoid iterating all keys by either (1) extending
the opaque session/SessionData to include a keyId and then lookup the single key
by primary id and validate it (update session creation and convertToAuthSession
to use keyId and a direct DB fetch instead of findKeyList), or (2) store/cache
key fingerprints server-side (persist fingerprint on the Key record or maintain
an in-memory/Redis cache keyed by keyId) and compare the stored fingerprint to
expectedFingerprint so convertToAuthSession only does a single fingerprint
compare and calls validateKey only for the matched key; update usages of
findKeyList, toKeyFingerprint, and validateKey accordingly.
| describe("POST /api/auth/login", () => { | ||
| let POST: (request: NextRequest) => Promise<Response>; | ||
|
|
||
| beforeEach(async () => { | ||
| vi.resetModules(); | ||
| const mockT = vi.fn((key: string) => `translated:${key}`); | ||
| mockGetTranslations.mockResolvedValue(mockT); | ||
| mockSetAuthCookie.mockResolvedValue(undefined); | ||
| mockGetSessionTokenMode.mockReturnValue("legacy"); | ||
|
|
||
| const mod = await import("@/app/api/auth/login/route"); | ||
| POST = mod.POST; | ||
| }); | ||
|
|
||
| it("returns 400 when key is missing from body", async () => { | ||
| const res = await POST(makeRequest({})); | ||
|
|
||
| expect(res.status).toBe(400); | ||
| const json = await res.json(); | ||
| expect(json).toEqual({ error: "translated:apiKeyRequired" }); | ||
| expect(mockValidateKey).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("returns 400 when key is empty string", async () => { | ||
| const res = await POST(makeRequest({ key: "" })); | ||
|
|
||
| expect(res.status).toBe(400); | ||
| const json = await res.json(); | ||
| expect(json).toEqual({ error: "translated:apiKeyRequired" }); | ||
| }); | ||
|
|
||
| it("returns 401 when validateKey returns null", async () => { | ||
| mockValidateKey.mockResolvedValue(null); | ||
|
|
||
| const res = await POST(makeRequest({ key: "bad-key" })); | ||
|
|
||
| expect(res.status).toBe(401); | ||
| const json = await res.json(); | ||
| expect(json).toEqual({ error: "translated:apiKeyInvalidOrExpired" }); | ||
| expect(mockValidateKey).toHaveBeenCalledWith("bad-key", { | ||
| allowReadOnlyAccess: true, | ||
| }); | ||
| }); | ||
|
|
||
| it("returns 200 with correct body shape on valid key", async () => { | ||
| mockValidateKey.mockResolvedValue(fakeSession); | ||
| mockGetLoginRedirectTarget.mockReturnValue("/dashboard"); | ||
|
|
||
| const res = await POST(makeRequest({ key: "valid-key" })); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| const json = await res.json(); | ||
| expect(json).toEqual({ | ||
| ok: true, | ||
| user: { | ||
| id: 1, | ||
| name: "Test User", | ||
| description: "desc", | ||
| role: "user", | ||
| }, | ||
| redirectTo: "/dashboard", | ||
| loginType: "dashboard_user", | ||
| }); | ||
| }); | ||
|
|
||
| it("calls setAuthCookie exactly once on success", async () => { | ||
| mockValidateKey.mockResolvedValue(fakeSession); | ||
| mockGetLoginRedirectTarget.mockReturnValue("/dashboard"); | ||
|
|
||
| await POST(makeRequest({ key: "valid-key" })); | ||
|
|
||
| expect(mockSetAuthCookie).toHaveBeenCalledTimes(1); | ||
| expect(mockSetAuthCookie).toHaveBeenCalledWith("valid-key"); | ||
| }); | ||
|
|
||
| it("returns redirectTo from getLoginRedirectTarget", async () => { | ||
| mockValidateKey.mockResolvedValue(fakeSession); | ||
| mockGetLoginRedirectTarget.mockReturnValue("/my-usage"); | ||
|
|
||
| const res = await POST(makeRequest({ key: "readonly-key" })); | ||
| const json = await res.json(); | ||
|
|
||
| expect(json.redirectTo).toBe("/my-usage"); | ||
| expect(mockGetLoginRedirectTarget).toHaveBeenCalledWith(fakeSession); | ||
| }); | ||
|
|
||
| it("returns loginType admin for admin session", async () => { | ||
| mockValidateKey.mockResolvedValue(adminSession); | ||
| mockGetLoginRedirectTarget.mockReturnValue("/dashboard"); | ||
|
|
||
| const res = await POST(makeRequest({ key: "admin-key" })); | ||
| const json = await res.json(); | ||
|
|
||
| expect(json.loginType).toBe("admin"); | ||
| expect(json.redirectTo).toBe("/dashboard"); | ||
| }); | ||
|
|
||
| it("returns loginType dashboard_user for canLoginWebUi user session", async () => { | ||
| mockValidateKey.mockResolvedValue(fakeSession); | ||
| mockGetLoginRedirectTarget.mockReturnValue("/dashboard"); | ||
|
|
||
| const res = await POST(makeRequest({ key: "dashboard-key" })); | ||
| const json = await res.json(); | ||
|
|
||
| expect(json.loginType).toBe("dashboard_user"); | ||
| expect(json.redirectTo).toBe("/dashboard"); | ||
| }); | ||
|
|
||
| it("returns loginType readonly_user for readonly session", async () => { | ||
| mockValidateKey.mockResolvedValue(readonlySession); | ||
| mockGetLoginRedirectTarget.mockReturnValue("/my-usage"); | ||
|
|
||
| const res = await POST(makeRequest({ key: "readonly-key" })); | ||
| const json = await res.json(); | ||
|
|
||
| expect(json.loginType).toBe("readonly_user"); | ||
| expect(json.redirectTo).toBe("/my-usage"); | ||
| }); | ||
|
|
||
| it("returns 500 when validateKey throws", async () => { | ||
| mockValidateKey.mockRejectedValue(new Error("DB connection failed")); | ||
|
|
||
| const res = await POST(makeRequest({ key: "some-key" })); | ||
|
|
||
| expect(res.status).toBe(500); | ||
| const json = await res.json(); | ||
| expect(json).toEqual({ error: "translated:serverError" }); | ||
| expect(mockLogger.error).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("returns 500 when request.json() throws (malformed body)", async () => { | ||
| const req = new NextRequest("http://localhost/api/auth/login", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: "not-valid-json{{{", | ||
| }); | ||
|
|
||
| const res = await POST(req); | ||
|
|
||
| expect(res.status).toBe(500); | ||
| const json = await res.json(); | ||
| expect(json).toEqual({ error: "translated:serverError" }); | ||
| }); | ||
|
|
||
| it("uses NEXT_LOCALE cookie for translations", async () => { | ||
| mockValidateKey.mockResolvedValue(null); | ||
|
|
||
| await POST(makeRequest({ key: "x" }, { locale: "ja" })); | ||
|
|
||
| expect(mockGetTranslations).toHaveBeenCalledWith({ | ||
| locale: "ja", | ||
| namespace: "auth.errors", | ||
| }); | ||
| }); | ||
|
|
||
| it("detects locale from accept-language header", async () => { | ||
| mockValidateKey.mockResolvedValue(null); | ||
|
|
||
| await POST(makeRequest({ key: "x" }, { acceptLanguage: "ru;q=1.0" })); | ||
|
|
||
| expect(mockGetTranslations).toHaveBeenCalledWith({ | ||
| locale: "ru", | ||
| namespace: "auth.errors", | ||
| }); | ||
| }); | ||
|
|
||
| it("falls back to defaultLocale when getTranslations fails for requested locale", async () => { | ||
| const mockT = vi.fn((key: string) => `fallback:${key}`); | ||
| mockGetTranslations | ||
| .mockRejectedValueOnce(new Error("locale not found")) | ||
| .mockResolvedValueOnce(mockT); | ||
| mockValidateKey.mockResolvedValue(null); | ||
|
|
||
| const res = await POST(makeRequest({ key: "x" }, { locale: "ja" })); | ||
|
|
||
| expect(mockGetTranslations).toHaveBeenCalledTimes(2); | ||
| expect(mockGetTranslations).toHaveBeenNthCalledWith(1, { | ||
| locale: "ja", | ||
| namespace: "auth.errors", | ||
| }); | ||
| expect(mockGetTranslations).toHaveBeenNthCalledWith(2, { | ||
| locale: "zh-CN", | ||
| namespace: "auth.errors", | ||
| }); | ||
|
|
||
| const json = await res.json(); | ||
| expect(json.error).toBe("fallback:apiKeyInvalidOrExpired"); | ||
| }); | ||
|
|
||
| it("returns null translation when both locale and fallback fail", async () => { | ||
| mockGetTranslations | ||
| .mockRejectedValueOnce(new Error("fail")) | ||
| .mockRejectedValueOnce(new Error("fallback fail")); | ||
| mockValidateKey.mockResolvedValue(null); | ||
|
|
||
| const res = await POST(makeRequest({ key: "x" })); | ||
|
|
||
| expect(res.status).toBe(401); | ||
| const json = await res.json(); | ||
| expect(json).toEqual({ error: "Authentication failed" }); | ||
| expect(mockLogger.warn).toHaveBeenCalled(); | ||
| expect(mockLogger.error).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("falls back to defaultLocale when no locale cookie or accept-language", async () => { | ||
| mockValidateKey.mockResolvedValue(null); | ||
|
|
||
| await POST(makeRequest({ key: "x" })); | ||
|
|
||
| expect(mockGetTranslations).toHaveBeenCalledWith({ | ||
| locale: "zh-CN", | ||
| namespace: "auth.errors", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, run the verification script provided in the review comment
echo "=== CSRF route integration tests ==="
rg -n "CSRF_REJECTED\|403\|csrfGuard" tests/ --type=ts -C2 | head -60
echo ""
echo "=== Rate limiting tests ==="
rg -n "429\|RATE_LIMITED\|retryAfter\|loginPolicy" tests/ --type=ts -C2 | head -60
echo ""
echo "=== Session mode tests for login ==="
rg -n "dual\|opaque.*session\|SESSION_TOKEN_MODE" tests/ --type=ts -l | head -20Repository: ding113/claude-code-hub
Length of output: 170
🏁 Script executed:
# Check the actual route implementation to see what features it handles
echo "=== Route implementation ==="
fd -e ts -e tsx "auth/login/route" src/ || fd -e ts -e tsx "auth/login/route" . --exclude=node_modules --exclude=.nextRepository: ding113/claude-code-hub
Length of output: 95
🏁 Script executed:
# Check test file to see if it mentions coverage requirements
echo "=== Check for test files in security or integration folder ==="
fd -e test.ts -e test.tsx "security\|integration" tests/ 2>/dev/null | head -20
echo ""
echo "=== List all test files in tests directory ==="
find tests -name "*.test.ts" -o -name "*.test.tsx" | head -30Repository: ding113/claude-code-hub
Length of output: 1474
🏁 Script executed:
# Check the security test files for CSRF, rate limiting, and session mode coverage
echo "=== Check auth-csrf-route-integration.test.ts ==="
wc -l tests/security/auth-csrf-route-integration.test.ts
rg -n "403\|CSRF_REJECTED\|csrf" tests/security/auth-csrf-route-integration.test.ts --type=ts -C1 | head -40
echo ""
echo "=== Check login-abuse-policy.test.ts ==="
wc -l tests/security/login-abuse-policy.test.ts
rg -n "429\|RATE_LIMITED\|retryAfter\|loginPolicy" tests/security/login-abuse-policy.test.ts --type=ts -C1 | head -40
echo ""
echo "=== Check auth-dual-read.test.ts ==="
wc -l tests/security/auth-dual-read.test.ts
rg -n "dual\|opaque\|SESSION_TOKEN_MODE\|setAuthCookie" tests/security/auth-dual-read.test.ts --type=ts -C1 | head -40Repository: ding113/claude-code-hub
Length of output: 339
🏁 Script executed:
# Check the actual route implementation
echo "=== Find route implementation ==="
find . -path "*/app/api/auth/login/route*" -type f 2>/dev/null | head -5
# If found, check its size
if [ -f "./src/app/api/auth/login/route.ts" ]; then
echo "Found at src/"
wc -l ./src/app/api/auth/login/route.ts
elif [ -f "./app/api/auth/login/route.ts" ]; then
echo "Found at root"
wc -l ./app/api/auth/login/route.ts
else
echo "Not found, searching broader"
find . -name "route.ts" -path "*auth/login*" 2>/dev/null
fi
# Look for any references to the features mentioned
echo ""
echo "=== Search for CSRF, rate limiting in route files ==="
rg "CSRF_REJECTED\|csrfGuard\|429\|rateLimitExceeded\|dual.*session\|opaque.*session" src/ app/ --type=ts 2>/dev/null | head -30Repository: ding113/claude-code-hub
Length of output: 241
🏁 Script executed:
# Check the test file mentioned (auth-login-route.test.ts)
echo "=== Check the test file structure ==="
wc -l tests/unit/api/auth-login-route.test.ts
echo ""
echo "=== Search for all describe blocks or test types in the test file ==="
rg "describe\|it\(" tests/unit/api/auth-login-route.test.ts --type=ts | wc -l
echo ""
echo "=== Check what the test file imports and mocks ==="
head -50 tests/unit/api/auth-login-route.test.tsRepository: ding113/claude-code-hub
Length of output: 1822
🏁 Script executed:
# Check the structure of security test files that likely cover missing scenarios
echo "=== Check auth-csrf-route-integration.test.ts content ==="
head -80 tests/security/auth-csrf-route-integration.test.ts
echo ""
echo "=== Check login-abuse-policy.test.ts content ==="
head -80 tests/security/login-abuse-policy.test.tsRepository: ding113/claude-code-hub
Length of output: 5028
🏁 Script executed:
# Check the route implementation to see what features it actually has
echo "=== Route implementation content ==="
cat -n ./src/app/api/auth/login/route.ts | head -100
echo ""
echo "=== Check for CSRF, rate limiting, session mode handling in route ==="
rg "CSRF\|403\|429\|rateLimitExceeded\|dual\|opaque\|SESSION_TOKEN_MODE" ./src/app/api/auth/login/route.ts --type=ts -B2 -A2
echo ""
echo "=== Check the end of the route file ==="
tail -50 ./src/app/api/auth/login/route.tsRepository: ding113/claude-code-hub
Length of output: 5575
🏁 Script executed:
# Check if CSRF and rate limiting are in middleware or guards
echo "=== Search for CSRF guard in security directory ==="
rg "CSRF_REJECTED\|403.*CSRF\|csrfGuard" src/lib/security/ --type=ts -l
echo ""
echo "=== Search for rate limiting implementation ==="
rg "LoginAbusePolicy\|rateLimitExceeded\|429" src/lib/security/ --type=ts -l
echo ""
echo "=== Check if route uses these modules ==="
rg "LoginAbusePolicy\|csrfGuard\|CSRF" ./src/app/api/auth/login/route.tsRepository: ding113/claude-code-hub
Length of output: 210
🏁 Script executed:
# Check what the security test files actually test
echo "=== Check if auth-csrf-route-integration covers 403 responses ==="
rg "403\|expect.*status.*403\|expectStatus.*403" tests/security/auth-csrf-route-integration.test.ts --type=ts -C2
echo ""
echo "=== Check if login-abuse-policy test is unit or integration ==="
rg "429\|expect.*status.*429\|retryAfter" tests/security/login-abuse-policy.test.ts --type=ts -C2
echo ""
echo "=== Check if the route test imports or references security modules ==="
rg "LoginAbusePolicy\|csrf\|CSRF" tests/unit/api/auth-login-route.test.ts --type=tsRepository: ding113/claude-code-hub
Length of output: 269
🏁 Script executed:
# Check the actual lines in the unit test file to see exactly what's there
echo "=== Check lines around 102-315 in the unit test ==="
sed -n '102,315p' tests/unit/api/auth-login-route.test.ts | wc -l
echo ""
echo "=== Check what comes after line 315 ==="
sed -n '315,316p' tests/unit/api/auth-login-route.test.ts
tail -5 tests/unit/api/auth-login-route.test.tsRepository: ding113/claude-code-hub
Length of output: 246
🏁 Script executed:
# Check the testing guidelines - what's the expected coverage for this test file?
echo "=== Check if there's a test coverage configuration ==="
find . -name "vitest.config.*" -o -name "coverage.config.*" -o -name ".nycrc*" | xargs cat 2>/dev/null | head -50Repository: ding113/claude-code-hub
Length of output: 1460
🏁 Script executed:
# Get the complete route implementation
echo "=== Full route implementation ==="
cat -n ./src/app/api/auth/login/route.ts | tail -200
echo ""
echo "=== Search for CSRF check and rate limit check in route ==="
rg -n "csrfGuard\|loginPolicy\|CSRF_REJECTED\|RateLimitPolicy" ./src/app/api/auth/login/route.ts --type=ts -B1 -A3Repository: ding113/claude-code-hub
Length of output: 7527
🏁 Script executed:
# Check what happens in the CSRF and rate limiting tests
echo "=== Check CSRF test for 403 status ==="
grep -n "403\|CSRF_REJECTED" tests/security/auth-csrf-route-integration.test.ts
echo ""
echo "=== Check rate limiting test expectations ==="
grep -n "429\|ip_rate_limited\|retryAfter" tests/security/login-abuse-policy.test.ts
echo ""
echo "=== Check dual session test ==="
grep -n "dual\|opaque\|SESSION_TOKEN_MODE" tests/security/auth-dual-read.test.ts | head -20Repository: ding113/claude-code-hub
Length of output: 1925
🏁 Script executed:
# Verify if the route actually implements all the features mentioned in review
echo "=== Search for all response status codes in the route ==="
rg "status:" ./src/app/api/auth/login/route.ts --type=ts
echo ""
echo "=== Search for errorCode or failure taxonomy ==="
rg "errorCode\|failureTaxonomy\|shouldInclude" ./src/app/api/auth/login/route.ts --type=ts -B1 -A1
echo ""
echo "=== Check line count and structure of route POST handler ==="
grep -n "export.*POST\|async function POST" ./src/app/api/auth/login/route.tsRepository: ding113/claude-code-hub
Length of output: 748
🏁 Script executed:
# Verify if there are integration tests that cover the full login route with CSRF/rate limiting
echo "=== Check auth integration test ==="
wc -l tests/integration/auth.test.ts
grep -n "POST.*login\|setAuthCookie.*dual\|setAuthCookie.*opaque" tests/integration/auth.test.ts
echo ""
echo "=== Check full-security-regression for login route coverage ==="
grep -n "login\|POST.*auth" tests/security/full-security-regression.test.ts | head -10Repository: ding113/claude-code-hub
Length of output: 282
单元测试缺少关键路径覆盖——CSRF、速率限制和 session 模式
当前单元测试覆盖了主要的基础路径（400、401、200、500 和翻译回退），但遗漏了以下由路由实现的重要功能：
- CSRF 拒绝返回 403（route 第 135-140 行实现）
- 速率限制返回 429 + Retry-After 头（route 第 146-163 行实现）
dual/opaquesession 模式下的setAuthCookie行为（route 第 219-241 行实现）hasSecureCookieHttpMismatch场景下的httpMismatchGuidance字段（route 第 208-214 行实现）shouldIncludeFailureTaxonomy为 true 时的errorCode字段变化（route 多处实现）
这些功能虽然在 tests/security/auth-csrf-route-integration.test.ts 和 tests/security/auth-dual-read.test.ts 中分别覆盖，但建议确认整体单元测试覆盖率是否达到 80% 的要求。若需在单元测试中补充这些场景，应添加对 CSRF 拒绝、速率限制和各 session 模式的测试用例。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/auth-login-route.test.ts` around lines 102 - 315, Add unit
tests for the POST handler to cover missing paths: simulate CSRF rejection to
assert 403 body (exercise the POST exported handler to hit the CSRF branch),
simulate rate-limit exceeded to assert 429 status and presence of Retry-After
header, test session-mode variants by setting mockGetSessionTokenMode to "dual"
and "opaque" and assert setAuthCookie behavior is called or skipped accordingly
(look for setAuthCookie usage in POST), test hasSecureCookieHttpMismatch true to
assert httpMismatchGuidance appears in response (refer to
hasSecureCookieHttpMismatch handling), and add tests toggling
shouldIncludeFailureTaxonomy to true to assert errorCode is present in failure
responses; use the same mocking pattern already in the file
(mockGetTranslations, mockValidateKey, mockSetAuthCookie,
mockGetLoginRedirectTarget, mockLogger) to drive these branches and assert the
exact status, headers and JSON fields.
| } else { | ||
| try { | ||
| const opaqueSession = await createOpaqueSession(key, session); | ||
| await setAuthCookie(opaqueSession.sessionId); | ||
| } catch (error) { | ||
| logger.error("Failed to create opaque session, falling back to legacy cookie", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| await setAuthCookie(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Opaque-mode fallback creates an unusable session
When createOpaqueSession fails in opaque mode (e.g., Redis is down), line 239 falls back to setAuthCookie(key), storing the raw API key in the cookie. The user receives a successful login response (ok: true).
However, validateAuthToken in src/lib/auth.ts:262-287 does not fall through to validateKey when mode is "opaque" — it only does so for "legacy" or "dual". So on the next request:
sessionStore.read(rawApiKey)returns null (raw key is not asid_session ID)- The
mode === "legacy" || mode === "dual"check atauth.ts:282is false validateAuthTokenreturnsnull→ user is immediately redirected to login
The result: the user sees "login successful" but is kicked out on the very next page load. The fallback either needs to be removed (return an error instead) or validateAuthToken needs to accept legacy tokens in opaque mode as a degraded fallback.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 231-241
Comment:
**Opaque-mode fallback creates an unusable session**
When `createOpaqueSession` fails in opaque mode (e.g., Redis is down), line 239 falls back to `setAuthCookie(key)`, storing the raw API key in the cookie. The user receives a successful login response (`ok: true`).
However, `validateAuthToken` in `src/lib/auth.ts:262-287` does **not** fall through to `validateKey` when mode is `"opaque"` — it only does so for `"legacy"` or `"dual"`. So on the next request:
1. `sessionStore.read(rawApiKey)` returns null (raw key is not a `sid_` session ID)
2. The `mode === "legacy" || mode === "dual"` check at `auth.ts:282` is false
3. `validateAuthToken` returns `null` → user is immediately redirected to login
The result: the user sees "login successful" but is kicked out on the very next page load. The fallback either needs to be removed (return an error instead) or `validateAuthToken` needs to accept legacy tokens in opaque mode as a degraded fallback.
How can I resolve this? If you propose a fix, please make it concise.| export async function applyProviderBatchPatch( | ||
| input: unknown | ||
| ): Promise<ActionResult<ApplyProviderBatchPatchResult>> { | ||
| try { | ||
| const session = await getSession(); | ||
| if (!session || session.user.role !== "admin") { | ||
| return { ok: false, error: "无权限执行此操作" }; | ||
| } | ||
|
|
||
| const parsed = ApplyProviderBatchPatchSchema.safeParse(input); | ||
| if (!parsed.success) { | ||
| return buildActionValidationError(parsed.error); | ||
| } | ||
|
|
||
| const nowMs = Date.now(); | ||
| cleanupProviderPatchStores(nowMs); | ||
|
|
||
| const snapshot = providerBatchPatchPreviewStore.get(parsed.data.previewToken); | ||
| if (!snapshot || snapshot.previewExpiresAt <= nowMs) { | ||
| providerBatchPatchPreviewStore.delete(parsed.data.previewToken); | ||
| return { | ||
| ok: false, | ||
| error: "预览已过期，请重新预览", | ||
| errorCode: PROVIDER_BATCH_PATCH_ERROR_CODES.PREVIEW_EXPIRED, | ||
| }; | ||
| } | ||
|
|
||
| const normalizedPatch = normalizeProviderBatchPatchDraft(parsed.data.patch); | ||
| if (!normalizedPatch.ok) { | ||
| return { | ||
| ok: false, | ||
| error: normalizedPatch.error.message, | ||
| errorCode: PROVIDER_PATCH_ERROR_CODES.INVALID_PATCH_SHAPE, | ||
| }; | ||
| } | ||
|
|
||
| if (!hasProviderBatchPatchChanges(normalizedPatch.data)) { | ||
| return buildNoChangesError(); | ||
| } | ||
|
|
||
| const providerIds = dedupeProviderIds(parsed.data.providerIds); | ||
| const patchSerialized = JSON.stringify(normalizedPatch.data); | ||
| const isStale = | ||
| parsed.data.previewRevision !== snapshot.previewRevision || | ||
| !isSameProviderIdList(providerIds, snapshot.providerIds) || | ||
| patchSerialized !== snapshot.patchSerialized; | ||
|
|
||
| if (parsed.data.idempotencyKey) { | ||
| const existingResult = snapshot.appliedResultByIdempotencyKey.get(parsed.data.idempotencyKey); | ||
| if (existingResult) { | ||
| return { ok: true, data: existingResult }; | ||
| } | ||
| } | ||
|
|
||
| if (isStale || snapshot.applied) { | ||
| return { | ||
| ok: false, | ||
| error: "预览内容已失效，请重新预览", | ||
| errorCode: PROVIDER_BATCH_PATCH_ERROR_CODES.PREVIEW_STALE, | ||
| }; | ||
| } | ||
|
|
||
| const appliedAt = new Date(nowMs).toISOString(); | ||
| const undoToken = createProviderPatchUndoToken(); | ||
| const undoExpiresAtMs = nowMs + PROVIDER_PATCH_UNDO_TTL_MS; | ||
|
|
||
| const applyResult: ApplyProviderBatchPatchResult = { | ||
| operationId: createProviderPatchOperationId(), | ||
| appliedAt, | ||
| updatedCount: providerIds.length, | ||
| undoToken, | ||
| undoExpiresAt: new Date(undoExpiresAtMs).toISOString(), | ||
| }; | ||
|
|
||
| snapshot.applied = true; | ||
| if (parsed.data.idempotencyKey) { | ||
| snapshot.appliedResultByIdempotencyKey.set(parsed.data.idempotencyKey, applyResult); | ||
| } | ||
|
|
||
| providerPatchUndoStore.set(undoToken, { | ||
| undoToken, | ||
| undoExpiresAt: undoExpiresAtMs, | ||
| operationId: applyResult.operationId, | ||
| providerIds, | ||
| }); | ||
|
|
||
| return { ok: true, data: applyResult }; | ||
| } catch (error) { | ||
| logger.error("应用批量补丁失败:", error); | ||
| const message = error instanceof Error ? error.message : "应用批量补丁失败"; | ||
| return { ok: false, error: message }; | ||
| } | ||
| } |
There was a problem hiding this comment.
applyProviderBatchPatch never persists changes to the database
This function performs validation, staleness checks, idempotency deduplication, and returns { ok: true, data: { updatedCount: providerIds.length, ... } } — but never actually calls updateProvidersBatch (or any repository function) to write changes to the database. The updatedCount reported in the response is the number of providers that should have been updated, not the actual count of affected rows.
Similarly, undoProviderPatch (line 1356) doesn't perform any database revert — it simply deletes the undo token from an in-memory Map and returns success.
Callers will receive a success response believing providers have been updated, but no actual mutation occurs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/providers.ts
Line: 1262-1354
Comment:
**`applyProviderBatchPatch` never persists changes to the database**
This function performs validation, staleness checks, idempotency deduplication, and returns `{ ok: true, data: { updatedCount: providerIds.length, ... } }` — but never actually calls `updateProvidersBatch` (or any repository function) to write changes to the database. The `updatedCount` reported in the response is the number of providers that _should_ have been updated, not the actual count of affected rows.
Similarly, `undoProviderPatch` (line 1356) doesn't perform any database revert — it simply deletes the undo token from an in-memory `Map` and returns success.
Callers will receive a success response believing providers have been updated, but no actual mutation occurs.
How can I resolve this? If you propose a fix, please make it concise.| function getClientIp(request: NextRequest): string { | ||
| return ( | ||
| request.headers.get("x-forwarded-for")?.split(",")[0]?.trim() || | ||
| request.headers.get("x-real-ip")?.trim() || | ||
| "unknown" | ||
| ); | ||
| } |
There was a problem hiding this comment.
x-forwarded-for trusted without proxy validation
getClientIp reads the client IP from the x-forwarded-for header, which is trivially spoofable by any HTTP client unless there's a trusted reverse proxy that overwrites this header. Since there's no trusted proxy configuration in this codebase, an attacker can bypass the IP-based rate limiter by setting X-Forwarded-For: <random-ip> on each request.
Consider either:
- Documenting the assumption that a trusted reverse proxy is required for rate limiting to be effective, or
- Adding a configuration option for trusted proxy IPs and only reading
x-forwarded-forwhen the immediate connection IP is trusted
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 105-111
Comment:
**`x-forwarded-for` trusted without proxy validation**
`getClientIp` reads the client IP from the `x-forwarded-for` header, which is trivially spoofable by any HTTP client unless there's a trusted reverse proxy that overwrites this header. Since there's no trusted proxy configuration in this codebase, an attacker can bypass the IP-based rate limiter by setting `X-Forwarded-For: <random-ip>` on each request.
Consider either:
- Documenting the assumption that a trusted reverse proxy is required for rate limiting to be effective, or
- Adding a configuration option for trusted proxy IPs and only reading `x-forwarded-for` when the immediate connection IP is trusted
How can I resolve this? If you propose a fix, please make it concise.
src/app/api/auth/logout/route.ts
Outdated
| try { | ||
| const sessionId = await resolveAuthCookieToken(); | ||
| if (sessionId) { | ||
| const store = new RedisSessionStore(); |
There was a problem hiding this comment.
New RedisSessionStore instantiated per logout request
A new RedisSessionStore() is created on every logout call. The login route already addresses this with a module-level getLoginSessionStore() that caches the instance (lines 113-123 of login/route.ts). Consider using the same pattern here for consistency and to avoid unnecessary object allocation.
| const store = new RedisSessionStore(); | |
| const store = await getLoginSessionStore(); |
(This would require either extracting the cached store getter into a shared module or duplicating the pattern here.)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/logout/route.ts
Line: 49
Comment:
**New `RedisSessionStore` instantiated per logout request**
A new `RedisSessionStore()` is created on every logout call. The login route already addresses this with a module-level `getLoginSessionStore()` that caches the instance (lines 113-123 of `login/route.ts`). Consider using the same pattern here for consistency and to avoid unnecessary object allocation.
```suggestion
const store = await getLoginSessionStore();
```
(This would require either extracting the cached store getter into a shared module or duplicating the pattern here.)
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Fix TypeScript error where obj.userId was typed as 'unknown' despite validation check. Add safe type assertion to number. CI Run: https://github.com/ding113/claude-code-hub/actions/runs/22131418119 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| const originValue = request.headers.get("origin"); | ||
| const origin = originValue ? normalizeOrigin(originValue) : null; | ||
|
|
||
| if (!origin) { | ||
| if (fetchSite === "cross-site") { | ||
| return { | ||
| allowed: false, | ||
| reason: "Cross-site request blocked: missing Origin header", | ||
| }; | ||
| } | ||
|
|
||
| return { allowed: true }; | ||
| } | ||
|
|
||
| if (allowedOrigins.has(origin)) { | ||
| return { allowed: true }; | ||
| } | ||
|
|
||
| return { allowed: false, reason: `Origin ${origin} not in allowlist` }; |
There was a problem hiding this comment.
CSRF guard rejects same-origin requests behind header-stripping proxies
When a reverse proxy strips the Sec-Fetch-Site header but preserves the Origin header, the guard falls through to the allowedOrigins check at line 59. Since both the login and logout routes configure allowedOrigins: [] (empty), any request carrying an Origin header — including legitimate same-origin browser requests — will be rejected with "Origin not in allowlist".
This is distinct from the previously discussed scenario where both headers are missing. Here, the browser correctly sends Origin: https://myapp.com but the proxy drops Sec-Fetch-Site, and the guard has no way to recognize it as same-origin because it only checks sec-fetch-site for same-origin detection — it never compares the Origin header against the request's Host.
Consider adding a Host-vs-Origin comparison as a fallback when sec-fetch-site is absent:
const hostValue = request.headers.get("host");
if (origin && hostValue && allowSameOrigin) {
try {
const originHost = new URL(origin).host;
if (originHost === hostValue) {
return { allowed: true };
}
} catch { /* invalid origin URL, fall through */ }
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/csrf-origin-guard.ts
Line: 45-63
Comment:
**CSRF guard rejects same-origin requests behind header-stripping proxies**
When a reverse proxy strips the `Sec-Fetch-Site` header but preserves the `Origin` header, the guard falls through to the `allowedOrigins` check at line 59. Since both the login and logout routes configure `allowedOrigins: []` (empty), any request carrying an `Origin` header — including legitimate same-origin browser requests — will be rejected with "Origin not in allowlist".
This is distinct from the previously discussed scenario where both headers are missing. Here, the browser correctly sends `Origin: https://myapp.com` but the proxy drops `Sec-Fetch-Site`, and the guard has no way to recognize it as same-origin because it only checks `sec-fetch-site` for same-origin detection — it never compares the `Origin` header against the request's `Host`.
Consider adding a `Host`-vs-`Origin` comparison as a fallback when `sec-fetch-site` is absent:
```
const hostValue = request.headers.get("host");
if (origin && hostValue && allowSameOrigin) {
try {
const originHost = new URL(origin).host;
if (originHost === hostValue) {
return { allowed: true };
}
} catch { /* invalid origin URL, fall through */ }
}
```
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| } | ||
|
|
||
| loginPolicy.recordSuccess(clientIp); |
There was a problem hiding this comment.
Successful login resets IP failure counter, enabling brute-force bypass
loginPolicy.recordSuccess(clientIp) calls reset() which deletes the entire attempt record for this IP. An attacker with one valid key can:
- Send 9 invalid login attempts (below the threshold of 10)
- Send 1 valid login with their own key → counter resets to 0
- Repeat from step 1
This gives them unlimited brute-force attempts against other users' keys, as long as they possess a single valid key. Consider either:
- Not resetting on success (let the window expire naturally), or
- Only decrementing the counter by 1 on success rather than wiping it entirely
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 243
Comment:
**Successful login resets IP failure counter, enabling brute-force bypass**
`loginPolicy.recordSuccess(clientIp)` calls `reset()` which deletes the entire attempt record for this IP. An attacker with one valid key can:
1. Send 9 invalid login attempts (below the threshold of 10)
2. Send 1 valid login with their own key → counter resets to 0
3. Repeat from step 1
This gives them unlimited brute-force attempts against other users' keys, as long as they possess a single valid key. Consider either:
- Not resetting on success (let the window expire naturally), or
- Only decrementing the counter by 1 on success rather than wiping it entirely
How can I resolve this? If you propose a fix, please make it concise.| function hasSecureCookieHttpMismatch(request: NextRequest): boolean { | ||
| const env = getEnvConfig(); | ||
| const forwardedProto = request.headers.get("x-forwarded-proto")?.split(",")[0]?.trim(); | ||
| return env.ENABLE_SECURE_COOKIES && forwardedProto === "http"; | ||
| } | ||
|
|
||
| function shouldIncludeFailureTaxonomy(request: NextRequest): boolean { | ||
| return request.headers.has("x-forwarded-proto"); | ||
| } |
There was a problem hiding this comment.
shouldIncludeFailureTaxonomy heuristic may be incorrect
This function uses the presence of x-forwarded-proto as a proxy for determining whether to include error codes in the response. The comment/naming suggests this controls whether to include a detailed "failure taxonomy" (error codes like KEY_REQUIRED, KEY_INVALID, SERVER_ERROR) in the response body.
However, x-forwarded-proto is set by reverse proxies and has no semantic relationship to whether the client expects structured error codes. In practice, most production deployments behind a load balancer will always have this header, making the non-taxonomy branch (lines 170-176, 190-197, 271-273) unreachable in production. Conversely, direct connections during development won't have it, meaning error codes are hidden during testing.
If the intent is to always include error codes for programmatic clients, consider making this unconditional or keying it off a more appropriate signal like an Accept header or API version.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 95-103
Comment:
**`shouldIncludeFailureTaxonomy` heuristic may be incorrect**
This function uses the presence of `x-forwarded-proto` as a proxy for determining whether to include error codes in the response. The comment/naming suggests this controls whether to include a detailed "failure taxonomy" (error codes like `KEY_REQUIRED`, `KEY_INVALID`, `SERVER_ERROR`) in the response body.
However, `x-forwarded-proto` is set by reverse proxies and has no semantic relationship to whether the client expects structured error codes. In practice, most production deployments behind a load balancer will always have this header, making the non-taxonomy branch (lines 170-176, 190-197, 271-273) unreachable in production. Conversely, direct connections during development won't have it, meaning error codes are hidden during testing.
If the intent is to always include error codes for programmatic clients, consider making this unconditional or keying it off a more appropriate signal like an `Accept` header or API version.
How can I resolve this? If you propose a fix, please make it concise.
| const mode = getSessionTokenMode(); | ||
| if (mode === "legacy") { | ||
| await setAuthCookie(key); | ||
| } else if (mode === "dual") { | ||
| await setAuthCookie(key); | ||
| try { | ||
| await createOpaqueSession(key, session); | ||
| } catch (error) { | ||
| logger.warn("Failed to create opaque session in dual mode", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| } else { | ||
| try { | ||
| const opaqueSession = await createOpaqueSession(key, session); | ||
| await setAuthCookie(opaqueSession.sessionId); | ||
| } catch (error) { | ||
| logger.error("Failed to create opaque session in opaque mode", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| const serverError = t?.("serverError") ?? "Internal server error"; | ||
| return withAuthResponseHeaders( | ||
| NextResponse.json( | ||
| { error: serverError, errorCode: "SESSION_CREATE_FAILED" }, | ||
| { status: 503 } | ||
| ) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Dual-mode login doesn't store opaque session ID in cookie
In dual mode (line 222-230), the raw API key is stored in the cookie via setAuthCookie(key), and createOpaqueSession is called fire-and-forget (only logged on failure). However, the opaque session's sessionId (a sid_<uuid> string) is never stored anywhere accessible to the client.
This means:
- On subsequent requests,
validateAuthTokenreads the raw API key from the cookie - It tries
sessionStore.read(rawApiKey)which returnsnull(the session is stored undersid_<uuid>, not the raw key) - It falls through to
validateKey(token)which works — but the opaque session in Redis is never actually used in dual mode
The dual-mode opaque session becomes dead data in Redis. While this doesn't break login (because validateKey fallback works), it means dual mode never actually exercises the opaque session read path, defeating the purpose of a gradual migration validation step. When you flip to opaque mode, the session read path will be used for the first time in production — without the "dual" validation period catching any issues.
Consider storing the opaque session ID in the cookie when createOpaqueSession succeeds in dual mode, so that the opaque read path is exercised alongside the legacy fallback.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 219-247
Comment:
**Dual-mode login doesn't store opaque session ID in cookie**
In `dual` mode (line 222-230), the raw API key is stored in the cookie via `setAuthCookie(key)`, and `createOpaqueSession` is called fire-and-forget (only logged on failure). However, the opaque session's `sessionId` (a `sid_<uuid>` string) is never stored anywhere accessible to the client.
This means:
1. On subsequent requests, `validateAuthToken` reads the raw API key from the cookie
2. It tries `sessionStore.read(rawApiKey)` which returns `null` (the session is stored under `sid_<uuid>`, not the raw key)
3. It falls through to `validateKey(token)` which works — but the opaque session in Redis is never actually used in dual mode
The dual-mode opaque session becomes dead data in Redis. While this doesn't break login (because `validateKey` fallback works), it means dual mode never actually exercises the opaque session read path, defeating the purpose of a gradual migration validation step. When you flip to `opaque` mode, the session read path will be used for the first time in production — without the "dual" validation period catching any issues.
Consider storing the opaque session ID in the cookie when `createOpaqueSession` succeeds in dual mode, so that the opaque read path is exercised alongside the legacy fallback.
How can I resolve this? If you propose a fix, please make it concise.| if (allowedOrigins.has(origin)) { | ||
| return { allowed: true }; | ||
| } | ||
|
|
||
| return { allowed: false, reason: `Origin ${origin} not in allowlist` }; |
There was a problem hiding this comment.
CSRF guard rejects legitimate same-origin requests that include an Origin header but lack Sec-Fetch-Site
When a browser sends a same-origin POST with Origin: https://myapp.com but without Sec-Fetch-Site (e.g., due to proxy stripping, or older browser behavior), the guard reaches line 59. Since allowedOrigins is configured as [] (empty) for both login and logout routes, allowedOrigins.has(origin) is always false, and the request is blocked with "Origin not in allowlist".
The guard correctly handles: (1) sec-fetch-site: same-origin → allow, (2) no Origin + no fetch-site → allow, (3) Origin + cross-site fetch-site → block. But this specific case (Origin present, no Sec-Fetch-Site) has no same-origin detection.
Consider adding a Host-vs-Origin comparison as a fallback when sec-fetch-site is absent:
| if (allowedOrigins.has(origin)) { | |
| return { allowed: true }; | |
| } | |
| return { allowed: false, reason: `Origin ${origin} not in allowlist` }; | |
| if (allowedOrigins.has(origin)) { | |
| return { allowed: true }; | |
| } | |
| // Fallback: compare Origin against Host header for same-origin detection | |
| // when sec-fetch-site is absent (e.g., stripped by proxy) | |
| const hostValue = request.headers.get("host"); | |
| if (hostValue && !fetchSite) { | |
| const originHost = origin.replace(/^https?:\/\//, "").replace(/\/$/, ""); | |
| const normalizedHost = hostValue.trim().toLowerCase(); | |
| if (originHost === normalizedHost) { | |
| return { allowed: true }; | |
| } | |
| } | |
| return { allowed: false, reason: `Origin ${origin} not in allowlist` }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/csrf-origin-guard.ts
Line: 59-63
Comment:
**CSRF guard rejects legitimate same-origin requests that include an Origin header but lack Sec-Fetch-Site**
When a browser sends a same-origin POST with `Origin: https://myapp.com` but without `Sec-Fetch-Site` (e.g., due to proxy stripping, or older browser behavior), the guard reaches line 59. Since `allowedOrigins` is configured as `[]` (empty) for both login and logout routes, `allowedOrigins.has(origin)` is always `false`, and the request is blocked with "Origin not in allowlist".
The guard correctly handles: (1) `sec-fetch-site: same-origin` → allow, (2) no Origin + no fetch-site → allow, (3) Origin + cross-site fetch-site → block. But this specific case (Origin present, no Sec-Fetch-Site) has no same-origin detection.
Consider adding a `Host`-vs-`Origin` comparison as a fallback when `sec-fetch-site` is absent:
```suggestion
if (allowedOrigins.has(origin)) {
return { allowed: true };
}
// Fallback: compare Origin against Host header for same-origin detection
// when sec-fetch-site is absent (e.g., stripped by proxy)
const hostValue = request.headers.get("host");
if (hostValue && !fetchSite) {
const originHost = origin.replace(/^https?:\/\//, "").replace(/\/$/, "");
const normalizedHost = hostValue.trim().toLowerCase();
if (originHost === normalizedHost) {
return { allowed: true };
}
}
return { allowed: false, reason: `Origin ${origin} not in allowlist` };
```
How can I resolve this? If you propose a fix, please make it concise.| rotate(oldSessionId: string): Promise<SessionData | null>; | ||
| } | ||
|
|
||
| export const DEFAULT_SESSION_TTL = 86400; |
There was a problem hiding this comment.
DEFAULT_SESSION_TTL (86400s) disagrees with SESSION_TTL env default (300s)
DEFAULT_SESSION_TTL is hardcoded to 86400 seconds (1 day), but env.schema.ts:97 defaults SESSION_TTL to 300 seconds. The RedisSessionStore constructor uses DEFAULT_SESSION_TTL from this file as its default, meaning unless defaultTtlSeconds is explicitly passed, sessions are created with a 24-hour TTL — not the 5-minute TTL the env config suggests.
Neither the login route nor getSessionStore() in auth.ts passes SESSION_TTL from the env config to the store constructor. This means the env variable SESSION_TTL=300 has no effect on actual session duration in Redis.
| export const DEFAULT_SESSION_TTL = 86400; | |
| export const DEFAULT_SESSION_TTL = 86400; // TODO: read from getEnvConfig().SESSION_TTL |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/auth-session-store/index.ts
Line: 20
Comment:
**`DEFAULT_SESSION_TTL` (86400s) disagrees with `SESSION_TTL` env default (300s)**
`DEFAULT_SESSION_TTL` is hardcoded to 86400 seconds (1 day), but `env.schema.ts:97` defaults `SESSION_TTL` to 300 seconds. The `RedisSessionStore` constructor uses `DEFAULT_SESSION_TTL` from this file as its default, meaning unless `defaultTtlSeconds` is explicitly passed, sessions are created with a 24-hour TTL — not the 5-minute TTL the env config suggests.
Neither the login route nor `getSessionStore()` in `auth.ts` passes `SESSION_TTL` from the env config to the store constructor. This means the env variable `SESSION_TTL=300` has no effect on actual session duration in Redis.
```suggestion
export const DEFAULT_SESSION_TTL = 86400; // TODO: read from getEnvConfig().SESSION_TTL
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
The user is effectively logged out after 5 minutes despite the cookie being present. This creates a confusing UX where the user has a valid cookie but is always sent to login. Consider either aligning the cookie Prompt To Fix With AIThis is a comment left during a code review.
Path: src/lib/auth.ts
Line: 240-250
Comment:
**Cookie max-age (7 days) vastly exceeds session TTL (300s default)**
`AUTH_COOKIE_MAX_AGE` is hardcoded to 7 days, but the `SESSION_TTL` env var defaults to 300 seconds. In opaque mode, the Redis session will expire after 5 minutes, while the cookie persists for 7 days. For the remaining ~6 days 23 hours 55 minutes, every authenticated request will:
1. Read the `sid_<uuid>` from the cookie
2. Try `sessionStore.read()` → `null` (Redis key expired)
3. Return `null` (in opaque mode, no fallback to `validateKey`)
4. Redirect to login
The user is effectively logged out after 5 minutes despite the cookie being present. This creates a confusing UX where the user has a valid cookie but is always sent to login.
Consider either aligning the cookie `maxAge` with the session TTL (e.g., `getEnvConfig().SESSION_TTL`), or making the cookie max-age configurable alongside `SESSION_TTL`.
How can I resolve this? If you propose a fix, please make it concise.
The proxy handler previously called While the comment explains this avoids a "death loop" from transient validation failures, this change means:
The approach is reasonable for resilience, but it relies heavily on every downstream layout performing full validation. Is there a pattern or shared wrapper ensuring all protected layouts validate the session? Without that, this creates a potential for auth bypass on routes that forget to check. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/proxy.ts
Line: 56-78
Comment:
**Middleware auth check reduced to cookie-existence only — stale sessions no longer rejected at the edge**
The proxy handler previously called `validateKey(authToken.value)` to verify the key's validity, clearing the cookie on failure. Now it only checks whether the `auth-token` cookie exists (line 65) and passes through to downstream layouts for full validation.
While the comment explains this avoids a "death loop" from transient validation failures, this change means:
1. Expired or disabled users with stale cookies can navigate through the middleware to every protected route
2. The downstream layout must now handle all auth validation, including the redirect-to-login flow
3. If any protected route's layout doesn't call `getSession()` or `validateSession()`, it becomes unauthenticated
The approach is reasonable for resilience, but it relies heavily on every downstream layout performing full validation. Is there a pattern or shared wrapper ensuring all protected layouts validate the session? Without that, this creates a potential for auth bypass on routes that forget to check.
How can I resolve this? If you propose a fix, please make it concise. |
Removed duplicate/orphan test code that was outside of any describe block, causing parse errors during format check. CI Run: https://github.com/ding113/claude-code-hub/actions/runs/22132400374 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| onConfigChange={(newConfig) => { | ||
| dispatch({ | ||
| type: "SET_ADAPTIVE_THINKING_EFFORT", | ||
| payload: newConfig.effort, | ||
| }); | ||
| dispatch({ | ||
| type: "SET_ADAPTIVE_THINKING_MODEL_MATCH_MODE", | ||
| payload: newConfig.modelMatchMode, | ||
| }); | ||
| dispatch({ | ||
| type: "SET_ADAPTIVE_THINKING_MODELS", | ||
| payload: newConfig.models, | ||
| }); | ||
| }} |
There was a problem hiding this comment.
Triple dispatch causes intermediate renders
The onConfigChange callback dispatches three separate actions sequentially (SET_ADAPTIVE_THINKING_EFFORT, SET_ADAPTIVE_THINKING_MODEL_MATCH_MODE, SET_ADAPTIVE_THINKING_MODELS). While React 18 batches state updates in event handlers, these dispatches can still trigger intermediate state transitions in the reducer — e.g., the effort changes while modelMatchMode still reflects the old value.
Consider either:
- Adding a single
SET_ADAPTIVE_THINKING_CONFIGreducer action that updates the entire config atomically, or - Passing a unified update object through one dispatch call
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Line: 643-656
Comment:
**Triple dispatch causes intermediate renders**
The `onConfigChange` callback dispatches three separate actions sequentially (`SET_ADAPTIVE_THINKING_EFFORT`, `SET_ADAPTIVE_THINKING_MODEL_MATCH_MODE`, `SET_ADAPTIVE_THINKING_MODELS`). While React 18 batches state updates in event handlers, these dispatches can still trigger intermediate state transitions in the reducer — e.g., the effort changes while `modelMatchMode` still reflects the old value.
Consider either:
- Adding a single `SET_ADAPTIVE_THINKING_CONFIG` reducer action that updates the entire config atomically, or
- Passing a unified update object through one dispatch call
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| setStatus("success"); | ||
| const loginType = parseLoginType(data.loginType); | ||
| const fallbackPath = loginType ? getLoginTypeFallbackPath(loginType) : from; | ||
| const redirectTarget = resolveLoginRedirectTarget(data.redirectTo, fallbackPath); | ||
| router.push(redirectTarget); | ||
| router.refresh(); |
There was a problem hiding this comment.
Loading overlay persists indefinitely if navigation fails
When login succeeds, setStatus("success") is called and then router.push() + router.refresh() follow. The isLoading state (which shows the fullscreen overlay) is true when status === "success", but there's no mechanism to reset it if router.push() silently fails or the navigation takes too long (e.g., the redirect target returns an error).
Unlike the old code that used finally { setLoading(false) }, the "success" status is never cleared — so the user could be stuck with a spinner overlay with no way to interact with the page. Consider adding a timeout that resets status if the page hasn't navigated within a reasonable window.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/login/page.tsx
Line: 191-196
Comment:
**Loading overlay persists indefinitely if navigation fails**
When login succeeds, `setStatus("success")` is called and then `router.push()` + `router.refresh()` follow. The `isLoading` state (which shows the fullscreen overlay) is `true` when `status === "success"`, but there's no mechanism to reset it if `router.push()` silently fails or the navigation takes too long (e.g., the redirect target returns an error).
Unlike the old code that used `finally { setLoading(false) }`, the "success" status is never cleared — so the user could be stuck with a spinner overlay with no way to interact with the page. Consider adding a timeout that resets status if the page hasn't navigated within a reasonable window.
How can I resolve this? If you propose a fix, please make it concise.| const decision = loginPolicy.check(clientIp); | ||
| if (!decision.allowed) { | ||
| const response = withAuthResponseHeaders( | ||
| NextResponse.json( | ||
| { | ||
| error: t?.("loginFailed") ?? t?.("serverError") ?? "Too many attempts", | ||
| errorCode: "RATE_LIMITED", | ||
| }, | ||
| { status: 429 } | ||
| ) | ||
| ); | ||
|
|
||
| if (decision.retryAfterSeconds != null) { | ||
| response.headers.set("Retry-After", String(decision.retryAfterSeconds)); | ||
| } | ||
|
|
||
| return response; | ||
| } |
There was a problem hiding this comment.
Rate-limited response leaks error translation key semantics
When the rate limiter blocks a request, the error message falls through to t?.("loginFailed") ?? t?.("serverError") ?? "Too many attempts". The "loginFailed" translation ("Login failed") is misleading for a rate-limited request — the user didn't attempt to log in yet. The 429 status code is correct, but the error message should clearly convey that the user is being rate-limited, not that their credentials are wrong.
Consider using a dedicated translation key (e.g., "tooManyAttempts") as the primary message for 429 responses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 146-163
Comment:
**Rate-limited response leaks error translation key semantics**
When the rate limiter blocks a request, the error message falls through to `t?.("loginFailed") ?? t?.("serverError") ?? "Too many attempts"`. The `"loginFailed"` translation ("Login failed") is misleading for a rate-limited request — the user didn't attempt to log in yet. The `429` status code is correct, but the error message should clearly convey that the user is being rate-limited, not that their credentials are wrong.
Consider using a dedicated translation key (e.g., `"tooManyAttempts"`) as the primary message for 429 responses.
How can I resolve this? If you propose a fix, please make it concise.| useEffect(() => { | ||
| let active = true; | ||
|
|
||
| void fetch("/api/version") | ||
| .then((response) => response.json() as Promise<{ current?: unknown; hasUpdate?: unknown }>) | ||
| .then((data) => { | ||
| if (!active || typeof data.current !== "string") { | ||
| return; | ||
| } | ||
|
|
||
| setVersionInfo({ | ||
| current: data.current, | ||
| hasUpdate: Boolean(data.hasUpdate), | ||
| }); | ||
| }) | ||
| .catch(() => {}); | ||
|
|
||
| return () => { | ||
| active = false; | ||
| }; | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| let active = true; | ||
|
|
There was a problem hiding this comment.
Unauthenticated fetch to /api/version and /api/system-settings on every login page load
The login page makes two unauthenticated fetch calls on mount to /api/version and /api/system-settings. If these endpoints are protected (returning 401/403), the calls silently fail via .catch(() => {}). If they're public, they expose system metadata (version info including hasUpdate, site title) to unauthenticated users.
Verify that these endpoints are intentionally public and that exposing version/update status to unauthenticated users is acceptable from a security standpoint (version disclosure can assist targeted attacks).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/login/page.tsx
Line: 121-145
Comment:
**Unauthenticated fetch to `/api/version` and `/api/system-settings` on every login page load**
The login page makes two unauthenticated `fetch` calls on mount to `/api/version` and `/api/system-settings`. If these endpoints are protected (returning 401/403), the calls silently fail via `.catch(() => {})`. If they're public, they expose system metadata (version info including `hasUpdate`, site title) to unauthenticated users.
Verify that these endpoints are intentionally public and that exposing version/update status to unauthenticated users is acceptable from a security standpoint (version disclosure can assist targeted attacks).
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
Both If these are intended as future extension points, consider adding a comment explaining the intent. Otherwise, consumers may call these expecting specific dual-read or session-validation behavior that doesn't exist, potentially masking bugs during the migration to opaque tokens. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/lib/auth.ts
Line: 386-400
Comment:
**`getSessionWithDualRead` and `validateSession` are redundant pass-through wrappers**
Both `getSessionWithDualRead` and `validateSession` simply delegate to `getSession` with no additional logic. This creates API surface that suggests different behavior (the names imply dual-read semantics or session-specific validation) but actually does nothing beyond what `getSession` already provides.
If these are intended as future extension points, consider adding a comment explaining the intent. Otherwise, consumers may call these expecting specific dual-read or session-validation behavior that doesn't exist, potentially masking bugs during the migration to opaque tokens.
How can I resolve this? If you propose a fix, please make it concise.
This is currently safe because the Prompt To Fix With AIThis is a comment left during a code review.
Path: src/proxy.ts
Line: 51-53
Comment:
**`/usage-doc` prefix match bypasses auth for all sub-paths**
`PUBLIC_PATH_PATTERNS` uses `pathWithoutLocale.startsWith(pattern)` for matching, and `/usage-doc` is in the public list. This means any path starting with `/usage-doc` — including hypothetical future routes like `/usage-doc-admin` — would be treated as public.
This is currently safe because the `/usage-doc` layout already checks `getSession()` optionally and the page itself only shows data based on the auth context. However, if a future sub-route under `/usage-doc/` requires authentication, it would be inadvertently bypassed at the proxy level. Consider using exact path or trailing-slash-aware matching (e.g., `pattern + "/"`) for more precise public path detection.
How can I resolve this? If you propose a fix, please make it concise. |
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Complete security hardening of the authentication and session management system, implemented across 7 progressive waves:
Key Changes
Stats
Test plan
bun run test- all unit/integration tests passbun run typecheck- no type errorsbun run lint- no lint violationsbun run build- production build succeedsGreptile Summary
This PR implements a comprehensive security overhaul of the authentication and session management system across 7 progressive waves. Key changes include Redis-backed opaque session storage, dual-read auth for zero-downtime migration, CSRF origin guard, anti-bruteforce rate limiting, security headers middleware, and a login page UI redesign. The PR also introduces provider batch patch operations (preview/apply/undo) and refactors several provider settings components.
RedisSessionStorewith create/read/revoke/rotate operations, opaque session IDs (sid_prefix), and application-level expiry checks. ThevalidateAuthTokenfunction inauth.tssupports legacy, dual, and opaque modes.getSession()checks.applyProviderBatchPatchandundoProviderPatchserver actions have validation, staleness checks, and idempotency — but critically do not persist changes to the database (previously flagged).Confidence Score: 2/5
src/app/api/auth/login/route.ts(rate limiter integration gaps, brute-force bypass),src/app/api/auth/logout/route.ts(dual-mode revocation),src/actions/providers.ts(missing DB persistence in batch patch), andsrc/lib/config/env.schema.ts(default opaque mode).Important Files Changed
sid_prefixed Redis key, leaving opaque sessions alive until TTL expiry.getSessionWithDualReadandvalidateSessionare pass-through wrappers.sid_prefix. Store uses DEFAULT_SESSION_TTL (86400s) which disagrees with env SESSION_TTL (300s). Rotation creates new session before revoking old one (safe order).unsafe-inlineandunsafe-evalfor script-src (likely needed for Next.js).applyProviderBatchPatchnever calls any repository function to persist changes to database — returns success without actually updating providers.undoProviderPatchsimilarly doesn't perform any database revert.validateAuthToken(supports opaque sessions) instead ofvalidateKey, and uses exportedAUTH_COOKIE_NAMEconstant instead of hardcoded string. Clean migration.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Browser POST /login] --> B{CSRF Guard} B -->|blocked| C[403 CSRF Rejected] B -->|allowed| D{Rate Limiter} D -->|blocked| E[429 Rate Limited] D -->|allowed| F[Parse request body] F --> G{Validate credential} G -->|invalid| H[Record failure + 401] G -->|valid| I{Session Token Mode} I -->|legacy| J[Set cookie: raw value] I -->|dual| K[Set cookie: raw value] K --> L[Create Redis session fire-and-forget] I -->|opaque| M{Create Redis session} M -->|success| N[Set cookie: session ID] M -->|failure| O[503 Session Create Failed] J --> P[200 OK + redirect] N --> P L --> P Q[Browser request to protected route] --> R{Proxy: cookie exists?} R -->|no| S[Redirect to login] R -->|yes| T[Pass to layout] T --> U{Layout: getSession} U -->|opaque/dual| V[Redis read + DB fingerprint match] U -->|legacy/dual fallback| W[Direct DB validation] V --> X{Valid?} W --> X X -->|yes| Y[Render page] X -->|no| SLast reviewed commit: 902b4e3