fix: restore admin token auth in opaque session mode#884
Conversation
In opaque mode, validateAuthToken() rejected raw ADMIN_TOKEN passed via cookie or Authorization header, breaking programmatic API access that worked in legacy/dual mode. Add a constantTimeEqual check for the admin token before returning null, so server-side env secret still authenticates while regular API keys remain correctly rejected.
Summary of ChangesHello, 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 authentication regression where the ADMIN_TOKEN could not be used for programmatic API access when the system was configured in opaque session mode. By integrating a direct validation check for the ADMIN_TOKEN within the opaque session handling logic, the change restores essential administrative functionality without compromising the security design for other API keys. This ensures that systems relying on the ADMIN_TOKEN for automated tasks can continue to operate seamlessly after the v0.6 update. 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总体概述在不透明模式下的身份验证中添加了后向兼容的管理员令牌回退机制。当配置的管理员令牌存在且与提供的令牌相匹配时，系统调用密钥验证逻辑，启用了编程式访问。同时补充了相应的单元测试覆盖。 变更
代码审查工作量评估🎯 3 (中等) | ⏱️ ~20 分钟 🚥 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 |
![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 correctly restores admin token authentication when using the opaque session mode, which is an important backward-compatibility fix. The change in src/lib/auth.ts is direct and addresses the issue by adding a fallback check for the admin token. The accompanying new test file, tests/unit/auth/admin-token-opaque-fallback.test.ts, is thorough and provides excellent coverage for the fix, including success cases, security-related failure cases, and regressions for other modes. I've added a couple of suggestions to improve the maintainability of the new test file by reducing code duplication.
| const mockConfig = vi.hoisted(() => ({ | ||
| auth: { adminToken: "test-admin-secret-token-12345" }, | ||
| })); |
There was a problem hiding this comment.
To improve maintainability and avoid duplicating the magic string for the admin token, it's better to define ADMIN_TOKEN as a constant at the top of the module. This constant can then be used both here in the hoisted mock and within the test suite. This prevents potential inconsistencies if the token value needs to be updated in the future.
| const mockConfig = vi.hoisted(() => ({ | |
| auth: { adminToken: "test-admin-secret-token-12345" }, | |
| })); | |
| const ADMIN_TOKEN = "test-admin-secret-token-12345"; | |
| const mockConfig = vi.hoisted(() => ({ | |
| auth: { adminToken: ADMIN_TOKEN }, | |
| })); |
| } | ||
|
|
||
| describe("admin token opaque-mode fallback", () => { | ||
| const ADMIN_TOKEN = "test-admin-secret-token-12345"; |
There was a problem hiding this comment.
With ADMIN_TOKEN defined at the module level as suggested previously, this local declaration becomes redundant and should be removed to avoid shadowing and confusion.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/auth/admin-token-opaque-fallback.test.ts (2)
238-246: Mock 值与生产环境行为不一致。测试将
mockConfig.auth.adminToken设置为空字符串""，但根据env.schema.ts的预处理逻辑，空字符串会被转换为undefined。建议使用undefined以更准确地反映生产环境行为：♻️ 建议修改
it("opaque mode + admin token not configured -> auth fails for raw token", async () => { - mockConfig.auth.adminToken = ""; + mockConfig.auth.adminToken = undefined; setAuthCookie("some-random-token"); const { getSession } = await import("@/lib/auth"); const session = await getSession(); expect(session).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth/admin-token-opaque-fallback.test.ts` around lines 238 - 246, The test sets mockConfig.auth.adminToken = "" which doesn't match production preprocessing that converts empty strings to undefined; update the test to set mockConfig.auth.adminToken = undefined (in the "opaque mode + admin token not configured -> auth fails for raw token" test) so behavior matches env.schema preprocessing, keeping the calls to setAuthCookie("some-random-token") and the subsequent import and call to getSession() unchanged.
18-20: Mock 类型需支持undefined。若要在测试中将
adminToken设置为undefined，需要更新 mock 定义的类型：♻️ 建议修改
const mockConfig = vi.hoisted(() => ({ - auth: { adminToken: "test-admin-secret-token-12345" }, + auth: { adminToken: "test-admin-secret-token-12345" as string | undefined }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth/admin-token-opaque-fallback.test.ts` around lines 18 - 20, The current hoisted mock `mockConfig` defines `auth.adminToken` as a string only, which prevents tests from setting it to undefined; update the mock's type/signature so `adminToken` can be string | undefined (for example by annotating `vi.hoisted` with a type where `auth: { adminToken?: string | undefined }` or `adminToken: string | undefined`) and adjust the returned object to allow `adminToken` to be undefined when needed so tests that set `adminToken = undefined` compile and run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/auth/admin-token-opaque-fallback.test.ts`:
- Around line 238-246: The test sets mockConfig.auth.adminToken = "" which
doesn't match production preprocessing that converts empty strings to undefined;
update the test to set mockConfig.auth.adminToken = undefined (in the "opaque
mode + admin token not configured -> auth fails for raw token" test) so behavior
matches env.schema preprocessing, keeping the calls to
setAuthCookie("some-random-token") and the subsequent import and call to
getSession() unchanged.
- Around line 18-20: The current hoisted mock `mockConfig` defines
`auth.adminToken` as a string only, which prevents tests from setting it to
undefined; update the mock's type/signature so `adminToken` can be string |
undefined (for example by annotating `vi.hoisted` with a type where `auth: {
adminToken?: string | undefined }` or `adminToken: string | undefined`) and
adjust the returned object to allow `adminToken` to be undefined when needed so
tests that set `adminToken = undefined` compile and run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9ca0138-3aaa-4308-9b3b-c5306bc6aa5b
📒 Files selected for processing (2)
src/lib/auth.tstests/unit/auth/admin-token-opaque-fallback.test.ts
![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: M
- Lines changed: 254
- Files changed: 2
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.
Reviewed PR #884 (“fix: restore admin token auth in opaque session mode”).
- Applied label:
size/M(254 lines changed, 2 files). - Posted PR review summary via
gh pr review(no diff-scoped issues met the >=80 confidence reporting threshold, so no inline review comments were added).
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Fixes a regression in v0.6 where
SESSION_TOKEN_MODE=opaque(the default) broke programmatic API access using rawADMIN_TOKEN. Adds secure fallback validation to restore backward compatibility for/api/*endpoints while maintaining the security posture of opaque session mode.Problem
In v0.6, the session token mode defaults to
opaqueas part of the security overhaul (#803). However, this broke a critical use case:ADMIN_TOKENvia cookie or Bearer header were rejectedvalidateAuthToken()only accepted opaque session IDs (sid_*) in opaque modeADMIN_TOKENvalidation path was never reached when mode wasopaqueThis prevented server-side scripts and external integrations from authenticating with the admin token when the application was in opaque session mode.
Solution
Added a
constantTimeEqualcheck for the admin token in the opaque-mode path ofvalidateAuthToken()before returning null:Security considerations:
Changes
src/lib/auth.tstests/unit/auth/admin-token-opaque-fallback.test.tsRelated
Testing
New Unit Tests
tests/unit/auth/admin-token-opaque-fallback.test.ts(6 cases)Regression Tests
tests/security/auth-dual-read.test.ts(6/6 pass)tests/unit/auth/opaque-admin-session.test.ts(3/3 pass)Checklist
Original description preserved below
Summary
SESSION_TOKEN_MODEdefaults toopaque. This broke programmatic API calls using rawADMIN_TOKENvia cookie or Bearer header, sincevalidateAuthToken()only accepted opaque session IDs (sid_*) and never fell back to key validation.constantTimeEqualcheck for admin token in the opaque-mode path ofvalidateAuthToken(), restoring backward-compatible access for all/api/*endpoints.Test plan
tests/unit/auth/admin-token-opaque-fallback.test.ts(6 cases)tests/security/auth-dual-read.test.ts(6/6 pass)tests/unit/auth/opaque-admin-session.test.ts(3/3 pass)Greptile Summary
This PR restores backward-compatible
ADMIN_TOKENaccess in opaque session mode by adding aconstantTimeEqualfallback insidevalidateAuthToken()that executes when the opaque session store returns no match. Regular (non-admin) raw API keys remain correctly rejected in opaque mode, preserving security posture.Implementation: Adds a 7-line admin-token fallback block at the bottom of
validateAuthToken()(after the opaque session store lookup, after thelegacy/dualmode branch) that delegates to the existingvalidateKey()function, which constructs the virtual admin user. Test coverage is comprehensive (6 cases covering cookie, Bearer header, opaque session, legacy mode, and unconfigured token paths).Security concern: The raw
ADMIN_TOKENvalue is passed tosessionStore.read(token)(line 272) before theconstantTimeEqualcheck fires (line 297). In production environments with Redis command logging or observability sidecars, this exposes the plaintext token in logs. Moving the admin-token check to run before the session-store call would eliminate this exposure and save a Redis round-trip for every programmatic admin request.Confidence Score: 2/5
Sequence Diagram
sequenceDiagram participant C as Client participant VA as validateAuthToken() participant SS as SessionStore (Redis) participant VK as validateKey() C->>VA: token (cookie or Bearer) alt mode == legacy VA->>VK: validateKey(token) VK-->>VA: AuthSession | null else mode == dual VA->>SS: read(token) SS-->>VA: session | null alt session found & valid VA->>VK: validateKey via convertToAuthSession VK-->>VA: AuthSession else session not found VA->>VK: validateKey(token) VK-->>VA: AuthSession | null end else mode == opaque (current PR) VA->>SS: read(token) ⚠️ raw ADMIN_TOKEN sent here SS-->>VA: session | null alt session found & valid VA->>VK: convertToAuthSession → validateKey VK-->>VA: AuthSession else session not found VA->>VA: constantTimeEqual(token, adminToken)? alt token == ADMIN_TOKEN VA->>VK: validateKey(token) VK-->>VA: admin AuthSession (id=-1) else not admin token VA-->>C: null (rejected) end end end VA-->>C: AuthSession | nullComments Outside Diff (1)
src/lib/auth.ts, line 269-301 (link)Raw admin token forwarded to Redis before equality check
In opaque mode, the raw
tokenstring is passed tosessionStore.read(token)on line 272 before theconstantTimeEqualguard on line 297. This means if a request includes a rawADMIN_TOKENin the cookie or Bearer header, it will be sent to Redis as a lookup key. If Redis command logging or any monitoring sidecar is enabled in production, the plaintextADMIN_TOKENwill appear in those logs.Recommend moving the admin-token check to execute before the session-store lookup (guarded with
mode === "opaque"). This would:legacyanddualmodesLast reviewed commit: 171fa9c