fix(GRO-634): atomic confirmation token in book.ts, correct RBAC error message
- Replace SELECT-then-UPDATE with atomic UPDATE ... WHERE token=? AND status='pending' RETURNING * to prevent confirmation token replay attacks (TOCTOU race condition) - Fix requireRoleOrSuperUser() error message: swap the conditional branches so 'Forbidden: super user privileges required' is returned when user lacks role, and 'Forbidden: role X is not permitted' when user is not superuser - Add 'and' mock export to confirmation.test.ts and rbac.test.ts for new query patterns - Update test expectations to match corrected error message semantics
This commit is contained in:
@@ -68,6 +68,7 @@ vi.mock("@groombook/db", () => {
|
|||||||
}),
|
}),
|
||||||
appointments,
|
appointments,
|
||||||
eq: () => ({}),
|
eq: () => ({}),
|
||||||
|
and: (...clauses: unknown[]) => ({}),
|
||||||
};
|
};
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -78,6 +78,7 @@ vi.mock("@groombook/db", () => {
|
|||||||
}),
|
}),
|
||||||
staff,
|
staff,
|
||||||
eq: vi.fn((_col: unknown, _val: unknown) => ({ col: _col, val: _val })),
|
eq: vi.fn((_col: unknown, _val: unknown) => ({ col: _col, val: _val })),
|
||||||
|
and: vi.fn((..._clauses: unknown[]) => ({})),
|
||||||
};
|
};
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -362,7 +363,7 @@ describe("requireRoleOrSuperUser", () => {
|
|||||||
const res = await app.request("/test");
|
const res = await app.request("/test");
|
||||||
expect(res.status).toBe(403);
|
expect(res.status).toBe(403);
|
||||||
const body = await res.json();
|
const body = await res.json();
|
||||||
expect(body.error).toMatch(/super user privileges required/i);
|
expect(body.error).toMatch(/role.*not permitted/i);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("blocks a non-super-user groomer from manager-only routes", async () => {
|
it("blocks a non-super-user groomer from manager-only routes", async () => {
|
||||||
@@ -370,7 +371,7 @@ describe("requireRoleOrSuperUser", () => {
|
|||||||
const res = await app.request("/test");
|
const res = await app.request("/test");
|
||||||
expect(res.status).toBe(403);
|
expect(res.status).toBe(403);
|
||||||
const body = await res.json();
|
const body = await res.json();
|
||||||
expect(body.error).toMatch(/super user privileges required/i);
|
expect(body.error).toMatch(/role.*not permitted/i);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("allows a manager with multiple allowed roles", async () => {
|
it("allows a manager with multiple allowed roles", async () => {
|
||||||
|
|||||||
@@ -149,9 +149,9 @@ export function requireRoleOrSuperUser(
|
|||||||
}
|
}
|
||||||
return c.json(
|
return c.json(
|
||||||
{
|
{
|
||||||
error: staffRow.isSuperUser
|
error: hasAllowedRole
|
||||||
? `Forbidden: role '${staffRow.role}' is not permitted`
|
? "Forbidden: super user privileges required"
|
||||||
: "Forbidden: super user privileges required",
|
: `Forbidden: role '${staffRow.role}' is not permitted`,
|
||||||
},
|
},
|
||||||
403
|
403
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -277,14 +277,24 @@ bookRouter.get("/confirm/:token", async (c) => {
|
|||||||
return c.redirect(`${BASE_URL()}/booking/error`);
|
return c.redirect(`${BASE_URL()}/booking/error`);
|
||||||
}
|
}
|
||||||
|
|
||||||
await db
|
const updated = await db
|
||||||
.update(appointments)
|
.update(appointments)
|
||||||
.set({
|
.set({
|
||||||
confirmationStatus: "confirmed",
|
confirmationStatus: "confirmed",
|
||||||
confirmedAt: new Date(),
|
confirmedAt: new Date(),
|
||||||
updatedAt: new Date(),
|
updatedAt: new Date(),
|
||||||
})
|
})
|
||||||
.where(eq(appointments.id, appt.id));
|
.where(
|
||||||
|
and(
|
||||||
|
eq(appointments.confirmationToken, token),
|
||||||
|
eq(appointments.confirmationStatus, "pending")
|
||||||
|
)
|
||||||
|
)
|
||||||
|
.returning();
|
||||||
|
|
||||||
|
if (updated.length === 0) {
|
||||||
|
return c.redirect(`${BASE_URL()}/booking/error`);
|
||||||
|
}
|
||||||
|
|
||||||
return c.redirect(`${BASE_URL()}/booking/confirmed`);
|
return c.redirect(`${BASE_URL()}/booking/confirmed`);
|
||||||
});
|
});
|
||||||
@@ -314,7 +324,7 @@ bookRouter.get("/cancel/:token", async (c) => {
|
|||||||
return c.redirect(`${BASE_URL()}/booking/error`);
|
return c.redirect(`${BASE_URL()}/booking/error`);
|
||||||
}
|
}
|
||||||
|
|
||||||
await db
|
const updated = await db
|
||||||
.update(appointments)
|
.update(appointments)
|
||||||
.set({
|
.set({
|
||||||
confirmationStatus: "cancelled",
|
confirmationStatus: "cancelled",
|
||||||
@@ -322,7 +332,17 @@ bookRouter.get("/cancel/:token", async (c) => {
|
|||||||
confirmationToken: null,
|
confirmationToken: null,
|
||||||
updatedAt: new Date(),
|
updatedAt: new Date(),
|
||||||
})
|
})
|
||||||
.where(eq(appointments.id, appt.id));
|
.where(
|
||||||
|
and(
|
||||||
|
eq(appointments.confirmationToken, token),
|
||||||
|
eq(appointments.confirmationStatus, "pending")
|
||||||
|
)
|
||||||
|
)
|
||||||
|
.returning();
|
||||||
|
|
||||||
|
if (updated.length === 0) {
|
||||||
|
return c.redirect(`${BASE_URL()}/booking/error`);
|
||||||
|
}
|
||||||
|
|
||||||
return c.redirect(`${BASE_URL()}/booking/cancelled`);
|
return c.redirect(`${BASE_URL()}/booking/cancelled`);
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user