mirror of
https://github.com/ZHANGTIANYAO1/teamspeak-music-bot.git
synced 2026-10-02 04:52:50 +08:00
fix(guest): guard reserved __guest__ principal in user mgmt
The by-id user-management handlers use findById, which has no role filter, so supplying the synthetic GUEST_USER_ID let an admin delete, re-role, reset-password, and read/write permissions on the shared guest principal (privilege-escalation / DoS / credential-login holes). - web/api/users.ts: 404-guard every :id handler against GUEST_USER_ID (DELETE, reset-password, role, GET/PUT permissions). - data/users.ts: defense-in-depth — setRoleIfNotLastAdmin and deleteUserIfNotLastAdmin return "not_found" for any role=guest row. - web/api/session.ts: wrap POST /guest createSession in try/catch so a missing guest row yields 503 instead of an unhandled 500. - Tests: data-layer guest-protection + users-router 404 by-id guards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
45414b3baa
commit
365352cdd3
5 files changed
+79
-3
No files matched your search
@@ -208,4 +208,19 @@ describe("guest row exclusion", () => {
|
|||||||
expect(users.countUsers()).toBe(1); // alice only
|
expect(users.countUsers()).toBe(1); // alice only
|
||||||
expect(users.listUsers().some((u) => u.id === GUEST_USER_ID)).toBe(false);
|
expect(users.listUsers().some((u) => u.id === GUEST_USER_ID)).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("setRoleIfNotLastAdmin refuses to re-role the reserved guest principal", () => {
|
||||||
|
// The guest row is seeded by createDatabase via ensureGuestUser.
|
||||||
|
expect(users.findById(GUEST_USER_ID)!.role).toBe("guest"); // sanity
|
||||||
|
expect(users.setRoleIfNotLastAdmin(GUEST_USER_ID, "admin")).toBe("not_found");
|
||||||
|
// The guest row's role is unchanged.
|
||||||
|
expect(users.findById(GUEST_USER_ID)!.role).toBe("guest");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("deleteUserIfNotLastAdmin refuses to delete the reserved guest principal", () => {
|
||||||
|
expect(users.findById(GUEST_USER_ID)).not.toBeNull(); // sanity
|
||||||
|
expect(users.deleteUserIfNotLastAdmin(GUEST_USER_ID)).toBe("not_found");
|
||||||
|
// The guest row still exists.
|
||||||
|
expect(users.findById(GUEST_USER_ID)).not.toBeNull();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
@@ -137,6 +137,7 @@ export function createUserStore(db: Database.Database): UserStore {
|
|||||||
const tx = db.transaction(() => {
|
const tx = db.transaction(() => {
|
||||||
const row = findByIdStmt.get(id) as UserRow | undefined;
|
const row = findByIdStmt.get(id) as UserRow | undefined;
|
||||||
if (!row) return "not_found" as const;
|
if (!row) return "not_found" as const;
|
||||||
|
if (row.role === "guest") return "not_found" as const; // reserved synthetic principal
|
||||||
if (row.role === newRole) return "ok" as const; // no-op
|
if (row.role === newRole) return "ok" as const; // no-op
|
||||||
if (row.role === "admin" && newRole === "member") {
|
if (row.role === "admin" && newRole === "member") {
|
||||||
const adminCount = (countAdminsStmt.get() as { n: number }).n;
|
const adminCount = (countAdminsStmt.get() as { n: number }).n;
|
||||||
@@ -161,6 +162,7 @@ export function createUserStore(db: Database.Database): UserStore {
|
|||||||
const tx = db.transaction(() => {
|
const tx = db.transaction(() => {
|
||||||
const row = findByIdStmt.get(id) as UserRow | undefined;
|
const row = findByIdStmt.get(id) as UserRow | undefined;
|
||||||
if (!row) return "not_found" as const;
|
if (!row) return "not_found" as const;
|
||||||
|
if (row.role === "guest") return "not_found" as const; // reserved synthetic principal
|
||||||
if (row.role === "admin") {
|
if (row.role === "admin") {
|
||||||
const adminCount = (countAdminsStmt.get() as { n: number }).n;
|
const adminCount = (countAdminsStmt.get() as { n: number }).n;
|
||||||
if (adminCount <= 1) return "would_orphan" as const;
|
if (adminCount <= 1) return "would_orphan" as const;
|
||||||
|
|||||||
+10
-1
@@ -141,7 +141,16 @@ export function createSessionRouter(
|
|||||||
res.status(403).json({ error: "guest mode disabled" });
|
res.status(403).json({ error: "guest mode disabled" });
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
const { token } = sessions.createSession(GUEST_USER_ID, { ttlMs: GUEST_SESSION_TTL_MS, skipCap: true });
|
let token: string;
|
||||||
|
try {
|
||||||
|
// If the reserved guest row is somehow missing, the session FK would
|
||||||
|
// throw; surface a clean 503 rather than letting it become a 500.
|
||||||
|
({ token } = sessions.createSession(GUEST_USER_ID, { ttlMs: GUEST_SESSION_TTL_MS, skipCap: true }));
|
||||||
|
} catch (err) {
|
||||||
|
logger.error({ err }, "guest session creation failed");
|
||||||
|
res.status(503).json({ error: "guest unavailable" });
|
||||||
|
return;
|
||||||
|
}
|
||||||
setSessionCookie(res, token);
|
setSessionCookie(res, token);
|
||||||
res.json({ id: GUEST_USER_ID, username: GUEST_USERNAME, role: "guest" });
|
res.json({ id: GUEST_USER_ID, username: GUEST_USERNAME, role: "guest" });
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -4,7 +4,7 @@ import cookieParser from "cookie-parser";
|
|||||||
import request from "supertest";
|
import request from "supertest";
|
||||||
import pino from "pino";
|
import pino from "pino";
|
||||||
import { createDatabase, type BotDatabase } from "../../data/database.js";
|
import { createDatabase, type BotDatabase } from "../../data/database.js";
|
||||||
import { createUserStore, type UserStore } from "../../data/users.js";
|
import { createUserStore, GUEST_USER_ID, type UserStore } from "../../data/users.js";
|
||||||
import { createSessionStore, type SessionStore } from "../../data/sessions.js";
|
import { createSessionStore, type SessionStore } from "../../data/sessions.js";
|
||||||
import { createAuditStore, type AuditStore } from "../../data/audit.js";
|
import { createAuditStore, type AuditStore } from "../../data/audit.js";
|
||||||
import { createPermissionStore, type PermissionStore } from "../../data/permissions.js";
|
import { createPermissionStore, type PermissionStore } from "../../data/permissions.js";
|
||||||
@@ -342,4 +342,49 @@ describe("users router", () => {
|
|||||||
expect(perms.status).toBe(200);
|
expect(perms.status).toBe(200);
|
||||||
expect(perms.body).toEqual({ capabilities: [], bots: [] });
|
expect(perms.body).toEqual({ capabilities: [], bots: [] });
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// --- reserved guest principal is never mutable/visible via user mgmt -------
|
||||||
|
// The synthetic __guest__ row is seeded by createDatabase. findById has no
|
||||||
|
// role filter, so without the 404-guard these by-id handlers would operate
|
||||||
|
// on it (privilege-escalation / DoS / cred-login holes).
|
||||||
|
describe("reserved __guest__ principal is 404 on every by-id handler", () => {
|
||||||
|
it("DELETE /:id → 404 and the guest row survives", async () => {
|
||||||
|
const res = await request(app).delete(`/api/users/${GUEST_USER_ID}`).set("Cookie", aliceCookie);
|
||||||
|
expect(res.status).toBe(404);
|
||||||
|
expect(users.findById(GUEST_USER_ID)).not.toBeNull();
|
||||||
|
expect(users.findById(GUEST_USER_ID)!.role).toBe("guest");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("PATCH /:id/role {role:'admin'} → 404 and the guest role is unchanged", async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.patch(`/api/users/${GUEST_USER_ID}/role`)
|
||||||
|
.set("Cookie", aliceCookie)
|
||||||
|
.send({ role: "admin" });
|
||||||
|
expect(res.status).toBe(404);
|
||||||
|
expect(users.findById(GUEST_USER_ID)!.role).toBe("guest");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("POST /:id/reset-password → 404 (cannot give the guest a login)", async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.post(`/api/users/${GUEST_USER_ID}/reset-password`)
|
||||||
|
.set("Cookie", aliceCookie)
|
||||||
|
.send({ newPassword: "guest-new-pw" });
|
||||||
|
expect(res.status).toBe(404);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("GET /:id/permissions → 404", async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.get(`/api/users/${GUEST_USER_ID}/permissions`)
|
||||||
|
.set("Cookie", aliceCookie);
|
||||||
|
expect(res.status).toBe(404);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("PUT /:id/permissions → 404", async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.put(`/api/users/${GUEST_USER_ID}/permissions`)
|
||||||
|
.set("Cookie", aliceCookie)
|
||||||
|
.send({ capabilities: ["player.control"], bots: "all" });
|
||||||
|
expect(res.status).toBe(404);
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
@@ -1,7 +1,7 @@
|
|||||||
import { Router } from "express";
|
import { Router } from "express";
|
||||||
import type { Logger } from "../../logger.js";
|
import type { Logger } from "../../logger.js";
|
||||||
import type { UserStore } from "../../data/users.js";
|
import type { UserStore } from "../../data/users.js";
|
||||||
import { UsernameTakenError } from "../../data/users.js";
|
import { UsernameTakenError, GUEST_USER_ID } from "../../data/users.js";
|
||||||
import type { SessionStore } from "../../data/sessions.js";
|
import type { SessionStore } from "../../data/sessions.js";
|
||||||
import type { AuditStore } from "../../data/audit.js";
|
import type { AuditStore } from "../../data/audit.js";
|
||||||
import { isCapability, BASIC_TIER_CAPABILITIES, type PermissionStore } from "../../data/permissions.js";
|
import { isCapability, BASIC_TIER_CAPABILITIES, type PermissionStore } from "../../data/permissions.js";
|
||||||
@@ -63,6 +63,7 @@ export function createUsersRouter(
|
|||||||
|
|
||||||
router.delete("/:id", (req, res) => {
|
router.delete("/:id", (req, res) => {
|
||||||
const targetId = req.params.id;
|
const targetId = req.params.id;
|
||||||
|
if (targetId === GUEST_USER_ID) { res.status(404).json({ error: "not found" }); return; }
|
||||||
// Snapshot target's username BEFORE deletion for audit
|
// Snapshot target's username BEFORE deletion for audit
|
||||||
const target = users.findById(targetId);
|
const target = users.findById(targetId);
|
||||||
if (!target) {
|
if (!target) {
|
||||||
@@ -104,6 +105,7 @@ export function createUsersRouter(
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
const targetId = req.params.id;
|
const targetId = req.params.id;
|
||||||
|
if (targetId === GUEST_USER_ID) { res.status(404).json({ error: "not found" }); return; }
|
||||||
const target = users.findById(targetId);
|
const target = users.findById(targetId);
|
||||||
if (!target) {
|
if (!target) {
|
||||||
res.status(404).json({ error: "not found" });
|
res.status(404).json({ error: "not found" });
|
||||||
@@ -130,6 +132,7 @@ export function createUsersRouter(
|
|||||||
|
|
||||||
router.patch("/:id/role", (req, res) => {
|
router.patch("/:id/role", (req, res) => {
|
||||||
const targetId = req.params.id;
|
const targetId = req.params.id;
|
||||||
|
if (targetId === GUEST_USER_ID) { res.status(404).json({ error: "not found" }); return; }
|
||||||
const { role: newRole } = req.body ?? {};
|
const { role: newRole } = req.body ?? {};
|
||||||
if (newRole !== "admin" && newRole !== "member") {
|
if (newRole !== "admin" && newRole !== "member") {
|
||||||
res.status(400).json({ error: "invalid role" });
|
res.status(400).json({ error: "invalid role" });
|
||||||
@@ -168,6 +171,7 @@ export function createUsersRouter(
|
|||||||
});
|
});
|
||||||
|
|
||||||
router.get("/:id/permissions", (req, res) => {
|
router.get("/:id/permissions", (req, res) => {
|
||||||
|
if (req.params.id === GUEST_USER_ID) { res.status(404).json({ error: "not found" }); return; }
|
||||||
const user = users.findById(req.params.id);
|
const user = users.findById(req.params.id);
|
||||||
if (!user) {
|
if (!user) {
|
||||||
res.status(404).json({ error: "not_found" });
|
res.status(404).json({ error: "not_found" });
|
||||||
@@ -180,6 +184,7 @@ export function createUsersRouter(
|
|||||||
});
|
});
|
||||||
|
|
||||||
router.put("/:id/permissions", (req, res) => {
|
router.put("/:id/permissions", (req, res) => {
|
||||||
|
if (req.params.id === GUEST_USER_ID) { res.status(404).json({ error: "not found" }); return; }
|
||||||
const user = users.findById(req.params.id);
|
const user = users.findById(req.params.id);
|
||||||
if (!user) {
|
if (!user) {
|
||||||
res.status(404).json({ error: "not_found" });
|
res.status(404).json({ error: "not_found" });
|
||||||
|
|||||||
Reference in new issue
Block a user