From b9c79c81381d6f0de2e099b2885842a50a8a3cf9 Mon Sep 17 00:00:00 2001 From: TIANYAO ZHANG <88520881+ZHANGTIANYAO1@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:17:34 +0800 Subject: [PATCH] fix: revoke API keys on password rotation and audit key owners --- docs/API.md | 15 ++++++-- src/data/api-keys.ts | 9 ++++- src/web/api/api-keys.test.ts | 16 +++++++-- src/web/api/api-keys.ts | 15 ++++---- src/web/api/session.test.ts | 68 ++++++++++++++++++++++++++++++++++-- src/web/api/session.ts | 5 ++- src/web/server.ts | 2 +- 7 files changed, 110 insertions(+), 20 deletions(-) diff --git a/docs/API.md b/docs/API.md index 331db3f..3cb9394 100644 --- a/docs/API.md +++ b/docs/API.md @@ -12,7 +12,15 @@ Authorization: Bearer tsmb_xxxxxxxxxxxx X-API-Key: tsmb_xxxxxxxxxxxx ``` -Key 在 WebUI 设置页创建,权限与所属账户一致。浏览器 session(cookie)也可调用全部端点,两者行为相同。 +Key 在 WebUI 设置页创建,权限与所属账户一致。除标注「仅浏览器 session」的端点外,API Key 与浏览器 session(cookie)使用相同的账户权限。 + +管理员 API Key 保留完整的 REST 管理权限,包括 `/api/users` 的创建用户、重置密码与权限变更;因此也可以创建新的可登录账户。`/api/keys` 的 session 限制只约束直接密钥管理,不能作为管理员 Key 的权限隔离措施。 + +### 密钥吊销与密码变更 + +API Key 没有自动到期时间,可在设置页随时吊销。删除账户会同时删除其全部 Key。成功修改自己的密码或由管理员重置密码,都会吊销该账户的全部 Key;依赖这些 Key 的外部集成需要重新生成并更新凭据。失败的密码变更不会吊销 Key。 + +修改自己的密码会保留当前浏览器 session,使其余 session 失效。管理员重置其他账户的密码会使目标账户的全部 session 失效;重置自己的密码时同样保留当前浏览器 session。 ### 错误格式 @@ -264,6 +272,7 @@ ProfileConfig:`{ avatarEnabled, descriptionEnabled, nicknameEnabled, awayStatusE ```bash curl -X POST -H "X-API-Key: $KEY" -H "x-filename: theme.mp3" \ + -H "Content-Type: application/octet-stream" \ --data-binary @theme.mp3 http://127.0.0.1:3000/api/music/local/upload ``` @@ -330,7 +339,7 @@ curl -X POST -H "X-API-Key: $KEY" -H "x-filename: theme.mp3" \ ## API 密钥管理 /api/keys(仅浏览器 session) -API Key **不能**调用这些端点(403)——泄露的 Key 无法自我复制;游客 session 也被拒绝。浏览器登录后调用。 +API Key **不能直接调用这些密钥管理端点**(403);游客 session 也被拒绝。浏览器登录后调用。管理员 Key 仍保留上文所述的用户管理权限。 | 方法 | 路径 | 参数 | 返回 | |------|------|------|------| @@ -347,7 +356,7 @@ API Key **不能**调用这些端点(403)——泄露的 Key 无法自我复制; | GET | `/` | — | `{ users: [{ id, username, createdAt, role }] }` | | POST | `/` | `{ username, password(≥8位), role: "admin"|"member" }` | `201 { id, username, role }`;重名 409 | | DELETE | `/:id` | — | `204`(级联删除其 session 与 API Key) | -| POST | `/:id/reset-password` | `{ newPassword }` | `204`(该用户的 session 与 API Key 全部失效) | +| POST | `/:id/reset-password` | `{ newPassword }` | `204`(该用户的 API Key 全部失效,session 按上文密码变更规则处理) | | PATCH | `/:id/role` | `{ role: "admin"|"member" }` | `204`(不能降级最后一个管理员) | | GET | `/:id/permissions` | — | `{ capabilities: string[], bots: "all" | string[] }` | | PUT | `/:id/permissions` | `{ capabilities, bots: "all"|string[] }` | `{ success: true }` | diff --git a/src/data/api-keys.ts b/src/data/api-keys.ts index a203a11..db72eb5 100644 --- a/src/data/api-keys.ts +++ b/src/data/api-keys.ts @@ -35,6 +35,7 @@ export interface CreatedApiKey { export interface ApiKeyStore { /** Returns null when the per-user key cap is reached. */ create(userId: string, name: string): CreatedApiKey | null; + findById(id: string): ApiKeyWithUser | null; listForUser(userId: string): ApiKeyRow[]; listAll(): ApiKeyWithUser[]; /** With userId, only deletes a key owned by that user. */ @@ -60,7 +61,9 @@ export function createApiKeyStore(db: Database.Database): ApiKeyStore { ORDER BY k.createdAt DESC` ); const selectByIdStmt = db.prepare( - "SELECT id, userId, name, keyPrefix, createdAt, lastUsedAt FROM api_keys WHERE id = ?" + `SELECT k.id, k.userId, k.name, k.keyPrefix, k.createdAt, k.lastUsedAt, u.username + FROM api_keys k INNER JOIN users u ON u.id = k.userId + WHERE k.id = ?` ); const deleteStmt = db.prepare("DELETE FROM api_keys WHERE id = ?"); const deleteAllForUserStmt = db.prepare("DELETE FROM api_keys WHERE userId = ?"); @@ -91,6 +94,10 @@ export function createApiKeyStore(db: Database.Database): ApiKeyStore { return { key: row, rawKey }; }, + findById(id) { + return (selectByIdStmt.get(id) as ApiKeyWithUser | undefined) ?? null; + }, + listForUser(userId) { return selectForUserStmt.all(userId) as ApiKeyRow[]; }, diff --git a/src/web/api/api-keys.test.ts b/src/web/api/api-keys.test.ts index 7ff4541..7aa985e 100644 --- a/src/web/api/api-keys.test.ts +++ b/src/web/api/api-keys.test.ts @@ -5,7 +5,7 @@ import request from "supertest"; import { createDatabase, type BotDatabase } from "../../data/database.js"; import { createUserStore } from "../../data/users.js"; import { createSessionStore, type SessionStore } from "../../data/sessions.js"; -import { createAuditStore } from "../../data/audit.js"; +import { createAuditStore, type AuditStore } from "../../data/audit.js"; import { createApiKeyStore, MAX_API_KEYS_PER_USER, type ApiKeyStore } from "../../data/api-keys.js"; import { createPermissionStore } from "../../data/permissions.js"; import { createRequireAuth } from "../middleware/requireAuth.js"; @@ -17,6 +17,7 @@ describe("api-keys router", () => { let app: express.Express; let sessions: SessionStore; let apiKeys: ApiKeyStore; + let audit: AuditStore; let adminId: string; let memberId: string; let adminToken: string; @@ -26,7 +27,7 @@ describe("api-keys router", () => { botDb = createDatabase(":memory:"); const users = createUserStore(botDb.db); sessions = createSessionStore(botDb.db); - const audit = createAuditStore(botDb.db); + audit = createAuditStore(botDb.db); const permissions = createPermissionStore(botDb.db); apiKeys = createApiKeyStore(botDb.db); const admin = await users.createUser("alice", "pw-alice", "admin"); @@ -112,11 +113,20 @@ describe("api-keys router", () => { expect(apiKeys.listForUser(adminId)).toHaveLength(1); }); - it("an admin can delete another user's key", async () => { + it("an admin revoking another user's key audits that key's owner", async () => { const { key } = apiKeys.create(memberId, "member-key")!; const res = await asAdmin().delete(`/api/keys/${key.id}`); expect(res.status).toBe(200); expect(apiKeys.listForUser(memberId)).toHaveLength(0); + expect(audit.list(10, 0)).toEqual([ + expect.objectContaining({ + actorId: adminId, + actorUsername: "alice", + targetUserId: memberId, + targetUsername: "bob", + action: "api_key.deleted", + }), + ]); }); it("admin can list all keys with ?all=1, members cannot", async () => { diff --git a/src/web/api/api-keys.ts b/src/web/api/api-keys.ts index c278433..6aa15d7 100644 --- a/src/web/api/api-keys.ts +++ b/src/web/api/api-keys.ts @@ -7,8 +7,8 @@ import type { Logger } from "../../logger.js"; /** * API-key management (list / create / revoke), mounted at /api/keys. - * Only interactive sessions may manage keys: a leaked key must never be able - * to mint its own replacements. + * Only interactive sessions may call these endpoints. Administrator keys + * retain user-management authority through /api/users. */ export function createApiKeysRouter(apiKeys: ApiKeyStore, audit: AuditStore, logger: Logger): Router { const router = Router(); @@ -63,11 +63,8 @@ export function createApiKeysRouter(apiKeys: ApiKeyStore, audit: AuditStore, log // DELETE /api/keys/:id — revoke; members only their own, admins any. router.delete("/:id", (req, res) => { const user = req.user!; - const ok = - user.role === "admin" - ? apiKeys.delete(req.params.id) - : apiKeys.delete(req.params.id, user.id); - if (!ok) { + const key = apiKeys.findById(req.params.id); + if (!key || !apiKeys.delete(key.id, user.role === "admin" ? undefined : user.id)) { res.status(404).json({ error: "API key not found" }); return; } @@ -75,8 +72,8 @@ export function createApiKeysRouter(apiKeys: ApiKeyStore, audit: AuditStore, log audit.record({ actorId: user.id, actorUsername: user.username, - targetUserId: user.id, - targetUsername: user.username, + targetUserId: key.userId, + targetUsername: key.username, action: "api_key.deleted", }); } catch (auditErr) { diff --git a/src/web/api/session.test.ts b/src/web/api/session.test.ts index be51ca9..5e16ab1 100644 --- a/src/web/api/session.test.ts +++ b/src/web/api/session.test.ts @@ -6,6 +6,7 @@ import pino from "pino"; import { createDatabase, type BotDatabase } from "../../data/database.js"; import { createUserStore, type UserStore } from "../../data/users.js"; import { createSessionStore, type SessionStore } from "../../data/sessions.js"; +import { createApiKeyStore, type ApiKeyStore } from "../../data/api-keys.js"; import { createAuditStore } from "../../data/audit.js"; import { createPermissionStore } from "../../data/permissions.js"; import { getDefaultConfig, type GuestModeConfig } from "../../data/config.js"; @@ -13,7 +14,7 @@ import type { GuestPermissions, BotAccess } from "../../data/permissions.js"; import { createSessionRouter } from "./session.js"; import { SESSION_COOKIE_NAME } from "../auth/validateSession.js"; -function makeApp(botDb: BotDatabase, users: UserStore, sessions: SessionStore) { +function makeApp(botDb: BotDatabase, users: UserStore, sessions: SessionStore, apiKeys?: ApiKeyStore) { const app = express(); app.use(express.json()); app.use(cookieParser()); @@ -27,7 +28,8 @@ function makeApp(botDb: BotDatabase, users: UserStore, sessions: SessionStore) { audit, pino({ level: "silent" }), permissions, - () => getDefaultConfig().guestMode + () => getDefaultConfig().guestMode, + apiKeys ) ); return app; @@ -176,6 +178,68 @@ describe("session router", () => { }, 20000); }); +describe("session router — API key revocation", () => { + let botDb: BotDatabase; + let users: UserStore; + let sessions: SessionStore; + let apiKeys: ApiKeyStore; + let app: express.Express; + let userId: string; + let currentCookie: string; + let rawKey: string; + + beforeEach(async () => { + botDb = createDatabase(":memory:"); + users = createUserStore(botDb.db); + sessions = createSessionStore(botDb.db); + apiKeys = createApiKeyStore(botDb.db); + const member = await users.createUser("alice", "old-password", "member"); + userId = member.id; + currentCookie = `${SESSION_COOKIE_NAME}=${sessions.createSession(userId).token}`; + rawKey = apiKeys.create(userId, "integration")!.rawKey; + app = makeApp(botDb, users, sessions, apiKeys); + }); + + afterEach(() => botDb.close()); + + it("successful password change revokes all owned keys and preserves only the active browser session", async () => { + const secondKey = apiKeys.create(userId, "another-integration")!.rawKey; + const otherSession = `${SESSION_COOKIE_NAME}=${sessions.createSession(userId).token}`; + const otherUser = await users.createUser("bob", "other-password", "member"); + const otherUserKey = apiKeys.create(otherUser.id, "other-user-integration")!.rawKey; + + const changed = await request(app).post("/api/session/change-password") + .set("Cookie", currentCookie) + .send({ oldPassword: "old-password", newPassword: "new-password" }); + expect(changed.status).toBe(204); + expect(apiKeys.validateAndTouch(rawKey)).toBeNull(); + expect(apiKeys.validateAndTouch(secondKey)).toBeNull(); + expect(apiKeys.listForUser(userId)).toEqual([]); + expect(apiKeys.validateAndTouch(otherUserKey)?.userId).toBe(otherUser.id); + expect((await request(app).get("/api/session/me").set("Cookie", currentCookie)).status).toBe(200); + expect((await request(app).get("/api/session/me").set("Cookie", otherSession)).status).toBe(401); + }, 20_000); + + it.each([ + { oldPassword: "wrong-password", newPassword: "new-password", status: 401 }, + { oldPassword: "old-password", newPassword: "short", status: 400 }, + ])("failed password change ($status) leaves API keys valid", async ({ oldPassword, newPassword, status }) => { + const changed = await request(app).post("/api/session/change-password") + .set("Cookie", currentCookie) + .send({ oldPassword, newPassword }); + expect(changed.status).toBe(status); + expect(apiKeys.validateAndTouch(rawKey)?.userId).toBe(userId); + expect((await request(app).get("/api/session/me").set("Cookie", currentCookie)).status).toBe(200); + }); + + it("unauthenticated password change leaves API keys valid", async () => { + const changed = await request(app).post("/api/session/change-password") + .send({ oldPassword: "old-password", newPassword: "new-password" }); + expect(changed.status).toBe(401); + expect(apiKeys.validateAndTouch(rawKey)?.userId).toBe(userId); + }); +}); + describe("session router — guest mode", () => { let botDb: BotDatabase; diff --git a/src/web/api/session.ts b/src/web/api/session.ts index 4f4d5a0..5c56a02 100644 --- a/src/web/api/session.ts +++ b/src/web/api/session.ts @@ -3,6 +3,7 @@ import type { Request, Response, NextFunction } from "express"; import type { Logger } from "../../logger.js"; import type { UserStore } from "../../data/users.js"; import type { SessionStore } from "../../data/sessions.js"; +import type { ApiKeyStore } from "../../data/api-keys.js"; import type { AuditStore } from "../../data/audit.js"; import { resolvePermissionContext, type PermissionStore } from "../../data/permissions.js"; import { SESSION_TTL_MS, GUEST_SESSION_TTL_MS } from "../../data/sessions.js"; @@ -54,7 +55,8 @@ export function createSessionRouter( audit: AuditStore, logger: Logger, permissions: PermissionStore, - getGuestConfig: () => GuestModeConfig + getGuestConfig: () => GuestModeConfig, + apiKeys?: ApiKeyStore ): Router { const router = Router(); @@ -202,6 +204,7 @@ export function createSessionRouter( await users.changePassword(u.id, newPassword); const currentToken = parseTokenFromCookie(req.headers.cookie); sessions.deleteAllForUser(u.id, currentToken ?? undefined); + apiKeys?.deleteAllForUser(u.id); try { audit.record({ actorId: u.id, actorUsername: u.username, diff --git a/src/web/server.ts b/src/web/server.ts index f46df14..6dadb43 100755 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -131,7 +131,7 @@ export function createWebServer(options: WebServerOptions): WebServer { app.use("/api/session/login", loginLimit); app.use("/api/session/setup", setupLimit); - app.use("/api/session", createSessionRouter(users, sessions, audit, logger, permissions, () => options.config.guestMode)); + app.use("/api/session", createSessionRouter(users, sessions, audit, logger, permissions, () => options.config.guestMode, apiKeys)); // ─── Gates for everything else under /api ─────────────────────────────── const requireAuth = createRequireAuth(sessions, permissions, () => options.config.guestMode, apiKeys);