Skip to content

fix: validate canLoginWebUi permission in middleware#4

Merged
ding113 merged 1 commit into
mainfrom
claude/issue-3-20251027-1511
Oct 27, 2025
Merged

fix: validate canLoginWebUi permission in middleware#4
ding113 merged 1 commit into
mainfrom
claude/issue-3-20251027-1511

Conversation

@ding113

@ding113 ding113 commented Oct 27, 2025

Copy link
Copy Markdown
Owner

修复 Bug: key 不允许登陆 web 面板的设置无效

问题描述

  • 登录接口正确检查了 canLoginWebUi 权限
  • 但中间件只检查 cookie 是否存在,没有重新验证 key 的权限
  • 导致用户登录后,管理员修改 canLoginWebUi=false 时,用户仍可访问 dashboard

修复方案

  • 在 middleware 中调用 validateKey() 验证完整权限
  • 包括 canLoginWebUiisEnabledexpiresAt 等检查
  • 如果验证失败,清除 cookie 并重定向到登录页

Fixes #3

🤖 Generated with Claude Code

修复 key 不允许登陆 web 面板的设置无效的问题

**问题描述**:
- 登录接口正确检查了 canLoginWebUi 权限
- 但中间件只检查 cookie 是否存在,没有重新验证 key 的权限
- 导致用户登录后,管理员修改 canLoginWebUi=false 时,用户仍可访问 dashboard

**修复方案**:
- 在 middleware 中调用 validateKey() 验证完整权限
- 包括 canLoginWebUi、isEnabled、expiresAt 等检查
- 如果验证失败,清除 cookie 并重定向到登录页

Fixes #3

Co-authored-by: Ding <ding113@users.noreply.github.com>
@claude

claude Bot commented Oct 27, 2025

Copy link
Copy Markdown
Contributor

Claude finished @ding113's task —— View job


代码审查完成 ✅

任务清单

  • 读取并分析 src/middleware.ts 的变更
  • 验证 validateKey() 函数的实现
  • 检查安全性和边界情况
  • 提供代码审查反馈

审查结果:✅ 批准合并

这个 PR 很好地解决了 Issue #3 中描述的安全漏洞。代码质量高,逻辑清晰,修复方案完善。

优点

  1. ✅ 准确定位问题

    • 正确识别出中间件只检查 cookie 存在性,未验证实时权限的安全漏洞
    • 修复后确保每次请求都验证 key 的完整权限
  2. ✅ 修复方案完善

    • 调用 validateKey() 进行完整的权限检查:
      • canLoginWebUi - Web UI 登录权限
      • isEnabled - Key 是否启用
      • expiresAt - Key 是否过期
      • 用户是否存在且未删除
    • 验证失败时正确清除 cookie 并重定向
    • 保留了 from 参数,用户体验良好
  3. ✅ 代码质量

    • 清晰的注释说明修复意图
    • 函数签名正确添加了 async
    • 错误处理逻辑完整(清除 cookie + 重定向)
    • 符合项目现有代码风格
  4. ✅ 安全性提升

    • 关闭了权限绕过漏洞
    • 确保管理员修改权限后立即生效
    • 不再依赖 7 天 cookie 过期才生效

技术细节验证

我已验证以下关键点:

  1. validateKey() 函数实现正确 (src/lib/auth.ts:17-67)

    • Admin Token 特殊处理(始终允许)
    • 调用 findActiveKeyByKeyString() 检查 key 状态
    • findActiveKeyByKeyString() 已在 SQL 层面过滤:
      • isEnabled = true
      • deletedAt IS NULL
      • expiresAt IS NULL OR expiresAt > NOW()
    • 额外检查 canLoginWebUi 权限
    • 验证关联的 user 是否存在
  2. 性能考虑

    • 每次请求都会查询数据库验证权限
    • 对于受保护路由这是必要的安全开销
    • 建议:如果未来流量大,可考虑添加短期缓存(如 30-60 秒),平衡安全性和性能
  3. 边界情况处理正确

    • Cookie 不存在 → 重定向登录 ✅
    • Key 无效/过期/禁用 → 清除 cookie + 重定向 ✅
    • 权限验证通过 → 正常访问 ✅

建议(可选)

虽然当前实现已经很好,但如果考虑性能优化,未来可以:

  1. 添加短期缓存(仅供参考,不要求本 PR 实现)

    // 可以考虑在 Redis 中缓存 validateKey 结果 30-60 秒
    // 既能及时响应权限变更,又能减少数据库压力
    // 但要权衡安全性(权限变更延迟)和性能
  2. 日志记录(仅供参考)

    // 建议在权限验证失败时记录日志,便于监控异常访问
    if (!session) {
      logger.warn('Key validation failed in middleware', { 
        authToken: authToken.value.substring(0, 10) + '...', 
        pathname 
      });
    }

测试建议

建议手动测试以下场景:

  1. ✅ 用户正常登录并访问 dashboard
  2. ✅ 管理员修改 canLoginWebUi=false,用户刷新页面应立即被登出
  3. ✅ 管理员修改 isEnabled=false,用户刷新页面应立即被登出
  4. ✅ Key 过期后,用户刷新页面应立即被登出
  5. ✅ Admin Token 仍然可以正常访问

总结

这是一个高质量的安全修复 PR,建议立即合并。修复方案简洁有效,没有引入不必要的复杂性,同时解决了多个相关的权限实时性问题。


@ding113
ding113 merged commit 1866fac into main Oct 27, 2025
2 checks passed
@ding113
ding113 deleted the claude/issue-3-20251027-1511 branch October 27, 2025 15:20
@claude claude Bot added the bug Something isn't working label Oct 27, 2025
ding113 added a commit that referenced this pull request May 2, 2026
…1149)

* fix: keys:reveal endpoint, redirect fallback regression test, bill-on-failure toggle

Bundles four fixes raised against dev:

1. **keys:reveal endpoint (Issue #1)** — Add `GET /api/v1/keys/{id}:reveal`
   mirroring the provider key:reveal pattern (audit log, no-store cache,
   admin-only). Rewire dashboard key row + key list to fetch the unmasked
   key on click instead of pre-loading it in user-list payloads. Removes
   `fullKey` from `UserKeyDisplay` and replaces it with a `canReveal`
   permission flag — unmasked keys never leave the server unless explicitly
   requested.

2. **Model redirect fallback regression test (Issue #2)** — Pin the
   contract: when fallback occurs, the next provider's redirect rules MUST
   match against the user-requested model (not the model the previous
   provider rewrote it to). Static analysis + 4 new test scenarios confirm
   the existing `ModelRedirector.apply` already implements the contract.
   Test file now guards against future regression.

3. **499 frequency investigation (Issue #3)** — Read-only audit of the
   recent abort-listener cleanup commits (bcba5d0, 8968a42). Conclusion:
   no regression — those commits only fixed listener leaks in normal-
   completion paths. The user's log (Codex CLI cancelling stream at ~3s)
   is genuine and correctly classified as 499. No code change for this
   issue; the new bill-on-failure toggle (#4) gives operators a way to
   recover token billing for these cases.

4. **billNonSuccessfulRequests toggle (Issue #4)** — New system setting
   (default OFF). When ON, requests with non-2xx status that received
   positive token usage from the upstream (e.g., 499 mid-stream client
   abort with partial usage) are billed normally. The existing fake-200
   detector still skips billing for fake-success error payloads
   regardless of toggle state. Schema migration + cache + UI toggle (5
   locales) + unit tests.

Tests added:
- `tests/unit/actions/keys-reveal.test.ts` — admin/auth/404 paths and
  audit redaction for the new action.
- `tests/unit/proxy/model-redirect-fallback.test.ts` — 4 scenarios for
  fallback redirect behavior.
- `tests/unit/proxy/response-handler-bill-non-success.test.ts` — toggle
  semantics, fake-200 still skipped, setting-read failure fail-closed.

Migration: 0102_useful_lionheart.sql adds
`bill_non_successful_requests boolean default false not null` to
`system_settings` (idempotent ADD COLUMN IF NOT EXISTS).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(pr-review): address review feedback on keys:reveal

Resolves feedback from CodeRabbit, Greptile, gemini-code-assist, and chatgpt-codex
on PR #1149:

1. **Allow key owner to reveal their own key (HIGH)** — `canExposeFullKey` already
   marks an owner's keys as `canReveal: true` in list payloads, but the new
   `getUnmaskedKey` action and v1 router gated everything behind admin, causing
   a 403 regression for self-service users.
   - `getUnmaskedKey`: replace admin-only check with admin-OR-owner.
   - v1 `:reveal` route: relax `requireAuth("admin")` -> `requireAuth("read")`,
     update OpenAPI `x-required-access` to `admin | owner` and the description
     to reflect that ownership is checked at the action layer.
   - Test: replace "rejects non-admin" with two cases — owner allowed, non-owner
     rejected.

2. **Toast feedback on reveal/copy failure in key-list.tsx (HIGH)** —
   `fetchUnmaskedKey` was silently returning null. Mirror the key-row-item
   pattern: import `toast` from sonner and surface failures via
   `tCommon("copyFailed")`.

3. **Drop stale revealedKey cache when row identity changes (MAJOR)** — two
   call sites:
   - `key-row-item.tsx`: `useEffect` resets `revealedKey` and closes the dialog
     whenever `keyData.id` or `keyData.maskedKey` changes (parent may reuse the
     same component instance for a different row).
   - `key-list.tsx`: prune `revealedKeys` and `visibleKeyIds` whenever the
     incoming `keys` set changes so removed/replaced ids cannot leak their
     cached plaintext onto a new row.

4. **Missing assertion in model-redirect-fallback test 3 (MINOR)** — the
   "no rules" reset path must rewrite both `request.model` and
   `request.message.model`. Add the missing `request.message.model` check to
   guard against half-reset regressions.

Tests: 16/16 pass (3 files), typecheck clean, lint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(pr-review): align keys:reveal x-required-access with the contract enum

CI on commit 12bc4d2 failed two checks:
- openapi-contract: 'admin | owner' is not in the allowed enum
  (public/read/admin) for x-required-access.
- openapi-types-drift: generated types out of date because the route
  description / annotation changed.

Fix: keep the auth tier as 'read' (already what `requireAuth("read")`
applies on the route) and document the admin-OR-owner contract in the
OpenAPI 'description' field instead. Regenerate openapi-types.gen.ts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: key 不允许登陆 web 面板的设置无效

1 participant