feat: IP 记录 & 审计日志 & IP 归属地查询#1027
Merged
Merged
Conversation
Phase 1–4 of IP recording & audit-log feature: Schema: - Add `client_ip` column to `message_request` and `usage_ledger` (plus a filtered index on message_request). The `fn_upsert_usage_ledger` trigger is updated (in-migration) to propagate `client_ip` and the previously-unwired `group_cost_multiplier` from message_request. - Add `ip_extraction_config` (jsonb) and `ip_geo_lookup_enabled` (bool) to `system_settings`. - New `audit_log` table with indexes for category/operator/target/time keyset pagination. - Also bundles the orphaned schema delta from #1025 (provider_groups table, group_cost_multiplier, cost_breakdown) which was committed to schema.ts without a matching migration; migration 0089 brings the on-disk state back in sync. Unified IP extractor (src/lib/ip/): - `extractClientIp(headers, config)` — configurable rule chain supporting leftmost / rightmost / 0-based-index picks for XFF-style headers, with IPv4/IPv6 parsing, port/bracket stripping, and graceful fallback. - `isPrivateIp(ip)` — RFC 1918, loopback, ULA, link-local, CGN classifier. - `getClientIp(headers)` convenience wrapper that reads system settings' `ipExtractionConfig` from the in-memory cache. - 43 unit tests covering header variants, XFF picks, invalid input, IPv6, case-insensitivity, and settings integration. Proxy pipeline: - `ProxySession.clientIp` is populated by `ProxyAuthenticator` using the new unified extractor (replaces the ad-hoc prefer-x-real-ip-then-XFF logic in auth-guard.ts). - `ProxyMessageService` forwards the IP into `createMessageRequest`; the repository persists it and includes it in SELECT paths. - `/api/auth/login` drops its copy of the extractor and uses the unified util too (still respects platform-provided IP first). IP geolocation client (src/lib/ip-geo/): - `lookupIp(ip, { lang })` — Redis-cached (TTL configurable, 3600s default), skips upstream for private IPs, caches negative results for 60s, aborts via AbortController on timeout, never throws. - Env: `IP_GEO_API_URL` (default https://ip.api.claude-code-hub.app), `IP_GEO_API_TOKEN` (optional Bearer), `IP_GEO_CACHE_TTL_SECONDS`, `IP_GEO_TIMEOUT_MS`. - `/api/ip-geo/:ip` route exposes the client to the dashboard for authenticated users; gated by the new `ipGeoLookupEnabled` system setting. - 8 tests covering cache hits, Bearer auth, private short-circuit, upstream 5xx / network / malformed / timeout fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 5–7: Audit infrastructure (src/lib/audit/): - `request-context.ts` — AsyncLocalStorage holding operator IP + UA. Populated by the action adapter once per request so audit hooks don't re-parse headers. - `redact.ts` — deep-copy with sensitive keys (key/apiKey/token/secret/ password/authorization/webhook_secret) replaced by `[REDACTED]`; extraKeys passed through by callers (e.g. provider.key, key.key). - `with-audit.ts` — generic wrapper that snapshots before/after, emits a success row on completion and a failure row + rethrow on exception. - `emit.ts` — one-shot helper used by action authors who prefer inline success/failure emits over function wrapping. Tolerant of partial `@/lib/auth` mocks in tests (falls back to null session). Audit-log repository (src/repository/audit-log.ts): - `createAuditLogAsync` — fire-and-forget insert; never blocks or fails the hot path. - `listAuditLogs` — keyset-paginated read with category/operator/target/time filters. - `getAuditLog`, `countAuditLogs` — single-row + count helpers. Action adapter (src/lib/api/action-adapter-openapi.ts): - Populate `runWithRequestContext` with operator IP + UA around every action execution. Defensive against missing Hono context fields (tests may mock `c.req` with only a subset of methods). Login audit (src/app/api/auth/login/route.ts): - `auth.login.success` on every successful admin-token or user-key login (operator identity captured from the resolved session). - `auth.login.failure` on missing key, invalid key. - `auth.login.rate_limited` on `LoginAbusePolicy` deny. Mutation audit (src/actions/*.ts): - users.ts: addUser / editUser / removeUser - providers.ts: addProvider / editProvider / removeProvider - provider-groups.ts: createProviderGroup / updateProviderGroup / deleteProviderGroup - keys.ts: addKey / editKey / removeKey (raw key never logged; redactor strips `key` from before/after) - system-config.ts: saveSystemSettings (full before/after snapshot) - notifications.ts: updateNotificationSettingsAction - sensitive-words.ts: create / update / delete - model-prices.ts: upsertSingleModelPrice / deleteSingleModelPrice / uploadPriceTable / syncLiteLLMPrices Tests: +11 unit tests for redact + withAudit (`src/lib/audit/*.test.ts`). Full suite: 4468 pass, 1 pre-existing failure unrelated to this change (`tests/api/api-endpoints.test.ts` 健康检查 — fails on dev before this PR because the test environment lacks DSN). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… + i18n Phase 8 (partial): IP details dialog (src/app/[locale]/dashboard/_components/ip-details-dialog.tsx): - Reusable Radix Dialog that renders the full `IpGeoLookupResult` from the `/api/ip-geo/:ip` endpoint: country + flag, region / city / postal / coordinates, timezone, ASN + organization + route + RIR, privacy badges (VPN / Proxy / Tor / Relay / Threat / Clean), threat score + risk level, and abuse contact. - useQuery is gated to `open &&` — tests that never open the dialog don't need a QueryClientProvider in their harness. Request-detail drawer (SummaryTab / ErrorDetailsDialog): - Show client IP alongside User-Agent and Endpoint in the client-info card. - IP renders as a clickable underlined button that opens the IP-details dialog. - `clientIp` plumbed through ErrorDetailsDialog → SummaryTab via the TabSharedProps type, and populated from the usage-log rows. System settings form (src/app/[locale]/settings/config/_components/system-settings-form.tsx): - New "IP logging & extraction" section with: - Toggle for `ipGeoLookupEnabled` - JSON textarea for `ipExtractionConfig` + "Reset to default" button - Helper text describing the fallback chain semantics - Wired through `saveSystemSettings`, `UpdateSystemSettingsSchema`, and the system-config repo update/returning columns. i18n: - New `ipDetails` and `auditLogs` namespaces for all 5 locales (en / zh-CN / zh-TW / ja / ru), registered in each locale's index. - New `ipLogging` section under `settings/config.json` for all 5 locales. Repo/types: - `UsageLogRow.clientIp` added; select list on both messageRequest and ledger-fallback query paths updated. - `messageRequest.clientIp` + `usageLedger.clientIp` already added in phase 1. Tests: test fixtures for usage-logs tables updated with `clientIp: null`. Full suite: 4468 pass, 1 pre-existing unrelated failure (tests/api/api-endpoints.test.ts 健康检查 — fails on dev because the test environment lacks DSN, unchanged by this PR). Not yet included in this commit: audit-log dashboard page, IP column in usage-logs table. Those land in phase 9 if time permits; the column in the request-detail drawer already unblocks IP visibility. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 8 (complete): Audit logs dashboard: - `src/actions/audit-logs.ts` — admin-only server action pair (`getAuditLogsBatch` + `getAuditLogDetail`) wrapping the existing audit-log repository. ISO date strings on the filter converted to `Date` internally. - `src/app/[locale]/dashboard/audit-logs/page.tsx` — admin-gated server page (redirects non-admins to /dashboard). - `_components/audit-logs-view.tsx` — virtualized TanStack-Query infinite list with category + status filters; IP clicks open `IpDetailsDialog`; row clicks open the detail sheet. - `_components/audit-log-detail-sheet.tsx` — Sheet showing full entry: category, action, target, operator (name + key name + user id with "Admin Token" fallback), IP (clickable), user-agent, error message, and before/after JSON in `<pre>` blocks. - Registered both actions in `src/app/api/actions/[...route]/route.ts` (admin-only). - Added admin-only "Audit logs" nav entry in `dashboard-header.tsx`. IP column on usage logs table: - Non-virtualized (`usage-logs-table.tsx`) and virtualized (`virtualized-logs-table.tsx`) both gain a new IP column between sessionId and provider. Cell is a clickable underlined monospace IP that opens `IpDetailsDialog`; null → `—`. Empty-row colSpan bumped. - `src/lib/column-visibility.ts` — added `"ip"` to the column union + default visible set. - `column-visibility-dropdown.tsx` — added the `ip` label key. Audit-log repository — `createAuditLogAsync` is now an `async` function (returns a Promise callers should `void`) so the file satisfies Next's "use server" constraint. Behaviour unchanged: still fire-and-forget, still swallows errors with a warn-level log. Fixes a production-build failure. i18n: - `dashboard.json` gains `nav.auditLogs` + `logs.columns.ip` across all 5 locales. Pre-commit (per CLAUDE.md): - `bun run build` ✓ (was failing before the async fix; now clean). - `bun run typecheck` ✓. - `bun run lint` ✓ (20 pre-existing warnings, no errors). - `bun run test` — 4468 pass, 1 pre-existing unrelated failure (`tests/api/api-endpoints.test.ts 健康检查`, fails on dev too: needs DSN in test env). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough新增审计日志表与持久化/查询实现，集成审计发射点并记录操作快照；增加可配置的客户端 IP 提取、私有 IP 检测与 IP 地理查找（API、缓存、前端 Hook、UI）；扩展数据库/仓库/类型以携带 client_ip 并新增相关索引与触发器变更。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
github-actions
Bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive audit logging system and enhanced IP geolocation features. Key additions include a new audit_log table, server-side mechanisms for capturing administrative mutations, and a dashboard UI for viewing logs. It also implements configurable client IP extraction and a geolocation lookup service with Redis caching. Feedback focuses on ensuring audit context is correctly captured during direct Server Action calls, fixing a virtualization bug in the audit log view that prevents the loading indicator from appearing, and improving localization for IP geolocation results.
Comment on lines
+40
to
+42
| export function emitActionAudit(args: EmitActionAuditArgs): void { | ||
| const session = safeGetScopedAuthSession(); | ||
| const { ip, userAgent } = getRequestContext(); |
Contributor
There was a problem hiding this comment.
审计日志机制目前依赖于 AsyncLocalStorage（通过 getScopedAuthSession 和 getRequestContext）来捕获操作人身份、IP 和 User-Agent。虽然这对于经过 API 适配器包装的请求有效，但对于从 Dashboard UI 直接调用的标准 Next.js Server Actions，由于没有填充这些上下文，会导致生成的审计日志丢失操作人信息且 IP/UA 为空。这使得审计日志在 Web 端操作场景下的有效性大打折扣。建议考虑在 Server Actions 中也填充这些上下文，或者让审计函数能够尝试从 next/headers 中获取信息。
| const getItemKey = useCallback((index: number) => rows[index]?.id ?? `loader-${index}`, [rows]); | ||
|
|
||
| const { parentRef, rowVirtualizer, virtualItems, handleScroll } = useVirtualizedInfiniteList({ | ||
| itemCount: rows.length, |
Contributor
There was a problem hiding this comment.
itemCount 应该在 hasNextPage 为真时包含一个额外的行，用于显示加载指示器。目前 itemCount 仅设置为 rows.length，这会导致第 200 行的 isLoaderRow 逻辑永远不会被触发，因为虚拟化列表的索引永远不会达到 rows.length。这会导致用户在滚动加载下一页时看不到加载动画。
Suggested change
| itemCount: rows.length, | |
| itemCount: rows.length + (hasNextPage ? 1 : 0), |
| return Response.json({ error: "ip geolocation disabled" }, { status: 404 }); | ||
| } | ||
|
|
||
| const result = await lookupIp(decodeURIComponent(ip)); |
Contributor
There was a problem hiding this comment.
IP 归属地查询目前默认使用英文 (en)，因为调用 lookupIp 时未传递语言参数。为了提供更好的本地化体验，建议 API 接收来自前端的 lang 参数（基于用户当前的 UI 语言），并将其传递给归属地查询服务。这样用户在查看 IP 详情时可以看到对应语言的国家和城市名称。
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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: eb03d9b315
ℹ️ 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".
|
|
||
| export const DEFAULT_IP_EXTRACTION_CONFIG: IpExtractionConfig = { | ||
| headers: [ | ||
| { name: "cf-connecting-ip" }, |
There was a problem hiding this comment.
The default extraction order trusts cf-connecting-ip first, but getClientIp() falls back to this chain for both login and proxy auth rate-limiting paths. On deployments not actually behind Cloudflare, clients can set this header themselves, spoof per-request IPs, bypass LoginAbusePolicy, and poison stored client_ip/audit attribution. The default should prefer trusted proxy headers (or only honor CF headers behind explicit trust gating).
Useful? React with 👍 / 👎.
Comment on lines
+15
to
+16
| function isPlainObject(value: unknown): value is Record<string, unknown> { | ||
| return typeof value === "object" && value !== null && !Array.isArray(value); |
There was a problem hiding this comment.
This object test matches every non-array object, so redactSensitive() recursively rewrites Date/class instances via Object.entries, which turns them into {}. Because many audit snapshots include date fields (for example createdAt/expiresAt from entities), before/after payloads lose important timestamp information and become misleading. Restrict recursion to plain-object prototypes (or explicitly serialize Date values) before redaction.
Useful? React with 👍 / 👎.
| ); | ||
| } | ||
|
|
||
| const operator = log.operatorUserName ?? "Admin Token"; |
There was a problem hiding this comment.
Using "Admin Token" as the fallback when operatorUserName is null mislabels unauthenticated/failed-auth audit rows as admin-token actions. In this feature set, failed login events can legitimately have no operator user, so this default creates false attribution in the audit UI and can mislead incident analysis. Use a neutral fallback unless admin-token identity is explicitly present in the row.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1
to
+5
| import { getScopedAuthSession } from "@/lib/auth"; | ||
| import { createAuditLogAsync } from "@/repository/audit-log"; | ||
| import type { AuditCategory } from "@/types/audit-log"; | ||
| import { redactSensitive } from "./redact"; | ||
| import { getRequestContext } from "./request-context"; |
There was a problem hiding this comment.
withAudit is exported but never used in production code
withAudit and its companion types (WithAuditOptions, AuditTargetResolution) are fully implemented and tested, but no action handler in the codebase imports or calls them — every mutation uses emitActionAudit from emit.ts instead. This leaves two overlapping audit APIs that diverge subtly: with-audit.ts calls getScopedAuthSession() directly (can throw if the module is partially mocked), while emit.ts uses the try/catch safeGetScopedAuthSession() wrapper. If a future contributor wires up withAudit they will hit unexpected throws in test environments. Either wire it up to replace emitActionAudit where appropriate, or remove it until it is needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/audit/with-audit.ts
Line: 1-5
Comment:
**`withAudit` is exported but never used in production code**
`withAudit` and its companion types (`WithAuditOptions`, `AuditTargetResolution`) are fully implemented and tested, but no action handler in the codebase imports or calls them — every mutation uses `emitActionAudit` from `emit.ts` instead. This leaves two overlapping audit APIs that diverge subtly: `with-audit.ts` calls `getScopedAuthSession()` directly (can throw if the module is partially mocked), while `emit.ts` uses the try/catch `safeGetScopedAuthSession()` wrapper. If a future contributor wires up `withAudit` they will hit unexpected throws in test environments. Either wire it up to replace `emitActionAudit` where appropriate, or remove it until it is needed.
How can I resolve this? If you propose a fix, please make it concise.| operatorUserName: session?.user.name ?? null, | ||
| operatorKeyId: session?.key.id ?? null, | ||
| operatorKeyName: session?.key.name ?? null, | ||
| operatorIp: ip, |
There was a problem hiding this comment.
void to signal fire-and-forget intent
createAuditLogAsync(...) returns a Promise<void> that is discarded here. The intent is fire-and-forget, but the missing void makes this an unhandled floating promise that strict linters (and readers) will flag. Add void to make the intent explicit.
Suggested change
| operatorIp: ip, | |
| void createAuditLogAsync({ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/audit/emit.ts
Line: 57
Comment:
**Floating promise — add `void` to signal fire-and-forget intent**
`createAuditLogAsync(...)` returns a `Promise<void>` that is discarded here. The intent is fire-and-forget, but the missing `void` makes this an unhandled floating promise that strict linters (and readers) will flag. Add `void` to make the intent explicit.
```suggestion
void createAuditLogAsync({
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+17
to
+20
| const session = await getSession(); | ||
| if (!session) { | ||
| return Response.json({ error: "unauthenticated" }, { status: 401 }); | ||
| } |
There was a problem hiding this comment.
The endpoint uses getSession() which passes for any authenticated user, not just admins. This means a regular user with dashboard access can query geolocation for any arbitrary IP address — including other users' IPs surfaced in the logs UI. The audit-log page itself is admin-gated, but the IP lookup API is not. Consider restricting to session.user.role === "admin" unless the design intent is to expose this to all users. Was it intentional to allow all authenticated users to query IP geolocation, or should this be admin-only like the audit-log page?
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/ip-geo/[ip]/route.ts
Line: 17-20
Comment:
**IP geo lookup is open to all authenticated roles**
The endpoint uses `getSession()` which passes for any authenticated user, not just admins. This means a regular user with dashboard access can query geolocation for any arbitrary IP address — including other users' IPs surfaced in the logs UI. The audit-log page itself is admin-gated, but the IP lookup API is not. Consider restricting to `session.user.role === "admin"` unless the design intent is to expose this to all users. Was it intentional to allow all authenticated users to query IP geolocation, or should this be admin-only like the audit-log page?
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/actions/notifications.ts (1)
42-74:⚠️ Potential issue | 🟡 Minor审计日志可能与实际结果不一致
若
updateNotificationSettings(payload)成功但随后的scheduleNotifications()抛错，会进入catch分支并以success: false写入审计日志；但数据库中的设置实际上已经更新。建议将审计 emit 的成功/失败判定与 DB 写入结果对齐（例如把 schedule 调用放在 emit 之后，或在 schedule 失败时单独记录而不是覆盖整体结果为失败）。建议调整顺序
const before = await getNotificationSettings(); const updated = await updateNotificationSettings(payload); + emitActionAudit({ + category: "notification", + action: "notification.update", + targetType: "notification", + before, + after: updated, + success: true, + }); + // 重新调度通知任务（仅生产环境） if (process.env.NODE_ENV === "production") { const { scheduleNotifications } = await import("@/lib/notification/notification-queue"); await scheduleNotifications(); } else { logger.warn({ ... }); } - - emitActionAudit({ - category: "notification", - action: "notification.update", - targetType: "notification", - before, - after: updated, - success: true, - }); return { ok: true, data: updated };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/notifications.ts` around lines 42 - 74, The audit currently marks the whole operation as failed if scheduleNotifications() throws after updateNotificationSettings(payload) succeeded; move the emitActionAudit(...) call that logs success (with after: updated) to occur immediately after updateNotificationSettings returns (before calling scheduleNotifications), and then call scheduleNotifications() in a try/catch that logs a separate audit/event or logger.warn/error if it fails; keep the existing catch to handle failures from updateNotificationSettings and only emit a failure audit there. Ensure you reference updateNotificationSettings, scheduleNotifications, and emitActionAudit when adjusting ordering and adding the separate error handling for scheduling.src/repository/message.ts (1)
320-388:⚠️ Potential issue | 🟡 MinorLedger 回退路径未透传
clientIp。迁移脚本已通过触发器把
client_ip同步到usage_ledger，PR 目标也明确提到要在 ledger-only 模式下用于展示。但此处三个 ledger 回退查询（findMessageRequestById、findMessageRequestBySessionId、findUsageLogs）都没有selectusageLedger.clientIp，也没有把它塞进toMessageRequest的入参，导致 ledger 回退模式下请求详情/使用日志的 IP 列永远为空。建议在这三处补齐。建议修改（以 findMessageRequestById 为例，其他两处类似）
sessionId: usageLedger.sessionId, + clientIp: usageLedger.clientIp, createdAt: usageLedger.createdAt, })userAgent: null, + clientIp: ledgerRow.clientIp, endpoint: ledgerRow.endpoint,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/message.ts` around lines 320 - 388, The ledger fallback is not selecting or mapping usageLedger.clientIp into the returned MessageRequest, so client IPs are always empty in ledger-only mode; update the DB select in findMessageRequestById to include usageLedger.clientIp and pass that value into toMessageRequest (e.g., clientIp: ledgerRow.clientIp), and make the same change in the other two fallback queries findMessageRequestBySessionId and findUsageLogs so each selects usageLedger.clientIp and maps it into the toMessageRequest call.src/actions/users.ts (1)
1152-1217:⚠️ Potential issue | 🟡 Minor权限拒绝/校验失败路径未写审计日志
addUser只在抛异常的 catch 分支里写审计（L1302–L1309），但权限检查失败（L1153–L1158）和 Zod 校验失败（L1183–L1217）都是直接return，不会产生审计记录。对比login/route.ts把 rate-limit 失败也作为login.failure审计，这里的策略不一致，会让“谁反复尝试创建非法用户”这类可疑行为难以追溯。建议在这些 early-return 分支也
emitActionAudit({ success: false, errorMessage: ... })，editUser也存在同样的缺口。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/users.ts` around lines 1152 - 1217, The permission-check and Zod validation early-return paths in addUser (the block around getSession() role check and the CreateUserSchema.safeParse handling) do not emit audit events, so suspicious attempts are not traceable; add calls to emitActionAudit(...) in those branches (and mirror the same fix in editUser) before returning, passing success: false, a descriptive errorMessage (e.g., "permission denied" or the formatted Zod error), and relevant context (actor/session info and attempted user data/id) so the audit record matches the existing catch-path audit format used in addUser.src/actions/model-prices.ts (1)
600-608:⚠️ Potential issue | 🟡 Minor权限拒绝的早退路径未落审计，可能遗漏未授权操作记录
upsertSingleModelPrice与deleteSingleModelPrice在非管理员会话时直接return { ok: false, error: "无权限执行此操作" }，此时并未调用emitActionAudit。对照本 PR 审计目标（"Records … backend mutations … and failure cases"），未授权的敏感写操作通常也应留痕，便于排查越权尝试。如果项目层面约定由
action adapter或上层中间件统一记录鉴权失败，请忽略；否则建议在session.user.role !== "admin"分支内补发一条success: false, errorMessage: "unauthorized"的审计。同类逻辑也适用于本文件其余早退点（价格非法、模型名为空等），可按策略决定是否审计"校验失败"。Also applies to: 725-731
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/model-prices.ts` around lines 600 - 608, The admin-role early-exit in upsertSingleModelPrice (and likewise in deleteSingleModelPrice) returns without auditing; modify the branch that checks session/user.role !== "admin" to call emitActionAudit with success: false and an errorMessage like "unauthorized" (include any contextual identifiers from the input), then return the error result; similarly, add audit calls for other early-return validation failures in this file (e.g., invalid price, empty model name) so emitActionAudit is invoked on those failure paths as well.
🧹 Nitpick comments (18)
messages/en/dashboard.json (1)
754-754: 导航文案大小写风格建议统一。Line 754 建议改为
Audit Logs，与同级导航文案风格保持一致。建议修改
- "auditLogs": "Audit logs", + "auditLogs": "Audit Logs",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@messages/en/dashboard.json` at line 754, The "auditLogs" navigation label in messages/en/dashboard.json uses inconsistent casing ("Audit logs"); update the value for the "auditLogs" key to "Audit Logs" to match sibling navigation entries' Title Case style and ensure consistency across the dashboard localization file.src/app/v1/_lib/proxy/session.ts (1)
86-88:clientIp字段定义可考虑与userAgent对齐。
userAgent使用readonly并在构造器中注入，而clientIp作为可变 public 字段由ProxyAuthenticator在 ensure 阶段写入。考虑到 IP 解析依赖于系统设置（异步加载），目前的可变字段设计是合理的；但为了避免在 ensure 之前被意外读取（例如ProxyMessageService.ensureContext也发生在 ensure 之后，顺序依赖隐式），建议后续通过setClientIp()setter + private 字段的方式显式化写入时机，与其它可变状态（sessionId的setSessionId）保持封装一致性。当前实现功能无误。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/session.ts` around lines 86 - 88, The clientIp field is currently a public mutable property while userAgent is readonly and injected in the constructor; make clientIp private and provide a setClientIp(ip: string) setter (mirroring setSessionId) so writes are explicit and controlled by ProxyAuthenticator during its ensure phase; update any reads to use the private field via its getter or direct internal access, and ensure ProxyMessageService.ensureContext and other callers use the new setter rather than writing clientIp directly.src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx (1)
440-458: 建议给 IP 单元格增加 Tooltip，与 SessionId 列保持一致当前 SessionId 列使用
Tooltip在 hover 时展示完整值并支持点击复制，而新 IP 列仅支持点击打开归属地弹窗，无复制能力且在被truncate截断时无法查看完整 IP（IPv6 较长时较明显）。可考虑增加 Tooltip 或右键/辅助复制入口，UX 上更统一。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/virtualized-logs-table.tsx around lines 440 - 458, Wrap the IP cell rendering (the block using hideIpColumn, log.clientIp, setIpDialogValue and setIpDialogOpen) in the same Tooltip/copy UX used by the SessionId column: show the full IP in a Tooltip on hover (so truncated IPv6 is readable) and provide a small copy-to-clipboard action in the Tooltip (or a click/secondary action) while preserving the existing onClick that opens the IP dialog; ensure the visible button still triggers setIpDialogValue(log.clientIp) and setIpDialogOpen(true) and the Tooltip uses the raw log.clientIp as its content.src/actions/sensitive-words.ts (1)
239-260: 删除操作缺少before快照。
createSensitiveWordAction/updateSensitiveWordAction都记录了after快照,但deleteSensitiveWordAction在成功和失败分支都没有before,审计日志里仅剩一个id,事后很难还原“删了什么”。建议在调用repo.deleteSensitiveWord(id)之前先拿一次现有记录,或让仓储返回被删行,用作before。♻️ 建议改动
- const deleted = await repo.deleteSensitiveWord(id); - - if (!deleted) { + const existing = await repo.getSensitiveWordById(id); // 或在 deleteSensitiveWord 中返回被删行 + const deleted = await repo.deleteSensitiveWord(id); + + if (!deleted) { return { ok: false, error: "敏感词不存在", }; } ... emitActionAudit({ category: "sensitive_word", action: "sensitive_word.delete", targetType: "sensitive_word", targetId: String(id), + targetName: existing?.word ?? null, + before: existing + ? { + id: existing.id, + word: existing.word, + matchType: existing.matchType, + description: existing.description, + isEnabled: existing.isEnabled, + } + : null, success: true, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sensitive-words.ts` around lines 239 - 260, The deleteSensitiveWordAction currently emits audit logs with only the id and no "before" snapshot; before calling repo.deleteSensitiveWord(id) fetch the existing record (e.g. via an existing repo method or by changing repo.deleteSensitiveWord to return the deleted row) and include that object as the before field in emitActionAudit for both the success and catch branches so the audit contains the full pre-delete state (refer to createSensitiveWordAction, updateSensitiveWordAction for the expected "after" shape and to emitActionAudit and repo.deleteSensitiveWord for where to add the snapshot).src/app/api/actions/[...route]/route.ts (1)
1493-1511:beforeValue/afterValue使用z.any()削弱了 OpenAPI 文档价值。虽然快照内容在不同
category下结构不同,使用z.any()可以工作,但在 OpenAPI 文档中会渲染为无类型字段。若时间允许,建议后续用z.unknown()或 discriminated union 细化;当前可接受。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/actions/`[...route]/route.ts around lines 1493 - 1511, The auditLogRowSchema uses z.any() for beforeValue and afterValue which renders as an unhelpful type in the generated OpenAPI; replace z.any() with z.unknown() on the beforeValue and afterValue properties in auditLogRowSchema (or, if you want stronger typing later, implement a discriminated union keyed by actionCategory) so the schema preserves a safer/clearer unknown type and improves OpenAPI output while leaving room to refine per-category types later.src/app/api/ip-geo/[ip]/route.ts (2)
24-27: 功能关闭时返回 404 语义欠妥。
ipGeoLookupEnabled = false表示“功能已禁用”,返回404容易让前端/监控误以为是路由错误。建议使用403 Forbidden或503 Service Unavailable(或明确的409/412),并在响应体中带code字段,便于客户端use-ip-geo区分“未登录 / 未启用 / 真实错误”。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/ip-geo/`[ip]/route.ts around lines 24 - 27, The route handler currently returns a 404 when ip geolocation is disabled; update the check after getCachedSystemSettings() to return a 403 (or 503 if you prefer transient service unavailability) instead of 404 and include a machine-readable code field in the JSON body (e.g. { error: "ip geolocation disabled", code: "feature_disabled" }) so the client (use-ip-geo) can distinguish disabled vs not-authenticated vs real errors; locate the return in the ip-geo route (the block using settings.ipGeoLookupEnabled) and replace the Response.json payload and status accordingly.
16-33: 建议添加入口层 IP 格式校验和异常处理，改进代码一致性。
lookupIp()内部已通过isIP()校验且具备负结果缓存机制,但当前路由缺少防御性编程:
decodeURIComponent(ip)在参数非法百分号编码时会抛出异常,导致无法控制的 500 错误- 其他 API 路由均采用 try/catch 模式(如
/api/admin/database/*),建议保持一致可选方案: 在路由入口用 try/catch 包裹主逻辑,同时在调用
lookupIp()前做 IP 格式预检(快速失败返回 400),提升错误可追溯性。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/ip-geo/`[ip]/route.ts around lines 16 - 33, The GET route handler should defensively validate and handle errors: before calling decodeURIComponent and lookupIp in GET, pre-validate the raw params.ip using the same IP check used by lookupIp (e.g., isIP or net.isIP) and return a 400 JSON response for invalid formats; wrap the main handler body (session check, settings check, decodeURIComponent, lookupIp call) in a try/catch to catch decodeURIComponent or other runtime errors and return a controlled JSON error with an appropriate status (500) and log the error; update references to GET, lookupIp, and getCachedSystemSettings to ensure the flow remains unchanged but now uses pre-check + try/catch for consistency with other routes.src/lib/audit/redact.ts (1)
29-45: 建议增加循环引用防护。
walk()没有WeakSet去重，传入含循环引用的对象（例如某些 ORM 关联对象或手工拼接的after快照）会直接栈溢出。审计链路位于写路径，抛错会污染业务错误处理。对 plain-object 传入一个seen: WeakSet即可避免此问题，开销可忽略。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/audit/redact.ts` around lines 29 - 45, walk() can stack-overflow on objects with circular references because it doesn't track seen objects; add a seen: WeakSet<object> parameter to walk (defaulting to a new WeakSet at top-level call), check isPlainObject(value) objects against seen and return value or REDACTED when already seen, and add objects to seen before recursing so Array and object branches call walk(item, keys, seen) / walk(v, keys, seen); update any call sites to pass the new seen param (or rely on the default) and keep existing behavior for REDACTED and isPlainObject checks.src/lib/config/env.schema.ts (1)
143-143: 建议对IP_GEO_API_URL做 URL 校验。其他 URL 类字段（如
DSN、LANGFUSE_BASE_URL）本文件虽也未使用.url()，但此处用户通常会自定义自托管地址，启动期就拦截非法值更友好。改成z.string().url().default(...)即可在getEnvConfig()首次解析时把错误配置暴露出来。建议修改
- IP_GEO_API_URL: z.string().default("https://ip.api.claude-code-hub.app"), + IP_GEO_API_URL: z.string().url().default("https://ip.api.claude-code-hub.app"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/config/env.schema.ts` at line 143, The IP_GEO_API_URL environment schema uses z.string() without URL validation; update the schema entry for IP_GEO_API_URL to use z.string().url().default("https://ip.api.claude-code-hub.app") so invalid URLs are rejected during getEnvConfig() parsing; locate the IP_GEO_API_URL key in src/lib/config/env.schema.ts and apply the same .url() pattern if you want consistent validation for other URL-like keys referenced by getEnvConfig().src/actions/keys.ts (1)
602-644:editKey的 after 只是调用补丁，建议记录更新后的实际状态
before用完整的key实体（L608–L625），after却是validatedData/data.isEnabled等补丁字段的混合（L626–L641）。由于存在await updateKey(...)之后可能因 DB 默认值、triggers 等造成补丁与实际存库值不一致，建议在updateKey之后再findKeyById(keyId)取一次真实快照作为after，让审计真正可审计（与users.ts的相同问题类似）。若出于性能顾虑不想多一次查询，至少在注释中注明 after 是“调用补丁”而非真实状态。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/keys.ts` around lines 602 - 644, The audit "after" currently logs the incoming patch values instead of the persisted record; in editKey, after calling updateKey(...) fetch the real saved record (e.g. const updatedKey = await findKeyById(keyId)) and populate the emitActionAudit.after fields from that updatedKey (using the same field names as in before) so the audit reflects the actual DB state; if you intentionally want to avoid an extra query, add a clear comment next to emitActionAudit that "after contains the submitted patch, not the persisted record" and keep the existing behavior.src/lib/ip/private-ip.ts (1)
44-45: IPv6 链路本地检测存在冗余判断
lower.startsWith("fe80:")和lower === "fe80::"都会被后面的/^fe[89ab]/.test(lower)涵盖（fe80的首字符满足fe[89ab]且已被作为前缀正则匹配）。建议合并为单一判断以减少阅读负担，或把::与::1的 loopback 处理与 link-local 的分支显式分离说明。功能上没有缺陷。♻️ 可合并的检测分支
- if (lower === "::1" || lower === "::") return true; - if (lower.startsWith("fe80:") || lower === "fe80::" || /^fe[89ab]/.test(lower)) return true; - if (/^f[cd]/.test(lower)) return true; // fc00::/7 (ULA) + // loopback: ::1 or the unspecified address :: + if (lower === "::1" || lower === "::") return true; + // link-local fe80::/10 -> first hextet in [fe80, febf] + if (/^fe[89ab][0-9a-f]*:/.test(lower)) return true; + // Unique Local Address fc00::/7 + if (/^f[cd]/.test(lower)) return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ip/private-ip.ts` around lines 44 - 45, The link-local IPv6 checks are redundant—remove the explicit lower.startsWith("fe80:") and lower === "fe80::" branches and rely on the existing /^fe[89ab]/.test(lower) (or replace with a single clearer check) while keeping the loopback checks (lower === "::1" || lower === "::") separate; update the condition in the function in src/lib/ip/private-ip.ts that inspects the lower variable so it only checks loopback first and then uses the single regex-based link-local test (/^fe[89ab]/.test(lower)) to determine link-local addresses.src/lib/ip/extract-client-ip.test.ts (1)
1-172: LGTM测试覆盖了默认优先级、pick 变体（包括越界/负索引）、解析健壮性（空白、尾逗号、端口剥离、方括号 IPv6、纯 IPv6、非法值跳过、空 header 跳过）以及
HeadersLike宽容输入，整体覆盖面良好。如要再加固一点，可以考虑补上以下几个用例（非阻塞）：
- IPv6 带 scope id（
fe80::1%eth0）的处理是否符合预期。- 形如
"x-forwarded-for": "1.1.1.1, invalid, 4.4.4.4"在rightmost+ 非法值时的遍历回退语义（是继续向左找还是放弃整条规则）。- 超长 XFF（异常流量）下不会触发回溯性能退化。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ip/extract-client-ip.test.ts` around lines 1 - 172, Add three non-blocking tests to strengthen extractClientIp coverage: (1) a test that passes an IPv6 address with a scope id like "fe80::1%eth0" to extractClientIp (use headers config [{ name: "x-real-ip" }]) and assert expected normalization/acceptance; (2) a test for XFF containing an invalid middle token such as "1.1.1.1, invalid, 4.4.4.4" with pick "rightmost" to assert whether extractClientIp returns "4.4.4.4" (i.e., skips invalid entries and continues scanning) or the chosen behavior your implementation should enforce; and (3) a test that feeds a very long x-forwarded-for string (many repeated entries) to ensure extractClientIp finishes quickly (e.g., assert result and keep input large) to guard against pathological quadratic behavior; add these alongside the existing describe blocks in extract-client-ip.test.ts referencing extractClientIp and DEFAULT_IP_EXTRACTION_CONFIG as appropriate.src/actions/model-prices.ts (1)
676-676:findLatestPriceByModel失败会让整个操作失败，审计 before 取不到应降级而非阻塞第 676 / 739 行在写入前读取 before 快照：若此查询因瞬时错误抛出，将进入外层
catch，使整个写/删操作被中断（本来并不需要 before 也能完成主业务）。考虑将 before 读取包在独立 try/catch 中，失败时让beforePrice = null并继续执行主业务：审计失真比用户操作失败代价更低。建议修复（以 upsertSingleModelPrice 为例）
- // 捕获 before 快照 - const beforePrice = await findLatestPriceByModel(input.modelName.trim()); + // 捕获 before 快照（失败不阻塞主流程） + let beforePrice: Awaited<ReturnType<typeof findLatestPriceByModel>> | null = null; + try { + beforePrice = await findLatestPriceByModel(input.modelName.trim()); + } catch (err) { + logger.warn("[ModelPrices] capture before-snapshot failed", { + error: err instanceof Error ? err.message : String(err), + }); + }Also applies to: 739-739
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/model-prices.ts` at line 676, The call to findLatestPriceByModel in upsertSingleModelPrice should not block the main write/delete operation if it throws; wrap the before snapshot read in its own try/catch, assign beforePrice = null on any error, log the failure for audit debugging, and continue with the rest of upsertSingleModelPrice so the primary write proceeds even when the before read fails.src/app/[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsx (1)
61-68:try/catch包裹t(key)的兜底模式值得斟酌
next-intl在未配置onError的默认情况下，缺失 key 会抛错但也可能按命名空间降级为返回 key 字符串，不同版本行为不完全一致。此处用try/catch回退到log.actionCategory是安全的兜底，但如果 5 个语言包里都完整定义了所有categories.*，这一兜底主要服务于"后端新增 category 未来未同步 i18n"的场景——建议在注释里标注意图，避免后续同事误删。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsx around lines 61 - 68, The inline try/catch around calling t(key) for computing categoryLabel is intentionally serving as a safe fallback for missing i18n keys (so backend-added categories display log.actionCategory instead of breaking); preserve the current try/catch and add a clear comment above the IIFE referencing symbols categoryLabel, key, t and log.actionCategory that explains this is a deliberate fallback for backend-introduced categories (and not accidental dead code to be removed), and mention that next-intl behavior varies by version so the guard is required.drizzle/0089_curly_grey_gargoyle.sql (1)
122-125:EXCEPTION WHEN OTHERS会静默吞掉所有错误，包含真实数据完整性问题该兜底捕获范围过大：除了你想规避的"同步失败不影响主表"场景外，它还会吞掉：
usage_ledger的 NOT NULL / CHECK / 外键约束违反（数据不一致）- 列/类型不兼容（schema drift 引发的 bug，部署后只会以 NOTICE 形式隐身）
- 意外的 JSON 结构导致的
->/ 类型转换异常
RAISE WARNING在多数生产 Postgres 日志配置下会出现，但不会被告警/监控系统显式捕获，问题容易"长期带病运行"。建议：
- 用更窄的
EXCEPTION WHEN invalid_parameter_value OR data_exception THEN ...，或- 至少把告警通过
RAISE WARNING USING ERRCODE = SQLSTATE打入，并确保告警平台订阅该 SQLSTATE，- 在
usage_ledger侧建立一张usage_ledger_failures表，捕获SQLSTATE + SQLERRM + NEW.id以便回放/补数。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/0089_curly_grey_gargoyle.sql` around lines 122 - 125, 当前的 EXCEPTION WHEN OTHERS 在函数 fn_upsert_usage_ledger 中过于宽泛，会吞掉数据完整性和 schema 错误；请改为只捕获明确的 SQLSTATE（例如 invalid_parameter_value 或 data_exception）或在捕获时把 SQLSTATE 和 SQLERRM 记录到一个新的表 usage_ledger_failures（保存 NEW.id、SQLSTATE、SQLERRM 和上下文），并确保 RAISE/日志使用 USING ERRCODE = SQLSTATE 将错误码传递出去以便告警系统能订阅到；在需要继续忽略的同步失败路径只保留这些窄化的异常分支，其他异常让事务抛出以避免静默丢失问题。src/lib/audit/with-audit.test.ts (2)
129-146:vi.doMock+resetModules的顺序依赖较脆弱该用例依赖两点：1) 顶层
vi.mock("@/lib/auth", ...)的提升生效；2) 之后vi.doMock覆盖并resetModules()让新 import 拿到 null session。如果后续新增用例在此 describe 之前调用await import("./with-audit")，模块缓存会复用已 mock 为有 session 的版本，导致本用例的"无 session"断言假通过。建议：
- 在
afterEach（或此 describe 的afterAll）显式vi.doUnmock("@/lib/auth")+vi.resetModules()，保证隔离。- 或改为在
getScopedAuthSessionmock 上使用mockReturnValueOnce(null)，避免整个模块重新加载。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/audit/with-audit.test.ts` around lines 129 - 146, The test relies on fragile ordering of vi.doMock and vi.resetModules so imports of withAudit may get a cached auth mock; update the test to ensure isolation by either reverting the mock after the test or making the mock per-call: call vi.doUnmock("@/lib/auth") and vi.resetModules() in an afterEach/afterAll for this describe to clear the module cache, or instead replace the vi.doMock usage with a stable mock on the getScopedAuthSession function using mockReturnValueOnce(null) so the getScopedAuthSession mock returns null for this case without needing to reload withAudit; reference vi.doMock, vi.resetModules, vi.doUnmock, getScopedAuthSession, withAudit and emittedEntries when applying the fix.
115-115: 断言语义略模糊
expect(entry.afterValue ?? null).toBeNull()会同时接受afterValue === null与afterValue === undefined。若withAudit的契约是失败时必须显式落afterValue: null（与emit.ts里args.after !== undefined ? … : null保持一致），直接断言toBeNull()更严格；若契约是可选字段，则显式toBeUndefined()。当前写法可能掩盖契约漂移。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/audit/with-audit.test.ts` at line 115, The assertion uses a nullish coalescing that accepts undefined, which can hide contract drift; change the test in with-audit.test.ts to assert the exact expected value per the withAudit/emit contract — since emit.ts sets args.after !== undefined ? … : null, replace expect(entry.afterValue ?? null).toBeNull() with a strict expectation expect(entry.afterValue).toBeNull() so failures must explicitly set afterValue to null (reference symbols: withAudit, entry.afterValue, emit.ts args.after).src/lib/ip-geo/client.test.ts (1)
217-228: 超时用例依赖真实墙钟时间，可能拖慢测试套件该用例未对定时器做 mock，
lookupIp内部的setTimeout(…, IP_GEO_TIMEOUT_MS)（配置为 1500ms）会真实触发 abort，导致本用例每次至少耗时约 1.5 秒。可考虑使用vi.useFakeTimers()并通过vi.advanceTimersByTimeAsync(1500)精确驱动中止路径，或在此describe内临时下调IP_GEO_TIMEOUT_MSmock。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ip-geo/client.test.ts` around lines 217 - 228, The test for aborting slow upstream relies on real time and waits for IP_GEO_TIMEOUT_MS (1500ms); update the test to use fake timers and drive the abort deterministically or mock the timeout value: before calling lookupIp in this test use vi.useFakeTimers(), then trigger the fetch abort path by advancing timers with vi.advanceTimersByTimeAsync(IP_GEO_TIMEOUT_MS) (or temporarily stub IP_GEO_TIMEOUT_MS to a small value) so the setTimeout inside lookupIp triggers synchronously and the test completes quickly; ensure to restore timers after the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drizzle/0089_curly_grey_gargoyle.sql`:
- Around line 91-119: The ON CONFLICT (request_id) DO UPDATE SET clause is
incorrectly overwriting the original ledger creation timestamp; remove the
"created_at = EXCLUDED.created_at" entry from the DO UPDATE SET list so that
created_at remains the original insert value. Locate the UPSERT block starting
with "ON CONFLICT (request_id) DO UPDATE SET" (referencing request_id,
EXCLUDED.created_at, and created_at) and delete that assignment; keep all other
EXCLUDED.* updates unchanged so retries/updates don't modify
usage_ledger.created_at.
In `@src/actions/keys.ts`:
- Around line 356-364: The audit is currently writing raw error.message into
emitActionAudit (seen at emitActionAudit({... errorMessage: message ...})),
which can leak DB/PG internals; change the flow to map thrown errors in the key
creation handlers to a stable error code or enum (align with
ActionResult.errorCode) and pass that code as errorMessage to
emitActionAudit/createAuditLog, while sending the original error.message to the
application logger (and/or to a separate non-admin-only audit field).
Alternatively implement a sanitization step inside
emitActionAudit/createAuditLog that truncates and redacts free-text error
messages (reusing the redactExtraKeys redactor) before persisting; update all
occurrences mentioned (around lines with emitActionAudit in key.create and the
other sites called out) to use the mapping or sanitization helper and keep
original message only in logs.
In `@src/actions/users.ts`:
- Around line 1615-1624: 在调用 updateUser/执行更新前用 findUserById(userId) 读取并保存
"before" 快照，然后在 emitActionAudit 调用中包含 before 字段；不要把 validatedData 当作完整的 after
快照——在更新成功后用 updateUser 的返回值或再次查询用户来填充完整的 "after" 快照；同时在失败的错误分支也把之前读取到的 before
一并记录并在 emitActionAudit 的 error 分支中输出。确保你修改的是包含 revalidatePath(...) 和
emitActionAudit(...) 的 editUser/updateUser 逻辑并在所有分支（success/error）一致地包含 before
和完整 after。
In `@src/app/`[locale]/dashboard/_components/ip-details-dialog.tsx:
- Around line 155-158: The IP details dialog currently renders
data.data.threat.risk_level directly; update rendering to translate the enum
before display by mapping the upstream values to i18n keys like
ipDetails.riskLevels.low|medium|high|critical using the t() function (e.g.
t(`ipDetails.riskLevels.${riskKey}`)), ensure you normalize the enum
(toLowerCase()) and fall back to the raw risk string when the translation key is
missing; change the JSX around data.data.threat.risk_level (and keep the
existing t("fields.riskLevel") usage) so all user-facing risk labels go through
i18n.
In
`@src/app/`[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsx:
- Line 59: Several user-facing strings are hardcoded and must use next-intl
translations: replace the fallback "Admin Token" (operatorName), the inline
labels/values shown as `id: {log.targetId}`, `key: {log.operatorKeyName}`, `user
id: {log.operatorUserId}`, and the Row label "User-Agent" with translation
lookups; add keys such as detail.adminToken, detail.targetIdLabel,
detail.keyLabel, detail.userIdLabel and columns.userAgent to
messages/*/auditLogs.json for each locale, then update the component
(references: operatorName, the JSX that renders `id: {log.targetId}`, `key:
{log.operatorKeyName}`, `user id: {log.operatorUserId}`, and the Row with label
"User-Agent") to use the intl/messages helper (e.g., t('...')) instead of
hardcoded literals so all five locales are covered.
In `@src/app/`[locale]/dashboard/audit-logs/_components/audit-logs-view.tsx:
- Line 186: The component audit-logs-view.tsx currently hardcodes user-facing
strings ("Error" at the error fallback and "Admin Token" as a default operator
name); add keys errors.generic and operator.adminToken to
messages/*/auditLogs.json for all five locales (zh-CN, zh-TW, en, ja, ru), then
replace the hardcoded literals with the i18n calls (e.g., use the file's
translation hook/namespace like t('errors.generic') for the error fallback and
t('operator.adminToken') for the default operator name) and ensure the component
imports/uses the same auditLogs translation namespace.
In `@src/app/`[locale]/dashboard/logs/_components/usage-logs-table.tsx:
- Line 36: The new import uses a relative path for IpDetailsDialog which
violates the repo guideline to use the "@/..." alias; update the import of
IpDetailsDialog (symbol: IpDetailsDialog) to use the "@/..." path alias that
maps to ./src (e.g. replace "../../_components/ip-details-dialog" with the
corresponding "@/..." alias import), ensuring the rest of the file (usage of
IpDetailsDialog) remains unchanged.
In `@src/app/`[locale]/settings/config/_components/system-settings-form.tsx:
- Around line 192-209: The code silently converts invalid or non-matching JSON
from ipExtractionConfigText into null (ipExtractionConfigToSave) and allows
saveSystemSettings to succeed, so change the submit logic to validate and block
saving instead: parse and run a runtime schema (e.g., Zod) against the parsed
object (use IpExtractionConfig shape with headers: IpHeaderRule[] and per-item
checks like non-empty name and legal pick union) and if parsing/validation
fails, prevent calling saveSystemSettings and surface a localized validation
error to the user (use the ipLogging namespace keys you must add to
messages/*/settings/config.json), otherwise set ipExtractionConfigToSave to the
validated value and proceed to save.
In `@src/app/api/auth/login/route.ts`:
- Around line 107-114: The resolveClientIp function contains a dead branch
checking request.ip (Next.js 16 removed NextRequest.ip); remove the platformIp
extraction and return getClientIp(request.headers) ?? "unknown" instead, or if
running on Vercel detect that environment and call ipAddress(request) from
`@vercel/functions` and fall back to getClientIp(request.headers) ?? "unknown";
update imports and remove the cast to { ip?: string } and ensure resolveClientIp
only references getClientIp and/or ipAddress.
In `@src/app/api/ip-geo/`[ip]/route.ts:
- Line 29: The issue is double-decoding the route param causing URIError for
percent sequences (e.g. IPv6 zone ids); in the handler where you call
lookupIp(decodeURIComponent(ip)), stop blindly calling decodeURIComponent and
instead pass the already-decoded ip directly to lookupIp (lookupIp(ip)); if you
must accept percent-encoded input, perform a single safe decode: wrap
decodeURIComponent(ip) in a try/catch and on decode failure fall back to the
original ip and return a 400 error for clearly malformed percent sequences;
update the code around the lookupIp call and the route handler (the variable ip
and the lookupIp invocation) to use the safe approach so invalid percent
sequences do not crash the server.
In `@src/lib/audit/redact.ts`:
- Around line 1-27: The DEFAULT_SENSITIVE_KEYS set contains a camelCase
"webhookSecret" so comparisons using k.toLowerCase() in walk miss it; fix by
normalizing the sensitive-key set to lowercase (e.g., transform
DEFAULT_SENSITIVE_KEYS entries with .map/.forEach to .toLowerCase() or rebuild
the set with lowercase strings) or remove the camelCase entry and add the
lowercase "webhooksecret", and ensure redactSensitive's keys construction keeps
everything lowercased (the code that builds keys in redactSensitive should union
DEFAULT_SENSITIVE_KEYS and extraKeys after lowercasing both); update
DEFAULT_SENSITIVE_KEYS or redactSensitive accordingly so walk sees matching
lowercase keys.
In `@src/lib/audit/with-audit.ts`:
- Around line 49-77: The current try block runs options.extractAfter(result) and
redactSensitive(...) inline so any exception there will bubble up and mark the
mutation as failed; change the flow so the core operation (await fn()) always
returns its result and is not blocked by auditing: call await fn() and
immediately return the result to the caller, but separately (fire-and-forget)
perform extractAfter and redactSensitive inside their own try/catch and then
call emitAudit inside another try/catch (or the same audit-protection block) so
any errors from options.extractAfter, redactSensitive, or emitAudit are caught
and do not alter the returned result or the success flag; reference
resolveTarget(options.target, result), options.extractAfter,
redactSensitive(after, options.redactExtraKeys), emitAudit({... success: true
...}) and keep the existing catch branch for fn() failures that uses
resolveTarget(options.target, undefined as T) and emits a failure audit.
In `@src/lib/ip-geo/client.ts`:
- Around line 81-85: The current check only ensures "ip" exists on the parsed
IpGeoLookupResult and can let incomplete objects be cached under
IP_GEO_CACHE_TTL_SECONDS; update the validation in the code that handles (await
response.json()) -> data to perform stricter shape checks (e.g., ensure
data.location is an object and data.location.country (and any other UI-required
keys) are present and non-null) using a lightweight guard or zod schema; on
validation failure return an error/negative result so callers use a short
negative cache TTL (introduce or use an IP_GEO_NEGATIVE_CACHE_TTL_SECONDS
constant) instead of caching the malformed object for IP_GEO_CACHE_TTL_SECONDS.
In `@src/lib/validation/schemas.ts`:
- Around line 1008-1020: The ipExtractionConfig schema currently permits any
value for the pick field via z.any().optional(), letting invalid values be
persisted; change the pick validator inside the ipExtractionConfig → headers
array item to a discriminated union matching XffPick: accept the literal strings
"leftmost" and "rightmost" and an object { kind: "index"; index: number } (with
proper number validation), keep pick optional and preserve the surrounding union
with z.null(), and ensure the schema name/ipExtractionConfig and header item
object are updated so extract-client-ip.ts will receive only those validated
shapes.
In `@src/repository/audit-log.ts`:
- Around line 147-157: countAuditLogs is missing several filter fields that
listAuditLogs supports, causing mismatched totals; extract the shared filtering
logic into a helper (e.g., buildFilterConditions or buildConditions) that
accepts AuditLogFilter and returns the array of conditions or the final where
clause, and update both countAuditLogs and listAuditLogs to call this helper;
ensure the helper adds conditions for category, actionType, operatorUserId,
operatorIp, targetType, targetId, success, from, and to (using eq/gte/lte as
appropriate) so both functions use identical filtering logic.
---
Outside diff comments:
In `@src/actions/model-prices.ts`:
- Around line 600-608: The admin-role early-exit in upsertSingleModelPrice (and
likewise in deleteSingleModelPrice) returns without auditing; modify the branch
that checks session/user.role !== "admin" to call emitActionAudit with success:
false and an errorMessage like "unauthorized" (include any contextual
identifiers from the input), then return the error result; similarly, add audit
calls for other early-return validation failures in this file (e.g., invalid
price, empty model name) so emitActionAudit is invoked on those failure paths as
well.
In `@src/actions/notifications.ts`:
- Around line 42-74: The audit currently marks the whole operation as failed if
scheduleNotifications() throws after updateNotificationSettings(payload)
succeeded; move the emitActionAudit(...) call that logs success (with after:
updated) to occur immediately after updateNotificationSettings returns (before
calling scheduleNotifications), and then call scheduleNotifications() in a
try/catch that logs a separate audit/event or logger.warn/error if it fails;
keep the existing catch to handle failures from updateNotificationSettings and
only emit a failure audit there. Ensure you reference
updateNotificationSettings, scheduleNotifications, and emitActionAudit when
adjusting ordering and adding the separate error handling for scheduling.
In `@src/actions/users.ts`:
- Around line 1152-1217: The permission-check and Zod validation early-return
paths in addUser (the block around getSession() role check and the
CreateUserSchema.safeParse handling) do not emit audit events, so suspicious
attempts are not traceable; add calls to emitActionAudit(...) in those branches
(and mirror the same fix in editUser) before returning, passing success: false,
a descriptive errorMessage (e.g., "permission denied" or the formatted Zod
error), and relevant context (actor/session info and attempted user data/id) so
the audit record matches the existing catch-path audit format used in addUser.
In `@src/repository/message.ts`:
- Around line 320-388: The ledger fallback is not selecting or mapping
usageLedger.clientIp into the returned MessageRequest, so client IPs are always
empty in ledger-only mode; update the DB select in findMessageRequestById to
include usageLedger.clientIp and pass that value into toMessageRequest (e.g.,
clientIp: ledgerRow.clientIp), and make the same change in the other two
fallback queries findMessageRequestBySessionId and findUsageLogs so each selects
usageLedger.clientIp and maps it into the toMessageRequest call.
---
Nitpick comments:
In `@drizzle/0089_curly_grey_gargoyle.sql`:
- Around line 122-125: 当前的 EXCEPTION WHEN OTHERS 在函数 fn_upsert_usage_ledger
中过于宽泛，会吞掉数据完整性和 schema 错误；请改为只捕获明确的 SQLSTATE（例如 invalid_parameter_value 或
data_exception）或在捕获时把 SQLSTATE 和 SQLERRM 记录到一个新的表 usage_ledger_failures（保存
NEW.id、SQLSTATE、SQLERRM 和上下文），并确保 RAISE/日志使用 USING ERRCODE = SQLSTATE
将错误码传递出去以便告警系统能订阅到；在需要继续忽略的同步失败路径只保留这些窄化的异常分支，其他异常让事务抛出以避免静默丢失问题。
In `@messages/en/dashboard.json`:
- Line 754: The "auditLogs" navigation label in messages/en/dashboard.json uses
inconsistent casing ("Audit logs"); update the value for the "auditLogs" key to
"Audit Logs" to match sibling navigation entries' Title Case style and ensure
consistency across the dashboard localization file.
In `@src/actions/keys.ts`:
- Around line 602-644: The audit "after" currently logs the incoming patch
values instead of the persisted record; in editKey, after calling updateKey(...)
fetch the real saved record (e.g. const updatedKey = await findKeyById(keyId))
and populate the emitActionAudit.after fields from that updatedKey (using the
same field names as in before) so the audit reflects the actual DB state; if you
intentionally want to avoid an extra query, add a clear comment next to
emitActionAudit that "after contains the submitted patch, not the persisted
record" and keep the existing behavior.
In `@src/actions/model-prices.ts`:
- Line 676: The call to findLatestPriceByModel in upsertSingleModelPrice should
not block the main write/delete operation if it throws; wrap the before snapshot
read in its own try/catch, assign beforePrice = null on any error, log the
failure for audit debugging, and continue with the rest of
upsertSingleModelPrice so the primary write proceeds even when the before read
fails.
In `@src/actions/sensitive-words.ts`:
- Around line 239-260: The deleteSensitiveWordAction currently emits audit logs
with only the id and no "before" snapshot; before calling
repo.deleteSensitiveWord(id) fetch the existing record (e.g. via an existing
repo method or by changing repo.deleteSensitiveWord to return the deleted row)
and include that object as the before field in emitActionAudit for both the
success and catch branches so the audit contains the full pre-delete state
(refer to createSensitiveWordAction, updateSensitiveWordAction for the expected
"after" shape and to emitActionAudit and repo.deleteSensitiveWord for where to
add the snapshot).
In
`@src/app/`[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsx:
- Around line 61-68: The inline try/catch around calling t(key) for computing
categoryLabel is intentionally serving as a safe fallback for missing i18n keys
(so backend-added categories display log.actionCategory instead of breaking);
preserve the current try/catch and add a clear comment above the IIFE
referencing symbols categoryLabel, key, t and log.actionCategory that explains
this is a deliberate fallback for backend-introduced categories (and not
accidental dead code to be removed), and mention that next-intl behavior varies
by version so the guard is required.
In `@src/app/`[locale]/dashboard/logs/_components/virtualized-logs-table.tsx:
- Around line 440-458: Wrap the IP cell rendering (the block using hideIpColumn,
log.clientIp, setIpDialogValue and setIpDialogOpen) in the same Tooltip/copy UX
used by the SessionId column: show the full IP in a Tooltip on hover (so
truncated IPv6 is readable) and provide a small copy-to-clipboard action in the
Tooltip (or a click/secondary action) while preserving the existing onClick that
opens the IP dialog; ensure the visible button still triggers
setIpDialogValue(log.clientIp) and setIpDialogOpen(true) and the Tooltip uses
the raw log.clientIp as its content.
In `@src/app/api/actions/`[...route]/route.ts:
- Around line 1493-1511: The auditLogRowSchema uses z.any() for beforeValue and
afterValue which renders as an unhelpful type in the generated OpenAPI; replace
z.any() with z.unknown() on the beforeValue and afterValue properties in
auditLogRowSchema (or, if you want stronger typing later, implement a
discriminated union keyed by actionCategory) so the schema preserves a
safer/clearer unknown type and improves OpenAPI output while leaving room to
refine per-category types later.
In `@src/app/api/ip-geo/`[ip]/route.ts:
- Around line 24-27: The route handler currently returns a 404 when ip
geolocation is disabled; update the check after getCachedSystemSettings() to
return a 403 (or 503 if you prefer transient service unavailability) instead of
404 and include a machine-readable code field in the JSON body (e.g. { error:
"ip geolocation disabled", code: "feature_disabled" }) so the client
(use-ip-geo) can distinguish disabled vs not-authenticated vs real errors;
locate the return in the ip-geo route (the block using
settings.ipGeoLookupEnabled) and replace the Response.json payload and status
accordingly.
- Around line 16-33: The GET route handler should defensively validate and
handle errors: before calling decodeURIComponent and lookupIp in GET,
pre-validate the raw params.ip using the same IP check used by lookupIp (e.g.,
isIP or net.isIP) and return a 400 JSON response for invalid formats; wrap the
main handler body (session check, settings check, decodeURIComponent, lookupIp
call) in a try/catch to catch decodeURIComponent or other runtime errors and
return a controlled JSON error with an appropriate status (500) and log the
error; update references to GET, lookupIp, and getCachedSystemSettings to ensure
the flow remains unchanged but now uses pre-check + try/catch for consistency
with other routes.
In `@src/app/v1/_lib/proxy/session.ts`:
- Around line 86-88: The clientIp field is currently a public mutable property
while userAgent is readonly and injected in the constructor; make clientIp
private and provide a setClientIp(ip: string) setter (mirroring setSessionId) so
writes are explicit and controlled by ProxyAuthenticator during its ensure
phase; update any reads to use the private field via its getter or direct
internal access, and ensure ProxyMessageService.ensureContext and other callers
use the new setter rather than writing clientIp directly.
In `@src/lib/audit/redact.ts`:
- Around line 29-45: walk() can stack-overflow on objects with circular
references because it doesn't track seen objects; add a seen: WeakSet<object>
parameter to walk (defaulting to a new WeakSet at top-level call), check
isPlainObject(value) objects against seen and return value or REDACTED when
already seen, and add objects to seen before recursing so Array and object
branches call walk(item, keys, seen) / walk(v, keys, seen); update any call
sites to pass the new seen param (or rely on the default) and keep existing
behavior for REDACTED and isPlainObject checks.
In `@src/lib/audit/with-audit.test.ts`:
- Around line 129-146: The test relies on fragile ordering of vi.doMock and
vi.resetModules so imports of withAudit may get a cached auth mock; update the
test to ensure isolation by either reverting the mock after the test or making
the mock per-call: call vi.doUnmock("@/lib/auth") and vi.resetModules() in an
afterEach/afterAll for this describe to clear the module cache, or instead
replace the vi.doMock usage with a stable mock on the getScopedAuthSession
function using mockReturnValueOnce(null) so the getScopedAuthSession mock
returns null for this case without needing to reload withAudit; reference
vi.doMock, vi.resetModules, vi.doUnmock, getScopedAuthSession, withAudit and
emittedEntries when applying the fix.
- Line 115: The assertion uses a nullish coalescing that accepts undefined,
which can hide contract drift; change the test in with-audit.test.ts to assert
the exact expected value per the withAudit/emit contract — since emit.ts sets
args.after !== undefined ? … : null, replace expect(entry.afterValue ??
null).toBeNull() with a strict expectation expect(entry.afterValue).toBeNull()
so failures must explicitly set afterValue to null (reference symbols:
withAudit, entry.afterValue, emit.ts args.after).
In `@src/lib/config/env.schema.ts`:
- Line 143: The IP_GEO_API_URL environment schema uses z.string() without URL
validation; update the schema entry for IP_GEO_API_URL to use
z.string().url().default("https://ip.api.claude-code-hub.app") so invalid URLs
are rejected during getEnvConfig() parsing; locate the IP_GEO_API_URL key in
src/lib/config/env.schema.ts and apply the same .url() pattern if you want
consistent validation for other URL-like keys referenced by getEnvConfig().
In `@src/lib/ip-geo/client.test.ts`:
- Around line 217-228: The test for aborting slow upstream relies on real time
and waits for IP_GEO_TIMEOUT_MS (1500ms); update the test to use fake timers and
drive the abort deterministically or mock the timeout value: before calling
lookupIp in this test use vi.useFakeTimers(), then trigger the fetch abort path
by advancing timers with vi.advanceTimersByTimeAsync(IP_GEO_TIMEOUT_MS) (or
temporarily stub IP_GEO_TIMEOUT_MS to a small value) so the setTimeout inside
lookupIp triggers synchronously and the test completes quickly; ensure to
restore timers after the test.
In `@src/lib/ip/extract-client-ip.test.ts`:
- Around line 1-172: Add three non-blocking tests to strengthen extractClientIp
coverage: (1) a test that passes an IPv6 address with a scope id like
"fe80::1%eth0" to extractClientIp (use headers config [{ name: "x-real-ip" }])
and assert expected normalization/acceptance; (2) a test for XFF containing an
invalid middle token such as "1.1.1.1, invalid, 4.4.4.4" with pick "rightmost"
to assert whether extractClientIp returns "4.4.4.4" (i.e., skips invalid entries
and continues scanning) or the chosen behavior your implementation should
enforce; and (3) a test that feeds a very long x-forwarded-for string (many
repeated entries) to ensure extractClientIp finishes quickly (e.g., assert
result and keep input large) to guard against pathological quadratic behavior;
add these alongside the existing describe blocks in extract-client-ip.test.ts
referencing extractClientIp and DEFAULT_IP_EXTRACTION_CONFIG as appropriate.
In `@src/lib/ip/private-ip.ts`:
- Around line 44-45: The link-local IPv6 checks are redundant—remove the
explicit lower.startsWith("fe80:") and lower === "fe80::" branches and rely on
the existing /^fe[89ab]/.test(lower) (or replace with a single clearer check)
while keeping the loopback checks (lower === "::1" || lower === "::") separate;
update the condition in the function in src/lib/ip/private-ip.ts that inspects
the lower variable so it only checks loopback first and then uses the single
regex-based link-local test (/^fe[89ab]/.test(lower)) to determine link-local
addresses.
🪄 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: 6afc3379-2f9c-4f65-8ec5-04bd83dc083d
📒 Files selected for processing (89)
drizzle/0089_curly_grey_gargoyle.sqldrizzle/meta/0089_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/auditLogs.jsonmessages/en/dashboard.jsonmessages/en/index.tsmessages/en/ipDetails.jsonmessages/en/settings/config.jsonmessages/ja/auditLogs.jsonmessages/ja/dashboard.jsonmessages/ja/index.tsmessages/ja/ipDetails.jsonmessages/ja/settings/config.jsonmessages/ru/auditLogs.jsonmessages/ru/dashboard.jsonmessages/ru/index.tsmessages/ru/ipDetails.jsonmessages/ru/settings/config.jsonmessages/zh-CN/auditLogs.jsonmessages/zh-CN/dashboard.jsonmessages/zh-CN/index.tsmessages/zh-CN/ipDetails.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/auditLogs.jsonmessages/zh-TW/dashboard.jsonmessages/zh-TW/index.tsmessages/zh-TW/ipDetails.jsonmessages/zh-TW/settings/config.jsonsrc/actions/audit-logs.tssrc/actions/keys.tssrc/actions/model-prices.tssrc/actions/notifications.tssrc/actions/provider-groups.tssrc/actions/providers.tssrc/actions/sensitive-words.tssrc/actions/system-config.tssrc/actions/users.tssrc/app/[locale]/dashboard/_components/dashboard-header.tsxsrc/app/[locale]/dashboard/_components/ip-details-dialog.tsxsrc/app/[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsxsrc/app/[locale]/dashboard/audit-logs/_components/audit-logs-view.tsxsrc/app/[locale]/dashboard/audit-logs/page.tsxsrc/app/[locale]/dashboard/logs/_components/column-visibility-dropdown.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/index.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/types.tssrc/app/[locale]/dashboard/logs/_components/usage-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/api/actions/[...route]/route.tssrc/app/api/auth/login/route.tssrc/app/api/ip-geo/[ip]/route.tssrc/app/v1/_lib/proxy/auth-guard.tssrc/app/v1/_lib/proxy/message-service.tssrc/app/v1/_lib/proxy/session.tssrc/drizzle/schema.tssrc/hooks/use-ip-geo.tssrc/lib/api/action-adapter-openapi.tssrc/lib/audit/emit.tssrc/lib/audit/redact.test.tssrc/lib/audit/redact.tssrc/lib/audit/request-context.tssrc/lib/audit/with-audit.test.tssrc/lib/audit/with-audit.tssrc/lib/column-visibility.tssrc/lib/config/env.schema.tssrc/lib/config/system-settings-cache.tssrc/lib/ip-geo/client.test.tssrc/lib/ip-geo/client.tssrc/lib/ip/extract-client-ip.test.tssrc/lib/ip/extract-client-ip.tssrc/lib/ip/index.tssrc/lib/ip/private-ip.test.tssrc/lib/ip/private-ip.tssrc/lib/ledger-backfill/trigger.sqlsrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/audit-log.tssrc/repository/message.tssrc/repository/system-config.tssrc/repository/usage-logs.tssrc/types/audit-log.tssrc/types/ip-extraction.tssrc/types/ip-geo.tssrc/types/message.tssrc/types/system-config.ts
Comment on lines
+356
to
+364
| emitActionAudit({ | ||
| category: "key", | ||
| action: "key.create", | ||
| targetType: "key", | ||
| targetName: data.name ?? null, | ||
| success: false, | ||
| errorMessage: message, | ||
| redactExtraKeys: ["key"], | ||
| }); |
There was a problem hiding this comment.
错误分支审计用的是 error.message，可能把内部错误信息写进库
message = error instanceof Error ? error.message : ... 之后直接作为 errorMessage 写入审计。对于 DB 错误、pg 错误，error.message 可能包含 SQL 片段、表/列名或部分用户输入（如 duplicate key 的值），落库后会进入管理员面板。建议：
- 映射到稳定的错误代码（和
ActionResult.errorCode对齐），错误码入errorMessage，原始 message 留给 logger。 - 或在
createAuditLog/emitActionAudit层做一次固定长度截断 + 脱敏（复用redactExtraKeys之外的 free-text redactor）。
Also applies to: 649-657, 750-758
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/keys.ts` around lines 356 - 364, The audit is currently writing
raw error.message into emitActionAudit (seen at emitActionAudit({...
errorMessage: message ...})), which can leak DB/PG internals; change the flow to
map thrown errors in the key creation handlers to a stable error code or enum
(align with ActionResult.errorCode) and pass that code as errorMessage to
emitActionAudit/createAuditLog, while sending the original error.message to the
application logger (and/or to a separate non-admin-only audit field).
Alternatively implement a sanitization step inside
emitActionAudit/createAuditLog that truncates and redacts free-text error
messages (reusing the redactExtraKeys redactor) before persisting; update all
occurrences mentioned (around lines with emitActionAudit in key.create and the
other sites called out) to use the mapping or sanitization helper and keep
original message only in logs.
Comment on lines
1615
to
+1624
| revalidatePath("/dashboard"); | ||
| emitActionAudit({ | ||
| category: "user", | ||
| action: "user.update", | ||
| targetType: "user", | ||
| targetId: String(userId), | ||
| targetName: validatedData.name ?? null, | ||
| after: validatedData, | ||
| success: true, | ||
| }); |
There was a problem hiding this comment.
editUser 审计缺失 before 快照且 after 只记录补丁
当前只把 validatedData（部分字段的更新补丁）作为 after，没有读取更新前的用户状态作为 before，这与 removeUser 中使用 findUserById(userId) 构造 before 的做法不一致。按 PR 目标“每条审计需记录 before/after 快照”的说法，这里的 diff 实际上是残缺的（既不是完整 after，也没有 before），回溯定位被谁改了哪些字段时会比较困难。
建议在 updateUser 调用前读取一次用户，失败审计的 error 分支也能带上 before；或至少在注释中明确这里记录的是“更新补丁”而非快照。
🛠️ 建议改动
- const validatedData = validationResult.data;
+ const validatedData = validationResult.data;
+ const beforeUser = await findUserById(userId);
@@
- await updateUser(userId, {
+ const updatedUser = await updateUser(userId, {
...
});
@@
revalidatePath("/dashboard");
emitActionAudit({
category: "user",
action: "user.update",
targetType: "user",
targetId: String(userId),
- targetName: validatedData.name ?? null,
- after: validatedData,
+ targetName: validatedData.name ?? beforeUser?.name ?? null,
+ before: beforeUser ?? undefined,
+ after: updatedUser ?? validatedData,
success: true,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/users.ts` around lines 1615 - 1624, 在调用 updateUser/执行更新前用
findUserById(userId) 读取并保存 "before" 快照，然后在 emitActionAudit 调用中包含 before 字段；不要把
validatedData 当作完整的 after 快照——在更新成功后用 updateUser 的返回值或再次查询用户来填充完整的 "after"
快照；同时在失败的错误分支也把之前读取到的 before 一并记录并在 emitActionAudit 的 error 分支中输出。确保你修改的是包含
revalidatePath(...) 和 emitActionAudit(...) 的 editUser/updateUser
逻辑并在所有分支（success/error）一致地包含 before 和完整 after。
Comment on lines
+155
to
+158
| <p className="mt-2 text-xs text-muted-foreground"> | ||
| {t("fields.riskLevel")}: {data.data.threat.risk_level} (score{" "} | ||
| {data.data.threat.score.toFixed(3)}) | ||
| </p> |
There was a problem hiding this comment.
risk_level 直接渲染上游英文枚举值，未走 i18n
第 156 行将 data.data.threat.risk_level（例如 "low" / "medium" / "high" / "critical"）原样展示，中/日/俄等语言下显示为英文。若上游枚举值稳定，建议映射到 ipDetails.riskLevels.{low|medium|high|critical} 等翻译键后再展示；未命中时回退原值，兼容未来新增枚举。
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]/dashboard/_components/ip-details-dialog.tsx around lines
155 - 158, The IP details dialog currently renders data.data.threat.risk_level
directly; update rendering to translate the enum before display by mapping the
upstream values to i18n keys like ipDetails.riskLevels.low|medium|high|critical
using the t() function (e.g. t(`ipDetails.riskLevels.${riskKey}`)), ensure you
normalize the enum (toLowerCase()) and fall back to the raw risk string when the
translation key is missing; change the JSX around data.data.threat.risk_level
(and keep the existing t("fields.riskLevel") usage) so all user-facing risk
labels go through i18n.
| const beforeText = formatJson(log.beforeValue); | ||
| const afterText = formatJson(log.afterValue); | ||
|
|
||
| const operatorName = log.operatorUserName ?? "Admin Token"; |
There was a problem hiding this comment.
多处用户可见文案硬编码，违反 i18n 规范
以下位置直接嵌入英文字面量，未走 next-intl：
- 第 59 行
"Admin Token"（operator 名称兜底） - 第 127 行
id: {log.targetId} - 第 142 行
key: {log.operatorKeyName} - 第 147 行
user id: {log.operatorUserId} - 第 167 行
<Row label="User-Agent">（字段标签，虽是技术术语但作为标签展示，应走翻译以保持 5 语种一致）
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"。
建议在 messages/*/auditLogs.json 的 detail / columns 命名空间补键（如 detail.adminToken、detail.targetIdLabel、detail.keyLabel、detail.userIdLabel、columns.userAgent）后替换。
Also applies to: 127-127, 142-142, 147-147, 167-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsx
at line 59, Several user-facing strings are hardcoded and must use next-intl
translations: replace the fallback "Admin Token" (operatorName), the inline
labels/values shown as `id: {log.targetId}`, `key: {log.operatorKeyName}`, `user
id: {log.operatorUserId}`, and the Row label "User-Agent" with translation
lookups; add keys such as detail.adminToken, detail.targetIdLabel,
detail.keyLabel, detail.userIdLabel and columns.userAgent to
messages/*/auditLogs.json for each locale, then update the component
(references: operatorName, the JSX that renders `id: {log.targetId}`, `key:
{log.operatorKeyName}`, `user id: {log.operatorUserId}`, and the Row with label
"User-Agent") to use the intl/messages helper (e.g., t('...')) instead of
hardcoded literals so all five locales are covered.
Comment on lines
+49
to
+77
| try { | ||
| const result = await fn(); | ||
| const target = resolveTarget(options.target, result); | ||
| const after = options.extractAfter !== undefined ? options.extractAfter(result) : result; | ||
| const redactedAfter = | ||
| after !== undefined ? redactSensitive(after, options.redactExtraKeys) : undefined; | ||
|
|
||
| emitAudit({ | ||
| category: options.category, | ||
| action: options.action, | ||
| target, | ||
| success: true, | ||
| before: redactedBefore, | ||
| after: redactedAfter, | ||
| }); | ||
| return result; | ||
| } catch (error) { | ||
| const target = resolveTarget(options.target, undefined as T); | ||
| emitAudit({ | ||
| category: options.category, | ||
| action: options.action, | ||
| target, | ||
| success: false, | ||
| before: redactedBefore, | ||
| errorMessage: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
成功路径中的 extractAfter / redactSensitive 可能把成功的 mutation 变成失败。
注释（第 38-43 行）声称 "Audit writes are fire-and-forget so they never block or fail the mutation"，但第 52-54 行的 options.extractAfter(result) 和 redactSensitive(after, ...) 都在 try 块中同步执行且未做保护。一旦任意一个抛错（例如调用方的 extractAfter 越过 null、redactSensitive 在特殊输入上抛异常），就会进入第 65 行 catch 分支：
- 调用方拿不到已经写库的
result，收到的反而是一个审计内部错误； - 会记录一条
success: false的审计日志，但这条 mutation 其实是成功的——审计结果与事实相反。
建议把 after 的提取/脱敏与审计写入隔离在各自的 try/catch 里，保证哪怕审计链路全挂，也不影响返回值与审计的正确状态。
🛠️ 建议修复
try {
const result = await fn();
- const target = resolveTarget(options.target, result);
- const after = options.extractAfter !== undefined ? options.extractAfter(result) : result;
- const redactedAfter =
- after !== undefined ? redactSensitive(after, options.redactExtraKeys) : undefined;
-
- emitAudit({
- category: options.category,
- action: options.action,
- target,
- success: true,
- before: redactedBefore,
- after: redactedAfter,
- });
+ try {
+ const target = resolveTarget(options.target, result);
+ let redactedAfter: unknown = undefined;
+ try {
+ const after =
+ options.extractAfter !== undefined ? options.extractAfter(result) : result;
+ redactedAfter =
+ after !== undefined ? redactSensitive(after, options.redactExtraKeys) : undefined;
+ } catch (redactErr) {
+ // never let redaction/extraction fail the mutation
+ // fall through with redactedAfter === undefined
+ }
+ emitAudit({
+ category: options.category,
+ action: options.action,
+ target,
+ success: true,
+ before: redactedBefore,
+ after: redactedAfter,
+ });
+ } catch {
+ // audit emission must never affect the caller
+ }
return result;
} catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/audit/with-audit.ts` around lines 49 - 77, The current try block runs
options.extractAfter(result) and redactSensitive(...) inline so any exception
there will bubble up and mark the mutation as failed; change the flow so the
core operation (await fn()) always returns its result and is not blocked by
auditing: call await fn() and immediately return the result to the caller, but
separately (fire-and-forget) perform extractAfter and redactSensitive inside
their own try/catch and then call emitAudit inside another try/catch (or the
same audit-protection block) so any errors from options.extractAfter,
redactSensitive, or emitAudit are caught and do not alter the returned result or
the success flag; reference resolveTarget(options.target, result),
options.extractAfter, redactSensitive(after, options.redactExtraKeys),
emitAudit({... success: true ...}) and keep the existing catch branch for fn()
failures that uses resolveTarget(options.target, undefined as T) and emits a
failure audit.
Comment on lines
+1008
to
+1020
| ipExtractionConfig: z | ||
| .union([ | ||
| z.null(), | ||
| z.object({ | ||
| headers: z.array( | ||
| z.object({ | ||
| name: z.string(), | ||
| pick: z.any().optional(), | ||
| }) | ||
| ), | ||
| }), | ||
| ]) | ||
| .optional(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the runtime consumer of ipExtractionConfig.pick to confirm expected shape
rg -nP -C3 '\bpick\b' --type=ts -g '!**/*.test.*' -g '!messages/**'
rg -nP -C3 'ipExtractionConfig' --type=tsRepository: ding113/claude-code-hub
Length of output: 16323
🏁 Script executed:
#!/bin/bash
# Find XffPick type definition
rg -nP 'type XffPick|interface XffPick' --type=ts
# Get complete pickFromChain function implementation
sed -n '46,70p' src/lib/ip/extract-client-ip.ts
# Search for any index-related handling in IP extraction code
rg -nP -C5 'index' src/lib/ip/extract-client-ip.ts | head -50Repository: ding113/claude-code-hub
Length of output: 1372
🏁 Script executed:
#!/bin/bash
# Check form component for pick field validation
sed -n '193,220p' src/app/[locale]/settings/config/_components/system-settings-form.tsx
# Search for tests related to ipExtractionConfig validation
fd -e test.ts -e spec.ts | xargs rg -l 'ipExtractionConfig|pick' 2>/dev/null | head -5
# Search i18n messages for ipExtractionConfig documentation
fd -path '*messages*' -type f | xargs rg -l 'pick|extraction' 2>/dev/null | head -5Repository: ding113/claude-code-hub
Length of output: 1625
🏁 Script executed:
#!/bin/bash
# Find UpdateSystemSettingsSchema definition and usage
rg -nP 'UpdateSystemSettingsSchema|SystemSettingsSchema' --type=ts | grep -E '(export|const|schema)' | head -20
# Look at messages directory structure and content
find . -path '*messages*' -type f \( -name '*.json' -o -name '*.ts' \) | head -10
# Check if there's i18n for ipExtractionConfig or pick
rg -rn 'pick|extraction' --type=json . 2>/dev/null | grep -i message | head -20Repository: ding113/claude-code-hub
Length of output: 4814
🏁 Script executed:
#!/bin/bash
# Get complete IpHeaderRule and IpExtractionConfig definitions
sed -n '1,20p' src/types/ip-extraction.ts
# Also verify the field name in the schema
sed -n '1008,1020p' src/lib/validation/schemas.tsRepository: ding113/claude-code-hub
Length of output: 730
pick 字段校验过于宽松，允许非法值被持久化
按 i18n 文案中承诺的契约以及 XffPick 类型定义，pick 应为 "leftmost" | "rightmost" | { kind: "index"; index: number }。但当前使用 z.any().optional() 允许任意值（包括错误类型、未知字符串）通过校验并被保存到数据库，违反类型契约。虽然运行时代码在 extract-client-ip.ts 中会安全地处理这些非法值（返回 null），但这样的设计允许坏数据进入持久化层，增加后续维护成本。建议收紧为 discriminated 联合类型：
建议改为严格校验
ipExtractionConfig: z
.union([
z.null(),
z.object({
headers: z.array(
z.object({
name: z.string().min(1),
- pick: z.any().optional(),
+ pick: z
+ .union([
+ z.literal("leftmost"),
+ z.literal("rightmost"),
+ z.object({
+ kind: z.literal("index"),
+ index: z.number().int(),
+ }),
+ ])
+ .optional(),
})
),
}),
])
.optional(),📝 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
| ipExtractionConfig: z | |
| .union([ | |
| z.null(), | |
| z.object({ | |
| headers: z.array( | |
| z.object({ | |
| name: z.string(), | |
| pick: z.any().optional(), | |
| }) | |
| ), | |
| }), | |
| ]) | |
| .optional(), | |
| ipExtractionConfig: z | |
| .union([ | |
| z.null(), | |
| z.object({ | |
| headers: z.array( | |
| z.object({ | |
| name: z.string().min(1), | |
| pick: z | |
| .union([ | |
| z.literal("leftmost"), | |
| z.literal("rightmost"), | |
| z.object({ | |
| kind: z.literal("index"), | |
| index: z.number().int(), | |
| }), | |
| ]) | |
| .optional(), | |
| }) | |
| ), | |
| }), | |
| ]) | |
| .optional(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/validation/schemas.ts` around lines 1008 - 1020, The
ipExtractionConfig schema currently permits any value for the pick field via
z.any().optional(), letting invalid values be persisted; change the pick
validator inside the ipExtractionConfig → headers array item to a discriminated
union matching XffPick: accept the literal strings "leftmost" and "rightmost"
and an object { kind: "index"; index: number } (with proper number validation),
keep pick optional and preserve the surrounding union with z.null(), and ensure
the schema name/ipExtractionConfig and header item object are updated so
extract-client-ip.ts will receive only those validated shapes.
![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 introduces IP extraction, IP geolocation, and comprehensive audit logging across three major features. The implementation is solid with good test coverage (~70 new tests), proper error handling, and fire-and-forget audit writes that never block the hot path.
PR Size: XL
- Lines changed: 8,638 (8,582 additions + 56 deletions)
- Files changed: 89
Recommendation: This PR is large but well-structured across 4 logical commits. The features are orthogonal and could be split into separate PRs for easier review:
- IP extraction + geolocation (Phase 1-3)
- Audit infrastructure (Phase 5-7)
- UI components (Phase 8)
However, since the work is already complete and tested, splitting now may not be worth the overhead.
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 |
| Code Quality | 0 | 1 | 1 | 0 |
High Priority Issues (Should Fix)
-
src/lib/audit/emit.ts:44 - Floating promise:
createAuditLogAsync(...)is called withoutvoid, creating an unhandled floating promise. Addvoidto signal fire-and-forget intent explicitly. -
src/lib/audit/with-audit.ts:1-126 - Dead code:
withAuditwrapper is fully implemented and tested but never used in production. All mutations useemitActionAuditinstead. The two implementations diverge in how they handle auth sessions (direct call vs try/catch wrapper). Either wire it up or remove it.
Medium Priority Issues (Consider Fixing)
- src/app/api/ip-geo/[ip]/route.ts:17-20 - Access control scope: The IP geolocation endpoint uses
getSession()which allows any authenticated user to query arbitrary IPs. The audit-log page is admin-only, but this API is not. Consider restricting to admin role if the design intent is to limit IP lookups to administrators.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - 1 floating promise issue
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Excellent (~70 new tests)
- Code clarity - 1 dead code issue, 1 access control question
Strengths
- Excellent test coverage for IP extraction (43 tests), geolocation (8 tests), and audit redaction (11 tests)
- Fire-and-forget audit writes never block the hot path
- Proper Redis caching with negative TTL for geolocation
- Clean separation of concerns (extraction, lookup, audit)
- Comprehensive i18n support across all 5 languages
- Migration includes missing schema from PR #1025
Automated review by Claude AI
| const session = await getSession(); | ||
| if (!session || session.user.role !== "admin") { | ||
| return { ok: false, error: "权限不足" }; | ||
| } |
Contributor
There was a problem hiding this comment.
[MEDIUM] [STANDARD-VIOLATION] ActionResult errors bypass i18n (errorCode missing)
Hardcoded error strings in new actions prevent localization and bypass the recommended errorCode path for new code.
Problematic line:
return { ok: false, error: "权限不足" };Repo requirement: CLAUDE.md — "All user-facing strings must use i18n (5 languages supported). Never hardcode display text".
Suggested fix:
import { getTranslations } from "next-intl/server";
import { ERROR_CODES } from "@/lib/utils/error-messages";
const tError = await getTranslations("errors");
if (!session || session.user.role !== "admin") {
return {
ok: false,
error: tError("PERMISSION_DENIED"),
errorCode: ERROR_CODES.PERMISSION_DENIED,
};
}
| } | ||
|
|
||
| const operator = log.operatorUserName ?? "Admin Token"; | ||
|
|
Contributor
There was a problem hiding this comment.
[MEDIUM] [STANDARD-VIOLATION] Hardcoded UI strings bypass i18n
This component uses next-intl, but still hardcodes user-facing strings:
const operator = log.operatorUserName ?? "Admin Token";
...
{error instanceof Error ? error.message : "Error"}Repo requirement: CLAUDE.md — "All user-facing strings must use i18n (5 languages supported). Never hardcode display text".
Suggested fix (add keys to messages/*/auditLogs.json and use them here):
const operator = log.operatorUserName ?? t("common.adminToken");
...
{error instanceof Error ? error.message : t("common.unknownError")}
| </Row> | ||
|
|
||
| <Row label="User-Agent"> | ||
| {log.userAgent ? ( |
Contributor
There was a problem hiding this comment.
[MEDIUM] [STANDARD-VIOLATION] Audit detail sheet hardcodes user-facing labels
This file hardcodes multiple UI strings in a localized page:
const operatorName = log.operatorUserName ?? "Admin Token";
...
<Row label="User-Agent">...
...
id: {log.targetId}
key: {log.operatorKeyName}
user id: {log.operatorUserId}Repo requirement: CLAUDE.md — "All user-facing strings must use i18n (5 languages supported). Never hardcode display text".
Suggested fix (add keys to messages/*/auditLogs.json and use them here):
const operatorName = log.operatorUserName ?? t("common.adminToken");
<Row label={t("detail.userAgent")}>
...
</Row>
{log.targetId && (
<div className="text-xs text-muted-foreground font-mono">
{t("detail.id")}: {log.targetId}
</div>
)}
| </div> | ||
| <p className="mt-2 text-xs text-muted-foreground"> | ||
| {t("fields.riskLevel")}: {data.data.threat.risk_level} (score{" "} | ||
| {data.data.threat.score.toFixed(3)}) |
Contributor
There was a problem hiding this comment.
[MEDIUM] [STANDARD-VIOLATION] Hardcoded score breaks i18n
This localized component still hardcodes an English label:
{t("fields.riskLevel")}: {data.data.threat.risk_level} (score {data.data.threat.score.toFixed(3)})Repo requirement: CLAUDE.md — "All user-facing strings must use i18n (5 languages supported). Never hardcode display text".
Suggested fix (add a translation key, e.g. ipDetails.fields.score, and use it):
{t("fields.riskLevel")}: {data.data.threat.risk_level} (
{t("fields.score")} {data.data.threat.score.toFixed(3)}
)
| } | ||
| } catch { | ||
| // Treat invalid JSON as "use default" | ||
| ipExtractionConfigToSave = null; |
Contributor
There was a problem hiding this comment.
[HIGH] [ERROR-NO-USER-FEEDBACK] Invalid IP extraction JSON is silently discarded
In handleSubmit, invalid (or wrong-shape) JSON is treated as null (default chain) without notifying the admin:
} catch {
// Treat invalid JSON as "use default"
ipExtractionConfigToSave = null;
}This makes "Save" succeed while the operator's intended config is ignored (easy to misconfigure IP extraction).
Suggested fix (surface validation error + abort submit):
try {
const parsed = JSON.parse(trimmedIpExtractionConfig);
if (!parsed || typeof parsed !== "object" || !Array.isArray((parsed as { headers?: unknown }).headers)) {
toast.error(tIpLogging("extractionConfigInvalid"));
return;
}
ipExtractionConfigToSave = parsed as IpExtractionConfig;
} catch (error) {
toast.error(tIpLogging("extractionConfigInvalidJson"));
return;
}
| */ | ||
| export async function withAudit<T>(options: WithAuditOptions<T>, fn: () => Promise<T>): Promise<T> { | ||
| const before = options.snapshotBefore ? await options.snapshotBefore() : undefined; | ||
| const redactedBefore = |
Contributor
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] withAudit can change mutation behavior when audit helpers throw
The docstring promises audit should not affect the wrapped operation:
// ... Audit writes are fire-and-forget so they never block or fail the mutation.
// On exception, a failure audit row is emitted before the error is rethrown.But snapshotBefore runs before the try, so if it throws there is no failure audit row. Also extractAfter / redaction run after fn() resolves; if either throws, the wrapper emits a failure row and rethrows, failing the overall mutation even though fn() succeeded.
Suggested fix (guard audit-only code paths so they never alter control-flow):
import { logger } from "@/lib/logger";
let redactedBefore: unknown | undefined;
try {
const before = options.snapshotBefore ? await options.snapshotBefore() : undefined;
redactedBefore = before !== undefined ? redactSensitive(before, options.redactExtraKeys) : undefined;
} catch (error) {
logger.warn("[Audit] snapshotBefore failed", {
action: options.action,
error: error instanceof Error ? error.message : String(error),
});
}
try {
const result = await fn();
let redactedAfter: unknown | undefined;
try {
const after = options.extractAfter ? options.extractAfter(result) : result;
redactedAfter = after !== undefined ? redactSensitive(after, options.redactExtraKeys) : undefined;
} catch (error) {
logger.warn("[Audit] extractAfter/redact failed", {
action: options.action,
error: error instanceof Error ? error.message : String(error),
});
}
emitAudit({ ... , success: true, before: redactedBefore, after: redactedAfter });
return result;
} catch (error) {
emitAudit({ ... , success: false, before: redactedBefore, errorMessage: ... });
throw error;
}(Also add unit tests for snapshotBefore/extractAfter throwing so this doesn't regress.)
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR bundles multiple large features (IP extraction/logging, IP geolocation proxy, audit logging infrastructure, and audit log dashboard UI) into a single XL change set. Main concerns are (1) i18n leaks from new UI/actions, and (2) a couple of correctness/UX issues where invalid config is silently discarded and the audit wrapper can affect mutation behavior.
PR Size: XL
- Lines changed: 8638
- Files changed: 89
- Split suggestions:
- PR 1: DB schema + migrations (audit_log table, client_ip columns, triggers)
- PR 2: IP extraction library + settings UI (ipExtractionConfig/ipGeoLookupEnabled)
- PR 3: IP geolocation client +
/api/ip-geo/:ip+ IP details dialog - PR 4: Audit infrastructure (request context, redaction, repository) + action instrumentation
- PR 5: Audit logs dashboard UI + i18n message updates
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 4 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
- None
High Priority Issues (Should Fix)
src/lib/audit/with-audit.ts:46—withAuditcan change mutation behavior when audit helpers throw (Confidence: 85)src/app/[locale]/settings/config/_components/system-settings-form.tsx:207— invalid JSON is silently discarded, making “Save” succeed with unintended defaults (Confidence: 85)src/actions/audit-logs.ts:51— new action returns hardcodederrorstrings without i18n/errorCode path (Confidence: 80)src/app/[locale]/dashboard/audit-logs/_components/audit-logs-view.tsx:223— hardcoded UI strings bypass i18n ("Admin Token","Error") (Confidence: 80)src/app/[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsx:168— hardcoded detail labels bypass i18n ("Admin Token","User-Agent", etc.) (Confidence: 80)src/app/[locale]/dashboard/_components/ip-details-dialog.tsx:157— hardcodedscorelabel bypasses i18n (Confidence: 80)
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.
- PR reviewed:
#1027(feat: IP 记录 & 审计日志 & IP 归属地查询) - Applied PR size label:
size/XL - Posted inline review comments on:
src/lib/audit/with-audit.ts:46(audit wrapper can affect mutation behavior if audit helpers throw)src/app/[locale]/settings/config/_components/system-settings-form.tsx:207(invalid JSON silently discarded)src/actions/audit-logs.ts:51(new action hardcodes error strings / missing i18n errorCode path)src/app/[locale]/dashboard/audit-logs/_components/audit-logs-view.tsx:223(hardcoded UI strings bypass i18n)src/app/[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsx:168(hardcoded detail labels bypass i18n)src/app/[locale]/dashboard/_components/ip-details-dialog.tsx:157(hardcodedscorelabel bypasses i18n)
- Submitted the required review summary via
gh pr review --comment
Security / correctness:
- P1: remove `cf-connecting-ip` from DEFAULT_IP_EXTRACTION_CONFIG. Trusted
by default it was spoofable in non-CF deployments (login / pre-auth
rate-limit bypass). Operators fronted by Cloudflare add it explicitly.
- Critical: `fn_upsert_usage_ledger` no longer overwrites `created_at` on
conflict. Ledger rows' insert time is immutable by design.
- Major: `redactSensitive()` now only walks POJOs — Date / Map / Buffer /
class instances pass through intact instead of being rewritten to `{}`.
- Major: fix `webhookSecret` redaction. DEFAULT_SENSITIVE_KEYS was mixed-
case vs the lowercased walker; camelCase webhook secrets slipped through
unscrubbed. All keys normalized to lowercase; also added `api-key` and
`webhook-secret` variants. +3 tests.
- Restrict `/api/ip-geo/:ip` to admin (IP lookup is an operator tool;
non-admins going through it would scan IPs against our upstream quota).
- Never write raw `error.message` into `audit_log.error_message`. Pg errors
can carry SQL fragments, constraint names, or user input (duplicate key
values). All mutation actions now persist a stable `UPDATE_FAILED` /
`CREATE_FAILED` / `DELETE_FAILED` / `SYNC_FAILED` code instead.
- Stricter IP-geo upstream shape validation. Cached-for-1h payloads must
now carry `ip`, `location.country.{code,name}`, and `connection.asn`;
partial drifts fall into the 60s negative cache instead of blowing up
the dashboard dialog.
Audit robustness:
- Remove dead `withAudit` wrapper. Production only uses `emitActionAudit`,
and the wrapper's own `extractAfter` / `redactSensitive` lived inside the
success-path try block and could turn a completed mutation into a thrown
exception. Flagged by two reviewers.
- `emitActionAudit` uses a new `resolveRequestContext()` that falls back
to `next/headers` when the OpenAPI adapter's AsyncLocalStorage is empty.
This captures operator IP / User-Agent for direct Next.js Server Actions
(e.g. the settings form's `startTransition`) that bypass the adapter.
- `editUser`: before-snapshot + after-snapshot via `safeFindUser()` (never
throws — audit is best-effort, must not break the mutation).
- Explicit `void` on `createAuditLogAsync(...)` to mark fire-and-forget
intent rather than leaving a floating promise.
UX / i18n:
- System settings: invalid IP-extraction JSON (or wrong shape) now blocks
save with a localized toast error rather than silently reverting to the
default chain. `ipLoggingInvalidJson` / `ipLoggingInvalidShape` keys in
all 5 locales.
- `risk_level` rendered from upstream now goes through `ipDetails.riskLevels.*`
(none/low/medium/high/critical) in all 5 locales; unknown future enums
fall back to the raw string.
- Localize all hardcoded strings in audit dashboard: "Admin Token" fallback,
"User-Agent" row label, "id/key/user id:" label prefixes, "Error" loader
fallback, `auditLogs.adminTokenOperator` / `loadError` / `errors.*`.
- `auditLogs` actions use `getTranslations("errors")` + `ERROR_CODES` so
non-admin / failure paths speak the operator's language and expose a
stable `errorCode` for the UI.
Smaller things:
- `/api/ip-geo/:ip` accepts `?lang=`; dashboard's `useIpGeo` forwards the
current locale so country/city names come back localized.
- `useIpGeo` route params no longer double-decoded (Next already decodes
path params; the old `decodeURIComponent` would throw on a lone `%`).
- `countAuditLogs` now accepts the same filter set as `listAuditLogs` so
paginated counts stay consistent with page contents.
- `audit-logs-view` virtualizer `itemCount` includes a trailing loader row
when `hasNextPage` is true, so the in-list loader actually renders.
- Replace `../../_components/ip-details-dialog` relative imports with the
`@/` alias across logs + audit-log views.
- Default `IP_GEO_API_URL` switched to `https://ip-api.claude-code-hub.app`.
Pre-commit: typecheck ✓ / lint ✓ (20 pre-existing warnings, 0 errors) /
build ✓ / test 4466 pass, 1 pre-existing unrelated failure
(`tests/api/api-endpoints.test.ts 健康检查`, fails on dev too — missing
DSN in test env).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
Comment on lines
+56
to
+74
| function isValidLookupResult(data: unknown): data is IpGeoLookupResult { | ||
| if (!data || typeof data !== "object") return false; | ||
| const d = data as Record<string, unknown>; | ||
| if (typeof d.ip !== "string" || d.ip.length === 0) return false; | ||
|
|
||
| const location = d.location as Record<string, unknown> | undefined; | ||
| if (!location || typeof location !== "object") return false; | ||
| const country = location.country as Record<string, unknown> | undefined; | ||
| if (!country || typeof country !== "object") return false; | ||
| if (typeof country.code !== "string" || typeof country.name !== "string") return false; | ||
|
|
||
| const connection = d.connection as Record<string, unknown> | undefined; | ||
| if (!connection || typeof connection !== "object") return false; | ||
| if (typeof connection.asn !== "number") return false; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| async function fetchFromUpstream( |
There was a problem hiding this comment.
isValidLookupResult doesn't validate flag despite the stated goal
The function comment on line 102 explicitly says "fail here rather than letting the dashboard blow up at data.location.country.flag.emoji", but the validator only checks country.code and country.name — it never verifies that flag (or flag.emoji) is present. If the upstream API returns a partial payload without flag, isValidLookupResult returns true, the result is cached for up to 1 hour, and ip-details-dialog.tsx line 99 crashes at data.data.location.country.flag.emoji (no optional chaining there).
Add the missing check:
Suggested change
| function isValidLookupResult(data: unknown): data is IpGeoLookupResult { | |
| if (!data || typeof data !== "object") return false; | |
| const d = data as Record<string, unknown>; | |
| if (typeof d.ip !== "string" || d.ip.length === 0) return false; | |
| const location = d.location as Record<string, unknown> | undefined; | |
| if (!location || typeof location !== "object") return false; | |
| const country = location.country as Record<string, unknown> | undefined; | |
| if (!country || typeof country !== "object") return false; | |
| if (typeof country.code !== "string" || typeof country.name !== "string") return false; | |
| const connection = d.connection as Record<string, unknown> | undefined; | |
| if (!connection || typeof connection !== "object") return false; | |
| if (typeof connection.asn !== "number") return false; | |
| return true; | |
| } | |
| async function fetchFromUpstream( | |
| if (typeof country.code !== "string" || typeof country.name !== "string") return false; | |
| const flag = country.flag as Record<string, unknown> | undefined; | |
| if (!flag || typeof flag !== "object") return false; | |
| if (typeof flag.emoji !== "string") return false; | |
| const connection = d.connection as Record<string, unknown> | undefined; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/ip-geo/client.ts
Line: 56-74
Comment:
**`isValidLookupResult` doesn't validate `flag` despite the stated goal**
The function comment on line 102 explicitly says "fail here rather than letting the dashboard blow up at `data.location.country.flag.emoji`", but the validator only checks `country.code` and `country.name` — it never verifies that `flag` (or `flag.emoji`) is present. If the upstream API returns a partial payload without `flag`, `isValidLookupResult` returns `true`, the result is cached for up to 1 hour, and `ip-details-dialog.tsx` line 99 crashes at `data.data.location.country.flag.emoji` (no optional chaining there).
Add the missing check:
```suggestion
if (typeof country.code !== "string" || typeof country.name !== "string") return false;
const flag = country.flag as Record<string, unknown> | undefined;
if (!flag || typeof flag !== "object") return false;
if (typeof flag.emoji !== "string") return false;
const connection = d.connection as Record<string, unknown> | undefined;
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e8cb50c49
ℹ️ 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".
Comment on lines
+18
to
+20
| const settings = getCachedSystemSettingsOnlyCache(); | ||
| const config = settings?.ipExtractionConfig ?? DEFAULT_IP_EXTRACTION_CONFIG; | ||
| return extractClientIp(source, config); |
There was a problem hiding this comment.
getClientIp() reads only the in-memory settings cache and silently falls back to DEFAULT_IP_EXTRACTION_CONFIG when that cache is empty, so a restart causes login/auth IP parsing to ignore the admin-configured ipExtractionConfig until some other code path warms the cache. The login route uses this function for LoginAbusePolicy, so deployments behind CDNs/proxies can temporarily collapse many users to one proxy IP, causing incorrect lockout behavior and wrong audit IP attribution right after startup.
Useful? React with 👍 / 👎.
Comment on lines
+65
to
+69
| if (typeof country.code !== "string" || typeof country.name !== "string") return false; | ||
|
|
||
| const connection = d.connection as Record<string, unknown> | undefined; | ||
| if (!connection || typeof connection !== "object") return false; | ||
| if (typeof connection.asn !== "number") return false; |
There was a problem hiding this comment.
The upstream shape check only requires country.code/name and connection.asn, but the UI (ip-details-dialog.tsx) unconditionally reads deeper fields like location.country.flag.emoji, timezone.id, and threat.score. A partial upstream payload can therefore pass validation, be cached as ok, and then crash or break the dialog rendering for subsequent requests until the cache expires.
Useful? React with 👍 / 👎.
Upstream returns null for many fields when the IP is CGN / bogon /
Tailscale (100.64/10, 198.18/15, etc.) — concrete example from prod:
{
"ip": "100.85.244.112",
"location": {
"country": { "code": "ZZ", "name": "Unknown", ... },
"region": null, "city": null, "postal_code": null,
"coordinates": { "latitude": 0, "longitude": 0, "accuracy_radius_km": null }
},
"connection": {
"asn": null, "handle": null, "route": null, "domain": null,
"organization": "Carrier-Grade NAT RFC6598", "rir": "UNKNOWN", ...
},
...
}
Two problems before this commit:
1. `isValidLookupResult` required `connection.asn: number`, so the entire
CGN payload was rejected as "unexpected shape" and negative-cached for
60s. Operators never saw the carrier/NAT org info we DO have.
2. The dialog rendered every field unconditionally, producing strings like
`ASnull`, a bare "Route:" row with no value, and `0, 0` coordinates.
Fixes:
- Types: `IpGeoConnection.asn` and `route` are `string | number | null`;
`IpGeoFlag.svg` and `png` are `string | null`. Mirrors reality.
- Validator: require only the truly UI-critical subtree (`ip`,
`location.country.{code,name}`, `timezone.id`, `connection` as an
object). Nullable asn / route pass through. Payloads still missing
country / timezone are still rejected.
- Dialog hides rows when their value is null or the "unknown" sentinel:
- `asn === null` → row hidden (instead of `ASnull`)
- `route === null` → row hidden
- `organization === null` → row hidden
- `rir === "UNKNOWN"` → row hidden
- `is_anycast === false` → row hidden (it's the common case)
- Coordinates hidden when `accuracy_radius_km === null` OR lat/lng are
both 0 — the API's "we don't actually know" signal. Pure helper
`hasMeaningfulCoordinates()` is exported and unit-tested.
Tests (+7 cases):
- `client.test.ts`: the full CGN payload is accepted; partial payload
missing top-level required subtree is still rejected.
- `ip-details-dialog.test.tsx` (new, happy-dom):
- `hasMeaningfulCoordinates()` across null accuracy / 0,0 null-island /
real lat+lng / accuracy=0 cases (4 cases).
- Full-render check with the CGN payload: asserts `ASnull` never
appears, coordinates row hidden, `UNKNOWN` RIR hidden, `Anycast` row
hidden when false, organization + hostname + timezone still present.
Typecheck / lint / build / test pass (1 pre-existing unrelated failure).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/actions/users.ts (1)
1604-1607:⚠️ Potential issue | 🟠 Major失败审计分支仍然丢失了
before快照。
beforeUser只存在于成功路径的局部作用域里，所以一旦updateUser()/deleteUser()抛错，catch 里发出的user.update/user.delete失败事件没有before、targetName等上下文。排查失败变更时就看不到当时试图修改/删除的是谁。建议把beforeUser提前提升到 try 外层，并在失败事件里一并带上。Also applies to: 1650-1660, 1685-1709
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/users.ts` around lines 1604 - 1607, The before-snapshot (beforeUser) is declared inside the success path so failures of updateUser()/deleteUser() end up emitting failure events without before/targetName context; move the safeFindUser(userId) call and the beforeUser variable declaration out above the try block (so beforeUser is defined for both success and catch paths), and update the catch handlers that emit "user.update" / "user.delete" failure events to include beforeUser and any derived targetName/identifier fields in the event payload; apply the same change to the other similar blocks (the other update/delete flows referenced by safeFindUser at the other ranges).src/lib/ip-geo/client.ts (1)
61-69:⚠️ Potential issue | 🟠 Major把
country.flag.emoji也纳入成功结果校验。现在的 guard 只保证了
country.code/name和connection.asn，但注释里点名要保护的data.location.country.flag.emoji仍然可能缺失并被当成成功结果缓存。这样上游只要返回一个“半截”的country对象，同一个 IP 仍会走status: "ok"并被缓存，详情弹窗还是会在读取 flag 时出错。建议把实际渲染依赖的flag.emoji（以及同级必需字段）一起校验，失败时走负缓存。Also applies to: 99-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ip-geo/client.ts` around lines 61 - 69, The guard currently validates location, country.code/name and connection.asn but omits country.flag.emoji so partial country objects get treated as success; update the validation in the guard (the block referencing location, country, connection and variables like d, location, country, connection) to also require that country.flag exists and country.flag.emoji is a string (e.g., check typeof country.flag === "object" and typeof country.flag.emoji === "string"), and if missing return false so the code falls into the negative-cache path; apply the same additional checks to the other identical validation block mentioned (the one around lines 99-105).
🧹 Nitpick comments (1)
src/actions/audit-logs.ts (1)
11-25: 建议用satisfies让AUDIT_CATEGORY_VALUES与AuditCategory编译期同步当前
AUDIT_CATEGORY_VALUES手工列举了所有分类，与src/types/audit-log.ts中的AuditCategory字面量联合类型完全重复。若将来在类型里新增一个分类（例如"webhook"）却忘了同步这里，前台传入该分类会被isAuditCategory静默判为false、过滤条件被丢弃，且无任何 TS 报错。可通过
satisfies readonly AuditCategory[]+ 反向穷尽性检查来让 TS 在遗漏时报错：♻️ 参考实现
-const AUDIT_CATEGORY_VALUES: AuditCategory[] = [ +const AUDIT_CATEGORY_VALUES = [ "auth", "user", "provider", "provider_group", "system_settings", "key", "notification", "sensitive_word", "model_price", -]; +] as const satisfies readonly AuditCategory[]; +// Exhaustiveness guard: fails to compile if AuditCategory gains a new member. +type _AuditCategoryExhaustive = + Exclude<AuditCategory, (typeof AUDIT_CATEGORY_VALUES)[number]> extends never ? true : never;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/audit-logs.ts` around lines 11 - 25, AUDIT_CATEGORY_VALUES is manually enumerated and can drift from the AuditCategory literal union; change its declaration to use TypeScript's "satisfies readonly AuditCategory[]" so the array is checked at compile time against the AuditCategory type, and add a reverse/exhaustiveness check (e.g., derive AuditCategory from the array or include a const assertion + a fallback exhaustive check) so missing variants cause a TS error; update the related isAuditCategory(value: string): value is AuditCategory to use the new typed AUDIT_CATEGORY_VALUES (no cast) for includes checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drizzle/0089_curly_grey_gargoyle.sql`:
- Around line 21-29: The provider_groups table defines created_at and updated_at
with DEFAULT now() but lacks NOT NULL; update the column definitions in the
CREATE TABLE for provider_groups so both created_at and updated_at are declared
as "timestamp with time zone DEFAULT now() NOT NULL" (matching the
audit_log.created_at pattern) to prevent explicit NULL writes and keep schema
consistency.
In `@messages/ru/settings/config.json`:
- Line 128: Update the Russian hint string so it reflects the actual
security-default chain defined in DEFAULT_IP_EXTRACTION_CONFIG (in
src/types/ip-extraction.ts) by replacing the current "cf-connecting-ip →
x-real-ip → x-forwarded-for (самое правое значение)" with "x-real-ip →
x-forwarded-for (самое правое значение)"; also keep any mention of
"cf-connecting-ip" only inside the custom-example portion (not as part of the
default chain) so users aren't misled about which header is trusted by default.
In `@src/actions/users.ts`:
- Around line 1268-1282: The current user.create audit is only emitted in the
addUser path and its after snapshot contains only a few fields; extract the
audit into a reusable helper (e.g. emitUserCreateAudit(user)) and call it from
both addUser() and createUserOnly() (or wherever createUser() flows end) so
every creation path emits user.create; when building the audit payload (the
emitActionAudit call), set after to the full User object returned by
createUser() (include id, name, role, isEnabled, providerGroup, quota/limits,
expiration/expiry, labels/tags and any other fields from the User repository
record) instead of the current partial shape so the audit contains the complete
post-creation snapshot.
In `@src/lib/audit/emit.ts`:
- Around line 40-72: emitAsync currently awaits resolveRequestContext() and
createAuditLogAsync(...) without any error handling, which can lead to unhandled
promise rejections; wrap the entire body of emitAsync in a try/catch (or at
least around resolveRequestContext and createAuditLogAsync) and log the caught
error (e.g., use an existing logger like processLogger or console.error) and
swallow it so the fire-and-forget caller emitActionAudit doesn't surface
failures to the caller; keep use of safeGetScopedAuthSession, redactSensitive,
and the createAuditLogAsync call intact but ensure any thrown errors are caught
and handled within emitAsync.
---
Duplicate comments:
In `@src/actions/users.ts`:
- Around line 1604-1607: The before-snapshot (beforeUser) is declared inside the
success path so failures of updateUser()/deleteUser() end up emitting failure
events without before/targetName context; move the safeFindUser(userId) call and
the beforeUser variable declaration out above the try block (so beforeUser is
defined for both success and catch paths), and update the catch handlers that
emit "user.update" / "user.delete" failure events to include beforeUser and any
derived targetName/identifier fields in the event payload; apply the same change
to the other similar blocks (the other update/delete flows referenced by
safeFindUser at the other ranges).
In `@src/lib/ip-geo/client.ts`:
- Around line 61-69: The guard currently validates location, country.code/name
and connection.asn but omits country.flag.emoji so partial country objects get
treated as success; update the validation in the guard (the block referencing
location, country, connection and variables like d, location, country,
connection) to also require that country.flag exists and country.flag.emoji is a
string (e.g., check typeof country.flag === "object" and typeof
country.flag.emoji === "string"), and if missing return false so the code falls
into the negative-cache path; apply the same additional checks to the other
identical validation block mentioned (the one around lines 99-105).
---
Nitpick comments:
In `@src/actions/audit-logs.ts`:
- Around line 11-25: AUDIT_CATEGORY_VALUES is manually enumerated and can drift
from the AuditCategory literal union; change its declaration to use TypeScript's
"satisfies readonly AuditCategory[]" so the array is checked at compile time
against the AuditCategory type, and add a reverse/exhaustiveness check (e.g.,
derive AuditCategory from the array or include a const assertion + a fallback
exhaustive check) so missing variants cause a TS error; update the related
isAuditCategory(value: string): value is AuditCategory to use the new typed
AUDIT_CATEGORY_VALUES (no cast) for includes checks.
🪄 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: 33a2388a-d1b6-430f-b851-9cd86bc0e476
📒 Files selected for processing (43)
drizzle/0089_curly_grey_gargoyle.sqlmessages/en/auditLogs.jsonmessages/en/ipDetails.jsonmessages/en/settings/config.jsonmessages/ja/auditLogs.jsonmessages/ja/ipDetails.jsonmessages/ja/settings/config.jsonmessages/ru/auditLogs.jsonmessages/ru/ipDetails.jsonmessages/ru/settings/config.jsonmessages/zh-CN/auditLogs.jsonmessages/zh-CN/ipDetails.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/auditLogs.jsonmessages/zh-TW/ipDetails.jsonmessages/zh-TW/settings/config.jsonsrc/actions/audit-logs.tssrc/actions/keys.tssrc/actions/model-prices.tssrc/actions/notifications.tssrc/actions/provider-groups.tssrc/actions/providers.tssrc/actions/sensitive-words.tssrc/actions/system-config.tssrc/actions/users.tssrc/app/[locale]/dashboard/_components/ip-details-dialog.tsxsrc/app/[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsxsrc/app/[locale]/dashboard/audit-logs/_components/audit-logs-view.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/api/ip-geo/[ip]/route.tssrc/hooks/use-ip-geo.tssrc/lib/audit/emit.tssrc/lib/audit/redact.test.tssrc/lib/audit/redact.tssrc/lib/audit/request-context.tssrc/lib/config/env.schema.tssrc/lib/ip-geo/client.tssrc/lib/ip/extract-client-ip.test.tssrc/lib/ledger-backfill/trigger.sqlsrc/repository/audit-log.tssrc/types/ip-extraction.ts
✅ Files skipped from review due to trivial changes (13)
- messages/zh-CN/auditLogs.json
- messages/en/ipDetails.json
- messages/ru/ipDetails.json
- messages/ru/auditLogs.json
- messages/ja/auditLogs.json
- messages/ja/ipDetails.json
- messages/en/auditLogs.json
- messages/zh-TW/auditLogs.json
- src/lib/ip/extract-client-ip.test.ts
- src/types/ip-extraction.ts
- messages/zh-CN/ipDetails.json
- messages/zh-TW/ipDetails.json
- src/app/[locale]/dashboard/audit-logs/_components/audit-logs-view.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- src/actions/notifications.ts
- src/lib/ledger-backfill/trigger.sql
- messages/zh-CN/settings/config.json
- src/lib/audit/redact.ts
- src/actions/system-config.ts
- src/actions/sensitive-words.ts
- src/lib/config/env.schema.ts
- src/actions/provider-groups.ts
- src/hooks/use-ip-geo.ts
- src/actions/providers.ts
- messages/zh-TW/settings/config.json
- src/actions/keys.ts
- messages/ja/settings/config.json
- src/app/[locale]/dashboard/audit-logs/_components/audit-log-detail-sheet.tsx
- src/lib/audit/request-context.ts
- messages/en/settings/config.json
- src/actions/model-prices.ts
- src/repository/audit-log.ts
Comment on lines
+197
to
+217
| if (trimmedIpExtractionConfig) { | ||
| let parsed: unknown; | ||
| try { | ||
| parsed = JSON.parse(trimmedIpExtractionConfig); | ||
| } catch (error) { | ||
| toast.error( | ||
| t("ipLoggingInvalidJson", { | ||
| message: error instanceof Error ? error.message : String(error), | ||
| }) | ||
| ); | ||
| return; | ||
| } | ||
| if ( | ||
| !parsed || | ||
| typeof parsed !== "object" || | ||
| !Array.isArray((parsed as { headers?: unknown }).headers) | ||
| ) { | ||
| toast.error(t("ipLoggingInvalidShape")); | ||
| return; | ||
| } | ||
| ipExtractionConfigToSave = parsed as IpExtractionConfig; |
There was a problem hiding this comment.
不要只校验 headers 数组，当前仍会保存无效规则。
这里会接受诸如 { "headers": [{ "name": "", "pick": "middle" }] } 这样的配置，因为现在只检查了 headers 是否为数组。可 src/types/ip-extraction.ts:1-8 里的 IpHeaderRule 实际只允许非空 name 和 leftmost/rightmost/{ kind: "index", index: number }；而 src/lib/validation/schemas.ts:1008-1023 目前也没补上这层约束，所以坏配置会直接落库，把异常留到请求热路径。建议把每个 rule 的 name / pick 也纳入一份共享的 runtime schema 校验。
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/lib/ip-geo/client.test.ts (1)
217-228: 超时测试会真实等待 1500ms这里没有使用
vi.useFakeTimers()，fetchFromUpstream中的setTimeout(..., env.IP_GEO_TIMEOUT_MS)会真实挂起 1.5s 才触发 abort，拉长单元测试整体耗时。建议把 mock 中的IP_GEO_TIMEOUT_MS下调到较小值（如 50），或切换到 fake timers 并手动推进。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ip-geo/client.test.ts` around lines 217 - 228, The test for lookupIp is waiting the real timeout from fetchFromUpstream because env.IP_GEO_TIMEOUT_MS is 1500ms; update the test to either set a smaller timeout (e.g., override env.IP_GEO_TIMEOUT_MS to ~50 before importing "./client" so lookupIp/fetchFromUpstream uses the shorter timeout) or switch the test to use fake timers (vi.useFakeTimers()), advance timers to trigger the abort, then restore timers; reference the test's usage of lookupIp and the implementation's fetchFromUpstream / env.IP_GEO_TIMEOUT_MS when making the change.src/app/[locale]/dashboard/_components/ip-details-dialog.test.tsx (1)
11-11: messages fixture 使用五级相对路径，且@/别名仅覆盖./src/
../../../../../messages/en/ipDetails.json对目录层级非常脆弱，一旦文件移动都会破坏。建议在tsconfig/vitest.config中为messages/也配置一个别名（例如@messages/），或在测试里直接内联最小化 messages 对象，便于后续维护。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/ip-details-dialog.test.tsx at line 11, The test imports the messages fixture via a fragile five-level relative path (import ipDetailsMessages from "../../../../../messages/en/ipDetails.json"), make this resilient by either: 1) replacing that import with an alias import (e.g. import ipDetailsMessages from "@messages/en/ipDetails.json") after adding a "@messages/*" mapping in tsconfig/vitest.config, or 2) simplifying the test by inlining a minimal messages object for the keys used by the ip-details-dialog tests (use the name ipDetailsMessages to keep references intact); update the import/reference in src/app/[locale]/dashboard/_components/ip-details-dialog.test.tsx accordingly.src/lib/ip-geo/client.ts (1)
78-84:connection结构校验可以再收紧一点目前仅校验
connection.asn的类型，其他字段（rir、type、scope、is_anycast）虽然 UI 在ip-details-dialog.tsx第 158/162/165 行有直接引用，但只要是对象即可通过。若上游将来漂移为connection: {}，会通过校验并被缓存 1 小时，UI 侧data.data.connection.type将渲染为undefined。建议顺手对rir: string、type: string、is_anycast: boolean也做最小类型 guard，与 UI 的依赖面对齐。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ip-geo/client.ts` around lines 78 - 84, Tighten the connection object guard by validating required fields rather than only asn: ensure the local variable connection (from d.connection) is an object and add checks that connection.rir and connection.type are strings (non-empty if desired) and that connection.is_anycast is a boolean (also validate scope if the UI depends on it); update the existing guard where connection.asn is checked to also validate these properties so malformed { } objects won't pass and cause undefined values in ip-details-dialog.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/dashboard/_components/ip-details-dialog.test.tsx:
- Line 70: The test fixture uses an emoji in flag.emoji ("🇿🇿") which violates
the guideline; update the fixture in ip-details-dialog.test.tsx by replacing
flag.emoji with a plain ASCII string (e.g., "ZZ" or "flag-zz") that preserves
the test semantics but removes emoji characters, leaving the rest of the flag
object (unicode, svg, png) unchanged; mirror the same change pattern used to fix
the similar issue in client.test.ts.
In `@src/app/`[locale]/dashboard/_components/ip-details-dialog.tsx:
- Around line 177-188: The clean badge condition currently shows when
!data.data.privacy.is_anonymous && !data.data.threat.is_threat which can
coincide with other privacy flags; change the condition for rendering the clean
Badge so it only appears when all privacy risk flags are false and threat is
false — i.e. check privacy.is_vpn, is_proxy, is_tor, is_relay, and is_anonymous
are all false and data.data.threat.is_threat is false before rendering the Badge
in the component that contains the Badge JSX (look for the div rendering Badge
items and the properties data.data.privacy.* and data.data.threat.is_threat).
In `@src/lib/ip-geo/client.test.ts`:
- Line 258: The test fixture contains a literal emoji in
CGN_PAYLOAD.location.country.flag.emoji ("🇿🇿"); replace this literal with a
non-emoji replacement such as an escaped Unicode sequence or a neutral
placeholder string (e.g. "\\uD83C\\uDFFF\\uD83C\\uDFFF" or "EMOJI_PLACEHOLDER")
in the test file (look for CGN_PAYLOAD and its nested location.country.flag
entries in src/lib/ip-geo/client.test.ts) so the fixture no longer contains raw
emoji characters while preserving the payload shape for assertions.
---
Nitpick comments:
In `@src/app/`[locale]/dashboard/_components/ip-details-dialog.test.tsx:
- Line 11: The test imports the messages fixture via a fragile five-level
relative path (import ipDetailsMessages from
"../../../../../messages/en/ipDetails.json"), make this resilient by either: 1)
replacing that import with an alias import (e.g. import ipDetailsMessages from
"@messages/en/ipDetails.json") after adding a "@messages/*" mapping in
tsconfig/vitest.config, or 2) simplifying the test by inlining a minimal
messages object for the keys used by the ip-details-dialog tests (use the name
ipDetailsMessages to keep references intact); update the import/reference in
src/app/[locale]/dashboard/_components/ip-details-dialog.test.tsx accordingly.
In `@src/lib/ip-geo/client.test.ts`:
- Around line 217-228: The test for lookupIp is waiting the real timeout from
fetchFromUpstream because env.IP_GEO_TIMEOUT_MS is 1500ms; update the test to
either set a smaller timeout (e.g., override env.IP_GEO_TIMEOUT_MS to ~50 before
importing "./client" so lookupIp/fetchFromUpstream uses the shorter timeout) or
switch the test to use fake timers (vi.useFakeTimers()), advance timers to
trigger the abort, then restore timers; reference the test's usage of lookupIp
and the implementation's fetchFromUpstream / env.IP_GEO_TIMEOUT_MS when making
the change.
In `@src/lib/ip-geo/client.ts`:
- Around line 78-84: Tighten the connection object guard by validating required
fields rather than only asn: ensure the local variable connection (from
d.connection) is an object and add checks that connection.rir and
connection.type are strings (non-empty if desired) and that
connection.is_anycast is a boolean (also validate scope if the UI depends on
it); update the existing guard where connection.asn is checked to also validate
these properties so malformed { } objects won't pass and cause undefined values
in ip-details-dialog.tsx.
🪄 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: f38c00a9-a554-4732-b35e-c6ef128bf0c2
📒 Files selected for processing (5)
src/app/[locale]/dashboard/_components/ip-details-dialog.test.tsxsrc/app/[locale]/dashboard/_components/ip-details-dialog.tsxsrc/lib/ip-geo/client.test.tssrc/lib/ip-geo/client.tssrc/types/ip-geo.ts
✅ Files skipped from review due to trivial changes (1)
- src/types/ip-geo.ts
| population: 0, | ||
| borders: [], | ||
| is_eu_member: false, | ||
| flag: { emoji: "🇿🇿", unicode: "U+1F1FF U+1F1FF", svg: null, png: null }, |
There was a problem hiding this comment.
测试 fixture 中使用 emoji 字符
flag.emoji: "🇿🇿" 与 client.test.ts 中的同一违规一致，断言逻辑并未依赖该字符，可替换为普通字符串。
As per coding guidelines: "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/app/`[locale]/dashboard/_components/ip-details-dialog.test.tsx at line
70, The test fixture uses an emoji in flag.emoji ("🇿🇿") which violates the
guideline; update the fixture in ip-details-dialog.test.tsx by replacing
flag.emoji with a plain ASCII string (e.g., "ZZ" or "flag-zz") that preserves
the test semantics but removes emoji characters, leaving the rest of the flag
object (unicode, svg, png) unchanged; mirror the same change pattern used to fix
the similar issue in client.test.ts.
Comment on lines
+177
to
+188
| <div className="flex flex-wrap gap-2"> | ||
| {data.data.privacy.is_vpn && <Badge variant="outline">{t("badges.vpn")}</Badge>} | ||
| {data.data.privacy.is_proxy && <Badge variant="outline">{t("badges.proxy")}</Badge>} | ||
| {data.data.privacy.is_tor && <Badge variant="outline">{t("badges.tor")}</Badge>} | ||
| {data.data.privacy.is_relay && <Badge variant="outline">{t("badges.relay")}</Badge>} | ||
| {data.data.threat.is_threat && ( | ||
| <Badge variant="destructive">{t("badges.threat")}</Badge> | ||
| )} | ||
| {!data.data.privacy.is_anonymous && !data.data.threat.is_threat && ( | ||
| <Badge variant="secondary">{t("badges.clean")}</Badge> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
clean badge 与其他风险 badge 可能同时出现
当 privacy.is_vpn / is_proxy / is_tor / is_relay 为 true，但 privacy.is_anonymous 与 threat.is_threat 恰好都为 false 时（上游确实会出现这种组合，比如透明代理），UI 会同时渲染 VPN（或 Proxy/Tor/Relay）badge 和 clean badge，语义互相矛盾。建议把 clean 的显示条件改为"所有 privacy 风险标志与 threat 标志全 false"再展示。
建议修复
- {!data.data.privacy.is_anonymous && !data.data.threat.is_threat && (
+ {!data.data.privacy.is_vpn &&
+ !data.data.privacy.is_proxy &&
+ !data.data.privacy.is_tor &&
+ !data.data.privacy.is_relay &&
+ !data.data.privacy.is_anonymous &&
+ !data.data.threat.is_threat && (
<Badge variant="secondary">{t("badges.clean")}</Badge>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/dashboard/_components/ip-details-dialog.tsx around lines
177 - 188, The clean badge condition currently shows when
!data.data.privacy.is_anonymous && !data.data.threat.is_threat which can
coincide with other privacy flags; change the condition for rendering the clean
Badge so it only appears when all privacy risk flags are false and threat is
false — i.e. check privacy.is_vpn, is_proxy, is_tor, is_relay, and is_anonymous
are all false and data.data.threat.is_threat is false before rendering the Badge
in the component that contains the Badge JSX (look for the div rendering Badge
items and the properties data.data.privacy.* and data.data.threat.is_threat).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0daaafa8cf
ℹ️ 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".
| "geoLookup": "Enable IP geolocation lookup", | ||
| "geoLookupHint": "When enabled, the dashboard will resolve IP addresses to country / city / ASN via the configured IP geolocation service.", | ||
| "extractionConfigLabel": "Header extraction chain (JSON)", | ||
| "extractionConfigHint": "Leave empty to use the built-in default: cf-connecting-ip → x-real-ip → x-forwarded-for (rightmost). Advanced: provide a JSON object with a `headers` array; each entry has `name` and optional `pick` (\"leftmost\" | \"rightmost\" | {\"kind\":\"index\",\"index\":N}).", |
There was a problem hiding this comment.
The settings help text says the built-in default chain starts with cf-connecting-ip, but the actual DEFAULT_IP_EXTRACTION_CONFIG now defaults to x-real-ip then rightmost x-forwarded-for (src/types/ip-extraction.ts). In deployments that rely on this hint, operators can leave config empty expecting Cloudflare IPs to be used and instead get proxy IP attribution/rate-limit behavior until they discover and override it manually. Please update this user-facing hint (and its locale equivalents) to match the real default.
Useful? React with 👍 / 👎.
| <div className="rounded-lg border bg-card divide-y"> | ||
| {clientIp && ( | ||
| <div className="p-3"> | ||
| <p className="text-xs text-muted-foreground mb-1">IP</p> |
There was a problem hiding this comment.
This newly introduced label is hardcoded as English text, so users on non-English locales will see mixed-language UI in the error details dialog. Given the repository rule that all user-facing strings must be i18n-backed, this should be moved to a translation key and rendered through t(...) to avoid localization regressions.
Useful? React with 👍 / 👎.
After re-reading every inline review comment against current code, 7 were
still valid and worth fixing:
Schema:
- `provider_groups.{created_at, updated_at}` gain NOT NULL in both
schema.ts and the migration CREATE TABLE. Matches the `audit_log.created_at`
pattern and prevents an explicit NULL slipping in via raw SQL. Drizzle
emitted migration 0090 as the catch-up ALTER for installs that already
ran 0089 with nullable columns; it's idempotent on fresh installs.
i18n:
- All 5 locale hint strings for `ipLogging.extractionConfigHint` now describe
the actual safe default chain (`x-real-ip → x-forwarded-for rightmost`),
not the old CF-first chain. `cf-connecting-ip` appears only inside the
"custom example for Cloudflare deployments" JSON so users aren't misled
about what the default trusts.
users.ts:
- `user.create` audit extracted to `emitUserCreateAudit(user)` and called
from BOTH `addUser` and `createUserOnly` (the unified-edit-dialog create
path). Previously `createUserOnly` emitted no audit at all.
- Success-path `after` snapshot is now the full user record returned by
`createUser()` (id, name, role, isEnabled, providerGroup, quota / limit
fields, expiresAt, tags, allowed/blocked clients & models, …) instead of
the partial shape.
- `createUserOnly`'s catch block now emits a `CREATE_FAILED` failure row
too.
- `editUser` / `removeUser`: hoisted `safeFindUser(userId)` above the
try/catch boundary so failure audit events also carry `before` snapshot
+ `targetName`. The helper never throws, so the hoist is safe.
emit.ts:
- Wrapped `emitAsync` body in try/catch. `createAuditLogAsync` and
`resolveRequestContext` already swallow their own errors, but
`redactSensitive()` walks caller-supplied values — a circular ref or a
throwing getter would otherwise surface as an unhandled rejection at the
`void emitAsync(...)` site. Caught errors are logged at `warn`.
ip-geo/client.ts:
- `isValidLookupResult` also requires `country.flag` to be an object with a
string `emoji`. The dialog dereferences `.flag.emoji` unconditionally, so
a drifted-upstream payload without the flag would render-crash. With the
extra gate such payloads fall into the 60s negative cache instead. (Note:
the review pointed at a phantom "second identical block around line
99-105" — that's the single `isValidLookupResult` caller in
`fetchFromUpstream`, not a duplicate. Only one validator needed updating.)
- +1 test: payload with flag but no `emoji` is rejected.
audit-logs.ts:
- `AUDIT_CATEGORY_VALUES` now uses `as const satisfies readonly AuditCategory[]`
+ an `Exclude<>`-based exhaustiveness type-check. Adding a variant to
`AuditCategory` without listing it here becomes a TS error instead of a
runtime-only drift.
Pre-commit: typecheck ✓ / lint ✓ / build ✓ / test 4476 pass, 1 pre-existing
unrelated failure (tests/api/api-endpoints.test.ts 健康检查).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
drizzle/0089_curly_grey_gargoyle.sql (1)
46-127: 触发器实现与src/lib/ledger-backfill/trigger.sql保持同步，行为正确。
client_ip与group_cost_multiplier已加入 INSERT 列表与ON CONFLICT DO UPDATE集合，created_at已按既往反馈从 UPDATE 中移除。EXCEPTION WHEN OTHERS吞掉错误并RETURN NEW的设计与源文件一致，用于保证message_request写入的韧性；相应地依赖后台 ledger backfill 任务来兜底修复可能的缺口，语义上合理。运维建议：
RAISE WARNING在许多日志设置下可能不会被采集。若希望对触发器失败可观测，可在后台 backfill 任务中暴露message_request与usage_ledger的差异计数为指标，以免长期缄默。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/0089_curly_grey_gargoyle.sql` around lines 46 - 127, fn_upsert_usage_ledger is implemented correctly and kept in sync with the reference trigger: keep client_ip and group_cost_multiplier in both INSERT and ON CONFLICT, keep created_at out of the UPDATE, and preserve the EXCEPTION WHEN OTHERS { RAISE WARNING ..., RETURN NEW } behavior to maintain write resilience; additionally, to improve observability (optional), expose a discrepancy metric in the ledger backfill job that compares message_request vs usage_ledger rows so RAISE WARNING (in fn_upsert_usage_ledger) failures aren’t silently lost.src/actions/audit-logs.ts (1)
79-90: 非法from/to被静默丢弃，建议至少记录一次调试日志。当调用方传入无法解析为
Date的字符串（例如前端手改 URL 参数或时区格式问题）时，当前实现会直接忽略该过滤项，用户看到的是“全量结果”而非错误提示，容易造成误以为筛选生效。对管理员审计视图而言，这种静默降级可能让异常不易被发现。建议至少logger.debug记录一次，或在入参校验层返回错误码。♻️ 建议的最小改动
if (input.filter?.from) { const from = new Date(input.filter.from); if (!Number.isNaN(from.getTime())) { filter.from = from; + } else { + logger.debug("[AuditLogsAction] Ignored invalid 'from' filter value", { + value: input.filter.from, + }); } } if (input.filter?.to) { const to = new Date(input.filter.to); if (!Number.isNaN(to.getTime())) { filter.to = to; + } else { + logger.debug("[AuditLogsAction] Ignored invalid 'to' filter value", { + value: input.filter.to, + }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/audit-logs.ts` around lines 79 - 90, The code silently ignores invalid date strings for input.filter.from and input.filter.to; update the parsing blocks so that when new Date(input.filter.from) or new Date(input.filter.to) yields an invalid date (Number.isNaN(...getTime()) === true) you emit a debug log (e.g., logger.debug) recording the original invalid value and the fact it was ignored (include the symbols input.filter.from, input.filter.to, and resulting from/to), rather than silently dropping the filter; keep the existing behavior of not setting filter.from/filter.to but ensure the debug message provides enough context for later troubleshooting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@drizzle/0089_curly_grey_gargoyle.sql`:
- Around line 46-127: fn_upsert_usage_ledger is implemented correctly and kept
in sync with the reference trigger: keep client_ip and group_cost_multiplier in
both INSERT and ON CONFLICT, keep created_at out of the UPDATE, and preserve the
EXCEPTION WHEN OTHERS { RAISE WARNING ..., RETURN NEW } behavior to maintain
write resilience; additionally, to improve observability (optional), expose a
discrepancy metric in the ledger backfill job that compares message_request vs
usage_ledger rows so RAISE WARNING (in fn_upsert_usage_ledger) failures aren’t
silently lost.
In `@src/actions/audit-logs.ts`:
- Around line 79-90: The code silently ignores invalid date strings for
input.filter.from and input.filter.to; update the parsing blocks so that when
new Date(input.filter.from) or new Date(input.filter.to) yields an invalid date
(Number.isNaN(...getTime()) === true) you emit a debug log (e.g., logger.debug)
recording the original invalid value and the fact it was ignored (include the
symbols input.filter.from, input.filter.to, and resulting from/to), rather than
silently dropping the filter; keep the existing behavior of not setting
filter.from/filter.to but ensure the debug message provides enough context for
later troubleshooting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68809c3e-07f8-47c6-8347-bcc64521280b
📒 Files selected for processing (15)
drizzle/0089_curly_grey_gargoyle.sqldrizzle/0090_demonic_captain_universe.sqldrizzle/meta/0090_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/settings/config.jsonmessages/ja/settings/config.jsonmessages/ru/settings/config.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/settings/config.jsonsrc/actions/audit-logs.tssrc/actions/users.tssrc/drizzle/schema.tssrc/lib/audit/emit.tssrc/lib/ip-geo/client.test.tssrc/lib/ip-geo/client.ts
✅ Files skipped from review due to trivial changes (4)
- drizzle/0090_demonic_captain_universe.sql
- drizzle/meta/_journal.json
- messages/ja/settings/config.json
- messages/en/settings/config.json
🚧 Files skipped from review as they are similar to previous changes (5)
- messages/zh-CN/settings/config.json
- messages/ru/settings/config.json
- messages/zh-TW/settings/config.json
- src/lib/audit/emit.ts
- src/lib/ip-geo/client.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 530ea49e43
ℹ️ 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".
| // extra outer catch keeps the contract consistent even if that | ||
| // implementation changes later. | ||
| try { | ||
| const session = safeGetScopedAuthSession(); |
There was a problem hiding this comment.
emitActionAudit() only reads identity from getScopedAuthSession(), so any action executed through the direct Next.js server-action path (for example saveSystemSettings() invoked from system-settings-form.tsx) records operatorUserId/operatorUserName/operatorKey* as null. This causes successful admin mutations to appear unattributed in the new audit log, which undermines incident/audit traceability; add a fallback identity source (or pass the already-validated session into the emitter) when scoped context is absent.
Useful? React with 👍 / 👎.
| headers: z.array( | ||
| z.object({ | ||
| name: z.string(), | ||
| pick: z.any().optional(), |
There was a problem hiding this comment.
ipExtractionConfig.pick before persisting settings
The schema accepts pick as z.any(), so malformed values (e.g. typoed strings or wrong object shape) are saved as valid config. In extractClientIp, unknown pick modes are treated as invalid and the rule is skipped, which can silently degrade all requests to missing client IPs and weaken IP-based lockout/audit attribution until someone fixes the JSON manually. Tighten validation to the supported union (leftmost/rightmost/{kind:"index",index:number}) at save time.
Useful? React with 👍 / 👎.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
This was referenced Apr 17, 2026
Closed
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
This PR introduces three orthogonal capabilities delivered together:
Unified IP Extraction Middleware — Parses client IPs using a configurable rule chain supporting multiple header fallbacks (XFF leftmost/rightmost/index-based), covering reverse proxy / CDN / multi-hop scenarios.
Request Logging with IP Persistence + Geo Details — Adds
client_ipcolumns tomessage_requestandusage_ledger; request detail modal allows clicking IP to open geolocation popup (via/api/ip-geo/:ip).Audit Logging — New
audit_logtable, repository, and dashboard page (admin-only). Records every panel login (admin token & user key, success/failure) and every admin mutation (users/providers/groups/keys/settings/notifications/sensitive-words/model-prices), including operator, IP, User-Agent, before/after snapshots, success status, error reason, and timestamp. Sensitive fields (apiKey/secret/token/password/webhook_secret) are automatically redacted.Geolocation API defaults to the official hosted service
https://ip.api.claude-code-hub.app, with self-hosting options via env varsIP_GEO_API_URL/IP_GEO_API_TOKEN/IP_GEO_CACHE_TTL_SECONDS/IP_GEO_TIMEOUT_MS. Results are Redis-cached (default 1h), with short-TTL negative caching for failures and local short-circuit for private IPs.Related PRs
0089_curly_grey_gargoyle.sqlsyncs the database schema withprovider_groupstable,message_request.group_cost_multiplier,message_request.cost_breakdown, andusage_ledger.group_cost_multiplier. Also updatesfn_upsert_usage_ledgertrigger to propagateclient_ipandgroup_cost_multiplierfrommessage_requesttousage_ledger.Architecture
Configuration
System Settings (
/settings/configadmin page) adds "IP logging & extraction" section:ipGeoLookupEnabled— Enable/disable geolocation lookup (default true; when disabled,/api/ip-geo/:ipreturns 404).ipExtractionConfig— JSON rule chain. Empty uses built-in default:cf-connecting-ip → x-real-ip → x-forwarded-for (rightmost).{ "headers": [ { "name": "cf-connecting-ip" }, { "name": "x-real-ip" }, { "name": "x-forwarded-for", "pick": "rightmost" } ] }pickaccepts"leftmost"/"rightmost"/{"kind":"index","index":N}.Environment Variables:
IP_GEO_API_URL(default: https://ip.api.claude-code-hub.app)IP_GEO_API_TOKEN(optional Bearer token)IP_GEO_CACHE_TTL_SECONDS(60–86400, default 3600)IP_GEO_TIMEOUT_MS(100–10000, default 1500)Key Files Changed
src/drizzle/schema.ts,drizzle/0089_curly_grey_gargoyle.sql,src/lib/ledger-backfill/trigger.sqlsrc/lib/ip/*(+ 43 tests)src/lib/ip-geo/*(+ 8 tests),src/app/api/ip-geo/[ip]/route.tssrc/lib/audit/*,src/repository/audit-log.ts,src/lib/api/action-adapter-openapi.tssrc/app/api/auth/login/route.tssrc/actions/{users,providers,provider-groups,keys,system-config,notifications,sensitive-words,model-prices}.tssrc/app/[locale]/dashboard/_components/ip-details-dialog.tsx,src/app/[locale]/dashboard/audit-logs/*, usage-logs IP column, request-detail IP row, settings form IP sectionmessages/{en,zh-CN,zh-TW,ja,ru}/{ipDetails,auditLogs}.json,dashboard.json,settings/config.jsonPre-commit Results
bun run build✓bun run typecheck✓bun run lint✓ (20 pre-existing warnings, 0 errors)bun run test— 4468 pass, 1 pre-existing failure (tests/api/api-endpoints.test.tshealth check — unrelated; dev environment lacks DSN)Test Plan
/dashboard/logslist displays IP column; clicking IP opens geolocation popup (country + flag + ASN).IP_GEO_API_URLto unreachable endpoint) — popup shows "unavailable" fallback state without crashing./settings/configset custom extraction chain (e.g., index=2), curl withX-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3to/v1/messages, verify log records3.3.3.3.audit_logaddsauth.login.failurerow with operator IP.audit_logaddsauth.login.successrow with operator = "Admin Token".audit_logaddsprovider.updaterow with before/after showing[REDACTED]forapiKeyfield./dashboard/audit-logspage visible (admin-only); category/status filters work; IP clicks open geolocation popup; row clicks open before/after JSON details.message_requestrows) →usage_ledgerIP column remains intact.Non-goals / Out of Scope
Enhanced description with related PR linkage 🤖
Greptile Summary
This PR adds unified client-IP extraction, IP geolocation lookup, and a comprehensive audit log system covering login events and all admin mutations. The implementation is well-structured — prior review concerns (IP geo admin-gating,
flag.emojivalidation, floating-promise inemit.ts) have all been addressed. The one remaining style nit is that fourcreateAuditLogAsync(...)calls inlogin/route.tsare missing thevoidprefix used elsewhere to signal fire-and-forget intent.Confidence Score: 5/5
Safe to merge — all prior P0/P1 concerns have been resolved; only one P2 style nit remains.
All blocking concerns from previous review rounds (admin-only gating on IP geo endpoint, flag.emoji validation gap, floating promise in emit.ts) have been addressed. The remaining finding is a missing
voidon four fire-and-forget calls in the login route — a style inconsistency that doesn't affect runtime behavior. Migration, schema, and trigger logic are correct. Test coverage for new modules is adequate.src/app/api/auth/login/route.ts — four createAuditLogAsync calls missing void prefix.
Important Files Changed
isValidLookupResultnow correctly validatesflag.emojiafter the prior round-3 fix.void emitAsync(args).listAuditLogsandcountAuditLogs(comment warns about manual sync, butcountAuditLogsis currently unused).createAuditLogAsynccalls are missingvoid— fire-and-forget intent is clear from context but inconsistent withemit.tswhich was already fixed.Sequence Diagram
sequenceDiagram participant Client participant LoginRoute as /api/auth/login participant ProxyHandler as /v1/* Proxy participant AuditEmit as audit/emit.ts participant AuditRepo as repository/audit-log.ts participant AuditALS as audit/request-context.ts participant ActionAdapter as action-adapter-openapi.ts participant ServerAction as Server Actions Client->>LoginRoute: POST /api/auth/login LoginRoute->>LoginRoute: resolveClientIp(request) LoginRoute-->>AuditRepo: createAuditLogAsync (fire-and-forget) AuditRepo-->>AuditRepo: INSERT audit_log Client->>ProxyHandler: POST /v1/messages ProxyHandler->>ProxyHandler: getClientIp(headers) via auth-guard ProxyHandler->>ProxyHandler: session.clientIp = ip ProxyHandler->>AuditRepo: createMessageRequest(client_ip) Note over AuditRepo: DB trigger propagates client_ip to usage_ledger Client->>ActionAdapter: POST /api/actions/{module}/{action} ActionAdapter->>ActionAdapter: getClientIp(rawHeaders) ActionAdapter->>AuditALS: runWithRequestContext({ip, userAgent}) AuditALS->>ServerAction: action(...args) ServerAction->>AuditEmit: emitActionAudit({category, action, before, after}) AuditEmit->>AuditALS: resolveRequestContext() → ip, userAgent AuditEmit-->>AuditRepo: createAuditLogAsync (fire-and-forget) AuditRepo-->>AuditRepo: INSERT audit_log (redacted before/after)Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "fix: follow-up review comments (round 3)" | Re-trigger Greptile