feat: security auth overhaul and provider batch operations#806
Merged
Conversation
📝 WalkthroughWalkthrough大规模引入提供商批量编辑的预览/应用/撤销流水线、撤销快照存储与类型；重构登录页与重定向安全；实现会话令牌模式及 Redis 会话存储；添加多项安全工具与大量 i18n 文本更新，并扩展单元/集成测试覆盖面（安全、批量补丁、UI）。 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 |
Contributor
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 enhances the application's security posture by implementing advanced authentication mechanisms and robust session management. Concurrently, it introduces powerful batch editing capabilities for providers, improving administrative efficiency and data integrity with a new undo system. The user experience is also refined through a redesigned login interface, ensuring a more secure, intuitive, and manageable platform. 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
enhancement
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+118
to
+129
| // 3. x-forwarded-for: take the rightmost (last) entry, which is the IP | ||
| // appended by the closest trusted proxy. The leftmost entry is | ||
| // client-controlled and can be spoofed. | ||
| const forwarded = request.headers.get("x-forwarded-for"); | ||
| if (forwarded) { | ||
| const ips = forwarded | ||
| .split(",") | ||
| .map((s) => s.trim()) | ||
| .filter(Boolean); | ||
| if (ips.length > 0) { | ||
| return ips[ips.length - 1]; | ||
| } |
There was a problem hiding this comment.
rightmost x-forwarded-for IP can be spoofed if intermediary proxies append IPs
The comment says "rightmost (last) entry, which is the IP appended by the closest trusted proxy" but this isn't always true. If an attacker controls a proxy in the chain, they can append arbitrary IPs. The rightmost IP is only trustworthy if you control all proxies in the chain.
For bruteforce protection, this could allow an attacker to bypass rate limits by rotating the spoofed IP in x-forwarded-for.
Consider:
- Document the deployment assumption (all proxies in chain are trusted)
- Or use a configurable "trusted proxy depth" approach
- Or validate that the deployment only has one trusted proxy layer
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 118-129
Comment:
rightmost `x-forwarded-for` IP can be spoofed if intermediary proxies append IPs
The comment says "rightmost (last) entry, which is the IP appended by the closest trusted proxy" but this isn't always true. If an attacker controls a proxy in the chain, they can append arbitrary IPs. The rightmost IP is only trustworthy if you control all proxies in the chain.
For bruteforce protection, this could allow an attacker to bypass rate limits by rotating the spoofed IP in `x-forwarded-for`.
Consider:
- Document the deployment assumption (all proxies in chain are trusted)
- Or use a configurable "trusted proxy depth" approach
- Or validate that the deployment only has one trusted proxy layer
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
208
to
+210
| const session = await validateKey(key, { allowReadOnlyAccess: true }); | ||
| if (!session) { | ||
| return NextResponse.json({ error: t?.("apiKeyInvalidOrExpired") }, { status: 401 }); | ||
| loginPolicy.recordFailure(clientIp); |
There was a problem hiding this comment.
rate limit failure not recorded for missing/invalid key
When the key is missing or malformed (line 190-206), login failure is not recorded with loginPolicy.recordFailure(clientIp). This creates a gap where an attacker can spam requests with empty/invalid keys without triggering bruteforce protection.
Suggested change
| const session = await validateKey(key, { allowReadOnlyAccess: true }); | |
| if (!session) { | |
| return NextResponse.json({ error: t?.("apiKeyInvalidOrExpired") }, { status: 401 }); | |
| loginPolicy.recordFailure(clientIp); | |
| const session = await validateKey(key, { allowReadOnlyAccess: true }); | |
| if (!session) { | |
| loginPolicy.recordFailure(clientIp, key); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 208-210
Comment:
rate limit failure not recorded for missing/invalid key
When the key is missing or malformed (line 190-206), login failure is not recorded with `loginPolicy.recordFailure(clientIp)`. This creates a gap where an attacker can spam requests with empty/invalid keys without triggering bruteforce protection.
```suggestion
const session = await validateKey(key, { allowReadOnlyAccess: true });
if (!session) {
loginPolicy.recordFailure(clientIp, key);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+27
to
+28
| const MAX_TRACKED_ENTRIES = 10_000; | ||
| const SWEEP_INTERVAL_MS = 60_000; |
There was a problem hiding this comment.
10k entry limit can be exhausted by attacker distributing across IPs
MAX_TRACKED_ENTRIES = 10_000 with FIFO eviction allows an attacker with a large IP pool to evade tracking by filling the map with 10k distinct IPs, causing older entries to be deleted before lockout expires.
Consider:
- LRU eviction instead of FIFO (keeps active attackers tracked longer)
- Redis-backed store for production scale
- Lower the max entries and rely more on lockout duration
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/login-abuse-policy.ts
Line: 27-28
Comment:
10k entry limit can be exhausted by attacker distributing across IPs
`MAX_TRACKED_ENTRIES = 10_000` with FIFO eviction allows an attacker with a large IP pool to evade tracking by filling the map with 10k distinct IPs, causing older entries to be deleted before lockout expires.
Consider:
- LRU eviction instead of FIFO (keeps active attackers tracked longer)
- Redis-backed store for production scale
- Lower the max entries and rely more on lockout duration
How can I resolve this? If you propose a fix, please make it concise.
src/lib/providers/undo-store.ts
Outdated
|
|
||
| import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "@/lib/provider-batch-patch-error-codes"; | ||
|
|
||
| const UNDO_SNAPSHOT_TTL_MS = 10_000; |
There was a problem hiding this comment.
10-second undo window may be too short for batch operations
UNDO_SNAPSHOT_TTL_MS = 10_000 (10 seconds) may be too short for users to realize they need to undo a batch operation, especially if they navigate away or review changes. Production deployments may need 30-60 seconds.
Consider making this configurable via environment variable with a reasonable default.
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/providers/undo-store.ts
Line: 5
Comment:
10-second undo window may be too short for batch operations
`UNDO_SNAPSHOT_TTL_MS = 10_000` (10 seconds) may be too short for users to realize they need to undo a batch operation, especially if they navigate away or review changes. Production deployments may need 30-60 seconds.
Consider making this configurable via environment variable with a reasonable default.
<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.
Comment on lines
+273
to
+279
| if (sessionData) { | ||
| if (sessionData.expiresAt <= Date.now()) { | ||
| logger.warn("Opaque session expired (application-level check)", { | ||
| sessionId: sessionData.sessionId, | ||
| expiresAt: sessionData.expiresAt, | ||
| }); | ||
| return null; |
There was a problem hiding this comment.
application-level expiry check after Redis read provides defense-in-depth
Session expiry checked both at application level and by Redis TTL. Ensure Redis TTL is always ≤ the expiresAt duration to prevent time windows where expired sessions could be read before TTL triggers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/auth.ts
Line: 273-279
Comment:
application-level expiry check after Redis read provides defense-in-depth
Session expiry checked both at application level and by Redis TTL. Ensure Redis TTL is always ≤ the `expiresAt` duration to prevent time windows where expired sessions could be read before TTL triggers.
How can I resolve this? If you propose a fix, please make it concise.![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is a substantial pull request that introduces a major security overhaul and a new provider batch operations feature. The changes are extensive and well-implemented. The security enhancements, including opaque session tokens, CSRF protection, and anti-brute-force measures, are particularly valuable and well-executed. The new batch editing and undo functionality significantly improves the user experience for managing providers.
My review focuses on two key areas for improvement: a critical architectural concern regarding the scalability of the new undo/preview feature's storage mechanism, and a medium-severity performance optimization for session validation. Addressing these points will ensure the new features are robust and performant at scale.
src/actions/providers.ts
Outdated
Comment on lines
+1249
to
+1251
| const providerBatchPatchPreviewStore = new Map<string, ProviderBatchPatchPreviewSnapshot>(); | ||
| const providerPatchUndoStore = new Map<string, ProviderPatchUndoSnapshot>(); | ||
| const providerDeleteUndoStore = new Map<string, ProviderDeleteUndoSnapshot>(); |
Contributor
There was a problem hiding this comment.
The use of in-memory Map objects for providerBatchPatchPreviewStore, providerPatchUndoStore, and providerDeleteUndoStore introduces a significant scalability and reliability issue. These stores will not be shared across multiple server instances or serverless function invocations. This means a user's request for preview, apply, or undo might be routed to a different instance that does not have the required snapshot, leading to feature failure. For these features to work reliably in a distributed environment, a shared store like Redis should be used, similar to how the session store is implemented.
Comment on lines
+371
to
+378
| const keyList = await findKeyList(sessionData.userId); | ||
|
|
||
| for (const key of keyList) { | ||
| const keyFingerprint = await toKeyFingerprint(key.key); | ||
| if (constantTimeEqual(keyFingerprint, expectedFingerprint)) { | ||
| return validateKey(key.key, options); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The current implementation of convertToAuthSession fetches all keys for a given userId and then iterates through them, hashing each one to find a match against the session's keyFingerprint. This can lead to performance degradation for users with a large number of API keys, as it involves multiple hashing operations on every authenticated request. A more efficient approach would be to either store the keyId in the session data to allow for a direct lookup, or to add an indexed key_fingerprint column to the keys table in the database for a direct query.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/api/action-adapter-auth-session.unit.test.ts (1)
4-15:⚠️ Potential issue | 🟡 Minor注释中的函数名已过时，应更新为
validateAuthToken。第 8 行注释仍写的是
validateKey，与本次更改后验证validateAuthToken的实际测试行为不符，会误导后续维护者。✏️ 建议修改
- * - adapter 层使用 hono 读取 Cookie/Authorization 并 validateKey + * - adapter 层使用 hono 读取 Cookie/Authorization 并 validateAuthToken🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/action-adapter-auth-session.unit.test.ts` around lines 4 - 15, Update the outdated function name in the test comment: replace the reference to validateKey with the current validateAuthToken so the comment matches the test behavior; locate the comment block in tests/api/action-adapter-auth-session.unit.test.ts that describes adapter auth and change the mention of validateKey to validateAuthToken to avoid confusion for future maintainers.
🟡 Minor comments (25)
src/app/v1/_lib/cors.ts-29-32 (1)
29-32:⚠️ Potential issue | 🟡 Minor重新审视 Origin 反射的安全设计
在
buildCorsHeaders函数中，当allowCredentials && options.origin同时为真时，代码会反射任意请求 Origin 并设置Access-Control-Allow-Credentials: true。虽然注释已说明这一风险，但函数本身不进行任何 Origin 校验，而是将所有安全责任推给调用方。关键发现：
- 生产环境的 CORS 中间件
registerCors并未传入allowCredentials标志，因此当前未暴露此风险- 但
applyCors和buildPreflightResponse作为导出函数，其 API 签名允许调用方传入该标志而无任何防护- 这是一个API 设计缺陷——如果有人直接调用这些函数并传入
allowCredentials: true，就会面临任意 Origin 反射的风险建议：要么在函数内添加 Origin 白名单校验（需增加参数），要么不公开
allowCredentials选项，确保通过中间件严格控制 CORS 行为。当前的做法虽然生产安全，但 API 设计本身不够健壮。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/cors.ts` around lines 29 - 32, The buildCorsHeaders function currently reflects any request Origin when allowCredentials && options.origin is true without validating it; update the API to prevent unsafe Origin reflection by either removing/privatizing the allowCredentials option from public exports (applyCors, buildPreflightResponse) or adding an explicit origin whitelist check inside buildCorsHeaders that validates options.origin against a supplied allowlist before setting Access-Control-Allow-Origin and Access-Control-Allow-Credentials. Specifically: modify the signatures of buildCorsHeaders/applyCors/buildPreflightResponse to accept a validatedOrigins list (or stop accepting allowCredentials from callers), enforce whitelist validation inside buildCorsHeaders, and ensure registerCors continues to call into the safe path so callers cannot enable reflection by passing allowCredentials: true directly.messages/ja/auth.json-25-27 (1)
25-27:⚠️ Potential issue | 🟡 Minor日语
brand.tagline的用词与其他语言存在细微差异。
"統合API管理コンソール"（统合 = 整合/合并）与其他语言环境的含义（zh-CN: "统一"，en: "Unified"）略有出入。建议将"統合"统一改为"統一"以保持跨语言语义一致性。✏️ 建议修改
- "tagline": "統合API管理コンソール" + "tagline": "統一API管理コンソール"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@messages/ja/auth.json` around lines 25 - 27, Change the Japanese translation for the brand tagline to match the same semantic meaning used in other locales by replacing "統合" with "統一" in the JSON key "brand.tagline" (messages/ja/auth.json) so the value becomes "統一API管理コンソール".src/app/api/auth/logout/route.ts-15-15 (1)
15-15:⚠️ Potential issue | 🟡 Minor将
process.env.VITEST检测嵌入生产代码中是反模式
enforceInDevelopment: process.env.VITEST === "true"使生产路由模块感知测试框架，违背了关注点分离原则。若该配置行为需要按环境区分，应使用专用的环境变量（如ENFORCE_CSRF_IN_DEV）或在测试 setup 文件中注入配置，而非将测试基础设施标识符硬编码到业务代码。建议修改
const csrfGuard = createCsrfOriginGuard({ allowedOrigins: [], allowSameOrigin: true, - enforceInDevelopment: process.env.VITEST === "true", + enforceInDevelopment: process.env.ENFORCE_CSRF_IN_DEV === "true", });🤖 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` at line 15, The route currently uses process.env.VITEST to set enforceInDevelopment which leaks test infra into production; change the check in the logout route's configuration (the enforceInDevelopment assignment) to read a purpose-specific environment variable (e.g. ENFORCE_CSRF_IN_DEV) or use an injected config value from test setup instead of process.env.VITEST, and update any test setup to set that env/config when running tests so production code no longer depends on VITEST.src/app/api/auth/logout/route.ts-30-44 (1)
30-44:⚠️ Potential issue | 🟡 Minor
resolveSessionTokenMode静默吞掉异常，可能遮蔽配置错误并跳过会话撤销
resolveSessionTokenMode捕获所有异常但不记录任何日志，默默回退到"legacy"模式。若getSessionTokenMode()因环境配置错误而抛出，这将导致会话撤销（第 56–68 行的mode !== "legacy"分支）被静默跳过，Redis 中的会话可能无法被正确清除。resolveAuthCookieToken同样存在相同问题。建议至少在回退时记录一条警告日志。建议修改
function resolveSessionTokenMode(): SessionTokenMode { try { return getSessionTokenMode(); - } catch { + } catch (err) { + logger.warn("[AuthLogout] Failed to resolve session token mode, defaulting to legacy", { + error: err instanceof Error ? err.message : String(err), + }); return "legacy"; } } async function resolveAuthCookieToken(): Promise<string | undefined> { try { return await getAuthCookie(); - } catch { + } catch (err) { + logger.warn("[AuthLogout] Failed to read auth cookie", { + error: err instanceof Error ? err.message : String(err), + }); return undefined; } }🤖 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 30 - 44, Both resolveSessionTokenMode and resolveAuthCookieToken currently swallow all exceptions and silently return fallbacks, which can hide configuration errors and prevent proper session revocation; update each function to catch errors but log a warning including the caught error details and context (e.g., "Failed to get session token mode, falling back to 'legacy'" and "Failed to read auth cookie, treating as undefined") before returning the existing fallback values; reference resolveSessionTokenMode -> getSessionTokenMode and resolveAuthCookieToken -> getAuthCookie so you add processLogger.warn (or the existing logger used in this module) with the error object/string when handling the catch and then return the same fallback as before.tests/security/session-store.test.ts-93-98 (1)
93-98:⚠️ Potential issue | 🟡 Minor应导出
buildSessionKey函数供测试使用，避免 Redis key 格式硬编码导致的耦合问题测试在多个位置（第 93、125、131、145、155 等行）直接使用硬编码的
"cch:session:"前缀。虽然当前前缀值与SESSION_KEY_PREFIX常量一致，但测试无法使用buildSessionKey函数（该函数未导出），导致测试与实现细节耦合。若SESSION_KEY_PREFIX变更，tests 使用硬编码前缀仍可能误判通过。建议将buildSessionKey导出，让测试直接使用该函数构造 key，保持一致性。🤖 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 93 - 98, Export the existing buildSessionKey function (which composes keys using SESSION_KEY_PREFIX) from the module that defines RedisSessionStore so tests can use it instead of hard-coding "cch:session:"; change its declaration to a named export (or add a named export wrapper) and update the tests to call buildSessionKey(session.sessionId) where they currently assemble the literal prefix, ensuring Redis key construction stays in sync with SESSION_KEY_PREFIX and rerun tests.tests/security/auth-dual-read.test.ts-197-225 (1)
197-225:⚠️ Potential issue | 🟡 Minor修复会话时间戳单位不匹配
会话数据的时间戳使用 10 位数值（秒级，
createdAt: 1_700_000_000，expiresAt: 1_700_000_600），而auth.ts第 274 行的验证逻辑与Date.now()（毫秒级）进行直接比较：if (sessionData.expiresAt <= Date.now())。这导致1_700_000_600 <= 1739904000000判定为真，会话被标记为已过期，测试将返回null而非预期的authResult。应改用
Date.now()相对时间戳以保持单位一致，与tests/security/session-store.test.ts中的其他测试对齐：建议的修复
mockReadSession.mockResolvedValue({ sessionId: "sid_opaque_found", keyFingerprint: toFingerprint(keyString), userId: 21, userRole: "user", - createdAt: 1_700_000_000, - expiresAt: 1_700_000_600, + createdAt: Date.now(), + expiresAt: Date.now() + 3_600_000, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/auth-dual-read.test.ts` around lines 197 - 225, The session expiry check compares sessionData.expiresAt (seconds) against Date.now() (milliseconds) causing valid sessions to be seen as expired; update the expiry/comparison in the auth validation (where sessionData.expiresAt is compared to Date.now()) to use consistent units—either convert Date.now() to seconds (e.g. Math.floor(Date.now()/1000)) or multiply sessionData.expiresAt by 1000 before comparing—so that the check (sessionData.expiresAt <= now) uses the same time unit and the test using createdAt/expiresAt as 10‑digit seconds timestamps passes.src/repository/provider.ts-972-972 (1)
972-972:⚠️ Potential issue | 🟡 Minor
anthropicAdaptiveThinking使用宽泛的object类型，缺乏运行时类型安全。
BatchProviderUpdates中anthropicAdaptiveThinking定义为object | null，而Provider接口和ProviderBatchApplyUpdates中使用的是AnthropicAdaptiveThinkingConfig | null。使用object允许任意形状的对象直接写入数据库，可能导致无效数据。建议使用具体类型
+import type { AnthropicAdaptiveThinkingConfig } from "@/types/provider"; + export interface BatchProviderUpdates { // ... - anthropicAdaptiveThinking?: object | null; + anthropicAdaptiveThinking?: AnthropicAdaptiveThinkingConfig | null; // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/provider.ts` at line 972, The BatchProviderUpdates type uses a too-broad anthropicAdaptiveThinking?: object | null which allows arbitrary shapes; change that property to use the concrete type AnthropicAdaptiveThinkingConfig | null to match Provider and ProviderBatchApplyUpdates, update any related imports/exports so the symbol AnthropicAdaptiveThinkingConfig is referenced in the BatchProviderUpdates definition, and if applicable add or reuse existing runtime validation when writing to the DB to ensure only valid AnthropicAdaptiveThinkingConfig shapes are persisted.src/app/v1/_lib/proxy/auth-guard.ts-43-63 (1)
43-63:⚠️ Potential issue | 🟡 Minor限流错误消息语言与文件其他部分不一致。
第 50 行的
"Too many authentication failures. Please retry later."为英文，而同文件中其余认证错误消息（第 122、142、163、186 行）均为中文。建议统一风格，或根据项目约定决定 API 错误消息的语言策略。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/auth-guard.ts` around lines 43 - 63, The rate-limit error message returned when proxyAuthPolicy.check blocks an IP uses English; update the message passed to ProxyResponses.buildError in the pre-auth block (the code using extractClientIp, proxyAuthPolicy.check, rateLimitDecision and retryAfter) to match the file’s Chinese error wording used elsewhere (keep the same error key "rate_limit_error" and preserve Retry-After header behavior), so the API error language is consistent with the other authentication error responses.src/app/[locale]/login/loading.tsx-30-30 (1)
30-30:⚠️ Potential issue | 🟡 Minor
LoadingState不传label会对非中文用户显示硬编码的 "加载中"
loading.tsx无法获取 locale 参数，因此无法调用getTranslations。由于LoadingState的默认值为label = "加载中"，所有语言的用户都会看到这段中文文字，违反了 i18n 规范。建议传入空字符串以移除可见文字，同时保留无障碍属性（
role="status"、aria-live）：🛠️ 建议修复
- <LoadingState className="text-center" /> + <LoadingState label="" className="text-center" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/login/loading.tsx at line 30, LoadingState currently renders with its default label "加载中" for all locales because loading.tsx can't access locale/getTranslations; update the call site to pass label="" to LoadingState (e.g., LoadingState label param) so no visible hard-coded text is shown while preserving its accessibility attributes (role="status", aria-live) and visual spinner behavior.tests/unit/actions/providers-undo-store.test.ts-26-60 (1)
26-60:⚠️ Potential issue | 🟡 Minor
UNDO_SNAPSHOT_TTL_MS应调整为更长的时间，当前 10 秒对用户操作过短代码中
UNDO_SNAPSHOT_TTL_MS设置为 10000 毫秒（10 秒），但 10 秒的时间窗口对需要用户看到 toast 提示、阅读、思考并点击"撤销"的场景来说太短。考虑网络延迟、用户分心或其他中断场景，这个时间容易超时。建议将 TTL 增加至 30 秒或 60 秒，以提供更好的用户体验。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/providers-undo-store.test.ts` around lines 26 - 60, Increase the undo snapshot TTL by updating the UNDO_SNAPSHOT_TTL_MS constant (used by storeUndoSnapshot/consumeUndoToken) from 10_000 to a larger value (e.g., 30_000 or 60_000), and then update the unit tests to match the new TTL: adjust the expected expiresAt in the first test to reflect the new expiry timestamp and change the vi.advanceTimersByTime call in the second test to exceed the new TTL (e.g., 30_001 or 60_001) so the UNDO_EXPIRED assertion still holds.tests/security/session-fixation-rotation.test.ts-84-90 (1)
84-90:⚠️ Potential issue | 🟡 Minor
simulatePostLoginSessionRotation直接调用 mock，未测试任何应用逻辑该辅助函数接受
rotate参数并直接调用（第 88 行），测试本身传入的就是mockRotate（第 172 行）。这实际上是在验证 mock 自身的行为，而非应用的 session rotation 代码路径（RedisSessionStore.rotate()）。建议改为通过登录路由或RedisSessionStore的实际实例来验证 rotation 语义。Also applies to: 161-177
🤖 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 84 - 90, The helper simulatePostLoginSessionRotation currently just invokes the passed-in mock (mockRotate) so tests validate the mock instead of application logic; replace its usage so the test exercises the real rotation path by either invoking the actual RedisSessionStore.rotate() on a real or test instance of RedisSessionStore or by performing a full login request against the app's login route that triggers session rotation, then assert the new session id returned by that real code; update tests referring to simulatePostLoginSessionRotation and mockRotate to call the store instance or login endpoint and verify RedisSessionStore.rotate() behavior rather than the mock.tests/unit/login/login-visual-regression.test.tsx-51-56 (1)
51-56:⚠️ Potential issue | 🟡 Minor
afterEach中act()未 await，React 19 下可能无法正确清理异步副作用
LoginPage内部含Suspense和可能的 async effects，同步act()不会等待这些副作用完成。document.body.removeChild(container)可能在卸载 Promise 解决之前运行，导致后续 state 更新尝试访问已移除的节点。🛠️ 建议修复
- afterEach(() => { - act(() => { - root.unmount(); - }); - document.body.removeChild(container); - }); + afterEach(async () => { + await act(async () => { + root.unmount(); + }); + document.body.removeChild(container); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/login/login-visual-regression.test.tsx` around lines 51 - 56, Make the afterEach handler async and await React's act around the unmount to ensure async Suspense/effects inside LoginPage finish before removing the container: change the afterEach to async, call await act(async () => { root.unmount(); }); and only then call document.body.removeChild(container); this ensures root.unmount and any async cleanup (Suspense/async effects) complete before DOM removal.src/app/[locale]/settings/providers/_components/provider-rich-list-item.tsx-214-246 (1)
214-246:⚠️ Potential issue | 🟡 Minor
res.data可能为undefined：建议在访问undoToken/operationId前添加防御性检查若
removeProvider的成功路径在某些场景下未填充data（例如旧版兼容路径或服务端跳过 undo 追踪），res.data.undoToken会在try/catch内抛出，导致删除实际成功但 toast 静默失败。TypeScript 类型约束足以防止编译期问题，但建议加一道运行时防护以避免难以调试的静默行为：🛠️ 建议修复
if (res.ok) { - const undoToken = res.data.undoToken; - const operationId = res.data.operationId; + const undoToken = res.data?.undoToken; + const operationId = res.data?.operationId; + + if (!undoToken || !operationId) { + queryClient.invalidateQueries({ queryKey: ["providers"] }); + queryClient.invalidateQueries({ queryKey: ["providers-health"] }); + queryClient.invalidateQueries({ queryKey: ["provider-vendors"] }); + return; + } toast.success(tBatchEdit("undo.singleDeleteSuccess"), {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/provider-rich-list-item.tsx around lines 214 - 246, Check that res.data is defined before reading res.data.undoToken and res.data.operationId in the success branch (after res.ok). If res.data is falsy, still show the success toast and run the queryClient.invalidateQueries calls but do not attach an undo action or attempt to call undoProviderDelete; if res.data exists, capture undoToken and operationId and only then render the toast.action with an onClick that calls undoProviderDelete. Also ensure the onClick closure references the locally guarded undoToken/operationId (not assumed properties on res) and keep the existing undoResult handling that checks PROVIDER_BATCH_PATCH_ERROR_CODES.UNDO_EXPIRED and invalidates queries after a successful undo.tests/security/session-login-integration.test.ts-193-236 (1)
193-236:⚠️ Potential issue | 🟡 Minor循环内
vi.clearAllMocks()后未重新设置mockGetTranslations第 200 行
vi.clearAllMocks()清除了所有 mock 实现，但循环内没有重新调用mockGetTranslations.mockResolvedValue(...)。路由通过t?.("...")可选链降级为undefined，不影响主断言，但翻译相关的逻辑未被真实覆盖。建议在循环开头补充：🛠️ 建议修复
for (const mode of modes) { vi.clearAllMocks(); + const mockT = vi.fn((key: string) => `translated:${key}`); + mockGetTranslations.mockResolvedValue(mockT); mockGetSessionTokenMode.mockReturnValue(mode); mockValidateKey.mockResolvedValue(readonlySession);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/session-login-integration.test.ts` around lines 193 - 236, The test clears all mocks inside the loop using vi.clearAllMocks(), but does not re-establish the translation mock, so mockGetTranslations is lost; after each vi.clearAllMocks() (inside the for loop in the "all modes preserve readonly redirect semantics" test) call mockGetTranslations.mockResolvedValue(...) to restore the translations mock (same value used outside the loop), ensuring t?.("...") behavior remains deterministic for each iteration and that translation-related logic is properly covered.tests/unit/login/login-footer-system-name.test.tsx-4-4 (1)
4-4:⚠️ Potential issue | 🟡 Minor应使用
@/路径别名，而非相对路径当前使用了
../../../src/app/[locale]/login/page相对路径导入，而同 PR 其他登录测试文件均使用@/app/[locale]/login/page。-import LoginPage from "../../../src/app/[locale]/login/page"; +import LoginPage from "@/app/[locale]/login/page";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/unit/login/login-footer-system-name.test.tsx` at line 4, Replace the relative import of the LoginPage in tests/unit/login/login-footer-system-name.test.tsx with the project path alias; change the import that currently references "../../../src/app/[locale]/login/page" to use "@/app/[locale]/login/page" so it matches other login tests and the coding guideline for referencing files under src.tests/unit/settings/providers/provider-batch-dialog-step1.test.tsx-8-8 (1)
8-8:⚠️ Potential issue | 🟡 Minor移除未使用的 eightProviders 测试夹具
第 319-321 行定义的
eightProviders在整个测试文件中未被使用，仅定义了twoProviders在默认属性中使用。删除未使用的eightProviders以保持代码清洁。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/providers/provider-batch-dialog-step1.test.tsx` at line 8, Remove the unused test fixture variable named eightProviders from the test file (it’s defined but never referenced); keep the existing twoProviders fixture used in default props and update any nearby comments if they reference eightProviders so the file contains only used fixtures (search for the identifier eightProviders and delete its declaration and any related unused references).src/lib/auth-session-store/redis-session-store.ts-34-60 (1)
34-60:⚠️ Potential issue | 🟡 Minor
parseSessionData的校验应与isOpaqueSessionContract保持一致
isOpaqueSessionContract（src/lib/auth.ts）额外验证了sessionId.length > 0、keyFingerprint.length > 0、userRole.length > 0及expiresAt > createdAt，但parseSessionData缺少这些检查。虽然下游代码（validateAuthToken）会通过isOpaqueSessionContract进行二次校验，但两处校验逻辑的不一致会增加维护成本且易引入 bug。建议补充以下验证以对齐校验逻辑：
建议补充的校验
if (typeof obj.sessionId !== "string") return null; + if (obj.sessionId.length === 0) return null; if (typeof obj.keyFingerprint !== "string") return null; + if (obj.keyFingerprint.length === 0) return null; if (typeof obj.userRole !== "string") return null; + if (obj.userRole.length === 0) return null; if (typeof obj.userId !== "number" || !Number.isInteger(obj.userId)) return null; if (!Number.isFinite(obj.createdAt) || typeof obj.createdAt !== "number") return null; if (!Number.isFinite(obj.expiresAt) || typeof obj.expiresAt !== "number") return null; + if (obj.expiresAt <= obj.createdAt) 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 34 - 60, parseSessionData currently validates field types but misses the same stricter checks performed by isOpaqueSessionContract; update parseSessionData (in src/lib/auth-session-store/redis-session-store.ts) to also ensure sessionId, keyFingerprint, and userRole are non-empty strings (length > 0) and that expiresAt is strictly greater than createdAt (expiresAt > createdAt), while preserving the existing type and integer/finite checks for userId, createdAt, and expiresAt so parsed SessionData matches isOpaqueSessionContract exactly.tests/unit/settings/providers/adaptive-thinking-editor.test.tsx-135-143 (1)
135-143:⚠️ Potential issue | 🟡 Minor测试之间未清理 mock 函数的调用记录。
mockOnEnabledChange和mockOnConfigChange在describe块顶层创建，但测试之间未调用vi.clearAllMocks()或对这些 mock 单独mockClear()。后续测试可能看到前置测试遗留的调用记录，导致断言产生误判。建议添加 beforeEach 清理
const mockOnEnabledChange = vi.fn(); const mockOnConfigChange = vi.fn(); + beforeEach(() => { + vi.clearAllMocks(); + }); + it("renders correctly in disabled state (switch off)", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/providers/adaptive-thinking-editor.test.tsx` around lines 135 - 143, The tests create top-level mocks mockOnEnabledChange and mockOnConfigChange but never clear their call history between tests, which can cause cross-test leakage; add a beforeEach hook inside the describe("AdaptiveThinkingEditor") block that resets mocks (e.g., call vi.clearAllMocks() or individually call mockOnEnabledChange.mockClear() and mockOnConfigChange.mockClear()) so each test starts with a fresh mock state and assertions are deterministic.src/app/[locale]/settings/providers/_components/vendor-keys-compact-list.tsx-324-350 (1)
324-350:⚠️ Potential issue | 🟡 Minor撤销窗口 TTL 与 toast 时长完全一致，存在竞态风险
toast 的
duration: 10000与undo-store.ts中的UNDO_SNAPSHOT_TTL_MS = 10_000完全相同。用户在 toast 即将消失前点击撤销按钮时，由于网络延迟或处理时间，服务端 token 可能已过期，导致撤销失败。建议将 toast 时长略短于服务端 TTL（例如8000），或将服务端 TTL 延长到15_000，以留出安全余量。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/vendor-keys-compact-list.tsx around lines 324 - 350, The toast duration equals the server-side undo TTL, creating a race where clicking the toast action (which calls undoProviderDelete with data.undoToken/data.operationId) may hit an expired token; update the toast duration in vendor-keys-compact-list.tsx to be slightly shorter (e.g. 8000) or alternatively increase the server TTL constant UNDO_SNAPSHOT_TTL_MS in undo-store.ts to 15000 so there is a safety buffer; ensure the change is applied where toast.success is called (duration) and/or where UNDO_SNAPSHOT_TTL_MS is defined so the client-visible undo button window reliably precedes server expiry.src/app/api/auth/login/route.ts-101-103 (1)
101-103:⚠️ Potential issue | 🟡 Minor
shouldIncludeFailureTaxonomy仅通过x-forwarded-protoheader 存在与否来判断，容易被客户端伪造。任何客户端都可以手动添加
x-forwarded-proto头来获取更详细的错误代码（如KEY_REQUIRED、KEY_INVALID）。虽然这些错误码不泄露敏感数据，但如果设计意图是限制 taxonomy 只对受信任的代理环境暴露，当前实现无法满足。如果这是有意为之（例如方便调试），建议补充注释说明。
🤖 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 101 - 103, The helper shouldIncludeFailureTaxonomy currently trusts any presence of the x-forwarded-proto header (request: NextRequest), which is spoofable by clients; change it to validate the header against a trusted-source signal (e.g., verify x-forwarded-for matches a configured list of trusted proxies from an env var like TRUSTED_PROXIES, require an internal-only header/cookie set by your reverse proxy, or check a runtime flag indicating internal deployment) or, if the behavior is intentionally permissive for debugging, add a clear comment above shouldIncludeFailureTaxonomy documenting that intent and the security tradeoffs so reviewers know the decision is deliberate.src/app/[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx-197-197 (1)
197-197:⚠️ Potential issue | 🟡 Minor
"Unknown error"为硬编码英文字符串，应使用 i18n。Lines 197 和 252 中的
"Unknown error"是用户可见的 toast 消息内容的一部分，应通过翻译函数处理。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".
修复建议
- const message = error instanceof Error ? error.message : "Unknown error"; + const message = error instanceof Error ? error.message : t("toast.unknownError");需在对应的
batchEdit.json翻译文件中为 5 种语言添加toast.unknownError键。Also applies to: 252-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx at line 197, Replace the hardcoded "Unknown error" user-facing string with the i18n translation key (e.g., use the component's translation function like t('toast.unknownError')) in both places where it's used (the occurrence inside the error handling that sets const message and the other occurrence around line with similar toast usage); also add the toast.unknownError key to the batchEdit.json translation files for all supported locales (zh-CN, zh-TW, en, ja, ru) so the toast shows localized text.src/app/[locale]/login/page.tsx-16-16 (1)
16-16:⚠️ Potential issue | 🟡 Minor请用
@/路径别名替代相对导入相对路径不符合项目约定，建议改用
@/路径别名以保持一致的导入规范。As per coding guidelines: Use path alias `@/` to reference files in `./src/` directorySuggested fix
-import { resolveLoginRedirectTarget } from "./redirect-safety"; +import { resolveLoginRedirectTarget } from "@/app/[locale]/login/redirect-safety";🤖 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 16, 在 src/app/[locale]/login/page.tsx 中将相对导入 resolveLoginRedirectTarget 从 "./redirect-safety" 改为使用项目路径别名，例如使用 "@/..." 形式引用相同模块（保持导入的符号名 resolveLoginRedirectTarget 不变），以遵循项目对 `@/` 路径别名的约定并替换该相对导入。src/app/[locale]/login/page.tsx-34-35 (1)
34-35:⚠️ Potential issue | 🟡 Minor请将默认站点标题与 API Key 标签纳入 i18n
这两处是用户可见文本，目前硬编码会导致多语言不一致；建议改为翻译键或由系统设置/翻译提供默认值。
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.
Also applies to: 336-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/login/page.tsx around lines 34 - 35, DEFAULT_SITE_TITLE and the visible "API Key" label are hardcoded; replace them with i18n lookups (e.g., use the translation hook/utility your app uses such as t('login.defaultSiteTitle') and t('login.apiKeyLabel')) and add corresponding keys/translations for zh-CN, zh-TW, en, ja, ru in the locale files; ensure DEFAULT_SITE_TITLE constant is removed or set from t(...) and update any usages (page.tsx references to DEFAULT_SITE_TITLE and the API key input/label rendering) to use the translation keys so all user-facing text is localized.src/actions/providers.ts-1680-1691 (1)
1680-1691:⚠️ Potential issue | 🟡 MinorskipReason 文本硬编码且未本地化
skipReason 会被直接渲染到预览文案中，当前英文硬编码在多语言界面下会显得突兀。建议返回稳定的 reason code/枚举，让前端按 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.
Based on learnings: In theding113/claude-code-hubrepository, Server Actions (files undersrc/actions/*.ts) currently return hardcoded Chinese error messages directly. This is a codebase-wide architectural decision that applies to all action files (e.g., model-prices.ts, users.ts, system-config.ts). Changing this pattern requires a coordinated i18n refactor across all Server Actions, which should be handled as a separate unified task rather than piecemeal changes in individual PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/providers.ts` around lines 1680 - 1691, The skipReason is currently hardcoded user-facing text; change it to return a stable reason code/enum instead of the literal message so the frontend can map to localized strings. In the block that sets isCompatible/skipReason (references: isClaudeOnly, isCodexOnly, isGeminiOnly, provider.providerType, isCompatible, skipReason), replace the message assignments with predefined keys (e.g., "only_applicable_claude", "only_applicable_codex", "only_applicable_gemini") or an exported enum constant, and ensure the function returns that code (not human text) so the UI can perform i18n. Ensure existing boolean logic remains unchanged and update any callers/tests expecting a string message to handle the reason code.src/lib/provider-patch-contract.ts-278-343 (1)
278-343:⚠️ Potential issue | 🟡 Minor校验错误消息为硬编码英文，可能直接透传到前端
这些 message 会被 actions 透传给 UI，建议改为返回 error code / message key，由前端按 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/lib/provider-patch-contract.ts` around lines 278 - 343, The validation messages in createInvalidPatchShapeError/createInvalidRootPatchShapeError and calls from normalizePatchField are hardcoded English strings; change them to return a locale-agnostic error identifier and optional params instead of display text so the UI can map to translations. Update createInvalidPatchShapeError and createInvalidRootPatchShapeError signatures to include an errorKey (e.g. "patch.invalid_shape", "patch.unknown_keys", "patch.must_be_object", "patch.choose_one_mode", "patch.set_requires_value", "patch.invalid_set_value") and an optional params object, and replace all literal message arguments in normalizePatchField with the corresponding errorKey and params; keep PROVIDER_PATCH_ERROR_CODES and field unchanged so callers can still detect error type. Ensure the ProviderPatchResult error shape is extended to include errorKey and params for i18n consumption.
src/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.ts
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/batch-edit/provider-batch-preview-step.tsx
Show resolved
Hide resolved
...app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Show resolved
Hide resolved
Comment on lines
+2
to
+7
| import { createCsrfOriginGuard } from "../../src/lib/security/csrf-origin-guard"; | ||
| import { LoginAbusePolicy } from "../../src/lib/security/login-abuse-policy"; | ||
| import { | ||
| buildSecurityHeaders, | ||
| DEFAULT_SECURITY_HEADERS_CONFIG, | ||
| } from "../../src/lib/security/security-headers"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
应使用 @/ 路径别名代替相对路径。
静态导入使用了 ../../src/lib/... 形式的相对路径，不符合编码规范。
As per coding guidelines: "Use path alias @/ to reference files in ./src/ directory"
建议修改为路径别名
-import { createCsrfOriginGuard } from "../../src/lib/security/csrf-origin-guard";
-import { LoginAbusePolicy } from "../../src/lib/security/login-abuse-policy";
-import {
- buildSecurityHeaders,
- DEFAULT_SECURITY_HEADERS_CONFIG,
-} from "../../src/lib/security/security-headers";
+import { createCsrfOriginGuard } from "@/lib/security/csrf-origin-guard";
+import { LoginAbusePolicy } from "@/lib/security/login-abuse-policy";
+import {
+ buildSecurityHeaders,
+ DEFAULT_SECURITY_HEADERS_CONFIG,
+} from "@/lib/security/security-headers";📝 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.
Suggested change
| import { createCsrfOriginGuard } from "../../src/lib/security/csrf-origin-guard"; | |
| import { LoginAbusePolicy } from "../../src/lib/security/login-abuse-policy"; | |
| import { | |
| buildSecurityHeaders, | |
| DEFAULT_SECURITY_HEADERS_CONFIG, | |
| } from "../../src/lib/security/security-headers"; | |
| import { createCsrfOriginGuard } from "@/lib/security/csrf-origin-guard"; | |
| import { LoginAbusePolicy } from "@/lib/security/login-abuse-policy"; | |
| import { | |
| buildSecurityHeaders, | |
| DEFAULT_SECURITY_HEADERS_CONFIG, | |
| } from "@/lib/security/security-headers"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/security/full-security-regression.test.ts` around lines 2 - 7, Replace
the relative imports in this test with the project path alias; update the import
statements that bring in createCsrfOriginGuard, LoginAbusePolicy,
buildSecurityHeaders, and DEFAULT_SECURITY_HEADERS_CONFIG so they use
"@/lib/security/..." (aliasing ./src/) instead of "../../src/lib/..."; ensure
each import specifier references the same exported symbols
(createCsrfOriginGuard, LoginAbusePolicy, buildSecurityHeaders,
DEFAULT_SECURITY_HEADERS_CONFIG) and that your tsconfig/module resolver supports
the "@/..." alias.
Comment on lines
+92
to
+119
| it("SESSION_TOKEN_MODE defaults to opaque", async () => { | ||
| delete process.env.SESSION_TOKEN_MODE; | ||
|
|
||
| vi.resetModules(); | ||
| const { getSessionTokenMode } = await import("../../src/lib/auth"); | ||
|
|
||
| expect(getSessionTokenMode()).toBe("opaque"); | ||
| }); | ||
|
|
||
| it("OpaqueSessionContract has required fields", async () => { | ||
| vi.resetModules(); | ||
| const { isOpaqueSessionContract } = await import("../../src/lib/auth"); | ||
|
|
||
| const contract = { | ||
| sessionId: "sid_opaque_session_123", | ||
| keyFingerprint: "sha256:abc123", | ||
| createdAt: 1_700_000_000, | ||
| expiresAt: 1_700_000_300, | ||
| userId: 42, | ||
| userRole: "admin", | ||
| }; | ||
|
|
||
| expect(isOpaqueSessionContract(contract)).toBe(true); | ||
|
|
||
| const missingUserRole = { ...contract } as Partial<typeof contract>; | ||
| delete missingUserRole.userRole; | ||
| expect(isOpaqueSessionContract(missingUserRole)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
动态导入同样应使用 @/ 路径别名。
await import("../../src/lib/auth") 和 await import("../../src/lib/auth-session-store/redis-session-store") 也应改为 @/lib/auth 和 @/lib/auth-session-store/redis-session-store。
As per coding guidelines: "Use path alias @/ to reference files in ./src/ directory"
建议修改动态导入路径
- const { getSessionTokenMode } = await import("../../src/lib/auth");
+ const { getSessionTokenMode } = await import("@/lib/auth");- const { isOpaqueSessionContract } = await import("../../src/lib/auth");
+ const { isOpaqueSessionContract } = await import("@/lib/auth");- const { RedisSessionStore } = await import(
- "../../src/lib/auth-session-store/redis-session-store"
- );
+ const { RedisSessionStore } = await import(
+ "@/lib/auth-session-store/redis-session-store"
+ );- const { AUTH_COOKIE_NAME, setAuthCookie } = await import("../../src/lib/auth");
+ const { AUTH_COOKIE_NAME, setAuthCookie } = await import("@/lib/auth");- const { setAuthCookie } = await import("../../src/lib/auth");
+ const { setAuthCookie } = await import("@/lib/auth");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/security/full-security-regression.test.ts` around lines 92 - 119,
Update the dynamic imports in the test to use the project path alias instead of
relative paths: replace await import("../../src/lib/auth") with await
import("@/lib/auth") (so getSessionTokenMode is imported via "@/lib/auth") and
similarly replace any await
import("../../src/lib/auth-session-store/redis-session-store") with await
import("@/lib/auth-session-store/redis-session-store") (so
isOpaqueSessionContract remains sourced from "@/lib/auth"); keep
vi.resetModules() as-is and only change the module specifiers.
| @@ -0,0 +1,253 @@ | |||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | |||
| import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "../../../src/lib/provider-batch-patch-error-codes"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
应使用 @/ 路径别名替代相对路径。
Line 2 及后续所有动态 import（lines 63, 78, 90, 101, 112, 125, 157, 180, 202, 219, 234）使用了 ../../../src/ 相对路径，违反了编码规范中要求使用 @/ 别名的规则。
As per coding guidelines, "Use path alias @/ to reference files in ./src/ directory".
修复示例（静态 import）
-import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "../../../src/lib/provider-batch-patch-error-codes";
+import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "@/lib/provider-batch-patch-error-codes";修复示例（动态 import）
- const { batchDeleteProviders } = await import("../../../src/actions/providers");
+ const { batchDeleteProviders } = await import("@/actions/providers");📝 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.
Suggested change
| import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "../../../src/lib/provider-batch-patch-error-codes"; | |
| import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "@/lib/provider-batch-patch-error-codes"; |
Suggested change
| import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "../../../src/lib/provider-batch-patch-error-codes"; | |
| const { batchDeleteProviders } = await import("@/actions/providers"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/actions/provider-undo-delete.test.ts` at line 2, Replace all
relative imports referencing ../../../src/ with the project path alias `@/` so
imports conform to the coding guideline; specifically change the static import
of PROVIDER_BATCH_PATCH_ERROR_CODES and every dynamic import occurrences (the
dynamic import calls around lines where modules are loaded — e.g., the import
expressions that currently use "../../../src/..." at lines referenced in the
review) to use "@/..." paths instead, ensuring module specifiers match the
src-relative alias and updating any test runner/tsconfig configs if necessary to
resolve the alias.
Comment on lines
+96
to
+97
| expect(result.error).toBe("无权限执行此操作"); | ||
| expect(deleteProvidersBatchMock).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if src/actions/providers.ts exists and search for hardcoded Chinese strings
fd 'providers.ts' --type f src/actions/Repository: ding113/claude-code-hub
Length of output: 91
🏁 Script executed:
# Check the test file to see the assertions at the specified lines
head -n 230 tests/unit/actions/provider-undo-delete.test.ts | tail -n +90Repository: ding113/claude-code-hub
Length of output: 5011
🏁 Script executed:
# Look for hardcoded Chinese error messages in src/actions/providers.ts
rg -n '无权限|请选择|批量操作|删除供应商|撤销' --type ts src/actions/Repository: ding113/claude-code-hub
Length of output: 5843
服务端操作中硬编码的中文错误消息缺少 i18n 国际化支持
测试正确反映了源码行为，但 src/actions/providers.ts 中的多个错误消息（"无权限执行此操作"、"请选择要删除的供应商"、"单次批量操作最多支持"、"撤销窗口已过期" 等）均为硬编码中文，未通过 i18n 支持 5 种语言。这违反了编码规范"所有用户展示字符串必须使用 i18n，支持 zh-CN、zh-TW、en、ja、ru 5 种语言"。
注：此问题为代码库级的架构性问题，不仅限于 providers.ts，所有 src/actions/*.ts 文件中的错误消息均存在此情况。建议作为统一的 i18n 重构任务处理，而非在单个 PR 中逐个修改。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/actions/provider-undo-delete.test.ts` around lines 96 - 97, The
tests and source use hardcoded Chinese error strings (e.g., "无权限执行此操作",
"请选择要删除的供应商", "单次批量操作最多支持", "撤销窗口已过期") in src/actions/providers.ts and other
src/actions/*.ts files; replace these with i18n lookups and update tests to
assert against localized messages or mock the i18n function. Specifically, in
providers.ts refactor all user-facing error strings to call the project i18n API
(e.g., the central translate function used elsewhere) and ensure tests (like
provider-undo-delete.test.ts) either import/mock that i18n function or compare
against i18n(key) instead of literal Chinese strings so coverage supports
zh-CN/zh-TW/en/ja/ru; treat this as a cross-file refactor rather than per-PR
one-off edits.
Comment on lines
+97
to
+110
| vi.mock("./forms/provider-form/components/section-card", () => ({ | ||
| SmartInputWrapper: ({ label, children }: { label: string; children: React.ReactNode }) => ( | ||
| <div data-testid="smart-input-wrapper"> | ||
| <label>{label}</label> | ||
| {children} | ||
| </div> | ||
| ), | ||
| ToggleRow: ({ label, children }: { label: string; children: React.ReactNode }) => ( | ||
| <div data-testid="toggle-row"> | ||
| <label>{label}</label> | ||
| {children} | ||
| </div> | ||
| ), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the adaptive-thinking-editor component file
echo "=== Locating AdaptiveThinkingEditor component ==="
fd "adaptive-thinking-editor" -e tsx -e ts --type f
echo -e "\n=== Checking test file location ==="
ls -la tests/unit/settings/providers/adaptive-thinking-editor.test.tsx
echo -e "\n=== Finding section-card component ==="
fd "section-card" -e tsx -e ts --type fRepository: ding113/claude-code-hub
Length of output: 543
🏁 Script executed:
#!/bin/bash
# Once we find the adaptive-thinking-editor component, check its imports
echo "=== Looking for AdaptiveThinkingEditor component ==="
find . -name "*adaptive-thinking-editor*" -type f
# Also check the directory structure
echo -e "\n=== Directory structure of tests/unit/settings/providers/ ==="
tree tests/unit/settings/providers/ || find tests/unit/settings/providers/ -type f -name "*.tsx" -o -name "*.ts"Repository: ding113/claude-code-hub
Length of output: 1189
🏁 Script executed:
#!/bin/bash
# Search for import statements related to section-card or SmartInputWrapper/ToggleRow
echo "=== Finding imports of section-card, SmartInputWrapper, or ToggleRow ==="
rg "import.*(?:section-card|SmartInputWrapper|ToggleRow)" -g "*.tsx" -g "*.ts" -A 2
echo -e "\n=== Checking src directory structure ==="
find src -type d | grep -E "(settings|providers|forms)" | head -20Repository: ding113/claude-code-hub
Length of output: 3944
mock 路径相对路径解析错误，导致 mock 无法生效。
vi.mock("./forms/provider-form/components/section-card", ...) 中的相对路径会从测试文件所在目录（tests/unit/settings/providers/）开始解析，而非源文件 src/app/[locale]/settings/providers/_components/adaptive-thinking-editor.tsx 所在目录。实际导入的 section-card 组件路径为 src/app/[locale]/settings/providers/_components/forms/provider-form/components/section-card.tsx，两者不匹配导致 mock 无法拦截导入。
建议改为使用路径别名：@/app/[locale]/settings/providers/_components/forms/provider-form/components/section-card
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/providers/adaptive-thinking-editor.test.tsx` around lines
97 - 110, The vi.mock call for the section-card module uses a relative path that
resolves from the test file location, so update the module specifier in the
vi.mock(...) call to the project alias used by the source file import (use
"@/app/[locale]/settings/providers/_components/forms/provider-form/components/section-card")
so the mock intercepts imports; keep the exported mock shape (SmartInputWrapper
and ToggleRow) unchanged.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR implements a comprehensive security auth overhaul and provider batch operations feature. The implementation is well-designed with proper security controls, comprehensive test coverage, and clear documentation.
PR Size: XL
- Lines changed: 20,068 (19,172 additions, 896 deletions)
- Files changed: 111
Note: This is a large PR. Consider splitting future changes of this magnitude into multiple PRs for easier review:
- Security auth overhaul (session tokens, CSRF, rate limiting)
- Provider batch operations (preview, apply, undo)
- Login page redesign
- Test additions
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Properly implemented
- Error handling - Appropriate with logging
- Type safety - No unsafe
anyusage found - Documentation accuracy - Comments match code
- Test coverage - Comprehensive (55+ test files, 2791+ tests)
- Code clarity - Good
Security Implementation Notes
The security implementations are well-designed:
-
IP extraction (
login/route.ts:118-129): Uses rightmost x-forwarded-for entry, which is the correct approach when trusted proxies are configured. The code comment explains this design decision. -
Rate limiting: Dual-scope (IP + key) rate limiting with configurable thresholds. IP-based limiting applies to all requests; key-based limiting only activates when a key is provided.
-
CSRF protection: Origin-based guard with same-origin bypass and configurable enforcement.
-
Constant-time comparison: Properly implemented using
crypto.timingSafeEqualwith length-padding to prevent timing attacks. -
Session management: Opaque session tokens with Redis-backed storage, dual-read migration support, and proper revocation on logout.
Automated review by Claude AI
| > | ||
| <div className="space-y-2"> | ||
| <Label htmlFor="apiKey">API Key</Label> | ||
| <div className="relative"> |
Contributor
There was a problem hiding this comment.
[Medium] [STANDARD-VIOLATION] Hardcoded UI string bypasses i18n
Evidence: src/app/[locale]/login/page.tsx:338 contains:
<Label htmlFor="apiKey">API Key</Label>Why this is a problem: This renders English text for all locales. The project standard requires: i18n Required - All user-facing strings must use i18n (5 languages supported). Never hardcode display text.
Suggested fix:
// src/app/[locale]/login/page.tsx
<Label htmlFor="apiKey">{t("form.apiKeyLabel")}</Label>Also add form.apiKeyLabel to messages/{en,zh-CN,zh-TW,ja,ru}/auth.json.
| label: t("toast.undo"), | ||
| onClick: async () => { | ||
| const undoResult = await undoProviderPatch({ undoToken, operationId }); | ||
| if (undoResult.ok) { |
Contributor
There was a problem hiding this comment.
[Medium] [ERROR-NO-USER-FEEDBACK] Undo action lacks exception handling + non-i18n fallback
Evidence:
src/app/[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx:239runs a server action inside a toast action without atry/catch:
const undoResult = await undoProviderPatch({ undoToken, operationId });- The same file uses a hardcoded user-facing fallback string (e.g.
src/app/[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx:197,:252):
const message = error instanceof Error ? error.message : "Unknown error";Why this is a problem:
- If the server action call rejects (network/server error), this will surface as an unhandled promise rejection and the user gets no toast feedback.
"Unknown error"bypasses the i18n requirement (i18n Required - All user-facing strings must use i18n (5 languages supported). Never hardcode display text).
Suggested fix:
action: {
label: t("toast.undo"),
onClick: async () => {
try {
const undoResult = await undoProviderPatch({ undoToken, operationId });
if (undoResult.ok) {
toast.success(t("toast.undoSuccess", { count: undoResult.data.revertedCount }));
await queryClient.invalidateQueries({ queryKey: ["providers"] });
return;
}
if (undoResult.errorCode === PROVIDER_BATCH_PATCH_ERROR_CODES.UNDO_EXPIRED) {
toast.error(t("toast.undoExpired"));
return;
}
toast.error(t("undo.failed"));
} catch (error) {
const message = error instanceof Error ? error.message : t("toast.unknownError");
toast.error(t("toast.undoFailed", { error: message }));
}
},
}Also replace the other "Unknown error" fallbacks with t("toast.unknownError") and add that key to messages/{en,zh-CN,zh-TW,ja,ru}/settings/providers/batchEdit.json.
src/app/api/auth/logout/route.ts
Outdated
| async function resolveAuthCookieToken(): Promise<string | undefined> { | ||
| try { | ||
| return await getAuthCookie(); | ||
| } catch { |
Contributor
There was a problem hiding this comment.
[High] [ERROR-SILENT] Logout can skip server-side session revocation without logging
Evidence:
src/app/api/auth/logout/route.ts:30swallows exceptions fromgetSessionTokenMode():
function resolveSessionTokenMode(): SessionTokenMode {
try {
return getSessionTokenMode();
} catch {
return "legacy";
}
}src/app/api/auth/logout/route.ts:41swallows exceptions fromgetAuthCookie():
async function resolveAuthCookieToken(): Promise<string | undefined> {
try {
return await getAuthCookie();
} catch {
return undefined;
}
}Why this is a problem: If getAuthCookie() / getSessionTokenMode() throws unexpectedly, the outer revocation try/catch never sees the error. Logout returns 200 and clears the client cookie, but the opaque session may remain valid and the failure is invisible in logs.
Suggested fix (let the existing outer try/catch log failures):
function resolveSessionTokenMode(): SessionTokenMode {
return getSessionTokenMode();
}
async function resolveAuthCookieToken(): Promise<string | undefined> {
return await getAuthCookie();
}If you want to keep the fallback behavior, at least logger.warn(...) in the catch and rethrow or return a sentinel that triggers logging.
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR is extremely large and mixes auth/security overhaul, login UI changes, provider batch operations, and extensive tests in a single diff, which increases review and rollback risk.
The inline comments focus on a few concrete, diff-local issues that can lead to user-facing inconsistencies or make session revocation failures invisible.
PR Size: XL
- Lines changed: 20068
- Files changed: 111
- Split suggestions (XL):
- Split auth/session/security changes (routes + session store + guards) from provider batch operations.
- Split login UI redesign from backend auth route changes.
- Split i18n message updates and test-only additions into follow-up PRs where possible.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 0 |
| Security | 0 | 1 | 0 | 0 |
| Error Handling | 0 | 1 | 1 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
- [High] swallows errors when reading auth state, which can skip opaque session revocation without logging.
- [Medium] hardcodes a user-facing label (bypasses i18n).
- [Medium] undo action lacks exception handling and uses a hardcoded fallback error string.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR is extremely large and mixes auth/security overhaul, login UI changes, provider batch operations, and extensive tests in a single diff, which increases review and rollback risk.
The inline comments focus on a few concrete, diff-local issues that can lead to user-facing inconsistencies or make session revocation failures invisible.
PR Size: XL
- Lines changed: 20068
- Files changed: 111
- Split suggestions (XL):
- Split auth/session/security changes (routes + session store + guards) from provider batch operations.
- Split login UI redesign from backend auth route changes.
- Split i18n message updates and test-only additions into follow-up PRs where possible.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 0 |
| Security | 0 | 1 | 0 | 0 |
| Error Handling | 0 | 1 | 1 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
- [High]
src/app/api/auth/logout/route.ts:41swallows errors when reading auth state, which can skip opaque session revocation without logging. - [Medium]
src/app/[locale]/login/page.tsx:338hardcodes a user-facing label (bypasses i18n). - [Medium]
src/app/[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx:239undo action lacks exception handling and uses a hardcoded fallback error string.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
- Identified PR
#806and applied the size labelsize/XL(20068 lines changed across 111 files). - Posted 3 inline review comments (diff-only) with concrete fixes:
src/app/[locale]/login/page.tsx:338— hardcoded"API Key"label bypasses i18n.src/app/[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx:239— undo toast action needstry/catch+ replace hardcoded"Unknown error"with i18n.src/app/api/auth/logout/route.ts:41— innertry/catchswallows errors and can skip opaque session revocation without logging.
- Submitted a review summary on the PR (includes XL split suggestions).
| import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "@/lib/provider-batch-patch-error-codes"; | ||
| import { RedisKVStore } from "@/lib/redis/redis-kv-store"; | ||
|
|
||
| const UNDO_SNAPSHOT_TTL_SECONDS = 30; |
There was a problem hiding this comment.
30-second TTL increased from 10s, but still might be too short for batch operations where users navigate away or review changes before realizing they need to undo
Suggested change
| const UNDO_SNAPSHOT_TTL_SECONDS = 30; | |
| const UNDO_SNAPSHOT_TTL_SECONDS = 60; |
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/providers/undo-store.ts
Line: 7
Comment:
30-second TTL increased from 10s, but still might be too short for batch operations where users navigate away or review changes before realizing they need to undo
```suggestion
const UNDO_SNAPSHOT_TTL_SECONDS = 60;
```
<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.
Comment on lines
+190
to
+191
| if (!key || typeof key !== "string") { | ||
| loginPolicy.recordFailure(clientIp); |
There was a problem hiding this comment.
missing key parameter in recordFailure() call means key-based rate limiting won't trigger for malformed keys, only IP-based limiting applies
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 190-191
Comment:
missing `key` parameter in `recordFailure()` call means key-based rate limiting won't trigger for malformed keys, only IP-based limiting applies
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+159
to
+161
| // LRU bump: delete + re-insert so locked entries survive eviction | ||
| this.attempts.delete(scopeKey); | ||
| this.attempts.set(scopeKey, { ...record, lockedUntil }); |
There was a problem hiding this comment.
implemented LRU bump strategy to mitigate the 10k entry limit; delete + re-insert moves entries to end of Map iteration order so eviction removes least-recently-used first
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/login-abuse-policy.ts
Line: 159-161
Comment:
implemented LRU bump strategy to mitigate the 10k entry limit; delete + re-insert moves entries to end of Map iteration order so eviction removes least-recently-used first
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+274
to
+279
| if (sessionData.expiresAt <= Date.now()) { | ||
| logger.warn("Opaque session expired (application-level check)", { | ||
| sessionId: sessionData.sessionId, | ||
| expiresAt: sessionData.expiresAt, | ||
| }); | ||
| return null; |
There was a problem hiding this comment.
added defense-in-depth expiry check at application level after Redis read; verify Redis TTL matches expiresAt duration
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/auth.ts
Line: 274-279
Comment:
added defense-in-depth expiry check at application level after Redis read; verify Redis TTL matches `expiresAt` duration
<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.
Comment on lines
+24
to
+27
| const LUA_GET_AND_DEL = ` | ||
| local val = redis.call('GET', KEYS[1]) | ||
| if val then redis.call('DEL', KEYS[1]) end | ||
| return val`; |
There was a problem hiding this comment.
implemented atomic GET+DEL via Lua script to prevent TOCTOU race on single-use tokens
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/redis/redis-kv-store.ts
Line: 24-27
Comment:
implemented atomic GET+DEL via Lua script to prevent TOCTOU race on single-use tokens
How can I resolve this? If you propose a fix, please make it concise.| const session = await validateKey(key, { allowReadOnlyAccess: true }); | ||
| if (!session) { | ||
| return NextResponse.json({ error: t?.("apiKeyInvalidOrExpired") }, { status: 401 }); | ||
| loginPolicy.recordFailure(clientIp); |
There was a problem hiding this comment.
missing key parameter in second recordFailure() call means invalid key attempts aren't tracked for key-based rate limiting, only IP-based limiting applies
Suggested change
| loginPolicy.recordFailure(clientIp); | |
| loginPolicy.recordFailure(clientIp, key); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 212
Comment:
missing `key` parameter in second `recordFailure()` call means invalid key attempts aren't tracked for key-based rate limiting, only IP-based limiting applies
```suggestion
loginPolicy.recordFailure(clientIp, 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/providers.ts (1)
3861-3863:⚠️ Potential issue | 🟡 Minor注释中包含 emoji，需移除以符合规范。
As per coding guidelines: "/*.{js,ts,tsx,jsx}: Never use emoji characters in any code, comments, or string literals".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/providers.ts` around lines 3861 - 3863, The inline comment containing an emoji in the object literal near the Response API fix must be cleaned up: in src/actions/providers.ts locate the object with properties type: "message" and role: "user" (the comment that currently reads "⭐ 修复 `#189`: Response API 要求 input 数组中的每个元素必须包含 type 字段") and remove the emoji character(s) so the comment reads plain text (e.g., "修复 `#189`: Response API 要求 input 数组中的每个元素必须包含 type 字段") or delete the comment entirely to comply with the no-emoji rule.
🧹 Nitpick comments (11)
src/app/api/auth/logout/route.ts (1)
18-28:getLogoutSessionStore存在并发初始化竞态条件，且内联类型注解可读性差1. 竞态条件：
getLogoutSessionStore在await import(...)前做了if (!sessionStoreInstance)检查，但这两个操作之间存在异步间隙。若两个并发退出请求同时通过 null 检查，将各自创建一个RedisSessionStore实例，后一次赋值会覆盖前一次，被覆盖的实例及其潜在 Redis 连接将被泄漏。建议改用 Promise 单例模式，确保初始化逻辑只执行一次：
2. 内联
import()类型注解：行 18-20 的内联动态import()类型写法可读性差，建议在文件顶部通过import type导入。♻️ 建议的修复方案
+import type { RedisSessionStore } from "@/lib/auth-session-store/redis-session-store"; -let sessionStoreInstance: - | import("@/lib/auth-session-store/redis-session-store").RedisSessionStore - | null = null; +let sessionStorePromise: Promise<RedisSessionStore> | null = null; async function getLogoutSessionStore() { - if (!sessionStoreInstance) { - const { RedisSessionStore } = await import("@/lib/auth-session-store/redis-session-store"); - sessionStoreInstance = new RedisSessionStore(); - } - return sessionStoreInstance; + if (!sessionStorePromise) { + sessionStorePromise = import("@/lib/auth-session-store/redis-session-store").then( + ({ RedisSessionStore }) => new RedisSessionStore(), + ); + } + return sessionStorePromise; }🤖 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 18 - 28, getLogoutSessionStore currently has a race: multiple concurrent callers can pass the null check and each call will dynamically import and construct a RedisSessionStore, leaking instances; also the inline dynamic import type annotation is hard to read. Fix by making sessionStoreInstance hold a Promise that is set on first call (a "promise singleton") so concurrent callers await the same initialization Promise and only one RedisSessionStore is constructed, e.g. replace the nullable instance with a Promise<RedisSessionStore> | null and ensure getLogoutSessionStore sets that Promise before awaiting the import/constructor; also move the type import to the top with an `import type { RedisSessionStore } from "@/lib/auth-session-store/redis-session-store"` and use that type for the Promise and return signature to improve readability.src/repository/provider.ts (1)
1355-1381:restoreProvidersBatch在单个事务内串行执行 N 次多步查询，大批量时事务持锁时间较长当前实现对每个
id顺序执行restoreProviderInTransaction，后者最多包含以下 DB 往返：
SELECT候选供应商UPDATE providers（乐观并发）SELECT活跃引用（activeReference）SELECT活跃端点（activeEndpoint）SELECT待恢复端点，UPDATE providerEndpoints对于 N 个供应商，这意味着 O(N × 5) 次串行查询共享同一个事务，事务持续时间随 N 线性增长。
虽然 60 秒的 TTL（
PROVIDER_RESTORE_MAX_AGE_MS）使实际批量规模保持较小，但建议在函数文档中明确说明此约束，或在批量较大时（如uniqueIds.length > 10）记录警告日志，以便运维排查。建议在大批量时添加日志警告
+ if (uniqueIds.length > 10) { + logger.warn("restoreProvidersBatch:large_batch", { count: uniqueIds.length }); + } + const restoredCount = await db.transaction(async (tx) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/provider.ts` around lines 1355 - 1381, restoreProvidersBatch performs sequential multi-step DB work for each id inside one long transaction (calling restoreProviderInTransaction repeatedly), which can cause long-held locks for large batches; update the function to (1) add a clear doc comment noting the 60s TTL constraint (PROVIDER_RESTORE_MAX_AGE_MS) and expected small batch sizes, and (2) emit a warning log when uniqueIds.length exceeds a safe threshold (e.g., > 10) before starting the transaction so operators are alerted to potential long transactions; reference restoreProvidersBatch, uniqueIds, restoredCount, and PROVIDER_RESTORE_MAX_AGE_MS in the message.src/app/[locale]/settings/providers/_components/thinking-budget-editor.tsx (1)
35-35: 建议将魔法数字提取为具名常量。
"10240"（自定义模式默认值）和"32000"（最大值）直接以字符串字面量散落在代码中，语义不清晰，且与min="1024"/max="32000"的 HTML 属性分散维护，一旦 Anthropic 调整限制，需多处同步修改。♻️ 建议提取常量
+"use client"; + +import { Info } from "lucide-react"; ... +const THINKING_BUDGET_DEFAULT = "10240"; +const THINKING_BUDGET_MAX = "32000"; +const THINKING_BUDGET_MIN = 1024; + const handleModeChange = (val: string) => { if (val === "inherit") { onChange("inherit"); } else { - onChange("10240"); + onChange(THINKING_BUDGET_DEFAULT); } }; const handleMaxOut = () => { - onChange("32000"); + onChange(THINKING_BUDGET_MAX); }; // Input 的 min/max 也统一使用常量 -min="1024" -max="32000" +min={String(THINKING_BUDGET_MIN)} +max={THINKING_BUDGET_MAX}Also applies to: 43-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/thinking-budget-editor.tsx at line 35, The hard-coded magic strings for the thinking budget ("10240" default and "32000" max, and the min "1024" used in attributes) should be extracted into named constants to centralize semantics and avoid duplication; update the onChange("10240") call, the input's min/max attributes, and any other occurrences (lines ~43-45) to use constants like DEFAULT_CUSTOM_THINKING_BUDGET, MIN_THINKING_BUDGET, and MAX_THINKING_BUDGET (choose string or numeric types consistently with the input handling and parse/format where needed) so future Anthropic limit changes require a single update and the code is clearer.src/lib/providers/undo-store.ts (1)
23-31:UNDO_CONFLICT代码出现在导出类型中，但consumeUndoToken从未返回它——建议缩窄函数返回类型或补充注释。
ConsumeUndoResult在code联合类型中包含"UNDO_CONFLICT"，但consumeUndoToken的实现始终只返回UNDO_EXPIRED。这意味着UNDO_CONFLICT的语义是由上层调用方（如src/actions/providers.ts）在完成状态校验后自行构造并返回的。当前代码对读者造成困惑：consumeUndoToken的签名暗示它可能返回UNDO_CONFLICT，但实际上不会。建议两种方案之一：
♻️ 方案 A：为 consumeUndoToken 声明更窄的返回类型
+type ConsumeTokenResult = + | { ok: true; snapshot: UndoSnapshot } + | { ok: false; code: "UNDO_EXPIRED" }; + -export async function consumeUndoToken(token: string): Promise<ConsumeUndoResult> { +export async function consumeUndoToken(token: string): Promise<ConsumeTokenResult> {上层调用方可将
ConsumeTokenResult与自己的冲突检测结果合并为ConsumeUndoResult。♻️ 方案 B：补充设计意图注释
+/** + * 消费撤销令牌，原子地获取并删除快照。 + * 注意：此函数只返回 ok:true 或 code:UNDO_EXPIRED。 + * UNDO_CONFLICT 由上层调用方在完成状态一致性检查后自行构造返回。 + */ export async function consumeUndoToken(token: string): Promise<ConsumeUndoResult> {Also applies to: 61-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/undo-store.ts` around lines 23 - 31, ConsumeUndoResult declares a possible code "UNDO_CONFLICT" but the implementation of consumeUndoToken never returns it, causing confusion; either narrow the signature or document intent. Option A (recommended): remove "UNDO_CONFLICT" from ConsumeUndoResult and export a narrower result used by consumeUndoToken (e.g., only ok:true|{ok:false;code:"UNDO_EXPIRED"}) then let callers in src/actions/providers.ts merge that with their conflict-detection to produce the full ConsumeUndoResult. Option B: add a clear comment above ConsumeUndoResult and on consumeUndoToken explaining that consumeUndoToken only returns UNDO_EXPIRED and that UNDO_CONFLICT is created by upstream callers (e.g., in src/actions/providers.ts) after additional validation.src/app/[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx (1)
36-47: 建议将相对导入改为@/别名路径。
当前新增的相对路径位于 src 内，按规范应统一使用别名，便于全局重构与一致性。
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 `@src/app/`[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx around lines 36 - 47, Several imports (FormTabNav, ProviderFormProvider, useProviderForm, BasicInfoSection, LimitsSection, NetworkSection, RoutingSection, TestingSection) use relative paths; change them to use the project path alias (start with "@/") so they reference the same modules via the src alias instead of "../...". Update the import statements that currently import those symbols from "../forms/..." to import them from the corresponding "@/..." alias equivalents, keeping the same exported names and ensuring module resolution/build passes.src/app/[locale]/login/loading.tsx (1)
4-4: 建议改为具名导出以符合导出规范。根据编码规范 **/*.{ts,tsx} 应优先使用具名导出。但需要注意，项目中所有 loading.tsx 文件（共 9 个）都统一采用默认导出模式，包括：
- dashboard/loading.tsx
- dashboard/quotas/loading.tsx
- my-usage/loading.tsx
- 等其他文件
建议如果应用此更改，应同时更新所有 loading.tsx 文件以保持一致性。
建议修改
-export default function LoginLoading() { +export function LoginLoading() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/login/loading.tsx at line 4, The component LoginLoading is currently a default export; change it to a named export by exporting the LoginLoading function as a named export (export function LoginLoading...) and remove the default export usage; update any import sites that rely on the default import to use the named import { LoginLoading } instead. If you intend to apply this convention project-wide, apply the same change to all other loading.tsx files (e.g., dashboard/loading.tsx, dashboard/quotas/loading.tsx, my-usage/loading.tsx) and update their consumers accordingly to maintain consistency.src/lib/security/login-abuse-policy.ts (2)
157-167:checkScope中count >= threshold分支是可以保留的防御性代码，但值得注释说明该分支仅在
lockedUntil未设置但count >= threshold时触发。然而根据recordFailureForScope的实现，每次count达到阈值时lockedUntil都会被立即设置。因此，在当前实现下此分支在正常情况下不可达——它是防止将来代码变更破坏不变量的安全兜底。建议加注释说明，避免后续维护者误删。+ // Defensive guard: in normal operation recordFailureForScope always sets + // lockedUntil when count reaches threshold, making this branch unreachable. + // It remains as a safety net against future invariant breaks. if (record.count >= threshold) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/security/login-abuse-policy.ts` around lines 157 - 167, The branch in checkScope that handles record.count >= threshold is defensive/unreachable under current logic because recordFailureForScope always sets lockedUntil when count reaches the threshold; add a clear comment above that branch explaining it is an intentional safety/defensive fallback to preserve behavior if invariants change, reference the related functions/variables (checkScope, recordFailureForScope, attempts, scopeKey, lockedUntil) and state that it ensures locked entries survive LRU eviction and prevents accidental removal by future maintainers.
27-28: 内存级限流在分布式部署下无法同步状态
LoginAbusePolicy将所有AttemptRecord存储在进程内存Map中，无法跨 Node.js 进程共享。在多 Pod/多实例部署场景下，攻击者可通过向不同实例轮发请求来绕过每实例限流阈值。MAX_TRACKED_ENTRIES和SWEEP_INTERVAL_MS的内存管理设计良好，但仅对单进程有效。鉴于本 PR 已引入 Redis 基础设施，后续可考虑将限流计数迁移至 Redis（滑动窗口计数器），使限流在水平扩展场景下保持一致。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/security/login-abuse-policy.ts` around lines 27 - 28, The LoginAbusePolicy currently stores AttemptRecord instances in an in-memory Map (and relies on MAX_TRACKED_ENTRIES and SWEEP_INTERVAL_MS) which won’t work across multiple Node.js processes; replace the per-process Map usage in LoginAbusePolicy with Redis-backed counters/sets using the repo’s existing Redis client, implement a distributed sliding-window rate limit (use INCR with EXPIRE for fixed-window or a small Lua script / sorted-set approach for true sliding-window) keyed by the same identifiers you currently use for AttemptRecord, remove or make MAX_TRACKED_ENTRIES/SWEEP_INTERVAL_MS local-cache fallbacks only, and ensure checks and increments are atomic so limits are enforced consistently across pods.src/app/api/auth/login/route.ts (2)
21-27:csrfGuard和loginPolicy均为模块级内存单例，在多进程/无服务器部署下行为受限
loginPolicy的频率限制状态仅存储在当前 Node.js 进程内存中。在多实例水平扩展（如 Kubernetes 多 Pod）或无服务器环境（如 Vercel 冷启动）下，每个实例拥有独立的计数器，攻击者可通过轮换请求目标不同实例来绕过限流。- 鉴于本 PR 已引入 Redis 基础设施（
RedisSessionStore），建议考虑将限流状态迁移到 Redis，或使用专用的分布式限流方案（如ioredis+ 滑动窗口 Lua 脚本）以保证多实例一致性。🤖 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 21 - 27, csrfGuard and loginPolicy are module-level singletons holding in-process state; migrate login/CSRF state to a shared Redis-backed store to support multi-process/edge deployments: modify LoginAbusePolicy to accept a Redis-backed counter/TTL interface (or replace with a distributed limiter library) and update usages where createCsrfOriginGuard and csrfGuard rely on in-memory data to use the RedisSessionStore (or a Redis-based rate-limiter) so counters/blocks are consistent across instances; ensure constructor/factory for LoginAbusePolicy and the CSRF guard accept an injected redis client or adapter and fallback to the existing in-memory behavior for local dev/tests.
53-93:getAuthErrorTranslations与getAuthSecurityTranslations存在重复代码两个函数的 try/catch/fallback 结构完全相同，仅
namespace参数不同，违反 DRY 原则。建议重构：提取公共帮助函数
+async function getTranslationsWithFallback(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; + } + } +} + -async function getAuthErrorTranslations(locale: Locale) { - try { - return await getTranslations({ locale, namespace: "auth.errors" }); - } catch (error) { - ... - } -} - -async function getAuthSecurityTranslations(locale: Locale) { - try { - return await getTranslations({ locale, namespace: "auth.security" }); - } catch (error) { - ... - } -} +const getAuthErrorTranslations = (locale: Locale) => + getTranslationsWithFallback(locale, "auth.errors"); +const getAuthSecurityTranslations = (locale: Locale) => + getTranslationsWithFallback(locale, "auth.security");🤖 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 shared try/catch/fallback logic into a single helper (e.g. getAuthTranslations(namespace: string, locale: Locale)) that calls getTranslations({ locale, namespace }), falls back to getTranslations({ locale: defaultLocale, namespace }) on error, logs using logger.warn/logger.error (include locale, defaultLocale, namespace and the error message), and returns the same nullable translation result; then have getAuthErrorTranslations and getAuthSecurityTranslations simply call getAuthTranslations("auth.errors", locale) and getAuthTranslations("auth.security", locale) respectively so no duplicated try/catch blocks remain.tests/security/login-abuse-policy.test.ts (1)
80-138: 缺少"IP 已被封锁时提供 key 参数"场景的测试用例
check(ip, key?)在 IP scope 已封锁时会提前返回ipDecision，不再检查 key scope（对应login-abuse-policy.ts第 80 行的if (!ipDecision.allowed || !key) { return ipDecision; }）。但当前测试中，"supports key-based throttling" 仅覆盖了 IP 正常 + key 封锁 的情形，未覆盖 IP 封锁 + 同时提供 key 的情形（应返回reason: "ip_rate_limited"，而非"key_rate_limited"）。建议补充如下测试：
it("IP blocked takes priority over key check", () => { const policy = new LoginAbusePolicy({ maxAttemptsPerIp: 1, maxAttemptsPerKey: 10, lockoutSeconds: 60, }); const ip = "10.0.0.1"; const key = "user@example.com"; policy.recordFailure(ip, key); const decision = policy.check(ip, key); expect(decision.allowed).toBe(false); expect(decision.reason).toBe("ip_rate_limited"); });同样建议补充
recordSuccess(ip, key)同时重置 key scope 的测试，以验证第 112~113 行的reset(ip, key)调用正确清除了 key 的跟踪记录。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/login-abuse-policy.test.ts` around lines 80 - 138, Add two tests to cover missing scenarios for LoginAbusePolicy: (1) verify that when an IP is rate-limited, calling check(ip, key) still returns the IP decision (reason "ip_rate_limited") rather than the key decision by creating a policy with maxAttemptsPerIp:1, recording one failure with both ip and key, then asserting check(ip, key) is blocked with reason "ip_rate_limited"; (2) verify recordSuccess(ip, key) resets both IP and key tracking by recording failures for both scopes, calling recordSuccess(ip, key) (which calls reset(ip, key)), and asserting subsequent check(...) for that key and/or ip returns { allowed: true }; use the existing methods recordFailure, recordSuccess, and check on the LoginAbusePolicy instance to implement these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@messages/ja/auth.json`:
- Around line 26-28: brand.tagline currently uses "統合" while form.description
uses "統一", causing inconsistent terminology on the same page; pick the correct
canonical term (either "統合" or "統一") based on zh-CN/EN source or product style,
then update the value of "brand.tagline" or "form.description" so both use the
same term; ensure you change the string only (preserve JSON structure) and
verify the keys "brand.tagline" and "form.description" match the chosen
terminology across the file.
In `@src/actions/providers.ts`:
- Around line 1859-1925: preimage currently captures all changedFields for every
matchedProviders before you apply type-specific filtering, so the undo can
restore fields that were never written; instead, build preimage only from the
actual fields that will be applied per provider. Concretely: move or recreate
the preimage population to occur after you decide which updates will be applied
per provider/type (use the same logic that creates providersByType and
filterRepositoryUpdatesByProviderType), and for each provider only record the
providerKey entries that exist in the filtered updates for that provider's
providerType (use PATCH_FIELD_TO_PROVIDER_KEY to map field names and
repositoryUpdates/filtered to decide which keys are applied). Ensure the object
saved to providerPatchUndoStore.set includes this filtered preimage so undo only
writes back fields that were actually updated.
- Around line 1661-1676: The current skipReason assignments in the compatibility
check (within the block using isClaudeOnly/isCodexOnly/isGeminiOnly and helpers
isClaudeProviderType/isCodexProviderType/isGeminiProviderType) set hard-coded
English messages; change these to structured reason codes instead (e.g. set
skipReason = "claude_only", "codex_only" or "gemini_only") so the preview
returns a code rather than a literal message; ensure the variable name used in
this function (skipReason) is assigned the code values and that any callers
(e.g. the preview payload consumed by provider-batch-preview-step.tsx) will
receive the code for UI-side i18n rendering.
In `@src/app/`[locale]/login/page.tsx:
- Line 34: Replace the hardcoded DEFAULT_SITE_TITLE string with an i18n key
lookup: remove the constant DEFAULT_SITE_TITLE and fetch the fallback title via
the app’s translation function inside the page component (e.g., useTranslation/t
or the project’s i18n helper) using a key like "site.defaultTitle"; add that key
to all locale resource files (zh-CN, zh-TW, en, ja, ru) so the API-empty
fallback displays localized text; ensure any place referencing
DEFAULT_SITE_TITLE now calls the translator (e.g., t('site.defaultTitle'))
during component initialization/rendering.
- Around line 208-224: The loading overlay currently uses role="dialog" and
aria-modal="true" (the div rendered when isLoading is true, containing Loader2
and the paragraph with aria-live) but has no focus management or focusable
controls, creating an accessibility trap; change the overlay to a non-modal
status region by removing role="dialog" and aria-modal="true" and instead use
role="status" (or rely on the existing aria-live on the paragraph) so screen
readers announce the login progress without requiring focus handling. Also
address the password visibility toggle (the button rendered with tabIndex={-1})
by removing the tabIndex restriction so keyboard users can reach the control
unless there is an explicit reason to keep it.
In `@src/app/api/auth/login/route.ts`:
- Around line 101-103: The function shouldIncludeFailureTaxonomy currently gates
inclusion of the errorCode field based on the presence of the
"x-forwarded-proto" header, making the API response shape dependent on
deployment topology; update the code so the API always emits an errorCode (or
remove the conditional entirely) instead of checking
request.headers.has("x-forwarded-proto"). Locate shouldIncludeFailureTaxonomy
and the response construction in route.ts (where error / errorCode are composed)
and change the logic to unconditionally include errorCode in the JSON error
response (or eliminate the helper and always set a stable error payload format)
so clients receive a consistent contract regardless of proxy headers.
- Around line 168-185: The rate-limit response is using the wrong translation
key; update the error message selection in the block that checks
loginPolicy.check(clientIp) (variables: decision, loginPolicy,
withAuthResponseHeaders, NextResponse, t) to use a semantically correct key such
as t?.("auth.errors.tooManyAttempts") or t?.("auth.errors.rateLimited") instead
of t?.("loginFailed"), and keep a sensible fallback like t?.("serverError") ??
"Too many attempts"; also ensure you add the corresponding translation entry
(auth.errors.tooManyAttempts or auth.errors.rateLimited) to the i18n resource
files.
- Around line 135-145: getLoginSessionStore 存在并发竞态：多个请求可能同时通过
sessionStoreInstance===null 并各自创建
RedisSessionStore，导致重复实例化；修复方式是在模块作用域引入一个用于创建实例的 Promise 缓存（例如
sessionStorePromise）并在 getLoginSessionStore 中先检查该 Promise，再在首次调用时赋值为
import("@/lib/auth-session-store/redis-session-store").then(m=>new
m.RedisSessionStore()), 其后 await 该 Promise 并将解析值存到
sessionStoreInstance（或直接返回解析值），确保只有一个 RedisSessionStore
被构造；引用符号：getLoginSessionStore、sessionStoreInstance、RedisSessionStore、"@/lib/auth-session-store/redis-session-store"。
In `@src/app/api/auth/logout/route.ts`:
- Around line 62-74: The logout currently calls resolveAuthCookieToken() and
passes that value to store.revoke(), but in "dual" mode the cookie contains the
API key (not the opaque session ID) so revoke fails; update the logic so that
when mode === "dual" you either (A) change the login flow to write the opaque
session ID into the auth cookie instead of the API key, or (B) keep login as-is
and, in the logout path, detect dual mode and perform a reverse lookup: take the
API key returned by resolveAuthCookieToken(), query the session store (or Redis)
to find the corresponding opaque session ID (e.g., via a new lookup method such
as getSessionIdByApiKey or store.findByApiKey), then call
store.revoke(opaqueSessionId); for non-dual modes continue using
resolveAuthCookieToken() result directly and call store.revoke(sessionId).
In `@src/repository/provider.ts`:
- Around line 968-1013: 在 BatchProviderUpdates 接口上加入一行注释，说明之所以没有包含
tpm、rpm、rpd、cc（Tokens Per Minute、Requests Per Minute、Requests Per
Day、并发连接数）是有意为之，因为这些字段是针对单个供应商的专属速率限额，不适合批量覆盖；引用相关符号以便定位：在声明
BatchProviderUpdates 的位置上添加注释，提及 updateProvider 和 updateProvidersBatch
接受/处理这些速率字段但 BatchProviderUpdates 刻意省略它们以避免批量操作造成不当覆盖。
---
Outside diff comments:
In `@src/actions/providers.ts`:
- Around line 3861-3863: The inline comment containing an emoji in the object
literal near the Response API fix must be cleaned up: in
src/actions/providers.ts locate the object with properties type: "message" and
role: "user" (the comment that currently reads "⭐ 修复 `#189`: Response API 要求 input
数组中的每个元素必须包含 type 字段") and remove the emoji character(s) so the comment reads
plain text (e.g., "修复 `#189`: Response API 要求 input 数组中的每个元素必须包含 type 字段") or
delete the comment entirely to comply with the no-emoji rule.
---
Duplicate comments:
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx:
- Around line 394-396: The conditional rendering for the 1M Context Window
missing "claude-auth" was fixed; ensure the SmartInputWrapper rendering
condition for the 1M Context Window (the expression that uses providerType and
isBatch) includes providerType === "claude-auth" alongside "claude" and isBatch,
matching the Anthropic Overrides check elsewhere, so update the conditional that
controls SmartInputWrapper to: (providerType === "claude" || providerType ===
"claude-auth" || isBatch).
In `@src/app/`[locale]/settings/providers/_components/thinking-budget-editor.tsx:
- Around line 39-41: handleInputChange currently should forward the raw input
string instead of converting empty strings to "inherit"; update the handler
(handleInputChange) to call onChange(e.target.value) and remove any legacy logic
that maps empty values to "inherit" (and remove related conditional branches or
constants tied to that behavior) so clearing the input no longer hides the
control; ensure any callers of onChange and unit/interaction tests expect the
raw empty string rather than "inherit".
In `@src/app/api/auth/logout/route.ts`:
- Around line 1-16: This route uses NextRequest/NextResponse but must be
converted to Hono-style handlers; replace the Next.js handler with a Hono
app/route handler (use Hono Context for request/response) and update how you
read/write cookies and headers: call createCsrfOriginGuard (csrfGuard) against
the Hono request, obtain the auth token via getAuthCookie from the Hono
request/cookie API, determine session mode with getSessionTokenMode, clear the
cookie using clearAuthCookie applied to the Hono response, and apply
withAuthResponseHeaders to the final Hono Response; export the Hono route (e.g.,
app.post or app.route) instead of exporting a Next.js handler so the file
matches the other routes using Hono.
---
Nitpick comments:
In `@src/app/`[locale]/login/loading.tsx:
- Line 4: The component LoginLoading is currently a default export; change it to
a named export by exporting the LoginLoading function as a named export (export
function LoginLoading...) and remove the default export usage; update any import
sites that rely on the default import to use the named import { LoginLoading }
instead. If you intend to apply this convention project-wide, apply the same
change to all other loading.tsx files (e.g., dashboard/loading.tsx,
dashboard/quotas/loading.tsx, my-usage/loading.tsx) and update their consumers
accordingly to maintain consistency.
In
`@src/app/`[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx:
- Around line 36-47: Several imports (FormTabNav, ProviderFormProvider,
useProviderForm, BasicInfoSection, LimitsSection, NetworkSection,
RoutingSection, TestingSection) use relative paths; change them to use the
project path alias (start with "@/") so they reference the same modules via the
src alias instead of "../...". Update the import statements that currently
import those symbols from "../forms/..." to import them from the corresponding
"@/..." alias equivalents, keeping the same exported names and ensuring module
resolution/build passes.
In `@src/app/`[locale]/settings/providers/_components/thinking-budget-editor.tsx:
- Line 35: The hard-coded magic strings for the thinking budget ("10240" default
and "32000" max, and the min "1024" used in attributes) should be extracted into
named constants to centralize semantics and avoid duplication; update the
onChange("10240") call, the input's min/max attributes, and any other
occurrences (lines ~43-45) to use constants like DEFAULT_CUSTOM_THINKING_BUDGET,
MIN_THINKING_BUDGET, and MAX_THINKING_BUDGET (choose string or numeric types
consistently with the input handling and parse/format where needed) so future
Anthropic limit changes require a single update and the code is clearer.
In `@src/app/api/auth/login/route.ts`:
- Around line 21-27: csrfGuard and loginPolicy are module-level singletons
holding in-process state; migrate login/CSRF state to a shared Redis-backed
store to support multi-process/edge deployments: modify LoginAbusePolicy to
accept a Redis-backed counter/TTL interface (or replace with a distributed
limiter library) and update usages where createCsrfOriginGuard and csrfGuard
rely on in-memory data to use the RedisSessionStore (or a Redis-based
rate-limiter) so counters/blocks are consistent across instances; ensure
constructor/factory for LoginAbusePolicy and the CSRF guard accept an injected
redis client or adapter and fallback to the existing in-memory behavior for
local dev/tests.
- Around line 53-93: Extract the shared try/catch/fallback logic into a single
helper (e.g. getAuthTranslations(namespace: string, locale: Locale)) that calls
getTranslations({ locale, namespace }), falls back to getTranslations({ locale:
defaultLocale, namespace }) on error, logs using logger.warn/logger.error
(include locale, defaultLocale, namespace and the error message), and returns
the same nullable translation result; then have getAuthErrorTranslations and
getAuthSecurityTranslations simply call getAuthTranslations("auth.errors",
locale) and getAuthTranslations("auth.security", locale) respectively so no
duplicated try/catch blocks remain.
In `@src/app/api/auth/logout/route.ts`:
- Around line 18-28: getLogoutSessionStore currently has a race: multiple
concurrent callers can pass the null check and each call will dynamically import
and construct a RedisSessionStore, leaking instances; also the inline dynamic
import type annotation is hard to read. Fix by making sessionStoreInstance hold
a Promise that is set on first call (a "promise singleton") so concurrent
callers await the same initialization Promise and only one RedisSessionStore is
constructed, e.g. replace the nullable instance with a
Promise<RedisSessionStore> | null and ensure getLogoutSessionStore sets that
Promise before awaiting the import/constructor; also move the type import to the
top with an `import type { RedisSessionStore } from
"@/lib/auth-session-store/redis-session-store"` and use that type for the
Promise and return signature to improve readability.
In `@src/lib/providers/undo-store.ts`:
- Around line 23-31: ConsumeUndoResult declares a possible code "UNDO_CONFLICT"
but the implementation of consumeUndoToken never returns it, causing confusion;
either narrow the signature or document intent. Option A (recommended): remove
"UNDO_CONFLICT" from ConsumeUndoResult and export a narrower result used by
consumeUndoToken (e.g., only ok:true|{ok:false;code:"UNDO_EXPIRED"}) then let
callers in src/actions/providers.ts merge that with their conflict-detection to
produce the full ConsumeUndoResult. Option B: add a clear comment above
ConsumeUndoResult and on consumeUndoToken explaining that consumeUndoToken only
returns UNDO_EXPIRED and that UNDO_CONFLICT is created by upstream callers
(e.g., in src/actions/providers.ts) after additional validation.
In `@src/lib/security/login-abuse-policy.ts`:
- Around line 157-167: The branch in checkScope that handles record.count >=
threshold is defensive/unreachable under current logic because
recordFailureForScope always sets lockedUntil when count reaches the threshold;
add a clear comment above that branch explaining it is an intentional
safety/defensive fallback to preserve behavior if invariants change, reference
the related functions/variables (checkScope, recordFailureForScope, attempts,
scopeKey, lockedUntil) and state that it ensures locked entries survive LRU
eviction and prevents accidental removal by future maintainers.
- Around line 27-28: The LoginAbusePolicy currently stores AttemptRecord
instances in an in-memory Map (and relies on MAX_TRACKED_ENTRIES and
SWEEP_INTERVAL_MS) which won’t work across multiple Node.js processes; replace
the per-process Map usage in LoginAbusePolicy with Redis-backed counters/sets
using the repo’s existing Redis client, implement a distributed sliding-window
rate limit (use INCR with EXPIRE for fixed-window or a small Lua script /
sorted-set approach for true sliding-window) keyed by the same identifiers you
currently use for AttemptRecord, remove or make
MAX_TRACKED_ENTRIES/SWEEP_INTERVAL_MS local-cache fallbacks only, and ensure
checks and increments are atomic so limits are enforced consistently across
pods.
In `@src/repository/provider.ts`:
- Around line 1355-1381: restoreProvidersBatch performs sequential multi-step DB
work for each id inside one long transaction (calling
restoreProviderInTransaction repeatedly), which can cause long-held locks for
large batches; update the function to (1) add a clear doc comment noting the 60s
TTL constraint (PROVIDER_RESTORE_MAX_AGE_MS) and expected small batch sizes, and
(2) emit a warning log when uniqueIds.length exceeds a safe threshold (e.g., >
10) before starting the transaction so operators are alerted to potential long
transactions; reference restoreProvidersBatch, uniqueIds, restoredCount, and
PROVIDER_RESTORE_MAX_AGE_MS in the message.
In `@tests/security/login-abuse-policy.test.ts`:
- Around line 80-138: Add two tests to cover missing scenarios for
LoginAbusePolicy: (1) verify that when an IP is rate-limited, calling check(ip,
key) still returns the IP decision (reason "ip_rate_limited") rather than the
key decision by creating a policy with maxAttemptsPerIp:1, recording one failure
with both ip and key, then asserting check(ip, key) is blocked with reason
"ip_rate_limited"; (2) verify recordSuccess(ip, key) resets both IP and key
tracking by recording failures for both scopes, calling recordSuccess(ip, key)
(which calls reset(ip, key)), and asserting subsequent check(...) for that key
and/or ip returns { allowed: true }; use the existing methods recordFailure,
recordSuccess, and check on the LoginAbusePolicy instance to implement these
tests.
Comment on lines
+26
to
+28
| "brand": { | ||
| "tagline": "統合API管理コンソール" | ||
| }, |
There was a problem hiding this comment.
同一页面内术语不一致：brand.tagline 与 form.description 用词不统一。
brand.tagline（第 27 行）使用「統合」（tōgō，统合/集成），而同文件 form.description（第 4 行）使用「統一」（tōitsu，统一/单一化）来指代同一个控制台。两词在日语中含义有所不同，用户在同一登录页面会看到两种不同说法，可能产生困惑。对比 zh-CN/auth.json，两处均使用「统一」，建议日语保持一致。
🔧 建议修复（以统一使用「統合」为例，或参照英文原文确认规范术语后再定）
- "description": "API キーを使用して統一管理コンソールにアクセスします",
+ "description": "API キーを使用して統合管理コンソールにアクセスします",或统一改为「統一」：
- "tagline": "統合API管理コンソール"
+ "tagline": "統一API管理コンソール"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@messages/ja/auth.json` around lines 26 - 28, brand.tagline currently uses
"統合" while form.description uses "統一", causing inconsistent terminology on the
same page; pick the correct canonical term (either "統合" or "統一") based on
zh-CN/EN source or product style, then update the value of "brand.tagline" or
"form.description" so both use the same term; ensure you change the string only
(preserve JSON structure) and verify the keys "brand.tagline" and
"form.description" match the chosen terminology across the file.
Comment on lines
+1661
to
+1676
| const isClaudeOnly = CLAUDE_ONLY_FIELDS.has(field); | ||
| const isCodexOnly = CODEX_ONLY_FIELDS.has(field); | ||
| const isGeminiOnly = GEMINI_ONLY_FIELDS.has(field); | ||
|
|
||
| let isCompatible = true; | ||
| let skipReason = ""; | ||
| if (isClaudeOnly && !isClaudeProviderType(provider.providerType)) { | ||
| isCompatible = false; | ||
| skipReason = `Field "${field}" is only applicable to claude/claude-auth providers`; | ||
| } else if (isCodexOnly && !isCodexProviderType(provider.providerType)) { | ||
| isCompatible = false; | ||
| skipReason = `Field "${field}" is only applicable to codex providers`; | ||
| } else if (isGeminiOnly && !isGeminiProviderType(provider.providerType)) { | ||
| isCompatible = false; | ||
| skipReason = `Field "${field}" is only applicable to gemini/gemini-cli providers`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "skipReason|fieldSkipped" -g "**/*.{ts,tsx,js,jsx}" -C2Repository: ding113/claude-code-hub
Length of output: 4889
skipReason 包含硬编码的英文提示，违反多语言要求
在 src/actions/providers.ts 行 1669、1672、1675 的三处 skipReason 赋值均使用了硬编码英文字符串。这些字符串经由 preview 结果返回到前端，在 provider-batch-preview-step.tsx 中被传入 i18n 函数 t("preview.fieldSkipped", { reason: row.skipReason })，直接暴露给用户而未进行翻译。
建议改为返回 reasonCode（如 "claude_only", "codex_only", "gemini_only"），由 UI 侧根据 reasonCode 使用 i18n 文案渲染。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/providers.ts` around lines 1661 - 1676, The current skipReason
assignments in the compatibility check (within the block using
isClaudeOnly/isCodexOnly/isGeminiOnly and helpers
isClaudeProviderType/isCodexProviderType/isGeminiProviderType) set hard-coded
English messages; change these to structured reason codes instead (e.g. set
skipReason = "claude_only", "codex_only" or "gemini_only") so the preview
returns a code rather than a literal message; ensure the variable name used in
this function (skipReason) is assigned the code values and that any callers
(e.g. the preview payload consumed by provider-batch-preview-step.tsx) will
receive the code for UI-side i18n rendering.
Comment on lines
+1859
to
+1925
| const allProviders = await findAllProvidersFresh(); | ||
| const effectiveIdSet = new Set(effectiveProviderIds); | ||
| const matchedProviders = allProviders.filter((p) => effectiveIdSet.has(p.id)); | ||
| const changedFields = getChangedPatchFields(normalizedPatch.data); | ||
| const preimage: Record<number, Record<string, unknown>> = {}; | ||
| for (const provider of matchedProviders) { | ||
| const fieldValues: Record<string, unknown> = {}; | ||
| for (const field of changedFields) { | ||
| const providerKey = PATCH_FIELD_TO_PROVIDER_KEY[field]; | ||
| fieldValues[providerKey] = provider[providerKey]; | ||
| } | ||
| preimage[provider.id] = fieldValues; | ||
| } | ||
|
|
||
| const repositoryUpdates = mapApplyUpdatesToRepositoryFormat(updatesResult.data); | ||
|
|
||
| const hasTypeSpecificFields = changedFields.some( | ||
| (f) => CLAUDE_ONLY_FIELDS.has(f) || CODEX_ONLY_FIELDS.has(f) || GEMINI_ONLY_FIELDS.has(f) | ||
| ); | ||
|
|
||
| let dbUpdatedCount: number; | ||
| if (!hasTypeSpecificFields) { | ||
| dbUpdatedCount = await updateProvidersBatch(effectiveProviderIds, repositoryUpdates); | ||
| } else { | ||
| const providersByType = new Map<string, number[]>(); | ||
| for (const provider of matchedProviders) { | ||
| const type = provider.providerType; | ||
| if (!providersByType.has(type)) providersByType.set(type, []); | ||
| providersByType.get(type)!.push(provider.id); | ||
| } | ||
|
|
||
| dbUpdatedCount = 0; | ||
| for (const [type, ids] of providersByType) { | ||
| const filtered = filterRepositoryUpdatesByProviderType(repositoryUpdates, type); | ||
| if (Object.keys(filtered).length > 0) { | ||
| dbUpdatedCount += await updateProvidersBatch(ids, filtered); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| await publishProviderCacheInvalidation(); | ||
|
|
||
| const appliedAt = new Date(nowMs).toISOString(); | ||
| const undoToken = createProviderPatchUndoToken(); | ||
| const undoExpiresAtMs = nowMs + PROVIDER_PATCH_UNDO_TTL_SECONDS * 1000; | ||
|
|
||
| const applyResult: ApplyProviderBatchPatchResult = { | ||
| operationId: createProviderPatchOperationId(), | ||
| appliedAt, | ||
| updatedCount: dbUpdatedCount, | ||
| undoToken, | ||
| undoExpiresAt: new Date(undoExpiresAtMs).toISOString(), | ||
| }; | ||
|
|
||
| snapshot.applied = true; | ||
| if (parsed.data.idempotencyKey) { | ||
| snapshot.appliedResultByIdempotencyKey[parsed.data.idempotencyKey] = applyResult; | ||
| } | ||
| await providerBatchPatchPreviewStore.set(parsed.data.previewToken, snapshot); | ||
|
|
||
| await providerPatchUndoStore.set(undoToken, { | ||
| undoToken, | ||
| operationId: applyResult.operationId, | ||
| providerIds: effectiveProviderIds, | ||
| preimage, | ||
| patch: normalizedPatch.data, | ||
| }); |
There was a problem hiding this comment.
预览/应用按类型过滤后，preimage 仍包含被过滤字段，撤销可能覆盖无关修改。
preimage 是在按类型过滤前按 changedFields 全量采集的；若某些字段因 providerType 被过滤未写入 DB，撤销时仍会把这些字段回写为旧值，可能覆盖后续独立修改。建议 preimage 仅记录“实际会写入”的字段集合。
修复思路示例
- for (const field of changedFields) {
+ const applicableFields = getApplicableFieldsForProvider(provider, changedFields);
+ for (const field of applicableFields) {
const providerKey = PATCH_FIELD_TO_PROVIDER_KEY[field];
fieldValues[providerKey] = provider[providerKey];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/providers.ts` around lines 1859 - 1925, preimage currently
captures all changedFields for every matchedProviders before you apply
type-specific filtering, so the undo can restore fields that were never written;
instead, build preimage only from the actual fields that will be applied per
provider. Concretely: move or recreate the preimage population to occur after
you decide which updates will be applied per provider/type (use the same logic
that creates providersByType and filterRepositoryUpdatesByProviderType), and for
each provider only record the providerKey entries that exist in the filtered
updates for that provider's providerType (use PATCH_FIELD_TO_PROVIDER_KEY to map
field names and repositoryUpdates/filtered to decide which keys are applied).
Ensure the object saved to providerPatchUndoStore.set includes this filtered
preimage so undo only writes back fields that were actually updated.
| hasUpdate: boolean; | ||
| } | ||
|
|
||
| const DEFAULT_SITE_TITLE = "Claude Code Hub"; |
There was a problem hiding this comment.
DEFAULT_SITE_TITLE 硬编码字符串违反 i18n 规范。
"Claude Code Hub" 作为站点标题兜底值会在 API 返回空值时直接展示给用户，属于用户可见文本，应通过 i18n 键管理。
🛡️ 建议修复
-const DEFAULT_SITE_TITLE = "Claude Code Hub";
+// 在 messages/{locale}/customs.json（或 auth.json）中添加 brand.defaultTitle 键然后在组件初始化时：
-const [siteTitle, setSiteTitle] = useState(DEFAULT_SITE_TITLE);
+const [siteTitle, setSiteTitle] = useState(() => tCustoms("brand.defaultTitle"));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/`[locale]/login/page.tsx at line 34, Replace the hardcoded
DEFAULT_SITE_TITLE string with an i18n key lookup: remove the constant
DEFAULT_SITE_TITLE and fetch the fallback title via the app’s translation
function inside the page component (e.g., useTranslation/t or the project’s i18n
helper) using a key like "site.defaultTitle"; add that key to all locale
resource files (zh-CN, zh-TW, en, ja, ru) so the API-empty fallback displays
localized text; ensure any place referencing DEFAULT_SITE_TITLE now calls the
translator (e.g., t('site.defaultTitle')) during component
initialization/rendering.
Comment on lines
+208
to
+224
| {isLoading && ( | ||
| <div | ||
| data-testid="loading-overlay" | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label={t("login.loggingIn")} | ||
| className="fixed inset-0 z-50 flex flex-col items-center justify-center bg-background/80 backdrop-blur-sm transition-all duration-200" | ||
| > | ||
| <Loader2 className="h-12 w-12 animate-spin motion-reduce:animate-none text-primary" /> | ||
| <p | ||
| className="mt-4 text-lg font-medium text-muted-foreground" | ||
| role="status" | ||
| aria-live="polite" | ||
| > | ||
| {t("login.loggingIn")} | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
role="dialog" 配合 aria-modal="true" 但缺少焦点管理——无障碍重大缺陷。
WAI-ARIA 规范要求：当 role="dialog" 出现时，焦点必须被移入对话框内的可聚焦元素；aria-modal="true" 会通知辅助技术将对话框外的内容视为不可交互。当前加载遮罩中没有任何可聚焦元素，也没有焦点转移代码。结果是：屏幕阅读器用户的焦点停留在已禁用的提交按钮上，部分屏幕阅读器会因 aria-modal 而将页面其余部分标记为不可访问，导致用户陷入无法导航的状态。
加载遮罩不是一个真正的对话框——应改用 role="status" 或依赖已有的 aria-live="polite" 段落来播报状态。
🛡️ 建议修复
- <div
- data-testid="loading-overlay"
- role="dialog"
- aria-modal="true"
- aria-label={t("login.loggingIn")}
- className="fixed inset-0 z-50 flex flex-col items-center justify-center bg-background/80 backdrop-blur-sm transition-all duration-200"
- >
+ <div
+ data-testid="loading-overlay"
+ aria-live="assertive"
+ aria-busy="true"
+ className="fixed inset-0 z-50 flex flex-col items-center justify-center bg-background/80 backdrop-blur-sm transition-all duration-200"
+ >附：第 358 行密码可见性切换按钮的 tabIndex={-1} 使仅使用键盘的视力正常用户无法通过 Tab 访问该控件，可考虑移除 tabIndex={-1} 以提升键盘可访问性（或至少确认此为有意设计决策）。
📝 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.
Suggested change
| {isLoading && ( | |
| <div | |
| data-testid="loading-overlay" | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label={t("login.loggingIn")} | |
| className="fixed inset-0 z-50 flex flex-col items-center justify-center bg-background/80 backdrop-blur-sm transition-all duration-200" | |
| > | |
| <Loader2 className="h-12 w-12 animate-spin motion-reduce:animate-none text-primary" /> | |
| <p | |
| className="mt-4 text-lg font-medium text-muted-foreground" | |
| role="status" | |
| aria-live="polite" | |
| > | |
| {t("login.loggingIn")} | |
| </p> | |
| </div> | |
| {isLoading && ( | |
| <div | |
| data-testid="loading-overlay" | |
| aria-live="assertive" | |
| aria-busy="true" | |
| className="fixed inset-0 z-50 flex flex-col items-center justify-center bg-background/80 backdrop-blur-sm transition-all duration-200" | |
| > | |
| <Loader2 className="h-12 w-12 animate-spin motion-reduce:animate-none text-primary" /> | |
| <p | |
| className="mt-4 text-lg font-medium text-muted-foreground" | |
| role="status" | |
| aria-live="polite" | |
| > | |
| {t("login.loggingIn")} | |
| </p> | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/login/page.tsx around lines 208 - 224, The loading overlay
currently uses role="dialog" and aria-modal="true" (the div rendered when
isLoading is true, containing Loader2 and the paragraph with aria-live) but has
no focus management or focusable controls, creating an accessibility trap;
change the overlay to a non-modal status region by removing role="dialog" and
aria-modal="true" and instead use role="status" (or rely on the existing
aria-live on the paragraph) so screen readers announce the login progress
without requiring focus handling. Also address the password visibility toggle
(the button rendered with tabIndex={-1}) by removing the tabIndex restriction so
keyboard users can reach the control unless there is an explicit reason to keep
it.
Comment on lines
+101
to
+103
| function shouldIncludeFailureTaxonomy(request: NextRequest): boolean { | ||
| return request.headers.has("x-forwarded-proto"); | ||
| } |
There was a problem hiding this comment.
shouldIncludeFailureTaxonomy 导致 API 响应结构依赖部署拓扑
function shouldIncludeFailureTaxonomy(request: NextRequest): boolean {
return request.headers.has("x-forwarded-proto");
}该函数仅依据代理头是否存在来决定是否在响应中包含 errorCode 字段，导致：
- 有反向代理时（生产环境）：响应包含
{ error, errorCode } - 无反向代理时（本地开发或直连场景）：响应仅包含
{ error }
这对客户端 SDK 或集成方来说是不稳定的契约——errorCode 字段的存在与否取决于部署配置而非 API 设计本身。建议始终包含 errorCode 字段，或彻底移除该条件分支。
🤖 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 101 - 103, The function
shouldIncludeFailureTaxonomy currently gates inclusion of the errorCode field
based on the presence of the "x-forwarded-proto" header, making the API response
shape dependent on deployment topology; update the code so the API always emits
an errorCode (or remove the conditional entirely) instead of checking
request.headers.has("x-forwarded-proto"). Locate shouldIncludeFailureTaxonomy
and the response construction in route.ts (where error / errorCode are composed)
and change the logic to unconditionally include errorCode in the JSON error
response (or eliminate the helper and always set a stable error payload format)
so clients receive a consistent contract regardless of proxy headers.
Comment on lines
+135
to
+145
| let sessionStoreInstance: | ||
| | import("@/lib/auth-session-store/redis-session-store").RedisSessionStore | ||
| | null = null; | ||
|
|
||
| async function getLoginSessionStore() { | ||
| if (!sessionStoreInstance) { | ||
| const { RedisSessionStore } = await import("@/lib/auth-session-store/redis-session-store"); | ||
| sessionStoreInstance = new RedisSessionStore(); | ||
| } | ||
| return sessionStoreInstance; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*redis-session-store*" | head -20Repository: ding113/claude-code-hub
Length of output: 118
🏁 Script executed:
fd -type f -name "*session*store*" | grep -E "\.(ts|tsx|js|jsx)$"Repository: ding113/claude-code-hub
Length of output: 237
🏁 Script executed:
cat -n src/app/api/auth/login/route.ts | sed -n '135,150p'Repository: ding113/claude-code-hub
Length of output: 747
🏁 Script executed:
cat -n ./src/lib/auth-session-store/redis-session-store.tsRepository: ding113/claude-code-hub
Length of output: 8053
🏁 Script executed:
wc -l ./src/lib/auth-session-store/redis-session-store.tsRepository: ding113/claude-code-hub
Length of output: 122
🏁 Script executed:
find . -type f -name "redis.ts" -o -name "redis.js" | grep -E "lib.*redis"Repository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
cat -n src/lib/redis.ts | head -80Repository: ding113/claude-code-hub
Length of output: 115
🏁 Script executed:
find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "getRedisClient" | head -10Repository: ding113/claude-code-hub
Length of output: 450
🏁 Script executed:
fd "redis" -e ts -e tsx | grep -v node_modulesRepository: ding113/claude-code-hub
Length of output: 249
🏁 Script executed:
cat -n src/lib/redis/redis-kv-store.ts | head -100Repository: ding113/claude-code-hub
Length of output: 3482
🏁 Script executed:
find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "export.*getRedisClient" | head -5Repository: ding113/claude-code-hub
Length of output: 117
🏁 Script executed:
cat -n src/lib/redis/client.tsRepository: ding113/claude-code-hub
Length of output: 6221
🏁 Script executed:
cat -n src/lib/redis/index.tsRepository: ding113/claude-code-hub
Length of output: 385
getLoginSessionStore() 存在竞态条件，会导致多个 RedisSessionStore 实例被创建
当两个并发请求同时检查 sessionStoreInstance === null 时，会各自通过条件判断进入 if 分支，分别执行 await import() 并创建独立的 RedisSessionStore 实例。第二个赋值会覆盖第一个，导致第一个实例被遗弃。虽然 RedisSessionStore 本身不直接管理 Redis 连接（连接由全局的 getRedisClient() 单例管理），但创建多个实例仍然是不必要的浪费，且违反单例模式的设计意图。
建议用 Promise 缓存代替实例缓存：
建议修复
-let sessionStoreInstance:
- | import("@/lib/auth-session-store/redis-session-store").RedisSessionStore
- | null = null;
+let sessionStorePromise:
+ | Promise<import("@/lib/auth-session-store/redis-session-store").RedisSessionStore>
+ | null = null;
async function getLoginSessionStore() {
- if (!sessionStoreInstance) {
- const { RedisSessionStore } = await import("@/lib/auth-session-store/redis-session-store");
- sessionStoreInstance = new RedisSessionStore();
+ if (!sessionStorePromise) {
+ sessionStorePromise = import("@/lib/auth-session-store/redis-session-store").then(
+ ({ RedisSessionStore }) => new RedisSessionStore(),
+ );
}
- return sessionStoreInstance;
+ return sessionStorePromise;
}🤖 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 135 - 145, getLoginSessionStore
存在并发竞态：多个请求可能同时通过 sessionStoreInstance===null 并各自创建
RedisSessionStore，导致重复实例化；修复方式是在模块作用域引入一个用于创建实例的 Promise 缓存（例如
sessionStorePromise）并在 getLoginSessionStore 中先检查该 Promise，再在首次调用时赋值为
import("@/lib/auth-session-store/redis-session-store").then(m=>new
m.RedisSessionStore()), 其后 await 该 Promise 并将解析值存到
sessionStoreInstance（或直接返回解析值），确保只有一个 RedisSessionStore
被构造；引用符号：getLoginSessionStore、sessionStoreInstance、RedisSessionStore、"@/lib/auth-session-store/redis-session-store"。
Comment on lines
+168
to
+185
| 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.
限流响应（429）使用了错误的翻译键 loginFailed
t?.("loginFailed") ?? t?.("serverError") ?? "Too many attempts" 中，loginFailed 键对应的文案通常是"登录失败"，用于认证失败场景，不适合描述"请求频率过高"的限流情况。"Too many attempts" 仅在两个翻译键都缺失时才作为兜底出现。
建议在 auth.errors 命名空间中添加专用的 tooManyAttempts 或 rateLimited 翻译键：
建议修复：使用语义匹配的翻译键
-error: t?.("loginFailed") ?? t?.("serverError") ?? "Too many attempts",
+error: t?.("tooManyAttempts") ?? t?.("loginFailed") ?? "Too many attempts",🤖 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 168 - 185, The rate-limit
response is using the wrong translation key; update the error message selection
in the block that checks loginPolicy.check(clientIp) (variables: decision,
loginPolicy, withAuthResponseHeaders, NextResponse, t) to use a semantically
correct key such as t?.("auth.errors.tooManyAttempts") or
t?.("auth.errors.rateLimited") instead of t?.("loginFailed"), and keep a
sensible fallback like t?.("serverError") ?? "Too many attempts"; also ensure
you add the corresponding translation entry (auth.errors.tooManyAttempts or
auth.errors.rateLimited) to the i18n resource files.
Comment on lines
+62
to
+74
| if (mode !== "legacy") { | ||
| try { | ||
| const sessionId = await resolveAuthCookieToken(); | ||
| if (sessionId) { | ||
| const store = await getLogoutSessionStore(); | ||
| await store.revoke(sessionId); | ||
| } | ||
| } catch (error) { | ||
| logger.warn("[AuthLogout] Failed to revoke opaque session during logout", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for dual mode, SessionTokenMode, and AUTH_COOKIE usage
rg -n "dual|SessionTokenMode|setAuthCookie|AUTH_COOKIE" --type ts -C 4 src/lib/auth.tsRepository: ding113/claude-code-hub
Length of output: 3254
🏁 Script executed:
# Also check the logout route itself to understand the context better
cat -n src/app/api/auth/logout/route.ts | head -80Repository: ding113/claude-code-hub
Length of output: 2908
🏁 Script executed:
# Look for where mode is determined and how it affects session handling
rg -n "mode.*dual|dual.*mode" --type ts -C 3 src/Repository: ding113/claude-code-hub
Length of output: 1845
🏁 Script executed:
# Check how createOpaqueSession works and how the opaque session ID is handled
rg -n "createOpaqueSession" --type ts -C 5 src/Repository: ding113/claude-code-hub
Length of output: 2199
🏁 Script executed:
# Look at the RedisSessionStore.revoke() implementation to understand its behavior
cat -n src/lib/auth-session-store/redis-session-store.ts | head -100Repository: ding113/claude-code-hub
Length of output: 3815
🏁 Script executed:
# Check the login route more carefully to see how opaque sessions are created
rg -n "createOpaqueSession|setAuthCookie" --type ts -B 2 -A 8 src/app/api/auth/login/route.tsRepository: ding113/claude-code-hub
Length of output: 1910
🏁 Script executed:
# Check what createOpaqueSession returns - especially the sessionId structure
rg -n "class RedisSessionStore|async create" --type ts -A 15 src/lib/auth-session-store/redis-session-store.ts | head -60Repository: ding113/claude-code-hub
Length of output: 1306
🏁 Script executed:
# Check what store.create() returns to understand the sessionId format
rg -n "return.*sessionId|interface SessionData" --type ts -B 2 -A 5 src/lib/auth-session-store/Repository: ding113/claude-code-hub
Length of output: 1076
🏁 Script executed:
# Check the revoke() method implementation in RedisSessionStore
rg -n "async revoke|revoke\(" --type ts -A 10 src/lib/auth-session-store/redis-session-store.tsRepository: ding113/claude-code-hub
Length of output: 814
🏁 Script executed:
# Also check if there's a get() method to see how session lookup works
rg -n "async get\|async revoke" --type ts -A 8 src/lib/auth-session-store/redis-session-store.tsRepository: ding113/claude-code-hub
Length of output: 49
修复 "dual" 模式下的会话撤销逻辑
当前代码在 "dual" 模式下存在会话撤销缺陷：登录时 cookie 设置为 API 密钥（行 247），而不透明会话的 ID 仅存储在 Redis 中。logout 时调用 store.revoke(sessionId) 传入的是 API 密钥而非不透明 session ID，导致 Redis 查找失败，会话无法被撤销，只能等待自然过期。
在 "opaque" 模式下，cookie 存储的是实际的 opaque sessionId（行 258），revoke 调用能正确匹配 Redis key。建议：
- 修改 dual 模式在登录时将不透明 sessionId 存储到 cookie，或
- 在 logout 时根据 mode 采用不同的撤销策略（dual 模式下需要从 API 密钥反向查找对应的不透明 session ID）
🤖 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 62 - 74, The logout currently
calls resolveAuthCookieToken() and passes that value to store.revoke(), but in
"dual" mode the cookie contains the API key (not the opaque session ID) so
revoke fails; update the logic so that when mode === "dual" you either (A)
change the login flow to write the opaque session ID into the auth cookie
instead of the API key, or (B) keep login as-is and, in the logout path, detect
dual mode and perform a reverse lookup: take the API key returned by
resolveAuthCookieToken(), query the session store (or Redis) to find the
corresponding opaque session ID (e.g., via a new lookup method such as
getSessionIdByApiKey or store.findByApiKey), then call
store.revoke(opaqueSessionId); for non-dual modes continue using
resolveAuthCookieToken() result directly and call store.revoke(sessionId).
Comment on lines
968
to
1013
| export interface BatchProviderUpdates { | ||
| isEnabled?: boolean; | ||
| priority?: number; | ||
| weight?: number; | ||
| costMultiplier?: string; | ||
| groupTag?: string | null; | ||
| modelRedirects?: Record<string, string> | null; | ||
| allowedModels?: string[] | null; | ||
| anthropicThinkingBudgetPreference?: string | null; | ||
| anthropicAdaptiveThinking?: AnthropicAdaptiveThinkingConfig | null; | ||
| // Routing | ||
| preserveClientIp?: boolean; | ||
| groupPriorities?: Record<string, number> | null; | ||
| cacheTtlPreference?: string | null; | ||
| swapCacheTtlBilling?: boolean; | ||
| context1mPreference?: string | null; | ||
| codexReasoningEffortPreference?: string | null; | ||
| codexReasoningSummaryPreference?: string | null; | ||
| codexTextVerbosityPreference?: string | null; | ||
| codexParallelToolCallsPreference?: string | null; | ||
| anthropicMaxTokensPreference?: string | null; | ||
| geminiGoogleSearchPreference?: string | null; | ||
| // Rate Limit | ||
| limit5hUsd?: string | null; | ||
| limitDailyUsd?: string | null; | ||
| dailyResetMode?: string; | ||
| dailyResetTime?: string; | ||
| limitWeeklyUsd?: string | null; | ||
| limitMonthlyUsd?: string | null; | ||
| limitTotalUsd?: string | null; | ||
| limitConcurrentSessions?: number; | ||
| // Circuit Breaker | ||
| circuitBreakerFailureThreshold?: number; | ||
| circuitBreakerOpenDuration?: number; | ||
| circuitBreakerHalfOpenSuccessThreshold?: number; | ||
| maxRetryAttempts?: number | null; | ||
| // Network | ||
| proxyUrl?: string | null; | ||
| proxyFallbackToDirect?: boolean; | ||
| firstByteTimeoutStreamingMs?: number; | ||
| streamingIdleTimeoutMs?: number; | ||
| requestTimeoutNonStreamingMs?: number; | ||
| // MCP | ||
| mcpPassthroughType?: string; | ||
| mcpPassthroughUrl?: string | null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the fields in createProvider (around line 221-224)
sed -n '215,235p' src/repository/provider.ts | cat -nRepository: ding113/claude-code-hub
Length of output: 1194
🏁 Script executed:
# Check the fields in updateProvider (around line 651-654)
sed -n '645,665p' src/repository/provider.ts | cat -nRepository: ding113/claude-code-hub
Length of output: 1314
🏁 Script executed:
# Check all occurrences of tpm, rpm, rpd, cc in the file
rg -n "(tpm|rpm|rpd|cc)" src/repository/provider.ts | head -40Repository: ding113/claude-code-hub
Length of output: 2045
🏁 Script executed:
# Check the BatchProviderUpdates interface definition (lines 968-1013)
sed -n '968,1013p' src/repository/provider.ts | cat -nRepository: ding113/claude-code-hub
Length of output: 1987
🏁 Script executed:
# Check updateProvidersBatch setClauses to see what fields are handled
sed -n '1042,1150p' src/repository/provider.ts | cat -nRepository: ding113/claude-code-hub
Length of output: 5232
在 BatchProviderUpdates 接口中补充注释，说明为何省略 tpm、rpm、rpd、cc 字段
updateProvider 支持 tpm、rpm、rpd、cc 字段（分别对应 Tokens Per Minute、Requests Per Minute、Requests Per Day 及并发连接数），但 BatchProviderUpdates 和 updateProvidersBatch 中均不包含这些字段。
由于这四个字段代表单个供应商的专属速率限额，不适合作为批量覆写操作的对象，其省略是合理的设计决策。但请在 BatchProviderUpdates 接口上添加注释明确说明这一点，以避免后续维护者疑惑。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/repository/provider.ts` around lines 968 - 1013, 在 BatchProviderUpdates
接口上加入一行注释，说明之所以没有包含 tpm、rpm、rpd、cc（Tokens Per Minute、Requests Per
Minute、Requests Per Day、并发连接数）是有意为之，因为这些字段是针对单个供应商的专属速率限额，不适合批量覆盖；引用相关符号以便定位：在声明
BatchProviderUpdates 的位置上添加注释，提及 updateProvider 和 updateProvidersBatch
接受/处理这些速率字段但 BatchProviderUpdates 刻意省略它们以避免批量操作造成不当覆盖。
… 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
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)
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>
Both thinking-budget-editor and adaptive-thinking-editor tests now use the same createRoot + happy-dom pattern, removing the broken dependency on @testing-library/react which is not installed in this project.
Map model_redirects, allowed_models, anthropic_thinking_budget_preference, and anthropic_adaptive_thinking through batchUpdateProviders. Empty allowed_models array normalizes to null (allow-all).
Add dropdown menus for selecting providers by type and by group tag. Selection is additive - adds matching providers to existing selection.
…ibility Enhance previewProviderBatchPatch to produce before/after rows for each provider-field combination. Anthropic-only fields on non-claude providers are marked as skipped. Rows stored in preview snapshot for apply engine.
…dialog Replace toggle-to-enable pattern with empty-means-no-change semantics. Add three sections (basic, routing, anthropic) reusing ThinkingBudgetEditor and AdaptiveThinkingEditor. Show affected-provider summary with masked keys.
…ure, and exclusion support - Enhanced applyProviderBatchPatch to write to DB via updateProvidersBatch - Added excludeProviderIds support for selective application - Capture preimage of changed fields for undo support - Map snake_case patch fields to camelCase repository format - Invalidate provider cache after successful write - Store preimage and patch in undo snapshot for T10 - 15 new tests covering writes, exclusion, idempotency, field mapping
…-provider restoration - Enhanced undoProviderPatch to read preimage and write original values back to DB - Group providers by identical preimage values to minimize DB round-trips - Handle costMultiplier number-to-string conversion for repository interface - Invalidate provider cache after successful undo writes - Consume undo token only after successful writes (preserved for retry on failure) - Updated contract test to populate preimage during apply step - 10 new undo engine tests covering all edge cases
…, and auth hardening Security: - Add constant-time string comparison utility to prevent timing attacks - Harden login route with improved token validation - Add rate limiting to proxy auth guard - Tighten CORS origin validation Provider batch edit: - Add build-patch-draft module for generating per-provider patches - Add provider-batch-preview-step with field-level diff display - Extend provider-form-context with batch mode support - Refactor form sections (basic-info, network, routing, testing) for batch compatibility - Expand provider-patch-contract with apply/undo engine types - Add repository helpers for bulk provider operations - Update i18n messages across all 5 languages Tests: - Add constant-time compare and proxy auth rate limit security tests - Add build-patch-draft, preview step, form context, and undo toast unit tests - Update existing batch dialog and patch contract tests
- Add generic RedisKVStore<T> with atomic get-and-delete via Lua script - Migrate undo store from in-memory Map to Redis persistence - Increase undo snapshot TTL from 10s to 30s - Extend auth session TTL from 1 day to 7 days - Add LRU eviction to login abuse policy - Record login failures in auth route - Add error logging to logout route - Type anthropicAdaptiveThinking as AnthropicAdaptiveThinkingConfig - i18n: add apiKeyLabel and unknownError keys across 5 locales - i18n: internationalize batch edit preview field labels - Fix thinking budget editor input handling - Add claude-auth routing group for 1M context window - Fix provider manager group filtering for default label - Add comprehensive tests for RedisKVStore, auth, undo, and providers
ding113
force-pushed
the
feat/security-auth-overhaul
branch
from
7c52261 to
4a0f06e
Compare
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
| if (!key) { | ||
| return NextResponse.json({ error: t?.("apiKeyRequired") }, { status: 400 }); | ||
| if (!key || typeof key !== "string") { | ||
| loginPolicy.recordFailure(clientIp); |
There was a problem hiding this comment.
missing key parameter means invalid key attempts aren't tracked for key-based rate limiting
Suggested change
| loginPolicy.recordFailure(clientIp); | |
| loginPolicy.recordFailure(clientIp, key); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 191
Comment:
missing `key` parameter means invalid key attempts aren't tracked for key-based rate limiting
```suggestion
loginPolicy.recordFailure(clientIp, key);
```
How can I resolve this? If you propose a fix, please make it concise.| const session = await validateKey(key, { allowReadOnlyAccess: true }); | ||
| if (!session) { | ||
| return NextResponse.json({ error: t?.("apiKeyInvalidOrExpired") }, { status: 401 }); | ||
| loginPolicy.recordFailure(clientIp); |
There was a problem hiding this comment.
missing key parameter means invalid key attempts aren't tracked for key-based rate limiting
Suggested change
| loginPolicy.recordFailure(clientIp); | |
| loginPolicy.recordFailure(clientIp, key); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 212
Comment:
missing `key` parameter means invalid key attempts aren't tracked for key-based rate limiting
```suggestion
loginPolicy.recordFailure(clientIp, key);
```
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| } | ||
|
|
||
| loginPolicy.recordSuccess(clientIp); |
There was a problem hiding this comment.
missing key parameter means successful logins don't reset key-based rate limiting
Suggested change
| loginPolicy.recordSuccess(clientIp); | |
| loginPolicy.recordSuccess(clientIp, key); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/auth/login/route.ts
Line: 273
Comment:
missing `key` parameter means successful logins don't reset key-based rate limiting
```suggestion
loginPolicy.recordSuccess(clientIp, 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: 5
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
messages/ja/settings/providers/batchEdit.json-81-92 (1)
81-92:⚠️ Potential issue | 🟡 Minor
undo节与toast节存在重复字符串值两个节中有完全相同的文本值：
键 值 undo.button(Line 82)"元に戻す"toast.undo(Line 98)"元に戻す"undo.expired(Line 84)"元に戻す期限が切れました"toast.undoExpired(Line 101)"元に戻す期限が切れました"这两组键的值完全一致，建议确认它们是否确实对应不同的 UI 使用场景（例如，一组用于内联 UI 组件，另一组用于 Toast 通知），还是其中一组存在冗余。如果两者用途相同，未来维护时若只更新其中一处，可能导致两个 UI 位置文案不一致。请验证其他语言的 locale 文件是否有相同的结构，以确认这是有意为之的设计。
Also applies to: 93-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@messages/ja/settings/providers/batchEdit.json` around lines 81 - 92, The undo and toast sections contain duplicate Japanese strings (e.g., undo.button vs toast.undo, undo.expired vs toast.undoExpired); inspect whether these keys serve distinct UI contexts or are redundant, then either (a) consolidate by removing the redundant keys and point callers to a single canonical key (e.g., use undo.button and toast.undo reference it) or (b) keep both but harmonize their values and add a comment explaining the difference; update all callers to use the chosen canonical keys (search for undo.button, toast.undo, undo.expired, toast.undoExpired) and verify/align the same change across other locale files to keep translations consistent.src/lib/security/csrf-origin-guard.ts-40-43 (1)
40-43:⚠️ Potential issue | 🟡 Minor
same-origin快速路径跳过了 Origin 头的校验，存在非浏览器客户端绕过的风险当
sec-fetch-site === "same-origin"且allowSameOrigin === true时，代码直接放行，即使请求中同时携带了不在白名单中的Origin头也不会被检查。
Sec-Fetch-*是浏览器的禁止修改头（Forbidden Headers），正常浏览器 CSRF 攻击无法伪造，因此浏览器发起的 CSRF 仍然会被正确的sec-fetch-site: cross-site拦截。但非浏览器 HTTP 客户端（如curl、自动化脚本、服务端转发）可以人工设置Sec-Fetch-Site: same-origin，结合有效凭证可绕过来源白名单，不符合纵深防御原则。建议：当
sec-fetch-site === "same-origin"且请求中同时存在Origin头时，额外校验该 Origin 是否符合预期（需在 config 中额外传入 server origin）。🤖 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 40 - 43, The fast-path that returns allowed when fetchSite === "same-origin" and allowSameOrigin is true skips Origin header validation and can be bypassed by non-browser clients; update the logic in csrf-origin-guard (look for the fetchSite, allowSameOrigin check) so that when fetchSite === "same-origin" and an Origin header is present you also validate that Origin against the configured trusted origin(s) (e.g., a new serverOrigin or allowedOrigins value provided in config) and only return { allowed: true } if that Origin matches the whitelist; otherwise treat it as disallowed.tests/security/csrf-origin-guard.test.ts-17-132 (1)
17-132:⚠️ Potential issue | 🟡 Minor缺少对多个关键分支的测试覆盖，可能不满足 80% 覆盖率要求
当前测试套件未覆盖以下场景，导致
createCsrfOriginGuard的部分条件分支未被验证：
allowSameOrigin: false+sec-fetch-site: same-origin：当前仅测试了allowSameOrigin: true路径（Line 17-31），布尔条件fetchSite === "same-origin" && allowSameOrigin的false分支（应穿透到 Origin 检查）没有覆盖。
sec-fetch-site: same-site和sec-fetch-site: none：这两个值既不触发同源快路径，也不触发跨域缺失 Origin 的拦截路径，应验证其走到 Origin 白名单检查。
sec-fetch-site: same-origin+ 不在白名单的Origin+allowSameOrigin: true：此组合当前被放行（快路径不检查 Origin），需要一个明确的测试用例来记录这一已知行为（见安全审查意见）。建议补充如下测试用例：
建议补充的测试用例
+ it("falls through to Origin check when allowSameOrigin is false and sec-fetch-site is same-origin", () => { + const guard = createCsrfOriginGuard({ + allowedOrigins: ["https://example.com"], + allowSameOrigin: false, + enforceInDevelopment: true, + }); + + const allowed = guard.check( + createRequest({ "sec-fetch-site": "same-origin", origin: "https://example.com" }) + ); + expect(allowed).toEqual({ allowed: true }); + + const blocked = guard.check( + createRequest({ "sec-fetch-site": "same-origin", origin: "https://evil.com" }) + ); + expect(blocked.allowed).toBe(false); + }); + + it("allows same-site requests with Origin in allowlist", () => { + const guard = createCsrfOriginGuard({ + allowedOrigins: ["https://example.com"], + allowSameOrigin: false, + enforceInDevelopment: true, + }); + const result = guard.check( + createRequest({ "sec-fetch-site": "same-site", origin: "https://example.com" }) + ); + expect(result).toEqual({ allowed: true }); + }); + + it("documents that same-origin fast-path allows requests even with non-allowlisted Origin", () => { + const guard = createCsrfOriginGuard({ + allowedOrigins: [], + allowSameOrigin: true, + enforceInDevelopment: true, + }); + const result = guard.check( + createRequest({ "sec-fetch-site": "same-origin", origin: "https://evil.example.com" }) + ); + // Documents known behavior: Origin header is not checked when sec-fetch-site is same-origin + expect(result).toEqual({ allowed: true }); + });基于代码规范，所有新功能的单元测试覆盖率不得低于 80%。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/csrf-origin-guard.test.ts` around lines 17 - 132, Tests for createCsrfOriginGuard are missing coverage for several branches: the path where allowSameOrigin is false but sec-fetch-site === "same-origin" (so requests should fall through to Origin checks), the sec-fetch-site values "same-site" and "none" (which should be validated against the allowedOrigins), and the explicit case sec-fetch-site === "same-origin" with allowSameOrigin === true but an Origin not present in allowedOrigins (to document the existing fast-path behavior). Add unit tests that call guard.check (produced by createCsrfOriginGuard) with combinations: (1) allowSameOrigin: false + sec-fetch-site: "same-origin" + an Origin both allowed and disallowed, asserting the Origin allowlist logic runs; (2) sec-fetch-site: "same-site" and "none" with both allowed and disallowed Origin values to ensure whitelist behavior; and (3) allowSameOrigin: true + sec-fetch-site: "same-origin" + Origin not in allowedOrigins to assert the request is allowed (or to capture the documented behavior), so all conditional branches in createCsrfOriginGuard are exercised.src/app/api/auth/login/route.ts-186-208 (1)
186-208:⚠️ Potential issue | 🟡 Minor
request.json()解析异常会被 catch 以 500 返回，而非 400。如果请求体不是合法 JSON（如空 body、格式错误），
await request.json()会抛出异常，进入第 296 行的 catch 块并返回 500 SERVER_ERROR。对客户端而言更合理的响应是 400 Bad Request。建议修复：在 try 顶部单独捕获 JSON 解析
try { - const { key } = await request.json(); + let key: unknown; + try { + ({ key } = await request.json()); + } catch { + loginPolicy.recordFailure(clientIp); + return withAuthResponseHeaders( + NextResponse.json( + { error: t?.("apiKeyRequired") ?? "Invalid request body", errorCode: "KEY_REQUIRED" }, + { status: 400 } + ) + ); + }🤖 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 186 - 208, The code currently lets JSON parse errors from await request.json() fall into the outer catch and return a 500; instead, wrap just the request.json() call in its own try/catch so JSON parse failures return a 400 Bad Request. In the new inner catch call loginPolicy.recordFailure(clientIp) and return the same withAuthResponseHeaders responses used for missing/invalid key (using t?.("apiKeyRequired") and the errorCode "KEY_REQUIRED" when shouldIncludeFailureTaxonomy(request) is true), then continue with the existing logic for a successfully parsed body.tests/security/security-headers-integration.test.ts-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor导入路径应统一使用
@/别名第 3 行和第 99、102 行使用了相对路径
"../../src/..."进行导入，与第 20-43 行的vi.mock()中使用的@/别名不一致。根据编码规范要求统一使用@/别名来引用./src/目录下的文件，其他测试文件（如notifier-circuit-breaker.test.ts）中的动态导入已成功使用@/别名。建议修复
-import { applyCors } from "../../src/app/v1/_lib/cors"; +import { applyCors } from "@/app/v1/_lib/cors";- 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/security-headers-integration.test.ts` at line 3, The test file uses relative imports for modules like applyCors instead of the project alias; update the import statements that reference "../../src/..." (including the import of applyCors and the other two relative imports near the mock section) to use the "@/..." alias (e.g., import applyCors from "@/app/v1/_lib/cors") so they match the vi.mock() usages; ensure only the import paths change and keep the same exported symbols/import names.messages/zh-CN/settings/providers/batchEdit.json-80-80 (1)
80-80:⚠️ Potential issue | 🟡 Minor术语不一致："服务商" 应统一为 "供应商"
第 80 行使用了"服务商"，而同文件其他所有涉及提供商的译文均使用"供应商"（如第 52、85、86 行等）。请保持术语一致。
建议修复
- "selectionHint": "选择多个服务商后可进行批量操作", + "selectionHint": "选择多个供应商后可进行批量操作",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@messages/zh-CN/settings/providers/batchEdit.json` at line 80, The text for the JSON key "selectionHint" uses the inconsistent term "服务商"; update its value to use the project's standard term "供应商" so it matches other entries (e.g., the strings around lines using "供应商"), e.g. change "选择多个服务商后可进行批量操作" to the equivalent with "供应商" to keep terminology consistent across the file.tests/security/session-fixation-rotation.test.ts-84-90 (1)
84-90:⚠️ Potential issue | 🟡 Minor"post-login rotation" 测试仅验证 mock 本身，未覆盖任何生产代码路径
simulatePostLoginSessionRotation直接接受mockRotate作为rotate参数并调用它，等同于：const result = await mockRotate(oldSessionId);该测试断言
mockRotate被正确调用并返回预设值，与登录路由或会话旋转的实际实现毫无关联。相较之下，同文件中的 logout 测试通过loadLogoutPost()真正导入了生产路由，才具有实质意义。建议将此用例改为通过动态导入真实登录路由（类似
loadLogoutPost()）来触发rotate()调用，以验证登录成功后setAuthCookie接收到旋转后的 session ID。Also applies to: 161-177
🤖 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 84 - 90, The test currently only calls the provided mock via simulatePostLoginSessionRotation, so it never exercises production code; replace the mock-level helper with a test that dynamically imports and invokes the real login POST handler (similar to loadLogoutPost()), trigger a successful login flow so the route executes its rotate() path, and assert that the real rotate() is called and setAuthCookie is invoked with the rotated sessionId; locate references to simulatePostLoginSessionRotation and mockRotate in the test and change them to import the actual login route (e.g., loadLoginPost or the router handler), wire any necessary mocks/stubs for external deps but let the route call rotate(), and verify setAuthCookie received the rotated id returned by rotate().tests/unit/auth/auth-cookie-constant-sync.test.ts-18-19 (1)
18-19:⚠️ Potential issue | 🟡 Minor正则表达式未覆盖反引号模板字符串形式的硬编码
当前正则
/["']auth-token["']/仅匹配单引号和双引号字符串，无法检测到`auth-token`这类模板字面量写法。若将来某处无意间使用模板字符串引用了硬编码值，测试会静默放行。建议修复
- expect(proxySource).not.toMatch(/["']auth-token["']/); - expect(actionAdapterSource).not.toMatch(/["']auth-token["']/); + expect(proxySource).not.toMatch(/["'`]auth-token["'`]/); + expect(actionAdapterSource).not.toMatch(/["'`]auth-token["'`]/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth/auth-cookie-constant-sync.test.ts` around lines 18 - 19, The current assertions on proxySource and actionAdapterSource use the regex /["']auth-token["']/ which only matches single- and double-quoted literals and misses backtick template literals; update both expectations to use a regex that also matches backticks (e.g., replace /["']auth-token["']/ with /(?:["'`])auth-token(?:["'`])/) so proxySource and actionAdapterSource will fail if `auth-token` is hardcoded with backticks.src/app/[locale]/login/loading.tsx-30-30 (1)
30-30:⚠️ Potential issue | 🟡 Minor
LoadingState传入空label导致无障碍文本缺失
LoadingState渲染一个role="status" aria-busy="true"的div，当label=""时，该区域没有任何文本内容。屏幕阅读器会感知到这个活跃区域，但无法向用户传达正在加载的内容，未能满足 WCAG 4.1.3 状态消息的要求。建议修复
- <LoadingState label="" className="text-center" /> + <LoadingState label={t("loading")} className="text-center" />其中
"loading"对应已有的 i18n 键；若尚不存在，可添加一个通用的加载提示文案。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/login/loading.tsx at line 30, LoadingState is being rendered with an empty label which leaves the role="status" aria-busy="true" region with no accessible text; update the call to LoadingState to pass a meaningful localized label (e.g., the existing i18n key "loading") instead of "" so screen readers receive a status message—locate the LoadingState usage in this file and replace label="" with the translated string (via your i18n helper like useTranslations or equivalent) or add a fallback "loading" i18n key if missing.src/app/[locale]/settings/providers/_components/adaptive-thinking-editor.tsx-74-105 (1)
74-105:⚠️ Potential issue | 🟡 Minor
TooltipTrigger asChild绑定在非聚焦div上，键盘用户无法触发提示三个
Tooltip块（第 74–105、111–145、152–173 行）均将TooltipTrigger asChild应用于<div>。根据 Radix UI 官方文档，TooltipTrigger的子元素必须是可聚焦的交互式元素。当改为div时，由于div默认无法获得焦点（tabIndex未设置），键盘用户无法触发Tooltip，导致帮助文本对键盘用户不可见。建议将
Tooltip仅包裹Info图标，将其改为可聚焦的<button>：建议修复（以 effort 段为例，其余两处同理）
- <SmartInputWrapper - label={t("sections.routing.anthropicOverrides.adaptiveThinking.effort.label")} - > - <Tooltip> - <TooltipTrigger asChild> - <div className="flex gap-2 items-center"> - <Select - value={config.effort} - onValueChange={(val) => - handleEffortChange(val as AnthropicAdaptiveThinkingEffort) - } - disabled={disabled} - > - <SelectTrigger className="w-40"> - <SelectValue /> - </SelectTrigger> - <SelectContent> - {(["low", "medium", "high", "max"] as const).map((level) => ( - <SelectItem key={level} value={level}> - {t(`sections.routing.anthropicOverrides.adaptiveThinking.effort.options.${level}`)} - </SelectItem> - ))} - </SelectContent> - </Select> - <Info className="h-4 w-4 text-muted-foreground shrink-0" /> - </div> - </TooltipTrigger> - <TooltipContent side="top" className="max-w-xs"> - <p className="text-sm"> - {t("sections.routing.anthropicOverrides.adaptiveThinking.effort.help")} - </p> - </TooltipContent> - </Tooltip> - </SmartInputWrapper> + <SmartInputWrapper + label={t("sections.routing.anthropicOverrides.adaptiveThinking.effort.label")} + > + <div className="flex gap-2 items-center"> + <Select + value={config.effort} + onValueChange={(val) => + handleEffortChange(val as AnthropicAdaptiveThinkingEffort) + } + disabled={disabled} + > + <SelectTrigger className="w-40"> + <SelectValue /> + </SelectTrigger> + <SelectContent> + {(["low", "medium", "high", "max"] as const).map((level) => ( + <SelectItem key={level} value={level}> + {t(`sections.routing.anthropicOverrides.adaptiveThinking.effort.options.${level}`)} + </SelectItem> + ))} + </SelectContent> + </Select> + <Tooltip> + <TooltipTrigger asChild> + <button + type="button" + className="p-0 h-4 w-4 text-muted-foreground hover:text-foreground" + > + <Info className="h-4 w-4 shrink-0" /> + <span className="sr-only"> + {t("sections.routing.anthropicOverrides.adaptiveThinking.effort.label")} + </span> + </button> + </TooltipTrigger> + <TooltipContent side="top" className="max-w-xs"> + <p className="text-sm"> + {t("sections.routing.anthropicOverrides.adaptiveThinking.effort.help")} + </p> + </TooltipContent> + </Tooltip> + </div> + </SmartInputWrapper>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/adaptive-thinking-editor.tsx around lines 74 - 105, The TooltipTrigger is currently wrapping a non-focusable <div> (around Select and Info) so keyboard users cannot open the tooltip; update the three Tooltip blocks (the one containing Select and Info for effort, and the two similar blocks) so the Tooltip only wraps the Info icon and the Trigger is a focusable element: replace the div-wrapped TooltipTrigger with TooltipTrigger asChild around a button (or make the Info component render a <button> with proper aria-label and focus styles) and leave the Select outside the Tooltip; ensure TooltipContent remains unchanged and that the button is keyboard-focusable so tooltip appears for keyboard users.src/app/[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx-61-66 (1)
61-66:⚠️ Potential issue | 🟡 Minor水平布局按钮缺少
focus-visible:ring样式，与桌面布局不一致。桌面布局（第 120 行）的按钮包含
focus-visible:ring-2 focus-visible:ring-ring，但水平布局仅有focus-visible:outline-none，键盘导航时没有可见的焦点指示器，影响无障碍访问。平板布局（第 177 行）同样缺少此样式。建议修复
className={cn( "relative flex items-center gap-2 px-4 py-3 text-sm font-medium transition-all whitespace-nowrap", - "hover:text-foreground focus-visible:outline-none", + "hover:text-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring", isActive ? "text-primary" : "text-muted-foreground", disabled && "opacity-50 cursor-not-allowed" )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/components/form-tab-nav.tsx around lines 61 - 66, 在水平布局的按钮 className（在 form-tab-nav.tsx 中使用 cn(...) 并包含 isActive/disabled 条件的那一行）仅有 "focus-visible:outline-none"，缺少与桌面布局一致的可见焦点指示器；将 "focus-visible:ring-2 focus-visible:ring-ring" 添加到该 className 的焦点样式中（并同样检查并在平板布局的按钮定义处添加相同样式），以确保键盘导航时有可见的 focus ring；参考的标识符：cn(...), isActive, disabled, className 属性。tests/unit/login/login-ui-redesign.test.tsx-44-55 (1)
44-55:⚠️ Potential issue | 🟡 Minor
global.fetch在afterEach中未恢复，会污染后续测试套件。
beforeEach覆盖了global.fetch，但afterEach没有将其恢复为原始值。同一 PR 中的login-footer-system-name.test.tsx和login-loading-state.test.tsx都正确保存并恢复了global.fetch。建议修复
+const globalFetch = global.fetch; + describe("LoginPage UI Redesign", () => { let container: HTMLDivElement; let root: ReturnType<typeof createRoot>; beforeEach(() => { container = document.createElement("div"); document.body.appendChild(container); root = createRoot(container); vi.clearAllMocks(); global.fetch = vi.fn().mockResolvedValue({ ok: true, json: async () => ({}), }); }); afterEach(() => { act(() => { root.unmount(); }); document.body.removeChild(container); + global.fetch = globalFetch; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/login/login-ui-redesign.test.tsx` around lines 44 - 55, Tests overwrite global.fetch in the beforeEach but never restore it in afterEach, which can leak into other tests; update the test setup to save the original global.fetch (e.g., const originalFetch = global.fetch) before mocking in the beforeEach and then restore it in afterEach by assigning global.fetch = originalFetch (perform restore after unmount/removeChild cleanup), referencing the existing beforeEach/afterEach and the global.fetch mock in login-ui-redesign.test.tsx to locate where to add the save/restore.src/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.ts-199-205 (1)
199-205:⚠️ Potential issue | 🟡 Minor
limitConcurrentSessions为null时的处理与其他 rate limit 字段不一致。其他 rate limit 字段为
null时均使用{ clear: true }（如limit5hUsd、limitDailyUsd、limitTotalUsd等），但limitConcurrentSessions为null时使用{ set: 0 }。如果0在此字段有特殊含义（如表示"禁止并发"），请添加注释说明；否则建议统一为{ clear: true }。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/batch-edit/build-patch-draft.ts around lines 199 - 205, The handling of rateLimit.limitConcurrentSessions is inconsistent: currently when state.rateLimit.limitConcurrentSessions === null the code sets draft.limit_concurrent_sessions = { set: 0 } while other rate limit nulls use { clear: true }. Update the branch in the dirtyFields.has("rateLimit.limitConcurrentSessions") block to use draft.limit_concurrent_sessions = { clear: true } when the state value is null to match the other fields; if 0 has a special semantic meaning for limit_concurrent_sessions, instead keep the { set: 0 } but add a clear comment above that branch explaining the special meaning and why it differs from the other rate limit fields (referencing dirtyFields.has("rateLimit.limitConcurrentSessions"), state.rateLimit.limitConcurrentSessions, and draft.limit_concurrent_sessions).tests/unit/actions/provider-undo-edit.test.ts-2-2 (1)
2-2:⚠️ Potential issue | 🟡 Minor静态导入使用相对路径，违反
@/路径别名规范第 2 行及后续所有动态
import()调用均使用"../../../src/..."相对路径。根据编码规范，引用./src/目录下文件应统一使用@/别名。🛠️ 建议修复（静态导入）
-import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "../../../src/lib/provider-batch-patch-error-codes"; +import { PROVIDER_BATCH_PATCH_ERROR_CODES } from "@/lib/provider-batch-patch-error-codes";所有动态
import("../../../src/actions/providers")也应改为import("@/actions/providers")。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/provider-undo-edit.test.ts` at line 2, Replace all relative "../../../src/..." imports with the project alias "@/..." to conform to the path-alias rule: change the static import of PROVIDER_BATCH_PATCH_ERROR_CODES to import from "@/lib/provider-batch-patch-error-codes" and update every dynamic import(...) like import("../../../src/actions/providers") to import("@/actions/providers"); ensure you update all occurrences in this test file (e.g., references to PROVIDER_BATCH_PATCH_ERROR_CODES and any dynamic import calls) so they use the "@/..." alias consistently.src/app/[locale]/settings/providers/_components/vendor-keys-compact-list.tsx-324-349 (1)
324-349:⚠️ Potential issue | 🟡 MinorToast 展示时长（10s）与撤销令牌有效期（30s）不一致，导致静默 UX 缺口
duration: 10000（10 秒）后 toast 消失，但providers-undo-store.test.ts中的断言（setexMock以 TTL=30 调用）证明撤销令牌实际有效期为 30 秒。Toast 消失后用户失去撤销入口，但令牌本身还有约 20 秒可用，这两者之间存在静默缺口。建议将 toast 持续时间与令牌有效期保持一致（同为 30 秒），或缩短令牌 TTL：
🛠️ 建议修复
toast.success(tBatchEdit("undo.singleDeleteSuccess"), { - duration: 10000, + duration: 30000, action: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/vendor-keys-compact-list.tsx around lines 324 - 349, The toast shown after deletion uses duration: 10000 (10s) but the undo token TTL is 30s (per providers-undo-store.test), creating a UX gap; update the toast success/action block (the toast.success call and its options near the undoProviderDelete usage and tBatchEdit labels) to use duration: 30000 so the toast stays visible for the same 30s as the undo token, or alternatively reduce the undo token TTL in the providers-undo-store (setexMock) to 10s—pick one approach and make the corresponding change consistently (toast duration in the vendor-keys-compact-list toast.success or token TTL in providers-undo-store) so both values match.src/app/[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx-226-252 (1)
226-252:⚠️ Potential issue | 🟡 Minor批量编辑撤销成功后仅失效
["providers"]查询，与单条编辑/删除撤销不一致。行 242 仅调用
queryClient.invalidateQueries({ queryKey: ["providers"] })，而provider-form/index.tsx中的撤销回调（行 387-390、479-482）会同时失效providers、providers-health、providers-statistics、provider-vendors四个查询。BatchConfirmDialog中删除撤销（行 420）也存在同样的问题。这会导致撤销后 UI 中健康状态/统计数据/供应商分组不更新。
补全查询失效列表
if (undoResult.ok) { toast.success(t("toast.undoSuccess", { count: undoResult.data.revertedCount })); - queryClient.invalidateQueries({ queryKey: ["providers"] }); + void Promise.all([ + queryClient.invalidateQueries({ queryKey: ["providers"] }), + queryClient.invalidateQueries({ queryKey: ["providers-health"] }), + queryClient.invalidateQueries({ queryKey: ["providers-statistics"] }), + queryClient.invalidateQueries({ queryKey: ["provider-vendors"] }), + ]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsx around lines 226 - 252, 当前批量编辑的撤销处理（provider-batch-dialog.tsx 中 result.ok 分支内的 undo 点击回调）只失效 ["providers"]，与 provider-form 中撤销回调（provider-form/index.tsx）和 BatchConfirmDialog 的删除撤销不同步，导致健康/统计/供应商分组不更新；请在 provider-batch-dialog.tsx 的 undoProviderPatch 成功分支以及 BatchConfirmDialog 的删除撤销处理处，像 provider-form 那样同时调用 queryClient.invalidateQueries 对以下查询键进行失效：["providers"], ["providers-health"], ["providers-statistics"], ["provider-vendors"]（可一次性逐个调用或循环调用），以确保 UI 在撤销后与单条编辑/删除行为一致。tests/unit/settings/providers/build-patch-draft.test.ts-420-467 (1)
420-467:⚠️ Potential issue | 🟡 Minor电路熔断器和网络超时字段对
undefined的处理策略确实存在显著差异，建议补充文档说明。验证表明这种差异是有意为之的，但在代码中完全缺乏说明：
- 电路熔断器字段（
failureThreshold、openDurationMinutes、halfOpenSuccessThreshold）：当值为undefined时，映射为{ set: 0 }——表示显式重置为默认值 0- 网络超时字段（
firstByteTimeoutStreamingSeconds、streamingIdleTimeoutSeconds、requestTimeoutNonStreamingSeconds）：当值为undefined时，完全跳过，不包含在 draft 中——表示"不修改此字段"这种语义差异在
buildPatchDraftFromFormState函数实现中缺少注释说明。建议在代码中添加注释，明确解释两组字段的不同策略原因（例如，电路熔断器是否需要显式的"恢复默认值"语义，而网络超时是否允许保持不变）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/providers/build-patch-draft.test.ts` around lines 420 - 467, In buildPatchDraftFromFormState, add a clear inline comment near the logic that maps circuit breaker fields (failureThreshold, openDurationMinutes, halfOpenSuccessThreshold -> circuit_breaker_failure_threshold, circuit_breaker_open_duration, etc.) and the logic that maps network timeout fields (firstByteTimeoutStreamingSeconds, streamingIdleTimeoutSeconds, requestTimeoutNonStreamingSeconds -> first_byte_timeout_streaming_ms, etc.) explaining that the circuit-breaker fields intentionally map undefined -> { set: 0 } to explicitly reset them to a zero default, whereas network timeout fields intentionally skip including the key when undefined to leave the server-side value unchanged; reference the function name buildPatchDraftFromFormState and the specific output keys (e.g., circuit_breaker_open_duration, circuit_breaker_failure_threshold, first_byte_timeout_streaming_ms) in the comment so future readers can quickly find and understand the divergent semantics.
Comment on lines
+202
to
+226
| function isValidSetValue(field: ProviderBatchPatchField, value: unknown): boolean { | ||
| switch (field) { | ||
| case "is_enabled": | ||
| case "preserve_client_ip": | ||
| case "swap_cache_ttl_billing": | ||
| case "proxy_fallback_to_direct": | ||
| return typeof value === "boolean"; | ||
| case "priority": | ||
| case "weight": | ||
| case "cost_multiplier": | ||
| case "limit_5h_usd": | ||
| case "limit_daily_usd": | ||
| case "limit_weekly_usd": | ||
| case "limit_monthly_usd": | ||
| case "limit_total_usd": | ||
| case "limit_concurrent_sessions": | ||
| case "circuit_breaker_failure_threshold": | ||
| case "circuit_breaker_open_duration": | ||
| case "circuit_breaker_half_open_success_threshold": | ||
| case "max_retry_attempts": | ||
| case "first_byte_timeout_streaming_ms": | ||
| case "streaming_idle_timeout_ms": | ||
| case "request_timeout_non_streaming_ms": | ||
| return typeof value === "number" && Number.isFinite(value); | ||
| case "group_tag": |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find schema files and inspect CreateProviderSchema/UpdateProviderSchema definitions
find . -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: ding113/claude-code-hub
Length of output: 668
🏁 Script executed:
# Search for CreateProviderSchema and UpdateProviderSchema
rg -n "CreateProviderSchema|UpdateProviderSchema" --type ts --type tsxRepository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
# Also examine the patch contract file to see full validation logic
cat -n src/lib/provider-patch-contract.ts | head -250Repository: ding113/claude-code-hub
Length of output: 8852
🏁 Script executed:
# Get rest of the validation function and search for schema files
tail -n +250 src/lib/provider-patch-contract.ts | head -100Repository: ding113/claude-code-hub
Length of output: 3266
🏁 Script executed:
# Search for CreateProviderSchema and UpdateProviderSchema definitions
rg -n "CreateProviderSchema|UpdateProviderSchema" -A 5 --type tsRepository: ding113/claude-code-hub
Length of output: 14858
🏁 Script executed:
# Search for provider schema files
fd -type f -name "*.ts" | xargs grep -l "CreateProviderSchema\|UpdateProviderSchema" | head -10Repository: ding113/claude-code-hub
Length of output: 237
🏁 Script executed:
# Inspect the actual numeric field definitions in CreateProviderSchema and UpdateProviderSchema
sed -n '400,500p' src/lib/validation/schemas.tsRepository: ding113/claude-code-hub
Length of output: 3560
🏁 Script executed:
# Continue reading the schema definitions
sed -n '500,650p' src/lib/validation/schemas.tsRepository: ding113/claude-code-hub
Length of output: 5241
补丁字段校验弱于 Create/Update Schema，多个字段约束未同步导致绕过校验风险
isValidSetValue() 仅校验数值类型和有限性，缺失整数性、范围、特殊规则等约束。例如：
weight在 Schema 中要求整数 1-100，但补丁允许 0.5 或 150priority在 Schema 中要求整数 ≥0，但补丁允许 -1limit_5h_usd等金额字段在 Schema 中有上限（10000/50000/200000/10000000），但补丁无限制circuit_breaker_open_duration在 Schema 中要求整数且 ≥1000ms，但补丁可接受任意数值request_timeout_non_streaming_ms等超时字段有特殊规则（0=禁用或指定范围），但补丁不区分
需对齐 CreateProviderSchema/UpdateProviderSchema 的所有约束条件（.int()、.min()、.max()、范围、特殊值规则等）。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/provider-patch-contract.ts` around lines 202 - 226, isValidSetValue
currently only checks basic types and finite numbers; update it to enforce the
same constraints as CreateProviderSchema/UpdateProviderSchema (e.g., integer
checks, min/max bounds, and special-value rules) for each field referenced
(notably weight, priority,
limit_5h_usd/limit_daily_usd/limit_weekly_usd/limit_monthly_usd/limit_total_usd,
circuit_breaker_open_duration, circuit_breaker_failure_threshold,
circuit_breaker_half_open_success_threshold, max_retry_attempts,
first_byte_timeout_streaming_ms, streaming_idle_timeout_ms,
request_timeout_non_streaming_ms, and any other numeric/time/limit fields);
implement integer assertions (Number.isInteger) where schemas use .int(),
enforce .min()/.max() ranges to match schema limits (e.g., weight 1–100,
priority >=0, specific upper bounds for limit_*), and encode special rules such
as “0 = disabled” or minimum ms (e.g., circuit_breaker_open_duration >=1000) for
timeout fields so the patch validation cannot accept out-of-range or fractional
values.
Comment on lines
+315
to
+354
| if (!isRecord(input)) { | ||
| return createInvalidPatchShapeError(field, "Patch input must be an object"); | ||
| } | ||
|
|
||
| const unknownKeys = Object.keys(input).filter((key) => !PATCH_INPUT_KEYS.has(key)); | ||
| if (unknownKeys.length > 0) { | ||
| return createInvalidPatchShapeError( | ||
| field, | ||
| `Patch input contains unknown keys: ${unknownKeys.join(",")}` | ||
| ); | ||
| } | ||
|
|
||
| const hasSet = Object.hasOwn(input, "set"); | ||
| const hasClear = input.clear === true; | ||
| const hasNoChange = input.no_change === true; | ||
| const modeCount = [hasSet, hasClear, hasNoChange].filter(Boolean).length; | ||
|
|
||
| if (modeCount !== 1) { | ||
| return createInvalidPatchShapeError(field, "Patch input must choose exactly one mode"); | ||
| } | ||
|
|
||
| if (hasSet) { | ||
| if (input.set === undefined) { | ||
| return createInvalidPatchShapeError(field, "set mode requires a defined value"); | ||
| } | ||
|
|
||
| if (!isValidSetValue(field, input.set)) { | ||
| return createInvalidPatchShapeError(field, "set mode value is invalid for this field"); | ||
| } | ||
|
|
||
| return { ok: true, data: { mode: "set", value: input.set as T } }; | ||
| } | ||
|
|
||
| if (hasNoChange) { | ||
| return { ok: true, data: createNoChangePatch() }; | ||
| } | ||
|
|
||
| if (!CLEARABLE_FIELDS[field]) { | ||
| return createInvalidPatchShapeError(field, "clear mode is not supported for this field"); | ||
| } |
There was a problem hiding this comment.
补丁校验错误消息为硬编码英文，违反 i18n 规范
这些错误消息会沿着 actions 返回给前端，属于用户可见文本，应改为 i18n key 或错误码，由调用方映射为多语言文案。
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/lib/provider-patch-contract.ts` around lines 315 - 354, Replace all
hardcoded, user-facing English strings passed to createInvalidPatchShapeError in
provider-patch-contract (occurrences around the validation checks using
isRecord, unknownKeys/PATCH_INPUT_KEYS, modeCount, hasSet, hasNoChange, and
CLEARABLE_FIELDS) with i18n keys or standardized error codes (e.g.,
PATCH_INVALID_SHAPE, PATCH_UNKNOWN_KEYS, PATCH_MODE_COUNT, PATCH_SET_REQUIRED,
PATCH_SET_INVALID, PATCH_CLEAR_UNSUPPORTED) and move variable details (like
unknownKeys.join(",")) into structured error metadata rather than the display
message; ensure callers map these keys/codes to localized text for zh-CN, zh-TW,
en, ja, ru.
Comment on lines
+18
to
+20
| function normalizeOrigin(origin: string): string { | ||
| return origin.trim().toLowerCase(); | ||
| } |
There was a problem hiding this comment.
normalizeOrigin 缺少 URL 规范化，可能导致合法来源被误拒
当前实现仅做 trim().toLowerCase()，但未处理以下等效来源形式：
https://example.com:443与https://example.com（RFC 6454 中默认端口等效）http://example.com:80与http://example.comhttps://example.com/与https://example.com（尾随斜杠）
若 allowedOrigins 中配置了含默认端口的来源（如 https://example.com:443），而浏览器发送的是不含端口的 https://example.com，将被错误拒绝。
建议使用 URL 构造函数进行规范化：
提议的修复：使用 URL 进行规范化
function normalizeOrigin(origin: string): string {
- return origin.trim().toLowerCase();
+ try {
+ const url = new URL(origin.trim());
+ // URL constructor already normalizes scheme/host to lowercase and drops default ports
+ return url.origin.toLowerCase();
+ } catch {
+ 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 and lowercases input, which fails to canonicalize
equivalent origins (e.g. default ports and trailing slashes) and can cause valid
origins to be rejected; update the normalizeOrigin function to parse the input
using the URL constructor (or a robust URL parser), normalize scheme and
hostname to lowercase, omit default ports (remove :80 for http and :443 for
https), remove any trailing slash from pathname, and return the canonical origin
string so comparisons against allowedOrigins (and functions that call
normalizeOrigin) correctly treat "https://example.com" and
"https://example.com:443/" as equal.
| import { act } from "react"; | ||
| import { createRoot } from "react-dom/client"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import LoginPage from "../../../src/app/[locale]/login/page"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
使用 @/ 路径别名代替相对路径。
同一 PR 中的其他登录测试文件（如 login-ui-redesign.test.tsx、login-loading-state.test.tsx）均使用 @/app/[locale]/login/page，此处应保持一致。
建议修复
-import LoginPage from "../../../src/app/[locale]/login/page";
+import LoginPage from "@/app/[locale]/login/page";As per coding guidelines: "Use path alias @/ to reference files in ./src/ directory".
📝 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.
Suggested change
| import LoginPage from "../../../src/app/[locale]/login/page"; | |
| import LoginPage from "@/app/[locale]/login/page"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/login/login-footer-system-name.test.tsx` at line 4, Replace the
relative import string "../../../src/app/[locale]/login/page" with the
path-alias import "@/app/[locale]/login/page" in the test so it matches other
login tests; update the import statement that brings in LoginPage (the imported
symbol LoginPage) to use the "@/..." alias.
Comment on lines
+57
to
+65
| const render = async () => { | ||
| await act(async () => { | ||
| root.render(<LoginPage />); | ||
| }); | ||
|
|
||
| await act(async () => { | ||
| await Promise.resolve(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file to see the actual code at lines 57-65
cat -n tests/unit/login/login-footer-version.test.tsx | head -80 | tail -30Repository: ding113/claude-code-hub
Length of output: 1099
🏁 Script executed:
# Get the full test file to understand context
wc -l tests/unit/login/login-footer-version.test.tsxRepository: ding113/claude-code-hub
Length of output: 117
🏁 Script executed:
# Find LoginPage component
fd -name "*LoginPage*" -type fRepository: ding113/claude-code-hub
Length of output: 297
🏁 Script executed:
# Find login-related components
fd -name "*login*" -type f | grep -E "\.(tsx?|jsx?)$" | head -20Repository: ding113/claude-code-hub
Length of output: 297
🏁 Script executed:
# Find LoginPage component files
find . -name "*.tsx" -o -name "*.ts" | grep -i login | grep -v test | grep -v node_modulesRepository: ding113/claude-code-hub
Length of output: 256
🏁 Script executed:
# Search for LoginPage component definition
rg "export.*LoginPage|function LoginPage|const LoginPage" --type ts --type tsx | grep -v testRepository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
# Search for login-footer-version or version fetch in login components
rg "login-footer-version|hasUpdate" --type ts --type tsx | head -20Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
# Read the login page component
cat -n ./src/app/[locale]/login/page.tsxRepository: ding113/claude-code-hub
Length of output: 19615
🏁 Script executed:
# Search for version fetch logic without specifying file type
rg "login-footer-version|hasUpdate|/api/version" -A 5 -B 5Repository: ding113/claude-code-hub
Length of output: 21349
🏁 Script executed:
# Look for LoginPage in test imports
head -30 tests/unit/login/login-footer-version.test.tsxRepository: ding113/claude-code-hub
Length of output: 1176
🏁 Script executed:
# Search for flushMicrotasks helper function definition
rg "flushMicrotasks|function.*flush" --type ts --type tsx tests/Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
# Get the complete login-footer-version.test.tsx file to see all details
cat tests/unit/login/login-footer-version.test.tsxRepository: ding113/claude-code-hub
Length of output: 3251
🏁 Script executed:
# Search for flushMicrotasks definition across test files
rg "flushMicrotasks" tests/Repository: ding113/claude-code-hub
Length of output: 3042
🏁 Script executed:
# Check the login-footer-system-name.test.tsx file to see its complete render pattern
cat tests/unit/login/login-footer-system-name.test.tsx | head -100Repository: ding113/claude-code-hub
Length of output: 2942
🏁 Script executed:
# Look for test utilities or setup files that might define async helpers
find tests -name "*.ts" -o -name "*.tsx" | xargs grep -l "flushMicrotasks\|act" | head -5Repository: ding113/claude-code-hub
Length of output: 324
🏁 Script executed:
# Check if there's a vitest config that might affect promise handling
find . -name "vitest.config.*" -o -name "vite.config.*" | head -5Repository: ding113/claude-code-hub
Length of output: 85
🏁 Script executed:
# See if the test file has been recently modified or if there are any CI issues noted
git log --oneline tests/unit/login/login-footer-version.test.tsx 2>/dev/null | head -5 || echo "Git not available or file new"Repository: ding113/claude-code-hub
Length of output: 139
🏁 Script executed:
# Check if there are any existing issues or test flakiness reported
rg "flaky|intermittent|unstable" tests/unit/login/login-footer-version.test.tsxRepository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
# Verify the exact async behavior needed by checking one more similar test helper
rg -A 10 "const flushMicrotasks" tests/unit/lib/provider-endpoints/probe-scheduler.test.tsRepository: ding113/claude-code-hub
Length of output: 49
单次 Promise.resolve() 不足以刷新 Suspense + useEffect 的完整异步链
LoginPage 通过 <Suspense> 包裹 LoginPageContent，其中 useEffect 发起 fetch("/api/version") → .json() → setVersionInfo() 的异步链。完整执行路径需要至少 3–4 个微任务队列轮次，而当前第二个 act 仅刷新一次 Promise.resolve()。同项目中相似测试（login-footer-system-name.test.tsx）使用两次 Promise.resolve() 调用，其他测试使用 6 次以上。当前实现在 CI 环境中可能导致测试不稳定。
建议改用多次微任务刷新：
修复方案
const render = async () => {
await act(async () => {
root.render(<LoginPage />);
});
+
+ // 刷新多个微任务轮次以确保 Suspense 解析、useEffect 执行、fetch 完成和状态更新
+ await act(async () => {
+ await Promise.resolve();
+ await Promise.resolve();
+ });
};或参考 login-footer-system-name.test.tsx 的 flushMicrotasks() 模式，更清晰地表达意图。
📝 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.
Suggested change
| const render = async () => { | |
| await act(async () => { | |
| root.render(<LoginPage />); | |
| }); | |
| await act(async () => { | |
| await Promise.resolve(); | |
| }); | |
| }; | |
| const render = async () => { | |
| await act(async () => { | |
| root.render(<LoginPage />); | |
| }); | |
| // 刷新多个微任务轮次以确保 Suspense 解析、useEffect 执行、fetch 完成和状态更新 | |
| await act(async () => { | |
| await Promise.resolve(); | |
| await Promise.resolve(); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/login/login-footer-version.test.tsx` around lines 57 - 65, The
test's render helper (the async render function that calls
root.render(<LoginPage />) inside act and then awaits a single
Promise.resolve()) doesn't flush enough microtask ticks to let Suspense +
LoginPageContent's useEffect fetch("/api/version") -> .json() ->
setVersionInfo() complete reliably; update that helper (the second act after
root.render) to await multiple microtask ticks (e.g., call Promise.resolve()
several times or loop awaiting Promise.resolve() 3–6 times) or replace it with
the existing flushMicrotasks() utility used in login-footer-system-name.test.tsx
so the full async chain in LoginPageContent runs to completion before
assertions.
This was referenced Feb 21, 2026
Merged
6 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Key changes
Security (19 files)
src/lib/auth.ts- opaque session mode, session rotation, expiration checkssrc/lib/auth-session-store/- Redis-backed session store with lazy-loadsrc/lib/security/- CSRF guard, login abuse policy, security headers, constant-time comparesrc/app/api/auth/login/route.ts- hardened login with bruteforce protectionsrc/app/api/auth/logout/route.ts- session revocation on logoutProvider batch (25 files)
src/actions/providers.ts- batch patch preview, apply, undo server actionssrc/lib/provider-patch-contract.ts- validation, normalization, field mappingsrc/repository/provider.ts- batch DB operations with preimage capturesrc/app/[locale]/dashboard/settings/providers/_components/batch-edit/- UI componentsTests (55 files, ~19k lines added)
Test plan
bun run test- 2791+ passing)bun run typecheck)tests/security/full-security-regression.test.ts)Greptile Summary
This PR implements a comprehensive security authentication overhaul and provider batch operations system. The security changes migrate from API-key-as-session-token to opaque session IDs stored in Redis, adding multiple layers of protection including CSRF origin guards, constant-time token comparison, bruteforce protection with LRU eviction, security headers (CSP/HSTS/X-Frame), and session rotation with server-side revocation. The provider batch operations add a preview/apply/undo pipeline with preimage capture, supporting 9-field sectioned editing with per-provider field compatibility checks. The login UI was redesigned with split panels, animations, and redirect safety.
Key accomplishments:
Issues found:
keyparameter inrecordFailure()andrecordSuccess()calls (lines 191, 212, 273) - prevents key-based rate limiting from working correctly for invalid/missing keys and successful loginsConfidence Score: 4/5
keyparameter in threerecordFailure/recordSuccesscalls creates a gap in the bruteforce protection where key-based rate limiting won't trigger correctly. This is a straightforward fix but important for the security posture.Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant LoginRoute as /api/auth/login participant CSRFGuard participant LoginPolicy as Bruteforce Policy participant AuthLib as auth.ts participant RedisSession as Redis Session Store participant DB as Database Client->>LoginRoute: POST /api/auth/login {key} LoginRoute->>CSRFGuard: check(request) CSRFGuard-->>LoginRoute: allowed/rejected alt CSRF rejected LoginRoute-->>Client: 403 CSRF_REJECTED end LoginRoute->>LoginPolicy: check(clientIp, key) LoginPolicy-->>LoginRoute: allowed/rate_limited alt Rate limited LoginRoute-->>Client: 429 RATE_LIMITED + Retry-After end LoginRoute->>AuthLib: validateKey(key) AuthLib->>DB: validateApiKeyAndGetUser(key) DB-->>AuthLib: {user, key} or null alt Invalid key AuthLib-->>LoginRoute: null LoginRoute->>LoginPolicy: recordFailure(clientIp, key) LoginRoute-->>Client: 401 KEY_INVALID end alt Valid key - opaque mode AuthLib-->>LoginRoute: {user, key} LoginRoute->>RedisSession: create(keyFingerprint, userId, userRole) RedisSession->>RedisSession: generate sessionId (sid_uuid) RedisSession-->>LoginRoute: sessionData {sessionId, expiresAt} LoginRoute->>LoginRoute: setAuthCookie(sessionId) LoginRoute->>LoginPolicy: recordSuccess(clientIp, key) LoginRoute-->>Client: 200 {ok, user, redirectTo, loginType} end Note over Client,DB: Subsequent authenticated requests Client->>AuthLib: getSession() [with cookie] AuthLib->>RedisSession: read(sessionId) RedisSession-->>AuthLib: sessionData or null alt Session expired AuthLib-->>Client: Redirect to login end AuthLib->>AuthLib: convertToAuthSession(sessionData) AuthLib->>DB: findKeyList(userId) DB-->>AuthLib: [keys] AuthLib->>AuthLib: verify keyFingerprint matches AuthLib-->>Client: AuthSession {user, key} Note over Client,DB: Logout flow Client->>LoginRoute: POST /api/auth/logout LoginRoute->>CSRFGuard: check(request) LoginRoute->>RedisSession: revoke(sessionId) RedisSession->>RedisSession: DEL session key LoginRoute->>LoginRoute: clearAuthCookie() LoginRoute-->>Client: 200 {ok}Last reviewed commit: 4a0f06e