fix(api): 透传 /api/actions 认证会话以避免 getUsers 返回空数据#720
Conversation
Summary of ChangesHello @WAY29, 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! 此拉取请求旨在修复 Highlights
Changelog
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
|
📝 WalkthroughWalkthrough在服务器端增加基于 AsyncLocalStorage 的全局作用域认证会话存储,并在 action adapter 中支持在存在认证会话时通过 runWithAuthSession 包装 action 调用;新增单元测试验证会话传播。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 分钟 Possibly related PRs
Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
Code Review
此拉取请求通过引入基于 AsyncLocalStorage 的认证会话透传机制,有效解决了 /api/actions 认证会话无法正确传递到 getUsers 等内部 action 函数的问题。新增的 runWithAuthSession 和 getScopedAuthSession 函数提供了在不同上下文(如 Hono 适配器和 Next.js getSession)中访问和管理认证会话的统一方式,这是一个很好的改进,提升了 API 的健壮性和可维护性。然而,我们发现 getSession 实现中存在一个高严重性安全漏洞,注入的会话绕过了 allowReadOnlyAccess 检查,可能导致受限的 API 密钥执行未经授权的操作。请务必应用建议的修复,以确保即使从作用域存储中检索会话时,其权限也能得到正确验证。
There was a problem hiding this comment.
Code Review Summary
This PR implements a clean solution to fix session propagation in the /api/actions adapter by using AsyncLocalStorage. The implementation is well-designed with proper defensive coding and adequate test coverage.
PR Size: S
- Lines changed: 169 (167 additions, 2 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 |
Key Implementation Highlights
Correct Use of AsyncLocalStorage: The PR uses Node.js AsyncLocalStorage to propagate authentication sessions from the Hono adapter layer to Server Actions, solving the issue where next/headers is unavailable in the Hono context.
Defensive Fallbacks: The code includes proper fallback logic - if the storage isn't initialized, it safely returns without session propagation. The test verifies this behavior by mocking next/headers to throw, ensuring the scoped session is used.
Type Safety: The type cast in auth-session-storage.node.ts is intentional to create a minimal interface contract, and the actual runtime behavior matches the type definitions.
Test Coverage: The regression test adequately covers the critical path (requiresAuth=true with successful authentication), and the test design correctly validates that the session propagation works by mocking next/headers.cookies() to throw an error.
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
* fix(proxy): add 'cannot be modified' error detection to thinking signature rectifier Extend the thinking signature rectifier to detect and handle the Anthropic API error when thinking/redacted_thinking blocks have been modified from their original response. This error occurs when clients inadvertently modify these blocks in multi-turn conversations. The rectifier will now remove these blocks and retry the request, similar to how it handles other thinking-related signature errors. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore(deps): bump jspdf in the npm_and_yarn group across 1 directory Bumps the npm_and_yarn group with 1 update in the / directory: [jspdf](https://github.com/parallax/jsPDF). Updates `jspdf` from 3.0.4 to 4.1.0 - [Release notes](https://github.com/parallax/jsPDF/releases) - [Changelog](https://github.com/parallax/jsPDF/blob/master/RELEASE.md) - [Commits](parallax/jsPDF@v3.0.4...v4.1.0) --- updated-dependencies: - dependency-name: jspdf dependency-version: 4.1.0 dependency-type: direct:production dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com> * fix: Hot-reload cache invalidation for Request Filters and Sensitive Words (#710) * fix: hot-reload request filters via globalThis singleton pattern EventEmitter and RequestFilterEngine now use globalThis caching to ensure the same instance is shared across different Next.js worker contexts. This fixes the issue where filter changes required Docker restart. Added diagnostic logging for event subscription and propagation. * fix(redis): wait for subscriber connection ready before subscribe - ensureSubscriber now returns Promise<Redis>, waits for 'ready' event - subscribeCacheInvalidation returns null on failure instead of noop - RequestFilterEngine checks cleanup !== null before logging success - Fixes false "Subscribed" log when Redis connection fails * feat(sensitive-words): add hot-reload via Redis pub/sub Enable real-time cache invalidation for sensitive words detector, matching the pattern used by request-filter-engine and error-rule-detector. * fix(redis): harden cache invalidation subscriptions Ensure sensitive-words CRUD emits update events so hot-reload propagates across workers. Roll back failed pub/sub subscriptions, add retry/timeout coverage, and avoid sticky provider-cache subscription state. * fix(codex): bump default User-Agent fallback Update the hardcoded Codex UA used when requests lack an effective user-agent (e.g. filtered out). Keep unit tests in sync with the new default. * fix(redis): resubscribe cache invalidation after reconnect Clear cached subscription state on disconnect and resubscribe on ready so cross-worker cache invalidation survives transient Redis reconnects. Add unit coverage, avoid misleading publish logs, track detector cleanup handlers, and translate leftover Russian comments to English. * fix(sensitive-words): use globalThis singleton detector Align SensitiveWordDetector with existing __CCH_* singleton pattern to avoid duplicate instances across module reloads. Extend singleton unit tests to cover the detector. * chore: format code (req-fix-dda97fd) * fix: address PR review comments - pubsub.ts: use `once` instead of `on` for ready event to prevent duplicate resubscription handlers on reconnect - forwarder.ts: extract DEFAULT_CODEX_USER_AGENT constant - provider-cache.ts: wrap subscribeCacheInvalidation in try/catch - tests: use exported constant instead of hardcoded UA string * fix(redis): resubscribe across repeated reconnects Ensure pub/sub resubscribe runs on every reconnect, extend unit coverage, and keep emitRequestFiltersUpdated resilient when logger import fails. --------- Co-authored-by: John Doe <johndoe@example.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * feat(logs): make cost column toggleable with improved type safety (#715) close #713 * fix(proxy): add OpenAI chat completion format support in usage extraction (#705) (#716) The `extractUsageMetrics` function was missing support for OpenAI chat completion format fields (`prompt_tokens`/`completion_tokens`), causing token statistics to not be recorded for OpenAI-compatible providers. Changes: - Add `prompt_tokens` -> `input_tokens` mapping - Add `completion_tokens` -> `output_tokens` mapping - Preserve priority: Claude > Gemini > OpenAI format - Add 5 unit tests for OpenAI format handling Closes #705 Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * fix(currency): respect system currencyDisplay setting in UI (#717) Fixes #678 - Currency display unit configuration was not applied. Root cause: - `users-page-client.tsx` hardcoded `currencyCode="USD"` - `UserLimitBadge` and `LimitStatusIndicator` had hardcoded `unit="$"` default - `big-screen/page.tsx` used hardcoded "$" in multiple places Changes: - Add `getCurrencySymbol()` helper function to currency.ts - Fetch system settings in `users-page-client.tsx` and pass to table - Pass `currencySymbol` from `user-key-table-row.tsx` to limit badges - Remove hardcoded "$" defaults from badge components - Update big-screen page to fetch settings and use dynamic symbol - Add unit tests for `getCurrencySymbol` Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * feat(gemini): add Google Search web access preference for Gemini providers (#721) * feat(gemini): add Google Search web access preference for Gemini providers Add provider-level preference for Gemini API type providers to control Google Search (web access) tool injection: - inherit: Follow client request (default) - enabled: Force inject googleSearch tool into request - disabled: Force remove googleSearch tool from request Changes: - Add geminiGoogleSearchPreference field to provider schema - Add GeminiGoogleSearchPreference type and validation - Implement applyGeminiGoogleSearchOverride with audit trail - Add UI controls in provider form (Gemini Overrides section) - Add i18n translations for 5 languages (en, zh-CN, zh-TW, ja, ru) - Integrate override logic in proxy forwarder for Gemini requests - Add 22 unit tests for the override logic Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(gemini): address code review feedback - Use explicit else-if for disabled preference check (gemini-code-assist) - Use i18n for SelectValue placeholder instead of hardcoded string (coderabbitai) - Sync overridden body back to session.request.message for log consistency (coderabbitai) - Persist Gemini special settings immediately, matching Anthropic pattern (coderabbitai) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(gemini): use strict types for provider config and audit - Narrow preference type to "enabled" | "disabled" (exclude unreachable "inherit") - Use ProviderType and GeminiGoogleSearchPreference types instead of string Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * fix(api): 透传 /api/actions 认证会话以避免 getUsers 返回空数据 (#720) * fix(api): 透传 /api/actions 认证会话以避免 getUsers 返回空数据 * fix(auth): 让 scoped session 继承 allowReadOnlyAccess 语义并支持内部降权校验 * chore: format code (dev-93585fa) * fix: bound SessionTracker active_sessions zsets by env TTL (#718) * fix(session): bound active_sessions zsets by env ttl Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix(rate-limit): pass session ttl to lua cleanup Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix(session): validate SESSION_TTL env and prevent ZSET leak on invalid values - Add input validation for SESSION_TTL (reject NaN, 0, negative; default 300) - Guard against invalid TTL in Lua script to prevent clearing all sessions - Use dynamic EXPIRE based on SESSION_TTL instead of hardcoded 3600s - Add unit tests for TTL validation and dynamic expiry behavior --------- Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix(provider): stop standard-path fallback to legacy provider url * feat(providers): expose vendor endpoint pools in settings UI (#719) * feat(providers): add endpoint status mapping * feat(providers): add endpoint pool hover * feat(providers): show vendor endpoints in list rows * feat(providers): extract vendor endpoint CRUD table * chore(i18n): add provider endpoint UI strings * fix(providers): integrate endpoint pool into provider form * fix(provider): wrap endpoint sync in transactions to prevent race conditions (#730) * fix(provider): wrap provider create/update endpoint sync in transactions Provider create and update operations now run vendor resolution and endpoint sync inside database transactions to prevent race conditions that could leave orphaned or inconsistent endpoint rows. Key changes: - createProvider: wrap vendor + insert + endpoint seed in a single tx - updateProvider: wrap vendor + update + endpoint sync in a single tx - Add syncProviderEndpointOnProviderEdit for atomic URL/type/vendor migration with in-place update, soft-delete, and conflict handling - Vendor cleanup failures degrade to warnings instead of propagating - Add comprehensive unit and integration tests for sync edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(provider): defer endpoint circuit reset until transaction commit Avoid running endpoint circuit reset side effects inside DB transactions to prevent rollback inconsistency. Run resets only after commit and add regression tests for deferred reset behavior in helper and provider update flows. * fix(provider): distinguish noop from created-next in endpoint sync action label When ensureNextEndpointActive() returns "noop" (concurrent transaction already created an active next endpoint), the action was incorrectly labelled "kept-previous-and-created-next". Add a new "kept-previous-and-kept-next" action to ProviderEndpointSyncAction and use a three-way branch so callers and logs reflect the true outcome. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review comments from PR #731 - fix(auth): prevent scoped session access widening via ?? -> && guard - fix(i18n): standardize zh-CN provider terminology to "服务商" - fix(i18n): use consistent Russian translations for circuit status - fix(i18n): replace raw formatDistanceToNow with locale-aware RelativeTime - fix(gemini): log warning for unknown google search preference values - fix(error-rules): check subscribeCacheInvalidation return value - fix(test): correct endpoint hover sort test to assert URLs not labels Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: export auth session storage and fix test mock types - Export authSessionStorage from auth-session-storage.node.ts to prevent undefined on named imports; remove duplicate declare global block - Fix mockEndpoints in provider-endpoint-hover test: remove nonexistent lastOk/lastLatencyMs fields, add missing lastProbe* fields, use Date objects for createdAt/updatedAt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: dependabot[bot] <49699333+dependabot[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: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: 泠音 <im@ling.plus> Co-authored-by: Longlone <41830147+WAY29@users.noreply.github.com> Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Summary
Fix session propagation in
/api/actionsendpoints to ensuregetSession()returns valid data when called from within actions.Problem
After PR #704 fixed the authentication issue (#687) by adding
allowReadOnlyAccess: trueto endpoints, a related problem remained: when the action adapter (Hono-based) successfully authenticates a user and the action internally callsgetSession(), it returnsnullbecause:validateKey()next/headersto read request contextgetSession()to returnnullok: truebut with empty dataRelated Issues:
Solution
Introduce
AsyncLocalStorageto propagate the authenticated session from the adapter layer to the action layer:auth-session-storage.node.ts): Creates a globalAsyncLocalStorageinstance for request-scoped session storageauth.ts): AddsrunWithAuthSession()to wrap action execution with session context, andgetScopedAuthSession()to retrieve the injected sessiongetSession()to first check for an adapter-injected session before falling back tonext/headersaction-adapter-openapi.ts): Wraps authenticated action calls withrunWithAuthSession()to inject the validated sessionChanges
Core Changes
src/lib/auth-session-storage.node.tssrc/lib/auth.tsrunWithAuthSession(),getScopedAuthSession(), and priority check ingetSession()src/lib/api/action-adapter-openapi.tsrunWithAuthSession()when authenticatedsrc/app/api/actions/[...route]/route.tsTest Changes
tests/api/action-adapter-auth-session.unit.test.tsTesting
Automated Tests
getSession()returns injected session within action contextnext/headersto throw error, ensuring the new path is usedManual Testing
Verified with curl command:
Regular user tokens now return valid user data instead of empty results.
Breaking Changes
None. This is a backwards-compatible fix that improves session handling without changing the API contract.
Checklist
Description enhanced by Claude AI
Greptile Overview
Greptile Summary
Fixes session propagation in
/api/actionsendpoints by introducingAsyncLocalStorage-based context passing, ensuringgetSession()returns valid data when called from within actions.Key Changes
auth-session-storage.node.ts): GlobalAsyncLocalStoragesingleton for request-scoped session storageauth.ts):runWithAuthSession()wraps execution context,getScopedAuthContext()retrieves injected sessiongetSession()checks ALS context first, falls back tonext/headersrunWithAuthSession()when authenticatedallowReadOnlyAccesssetting, with support for explicit downgradeTechnical Approach
The solution addresses the runtime incompatibility where actions cannot access Next.js
headers()context by:validateKey()AsyncLocalStoragebefore action executionTest Coverage
Comprehensive unit test verifies:
canLoginWebUi: false)getSession({allowReadOnlyAccess: false})next/headersin action contextConfidence Score: 5/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Adapter as Hono Adapter participant Route as Action Route Handler participant Storage as AsyncLocalStorage participant Action as Server Action participant GetSession as getSession() Client->>Adapter: HTTP Request with credentials Adapter->>Route: Forward to action route Route->>Route: validateKey() authenticates user Route->>Storage: runWithAuthSession(session, fn, opts) Storage->>Storage: Store session in ALS context Storage->>Action: Execute action function Action->>GetSession: Call getSession() GetSession->>Storage: getScopedAuthContext() Storage-->>GetSession: Return context from ALS Note over GetSession: Priority logic:<br/>1. ALS context (injected)<br/>2. next/headers (fallback) GetSession->>GetSession: Apply permission downgrade if needed GetSession-->>Action: Return session or null Action-->>Storage: Return result Storage-->>Route: Propagate result Route-->>Client: HTTP Response(2/5) Greptile learns from your feedback when you react with thumbs up/down!