Conversation
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where user rate limits were not being properly enforced, particularly for concurrent sessions and when limits were set to zero or null. It introduces robust checks for user-level concurrent session limits, refines existing key-level limit validations to handle null and zero values correctly, and enhances session tracking to support these new user-centric limits. The changes ensure that both key and user rate limits are applied consistently and accurately across the system, closing issue #634. Highlights
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
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthrough本次变更在速率限制与会话跟踪中引入用户级并发会话和 RPM 检查,调整代理层检查顺序,增加 key 限额对比用户限额的验证,并在多处将 session-tracker 的接口扩展为携带 userId,同时补充相应的错误消息与测试覆盖。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate 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 |
There was a problem hiding this comment.
Code Review
This pull request addresses the enforcement of user rate limits by adding checks to ensure that key limits do not exceed user limits. The changes include modifications to the addKey and editKey functions in src/actions/keys.ts to validate limit values, updates to src/app/v1/_lib/proxy/rate-limit-guard.ts to incorporate user-level concurrency limits, and enhancements to session tracking in src/lib/session-tracker.ts. Additionally, tests have been added and modified to ensure the correct enforcement of these limits.
| if ( | ||
| validatedData.limit5hUsd != null && | ||
| validatedData.limit5hUsd > 0 && | ||
| user.limit5hUsd != null && | ||
| user.limit5hUsd > 0 && | ||
| validatedData.limit5hUsd > user.limit5hUsd |
There was a problem hiding this comment.
The conditions for checking the 5-hour USD limit are quite verbose. Consider simplifying the condition by combining the null and greater-than-zero checks into a single expression for better readability and maintainability.
if (
validatedData.limit5hUsd > 0 &&
user.limit5hUsd > 0 &&
validatedData.limit5hUsd > user.limit5hUsd
) {| validatedData.limitConcurrentSessions != null && | ||
| validatedData.limitConcurrentSessions > 0 && | ||
| user.limitConcurrentSessions != null && | ||
| user.limitConcurrentSessions > 0 && | ||
| validatedData.limitConcurrentSessions > user.limitConcurrentSessions | ||
| ) { |
| : type === "provider" | ||
| ? await SessionTracker.getProviderSessionCount(id) | ||
| : await SessionTracker.getUserSessionCount(id); |
There was a problem hiding this comment.
| void SessionTracker.refreshSession(session.sessionId, key.id, provider.id, user.id).catch( | ||
| (error) => { | ||
| logger.error("[ResponseHandler] Failed to refresh session tracker:", error); | ||
| } |
| void SessionTracker.trackSession(sessionId, keyId, session.authState?.user?.id).catch( | ||
| (err) => { | ||
| logger.error("[ProxySessionGuard] Failed to track session:", err); | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/repository/_shared/transformers.ts`:
- Around line 27-43: The current transformer collapses undefined into null and
leaves limitConcurrentSessions unparsed; update the mapping for limit5hUsd,
limitWeeklyUsd, limitMonthlyUsd, limitTotalUsd and limitConcurrentSessions to
preserve undefined vs null and to parse numeric strings with a NaN fallback to
null: for each field on dbUser (e.g., dbUser.limit5hUsd, dbUser.limitWeeklyUsd,
dbUser.limitMonthlyUsd, dbUser.limitTotalUsd) return undefined if the property
is strictly === undefined, return null if === null, otherwise parseFloat(value)
and if Number.isNaN(result) return null; apply the same pattern for
limitConcurrentSessions but use Number.parseInt or Number(value) as appropriate
and NaN -> null so strings are not passed through raw.
🧹 Nitpick comments (2)
src/lib/rate-limit/service.ts (1)
971-985: checkRpmLimit 可考虑简化
type 已限定为 "user",当前 if/else 分支实际不可达;可去掉 type 参数或扩展为联合类型并显式处理非 user,减少死代码。src/repository/_shared/transformers.test.ts (1)
116-119: 修复类型断言问题静态分析工具指出
User类型与Record<string, unknown>之间的转换存在类型不兼容问题。建议先转换为unknown类型。♻️ 建议的修复
- ])("$title", ({ field, value, expected }) => { - const result = toUser({ ...baseDbUser, [field]: value }); - expect((result as Record<string, unknown>)[field]).toBe(expected); + ])("$title", ({ field, value, expected }) => { + const result = toUser({ ...baseDbUser, [field]: value }); + expect((result as unknown as Record<string, unknown>)[field]).toBe(expected); });
| }); | ||
|
|
||
| const syncUserProviderGroupFromKeysMock = vi.fn(async () => undefined); | ||
| vi.mock("../../src/actions/users", () => ({ |
There was a problem hiding this comment.
[HIGH] [TEST-BRITTLE] Mock path doesn't match imported module
Why this is a problem: This mock targets "../../src/actions/users", which resolves to tests/src/actions/users from this file and does not match the src/actions/users.ts module that @/actions/keys imports. The unit test can end up running the real syncUserProviderGroupFromKeys implementation.
Suggested fix:
vi.mock("@/actions/users", () => ({
syncUserProviderGroupFromKeys: syncUserProviderGroupFromKeysMock,
}));Evidence:
tests/unit/actions/keys-limit-validation.test.ts:44:vi.mock("../../src/actions/users", ...)CLAUDE.md:126:- **Path alias**:@/maps to./src/``
src/actions/keys.ts
Outdated
| return { | ||
| ok: false, | ||
| error: `Key的5小时消费上限(${data.limit5hUsd})不能超过用户限额(${user.limit5hUsd})`, | ||
| error: `Key的5小时消费上限(${validatedData.limit5hUsd})不能超过用户限额(${user.limit5hUsd})`, |
There was a problem hiding this comment.
[MEDIUM] [STANDARD-VIOLATION] User-facing error strings are hardcoded (missing i18n)
Why this is a problem: CLAUDE.md:14 requires: 3. **i18n Required** - All user-facing strings must use i18n (5 languages supported). Never hardcode display text. These addKey limit validation failures return hardcoded Chinese strings, so they cannot be localized via next-intl.
Suggested fix:
return {
ok: false,
error: tError("KEY_LIMIT_5H_EXCEEDS_USER_LIMIT", {
keyLimit: String(validatedData.limit5hUsd),
userLimit: String(user.limit5hUsd),
}),
};Add the new translation key(s) to all 5 locales under messages/*/errors.json, and apply the same pattern for daily/weekly/monthly/total/concurrent checks.
Evidence:
src/actions/keys.ts:194:error: \Key的5小时消费上限(${validatedData.limit5hUsd})不能超过用户限额(${user.limit5hUsd})`,`src/actions/keys.ts:207,src/actions/keys.ts:220,src/actions/keys.ts:233,src/actions/keys.ts:246,src/actions/keys.ts:259(same pattern)
Code Review SummaryThis PR adds user-level concurrent-session enforcement and propagates additional user limit fields into the auth/session pipeline. Two issues in the diff should be addressed before merge: a unit test mock path that likely doesn’t apply, and multiple user-facing error strings hardcoded without i18n. PR Size: L
Issues Found
Critical Issues (Must Fix)None. High Priority Issues (Should Fix)
Review Coverage
Automated review by Codex AI |
There was a problem hiding this comment.
Code Review Summary
This PR adds user-level concurrent-session enforcement and propagates additional user limit fields into the auth/session pipeline. Two issues in the diff should be addressed before merge: a unit test mock path that likely doesn’t apply, and multiple user-facing error strings hardcoded without i18n.
PR Size: L
- Lines changed: 657
- Files changed: 13
- Split suggestions:
- PR 1:
src/lib/session-tracker.ts+ session tracking callsites (src/app/v1/_lib/proxy/session-guard.ts,src/app/v1/_lib/proxy/response-handler.ts) - PR 2: rate-limit enforcement changes (
src/app/v1/_lib/proxy/rate-limit-guard.ts,src/lib/rate-limit/service.ts) - PR 3: key/user limit validation + data shaping + tests (
src/actions/keys.ts,src/repository/**/*,tests/unit/**/*)
- PR 1:
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 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 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
- [HIGH] [TEST-BRITTLE] (Confidence 85)
tests/unit/actions/keys-limit-validation.test.ts:44:vi.mock("../../src/actions/users", ...)doesn’t match the module imported by@/actions/keys, so the test can execute the realsyncUserProviderGroupFromKeys. - [MEDIUM] [STANDARD-VIOLATION] (Confidence 80)
src/actions/keys.ts:194(alsosrc/actions/keys.ts:207,src/actions/keys.ts:220,src/actions/keys.ts:233,src/actions/keys.ts:246,src/actions/keys.ts:259): hardcoded user-facing error strings violateCLAUDE.md:14i18n rule.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Identified PR
#643(“Fix user rate limit enforcement”), computed size L (607additions +50deletions,13files), and applied labelsize/L. - Posted 2 inline review comments (diff-only):
- [HIGH] [TEST-BRITTLE] wrong
vi.mocktarget intests/unit/actions/keys-limit-validation.test.ts:44(mock won’t match the imported module; fix suggested in comment). - [MEDIUM] [STANDARD-VIOLATION] hardcoded user-facing error strings in
src/actions/keys.ts:194(alsosrc/actions/keys.ts:207,src/actions/keys.ts:220,src/actions/keys.ts:233,src/actions/keys.ts:246,src/actions/keys.ts:259) violating i18n rule (fix pattern suggested in comment).
- [HIGH] [TEST-BRITTLE] wrong
- Submitted the required “Code Review Summary” as a PR review via REST (
POST /pulls/643/reviews) sincegh pr review(GraphQL) isn’t usable with this token in this environment.
| const parseOptionalNumber = (value: unknown): number | null | undefined => { | ||
| if (value === undefined) return undefined; | ||
| if (value === null) return null; | ||
| const parsed = Number.parseFloat(String(value)); | ||
| return Number.isNaN(parsed) ? null : parsed; |
There was a problem hiding this comment.
Inconsistent handling of 0 values - parseOptionalNumber preserves 0, but existing rpm and dailyQuota transforms convert 0 to null (lines 29-38). This inconsistency could cause issues since the validation logic in src/actions/keys.ts expects 0 to mean "unlimited" and uses > 0 checks to skip validation. If the database stores 0 but it gets converted to null, the validation will behave differently than intended.
| const parseOptionalNumber = (value: unknown): number | null | undefined => { | |
| if (value === undefined) return undefined; | |
| if (value === null) return null; | |
| const parsed = Number.parseFloat(String(value)); | |
| return Number.isNaN(parsed) ? null : parsed; | |
| const parseOptionalNumber = (value: unknown): number | null | undefined => { | |
| if (value === undefined) return undefined; | |
| if (value === null) return null; | |
| const parsed = Number.parseFloat(String(value)); | |
| if (Number.isNaN(parsed)) return null; | |
| return parsed > 0 ? parsed : null; // Convert 0 to null for consistency | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/_shared/transformers.ts
Line: 11:15
Comment:
Inconsistent handling of `0` values - `parseOptionalNumber` preserves `0`, but existing `rpm` and `dailyQuota` transforms convert `0` to `null` (lines 29-38). This inconsistency could cause issues since the validation logic in `src/actions/keys.ts` expects `0` to mean "unlimited" and uses `> 0` checks to skip validation. If the database stores `0` but it gets converted to `null`, the validation will behave differently than intended.
```suggestion
const parseOptionalNumber = (value: unknown): number | null | undefined => {
if (value === undefined) return undefined;
if (value === null) return null;
const parsed = Number.parseFloat(String(value));
if (Number.isNaN(parsed)) return null;
return parsed > 0 ? parsed : null; // Convert 0 to null for consistency
};
```
How can I resolve this? If you propose a fix, please make it concise.) Fixed TypeScript error TS2352 by adding intermediate 'unknown' cast when converting User type to Record<string, unknown> for dynamic field access in test assertions. Error: Conversion of type 'User' to type 'Record<string, unknown>' may be a mistake because neither type sufficiently overlaps with the other. Solution: Added intermediate cast to 'unknown' as suggested by TypeScript compiler. CI Run: https://github.com/ding113/claude-code-hub/actions/runs/21252279593 Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
| @@ -24,10 +38,11 @@ export function toUser(dbUser: any): User { | |||
| })(), | |||
| providerGroup: dbUser?.providerGroup ?? null, | |||
| tags: dbUser?.tags ?? [], | |||
| limitTotalUsd: | |||
| dbUser?.limitTotalUsd !== null && dbUser?.limitTotalUsd !== undefined | |||
| ? parseFloat(dbUser.limitTotalUsd) | |||
| : null, | |||
| limit5hUsd: parseOptionalNumber(dbUser?.limit5hUsd), | |||
| limitWeeklyUsd: parseOptionalNumber(dbUser?.limitWeeklyUsd), | |||
| limitMonthlyUsd: parseOptionalNumber(dbUser?.limitMonthlyUsd), | |||
| limitTotalUsd: parseOptionalNumber(dbUser?.limitTotalUsd), | |||
| limitConcurrentSessions: parseOptionalInteger(dbUser?.limitConcurrentSessions), | |||
There was a problem hiding this comment.
parseOptionalNumber preserves 0 but existing rpm/dailyQuota logic converts 0 to null (lines 29-38). This breaks the validation in src/actions/keys.ts which expects 0 to mean "unlimited" and uses > 0 checks. If the database stores 0 for limit5hUsd/limitWeeklyUsd/limitMonthlyUsd but these fields get returned as actual 0 instead of being converted to null like rpm/dailyQuota, validation will fail incorrectly.
| const parseOptionalNumber = (value: unknown): number | null | undefined => { | |
| if (value === undefined) return undefined; | |
| if (value === null) return null; | |
| const parsed = Number.parseFloat(String(value)); | |
| if (Number.isNaN(parsed)) return null; | |
| // Convert 0 to null for consistency with rpm/dailyQuota (0 = unlimited) | |
| return parsed > 0 ? parsed : null; | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/_shared/transformers.ts
Line: 11:45
Comment:
`parseOptionalNumber` preserves `0` but existing `rpm`/`dailyQuota` logic converts `0` to `null` (lines 29-38). This breaks the validation in `src/actions/keys.ts` which expects `0` to mean "unlimited" and uses `> 0` checks. If the database stores `0` for `limit5hUsd`/`limitWeeklyUsd`/`limitMonthlyUsd` but these fields get returned as actual `0` instead of being converted to `null` like `rpm`/`dailyQuota`, validation will fail incorrectly.
```suggestion
const parseOptionalNumber = (value: unknown): number | null | undefined => {
if (value === undefined) return undefined;
if (value === null) return null;
const parsed = Number.parseFloat(String(value));
if (Number.isNaN(parsed)) return null;
// Convert 0 to null for consistency with rpm/dailyQuota (0 = unlimited)
return parsed > 0 ? parsed : null;
};
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
Fixes critical rate limit enforcement bug where user-level limits were not properly validated, allowing keys to exceed user quotas.
Problem
Related Issues:
Root Cause:
The validation logic in
addKeyandeditKeyactions used incorrect null-checking conditions. The original code used truthy checks (if (data.limit && user.limit)) which treated0as falsy, causing validation to be skipped when either the key or user limit was set to0(unlimited). This allowed:00Related Work:
Solution
1. Fixed Validation Logic in Key Actions (src/actions/keys.ts)
Before:
After:
Key improvements:
!= nullchecks distinguish between0(unlimited) andnull(not set)> 0checks to skip validation when either limit is0(unlimited)limit5hUsd,limitDailyUsd,limitWeeklyUsd,limitMonthlyUsd,limitTotalUsd,limitConcurrentSessions2. Added User Concurrent Session Enforcement (src/app/v1/_lib/proxy/rate-limit-guard.ts)
Added missing user-level concurrent session check (step 4) in the rate limit pipeline:
Updated check order:
3. Enhanced Session Tracking (src/lib/session-tracker.ts)
userIdparameter totrackSession()andrefreshSession()user:${userId}:active_sessions(ZSET)getUserSessionCount()method for user-level concurrent session counting4. Extended Rate Limit Service (src/lib/rate-limit/service.ts)
checkSessionLimit()to support"user"type (previously only"key"and"provider")checkRpmLimit()wrapper method for consistent RPM checking5. Populated Missing User Limit Fields (src/repository/key.ts, src/repository/_shared/transformers.ts)
validateApiKeyAndGetUser()query:limit5hUsd,limitWeeklyUsd,limitMonthlyUsd,limitConcurrentSessions,dailyResetMode,dailyResetTimetoUser()transformer to properly parse numeric limit fields from database stringsChanges
Core Changes
src/actions/keys.ts(+67/-27): Fixed null-checking logic for all limit validations inaddKeyandeditKeysrc/app/v1/_lib/proxy/rate-limit-guard.ts(+47/-7): Added user concurrent session check, updated check ordersrc/lib/session-tracker.ts(+52/-3): Added user-level session tracking supportsrc/lib/rate-limit/service.ts(+23/-4): Extended session limit checking to support user typeSupporting Changes
src/repository/key.ts(+12/-0): Populated missing user limit fields in querysrc/repository/_shared/transformers.ts(+13/-0): Fixed user limit field parsingsrc/app/v1/_lib/proxy/response-handler.ts(+5/-3): Pass userId to session refreshsrc/app/v1/_lib/proxy/session-guard.ts(+5/-3): Pass userId to session trackingTests Added
tests/unit/actions/keys-limit-validation.test.ts(+153): Key limit validation scenariostests/unit/lib/rate-limit/service-extra.test.ts(+20): RPM limit checkingtests/unit/proxy/rate-limit-guard.test.ts(+25/-3): User concurrent session enforcementtests/unit/repository/key.test.ts(+138): User limit field populationsrc/repository/_shared/transformers.test.ts(+47): Limit field transformationBreaking Changes
✅ No breaking changes
Testing
Automated Tests
0(unlimited) vsnull(not set) edge casesTest Scenarios Verified
Scenario 1: Key limit exceeds user limit (should block)
Scenario 2: User limit is 0 (unlimited, should allow)
Scenario 3: User concurrent session enforcement
concurrent_sessionserrorChecklist
Description enhanced by Claude AI
Greptile Overview
Greptile Summary
Fixed critical rate limit enforcement bug where user-level limits were not validated due to incorrect null-checking (
if (data.limit && user.limit)treated0as falsy). Updated validation to use explicit!= nulland> 0checks, treating0as unlimited.Key Changes:
addKeyandeditKeyto properly handle0(unlimited) vsnull(not set)validateApiKeyAndGetUser()queryCritical Issue Found:
0handling in transformers: NewparseOptionalNumber()preserves0, but existingrpm/dailyQuotalogic converts0→null. This creates a mismatch where validation expects0= unlimited but transformers may return actual0values, breaking validation forlimit5hUsd,limitWeeklyUsd, andlimitMonthlyUsdfields.Confidence Score: 2/5
keys.ts, it introduces a new critical inconsistency intransformers.ts. The newparseOptionalNumber()function preserves0values, but the existingrpmanddailyQuotatransformation logic converts0tonull. This inconsistency means the validation inkeys.ts(which expects0= unlimited and uses> 0checks) will fail when the database has0stored for limit fields, as some will be returned as0(breaking validation) while others get converted tonull(working correctly). This undermines the entire fix.src/repository/_shared/transformers.ts- the inconsistent0handling must be fixed before mergeImportant Files Changed
0handling breaks validation -parseOptionalNumberpreserves0, butrpm/dailyQuotaconvert0tonull, causing mismatches with validation logic0(unlimited) vsnull(unset) with explicit!= nulland> 0checks across all limit fieldsgetUserSessionCount()method and user ZSET trackingSequence Diagram
sequenceDiagram participant Client participant SessionGuard participant RateLimitGuard participant SessionTracker participant RateLimitService participant KeyActions participant Transformers participant Redis Note over Client,Redis: Key Creation/Edit Flow Client->>KeyActions: addKey/editKey(limitConcurrentSessions=3) KeyActions->>Transformers: toUser(dbUser) Transformers-->>KeyActions: user with limitConcurrentSessions: 0 or null KeyActions->>KeyActions: Validate: key.limit vs user.limit Note over KeyActions: Check: limit != null AND limit > 0 AND<br/>user.limit != null AND user.limit > 0 AND<br/>key.limit > user.limit alt User limit is 0 (unlimited) KeyActions-->>Client: Allowed - 0 means unlimited else User limit > 0 and key exceeds it KeyActions-->>Client: Error: KEY_LIMIT_CONCURRENT_EXCEEDS_USER_LIMIT end Note over Client,Redis: Request Flow with Rate Limit Enforcement Client->>SessionGuard: Proxy Request SessionGuard->>SessionTracker: trackSession(sessionId, keyId, userId) SessionTracker->>Redis: ZADD active sessions for key SessionTracker->>Redis: ZADD active sessions for user SessionGuard->>RateLimitGuard: checkRateLimits() RateLimitGuard->>RateLimitService: checkSessionLimit(key.id, "key", limit) RateLimitService->>SessionTracker: getKeySessionCount(keyId) SessionTracker->>Redis: ZCOUNT key active sessions Redis-->>SessionTracker: count SessionTracker-->>RateLimitService: count alt Key concurrent exceeded RateLimitService-->>RateLimitGuard: not allowed RateLimitGuard-->>Client: Error: concurrent_sessions end RateLimitGuard->>RateLimitService: checkSessionLimit(user.id, "user", limit) RateLimitService->>SessionTracker: getUserSessionCount(userId) SessionTracker->>Redis: ZCOUNT user active sessions Redis-->>SessionTracker: count SessionTracker-->>RateLimitService: count alt User concurrent exceeded RateLimitService-->>RateLimitGuard: not allowed RateLimitGuard-->>Client: Error: concurrent_sessions end RateLimitGuard->>RateLimitService: checkRpmLimit(user.id, "user", rpm) alt RPM exceeded RateLimitService-->>RateLimitGuard: not allowed RateLimitGuard-->>Client: Error: rate_limit end RateLimitGuard-->>Client: Request allowed