feat(security): 加固并优化 API Key 与登录鉴权链路（Vacuum Filter → Redis → DB）#734
feat(security): 加固并优化 API Key 与登录鉴权链路（Vacuum Filter → Redis → DB）#734ding113 merged 15 commits intoding113:devfrom
Conversation
- 新增 VacuumFilter（AR + vacuuming + 插入失败回滚，支持删除）\n- 新增进程级 API Key 过滤器：仅 has==false 才短路，has==true 仍走 DB 校验\n- 启动预热 + 新 key 增量写入；插入失败自动禁用短路并后台重建\n- 添加单测覆盖（正确性/回滚/alt 可逆/短路避免 DB）\n\nEnv: ENABLE_API_KEY_VACUUM_FILTER=true
📝 WalkthroughWalkthrough新增进程内 VacuumFilter 与随机种子工具、ApiKeyVacuumFilter 单例与后台重载；新增基于 Redis 的 API Key 认证缓存与 pub/sub 频道；在仓库与认证流程中加入短路、缓存读写与失效发布；启动/关闭流程加入预热与跨实例同步；添加构建脚本与大量单元测试。 Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @tesgth032, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求引入了一项新的安全功能：API Key 真空过滤器。该功能旨在通过在数据库查询之前，利用概率数据结构快速判断 API Key 是否“肯定不存在”，从而有效减轻数据库负载并增强系统抵御 API Key 暴力破解攻击的能力。这显著提升了系统在面对高并发或攻击场景时的性能和安全性。 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
本次 PR 引入了 API Key 真空过滤器（Vacuum Filter），用于在查询数据库前快速过滤无效的 API Key，这是一个出色的安全性和性能优化，可以有效降低恶意扫描或爆破对数据库造成的压力。
整体实现质量非常高：
VacuumFilter数据结构的实现严谨，考虑了性能（ASCII 快速路径）和正确性（失败时回滚）。ApiKeyVacuumFilter管理器设计健壮，包含了单例模式、懒加载、后台自动重建和故障安全机制。- 在
repository/key.ts中的集成点选择恰当，实现了“负向短路”的核心目标。 - 配套的单元测试覆盖了核心算法、集成逻辑和各种边界情况，非常完善。
我只发现一个可以改进的地方：在 src/instrumentation.ts 中，新加入的过滤器预热逻辑加剧了生产环境和开发环境初始化代码块之间的重复。建议将这部分通用的初始化逻辑提取出来，以提升代码的可维护性。
总的来说，这是一次非常出色的代码贡献。
src/instrumentation.ts
Outdated
| // 预热 API Key Vacuum Filter（减少无效 key 对 DB 的压力） | ||
| apiKeyVacuumFilter.startBackgroundReload({ reason: "startup" }); |
There was a problem hiding this comment.
这行代码以及其周围的一些数据库初始化逻辑（如回填 provider vendors 和 endpoints）在开发环境的初始化代码块（313行附近）中也存在。为了提高代码的可维护性并避免重复，可以考虑将这些通用的、依赖数据库连接的初始化步骤提取到一个共享的辅助函数中。这样，未来对这部分逻辑的修改只需在一个地方进行。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/instrumentation.ts`:
- Around line 210-211: The call to apiKeyVacuumFilter.startBackgroundReload({
reason: "startup" }) is not protected by a try-catch and can throw
synchronously, stopping subsequent initialization; wrap this invocation in a
try-catch like other non-critical initializers (e.g.,
backfillProviderVendorsFromProviders) so any exception is logged via
processLogger.error (or existing logger) and does not abort startup, then
proceed without rethrowing to allow the rest of the init (price tables, error
rules, notification queues) to continue.
- Around line 313-314: The startup call to
apiKeyVacuumFilter.startBackgroundReload lacks try/catch protection (same issue
as prod path); wrap the call to apiKeyVacuumFilter.startBackgroundReload({
reason: "startup" }) in a try { ... } catch (err) { ... } block, log the error
with the existing logger (e.g., processLogger.error or logger.error) including
the error details and context ("apiKeyVacuumFilter startBackgroundReload startup
failed"), and avoid rethrowing so startup won't crash on failure.
In `@src/lib/security/api-key-vacuum-filter.ts`:
- Around line 122-128: The constructor currently enables the API key vacuum
filter by default because it checks process.env.ENABLE_API_KEY_VACUUM_FILTER !==
"false"; change the condition so the feature is opt-in (default off) by
requiring the env var to be exactly "true": keep the isEdgeRuntime check (typeof
process !== "undefined" && process.env.NEXT_RUNTIME === "edge") and set
this.enabled to false for edge runtimes or unless
process.env.ENABLE_API_KEY_VACUUM_FILTER === "true"; leave this.seed =
randomBytes(16) unchanged.
In `@src/lib/vacuum-filter/vacuum-filter.ts`:
- Around line 240-257: The switch in the MurmurHash3 tail handling (variables:
length, tail, k1) intentionally relies on fall-through but Biome flags
noFallthroughSwitchClause; add a Biome suppression comment directly above the
fall-through comment, e.g. add "/* biome-ignore noFallthroughSwitchClause */"
immediately before the existing fallthrough marker(s) (near the switch handling
in vacuum-filter.ts) so the intentional fall-through for cases 3→2→1 is
explicitly ignored by the linter while preserving the existing logic.
🧹 Nitpick comments (4)
tests/unit/security/api-key-vacuum-filter-build.test.ts (1)
4-16: 测试覆盖面较薄，建议补充边界场景。当前仅有一个测试用例。
buildVacuumFilterFromKeyStrings包含重试扩容逻辑（最多 6 次，每次maxItems *= 1.6）和异常抛出路径，建议至少补充：
- 空数组输入
keyStrings: []- 全空字符串
keyStrings: ["", ""]- 触发扩容重试的大量 key 场景（如
maxItems很小时能否自动扩容成功）这有助于达到 80% 以上的覆盖率目标。Based on learnings: "All new features must have unit test coverage of at least 80%"。
src/lib/security/api-key-vacuum-filter.ts (2)
4-18:randomBytes函数与vacuum-filter.ts（168-181 行）存在重复实现。两处功能完全一致，建议抽取到共享模块以避免维护上的不一致。
172-192:startBackgroundReload的并发与冷却保护逻辑合理。不过有一个边缘场景：当
noteExistingKey失败将vf置 null 并调用startBackgroundReload时，如果此时loadingPromise不为 null（上一次重建还在进行中），新的重建请求会被直接跳过（Line 174）。此时vf已被置 null，而进行中的旧重建可能很快完成并恢复vf，所以大多数情况下是安全的。但如果旧重建失败（被 catch 捕获后loadingPromise清空），而新请求又因冷却期reloadCooldownMs被拦截，则vf会在最多 10 秒内保持 null。这在安全性上是 ok 的（null 意味着不短路、全部走 DB），但在高负载下会导致短时间内所有请求都打到 DB，值得在文档或日志中明确说明。
src/lib/vacuum-filter/vacuum-filter.ts (1)
96-102:upperPower2对极大输入存在潜在无限循环风险。当
x超过2^53（Number.MAX_SAFE_INTEGER附近）时，ret *= 2会因浮点精度丢失而无法再增长，导致死循环。虽然实际 bucket count 不会达到这个量级，但建议添加防御性上界检查：建议
function upperPower2(x: number): number { if (x <= 1) return 1; let ret = 1; while (ret < x) ret *= 2; + if (!Number.isFinite(ret)) throw new Error("upperPower2 overflow"); return ret; }
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR introduces a Vacuum Filter (a variant of cuckoo filter) for negative short-circuiting API key lookups, avoiding unnecessary database queries for keys that definitely do not exist. The implementation is well-structured with careful attention to correctness invariants: no false negatives, safe degradation when the filter is unavailable, and proper rollback on insertion failure. The data structure implementation is solid with good test coverage.
PR Size: XL
- Lines changed: 1140 (1138 additions, 2 deletions)
- Files changed: 10
Split suggestion: This XL PR could be split into two smaller PRs for easier review:
- Core data structure:
src/lib/vacuum-filter/vacuum-filter.ts+tests/unit/vacuum-filter/vacuum-filter.test.ts+scripts/copy-version-to-standalone.js+package.jsonchange - Application integration:
src/lib/security/api-key-vacuum-filter.ts+src/repository/key.ts+src/instrumentation.ts+.env.example+ integration tests
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
No issues met the 80-confidence reporting threshold after validation. Key observations from the review:
- Error handling is well-designed: all catch blocks log errors, and filter unavailability degrades gracefully to direct DB access (no silent failures).
- Type safety is strong: the
VacuumFilterclass uses typed arrays (Uint32Array) and validates constructor parameters with NaN guards and defaults. - Correctness invariants are maintained: the
addIndexTagmethod performs full rollback on kick-out failure, preventing false negatives. ThealtIndexinvolution property is verified by tests. - Security design is sound: the filter is used only for negative short-circuiting (over-inclusion is safe), and the
isDefinitelyNotPresentmethod returns a three-state result (true/false/null) to distinguish "definitely absent" from "might exist" and "filter unavailable". - Test coverage is thorough: unit tests cover basic CRUD, high-load scenarios, rollback on failure, NaN edge cases, non-ASCII strings, and the altIndex involution invariant. Integration tests verify the short-circuit behavior end-to-end.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Claude AI
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const fs = require("node:fs"); | ||
| const path = require("node:path"); | ||
|
|
There was a problem hiding this comment.
Build script may crash
This script uses require(...) (CommonJS), but this repo may be running Node in ESM mode depending on package.json ("type": "module"). If type=module, node scripts/copy-version-to-standalone.js will throw at runtime. Please confirm module type; if ESM, convert this file to ESM (import fs from "node:fs") or rename to .cjs and update the build script accordingly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/copy-version-to-standalone.js
Line: 1:3
Comment:
**Build script may crash**
This script uses `require(...)` (CommonJS), but this repo may be running Node in ESM mode depending on `package.json` (`"type": "module"`). If `type=module`, `node scripts/copy-version-to-standalone.js` will throw at runtime. Please confirm module type; if ESM, convert this file to ESM (`import fs from "node:fs"`) or rename to `.cjs` and update the build script accordingly.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/instrumentation.ts
Line: 67:69
Comment:
**Duplicate warmup call**
`register()` now calls `apiKeyVacuumFilter.startBackgroundReload({ reason: "startup" })` in both the `autoMigrate` and `isConnected` branches, but those branches can both execute on startup (e.g., connected DB + migrations). This will schedule two reload attempts back-to-back and can cause unnecessary DB load/log noise. Consider ensuring the warmup happens only once per process (e.g., guard with a global flag here, or call it in a single common post-connect path).
How can I resolve this? If you propose a fix, please make it concise.
To fix: make Prompt To Fix With AIThis is a comment left during a code review.
Path: src/lib/vacuum-filter/vacuum-filter.ts
Line: 656:658
Comment:
**has/add not thread-safe**
`VacuumFilter` stores mutable scratch state (`scratch`, `hashOut`, `tmpIndex`, `tmpTag`) on the instance, and `has()` / `add()` / `delete()` all mutate that shared state. If the same `VacuumFilter` instance is accessed concurrently (e.g., via shared singleton in an async server handling overlapping requests), calls can race and corrupt each other’s intermediate values, producing incorrect `has()` results (including false negatives).
To fix: make `indexTag()` return `{ index, tag }` (locals) instead of writing to `this.tmp*`, and keep `hashOut`/`scratch` per-call (or use a small pool / `AsyncLocalStorage`), so each call is isolated.
How can I resolve this? If you propose a fix, please make it concise. |
| import { logger } from "@/lib/logger"; | ||
| import type { Key } from "@/types/key"; | ||
| import type { User } from "@/types/user"; |
There was a problem hiding this comment.
Edge runtime check inverted
isEdgeRuntime() currently returns true when process is defined (Node) and NEXT_RUNTIME !== "edge", which disables Redis caching in normal Node runtime and can enable it in environments where process is undefined (edge). This breaks the intended ENABLE_API_KEY_REDIS_CACHE behavior and can also make shouldUseRedisClient() touch process.env in edge.
Fix by checking for edge explicitly, e.g. return process.env.NEXT_RUNTIME === "edge"; (and guard typeof process === "undefined" accordingly).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 1:3
Comment:
**Edge runtime check inverted**
`isEdgeRuntime()` currently returns `true` when `process` is defined (Node) and `NEXT_RUNTIME !== "edge"`, which disables Redis caching in normal Node runtime and can enable it in environments where `process` is undefined (edge). This breaks the intended `ENABLE_API_KEY_REDIS_CACHE` behavior and can also make `shouldUseRedisClient()` touch `process.env` in edge.
Fix by checking for edge explicitly, e.g. `return process.env.NEXT_RUNTIME === "edge";` (and guard `typeof process === "undefined"` accordingly).
How can I resolve this? If you propose a fix, please make it concise.| limitTotalUsd: keys.limitTotalUsd, | ||
| limitConcurrentSessions: keys.limitConcurrentSessions, | ||
| providerGroup: keys.providerGroup, | ||
| cacheTtlPreference: keys.cacheTtlPreference, |
There was a problem hiding this comment.
New field may break insert
createKey() now includes cacheTtlPreference: keys.cacheTtlPreference in the .returning(...) projection (src/repository/key.ts:~170). If the underlying keys table/schema in the target branch doesn’t yet have this column, the insert will fail at runtime. This PR otherwise looks focused on Vacuum Filter/Redis caching, so please verify the migration/schema change for keys.cacheTtlPreference is part of this PR (or remove it from the projection).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 170:173
Comment:
**New field may break insert**
`createKey()` now includes `cacheTtlPreference: keys.cacheTtlPreference` in the `.returning(...)` projection (src/repository/key.ts:~170). If the underlying `keys` table/schema in the target branch doesn’t yet have this column, the insert will fail at runtime. This PR otherwise looks focused on Vacuum Filter/Redis caching, so please verify the migration/schema change for `keys.cacheTtlPreference` is part of this PR (or remove it from the projection).
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.env.example:
- Around line 66-69: The .env keys API_KEY_AUTH_CACHE_TTL_SECONDS and
ENABLE_API_KEY_REDIS_CACHE currently violate dotenv-linter rules: values lack
quotes and keys are unordered; fix by placing API_KEY_AUTH_CACHE_TTL_SECONDS
before ENABLE_API_KEY_REDIS_CACHE (alphabetical order) and wrap their values in
quotes (e.g., API_KEY_AUTH_CACHE_TTL_SECONDS="60" and
ENABLE_API_KEY_REDIS_CACHE="true") so both ValueWithoutQuotes and UnorderedKey
warnings are resolved.
In `@src/lib/security/api-key-auth-cache.ts`:
- Around line 140-147: The code is converting null to undefined for date fields
(expiresAt/deletedAt) using the pattern "expiresAt === undefined ? undefined :
expiresAt ?? undefined", which loses the semantic distinction where null means
"explicitly no expiration"; update the logic in hydrateUserFromCache and the Key
hydration in api-key-auth-cache.ts to preserve null by removing the "??
undefined" fallback and using an explicit undefined check (e.g., expiresAt ===
undefined ? undefined : expiresAt), and apply the same change for deletedAt and
any other parseOptionalDate usages so null remains null and only undefined stays
undefined.
In `@tests/unit/security/api-key-auth-cache.test.ts`:
- Around line 10-11: The vi.fn usage for getCachedActiveKey and getCachedUser
uses the old dual-generic form; update both to the Vitest 4.x single generic
function type form by replacing the current vi.fn<Parameters<...>,
Promise<...>>() signatures with vi.fn<(keyString: string) => Promise<Key |
null>>() and vi.fn<(userId: number) => Promise<User | null>>() respectively so
the mocks match the new Vitest generic convention and preserve the same
parameter/return types for getCachedActiveKey and getCachedUser.
🧹 Nitpick comments (3)
src/lib/security/api-key-auth-cache.ts (1)
70-76: API Key 缓存与ENABLE_RATE_LIMIT耦合
shouldUseRedisClient要求ENABLE_RATE_LIMIT === "true"才返回true。如果部署环境未启用限流但希望使用 API Key Redis 缓存，此条件会静默阻止缓存生效。虽然注释说明了与getRedisClient保持一致的意图，但建议在.env.example文档中明确说明此依赖关系，避免使用者困惑。src/repository/user.ts (1)
460-468:markUserExpired无论是否实际更新都会触发缓存失效
deleteUser（Line 450）在result.length > 0时才失效缓存，但markUserExpired在 Line 467 无条件调用invalidateCachedUser。当用户已被禁用（无匹配行）时会产生一次不必要的 Redis DEL 调用。建议保持一致：建议修改
+ if (result.length > 0) { invalidateCachedUser(userId).catch(() => {}); + } return result.length > 0;tests/unit/security/api-key-auth-cache.test.ts (1)
191-251: DB join 路径测试的 mock 结构较脆弱
dbSelect.mockReturnValueOnce({ from: () => ({ innerJoin: () => ({ where: async () => [joinRow] }) }) })深度链式 mock 与 Drizzle ORM 的实际调用链紧耦合。如果 Drizzle 内部链式调用顺序变化（如添加新方法），此 mock 将静默失败。这在集成测试中是可接受的折中，但需注意后续维护成本。
| function isEdgeRuntime(): boolean { | ||
| return typeof process !== "undefined" && process.env.NEXT_RUNTIME === "edge"; | ||
| } |
There was a problem hiding this comment.
Edge runtime detection breaks
isEdgeRuntime() currently returns true only when process is defined and NEXT_RUNTIME === "edge" (src/lib/security/api-key-auth-cache.ts:25-27). In actual Next.js Edge runtime, process is typically undefined, so this will evaluate to false and allow the Redis cache path to run (and shouldUseRedisClient() then reads process.env...), which will throw at runtime. Please guard for typeof process === "undefined" and treat that as edge (or otherwise make the edge check not rely on process).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 25:27
Comment:
**Edge runtime detection breaks**
`isEdgeRuntime()` currently returns `true` only when `process` is defined *and* `NEXT_RUNTIME === "edge"` (`src/lib/security/api-key-auth-cache.ts:25-27`). In actual Next.js Edge runtime, `process` is typically undefined, so this will evaluate to `false` and allow the Redis cache path to run (and `shouldUseRedisClient()` then reads `process.env...`), which will throw at runtime. Please guard for `typeof process === "undefined"` and treat that as edge (or otherwise make the edge check not rely on `process`).
How can I resolve this? If you propose a fix, please make it concise.| const isEdgeRuntime = | ||
| typeof process !== "undefined" && process.env.NEXT_RUNTIME === "edge"; | ||
| this.enabled = !isEdgeRuntime && process.env.ENABLE_API_KEY_VACUUM_FILTER !== "false"; | ||
| this.seed = randomBytes(16); |
There was a problem hiding this comment.
Edge runtime check unsafe
The isEdgeRuntime logic uses typeof process !== "undefined" && process.env.NEXT_RUNTIME === "edge" and then immediately reads process.env.ENABLE_API_KEY_VACUUM_FILTER (src/lib/security/api-key-vacuum-filter.ts:140-143). In Edge runtime where process is undefined, this will throw when evaluating process.env.... This needs a guard so no process.env access happens when process is missing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-vacuum-filter.ts
Line: 140:143
Comment:
**Edge runtime check unsafe**
The `isEdgeRuntime` logic uses `typeof process !== "undefined" && process.env.NEXT_RUNTIME === "edge"` and then immediately reads `process.env.ENABLE_API_KEY_VACUUM_FILTER` (`src/lib/security/api-key-vacuum-filter.ts:140-143`). In Edge runtime where `process` is undefined, this will throw when evaluating `process.env...`. This needs a guard so no `process.env` access happens when `process` is missing.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/instrumentation.ts
Line: 125:131
Comment:
**Instrumentation never runs**
`register()` is gated by `if (process.env.NEXT_RUNTIME === "nodejs")` (`src/instrumentation.ts:125-131`), but this file already sets `export const runtime = "nodejs";` and Next.js commonly sets `NEXT_RUNTIME` to `"nodejs" | "edge"` in route handlers, not necessarily for instrumentation. If `NEXT_RUNTIME` is unset, the entire instrumentation init (migrations, preload, schedulers) will be skipped in production and dev. Consider removing this gate or switching to a more reliable runtime detection.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/repository/key.ts
Line: 136:189
Comment:
**DB insert may fail**
`createKey()` now reads/returns `cacheTtlPreference: keys.cacheTtlPreference` in the `.returning(...)` projection (`src/repository/key.ts:136-189`). If the `keys` table schema/migration in `dev` doesn’t include `cache_ttl_preference` (or equivalent), this will fail at runtime even though the feature here is the vacuum filter/caching pipeline. Please ensure the schema change ships with this PR (or drop it from the projection).
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/security/api-key-vacuum-filter.ts`:
- Around line 170-202: noteExistingKey can write a new key into the old filter
while a background reload is in-flight, causing a transient false-negative when
the rebuild replaces vf; fix by, inside noteExistingKey after computing trimmed
and before/after calling vf.add(trimmed), check if this.loadingPromise is truthy
and then also add trimmed to this.pendingKeys (respecting this.pendingKeysLimit
and logging the overflow case like elsewhere) so keys observed during reload are
guaranteed to be included in the next rebuild; apply the same change to the
duplicate implementation in the 272-328 range and ensure you do not rely on
vf.has() but still call vf.add(trimmed) as before.
🧹 Nitpick comments (2)
src/repository/key.ts (2)
180-189:publishCacheInvalidation使用await可能延迟createKey响应。
publishCacheInvalidation内部已有 try-catch 和日志，但如果 Redis 连接缓慢或超时，await会阻塞创建 key 的返回。对于写入路径，可以考虑改为 fire-and-forget 以避免影响用户体验：建议修复
// 多实例：广播 key 集合变更，触发其它实例重建 Vacuum Filter，避免误拒绝 if (process.env.ENABLE_RATE_LIMIT === "true" && process.env.REDIS_URL) { - await publishCacheInvalidation(CHANNEL_API_KEYS_UPDATED); + publishCacheInvalidation(CHANNEL_API_KEYS_UPDATED).catch(() => {}); }
255-258:cacheActiveKey调用方式与createKey不一致。
createKey（Line 184）中cacheActiveKey为 fire-and-forget（不await），而updateKey中使用了await。建议保持一致——若缓存写入为 best-effort，两处均可不await：建议修复
- await cacheActiveKey(updated).catch(() => {}); + cacheActiveKey(updated).catch(() => {});
- ApiKeyVacuumFilter：reloading 时返回 null，避免 stale vf 误拒绝\n- VacuumFilter：MurmurHash tail 去掉 switch fallthrough\n- repo/key 与 instrumentation：按 Biome 规则整理 import\n- 测试：补齐 Redis 鉴权缓存 key/TTL/失效覆盖 + reloading 窗口单测\n- 杂项：清理无效 suppression 与多余依赖
|
|
||
| private lastReloadAttemptAt: number | null = null; | ||
| private readonly reloadCooldownMs = 10_000; |
There was a problem hiding this comment.
Edge runtime env access
constructor() computes isEdgeRuntime and then immediately reads process.env.ENABLE_API_KEY_VACUUM_FILTER. In actual Edge runtime process can be undefined, which will throw when evaluating process.env... even though you intended to disable the filter there. Make enabled derivation avoid any process.env access when typeof process === "undefined" (treat that as edge/disabled).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-vacuum-filter.ts
Line: 131:133
Comment:
**Edge runtime env access**
`constructor()` computes `isEdgeRuntime` and then immediately reads `process.env.ENABLE_API_KEY_VACUUM_FILTER`. In actual Edge runtime `process` can be `undefined`, which will throw when evaluating `process.env...` even though you intended to disable the filter there. Make `enabled` derivation avoid any `process.env` access when `typeof process === "undefined"` (treat that as edge/disabled).
How can I resolve this? If you propose a fix, please make it concise.| } catch (error) { | ||
| logger.debug( | ||
| { error: error instanceof Error ? error.message : String(error) }, | ||
| "[ApiKeyAuthCache] sha256 digest failed" | ||
| ); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Edge runtime process usage
shouldUseRedisClient() reads process.env... unconditionally. If this module is ever imported in an environment where process is missing (e.g. Next Edge), it will throw even though Redis caching should be disabled there. Align this with isEdgeRuntime() by guarding typeof process === "undefined" (or early-return false) before touching process.env.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 61:67
Comment:
**Edge runtime process usage**
`shouldUseRedisClient()` reads `process.env...` unconditionally. If this module is ever imported in an environment where `process` is missing (e.g. Next Edge), it will throw even though Redis caching should be disabled there. Align this with `isEdgeRuntime()` by guarding `typeof process === "undefined"` (or early-return false) before touching `process.env`.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/repository/key.ts
Line: 163:175
Comment:
**Potential build/runtime break**
`createKey()` now projects `cacheTtlPreference: keys.cacheTtlPreference` in `.returning(...)`. If the `keys` table/Drizzle schema on the PR’s base branch doesn’t include `cacheTtlPreference`, this insert will fail at runtime. Please ensure the corresponding schema/migration change is included in this PR/branch, or drop this field from the projection.
How can I resolve this? If you propose a fix, please make it concise. |
tesgth032
changed the title
| isEnabled: users.isEnabled, | ||
| expiresAt: users.expiresAt, | ||
| allowedClients: users.allowedClients, |
There was a problem hiding this comment.
User caching can bypass join semantics
In validateApiKeyAndGetUser, the Redis path treats getCachedUser(cachedKey.userId) as authoritative if deletedAt is null, but the DB join path also requires users.isEnabled = true and or(isNull(users.expiresAt), gt(users.expiresAt, now)). That means a disabled/expired user can still authenticate if their user cache entry is fresh.
Fix: apply the same isEnabled/expiresAt checks to the cached user before returning (and if invalid, invalidate the user cache and fall back to DB).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 530:532
Comment:
**User caching can bypass join semantics**
In `validateApiKeyAndGetUser`, the Redis path treats `getCachedUser(cachedKey.userId)` as authoritative if `deletedAt` is null, but the DB join path also requires `users.isEnabled = true` and `or(isNull(users.expiresAt), gt(users.expiresAt, now))`. That means a disabled/expired user can still authenticate if their user cache entry is fresh.
Fix: apply the same `isEnabled`/`expiresAt` checks to the cached user before returning (and if invalid, invalidate the user cache and fall back to DB).
How can I resolve this? If you propose a fix, please make it concise.| .update(keys) | ||
| .set({ deletedAt: new Date() }) | ||
| .where(and(eq(keys.id, id), isNull(keys.deletedAt))) |
There was a problem hiding this comment.
User cache not invalidated
deleteKey() only invalidates the key cache. If a user is deleted/disabled/expired, validateApiKeyAndGetUser can still return a cached user+key pair until TTL elapses because there’s no invalidation/broadcast on key mutations affecting the user side.
At minimum, when deleting a key you already have userId available in the returned row elsewhere; consider also invalidating user:${userId} (or ensure user mutations always clear it).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 419:421
Comment:
**User cache not invalidated**
`deleteKey()` only invalidates the key cache. If a user is deleted/disabled/expired, `validateApiKeyAndGetUser` can still return a cached user+key pair until TTL elapses because there’s no invalidation/broadcast on key mutations affecting the user side.
At minimum, when deleting a key you already have `userId` available in the returned row elsewhere; consider also invalidating `user:${userId}` (or ensure user mutations always clear it).
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| async function sha256Hex(value: string): Promise<string | null> { | ||
| const subtle = (globalThis as unknown as { crypto?: Crypto }).crypto?.subtle; | ||
| if (!subtle) return null; |
There was a problem hiding this comment.
Edge runtime env access
shouldUseRedisClient() reads process.env... unconditionally. In Next.js Edge runtime, process may be undefined, so importing this module can throw before isApiKeyRedisCacheEnabled() gates the code path.
Guard typeof process === "undefined" at the top of shouldUseRedisClient() (and/or avoid reading process.env anywhere when isEdgeRuntime() is true).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 52:56
Comment:
**Edge runtime env access**
`shouldUseRedisClient()` reads `process.env...` unconditionally. In Next.js Edge runtime, `process` may be undefined, so importing this module can throw before `isApiKeyRedisCacheEnabled()` gates the code path.
Guard `typeof process === "undefined"` at the top of `shouldUseRedisClient()` (and/or avoid reading `process.env` anywhere when `isEdgeRuntime()` is true).
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| if (okAll) { | ||
| return vf; | ||
| } | ||
|
|
There was a problem hiding this comment.
Edge runtime env access
The constructor computes isEdgeRuntime with a typeof process !== "undefined" guard, but then immediately evaluates process.env.ENABLE_API_KEY_VACUUM_FILTER regardless. If process is undefined (common in Edge), this will throw during module initialization.
Derive enabled without touching process.env when typeof process === "undefined" (treat that as edge/disabled).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-vacuum-filter.ts
Line: 86:90
Comment:
**Edge runtime env access**
The constructor computes `isEdgeRuntime` with a `typeof process !== "undefined"` guard, but then immediately evaluates `process.env.ENABLE_API_KEY_VACUUM_FILTER` regardless. If `process` is undefined (common in Edge), this will throw during module initialization.
Derive `enabled` without touching `process.env` when `typeof process === "undefined"` (treat that as edge/disabled).
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
This effect still reads Either put Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/user/forms/limit-rule-picker.tsx
Line: 106:111
Comment:
**Stale state due to deps**
This effect still reads `availableTypes` but removed it from the dependency list. If `availableTypes` changes while the dialog is open, the reset logic won’t run and the UI can show stale/invalid selections.
Either put `availableTypes` back in the deps or refactor so the effect no longer depends on it (e.g., compute a stable derived value inside the effect).
How can I resolve this? If you propose a fix, please make it concise. |
tesgth032
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/repository/key.ts`:
- Around line 493-553: The cache path in validateApiKeyAndGetUser must
defensively verify cachedKey and cachedUser active state like the DB path: after
obtaining cachedKey from getCachedActiveKey, check cachedKey.isEnabled and
cachedKey.expiresAt (treat expired or disabled as invalid), and if invalid call
invalidateCachedKey(keyString) and continue to the DB path; likewise after
getCachedUser(cachedKey.userId) verify cachedUser.isEnabled and
cachedUser.expiresAt/ deletedAt (treat deleted/disabled/expired as miss),
invalidate the user cache if invalid, and do not return the cached pair—allow
the existing DB lookup/join flow to run to produce a valid user/key result.
Ensure checks mirror the DB where users.deletedAt isNull and key/user
enabled/expiry semantics are enforced.
- Around line 430-448: The cached-path in findActiveKeyByKeyString returns the
Redis value without verifying it's still active; add defensive checks after
getCachedActiveKey returns to ensure cached.isEnabled === true and
cached.expiresAt (or equivalent expiry field) is in the future, and if either
check fails treat it as a cache-miss (optionally remove the stale cache via
deleteCachedActiveKey or invalidateCachedActiveKey) so the function proceeds to
DB lookup/handling; keep the existing apiKeyVacuumFilter.noteExistingKey
behavior when vfSaysMissing remains true.
- Around line 257-260: The updateKey flow unconditionally calls
cacheActiveKey(updated), which causes disabled or expired keys to remain cached
and bypass DB active checks used by findActiveKeyByKeyString and
validateApiKeyAndGetUser; change updateKey to only call cacheActiveKey(updated)
when updated.isEnabled is true and (updated.expiresAt is null or
updated.expiresAt > new Date()), otherwise call invalidateCachedKey(updated) so
non-active keys are removed from Redis cache instead of being written.
🧹 Nitpick comments (2)
src/repository/key.ts (1)
417-428:deleteKey未从 Vacuum Filter 中移除已删除的 key。Redis 缓存正确失效了（Line 425），但 Vacuum Filter 中仍保留该 key 的指纹。虽然不影响正确性（VF 只做"肯定不存在"的短路，已删除 key 通过 VF 后会被 DB 查询拒绝），但会浪费 VF 容量且无法对已删除 key 做负向短路。
如果 Vacuum Filter 支持
delete操作，可考虑调用以提升效率。tests/unit/security/api-key-auth-cache.test.ts (1)
125-289: 鉴权缓存链路的核心场景覆盖较全面。覆盖了 VF 误判+Redis 纠正、Redis 命中避免 DB、user 缓存 miss 回源、全量 DB join 回源并写入缓存等关键路径。
建议补充一个 VF 负向短路的测试用例：当
isDefinitelyNotPresent返回true且getCachedActiveKey返回null时，验证直接返回null且不访问 DB。此场景是 Vacuum Filter 的核心价值所在。参考用例
test("findActiveKeyByKeyString: VF 判定不存在且 Redis 未命中时应短路返回 null", async () => { isDefinitelyNotPresent.mockReturnValueOnce(true); getCachedActiveKey.mockResolvedValueOnce(null); const { findActiveKeyByKeyString } = await import("@/repository/key"); await expect(findActiveKeyByKeyString("sk-nonexistent")).resolves.toBeNull(); expect(dbSelect).not.toHaveBeenCalled(); });
| export async function findActiveKeyByKeyString(keyString: string): Promise<Key | null> { | ||
| const vfSaysMissing = apiKeyVacuumFilter.isDefinitelyNotPresent(keyString) === true; | ||
|
|
||
| // Redis 缓存命中：避免打 DB | ||
| const cached = await getCachedActiveKey(keyString); | ||
| if (cached) { | ||
| // 多实例一致性：若 Vacuum Filter 判定缺失但 Redis 命中，说明本机 filter 可能滞后。 | ||
| // 最佳努力将 key 写入本机 filter（不影响正确性，仅提升后续性能）。 | ||
| if (vfSaysMissing) { | ||
| apiKeyVacuumFilter.noteExistingKey(keyString); | ||
| } | ||
| return cached; | ||
| } | ||
|
|
||
| // Vacuum Filter 负向短路：肯定不存在则直接返回 null，避免打 DB | ||
| // 注意：此处必须放在 Redis 读取之后，避免多实例环境中新建 key 的短暂误拒绝窗口。 | ||
| if (vfSaysMissing) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
findActiveKeyByKeyString 缓存命中路径缺少活跃状态校验。
Line 435-441：Redis 返回的 cached 对象未经 isEnabled / expiresAt 验证即直接返回。配合上面 updateKey 的问题，可能返回已禁用或已过期的 key。
即使修复了 updateKey 的写入逻辑，也建议在缓存读取路径增加防御性校验，避免因 TTL 窗口内的数据不一致导致安全隐患。
防御性校验建议
const cached = await getCachedActiveKey(keyString);
if (cached) {
+ // 防御性校验：缓存可能包含已失效的 key
+ const isActive = cached.isEnabled
+ && (!cached.expiresAt || cached.expiresAt > new Date());
+ if (!isActive) {
+ invalidateCachedKey(keyString).catch(() => {});
+ return null;
+ }
if (vfSaysMissing) {
apiKeyVacuumFilter.noteExistingKey(keyString);
}
return cached;
}🤖 Prompt for AI Agents
In `@src/repository/key.ts` around lines 430 - 448, The cached-path in
findActiveKeyByKeyString returns the Redis value without verifying it's still
active; add defensive checks after getCachedActiveKey returns to ensure
cached.isEnabled === true and cached.expiresAt (or equivalent expiry field) is
in the future, and if either check fails treat it as a cache-miss (optionally
remove the stale cache via deleteCachedActiveKey or invalidateCachedActiveKey)
so the function proceeds to DB lookup/handling; keep the existing
apiKeyVacuumFilter.noteExistingKey behavior when vfSaysMissing remains true.
| const vfSaysMissing = apiKeyVacuumFilter.isDefinitelyNotPresent(keyString) === true; | ||
|
|
||
| // 默认鉴权链路：Vacuum Filter -> Redis -> DB | ||
| const cachedKey = await getCachedActiveKey(keyString); | ||
| if (cachedKey) { | ||
| // 多实例一致性：若 Vacuum Filter 判定缺失但 Redis 命中，说明本机 filter 可能滞后。 | ||
| // 最佳努力将 key 写入本机 filter（不影响正确性，仅提升后续性能）。 | ||
| if (vfSaysMissing) { | ||
| apiKeyVacuumFilter.noteExistingKey(keyString); | ||
| } | ||
|
|
||
| const cachedUser = await getCachedUser(cachedKey.userId); | ||
| if (cachedUser) { | ||
| return { user: cachedUser, key: cachedKey }; | ||
| } | ||
|
|
||
| // user 缓存 miss：仅补齐 user（相较 join 更轻量） | ||
| const [userRow] = await db | ||
| .select({ | ||
| id: users.id, | ||
| name: users.name, | ||
| description: users.description, | ||
| role: users.role, | ||
| rpm: users.rpmLimit, | ||
| dailyQuota: users.dailyLimitUsd, | ||
| providerGroup: users.providerGroup, | ||
| tags: users.tags, | ||
| createdAt: users.createdAt, | ||
| updatedAt: users.updatedAt, | ||
| deletedAt: users.deletedAt, | ||
| limit5hUsd: users.limit5hUsd, | ||
| limitWeeklyUsd: users.limitWeeklyUsd, | ||
| limitMonthlyUsd: users.limitMonthlyUsd, | ||
| limitTotalUsd: users.limitTotalUsd, | ||
| limitConcurrentSessions: users.limitConcurrentSessions, | ||
| dailyResetMode: users.dailyResetMode, | ||
| dailyResetTime: users.dailyResetTime, | ||
| isEnabled: users.isEnabled, | ||
| expiresAt: users.expiresAt, | ||
| allowedClients: users.allowedClients, | ||
| allowedModels: users.allowedModels, | ||
| }) | ||
| .from(users) | ||
| .where(and(eq(users.id, cachedKey.userId), isNull(users.deletedAt))); | ||
|
|
||
| if (!userRow) { | ||
| // join 语义：用户被删除则 key 无效；顺带清理 key 缓存避免重复 miss | ||
| invalidateCachedKey(keyString).catch(() => {}); | ||
| return null; | ||
| } | ||
|
|
||
| const user = toUser(userRow); | ||
| cacheUser(user).catch(() => {}); | ||
| return { user, key: cachedKey }; | ||
| } | ||
|
|
||
| // Vacuum Filter 负向短路：肯定不存在则直接返回 null，避免打 DB | ||
| // 注意：此处必须放在 Redis 读取之后，避免多实例环境中新建 key 的短暂误拒绝窗口。 | ||
| if (vfSaysMissing) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
validateApiKeyAndGetUser 缓存路径同样缺少活跃状态校验。
与 findActiveKeyByKeyString 相同的问题：Line 497-506 直接使用 cachedKey 而未校验 isEnabled / expiresAt。此外 Line 504 查询 user 后也未校验 user 的 isEnabled / expiresAt 状态（DB join 路径在 Line 609 有 isNull(users.deletedAt) 但缓存路径未做等价检查）。
建议对 cachedKey 和 cachedUser 都增加活跃状态的防御性校验，与 DB 路径的过滤条件保持一致。
🤖 Prompt for AI Agents
In `@src/repository/key.ts` around lines 493 - 553, The cache path in
validateApiKeyAndGetUser must defensively verify cachedKey and cachedUser active
state like the DB path: after obtaining cachedKey from getCachedActiveKey, check
cachedKey.isEnabled and cachedKey.expiresAt (treat expired or disabled as
invalid), and if invalid call invalidateCachedKey(keyString) and continue to the
DB path; likewise after getCachedUser(cachedKey.userId) verify
cachedUser.isEnabled and cachedUser.expiresAt/ deletedAt (treat
deleted/disabled/expired as miss), invalidate the user cache if invalid, and do
not return the cached pair—allow the existing DB lookup/join flow to run to
produce a valid user/key result. Ensure checks mirror the DB where
users.deletedAt isNull and key/user enabled/expiry semantics are enforced.
src/repository/key.ts
Outdated
| await cacheActiveKey(created).catch(() => {}); | ||
| // 多实例：广播 key 集合变更，触发其它实例重建 Vacuum Filter，避免误拒绝 | ||
| if (process.env.ENABLE_RATE_LIMIT === "true" && process.env.REDIS_URL) { | ||
| await publishCacheInvalidation(CHANNEL_API_KEYS_UPDATED); |
There was a problem hiding this comment.
await cacheActiveKey blocks createKey during Redis slowness
Line 186 awaits Redis write to reduce multi-instance 401 window. If Redis slow/down, key creation blocks. Comment says "尽量等待" (best-effort), but implementation waits. Consider fire-and-forget: void cacheActiveKey(created).catch(() => {})
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 186:189
Comment:
`await cacheActiveKey` blocks createKey during Redis slowness
Line 186 awaits Redis write to reduce multi-instance 401 window. If Redis slow/down, key creation blocks. Comment says "尽量等待" (best-effort), but implementation waits. Consider fire-and-forget: `void cacheActiveKey(created).catch(() => {})`
How can I resolve this? If you propose a fix, please make it concise.| const dsn = process.env.DSN || ""; | ||
| if ( | ||
| process.env.CI === "true" || | ||
| process.env.NODE_ENV === "test" || | ||
| process.env.VITEST === "true" || | ||
| !dsn || | ||
| dsn.includes("user:password@host:port") |
There was a problem hiding this comment.
CI detection may skip VF reload in non-test environments
Skips reload if CI=true or DSN contains placeholder. If staging sets CI=true for unrelated reasons, VF won't warm up. Consider explicit flag like SKIP_API_KEY_VF_WARMUP.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-vacuum-filter.ts
Line: 304:310
Comment:
CI detection may skip VF reload in non-test environments
Skips reload if `CI=true` or DSN contains placeholder. If staging sets `CI=true` for unrelated reasons, VF won't warm up. Consider explicit flag like `SKIP_API_KEY_VF_WARMUP`.
How can I resolve this? If you propose a fix, please make it concise.| const cachedUser = await getCachedUser(cachedKey.userId); | ||
| if (cachedUser) { | ||
| return { user: cachedUser, key: cachedKey }; |
There was a problem hiding this comment.
cached user bypasses isEnabled and expiresAt checks
DB join path (line 611-612) enforces users.isEnabled = true and expiry validation. Cached user only checks deletedAt. Disabled or expired user can authenticate via stale cache until TTL expires.
Add validation: check cachedUser.isEnabled and compare cachedUser.expiresAt with current time before returning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 511:513
Comment:
cached user bypasses `isEnabled` and `expiresAt` checks
DB join path (line 611-612) enforces `users.isEnabled = true` and expiry validation. Cached user only checks `deletedAt`. Disabled or expired user can authenticate via stale cache until TTL expires.
Add validation: check `cachedUser.isEnabled` and compare `cachedUser.expiresAt` with current time before returning.
How can I resolve this? If you propose a fix, please make it concise.| // validateApiKeyAndGetUser 的语义：user 仅要求“未删除” | ||
| if (hydrated.deletedAt) { | ||
| redis.del(redisKey).catch(() => {}); | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
getCachedUser validation insufficient for validateApiKeyAndGetUser
Only checks deletedAt. Auth path needs isEnabled and expiresAt validation (see key.ts:611-612). Mismatch enables disabled/expired user auth via cache.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 318:323
Comment:
`getCachedUser` validation insufficient for validateApiKeyAndGetUser
Only checks `deletedAt`. Auth path needs `isEnabled` and `expiresAt` validation (see key.ts:611-612). Mismatch enables disabled/expired user auth via cache.
How can I resolve this? If you propose a fix, please make it concise.
| .where(and(eq(users.id, cachedKey.userId), isNull(users.deletedAt))); | ||
|
|
||
| if (!userRow) { | ||
| // join 语义：用户被删除则 key 无效；顺带清理 key 缓存避免重复 miss |
There was a problem hiding this comment.
Cached user policy mismatch
validateApiKeyAndGetUser’s DB join path enforces users.isEnabled = true and users.expiresAt validity, but the “cachedKey -> cachedUser” branch returns the cached user as long as deletedAt is null. This means a disabled or expired user can continue authenticating until the user cache TTL elapses.
Fix: apply the same isEnabled/expiresAt checks to cachedUser before returning (and if invalid, invalidate user:${id} and fall back to DB).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 543:546
Comment:
**Cached user policy mismatch**
`validateApiKeyAndGetUser`’s DB join path enforces `users.isEnabled = true` and `users.expiresAt` validity, but the “cachedKey -> cachedUser” branch returns the cached user as long as `deletedAt` is null. This means a disabled or expired user can continue authenticating until the user cache TTL elapses.
Fix: apply the same `isEnabled`/`expiresAt` checks to `cachedUser` before returning (and if invalid, invalidate `user:${id}` and fall back to DB).
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Add Prompt To Fix With AIThis is a comment left during a code review.
Path: src/repository/key.ts
Line: 610:617
Comment:
**User checks missing in join**
The DB fallback query in `validateApiKeyAndGetUser` filters `users.deletedAt` but does not filter `users.isEnabled` / `users.expiresAt`, while the earlier `findActiveKeyByKeyString` path (and prior semantics) require enabled + not expired users. This can allow login/API auth for disabled/expired users whenever the DB path is hit.
Add `eq(users.isEnabled, true)` and `or(isNull(users.expiresAt), gt(users.expiresAt, new Date()))` to the `.where(...)` for the join query.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/repository/key.ts`:
- Around line 180-192: In createKey, protect the call to
apiKeyVacuumFilter.noteExistingKey(created.key) so VF failures don't abort
creation: since noteExistingKey may throw synchronously, wrap that call in a
try/catch (or otherwise catch thrown errors) and swallow or log the error to
preserve the existing "fail-open" semantics used for cacheActiveKey.catch(() =>
{}); ensure you reference apiKeyVacuumFilter.noteExistingKey and created.key
when making the change and keep behavior that a VF error does not prevent
returning created.
🧹 Nitpick comments (4)
src/lib/redis/client.ts (2)
56-68:baseOptions/ retry 策略在buildRedisOptionsForUrl和getRedisClient中重复定义。
buildRedisOptionsForUrl（Line 56-68）和getRedisClient（Line 100-113）包含完全相同的enableOfflineQueue、maxRetriesPerRequest和retryStrategy配置。如果日后需要调整重试策略，需要同步修改两处，容易遗漏。建议提取为共享常量或工厂函数：
提取公共 base options
+function buildBaseRedisOptions() { + return { + enableOfflineQueue: false, + maxRetriesPerRequest: 3, + retryStrategy(times: number) { + if (times > 5) { + logger.error("[Redis] Max retries reached, giving up"); + return null; + } + const delay = Math.min(times * 200, 2000); + logger.warn(`[Redis] Retry ${times}/5 after ${delay}ms`); + return delay; + }, + }; +}然后在
buildRedisOptionsForUrl和getRedisClient中都调用buildBaseRedisOptions()。Also applies to: 100-113
117-118:rejectUnauthorized计算冗余，仅用于日志。Line 117-118 计算了
rejectUnauthorized，但实际 TLS 配置由 Line 123 的buildTlsConfig(redisUrl)内部重新计算。此处的值仅在 Line 121 的日志中使用。虽然结果一致，但两处独立解析同一环境变量存在隐性耦合——如果buildTlsConfig的解析逻辑变更而此处未同步，日志与实际行为将不一致。建议让
buildTlsConfig返回解析后的rejectUnauthorized值供日志使用，或直接从返回的 config 对象中读取。tests/unit/security/api-key-auth-cache-redis-key.test.ts (1)
246-298: 循环内的子用例建议使用test.each改写以提升可读性与失败定位精度。当前
for...of循环遍历多个 case，若某个 case 失败，Vitest 报告只显示单条 test 失败，难以直接看出是哪个子 case（disabled/deleted/expired）出了问题。使用test.each或在循环内嵌套describe可使每个 case 独立报告。示例改写
- test("getCachedActiveKey：disabled/deleted/expired 应视为失效并清理", async () => { - vi.useFakeTimers(); - vi.setSystemTime(new Date("2026-01-10T00:00:00.000Z")); - - const { getCachedActiveKey } = await import("@/lib/security/api-key-auth-cache"); - - const cases: Array<{ - name: string; - payload: Record<string, unknown>; - }> = [ - ... - ]; - - for (const c of cases) { - ... - } - }); + describe("getCachedActiveKey：disabled/deleted/expired 应视为失效并清理", () => { + const cases = [ + { name: "disabled", payload: { isEnabled: false } }, + { name: "deleted", payload: { deletedAt: "2026-01-01T00:00:00.000Z" } }, + { name: "expired", payload: { expiresAt: "2026-01-01T00:00:00.000Z" } }, + ]; + + test.each(cases)("$name", async (c) => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-10T00:00:00.000Z")); + const { getCachedActiveKey } = await import("@/lib/security/api-key-auth-cache"); + // ... rest of test logic for single case + }); + });同样适用于 Lines 301-334 的
cacheActiveKey非活跃 key 测试。src/repository/key.ts (1)
186-191: 应使用getEnvConfig()中已验证的ENABLE_RATE_LIMIT配置，而非重复解析。环境变量已在
src/lib/config/env.schema.ts中通过 Zod schema 统一定义和验证，ENABLE_RATE_LIMIT使用booleanTransform转换为布尔值。建议改为调用getEnvConfig().ENABLE_RATE_LIMIT（已是布尔值），替代当前在key.ts、client.ts、api-key-auth-cache.ts和instrumentation.ts中重复的内联解析逻辑rateLimitRaw !== "false" && rateLimitRaw !== "0"。这样可以集中配置管理，避免多处维护同一解析规则。
| // validateApiKeyAndGetUser 的语义：user 仅要求“未删除” | ||
| if (hydrated.deletedAt) { | ||
| redis.del(redisKey).catch(() => {}); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
getCachedUser validation insufficient - only checks deletedAt but auth requires isEnabled and expiresAt checks (see key.ts:611-617).
Add validation or document that callers must validate. Current implementation allows disabled/expired users via cache.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 320:324
Comment:
`getCachedUser` validation insufficient - only checks `deletedAt` but auth requires `isEnabled` and `expiresAt` checks (see `key.ts:611-617`).
Add validation or document that callers must validate. Current implementation allows disabled/expired users via cache.
How can I resolve this? If you propose a fix, please make it concise.
src/repository/key.ts
Outdated
| // Redis 缓存（最佳努力，不影响正确性） | ||
| // 注意：多实例环境下其它实例可能在 Vacuum Filter 尚未重建时收到新 key 的请求。 | ||
| // 为减少“新 key 立刻使用偶发 401”的窗口，这里尽量等待 key 缓存写入完成（失败则降级）。 | ||
| await cacheActiveKey(created).catch(() => {}); |
There was a problem hiding this comment.
await cacheActiveKey blocks key creation during Redis slowness. Comment says "尽量等待" (best-effort) but implementation waits. Consider fire-and-forget:
| await cacheActiveKey(created).catch(() => {}); | |
| void cacheActiveKey(created).catch(() => {}); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 186:186
Comment:
`await cacheActiveKey` blocks key creation during Redis slowness. Comment says "尽量等待" (best-effort) but implementation waits. Consider fire-and-forget:
```suggestion
void cacheActiveKey(created).catch(() => {});
```
How can I resolve this? If you propose a fix, please make it concise.| .returning({ id: keys.id, key: keys.key }); | ||
|
|
||
| if (result.length > 0) { | ||
| await invalidateCachedKey(result[0].key).catch(() => {}); |
There was a problem hiding this comment.
deleteKey doesn't invalidate user cache. If key is deleted but user cache remains, validateApiKeyAndGetUser could return stale user data for other keys belonging to that user.
Consider invalidating user:${userId} when deleting keys, or ensure user mutations always clear related caches.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 433:433
Comment:
`deleteKey` doesn't invalidate user cache. If key is deleted but user cache remains, `validateApiKeyAndGetUser` could return stale user data for other keys belonging to that user.
Consider invalidating `user:${userId}` when deleting keys, or ensure user mutations always clear related caches.
How can I resolve this? If you propose a fix, please make it concise.| const dsn = process.env.DSN || ""; | ||
| if ( | ||
| process.env.CI === "true" || | ||
| process.env.NODE_ENV === "test" || | ||
| process.env.VITEST === "true" || | ||
| !dsn || | ||
| dsn.includes("user:password@host:port") | ||
| ) { |
There was a problem hiding this comment.
CI detection may skip VF reload in non-test environments. If staging/prod sets CI=true for unrelated reasons, VF won't warm up.
Consider explicit flag like SKIP_API_KEY_VF_WARMUP for clearer intent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-vacuum-filter.ts
Line: 304:311
Comment:
CI detection may skip VF reload in non-test environments. If staging/prod sets `CI=true` for unrelated reasons, VF won't warm up.
Consider explicit flag like `SKIP_API_KEY_VF_WARMUP` for clearer intent.
How can I resolve this? If you propose a fix, please make it concise.| const cachedUser = await getCachedUser(cachedKey.userId); | ||
| if (cachedUser) { | ||
| return { user: cachedUser, key: cachedKey }; |
There was a problem hiding this comment.
Cached user bypasses isEnabled and expiresAt validation, allowing disabled/expired users to authenticate via stale cache.
The DB join path (lines 611-617) enforces users.isEnabled = true and validates expiresAt, but the cached user path only checks deletedAt. Must apply same validation before returning cached user.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 512:514
Comment:
Cached user bypasses `isEnabled` and `expiresAt` validation, allowing disabled/expired users to authenticate via stale cache.
The DB join path (lines 611-617) enforces `users.isEnabled = true` and validates `expiresAt`, but the cached user path only checks `deletedAt`. Must apply same validation before returning cached user.
How can I resolve this? If you propose a fix, please make it concise.| function isEdgeRuntime(): boolean { | ||
| if (typeof process === "undefined") return true; | ||
| return process.env.NEXT_RUNTIME === "edge"; | ||
| } |
There was a problem hiding this comment.
isEdgeRuntime() detection correctly checks for undefined process but then isApiKeyRedisCacheEnabled (line 32) and shouldUseRedisClient (line 74) access process.env without guard.
Already fixed by guard on line 26, but ensure all callers of isEdgeRuntime() don't access process.env afterward.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 25:28
Comment:
`isEdgeRuntime()` detection correctly checks for undefined `process` but then `isApiKeyRedisCacheEnabled` (line 32) and `shouldUseRedisClient` (line 74) access `process.env` without guard.
Already fixed by guard on line 26, but ensure all callers of `isEdgeRuntime()` don't access `process.env` afterward.
How can I resolve this? If you propose a fix, please make it concise.| if (typeof process === "undefined") { | ||
| // Edge/浏览器等无 process 环境：强制关闭（避免访问 process.env 抛错） | ||
| this.enabled = false; | ||
| } else { | ||
| const isEdgeRuntime = process.env.NEXT_RUNTIME === "edge"; | ||
| const raw = process.env.ENABLE_API_KEY_VACUUM_FILTER?.trim(); | ||
| this.enabled = !isEdgeRuntime && raw !== "false" && raw !== "0"; | ||
| } |
There was a problem hiding this comment.
Edge runtime detection correctly guards process access on line 140, preventing edge runtime crashes mentioned in previous reviews.
Good fix - typeof process === "undefined" check ensures no process.env access when process is missing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-vacuum-filter.ts
Line: 140:147
Comment:
Edge runtime detection correctly guards `process` access on line 140, preventing edge runtime crashes mentioned in previous reviews.
Good fix - `typeof process === "undefined"` check ensures no `process.env` access when `process` is missing.
How can I resolve this? If you propose a fix, please make it concise.| if (vfSaysMissing) { | ||
| apiKeyVacuumFilter.noteExistingKey(keyString); | ||
| } |
There was a problem hiding this comment.
Multi-instance consistency: VF short-circuit after Redis read prevents false negatives when filter is stale but Redis has new key cache.
Good design - if VF says missing but Redis hits, backfills VF (line 447) for future performance while maintaining correctness.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 446:448
Comment:
Multi-instance consistency: VF short-circuit after Redis read prevents false negatives when filter is stale but Redis has new key cache.
Good design - if VF says missing but Redis hits, backfills VF (line 447) for future performance while maintaining correctness.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // 重建进行中：同时写入 pending，确保新 filter 不会漏包含该 key | ||
| if (this.loadingPromise) { | ||
| if (this.pendingKeys.size < this.pendingKeysLimit) { | ||
| this.pendingKeys.add(trimmed); | ||
| } else { | ||
| logger.warn("[ApiKeyVacuumFilter] Pending keys overflow; scheduling rebuild", { | ||
| limit: this.pendingKeysLimit, | ||
| }); | ||
| } | ||
|
|
||
| // 合并重建请求：当前重建结束后再跑一次，确保纳入 pendingKeys | ||
| this.startBackgroundReload({ reason: "pending_key_during_reload", force: true }); | ||
| } |
There was a problem hiding this comment.
Rebuild window safety: pending keys tracked during reload and merged into next build (lines 338-341) to prevent false negatives.
Good invariant protection - uses snapshot-swap pattern to avoid race conditions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-vacuum-filter.ts
Line: 199:211
Comment:
Rebuild window safety: pending keys tracked during reload and merged into next build (lines 338-341) to prevent false negatives.
Good invariant protection - uses snapshot-swap pattern to avoid race conditions.
How can I resolve this? If you propose a fix, please make it concise.| // 两个 bucket 都满：进入踢出 + vacuuming | ||
| // 关键语义：若最终插入失败，必须回滚所有修改，避免“丢元素”导致假阴性。 | ||
| const undo: UndoLog = { pos: [], prev: [] }; | ||
| let curIndex = this.rng.nextBool() ? i1 : i2; | ||
| let curTag = tag; | ||
|
|
||
| for (let count = 0; count < this.maxKickSteps; count++) { | ||
| // 1) 可能因上一次换位导致当前桶出现空位（保守再试一次） | ||
| if (this.insertTagToBucket(curIndex, curTag, undo)) { | ||
| this.numItems++; | ||
| return true; | ||
| } | ||
|
|
||
| // 2) Vacuuming（一跳前瞻）：尝试把当前桶内某个 tag 挪到它的 alternate bucket 的空位 | ||
| const start = this.bucketStart(curIndex); | ||
| for (let slot = 0; slot < BUCKET_SIZE; slot++) { | ||
| const existing = this.table[start + slot]; | ||
| if (existing === 0) continue; | ||
| const alt = this.altIndex(curIndex, existing); | ||
| if (this.insertTagToBucket(alt, existing, undo)) { | ||
| // 将空位“吸”到当前 slot：existing 移走，curTag 填入 | ||
| this.writeSlot(start + slot, curTag, undo); | ||
| this.numItems++; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // 3) 随机踢出一个 tag，继续链式搬运 | ||
| const r = this.rng.nextInt(BUCKET_SIZE); | ||
| const oldTag = this.table[start + r]; | ||
| this.writeSlot(start + r, curTag, undo); | ||
| curTag = oldTag; | ||
| curIndex = this.altIndex(curIndex, curTag); | ||
| } | ||
|
|
||
| this.rollback(undo); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Rollback on insert failure prevents false negatives by ensuring "no element is lost" during cuckoo eviction chains.
Well-designed safety mechanism - undo log (line 597) tracks all modifications and rolls back if max kick steps exceeded (line 630).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/vacuum-filter/vacuum-filter.ts
Line: 595:632
Comment:
Rollback on insert failure prevents false negatives by ensuring "no element is lost" during cuckoo eviction chains.
Well-designed safety mechanism - undo log (line 597) tracks all modifications and rolls back if max kick steps exceeded (line 630).
How can I resolve this? If you propose a fix, please make it concise.
tesgth032
changed the title
| // validateApiKeyAndGetUser 的语义：user 仅要求“未删除” | ||
| if (hydrated.deletedAt) { | ||
| redis.del(redisKey).catch(() => {}); | ||
| return null; | ||
| } | ||
|
|
||
| return hydrated; |
There was a problem hiding this comment.
getCachedUser only validates deletedAt, but callers in key.ts:522 need isEnabled and expiresAt validation to match DB semantics.
Either add validation here or document that callers must validate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 320:326
Comment:
`getCachedUser` only validates `deletedAt`, but callers in `key.ts:522` need `isEnabled` and `expiresAt` validation to match DB semantics.
Either add validation here or document that callers must validate.
How can I resolve this? If you propose a fix, please make it concise.| const [userRow] = await db | ||
| .select({ | ||
| id: users.id, | ||
| name: users.name, | ||
| description: users.description, |
There was a problem hiding this comment.
Cached user bypasses isEnabled and expiresAt validation that DB join enforces (lines 611-617), allowing disabled or expired users to authenticate via stale cache.
Add validation before returning cached user, and invalidate cache if checks fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 522:526
Comment:
Cached user bypasses `isEnabled` and `expiresAt` validation that DB join enforces (lines 611-617), allowing disabled or expired users to authenticate via stale cache.
Add validation before returning cached user, and invalidate cache if checks fail.
How can I resolve this? If you propose a fix, please make it concise.
|
已按 review 建议补齐：
|
| .update(keys) | ||
| .set({ deletedAt: new Date() }) | ||
| .where(and(eq(keys.id, id), isNull(keys.deletedAt))) | ||
| .returning({ id: keys.id }); | ||
| .returning({ id: keys.id, key: keys.key }); |
There was a problem hiding this comment.
User cache not invalidated
deleteKey() only invalidates the key cache. Any cached user entry for the same user can remain fresh and be reused by other keys for that user, so user-side auth state changes (disable/expire/delete) can persist in Redis until TTL. If you rely on user caching for auth, also invalidate the affected user cache here (return userId in .returning(...)) or ensure user mutations always clear the user cache.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 431:434
Comment:
**User cache not invalidated**
`deleteKey()` only invalidates the key cache. Any cached user entry for the same user can remain fresh and be reused by other keys for that user, so user-side auth state changes (disable/expire/delete) can persist in Redis until TTL. If you rely on user caching for auth, also invalidate the affected user cache here (return `userId` in `.returning(...)`) or ensure user mutations always clear the user cache.
How can I resolve this? If you propose a fix, please make it concise.| // 多实例一致性：若 Vacuum Filter 判定缺失但 Redis 命中，说明本机 filter 可能滞后。 | ||
| // 最佳努力将 key 写入本机 filter（不影响正确性，仅提升后续性能）。 | ||
| if (vfSaysMissing) { | ||
| apiKeyVacuumFilter.noteExistingKey(keyString); |
There was a problem hiding this comment.
Cached user bypasses checks
When the key cache hits, the function returns the cached user as long as it’s not deleted. The DB fallback path enforces isEnabled and expiry constraints, so a disabled/expired user can still authenticate until the user cache TTL expires. Add the same user-status checks on the cached-user path (and invalidate user cache on failure), or enforce those checks inside the user-cache hydration.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 510:513
Comment:
**Cached user bypasses checks**
When the key cache hits, the function returns the cached user as long as it’s not deleted. The DB fallback path enforces `isEnabled` and expiry constraints, so a disabled/expired user can still authenticate until the user cache TTL expires. Add the same user-status checks on the cached-user path (and invalidate user cache on failure), or enforce those checks inside the user-cache hydration.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/repository/key.ts
Line: 169:172
Comment:
**createKey blocks on Redis**
`createKey()` now `await`s `cacheActiveKey(created)` and then `await`s `publishCacheInvalidation(...)`. If Redis is slow/unavailable, key creation will be delayed even though the PR description says “best-effort / 不影响正确性”. Consider making these fire-and-forget (`void ...catch(() => {})`) or enforcing a short timeout so key creation latency isn’t coupled to Redis availability.
How can I resolve this? If you propose a fix, please make it concise. |
|
补充跟进 AI review 的一些合理建议（仅限本 PR 引入代码范围）：
全量验证： |
|
已跟进 Greptile 的合理建议：\n- createKey 的 Redis 写入(cacheActiveKey) + 广播(publishCacheInvalidation) 改为 best-effort 且设置 200ms 超时上限，避免 Redis 慢/不可用时拖慢 key 创建，同时尽量缩小新 key 跨实例偶发 401 窗口（见 src/repository/key.ts）。\n- PR 描述同步修正：vacuuming 为参考实现的一跳前瞻吸位（非全局 BFS）。\n\n本地验证：npm test / npm run typecheck / npm run build 通过。 |
| // 与 Redis client 的启用条件保持一致：未启用限流/未配置 Redis 时不尝试订阅，避免额外 warn 日志 | ||
| const rateLimitRaw = process.env.ENABLE_RATE_LIMIT?.trim(); | ||
| if (rateLimitRaw === "false" || rateLimitRaw === "0" || !process.env.REDIS_URL) { | ||
| return; |
There was a problem hiding this comment.
Missing VF enabled gate
startApiKeyVacuumFilterSync() returns early based on Redis/rate-limit envs, but it doesn’t check whether the Vacuum Filter is actually enabled. As written, setting ENABLE_RATE_LIMIT+REDIS_URL will subscribe and on every cch:cache:api_keys:updated message call apiKeyVacuumFilter.invalidateAndReload(...), which forces background reload attempts even when ENABLE_API_KEY_VACUUM_FILTER is off. This can trigger unexpected DB reads and log spam in deployments where VF is intentionally disabled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/instrumentation.ts
Line: 101:104
Comment:
**Missing VF enabled gate**
`startApiKeyVacuumFilterSync()` returns early based on Redis/rate-limit envs, but it doesn’t check whether the Vacuum Filter is actually enabled. As written, setting `ENABLE_RATE_LIMIT`+`REDIS_URL` will subscribe and on every `cch:cache:api_keys:updated` message call `apiKeyVacuumFilter.invalidateAndReload(...)`, which forces background reload attempts even when `ENABLE_API_KEY_VACUUM_FILTER` is off. This can trigger unexpected DB reads and log spam in deployments where VF is intentionally disabled.
How can I resolve this? If you propose a fix, please make it concise.| // 验证 API Key 并返回用户信息 | ||
| export async function validateApiKeyAndGetUser( | ||
| keyString: string | ||
| ): Promise<{ user: User; key: Key } | null> { | ||
| const vfSaysMissing = apiKeyVacuumFilter.isDefinitelyNotPresent(keyString) === true; | ||
|
|
||
| // 默认鉴权链路：Vacuum Filter -> Redis -> DB | ||
| const cachedKey = await getCachedActiveKey(keyString); | ||
| if (cachedKey) { | ||
| // 多实例一致性：若 Vacuum Filter 判定缺失但 Redis 命中，说明本机 filter 可能滞后。 | ||
| // 最佳努力将 key 写入本机 filter（不影响正确性，仅提升后续性能）。 | ||
| if (vfSaysMissing) { | ||
| apiKeyVacuumFilter.noteExistingKey(keyString); | ||
| } | ||
|
|
||
| const cachedUser = await getCachedUser(cachedKey.userId); | ||
| if (cachedUser) { | ||
| return { user: cachedUser, key: cachedKey }; | ||
| } | ||
|
|
||
| // user 缓存 miss：仅补齐 user（相较 join 更轻量） | ||
| const [userRow] = await db | ||
| .select({ | ||
| id: users.id, | ||
| name: users.name, | ||
| description: users.description, | ||
| role: users.role, | ||
| rpm: users.rpmLimit, | ||
| dailyQuota: users.dailyLimitUsd, | ||
| providerGroup: users.providerGroup, | ||
| tags: users.tags, | ||
| createdAt: users.createdAt, | ||
| updatedAt: users.updatedAt, | ||
| deletedAt: users.deletedAt, | ||
| limit5hUsd: users.limit5hUsd, | ||
| limitWeeklyUsd: users.limitWeeklyUsd, | ||
| limitMonthlyUsd: users.limitMonthlyUsd, | ||
| limitTotalUsd: users.limitTotalUsd, | ||
| limitConcurrentSessions: users.limitConcurrentSessions, | ||
| dailyResetMode: users.dailyResetMode, | ||
| dailyResetTime: users.dailyResetTime, | ||
| isEnabled: users.isEnabled, | ||
| expiresAt: users.expiresAt, | ||
| allowedClients: users.allowedClients, | ||
| allowedModels: users.allowedModels, | ||
| }) | ||
| .from(users) | ||
| .where(and(eq(users.id, cachedKey.userId), isNull(users.deletedAt))); | ||
|
|
||
| if (!userRow) { | ||
| // join 语义：用户被删除则 key 无效；顺带清理 key 缓存避免重复 miss | ||
| invalidateCachedKey(keyString).catch(() => {}); | ||
| return null; | ||
| } | ||
|
|
||
| const user = toUser(userRow); | ||
| cacheUser(user).catch(() => {}); | ||
| return { user, key: cachedKey }; | ||
| } |
There was a problem hiding this comment.
Cached-user semantics mismatch
In validateApiKeyAndGetUser(), the “cachedKey -> cachedUser” branch returns the cached user as long as deletedAt is null (key.ts:528-531), but the DB join path only enforces users.deletedAt IS NULL and does not enforce users.isEnabled / users.expiresAt. Callers like auth.ts and v1/_lib/proxy/auth-guard.ts do check those fields, but any other caller that relies on this function to fully validate auth could inadvertently allow disabled/expired users until TTL. If the intent is that this function is only ‘not deleted’, consider renaming or tightening semantics, or enforce user status checks consistently across both paths.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 513:571
Comment:
**Cached-user semantics mismatch**
In `validateApiKeyAndGetUser()`, the “cachedKey -> cachedUser” branch returns the cached user as long as `deletedAt` is null (`key.ts:528-531`), but the DB join path only enforces `users.deletedAt IS NULL` and does not enforce `users.isEnabled` / `users.expiresAt`. Callers like `auth.ts` and `v1/_lib/proxy/auth-guard.ts` do check those fields, but any other caller that relies on this function to fully validate auth could inadvertently allow disabled/expired users until TTL. If the intent is that this function is only ‘not deleted’, consider renaming or tightening semantics, or enforce user status checks consistently across both paths.
How can I resolve this? If you propose a fix, please make it concise.| export async function cacheAuthResult(keyString: string, value: { key: Key; user: User }): Promise<void> { | ||
| const redis = await getRedisForApiKeyAuthCache(); | ||
| if (!redis) return; | ||
|
|
||
| const { key, user } = value; | ||
| const keyHash = await sha256Hex(keyString); | ||
| if (!keyHash) return; | ||
| const keyRedisKey = REDIS_KEYS.keyByHash(keyHash); | ||
| const userRedisKey = REDIS_KEYS.userById(user.id); | ||
|
|
||
| const keyTtlSeconds = resolveKeyCacheTtlSeconds(key); | ||
| const userTtlSeconds = getCacheTtlSeconds(); | ||
|
|
||
| try { | ||
| const pipeline = redis.pipeline(); | ||
| if (keyTtlSeconds > 0 && key.isEnabled === true && !key.deletedAt) { | ||
| const keyPayload: CachedKeyPayloadV1 = { v: 1, key: stripKeySecret(key) }; | ||
| pipeline.setex(keyRedisKey, keyTtlSeconds, JSON.stringify(keyPayload)); | ||
| } else { | ||
| pipeline.del(keyRedisKey); | ||
| } | ||
|
|
||
| if (!user.deletedAt) { | ||
| const userPayload: CachedUserPayloadV1 = { v: 1, user }; | ||
| pipeline.setex(userRedisKey, userTtlSeconds, JSON.stringify(userPayload)); | ||
| } else { | ||
| pipeline.del(userRedisKey); | ||
| } | ||
|
|
There was a problem hiding this comment.
User cache TTL ignores expiresAt
cacheAuthResult() always caches the user for getCacheTtlSeconds() even if user.expiresAt is near/at expiration. This means an about-to-expire user can stay in Redis as “not deleted” for up to the full TTL, and whether that results in a real auth acceptance depends on the caller remembering to re-check expiresAt on every request. If you want the cache layer to be safe-by-default, the user TTL should be clamped to remaining expiresAt similarly to resolveKeyCacheTtlSeconds() for keys.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 367:395
Comment:
**User cache TTL ignores expiresAt**
`cacheAuthResult()` always caches the user for `getCacheTtlSeconds()` even if `user.expiresAt` is near/at expiration. This means an about-to-expire user can stay in Redis as “not deleted” for up to the full TTL, and whether that results in a real auth acceptance depends on the caller remembering to re-check `expiresAt` on every request. If you want the cache layer to be safe-by-default, the user TTL should be clamped to remaining `expiresAt` similarly to `resolveKeyCacheTtlSeconds()` for keys.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/repository/key.ts
Line: 441:450
Comment:
**User cache not invalidated**
`deleteKey()` only invalidates the key cache, but `validateApiKeyAndGetUser()` can return a cached user when the key cache hits. If user state changes (disable/expire/delete) need to take effect quickly, the user cache can remain stale until TTL unless it’s also invalidated/broadcast appropriately (e.g., on user mutations or key mutations where userId is known).
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/repository/key.ts`:
- Around line 462-464: The apiKeyVacuumFilter.noteExistingKey call is
unprotected in the hot auth paths (inside findActiveKeyByKeyString and
validateApiKeyAndGetUser) and can throw, so wrap each call to
apiKeyVacuumFilter.noteExistingKey in a try/catch (the same pattern already used
in createKey) to prevent VF exceptions from breaking authentication; catch and
log the error (use existing logger) and do not rethrow so the call remains
best-effort and does not affect auth correctness.
🧹 Nitpick comments (1)
src/repository/key.ts (1)
197-200: pubsub 广播的条件判断依赖ENABLE_RATE_LIMIT而非直接关联缓存/VF 功能开关，耦合度偏高。当前逻辑用
ENABLE_RATE_LIMIT作为"Redis 是否可用"的代理判断。如果未来有场景需要 Redis 缓存但不启用限流（或反之），此处逻辑会失配。考虑直接判断缓存/VF 相关的独立开关，或抽取一个isRedisEnabled()工具函数统一语义。这不阻塞合并，仅作为后续优化建议。
| if (vfSaysMissing) { | ||
| apiKeyVacuumFilter.noteExistingKey(keyString); | ||
| } |
There was a problem hiding this comment.
noteExistingKey 在鉴权热路径中缺少 try/catch 保护。
createKey（Line 182-186）已按之前的 review 建议用 try/catch 包裹 apiKeyVacuumFilter.noteExistingKey，但 findActiveKeyByKeyString（Line 463）和 validateApiKeyAndGetUser（Line 525）的同一调用仍然裸露。如果 VF 内部状态异常导致同步抛出，会中断鉴权流程并向调用方抛出 500 错误——而此处的语义是 best-effort，不应影响正确性。
建议统一加上 try/catch，与 createKey 保持一致。
修复建议
Line 462-464:
if (vfSaysMissing) {
- apiKeyVacuumFilter.noteExistingKey(keyString);
+ try { apiKeyVacuumFilter.noteExistingKey(keyString); } catch { /* ignore */ }
}Line 524-526:
if (vfSaysMissing) {
- apiKeyVacuumFilter.noteExistingKey(keyString);
+ try { apiKeyVacuumFilter.noteExistingKey(keyString); } catch { /* ignore */ }
}Also applies to: 524-526
🤖 Prompt for AI Agents
In `@src/repository/key.ts` around lines 462 - 464, The
apiKeyVacuumFilter.noteExistingKey call is unprotected in the hot auth paths
(inside findActiveKeyByKeyString and validateApiKeyAndGetUser) and can throw, so
wrap each call to apiKeyVacuumFilter.noteExistingKey in a try/catch (the same
pattern already used in createKey) to prevent VF exceptions from breaking
authentication; catch and log the error (use existing logger) and do not rethrow
so the call remains best-effort and does not affect auth correctness.
|
按升级体验要求调整：\n- API Key Vacuum Filter 改为默认启用（仅负向短路），无需用户新增 env 即可生效；如需排查/节省资源，可设 ENABLE_API_KEY_VACUUM_FILTER=false/0 显式关闭。\n- 同步更新 .env.example / README / 单测覆盖默认启用语义。\n\n本地验证：npm test / npm run typecheck / npm run build 通过。 |
| await Promise.race([ | ||
| Promise.all(redisTasks), | ||
| new Promise<void>((resolve) => setTimeout(resolve, redisBestEffortTimeoutMs)), | ||
| ]); |
There was a problem hiding this comment.
blocks key creation during Redis slowness despite "best-effort" comment
Comment says "尽量等待" (best-effort) to reduce multi-instance 401 window, but await Promise.race still blocks up to 200ms. If Redis is slow or hanging, key creation is delayed.
Consider fire-and-forget for true best-effort:
| await Promise.race([ | |
| Promise.all(redisTasks), | |
| new Promise<void>((resolve) => setTimeout(resolve, redisBestEffortTimeoutMs)), | |
| ]); | |
| if (redisTasks.length > 0) { | |
| Promise.race([ | |
| Promise.all(redisTasks), | |
| new Promise<void>((resolve) => setTimeout(resolve, redisBestEffortTimeoutMs)), | |
| ]).catch(() => {}); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 203:206
Comment:
blocks key creation during Redis slowness despite "best-effort" comment
Comment says "尽量等待" (best-effort) to reduce multi-instance 401 window, but `await Promise.race` still blocks up to 200ms. If Redis is slow or hanging, key creation is delayed.
Consider fire-and-forget for true best-effort:
```suggestion
if (redisTasks.length > 0) {
Promise.race([
Promise.all(redisTasks),
new Promise<void>((resolve) => setTimeout(resolve, redisBestEffortTimeoutMs)),
]).catch(() => {});
}
```
How can I resolve this? If you propose a fix, please make it concise.| async function startApiKeyVacuumFilterSync(): Promise<void> { | ||
| if (instrumentationState.__CCH_API_KEY_VF_SYNC_STARTED__) { | ||
| return; | ||
| } | ||
|
|
||
| // 与 Redis client 的启用条件保持一致：未启用限流/未配置 Redis 时不尝试订阅，避免额外 warn 日志 | ||
| const rateLimitRaw = process.env.ENABLE_RATE_LIMIT?.trim(); | ||
| if (rateLimitRaw === "false" || rateLimitRaw === "0" || !process.env.REDIS_URL) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const cleanup = await subscribeCacheInvalidation(CHANNEL_API_KEYS_UPDATED, () => { | ||
| apiKeyVacuumFilter.invalidateAndReload({ reason: "api_keys_updated" }); |
There was a problem hiding this comment.
subscribes and reloads VF even when VF is explicitly disabled
Currently checks rate limit and Redis config but not whether VF is enabled. If user sets the VF disable flag, subscription still fires and calls apiKeyVacuumFilter.invalidateAndReload on every broadcast, triggering unnecessary DB reads and log spam.
Check VF enabled status before subscribing:
| async function startApiKeyVacuumFilterSync(): Promise<void> { | |
| if (instrumentationState.__CCH_API_KEY_VF_SYNC_STARTED__) { | |
| return; | |
| } | |
| // 与 Redis client 的启用条件保持一致：未启用限流/未配置 Redis 时不尝试订阅，避免额外 warn 日志 | |
| const rateLimitRaw = process.env.ENABLE_RATE_LIMIT?.trim(); | |
| if (rateLimitRaw === "false" || rateLimitRaw === "0" || !process.env.REDIS_URL) { | |
| return; | |
| } | |
| try { | |
| const cleanup = await subscribeCacheInvalidation(CHANNEL_API_KEYS_UPDATED, () => { | |
| apiKeyVacuumFilter.invalidateAndReload({ reason: "api_keys_updated" }); | |
| async function startApiKeyVacuumFilterSync(): Promise<void> { | |
| if (instrumentationState.__CCH_API_KEY_VF_SYNC_STARTED__) { | |
| return; | |
| } | |
| // Check if VF is enabled (respect explicit disable) | |
| const vfRaw = process.env.ENABLE_API_KEY_VACUUM_FILTER?.trim(); | |
| if (vfRaw === "false" || vfRaw === "0") { | |
| return; | |
| } | |
| // 与 Redis client 的启用条件保持一致：未启用限流/未配置 Redis 时不尝试订阅，避免额外 warn 日志 | |
| const rateLimitRaw = process.env.ENABLE_RATE_LIMIT?.trim(); | |
| if (rateLimitRaw === "false" || rateLimitRaw === "0" || !process.env.REDIS_URL) { | |
| return; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/instrumentation.ts
Line: 96:109
Comment:
subscribes and reloads VF even when VF is explicitly disabled
Currently checks rate limit and Redis config but not whether VF is enabled. If user sets the VF disable flag, subscription still fires and calls `apiKeyVacuumFilter.invalidateAndReload` on every broadcast, triggering unnecessary DB reads and log spam.
Check VF enabled status before subscribing:
```suggestion
async function startApiKeyVacuumFilterSync(): Promise<void> {
if (instrumentationState.__CCH_API_KEY_VF_SYNC_STARTED__) {
return;
}
// Check if VF is enabled (respect explicit disable)
const vfRaw = process.env.ENABLE_API_KEY_VACUUM_FILTER?.trim();
if (vfRaw === "false" || vfRaw === "0") {
return;
}
// 与 Redis client 的启用条件保持一致：未启用限流/未配置 Redis 时不尝试订阅，避免额外 warn 日志
const rateLimitRaw = process.env.ENABLE_RATE_LIMIT?.trim();
if (rateLimitRaw === "false" || rateLimitRaw === "0" || !process.env.REDIS_URL) {
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.| const cachedUser = await getCachedUser(cachedKey.userId); | ||
| if (cachedUser) { | ||
| return { user: cachedUser, key: cachedKey }; |
There was a problem hiding this comment.
cached user bypasses isEnabled and expiresAt validation that DB path enforces
DB JOIN path (lines 627-634) enforces isEnabled and expiry checks in WHERE clause, but cached-user path only checks deletedAt. Callers like auth.ts do re-validate (lines 118-123), so this is safe but creates semantic mismatch: function returns users that may fail auth.
Consider either documenting that callers must validate status, or applying same validation here before returning cached user.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/key.ts
Line: 528:530
Comment:
cached user bypasses `isEnabled` and `expiresAt` validation that DB path enforces
DB JOIN path (lines 627-634) enforces `isEnabled` and expiry checks in WHERE clause, but cached-user path only checks `deletedAt`. Callers like `auth.ts` do re-validate (lines 118-123), so this is safe but creates semantic mismatch: function returns users that may fail auth.
Consider either documenting that callers must validate status, or applying same validation here before returning cached user.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| export async function cacheUser(user: User): Promise<void> { | ||
| const redis = await getRedisForApiKeyAuthCache(); | ||
| if (!redis) return; | ||
|
|
||
| if (user.deletedAt) return; | ||
|
|
||
| const ttlSeconds = getCacheTtlSeconds(); | ||
| const redisKey = REDIS_KEYS.userById(user.id); | ||
| const payload: CachedUserPayloadV1 = { v: 1, user }; | ||
| try { | ||
| await redis.setex(redisKey, ttlSeconds, JSON.stringify(payload)); | ||
| } catch (error) { | ||
| logger.debug( | ||
| { error: error instanceof Error ? error.message : String(error) }, | ||
| "[ApiKeyAuthCache] Write user cache failed" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
cacheUser ignores user expiry when setting TTL
cacheActiveKey clamps TTL to remaining expiresAt time (lines 188-198), but cacheUser always uses full getCacheTtlSeconds(). About-to-expire user can stay in Redis for up to 60s, relying on callers to re-check expiresAt on every request.
Clamp user TTL to remaining expiry time for consistency with key caching.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 336:353
Comment:
`cacheUser` ignores user expiry when setting TTL
`cacheActiveKey` clamps TTL to remaining `expiresAt` time (lines 188-198), but `cacheUser` always uses full `getCacheTtlSeconds()`. About-to-expire user can stay in Redis for up to 60s, relying on callers to re-check `expiresAt` on every request.
Clamp user TTL to remaining expiry time for consistency with key caching.
How can I resolve this? If you propose a fix, please make it concise.
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
背景
API Key 鉴权在爆破/无效 key 场景会产生大量 DB 压力；同时我们需要一个高装载率、查询/插入稳定、支持删除的动态过滤器，用于在进入 DB 前快速判定“肯定不存在”的 key。
方案（默认鉴权链路）
Vacuum Filter（仅负向短路） → Redis（key/user 鉴权缓存） → DB（权威校验）
安全语义：
has=false才能短路拒绝；has=true仍必须走 Redis/DB，避免假阳性误放行。覆盖范围（新增/扩大）
validateApiKeyAndGetUser/findActiveKeyByKeyString（代理层与 API key 校验）。validateKey复用同一条 VF→Redis→DB 链路，并补齐 user 禁用/过期校验，保护/api/auth/login与中间件鉴权热路径。关键实现点
API_KEY_AUTH_CACHE_TTL_SECONDS（默认 60，最大 3600），并按 key 的expiresAt剩余时间进一步收敛。validateApiKeyAndGetUser仅保证 user 未删除；user 禁用/过期由调用方统一校验（如auth.ts/ v1 proxy auth-guard / models endpoint）。cch:cache:api_keys:updated触发各实例 VF 失效并重建。createKeybest-effort 等待 key 缓存写入与广播（最多 200ms），缩小“刚创建就被路由到其它实例”的偶发 401 窗口，同时避免 Redis 慢导致创建阻塞。pendingKeys使用 Set 交换 + build 失败回滚，消除“快照-合并-清理”的竞态窗口。兼容性 / 升级体验
true/false与1/0（并兼容.env中引号写法）。bun run build在缺少node时自动回退使用bun执行 VERSION 拷贝脚本。升级注意事项（重要）
.env里显式设置过ENABLE_API_KEY_VACUUM_FILTER="false"（例如从旧版.env.example复制），升级后会继续保持关闭；如需启用请改为true/1或直接删除该行以使用默认值。ENABLE_RATE_LIMIT=true+ 配置REDIS_URL），以启用 “Redis 鉴权缓存 + Pub/Sub 广播触发各实例重建 Vacuum Filter”。否则在多实例但未启用 Redis 的情况下，新建 API key 可能在其它实例被 Vacuum Filter 短路拒绝（直到该实例重建/重启）。此场景请显式关闭ENABLE_API_KEY_VACUUM_FILTER=false/0或确保所有实例同步重启。DSN未配置（或仍是占位符），Vacuum Filter 无法从 DB 预热，会处于未就绪状态并自动不短路（不影响正确性，只是优化不生效）。ENABLE_API_KEY_VACUUM_FILTER=false/0关闭 Vacuum Filter（其余链路保持 Redis → DB）。配置项
ENABLE_API_KEY_VACUUM_FILTER：默认 true（可设为 false/0 显式关闭）；Edge runtime 自动关闭。.env.example默认也设置为ENABLE_API_KEY_VACUUM_FILTER="true"，升级后无需额外改动即可自动启用。ENABLE_API_KEY_REDIS_CACHE：默认 true；需 Redis 可用才会实际启用缓存，否则回落 DB。API_KEY_AUTH_CACHE_TTL_SECONDS：默认 60，最大 3600。测试
npm test/npm run typecheck/npm run build。