fix: keys:reveal + redirect fallback test + bill-on-failure toggle#1149
Conversation
…-failure toggle Bundles four fixes raised against dev: 1. **keys:reveal endpoint (Issue #1)** — Add `GET /api/v1/keys/{id}:reveal` mirroring the provider key:reveal pattern (audit log, no-store cache, admin-only). Rewire dashboard key row + key list to fetch the unmasked key on click instead of pre-loading it in user-list payloads. Removes `fullKey` from `UserKeyDisplay` and replaces it with a `canReveal` permission flag — unmasked keys never leave the server unless explicitly requested. 2. **Model redirect fallback regression test (Issue #2)** — Pin the contract: when fallback occurs, the next provider's redirect rules MUST match against the user-requested model (not the model the previous provider rewrote it to). Static analysis + 4 new test scenarios confirm the existing `ModelRedirector.apply` already implements the contract. Test file now guards against future regression. 3. **499 frequency investigation (Issue #3)** — Read-only audit of the recent abort-listener cleanup commits (bcba5d0, 8968a42). Conclusion: no regression — those commits only fixed listener leaks in normal- completion paths. The user's log (Codex CLI cancelling stream at ~3s) is genuine and correctly classified as 499. No code change for this issue; the new bill-on-failure toggle (#4) gives operators a way to recover token billing for these cases. 4. **billNonSuccessfulRequests toggle (Issue #4)** — New system setting (default OFF). When ON, requests with non-2xx status that received positive token usage from the upstream (e.g., 499 mid-stream client abort with partial usage) are billed normally. The existing fake-200 detector still skips billing for fake-success error payloads regardless of toggle state. Schema migration + cache + UI toggle (5 locales) + unit tests. Tests added: - `tests/unit/actions/keys-reveal.test.ts` — admin/auth/404 paths and audit redaction for the new action. - `tests/unit/proxy/model-redirect-fallback.test.ts` — 4 scenarios for fallback redirect behavior. - `tests/unit/proxy/response-handler-bill-non-success.test.ts` — toggle semantics, fake-200 still skipped, setting-read failure fail-closed. Migration: 0102_useful_lionheart.sql adds `bill_non_successful_requests boolean default false not null` to `system_settings` (idempotent ADD COLUMN IF NOT EXISTS). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough本 PR 包含两条相互独立的改动：1) 引入按需揭示 API 密钥的后端与前端流程 — 新增 ChangesAPI 密钥揭示功能
非成功请求计费功能
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
bug
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| const res = await getUnmaskedKey(keyId); | ||
| if (!res.ok || !res.data?.key) return null; | ||
| setRevealedKeys((prev) => ({ ...prev, [keyId]: res.data.key })); | ||
| return res.data.key; | ||
| } finally { | ||
| setRevealingKeyId(null); | ||
| } | ||
| }; | ||
|
|
||
| const success = await copyToClipboard(key.fullKey); | ||
| const handleCopyKey = async (key: UserKeyDisplay) => { |
There was a problem hiding this comment.
fetchUnmaskedKey returns null on API errors without giving the user any feedback — handleCopyKey and handleToggleVisibility both silently bail out. The parallel implementation in key-row-item.tsx correctly shows a toast.error(...) on failure. Without feedback, clicking the copy/reveal button will appear to do nothing when the request fails.
| try { | |
| const res = await getUnmaskedKey(keyId); | |
| if (!res.ok || !res.data?.key) return null; | |
| setRevealedKeys((prev) => ({ ...prev, [keyId]: res.data.key })); | |
| return res.data.key; | |
| } finally { | |
| setRevealingKeyId(null); | |
| } | |
| }; | |
| const success = await copyToClipboard(key.fullKey); | |
| const handleCopyKey = async (key: UserKeyDisplay) => { | |
| const fetchUnmaskedKey = async (keyId: number): Promise<string | null> => { | |
| if (revealedKeys[keyId]) return revealedKeys[keyId]; | |
| setRevealingKeyId(keyId); | |
| try { | |
| const res = await getUnmaskedKey(keyId); | |
| if (!res.ok || !res.data?.key) { | |
| toast.error((res as { ok: false; error?: string }).error ?? t("copyKeyFailed")); | |
| return null; | |
| } | |
| setRevealedKeys((prev) => ({ ...prev, [keyId]: res.data.key })); | |
| return res.data.key; | |
| } catch (err) { | |
| console.error("[KeyList] reveal failed", err); | |
| toast.error(t("copyKeyFailed")); | |
| return null; | |
| } finally { | |
| setRevealingKeyId(null); | |
| } | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/user/key-list.tsx
Line: 82-92
Comment:
**Silent failure on key reveal in key-list**
`fetchUnmaskedKey` returns `null` on API errors without giving the user any feedback — `handleCopyKey` and `handleToggleVisibility` both silently bail out. The parallel implementation in `key-row-item.tsx` correctly shows a `toast.error(...)` on failure. Without feedback, clicking the copy/reveal button will appear to do nothing when the request fails.
```suggestion
const fetchUnmaskedKey = async (keyId: number): Promise<string | null> => {
if (revealedKeys[keyId]) return revealedKeys[keyId];
setRevealingKeyId(keyId);
try {
const res = await getUnmaskedKey(keyId);
if (!res.ok || !res.data?.key) {
toast.error((res as { ok: false; error?: string }).error ?? t("copyKeyFailed"));
return null;
}
setRevealedKeys((prev) => ({ ...prev, [keyId]: res.data.key }));
return res.data.key;
} catch (err) {
console.error("[KeyList] reveal failed", err);
toast.error(t("copyKeyFailed"));
return null;
} finally {
setRevealingKeyId(null);
}
};
```
How can I resolve this? If you propose a fix, please make it concise.| let allowBillingNonSuccess = false; | ||
| try { | ||
| const settings = await getCachedSystemSettings(); | ||
| allowBillingNonSuccess = settings.billNonSuccessfulRequests === true; | ||
| } catch (error) { | ||
| logger.warn( | ||
| "[CostCalculation] Failed to read billNonSuccessfulRequests setting, defaulting to skip", | ||
| { error: error instanceof Error ? error.message : String(error) } | ||
| ); | ||
| } |
There was a problem hiding this comment.
getCachedSystemSettings() is awaited for every non-2xx response even when the feature will almost always remain OFF. While the 1-minute cache prevents database trips, the added promise chain still runs on every client abort (499), rate-limit (429), etc. This is minor since the cache is cheap, but worth noting for high-abort-rate deployments.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 411-420
Comment:
**Settings cache read on every non-2xx response**
`getCachedSystemSettings()` is awaited for every non-2xx response even when the feature will almost always remain OFF. While the 1-minute cache prevents database trips, the added promise chain still runs on every client abort (499), rate-limit (429), etc. This is minor since the cache is cheap, but worth noting for high-abort-rate deployments.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ac454171d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!session || session.user.role !== "admin") { | ||
| return { ok: false, error: "权限不足：仅管理员可查看完整密钥" }; |
There was a problem hiding this comment.
This new admin-only guard regresses the existing self-service flow for non-admin users: getUsers still marks a user's own keys as revealable (canExposeFullKey returns true for session.user.id === targetUser.id in src/actions/users.ts), and the UI now relies on getUnmaskedKey to fetch the full value on click, but this check rejects every non-admin caller. As a result, regular users on /dashboard/users see reveal/copy controls for their own keys that now always fail.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a security enhancement by removing full API keys from the initial list payload and implementing a lazy-loading mechanism via a new :reveal endpoint. It also adds a system setting to allow billing for non-successful requests (e.g., 499 client cancellations) when upstream token usage is reported. Feedback focuses on a regression where regular users can no longer reveal their own keys due to overly restrictive admin-only checks in the new action and router. Additionally, error handling in the UI should be improved to provide user feedback when the key retrieval fails.
| if (!session || session.user.role !== "admin") { | ||
| return { ok: false, error: "权限不足：仅管理员可查看完整密钥" }; | ||
| } | ||
|
|
||
| const key = await findKeyById(keyId); | ||
| if (!key) { | ||
| return { ok: false, error: "密钥不存在" }; | ||
| } |
There was a problem hiding this comment.
The getUnmaskedKey action currently restricts access to admins only. This prevents regular users from revealing or copying their own API keys in the dashboard, which is a regression since keys are no longer included in the initial list payload for security reasons. The permission check should be moved after findKeyById to allow both admins and the key owner to reveal the key. Additionally, the error messages are hardcoded in Chinese, which should be replaced with i18n keys or English to maintain consistency across locales.
| if (!session || session.user.role !== "admin") { | |
| return { ok: false, error: "权限不足：仅管理员可查看完整密钥" }; | |
| } | |
| const key = await findKeyById(keyId); | |
| if (!key) { | |
| return { ok: false, error: "密钥不存在" }; | |
| } | |
| const key = await findKeyById(keyId); | |
| if (!key) { | |
| return { ok: false, error: "Key not found" }; | |
| } | |
| const session = await getSession(); | |
| if (!session || (session.user.role !== "admin" && session.user.id !== key.userId)) { | |
| return { ok: false, error: "Permission denied" }; | |
| } |
| "x-required-access": "admin", | ||
| security, | ||
| request: { params: KeyIdParamSchema }, | ||
| responses: { | ||
| 200: { | ||
| description: "Unmasked key.", | ||
| content: { "application/json": { schema: KeyRevealResponseSchema } }, | ||
| }, | ||
| ...problemResponses, | ||
| }, | ||
| }); | ||
|
|
||
| keysRouter.openAPIRegistry.registerPath(revealKeyRoute); | ||
| keysRouter.get("/keys/:keyId{[0-9]+:reveal}", requireAuth("admin"), revealKey); |
There was a problem hiding this comment.
The :reveal endpoint is currently restricted to admin at the router level. To allow users to reveal their own keys (as suggested in the action logic), this restriction should be relaxed to requireAuth(), allowing the action to perform the specific ownership check. The OpenAPI documentation should also be updated to reflect that owners can access this endpoint.
| "x-required-access": "admin", | |
| security, | |
| request: { params: KeyIdParamSchema }, | |
| responses: { | |
| 200: { | |
| description: "Unmasked key.", | |
| content: { "application/json": { schema: KeyRevealResponseSchema } }, | |
| }, | |
| ...problemResponses, | |
| }, | |
| }); | |
| keysRouter.openAPIRegistry.registerPath(revealKeyRoute); | |
| keysRouter.get("/keys/:keyId{[0-9]+:reveal}", requireAuth("admin"), revealKey); | |
| "x-required-access": "admin | owner", | |
| security, | |
| request: { params: KeyIdParamSchema }, | |
| responses: { | |
| 200: { | |
| description: "Unmasked key.", | |
| content: { "application/json": { schema: KeyRevealResponseSchema } }, | |
| }, | |
| ...problemResponses, | |
| }, | |
| }); | |
| keysRouter.openAPIRegistry.registerPath(revealKeyRoute); | |
| keysRouter.get("/keys/:keyId{[0-9]+:reveal}", requireAuth(), revealKey); |
| setRevealingKeyId(keyId); | ||
| try { | ||
| const res = await getUnmaskedKey(keyId); | ||
| if (!res.ok || !res.data?.key) return null; |
There was a problem hiding this comment.
When getUnmaskedKey fails, fetchUnmaskedKey returns null silently. This results in a poor user experience where clicking the reveal or copy button does nothing. Consider adding a toast notification to inform the user of the failure, similar to the implementation in KeyRowItem.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/actions/users.ts (1)
413-421:⚠️ Potential issue | 🟠 Major | ⚡ Quick win让
canReveal/canCopy与 reveal 接口的真实权限保持一致。这里复用了旧的
canExposeFullKey(...)判定，但列表里已经不再下发fullKey，前端接下来只能调用GET /api/v1/keys/{id}:reveal。本 PR 又把这个 reveal 能力改成了管理员专用，所以普通用户查看自己的 key 时仍会收到canReveal: true/canCopy: true，按钮会显示出来，但点击后只能得到权限错误。建议把这两个字段改成与 reveal 端点一致的授权条件，或者在补齐非管理员 reveal 通路前不要暴露它们。
可参考的修正方向
- const canUserManageKey = canExposeFullKey(session, user, isAdmin); + const canRevealKey = isAdmin; return { id: key.id, name: key.name, maskedKey: maskKey(key.key), - canReveal: canUserManageKey, - canCopy: canUserManageKey, + canReveal: canRevealKey, + canCopy: canRevealKey,
getUsersBatch和getUsersBatchCore里的同类映射也需要一起收敛到同一套权限判断。Also applies to: 737-744, 886-891
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/users.ts` around lines 413 - 421, The canReveal/canCopy flags are computed with canExposeFullKey(...) but the frontend must reflect the actual reveal endpoint permission; update the mapping in the users list (the block that builds {id,name,maskedKey,canReveal,canCopy}) to use the same authorization predicate the reveal handler uses (the exact reveal-check used by GET /api/v1/keys/{id}:reveal) instead of canExposeFullKey, and apply the same change to the analogous mappings in getUsersBatch and getUsersBatchCore so all lists use the identical reveal permission logic (use session, user and the key id/owner as the reveal-check inputs).src/app/v1/_lib/proxy/response-handler.ts (1)
396-438:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
billNonSuccessfulRequests还没有覆盖finalizeRequestStats()这条计费路径。这里的新 gate 只保护了显式调用
resolveBillableUsageMetricsForCost()的分支，但同文件里finalizeRequestStats()在已解析出usageMetrics时，仍会在 Line 3779 和 Line 3794 直接基于normalizedUsage更新 DB / Redis。Gemini passthrough 这些调用点走的正是这条路径，所以在billNonSuccessfulRequests = false时，非 2xx 依然会被计费。建议把finalizeRequestStats()的 usage 分支也先过一遍这个 resolver，再决定是否执行updateRequestCostFromUsage()、trackCostToRedis()和 session cost 计算。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 396 - 438, The finalizeRequestStats() path currently updates costs directly from normalizedUsage (calling updateRequestCostFromUsage(), trackCostToRedis() and adjusting session cost) without consulting the new gate; ensure finalizeRequestStats() first calls resolveBillableUsageMetricsForCost(session, provider, normalizedUsage, statusCode, responseText) (or reuses its logic) and only proceeds with updateRequestCostFromUsage(), trackCostToRedis(), and session cost adjustments when that call returns a non-null UsageMetrics; also ensure the same hasPositiveBillableTokens check and billNonSuccessfulRequests behavior is applied so non-2xx requests are skipped when the setting is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/actions/keys.ts`:
- Around line 835-840: 当前不一致来自 getUnmaskedKey 对 reveal 强制仅 admin，而
canExposeFullKey (used to set canReveal/canCopy in the keys list) 仍允许“普通用户查看自己的
key”，导致列表显示入口但点击会 403；请在本 PR 中保持契约一致：修改 src/actions/users.ts 中的
canExposeFullKey(...)（及其在 keys 列表生成处）使其鉴权逻辑与 keys.getUnmaskedKey 相同（即仅 admin 返回
true），或者反向放宽 getUnmaskedKey，但二者必须一致——建议直接收紧 canExposeFullKey 到 admin-only 并更新生成
canReveal/canCopy 的代码以反映该变更。
In `@src/app/`[locale]/dashboard/_components/user/key-list.tsx:
- Around line 79-90: fetchUnmaskedKey currently returns null on failure and
callers (handleCopy/handleToggleVisibility) silently return, leaving the UI
appearing unresponsive; change fetchUnmaskedKey to surface a visible error
(e.g., set an error state or call the existing toast/notification helper) when
the API call fails or returns no key, and ensure it uses the i18n message keys
for copy/reveal errors; keep setting/clearing setRevealingKeyId and updating
revealedKeys as-is but add a localized visible notification before returning
null so callers no longer silently fail (apply the same pattern used around the
calls referenced at lines 92-95 and 103-110).
In `@src/app/`[locale]/dashboard/_components/user/key-row-item.tsx:
- Around line 209-235: The revealedKey cache can leak across different keys
because fetchUnmaskedKey only checks revealedKey and not which key it belongs
to; update the component to clear the cached revealedKey whenever the prop
keyData.id changes (e.g., add a useEffect that calls setRevealedKey(null) with
[keyData.id] as dependency) so fetchUnmaskedKey always fetches the correct
secret for the current key; ensure any related state like isRevealing is reset
or handled appropriately when keyData.id changes (use setIsRevealing(false) or
cancel outstanding requests if implemented).
In `@tests/unit/proxy/model-redirect-fallback.test.ts`:
- Around line 224-247: The test "Provider B without redirect rules resets
request.model to the original" only asserts session.request.model but misses
asserting session.request.message.model; update that test to also assert
session.request.message.model equals REQUESTED_MODEL after calling
ModelRedirector.apply(session, providerB) (this verifies resetToOriginal
correctly resets both session.request.model and session.request.message.model
when providerB has modelRedirects === null); reference the test name, the
ModelRedirector.apply call, and the resetToOriginal behavior to locate where to
add the extra assertion.
---
Outside diff comments:
In `@src/actions/users.ts`:
- Around line 413-421: The canReveal/canCopy flags are computed with
canExposeFullKey(...) but the frontend must reflect the actual reveal endpoint
permission; update the mapping in the users list (the block that builds
{id,name,maskedKey,canReveal,canCopy}) to use the same authorization predicate
the reveal handler uses (the exact reveal-check used by GET
/api/v1/keys/{id}:reveal) instead of canExposeFullKey, and apply the same change
to the analogous mappings in getUsersBatch and getUsersBatchCore so all lists
use the identical reveal permission logic (use session, user and the key
id/owner as the reveal-check inputs).
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 396-438: The finalizeRequestStats() path currently updates costs
directly from normalizedUsage (calling updateRequestCostFromUsage(),
trackCostToRedis() and adjusting session cost) without consulting the new gate;
ensure finalizeRequestStats() first calls
resolveBillableUsageMetricsForCost(session, provider, normalizedUsage,
statusCode, responseText) (or reuses its logic) and only proceeds with
updateRequestCostFromUsage(), trackCostToRedis(), and session cost adjustments
when that call returns a non-null UsageMetrics; also ensure the same
hasPositiveBillableTokens check and billNonSuccessfulRequests behavior is
applied so non-2xx requests are skipped when the setting is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f4eda3c-2014-4854-90d7-64beb266f3e1
📒 Files selected for processing (41)
drizzle/0102_useful_lionheart.sqldrizzle/meta/0102_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/auditLogs.jsonmessages/en/settings/config.jsonmessages/ja/auditLogs.jsonmessages/ja/settings/config.jsonmessages/ru/auditLogs.jsonmessages/ru/settings/config.jsonmessages/zh-CN/auditLogs.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/auditLogs.jsonmessages/zh-TW/settings/config.jsonsrc/actions/keys.tssrc/actions/system-config.tssrc/actions/users.tssrc/app/[locale]/dashboard/_components/user/key-list.tsxsrc/app/[locale]/dashboard/_components/user/key-row-item.tsxsrc/app/[locale]/dashboard/_components/user/user-key-table-row.tsxsrc/app/[locale]/dashboard/users/users-page-client.tsxsrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/api/actions/[...route]/route.tssrc/app/api/v1/resources/keys/handlers.tssrc/app/api/v1/resources/keys/router.tssrc/app/v1/_lib/proxy/response-handler.tssrc/drizzle/schema.tssrc/lib/api-client/v1/actions/keys.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/api/v1/action-migration-matrix.tssrc/lib/api/v1/schemas/keys.tssrc/lib/api/v1/schemas/system-config.tssrc/lib/config/system-settings-cache.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/system-config.tssrc/types/system-config.tssrc/types/user.tstests/unit/actions/keys-reveal.test.tstests/unit/proxy/model-redirect-fallback.test.tstests/unit/proxy/response-handler-bill-non-success.test.ts
💤 Files with no reviewable changes (1)
- src/app/[locale]/dashboard/users/users-page-client.tsx
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
[HIGH] [ERROR-SILENT] Silent failure on key reveal with no user feedback
Why this is a problem: When the API call to :reveal fails, the function returns null without any user feedback. Users clicking copy/reveal will see the button appear to do nothing - a poor UX that looks like a bug.
Compare to the parallel implementation in key-row-item.tsx (lines 224-237) which correctly shows toast.error() on failure.
Suggested fix:
Add error handling with toast notification per CLAUDE.md i18n requirements. Follow the pattern in key-row-item.tsx:
- Add 'toast' import from 'sonner'
- Add 'useTranslations' import from 'next-intl'
- Add try/catch with toast.error(t("copyKeyFailed")) on failure
There was a problem hiding this comment.
Code Review Summary
This XL-sized PR bundles four related changes: (1) a new keys:reveal endpoint for secure on-demand key retrieval, (2) regression tests for model redirect fallback behavior, (3) a billNonSuccessfulRequests system setting toggle, and (4) UI rewiring to fetch keys on-demand rather than shipping unmasked keys in list payloads.
Overall the implementation is solid with good test coverage. One UX issue identified where API failures are silent.
PR Size: XL
- Lines changed: 5,763 (5,713 additions, 50 deletions)
- Files changed: 41
Split Suggestion: This PR could be split into 3 independent PRs:
keys:revealendpoint + UI rewiring (security improvement)- Model redirect fallback regression tests (test-only)
billNonSuccessfulRequeststoggle feature (billing feature)
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
High Priority Issues (Should Fix)
[ERROR-SILENT] Silent failure on key reveal in key-list.tsx
- File:
src/app/[locale]/dashboard/_components/user/key-list.tsx - Issue: The
fetchUnmaskedKeyfunction returnsnullon API errors without any user feedback. Users clicking copy/reveal will see the button appear to do nothing on failure. - Fix: Add toast notification on error, following the pattern in
key-row-item.tsx(which correctly handles this case withtoast.error()).
Positive Notes
- Security: Unmasked keys are no longer shipped in user-list payloads - good security improvement
- Fail-closed design: The
billNonSuccessfulRequestssetting defaults to off and fails closed on settings read errors - Test coverage: 15 new unit tests covering all new code paths (4 for keys-reveal, 7 for bill-non-success, 4 for model-redirect)
- i18n compliance: All user-facing strings use i18n across 5 languages
- Audit logging: Key reveal operations are properly audited with redaction
- Cache headers: The reveal endpoint correctly returns
Cache-Control: no-store
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean, no issues found
- Error handling - One UX issue found (silent failure)
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate (15 new tests)
- Code clarity - Good
Automated review by Claude AI
Resolves feedback from CodeRabbit, Greptile, gemini-code-assist, and chatgpt-codex on PR #1149: 1. **Allow key owner to reveal their own key (HIGH)** — `canExposeFullKey` already marks an owner's keys as `canReveal: true` in list payloads, but the new `getUnmaskedKey` action and v1 router gated everything behind admin, causing a 403 regression for self-service users. - `getUnmaskedKey`: replace admin-only check with admin-OR-owner. - v1 `:reveal` route: relax `requireAuth("admin")` -> `requireAuth("read")`, update OpenAPI `x-required-access` to `admin | owner` and the description to reflect that ownership is checked at the action layer. - Test: replace "rejects non-admin" with two cases — owner allowed, non-owner rejected. 2. **Toast feedback on reveal/copy failure in key-list.tsx (HIGH)** — `fetchUnmaskedKey` was silently returning null. Mirror the key-row-item pattern: import `toast` from sonner and surface failures via `tCommon("copyFailed")`. 3. **Drop stale revealedKey cache when row identity changes (MAJOR)** — two call sites: - `key-row-item.tsx`: `useEffect` resets `revealedKey` and closes the dialog whenever `keyData.id` or `keyData.maskedKey` changes (parent may reuse the same component instance for a different row). - `key-list.tsx`: prune `revealedKeys` and `visibleKeyIds` whenever the incoming `keys` set changes so removed/replaced ids cannot leak their cached plaintext onto a new row. 4. **Missing assertion in model-redirect-fallback test 3 (MINOR)** — the "no rules" reset path must rewrite both `request.model` and `request.message.model`. Add the missing `request.message.model` check to guard against half-reset regressions. Tests: 16/16 pass (3 files), typecheck clean, lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/[locale]/dashboard/_components/user/key-row-item.tsx (1)
213-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick win仍有旧请求回写竞态，可能把上一行明文写入当前行
当前只在 key 切换时清空缓存，但没有防止“旧请求晚到并回写”。在组件复用场景下，这会造成跨行明文泄露。
建议修复（增加行身份守卫）
-import { useEffect, useState } from "react"; +import { useEffect, useRef, useState } from "react"; const [revealedKey, setRevealedKey] = useState<string | null>(null); const [isRevealing, setIsRevealing] = useState(false); + const activeKeyIdRef = useRef<number>(keyData.id); useEffect(() => { + activeKeyIdRef.current = keyData.id; setRevealedKey(null); setFullKeyDialogOpen(false); + setIsRevealing(false); }, [keyData.id, keyData.maskedKey]); const fetchUnmaskedKey = async (): Promise<string | null> => { if (revealedKey) return revealedKey; + const requestedKeyId = keyData.id; setIsRevealing(true); try { - const res = await getUnmaskedKey(keyData.id); + const res = await getUnmaskedKey(requestedKeyId); + if (activeKeyIdRef.current !== requestedKeyId) return null; if (!res.ok) { toast.error(res.error || translations.actions.copyFailed); return null; } if (!res.data?.key) { toast.error(translations.actions.copyFailed); return null; } setRevealedKey(res.data.key); return res.data.key; } catch (error) { console.error("[KeyRowItem] reveal failed", error); toast.error(translations.actions.copyFailed); return null; } finally { - setIsRevealing(false); + if (activeKeyIdRef.current === requestedKeyId) { + setIsRevealing(false); + } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/user/key-row-item.tsx around lines 213 - 243, The fetchUnmaskedKey async can suffer a stale-response race where a slow previous getUnmaskedKey response overwrites the current row's revealedKey; fix by adding an identity guard around the async call: capture a local token (e.g. current keyData.id or an incrementing requestId) at the start of fetchUnmaskedKey and, before calling setRevealedKey or changing state based on the response, verify the token still matches the current row (keyData.id or latest requestId). Alternatively implement an AbortController per fetch and abort any prior fetch when keyData.id/maskedKey changes (in the useEffect cleanup), and only update state if the fetch was not aborted. Ensure setIsRevealing and setFullKeyDialogOpen updates respect the same guard so stale responses cannot mutate the new row.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/actions/keys.ts`:
- Around line 879-891: The catch block is currently returning the raw
error.message to callers (via variable message) which leaks implementation
details; instead keep detailed error information in
logger.error(logger.error("获取密钥失败:", error)) and change the returned error to a
stable, non-sensitive token (e.g. "KEY_REVEAL_FAILED" or a generic
"INTERNAL_ERROR") so that the API response does not expose internal
messages—update the return to use that constant and remove the conditional that
sets message from error.message; leave emitActionAudit(...) as-is using the
existing error token.
In `@src/app/api/v1/resources/keys/router.ts`:
- Around line 236-239: KeyRevealResponseSchema 的字段描述仍写着“仅 admin”，与该路由在
router.ts（路由已标注为 admin | owner）权限不一致，会误导生成的 OpenAPI/SDK；请在
KeyRevealResponseSchema 中更新相关字段描述（或总体 schema 描述）去掉“仅 admin”字样并改为准确的权限语义（例如
“admin | owner” 或移除角色限定），确保与路由描述一致，同时检查任何使用 KeyRevealResponseSchema 的地方（如 200
响应定义）以同步更新文案。
---
Duplicate comments:
In `@src/app/`[locale]/dashboard/_components/user/key-row-item.tsx:
- Around line 213-243: The fetchUnmaskedKey async can suffer a stale-response
race where a slow previous getUnmaskedKey response overwrites the current row's
revealedKey; fix by adding an identity guard around the async call: capture a
local token (e.g. current keyData.id or an incrementing requestId) at the start
of fetchUnmaskedKey and, before calling setRevealedKey or changing state based
on the response, verify the token still matches the current row (keyData.id or
latest requestId). Alternatively implement an AbortController per fetch and
abort any prior fetch when keyData.id/maskedKey changes (in the useEffect
cleanup), and only update state if the fetch was not aborted. Ensure
setIsRevealing and setFullKeyDialogOpen updates respect the same guard so stale
responses cannot mutate the new row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3d929bd-815a-4bc3-a0af-d036c7bbf993
📒 Files selected for processing (6)
src/actions/keys.tssrc/app/[locale]/dashboard/_components/user/key-list.tsxsrc/app/[locale]/dashboard/_components/user/key-row-item.tsxsrc/app/api/v1/resources/keys/router.tstests/unit/actions/keys-reveal.test.tstests/unit/proxy/model-redirect-fallback.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/unit/proxy/model-redirect-fallback.test.ts
| } catch (error) { | ||
| logger.error("获取密钥失败:", error); | ||
| const message = error instanceof Error ? error.message : "获取密钥失败"; | ||
| emitActionAudit({ | ||
| category: "key", | ||
| action: "key.key_reveal", | ||
| targetType: "key", | ||
| targetId: String(keyId), | ||
| success: false, | ||
| errorMessage: "KEY_REVEAL_FAILED", | ||
| }); | ||
| return { ok: false, error: message }; | ||
| } |
There was a problem hiding this comment.
避免把底层异常原文返回给调用方
Line 881-882/890 直接回传 error.message，会把内部实现细节暴露到 API 响应。建议仅返回稳定错误文案，详细错误保留在日志中。
建议修复
} catch (error) {
logger.error("获取密钥失败:", error);
- const message = error instanceof Error ? error.message : "获取密钥失败";
+ const message = "获取密钥失败";
emitActionAudit({
category: "key",
action: "key.key_reveal",
targetType: "key",
targetId: String(keyId),
success: false,
errorMessage: "KEY_REVEAL_FAILED",
});
return { ok: false, error: message };
}📝 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.
| } catch (error) { | |
| logger.error("获取密钥失败:", error); | |
| const message = error instanceof Error ? error.message : "获取密钥失败"; | |
| emitActionAudit({ | |
| category: "key", | |
| action: "key.key_reveal", | |
| targetType: "key", | |
| targetId: String(keyId), | |
| success: false, | |
| errorMessage: "KEY_REVEAL_FAILED", | |
| }); | |
| return { ok: false, error: message }; | |
| } | |
| } catch (error) { | |
| logger.error("获取密钥失败:", error); | |
| const message = "获取密钥失败"; | |
| emitActionAudit({ | |
| category: "key", | |
| action: "key.key_reveal", | |
| targetType: "key", | |
| targetId: String(keyId), | |
| success: false, | |
| errorMessage: "KEY_REVEAL_FAILED", | |
| }); | |
| return { ok: false, error: message }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/keys.ts` around lines 879 - 891, The catch block is currently
returning the raw error.message to callers (via variable message) which leaks
implementation details; instead keep detailed error information in
logger.error(logger.error("获取密钥失败:", error)) and change the returned error to a
stable, non-sensitive token (e.g. "KEY_REVEAL_FAILED" or a generic
"INTERNAL_ERROR") so that the API response does not expose internal
messages—update the return to use that constant and remove the conditional that
sets message from error.message; leave emitActionAudit(...) as-is using the
existing error token.
| 200: { | ||
| description: "Unmasked key.", | ||
| content: { "application/json": { schema: KeyRevealResponseSchema } }, | ||
| }, |
There was a problem hiding this comment.
同步修正响应 schema 描述，避免 OpenAPI 权限语义冲突
当前路由描述（Line 231）已明确是 admin | owner，但 KeyRevealResponseSchema 的字段描述仍是“仅 admin”，会让生成文档/SDK 产生误导。
建议修复
--- a/src/lib/api/v1/schemas/keys.ts
+++ b/src/lib/api/v1/schemas/keys.ts
export const KeyRevealResponseSchema = z.object({
- key: z.string().describe("Unmasked key value. Returned only to admin callers."),
+ key: z.string().describe("Unmasked key value. Returned only to admin callers or the key owner."),
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/v1/resources/keys/router.ts` around lines 236 - 239,
KeyRevealResponseSchema 的字段描述仍写着“仅 admin”，与该路由在 router.ts（路由已标注为 admin |
owner）权限不一致，会误导生成的 OpenAPI/SDK；请在 KeyRevealResponseSchema 中更新相关字段描述（或总体 schema
描述）去掉“仅 admin”字样并改为准确的权限语义（例如 “admin | owner” 或移除角色限定），确保与路由描述一致，同时检查任何使用
KeyRevealResponseSchema 的地方（如 200 响应定义）以同步更新文案。
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12bc4d2f52
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const isAdmin = session.user.role === "admin"; | ||
| const isOwner = session.user.id === key.userId; | ||
| if (!isAdmin && !isOwner) { | ||
| return { ok: false, error: "无权限执行此操作" }; | ||
| } |
There was a problem hiding this comment.
getUnmaskedKey now authorizes any owner (session.user.id === key.userId) without checking session.key.canLoginWebUi, so a read-only credential that cannot log into the dashboard can still call GET /api/v1/keys/{id}:reveal and retrieve plaintext keys for that user. This breaks the existing exposure contract used in canExposeFullKey (which requires session.key.canLoginWebUi) and turns low-privilege API keys into a way to exfiltrate sibling keys.
Useful? React with 👍 / 👎.
… enum CI on commit 12bc4d2 failed two checks: - openapi-contract: 'admin | owner' is not in the allowed enum (public/read/admin) for x-required-access. - openapi-types-drift: generated types out of date because the route description / annotation changed. Fix: keep the auth tier as 'read' (already what `requireAuth("read")` applies on the route) and document the admin-OR-owner contract in the OpenAPI 'description' field instead. Regenerate openapi-types.gen.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
| ...problemResponses, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
[High] [LOGIC-BUG] canReveal/:reveal permission mismatch breaks key reveal/copy for non-admin users
Why this is a problem:
src/actions/users.ts:420setscanReveal: canUserManageKey, which is used by the dashboard to decide whether to render reveal/copy UI.src/app/[locale]/dashboard/_components/user/key-list.tsx:217renders the copy/show controls whenrecord.canRevealis true.- But the new management endpoint is guarded as admin-only:
src/app/api/v1/resources/keys/router.ts:245iskeysRouter.get("/keys/:keyId{[0-9]+:reveal}", requireAuth("admin"), revealKey);. - The new unit test also codifies admin-only behavior:
tests/unit/actions/keys-reveal.test.ts:74(it("rejects non-admin callers" ...)).
Net effect: a non-admin user will receive canReveal=true for their own keys (UI shows buttons), but the GET /api/v1/keys/{id}:reveal call is denied (403), so reveal/copy is effectively broken.
Suggested fix (choose one and make all call sites consistent):
// Option A (minimal, matches current router + tests): only advertise reveal/copy for admins
// src/actions/users.ts
const canUserManageKey = canExposeFullKey(session, user, isAdmin);
const canReveal = isAdmin && canUserManageKey;
return {
...,
canReveal,
canCopy: canReveal,
...,
};If non-admin users are intended to reveal their own keys, then requireAuth("admin") on src/app/api/v1/resources/keys/router.ts:245 needs to be relaxed and getUnmaskedKey must enforce session.user.id === key.userId || role === "admin".
There was a problem hiding this comment.
Code Review Summary
This PR bundles multiple independent changes (keys reveal flow, billing toggle, schema/migration artifacts, and proxy/test updates) into a single XL diff, which makes regression detection and review verification harder. There is a permission/contract mismatch in the new key reveal path that can surface broken reveal/copy UI for non-admin users.
PR Size: XL
- Lines changed: 5763
- Files changed: 41
- Split suggestion (recommended): split into (1)
keys:{id}:revealAPI + dashboard wiring, (2)billNonSuccessfulRequestsschema/migration + proxy cost-gate + settings UI, (3) model redirect fallback tests.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 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 |
Critical Issues (Must Fix)
- [High] [LOGIC-BUG]
canReveal/:revealpermission mismatch breaks key reveal/copy for non-admin users- Evidence:
src/actions/users.ts:420(canReveal: canUserManageKey,) + UI gatingsrc/app/[locale]/dashboard/_components/user/key-list.tsx:217({record.canReveal && ...}) vs admin-only endpointsrc/app/api/v1/resources/keys/router.ts:245(requireAuth("admin")) and admin-only action contract intests/unit/actions/keys-reveal.test.ts:74.
- Evidence:
High Priority Issues (Should Fix)
None.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Identified current PR as
#1149(fix: keys:reveal + redirect fallback test + bill-on-failure toggle) and applied the size labelsize/XL(41 files, 5763 lines changed). - Posted 1 inline review comment calling out a permission/contract mismatch that can break key reveal/copy for non-admin users:
src/app/api/v1/resources/keys/router.ts:245. - Submitted the required review summary via
gh pr review --comment(includes XL split suggestions and the issue matrix).
1 participant
Summary
Four fixes bundled into one PR against
dev:keys:revealendpoint — User-management page key reveal/copy was missing for the cases where the key was not pre-loaded in the list payload. Mirrors the providerkey:revealpattern (audit log, no-store cache, admin-only) and rewires the dashboard to fetch on click. Side benefit: unmasked keys are no longer shipped in user-list payloads.Model redirect fallback regression test — User reported that fallback may carry over the previous provider's redirected model. Static analysis + 4 new scenarios confirm the existing
ModelRedirector.applyalready implements the correct contract (each provider matches against the user's original model). Test file pins the contract to catch any future regression.499 frequency investigation — Audited the recent abort-listener cleanup commits (
bcba5d0d,8968a426). They only fixed listener leaks in normal-completion paths and do not change abort detection. The 499 in the user's log is a genuine Codex CLI mid-stream cancellation. No code change required; the new bill-on-failure toggle below recovers token billing in these cases.billNonSuccessfulRequestssystem setting — New toggle (default OFF). When ON, non-2xx responses (e.g., 499 client abort) that already received positive token usage from the upstream are billed by usage. The existing fake-200 detector remains in place — fake-success error payloads are never billed regardless of toggle state.Files
src/drizzle/schema.ts+ new migrationdrizzle/0102_useful_lionheart.sqlGET /api/v1/keys/{id}:reveal—router.ts,handlers.ts,keys.tsschema, action-migration matrixgetUnmaskedKeyinsrc/actions/keys.ts(mirrorsgetUnmaskedProviderKey)key-row-item.tsx,key-list.tsx,user-key-table-row.tsx— fetch on click;users.tsno longer shipsfullKeysystem-settings-form.tsx+ 5-locale i18n stringssrc/app/v1/_lib/proxy/response-handler.ts:resolveBillableUsageMetricsForCost(now exported for tests)Tests
tests/unit/actions/keys-reveal.test.ts— admin/auth/404/audit-redaction (4 cases)tests/unit/proxy/model-redirect-fallback.test.ts— original-model match, redirected-name no-match, no-rules reset, distinct targets (4 cases)tests/unit/proxy/response-handler-bill-non-success.test.ts— toggle off, toggle on, null/zero usage, cache-only tokens, fake-200 still skipped, setting-read fail-closed (7 cases)Test plan
bun run typecheckcleanbun run lintcleanbun run buildsucceeds (production bundle)bun run test— only 3 pre-existing failures remain (openai-image-compat,auth-middleware-api-key-admin,api-key-auth); all also fail ondevbaseline. New tests all pass (15/15).bun run openapi:generate)GET /api/v1/keys/{id}:reveal→{ key: "..." }withCache-Control: no-storefullKeyno longer present in/api/v1/users/.../keysresponsebillNonSuccessfulRequestson; abort a streaming request mid-flight; confirm DB row has cost > 0 (toggle off → cost 0)Notes
IF NOT EXISTSfor idempotency.response-handler.tsfails closed on Redis/DB read errors (defaults to skip billing).🤖 Generated with Claude Code
Greptile Summary
This PR bundles four changes: a
GET /api/v1/keys/{id}:revealendpoint that lets admins and key owners fetch the unmasked key on demand (eliminatingfullKeyfrom list payloads); abillNonSuccessfulRequestssystem setting that gates billing on non-2xx responses carrying positive upstream token usage; regression tests for model-redirect fallback; and the 499 investigation write-up. The implementation follows existing patterns closely (fallback cascade, audit redaction, no-store cache header), and all 15 new tests pass.Confidence Score: 5/5
Safe to merge — one minor OpenAPI description inaccuracy, no logic or security defects.
All findings are P2 (documentation wording). Core logic — auth ownership check, audit redaction, fail-closed billing toggle, and multi-level DB fallback cascade — is correct and well-tested across 15 new passing test cases.
src/lib/api/v1/schemas/keys.ts — one-line description fix needed for the generated OpenAPI spec.
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Dashboard UI participant REST as GET /api/v1/keys/{id}:reveal participant Action as getUnmaskedKey action participant DB as Database participant Audit as Audit Log UI->>REST: GET (requireAuth "read") REST->>Action: callAction(keyId) Action->>DB: findKeyById(keyId) DB-->>Action: key row Action->>Action: isAdmin OR isOwner? alt Not authorized Action-->>REST: {ok:false, error:"无权限"} REST-->>UI: 403 else Authorized Action->>Audit: emitActionAudit (key value redacted) Action-->>REST: {ok:true, data:{key:"sk-..."}} REST-->>UI: 200 Cache-Control:no-store endPrompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(pr-review): align keys:reveal x-requ..." | Re-trigger Greptile