fix(auth): defensive audit-record + self-reset preserves current session

This commit is contained in:
saopig1 committed 2026-05-27 15:16:55 +08:00
1 parent 7be4f13774
commit b0b61f8fce
3 files changed
+101 -26

No files matched your search

+18 -10
View File
@@ -88,11 +88,15 @@ export function createSessionRouter(
} }
const { token } = sessions.createSession(user.id); const { token } = sessions.createSession(user.id);
setSessionCookie(res, token); setSessionCookie(res, token);
audit.record({ try {
actorId: user.id, actorUsername: user.username, audit.record({
targetUserId: user.id, targetUsername: user.username, actorId: user.id, actorUsername: user.username,
action: "admin.first_created", targetUserId: user.id, targetUsername: user.username,
}); action: "admin.first_created",
});
} catch (auditErr) {
logger.warn({ err: auditErr, action: "admin.first_created" }, "audit insert failed");
}
logger.info({ userId: user.id, username }, "First admin created"); logger.info({ userId: user.id, username }, "First admin created");
res.json({ id: user.id, username: user.username }); res.json({ id: user.id, username: user.username });
} catch (err) { } catch (err) {
@@ -151,11 +155,15 @@ export function createSessionRouter(
await users.changePassword(u.id, newPassword); await users.changePassword(u.id, newPassword);
const currentToken = parseTokenFromCookie(req.headers.cookie); const currentToken = parseTokenFromCookie(req.headers.cookie);
sessions.deleteAllForUser(u.id, currentToken ?? undefined); sessions.deleteAllForUser(u.id, currentToken ?? undefined);
audit.record({ try {
actorId: u.id, actorUsername: u.username, audit.record({
targetUserId: u.id, targetUsername: u.username, actorId: u.id, actorUsername: u.username,
action: "user.password_changed", targetUserId: u.id, targetUsername: u.username,
}); action: "user.password_changed",
});
} catch (auditErr) {
logger.warn({ err: auditErr, action: "user.password_changed" }, "audit insert failed");
}
res.status(204).end(); res.status(204).end();
}); });
+52
View File
@@ -137,4 +137,56 @@ describe("users router", () => {
.send({ newPassword: "short" }); .send({ newPassword: "short" });
expect(res.status).toBe(400); expect(res.status).toBe(400);
}); });
it("returns 201 even if audit insert fails (POST /api/users)", async () => {
// Build a broken audit store that throws on record()
const brokenAudit = {
record: () => { throw new Error("simulated disk-full"); },
list: () => [],
};
// Reassemble app with the broken audit
const localApp = express();
localApp.use(express.json());
localApp.use(cookieParser());
localApp.use("/api", createRequireAuth(sessions));
localApp.use(
"/api/users",
createUsersRouter(users, sessions, brokenAudit, pino({ level: "silent" }))
);
const res = await request(localApp)
.post("/api/users")
.set("Cookie", aliceCookie)
.send({ username: "charlie", password: "charlie-pw" });
expect(res.status).toBe(201);
expect(users.countUsers()).toBe(3);
});
it("POST /:id/reset-password on self preserves the actor's current session", async () => {
// Alice resets her OWN password
const res = await request(app)
.post(`/api/users/${aliceId}/reset-password`)
.set("Cookie", aliceCookie)
.send({ newPassword: "alice-new-pw" });
expect(res.status).toBe(204);
// Alice's CURRENT session should still work
// (we'd need a protected endpoint to verify; use GET /api/users which is already mounted)
const followUp = await request(app).get("/api/users").set("Cookie", aliceCookie);
expect(followUp.status).toBe(200);
// The password hash IS updated (sanity check)
const alice = users.findById(aliceId);
expect(await users.verifyPassword("alice-new-pw", alice!.passwordHash)).toBe(true);
});
it("POST /:id/reset-password on another user does NOT preserve any of target's sessions", async () => {
const bobToken = sessions.createSession(bobId).token;
const res = await request(app)
.post(`/api/users/${bobId}/reset-password`)
.set("Cookie", aliceCookie)
.send({ newPassword: "bob-new-pw" });
expect(res.status).toBe(204);
// Bob's session should be dead
expect(sessions.validateAndTouch(bobToken)).toBeNull();
});
}); });
+31 -16
View File
@@ -4,6 +4,7 @@ import type { UserStore } from "../../data/users.js";
import { UsernameTakenError } from "../../data/users.js"; import { UsernameTakenError } 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 { extractSessionToken } from "../auth/validateSession.js";
function isValidUsername(v: unknown): v is string { function isValidUsername(v: unknown): v is string {
return typeof v === "string" && /^[A-Za-z0-9_\-.]{3,32}$/.test(v); return typeof v === "string" && /^[A-Za-z0-9_\-.]{3,32}$/.test(v);
@@ -33,11 +34,15 @@ export function createUsersRouter(
} }
try { try {
const u = await users.createUser(username, password); const u = await users.createUser(username, password);
audit.record({ try {
actorId: req.user!.id, actorUsername: req.user!.username, audit.record({
targetUserId: u.id, targetUsername: u.username, actorId: req.user!.id, actorUsername: req.user!.username,
action: "user.created", targetUserId: u.id, targetUsername: u.username,
}); action: "user.created",
});
} catch (auditErr) {
logger.warn({ err: auditErr, action: "user.created" }, "audit insert failed");
}
logger.info({ createdBy: req.user!.id, newUserId: u.id, username }, "User created"); logger.info({ createdBy: req.user!.id, newUserId: u.id, username }, "User created");
res.status(201).json({ id: u.id, username: u.username }); res.status(201).json({ id: u.id, username: u.username });
} catch (err) { } catch (err) {
@@ -67,11 +72,15 @@ export function createUsersRouter(
return; return;
} }
sessions.deleteAllForUser(targetId); sessions.deleteAllForUser(targetId);
audit.record({ try {
actorId: req.user!.id, actorUsername: req.user!.username, audit.record({
targetUserId: target.id, targetUsername: target.username, actorId: req.user!.id, actorUsername: req.user!.username,
action: "user.deleted", targetUserId: target.id, targetUsername: target.username,
}); action: "user.deleted",
});
} catch (auditErr) {
logger.warn({ err: auditErr, action: "user.deleted" }, "audit insert failed");
}
logger.info({ deletedBy: req.user!.id, deletedUserId: targetId }, "User deleted"); logger.info({ deletedBy: req.user!.id, deletedUserId: targetId }, "User deleted");
res.status(204).end(); res.status(204).end();
}); });
@@ -90,13 +99,19 @@ export function createUsersRouter(
} }
await users.changePassword(targetId, newPassword); await users.changePassword(targetId, newPassword);
// Invalidate all sessions for the target user (except current actor's if it's the same user) // Invalidate all sessions for the target user (except current actor's if it's the same user)
const exceptToken = targetId === req.user!.id ? undefined : undefined; const exceptToken = targetId === req.user!.id
? (extractSessionToken(req.headers.cookie) ?? undefined)
: undefined;
sessions.deleteAllForUser(targetId, exceptToken); sessions.deleteAllForUser(targetId, exceptToken);
audit.record({ try {
actorId: req.user!.id, actorUsername: req.user!.username, audit.record({
targetUserId: target.id, targetUsername: target.username, actorId: req.user!.id, actorUsername: req.user!.username,
action: "user.password_reset", targetUserId: target.id, targetUsername: target.username,
}); action: "user.password_reset",
});
} catch (auditErr) {
logger.warn({ err: auditErr, action: "user.password_reset" }, "audit insert failed");
}
logger.info({ resetBy: req.user!.id, targetUserId: targetId }, "Password reset"); logger.info({ resetBy: req.user!.id, targetUserId: targetId }, "Password reset");
res.status(204).end(); res.status(204).end();
}); });