feat(codex): implement session isolation for concurrent Codex requests (#430)#434
feat(codex): implement session isolation for concurrent Codex requests (#430)#434
Conversation
#430) Enable distinct session tracking for multiple concurrent Codex CLI sessions from the same user by extracting session_id from request headers/body. Changes: - Add session-extractor.ts with priority-based session ID extraction - Extend SessionManager.extractClientSessionId() to accept headers/userAgent - Pass headers/userAgent from ProxySessionGuard to session extraction - Preserve original session_id in Codex-to-Claude request transformation Security hardening: - Session ID length validation (21-256 chars) - Character whitelist validation (alphanumeric, dash, dot, colon) Backward compatible: Claude/OpenAI format requests unaffected. Closes #430 Generated with [Claude Code](https://claude.ai/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.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 significantly enhances the handling of Codex CLI requests by introducing robust session isolation. It achieves this through a new, dedicated session ID extraction utility that prioritizes various request fields and enforces strict security validations on the extracted IDs. This ensures that each concurrent request from a Codex client is correctly associated with its unique session, improving the reliability and security of the service. 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
|
github-actions
bot
added
enhancement
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces session isolation for concurrent Codex CLI requests by implementing a robust session ID extraction mechanism. The changes are well-structured, with new logic for session extraction, comprehensive unit tests, and appropriate security hardening. The integration into the existing session management flow is clean. I have one suggestion to improve consistency in session ID validation.
| const sessionIdRaw = originalMetadata?.session_id; | ||
| if (typeof sessionIdRaw === "string" && sessionIdRaw.trim()) { | ||
| return `codex_session_${sessionIdRaw.trim()}`; | ||
| } |
There was a problem hiding this comment.
For consistency and security, the session_id from metadata should be validated using the same logic as in session-extractor.ts. The current implementation only checks if it's a non-empty string, but misses the length and character validation performed by normalizeCodexSessionId.
I suggest reusing normalizeCodexSessionId here. This will require exporting it from src/app/v1/_lib/codex/session-extractor.ts.
Example of how to use it:
import { normalizeCodexSessionId } from "@/app/v1/_lib/codex/session-extractor";
// ... in generateUserID:
const sessionId = normalizeCodexSessionId(originalMetadata?.session_id);
if (sessionId) {
return `codex_session_${sessionId}`;
}| const sessionIdRaw = originalMetadata?.session_id; | |
| if (typeof sessionIdRaw === "string" && sessionIdRaw.trim()) { | |
| return `codex_session_${sessionIdRaw.trim()}`; | |
| } | |
| const sessionId = normalizeCodexSessionId(originalMetadata?.session_id); | |
| if (sessionId) { | |
| return `codex_session_${sessionId}`; | |
| } |
![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 implements session isolation for concurrent Codex CLI requests by extracting stable session IDs from headers and request body. The implementation is clean, well-tested, and follows security best practices with input validation. No significant issues were identified.
PR Size: M
- Lines changed: 273 (268 additions, 5 deletions)
- Files changed: 5
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 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean (proper input validation with length/character restrictions)
- Error handling - Clean (graceful fallbacks when session extraction fails)
- Type safety - Clean (proper null checks and type guards)
- Documentation accuracy - Clean (JSDoc comments accurately describe behavior)
- Test coverage - Adequate (11 unit tests covering extraction priority, security validation, edge cases)
- Code clarity - Good (clear function names and structure)
Notes
Positive aspects observed (not reported as issues per instructions):
- Security hardening with min/max length (21-256 chars) and character whitelist prevents Redis key bloat attacks
- Priority-based extraction chain is well-documented and logical
- Tests cover all extraction sources and validation edge cases
- Backward compatible with existing Claude/OpenAI format requests
Automated review by Claude AI
There was a problem hiding this comment.
Code Review Summary
This PR implements session isolation for concurrent Codex CLI requests by extracting stable session IDs from headers and request body. The implementation is clean, well-tested, and follows security best practices with input validation. No significant issues were identified.
PR Size: M
- Lines changed: 273 (268 additions, 5 deletions)
- Files changed: 5
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 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean (proper input validation with length/character restrictions)
- Error handling - Clean (graceful fallbacks when session extraction fails)
- Type safety - Clean (proper null checks and type guards)
- Documentation accuracy - Clean (JSDoc comments accurately describe behavior)
- Test coverage - Adequate (11 unit tests covering extraction priority, security validation, edge cases)
- Code clarity - Good (clear function names and structure)
Automated review by Claude AI
| userAgent?: string | null | ||
| ): string | null { | ||
| // Codex 请求：优先尝试从 headers/body 提取稳定的 session_id | ||
| if (headers && Array.isArray(requestMessage.input)) { |
There was a problem hiding this comment.
Critical [SECURITY-VULNERABILITY] Codex session_id validation can be bypassed via fallback
Why this is a problem: At src/lib/session-manager.ts:92, extractCodexSessionId(...) rejects too-short/too-long/invalid-character IDs, but this block falls through when result.sessionId is null and the function can later return metadata.user_id/metadata.session_id without those validations. A crafted metadata.session_id (e.g. extremely long or containing disallowed characters) would then be used as a Redis key component via getOrCreateSessionId(...), defeating the security hardening intent.
Suggested fix:
// Codex/Responses API request: only accept validated session IDs.
// If extraction fails, return null so SessionGuard falls back to deterministic session IDs.
if (headers && Array.isArray(requestMessage.input)) {
const result = extractCodexSessionId(headers, requestMessage, userAgent ?? null);
if (result.sessionId) {
logger.trace("SessionManager: Extracted session from Codex request", {
sessionId: result.sessionId,
source: result.source,
isCodexClient: result.isCodexClient,
});
return result.sessionId;
}
return null;
}
| const prevResponseId = normalizeCodexSessionId(requestBody.previous_response_id); | ||
| if (prevResponseId) { | ||
| return { | ||
| sessionId: `codex_prev_${prevResponseId}`, |
There was a problem hiding this comment.
Medium [LOGIC-BUG] codex_prev_ prefix can violate CODEX_SESSION_ID_MAX_LENGTH
Why this is a problem: normalizeCodexSessionId(...) enforces CODEX_SESSION_ID_MAX_LENGTH = 256 (src/app/v1/_lib/codex/session-extractor.ts:18) on previous_response_id, but sessionId: codex_prev_${prevResponseId}`` (src/app/v1/_lib/codex/session-extractor.ts:102) can be up to 267 chars. This undermines the stated “Prevent Redis key bloat from malicious input” constraint.
Suggested fix:
const PREV_PREFIX = "codex_prev_";
const prevResponseId = normalizeCodexSessionId(requestBody.previous_response_id);
if (prevResponseId) {
const sessionId = `${PREV_PREFIX}${prevResponseId}`;
if (sessionId.length <= CODEX_SESSION_ID_MAX_LENGTH) {
return {
sessionId,
source: "body_previous_response_id",
isCodexClient: officialClient,
};
}
}Suggested test:
test("rejects previous_response_id that would exceed 256 after prefix", () => {
const prev = "a".repeat(256);
const result = extractCodexSessionId(new Headers(), { previous_response_id: prev }, null);
expect(result.sessionId).toBe(null);
});
There was a problem hiding this comment.
Code Review Summary
This PR introduces Codex session ID extraction and wires it into session allocation. The main risk remaining is that invalid/overlong session IDs can still become clientSessionId via fallback paths and derived IDs can exceed the stated 256 char ceiling, which can bloat Redis keys and defeat the intended hardening.
PR Size: M
- Lines changed: 273
- Files changed: 5
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 0 |
| Security | 1 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/lib/session-manager.ts:92— Codex session_id validation can be bypassed via metadata fallback (Confidence: 95)
High Priority Issues (Should Fix)
src/app/v1/_lib/codex/session-extractor.ts:102—codex_prev_prefix can exceedCODEX_SESSION_ID_MAX_LENGTH(Confidence: 85)
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.
- PR
#434: applied labelsize/M(273 lines changed, 5 files). - Posted inline review comments on:
src/lib/session-manager.ts:92— Critical[SECURITY-VULNERABILITY]Codexsession_idvalidation bypass via fallbacksrc/app/v1/_lib/codex/session-extractor.ts:102— Medium[LOGIC-BUG]codex_prev_prefix can exceedCODEX_SESSION_ID_MAX_LENGTH
- Submitted the required
## Code Review Summaryreview comment to PR#434.
- Export normalizeCodexSessionId for unified validation - Add prefix length check to prevent sessionId > 256 chars - Block Codex request fallback to unvalidated metadata - Add boundary value tests (21 and 256 chars) Fixes: - Critical: session_id validation bypass via metadata fallback - Medium: codex_prev_ prefix could exceed 256 char limit - Medium: generateUserID lacked unified validation Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
🧪 测试结果
总体结果: ✅ 所有测试通过 |
1 participant
Summary
session_idfrom request headers/body with priority-based extractionChanges
src/app/v1/_lib/codex/session-extractor.tssrc/app/v1/_lib/codex/__tests__/session-extractor.test.tssrc/lib/session-manager.tsextractClientSessionId()signaturesrc/app/v1/_lib/proxy/session-guard.tssrc/app/v1/_lib/converters/codex-to-claude/request.tsSession ID Extraction Priority
headers["session_id"]headers["x-session-id"]body.metadata.session_idbody.previous_response_id(withcodex_prev_prefix)Security Hardening
/^[\w\-.:]+$/Test plan
Closes #430
Generated with Claude Code