宜博 / lemonboss

Commit 46c780abadd700b8c22b496dc8e283543004c277

Authored by yibo
1 parent f658bd05

fix(security): api-key 鉴权把原始用户行挂上 req.user；补 requireApiKey 的 4 条分支 

## 起因

integration 测试里 `process.env.API_KEYS = 'test-api-key-123'` 是死代码：
生产读的是 OPEN_API_KEYS（config.js:31），而这个测试文件里 API_KEYS 只出现
这一次赋值，没有任何 /api/open/* 请求、没有任何 key 相关断言。已删。

（更正上一轮的判断：它不是「为了错误的原因而绿」——它什么都没测，
所以也谈不上错。两个缺陷互相抵消成了无害。）

## 为什么错名字能活这么久

api-key-auth.test.js 原有 7 条用例**全在测 extractApiKey**（怎么从 header/query
取出字符串），而做鉴权决定的 requireApiKey 一条都没有。OPEN_API_KEYS 那条分支
从来没被执行过 —— 没有测试执行过的路径，喂给它什么名字都不会有人知道。

补 4 条（拦/放两侧各两条）：无 key → 401「需要 key」/ key 无效 → 401「key 不对」/
命中系统 key 列表 → 放行且挂 req.apiKey / 命中用户 api_key → 放行且挂 req.user。

## 补完当场抓到一个真缺陷（这一条才是重点）

requireApiKey 把 findByApiKey 返回的**原始数据库行**挂上 req.user，带着
password_hash。JWT 那条路（jwt-auth.extractUser）一直是过 toPublicUser 才挂的 ——
两条鉴权路径行为不一致，而 req.user 是 handler 唯一的身份来源。

当前没有实际泄漏（open-api 的 handler 只读 req.user?.id / ?.email，
没有哪个把整个对象回给调用方），但将来任何一个 res.json({ user: req.user })
都会把 bcrypt 哈希发给外部 API 调用方。改成过 toPublicUser。

顺带确认 requireApiKey 本身是 fail-closed 的：无 key 401；key 既不匹配用户
api_key 也不在配置列表 401。OPEN_API_KEYS 为空只是让系统级 key 全部失效，
不放开任何东西 —— 所以那个错名字不构成安全漏洞。

## 验证

862 全绿（85 文件）。4 处变异逐一精确变红后逐字还原。

其中 T3 值得记：第一版「无 key → 401」只断言状态码，而把 if (!key) 改成
if (false) 之后测试仍全绿 —— 因为纵深防御，key=null 会落到第二道门照样 401。
两者的区别在**文案**（「需要 key」vs「key 不对」），而我在另一条用例的注释里
自己写了「两种失败要分得开」却没断言它。补上文案断言后 T3 才真的变红。
只断言状态码的鉴权测试基本都有这个盲区。

数据核查：跑完全套后共享 users.sqlite 的 sha256 不变、用户数仍为 1
（新用例用 USERS_DB_PATH 打到 /tmp —— 这个覆盖口是上一轮刚补的，
在它存在之前这组用例会往共享生产库里插测试用户）。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Inline Side-by-side
Showing 3 changed files with 126 additions and 3 deletions
server/middleware/api-key-auth.js
  View file @46c780a
@@ -47,7 +47,13 @@ function requireApiKey(req, res, next) { @@ -47,7 +47,13 @@ function requireApiKey(req, res, next) {
47 47
48 try { 48 try {
49 const userDb = require('../services/user-db'); 49 const userDb = require('../services/user-db');
50 - validUser = userDb.findByApiKey(key); 50 + const row = userDb.findByApiKey(key);
  51 + // 过 toPublicUser 再挂上去：findByApiKey 返回的是**原始数据库行**，带 password_hash。
  52 + // JWT 那条路（jwt-auth.extractUser）一直是脱敏后才挂的，这里以前不是 ——
  53 + // 两条鉴权路径行为不一致，而 req.user 是 handler 唯一的身份来源：
  54 + // 将来任何一个 res.json({ user: req.user }) 都会把密码哈希发给外部 API 调用方。
  55 + // 目前所有 handler 只读 req.user.id / .email，还没真漏，但这个口子不该留着。
  56 + validUser = row ? userDb.toPublicUser(row) : null;
51 } catch (err) { 57 } catch (err) {
52 console.error('[OpenAPI] DB check failed for API key:', err); 58 console.error('[OpenAPI] DB check failed for API key:', err);
53 } 59 }
server/tests/integration/api-routes.test.js
  View file @46c780a
@@ -45,7 +45,6 @@ describe(&#39;API Integration Tests&#39;, () =&gt; { @@ -45,7 +45,6 @@ describe(&#39;API Integration Tests&#39;, () =&gt; {
45 process.env.SKILLS_CODE_DIR = TEST_SKILLS_DIR; 45 process.env.SKILLS_CODE_DIR = TEST_SKILLS_DIR;
46 process.env.SKILLS_DATA_DIR = TEST_DATA_DIR; 46 process.env.SKILLS_DATA_DIR = TEST_DATA_DIR;
47 process.env.JWT_SECRET = 'test-secret-key-for-integration'; 47 process.env.JWT_SECRET = 'test-secret-key-for-integration';
48 - process.env.API_KEYS = 'test-api-key-123';  
49 48
50 // Clear module cache for clean state 49 // Clear module cache for clean state
51 Object.keys(require.cache).filter(k => 50 Object.keys(require.cache).filter(k =>
server/tests/unit/api-key-auth.test.js
  View file @46c780a
1 /** 1 /**
2 * Unit Tests — middleware/api-key-auth.js 2 * Unit Tests — middleware/api-key-auth.js
3 */ 3 */
4 -import { describe, it, expect } from 'vitest'; 4 +import { describe, it, expect, beforeAll, afterAll } from 'vitest';
5 5
6 const { extractApiKey } = require('../../middleware/api-key-auth'); 6 const { extractApiKey } = require('../../middleware/api-key-auth');
7 7
@@ -50,3 +50,121 @@ describe(&#39;api-key-auth&#39;, () =&gt; { @@ -50,3 +50,121 @@ describe(&#39;api-key-auth&#39;, () =&gt; {
50 }); 50 });
51 }); 51 });
52 }); 52 });
  53 +
  54 +/**
  55 + * requireApiKey —— **做鉴权决定的那一半**。
  56 + *
  57 + * 上面那组 7 条全在测 extractApiKey（怎么从 header/query 里取出字符串），
  58 + * 而真正决定放行还是 401 的 requireApiKey 此前**一条用例都没有**。
  59 + * 后果不是假设：`OPEN_API_KEYS` 那条分支从来没被执行过，于是
  60 + * integration 测试里把变量名写成 `API_KEYS`（生产读的是 `OPEN_API_KEYS`）
  61 + * 存活了很久也没人发现 —— 没有测试执行过的路径，喂给它什么名字都不会有人知道。
  62 + *
  63 + * 四条分支各一条，覆盖「拦」和「放」两侧：
  64 + * 无 key → 401 / key 无效 → 401 / 命中系统 key 列表 → 放行 / 命中用户 key → 放行且挂 req.user
  65 + *
  66 + * 隔离两处，缺任一条这组用例就会打到真实环境：
  67 + * - `OPEN_API_KEYS`：config.js 在模块加载时求值，所以要先设 env 再清 require.cache
  68 + * （config 与本中间件都要清 —— 中间件在顶层 require config）
  69 + * - `USERS_DB_PATH`：requireApiKey 内部**惰性** require user-db 来查用户 key。
  70 + * 这个覆盖口是刚补上的；在它存在之前，这组用例会往**共享的生产用户库**里
  71 + * 插一个测试用户（那个库正是这么攒到 356 行夹具的）。
  72 + */
  73 +describe('requireApiKey — 鉴权判定（此前零覆盖）', () => {
  74 + const path = require('path');
  75 + const fs = require('fs');
  76 +
  77 + const SYSTEM_KEY = 'sk-system-test-key';
  78 + const TEST_DB_PATH = `/tmp/test-apikey-users-${process.pid}.sqlite`;
  79 + const DB_FILES = [TEST_DB_PATH, `${TEST_DB_PATH}-shm`, `${TEST_DB_PATH}-wal`];
  80 + const MODULES = ['../../config', '../../middleware/api-key-auth', '../../services/user-db'];
  81 +
  82 + let requireApiKey;
  83 + let userKey;
  84 + let prevOpenApiKeys;
  85 +
  86 + beforeAll(() => {
  87 + for (const f of DB_FILES) { try { fs.unlinkSync(f); } catch {} }
  88 + prevOpenApiKeys = process.env.OPEN_API_KEYS;
  89 + process.env.OPEN_API_KEYS = `${SYSTEM_KEY}, sk-another-one`; // 带空格：config 会 trim
  90 + process.env.USERS_DB_PATH = TEST_DB_PATH;
  91 + for (const m of MODULES) delete require.cache[require.resolve(m)];
  92 +
  93 + const userDb = require('../../services/user-db');
  94 + const user = userDb.createUser({
  95 + email: `apikey-${process.pid}@example.com`,
  96 + passwordHash: 'x',
  97 + displayName: 'API Key User',
  98 + provider: 'local',
  99 + });
  100 + userKey = user.api_key; // createUser 自己生成 sk-user-<hex>
  101 + expect(userKey).toMatch(/^sk-user-/);
  102 +
  103 + ({ requireApiKey } = require('../../middleware/api-key-auth'));
  104 + });
  105 +
  106 + afterAll(() => {
  107 + for (const f of DB_FILES) { try { fs.unlinkSync(f); } catch {} }
  108 + if (prevOpenApiKeys === undefined) delete process.env.OPEN_API_KEYS;
  109 + else process.env.OPEN_API_KEYS = prevOpenApiKeys;
  110 + delete process.env.USERS_DB_PATH;
  111 + for (const m of MODULES) delete require.cache[require.resolve(m)];
  112 + });
  113 +
  114 + /** 只记录结果，不真起 express */
  115 + function run(headers) {
  116 + const req = { headers, query: {} };
  117 + const res = {
  118 + statusCode: null,
  119 + body: null,
  120 + status(code) { this.statusCode = code; return this; },
  121 + json(payload) { this.body = payload; return this; },
  122 + };
  123 + let passed = false;
  124 + requireApiKey(req, res, () => { passed = true; });
  125 + return { req, res, passed };
  126 + }
  127 +
  128 + it('隔离生效：用的是临时用户库，不是共享的生产库', () => {
  129 + expect(fs.existsSync(TEST_DB_PATH)).toBe(true);
  130 + });
  131 +
  132 + it('① 完全不带 key → 401「需要 key」，且不调 next', () => {
  133 + const { res, passed } = run({});
  134 + expect(passed).toBe(false);
  135 + expect(res.statusCode).toBe(401);
  136 + expect(res.body.error).toBe('Unauthorized');
  137 + // 必须断言到**文案**而不是只断言 401：去掉这道门之后，key=null 会落到
  138 + // 下面那道「key 无效」的门，照样回 401 —— 只看状态码的话这个变异测不出来
  139 + // （实测过：把 if (!key) 改成 if (false)，只断言 401 的版本全绿）。
  140 + // 而两者要分得开：一个是「你忘带 key」，一个是「你的 key 不对」，排查时全靠它。
  141 + expect(res.body.message).toMatch(/API Key is required/);
  142 + });
  143 +
  144 + it('② key 既不在系统列表也不属于任何用户 → 401', () => {
  145 + const { res, passed } = run({ 'x-api-key': 'sk-not-a-real-key' });
  146 + expect(passed).toBe(false);
  147 + expect(res.statusCode).toBe(401);
  148 + // 与「没带 key」是两种不同的失败，文案要分得开（排查时全靠这个）
  149 + expect(res.body.message).toMatch(/Invalid API Key/);
  150 + });
  151 +
  152 + it('③ 命中 OPEN_API_KEYS 系统列表 → 放行，挂 req.apiKey，不挂 req.user', () => {
  153 + // 这条就是那条从来没被执行过的分支：它存在的话，把变量名写成 API_KEYS 会当场红
  154 + const { req, res, passed } = run({ authorization: `Bearer ${SYSTEM_KEY}` });
  155 + expect(passed).toBe(true);
  156 + expect(res.statusCode).toBeNull();
  157 + expect(req.apiKey).toBe(SYSTEM_KEY);
  158 + expect(req.user).toBeUndefined(); // 系统 key 不代表任何用户，不该凭空造一个身份
  159 + });
  160 +
  161 + it('④ 命中库里某个用户的 api_key → 放行并把该用户挂上 req.user', () => {
  162 + const { req, passed } = run({ 'x-api-key': userKey });
  163 + expect(passed).toBe(true);
  164 + expect(req.apiKey).toBe(userKey);
  165 + expect(req.user).toBeTruthy();
  166 + expect(req.user.email).toBe(`apikey-${process.pid}@example.com`);
  167 + // 走的是 toPublicUser，密码哈希不该跟着挂到请求上
  168 + expect(req.user).not.toHaveProperty('password_hash');
  169 + });
  170 +});