Conversation
…ss conversations generateDeterministicSessionId() hashes (UA, IP, API key prefix) with no time dimension, producing identical session IDs for the same user hours apart. This merges unrelated conversations into one session, polluting usage logs, session tracking, and concurrent session limits. The existing fallback in getOrCreateSessionId() (content hash -> random ID) already provides correct session continuity without collision risk. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
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 a critical issue where deterministic session IDs were causing collisions and data inaccuracies across user conversations. By completely removing the 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
|
📝 Walkthrough演练此拉取请求移除了确定性会话 ID 生成逻辑。该更改删除了 更改
估计代码审查工作量🎯 2 (简单) | ⏱️ ~10 分钟 🚥 Pre-merge checks | ✅ 4✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully addresses the session collision issue by removing the generateDeterministicSessionId method and its usage as a fallback in ProxySessionGuard. While this resolves the original problem by relying on the message content hash or random ID generation via SessionManager.getOrCreateSessionId for unique session identifiers, the new implementation introduces a critical security vulnerability. It relies on a flawed fallback mechanism and unvalidated client-provided session IDs, which could lead to session hijacking and cross-user session collisions. Specifically, the content-based hash fallback is global and does not account for the authenticated user or API key, and user-provided session IDs are trusted without verification. To mitigate these risks, session identifiers should be namespaced with the API key ID.
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
![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
No significant issues identified in this PR.
PR Size: S
- Lines changed: 62
- Files changed: 4
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 Codex AI
There was a problem hiding this comment.
Submitted PR review for #793 and applied label size/S.
Code Review Summary
No significant issues identified in this PR.
PR Size: S
- Lines changed: 62
- Files changed: 4
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 Codex AI
* fix(proxy): extract model from Gemini Vertex AI publishers path for correct billing
When Gemini requests use the Vertex AI URL format
/v1/publishers/google/models/{model}:generateContent, the system
failed to extract the model name, falling back to a hardcoded
"gemini-2.5-flash" default and causing incorrect billing.
Add publishers path regex to extractModelFromPath() and
detectFormatByEndpoint() to handle this URL pattern.
* fix(proxy): correct Host header to match actual request target in standard path
buildHeaders() derives Host from provider.url, but the actual fetch target
(proxyUrl) may use a different host when activeEndpoint.baseUrl differs or
MCP passthrough overrides the base URL. This causes undici TLS certificate
validation failures. After proxyUrl is computed, re-derive Host from it.
* perf(logs): hide stats summary panel when no filters are active
Skip rendering UsageLogsStatsPanel and its aggregation query when all
filter conditions are empty, preventing full-table scans that cause
CPU overload.
* fix(proxy): remove deterministic session ID to prevent collision across conversations (#793)
generateDeterministicSessionId() hashes (UA, IP, API key prefix) with no time
dimension, producing identical session IDs for the same user hours apart. This
merges unrelated conversations into one session, polluting usage logs, session
tracking, and concurrent session limits.
The existing fallback in getOrCreateSessionId() (content hash -> random ID)
already provides correct session continuity without collision risk.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
* perf(logs): hide stats panel in virtualized view when no filters active
Apply the same hasStatsFilters guard from the old view to the
virtualized logs view, preventing an unconditional full-table
aggregation query on page load. Also remove the unused legacy
usage-logs-view.tsx which is no longer imported anywhere.
* fix(my-usage): UX improvements for quota and statistics cards (#794)
* style(my-usage): use Badge for provider group values
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(my-usage): use currency symbol instead of code in quota cards
Replace manual `${currency} ${num.toFixed(2)}` formatting with
`formatCurrency()` so quota values display "$3.50" instead of "USD 3.50",
consistent with all other currency displays in the app.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* style(my-usage): replace unlimited text with infinity icon in quota cards
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(my-usage): paginate model breakdown in statistics summary card
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore(my-usage): suppress biome exhaustive-deps for intentional stats reset
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(my-usage): address PR #794 review comments
- Fix abbreviateModel/abbreviateClient crash on empty split parts
- Fix pagination reset on auto-refresh by using dateRange deps
- Restore noData fallback in model breakdown columns
- Add i18n for pagination controls with aria-labels (5 langs)
- Fix quota label overflow for long translations (w-8 -> w-auto)
- Rename Infinity -> InfinityIcon to avoid shadowing global
- Remove redundant span wrappers in TooltipTrigger asChild
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: John Doe <johndoe@example.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: miraserver <20286838+miraserver@users.noreply.github.com>
Co-authored-by: John Doe <johndoe@example.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Problem
generateDeterministicSessionId()hashes(User-Agent, IP, API Key prefix)with no time dimension, producing identical session IDs for the same user across separate conversations hours apart.Impact
Root Cause
session-guard.tsfalls back togenerateDeterministicSessionId()whenextractClientSessionId()returns null. Since the hash inputs are static per-user, the same ID is generated every time.Fix
Remove
generateDeterministicSessionId()entirely. The existing fallback ingetOrCreateSessionId()already handlesclientSessionId = nullcorrectly:generateSessionId()- ensures unique IDs for new conversationsChanges
session-guard.ts: Remove|| session.generateDeterministicSessionId()fallbacksession.ts: DeletegenerateDeterministicSessionId()method + unusedcryptoimportGreptile Summary
This PR removes the deterministic session ID generation mechanism that was causing session ID collisions across unrelated conversations. The
generateDeterministicSessionId()method hashed static per-user attributes (User-Agent, IP, API Key prefix) without any time dimension, producing identical session IDs for the same user across separate conversations.Key changes:
generateDeterministicSessionId()insession-guard.ts:88-92generateDeterministicSessionId()method fromsession.ts:352-384cryptoimport fromsession.ts:1Impact: The existing fallback logic in
SessionManager.getOrCreateSessionId()already handles null client session IDs correctly by using content hash (for multi-turn conversations) or random session ID generation (for new conversations). This fix resolves issues with usage log merging, inflated session durations, ZSET entry revival, and concurrent session limit bypasses.Confidence Score: 5/5
SessionManager.getOrCreateSessionId()already handles null client session IDs through content hash or random ID generation. All code paths are properly handled, tests are cleaned up, and the fix directly addresses the reported collision issues.Important Files Changed
generateDeterministicSessionId(), now passes null directly togetOrCreateSessionId()generateDeterministicSessionId()method and unusedcryptoimportgenerateDeterministicSessionId()methodgenerateDeterministicSessionId()from test fixturesFlowchart
flowchart TD Start[Request arrives at ProxySessionGuard] --> Extract[Extract client session ID<br/>from metadata/headers] Extract --> HasClient{Client session ID<br/>exists?} HasClient -->|Yes| UseClient[Use client-provided<br/>session ID] HasClient -->|No| ContentHash[Calculate content hash<br/>from first 3 messages] ContentHash --> HashExists{Hash calculated<br/>successfully?} HashExists -->|Yes| LookupHash[Lookup existing session<br/>in Redis by hash] HashExists -->|No| GenNew1[Generate new random<br/>session ID] LookupHash --> Found{Session found<br/>in Redis?} Found -->|Yes| Reuse[Reuse existing<br/>session ID] Found -->|No| GenNew2[Generate new random<br/>session ID] UseClient --> End[Session ID assigned] Reuse --> End GenNew1 --> End GenNew2 --> End style Start fill:#e1f5ff style End fill:#d4edda style Extract fill:#fff3cd style ContentHash fill:#fff3cd style GenNew1 fill:#f8d7da style GenNew2 fill:#f8d7daLast reviewed commit: 94a409b