mirror of
https://github.com/open-reception/appointment-booking-software.git
synced 2026-08-17 21:25:52 +02:00
Merge commit from fork
Fix two security advisories
This commit is contained in:
@@ -234,8 +234,6 @@ export const userInvite = pgTable(
|
|||||||
export const challengeThrottle = pgTable("challenge_throttle", {
|
export const challengeThrottle = pgTable("challenge_throttle", {
|
||||||
/** Primary key - identifier (email hash for PIN challenges, email for passkey challenges) */
|
/** Primary key - identifier (email hash for PIN challenges, email for passkey challenges) */
|
||||||
id: text("id").primaryKey(),
|
id: text("id").primaryKey(),
|
||||||
/** Tenant ID for scoping throttles to single tenants to restrict global lock-out. Might be null for global throttles on administrative accounts */
|
|
||||||
tenantId: uuid("tenant_id").references(() => tenant.id, { onDelete: "cascade" }),
|
|
||||||
/** Number of failed attempts */
|
/** Number of failed attempts */
|
||||||
failedAttempts: integer("failed_attempts").default(0).notNull(),
|
failedAttempts: integer("failed_attempts").default(0).notNull(),
|
||||||
/** When the throttle was last updated */
|
/** When the throttle was last updated */
|
||||||
|
|||||||
@@ -852,11 +852,7 @@ describe("AppointmentService", () => {
|
|||||||
);
|
);
|
||||||
|
|
||||||
expect(challengeStore.consume).toHaveBeenCalledWith("challenge-123", "tenant-123");
|
expect(challengeStore.consume).toHaveBeenCalledWith("challenge-123", "tenant-123");
|
||||||
expect(challengeThrottleService.clearThrottle).toHaveBeenCalledWith(
|
expect(challengeThrottleService.clearThrottle).toHaveBeenCalledWith("email-hash-123", "pin");
|
||||||
"email-hash-123",
|
|
||||||
"pin",
|
|
||||||
"tenant-123",
|
|
||||||
);
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should throw NotFoundError when challenge is not found", async () => {
|
it("should throw NotFoundError when challenge is not found", async () => {
|
||||||
@@ -925,7 +921,6 @@ describe("AppointmentService", () => {
|
|||||||
expect(challengeThrottleService.recordFailedAttempt).toHaveBeenCalledWith(
|
expect(challengeThrottleService.recordFailedAttempt).toHaveBeenCalledWith(
|
||||||
"email-hash-123",
|
"email-hash-123",
|
||||||
"pin",
|
"pin",
|
||||||
"tenant-123",
|
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -6,12 +6,36 @@ import { NotFoundError, ValidationError } from "../../utils/errors";
|
|||||||
vi.mock("../../db", () => ({
|
vi.mock("../../db", () => ({
|
||||||
centralDb: {
|
centralDb: {
|
||||||
insert: vi.fn(),
|
insert: vi.fn(),
|
||||||
select: vi.fn(),
|
select: vi.fn(() => ({
|
||||||
|
from: vi.fn(() => ({
|
||||||
|
where: vi.fn(),
|
||||||
|
})),
|
||||||
|
})),
|
||||||
update: vi.fn(),
|
update: vi.fn(),
|
||||||
delete: vi.fn(),
|
delete: vi.fn(),
|
||||||
transaction: vi.fn(),
|
transaction: vi.fn(),
|
||||||
limit: vi.fn(),
|
limit: vi.fn(),
|
||||||
},
|
},
|
||||||
|
db: {
|
||||||
|
insert: vi.fn(),
|
||||||
|
select: vi.fn(() => ({
|
||||||
|
from: vi.fn(() => ({
|
||||||
|
where: vi.fn(() => ({
|
||||||
|
limit: vi.fn(),
|
||||||
|
})),
|
||||||
|
})),
|
||||||
|
})),
|
||||||
|
update: vi.fn(() => ({
|
||||||
|
from: vi.fn(() => ({
|
||||||
|
where: vi.fn(() => ({
|
||||||
|
set: vi.fn(),
|
||||||
|
})),
|
||||||
|
})),
|
||||||
|
})),
|
||||||
|
delete: vi.fn(),
|
||||||
|
transaction: vi.fn(),
|
||||||
|
limit: vi.fn(),
|
||||||
|
},
|
||||||
}));
|
}));
|
||||||
|
|
||||||
// Mock the email service
|
// Mock the email service
|
||||||
@@ -41,17 +65,19 @@ import { UserService } from "../user-service";
|
|||||||
|
|
||||||
describe("UserService", () => {
|
describe("UserService", () => {
|
||||||
let mockCentralDb: any;
|
let mockCentralDb: any;
|
||||||
|
let mockDb: any;
|
||||||
let mockUuidv7: any;
|
let mockUuidv7: any;
|
||||||
let mockAddMinutes: any;
|
let mockAddMinutes: any;
|
||||||
let mockSendConfirmationEmail: any;
|
let mockSendConfirmationEmail: any;
|
||||||
let mockTenantAdminService: any;
|
let mockTenantAdminService: any;
|
||||||
|
|
||||||
beforeEach(async () => {
|
beforeEach(async () => {
|
||||||
vi.clearAllMocks();
|
vi.resetAllMocks();
|
||||||
|
|
||||||
// Get mocked modules
|
// Get mocked modules
|
||||||
const dbModule = await vi.importMock("../../db");
|
const dbModule = await vi.importMock("../../db");
|
||||||
mockCentralDb = dbModule.centralDb;
|
mockCentralDb = dbModule.centralDb;
|
||||||
|
mockDb = dbModule.db;
|
||||||
|
|
||||||
const uuidModule = await vi.importMock("uuidv7");
|
const uuidModule = await vi.importMock("uuidv7");
|
||||||
mockUuidv7 = uuidModule.uuidv7;
|
mockUuidv7 = uuidModule.uuidv7;
|
||||||
@@ -226,6 +252,14 @@ describe("UserService", () => {
|
|||||||
execute: vi.fn().mockResolvedValue({ count: 1 }),
|
execute: vi.fn().mockResolvedValue({ count: 1 }),
|
||||||
};
|
};
|
||||||
|
|
||||||
|
const mockInviteUpdateBuilder: any = {
|
||||||
|
set: vi.fn().mockReturnThis(),
|
||||||
|
where: vi.fn().mockReturnThis(),
|
||||||
|
returning: vi.fn().mockResolvedValue([{ id: "invite-123", used: true }]),
|
||||||
|
};
|
||||||
|
mockInviteUpdateBuilder.then = (resolve: any) => resolve([{ id: "invite-123", used: true }]);
|
||||||
|
mockDb.update.mockReturnValue(mockInviteUpdateBuilder);
|
||||||
|
|
||||||
// First call for user lookup, second call for tenant admin count, third for total count
|
// First call for user lookup, second call for tenant admin count, third for total count
|
||||||
mockCentralDb.select
|
mockCentralDb.select
|
||||||
.mockReturnValueOnce(mockUserInviteBuilder)
|
.mockReturnValueOnce(mockUserInviteBuilder)
|
||||||
|
|||||||
@@ -1097,13 +1097,13 @@ export class AppointmentService {
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Record failed attempt for throttling
|
// Record failed attempt for throttling
|
||||||
await challengeThrottleService.recordFailedAttempt(emailHash, "pin", this.tenantId);
|
await challengeThrottleService.recordFailedAttempt(emailHash, "pin");
|
||||||
|
|
||||||
throw new ValidationError("Invalid challenge response");
|
throw new ValidationError("Invalid challenge response");
|
||||||
}
|
}
|
||||||
|
|
||||||
// Clear throttle on successful verification
|
// Clear throttle on successful verification
|
||||||
await challengeThrottleService.clearThrottle(emailHash, "pin", this.tenantId);
|
await challengeThrottleService.clearThrottle(emailHash, "pin");
|
||||||
|
|
||||||
const db = await this.getDb();
|
const db = await this.getDb();
|
||||||
|
|
||||||
|
|||||||
@@ -53,23 +53,14 @@ class ChallengeThrottleService {
|
|||||||
* @param type - Type of challenge (pin or passkey)
|
* @param type - Type of challenge (pin or passkey)
|
||||||
* @param tenantId - Tenant ID for scoping the throttle check
|
* @param tenantId - Tenant ID for scoping the throttle check
|
||||||
*/
|
*/
|
||||||
async checkThrottle(
|
async checkThrottle(identifier: string, type: ThrottleType): Promise<ThrottleResult> {
|
||||||
identifier: string,
|
|
||||||
type: ThrottleType,
|
|
||||||
tenantId?: string,
|
|
||||||
): Promise<ThrottleResult> {
|
|
||||||
const now = new Date();
|
const now = new Date();
|
||||||
|
|
||||||
// Get throttle record from central DB
|
// Get throttle record from central DB
|
||||||
const records = await centralDb
|
const records = await centralDb
|
||||||
.select()
|
.select()
|
||||||
.from(challengeThrottle)
|
.from(challengeThrottle)
|
||||||
.where(
|
.where(and(eq(challengeThrottle.id, identifier)))
|
||||||
and(
|
|
||||||
eq(challengeThrottle.id, identifier),
|
|
||||||
tenantId ? eq(challengeThrottle.tenantId, tenantId) : undefined,
|
|
||||||
),
|
|
||||||
)
|
|
||||||
.limit(1);
|
.limit(1);
|
||||||
|
|
||||||
if (records.length === 0) {
|
if (records.length === 0) {
|
||||||
@@ -82,14 +73,7 @@ class ChallengeThrottleService {
|
|||||||
// Check if throttle has expired
|
// Check if throttle has expired
|
||||||
if (now > record.resetAt) {
|
if (now > record.resetAt) {
|
||||||
// Throttle expired, clean up and allow
|
// Throttle expired, clean up and allow
|
||||||
await centralDb
|
await centralDb.delete(challengeThrottle).where(and(eq(challengeThrottle.id, identifier)));
|
||||||
.delete(challengeThrottle)
|
|
||||||
.where(
|
|
||||||
and(
|
|
||||||
eq(challengeThrottle.id, identifier),
|
|
||||||
tenantId ? eq(challengeThrottle.tenantId, tenantId) : undefined,
|
|
||||||
),
|
|
||||||
);
|
|
||||||
return { allowed: true, retryAfterMs: 0, failedAttempts: 0 };
|
return { allowed: true, retryAfterMs: 0, failedAttempts: 0 };
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -123,11 +107,7 @@ class ChallengeThrottleService {
|
|||||||
* @param type - Type of challenge (pin or passkey)
|
* @param type - Type of challenge (pin or passkey)
|
||||||
* @param tenantId - Tenant ID for scoping the throttle record
|
* @param tenantId - Tenant ID for scoping the throttle record
|
||||||
*/
|
*/
|
||||||
async recordFailedAttempt(
|
async recordFailedAttempt(identifier: string, type: ThrottleType): Promise<void> {
|
||||||
identifier: string,
|
|
||||||
type: ThrottleType,
|
|
||||||
tenantId?: string,
|
|
||||||
): Promise<void> {
|
|
||||||
const now = new Date();
|
const now = new Date();
|
||||||
|
|
||||||
const resetAt = new Date(now.getTime() + THROTTLE_RESET_DURATION_MS);
|
const resetAt = new Date(now.getTime() + THROTTLE_RESET_DURATION_MS);
|
||||||
@@ -138,7 +118,6 @@ class ChallengeThrottleService {
|
|||||||
failedAttempts: 1,
|
failedAttempts: 1,
|
||||||
lastAttemptAt: now,
|
lastAttemptAt: now,
|
||||||
resetAt,
|
resetAt,
|
||||||
tenantId: tenantId,
|
|
||||||
})
|
})
|
||||||
.onConflictDoUpdate({
|
.onConflictDoUpdate({
|
||||||
target: challengeThrottle.id,
|
target: challengeThrottle.id,
|
||||||
@@ -160,15 +139,8 @@ class ChallengeThrottleService {
|
|||||||
* @param type - Type of challenge (pin or passkey)
|
* @param type - Type of challenge (pin or passkey)
|
||||||
* @param tenantId - Tenant ID for scoping the throttle clearance
|
* @param tenantId - Tenant ID for scoping the throttle clearance
|
||||||
*/
|
*/
|
||||||
async clearThrottle(identifier: string, type: ThrottleType, tenantId?: string): Promise<void> {
|
async clearThrottle(identifier: string, type: ThrottleType): Promise<void> {
|
||||||
await centralDb
|
await centralDb.delete(challengeThrottle).where(and(eq(challengeThrottle.id, identifier)));
|
||||||
.delete(challengeThrottle)
|
|
||||||
.where(
|
|
||||||
and(
|
|
||||||
eq(challengeThrottle.id, identifier),
|
|
||||||
tenantId ? eq(challengeThrottle.tenantId, tenantId) : undefined,
|
|
||||||
),
|
|
||||||
);
|
|
||||||
|
|
||||||
logger.debug(`Cleared ${type} challenge throttle`, {
|
logger.debug(`Cleared ${type} challenge throttle`, {
|
||||||
identifier: identifier.slice(0, 8),
|
identifier: identifier.slice(0, 8),
|
||||||
|
|||||||
@@ -329,6 +329,7 @@ export class UserService {
|
|||||||
.where(
|
.where(
|
||||||
and(
|
and(
|
||||||
eq(centralSchema.userInvite.inviteCode, linkToken),
|
eq(centralSchema.userInvite.inviteCode, linkToken),
|
||||||
|
eq(centralSchema.userInvite.used, false),
|
||||||
gt(centralSchema.userInvite.expiresAt, sql`timezone('utc', now())`),
|
gt(centralSchema.userInvite.expiresAt, sql`timezone('utc', now())`),
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
@@ -374,18 +375,19 @@ export class UserService {
|
|||||||
const retVal = await centralDb.insert(centralSchema.user).values(userDataForDb).returning();
|
const retVal = await centralDb.insert(centralSchema.user).values(userDataForDb).returning();
|
||||||
resultData.id = retVal[0].id;
|
resultData.id = retVal[0].id;
|
||||||
|
|
||||||
await InviteService.markInviteAsUsed(linkToken, resultData.id);
|
|
||||||
log.debug("Invitation marked as used", {
|
|
||||||
inviteCode: linkToken,
|
|
||||||
userId: resultData.id,
|
|
||||||
});
|
|
||||||
|
|
||||||
const adminService = await TenantAdminService.getTenantById(resultData.tenantId!);
|
const adminService = await TenantAdminService.getTenantById(resultData.tenantId!);
|
||||||
adminService.validateSetupState();
|
adminService.validateSetupState();
|
||||||
} else {
|
} else {
|
||||||
resultData = userData;
|
resultData = userData;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Mark the invite as used no matter what the user confirmation state is, to prevent re-use of the token
|
||||||
|
await InviteService.markInviteAsUsed(linkToken, resultData.id);
|
||||||
|
log.debug("Invitation marked as used", {
|
||||||
|
inviteCode: linkToken,
|
||||||
|
userId: resultData.id,
|
||||||
|
});
|
||||||
|
|
||||||
// Check if this is the first tenant admin for the tenant
|
// Check if this is the first tenant admin for the tenant
|
||||||
const numberOfUsers = await centralDb
|
const numberOfUsers = await centralDb
|
||||||
.select({ count: count() })
|
.select({ count: count() })
|
||||||
|
|||||||
@@ -107,11 +107,7 @@ export const POST: RequestHandler = async ({ request, params }) => {
|
|||||||
emailHash: body.emailHash,
|
emailHash: body.emailHash,
|
||||||
});
|
});
|
||||||
|
|
||||||
const throttleResult = await challengeThrottleService.checkThrottle(
|
const throttleResult = await challengeThrottleService.checkThrottle(binding, "passkey");
|
||||||
binding,
|
|
||||||
"passkey",
|
|
||||||
tenantId,
|
|
||||||
);
|
|
||||||
if (!throttleResult.allowed) {
|
if (!throttleResult.allowed) {
|
||||||
return json(
|
return json(
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -124,7 +124,7 @@ export const POST: RequestHandler = async ({ request, params }) => {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if (storedChallenge.emailHash !== binding) {
|
if (storedChallenge.emailHash !== binding) {
|
||||||
await challengeThrottleService.recordFailedAttempt(binding, "passkey", tenantId);
|
await challengeThrottleService.recordFailedAttempt(binding, "passkey");
|
||||||
throw new ValidationError("Invalid bootstrap challenge binding");
|
throw new ValidationError("Invalid bootstrap challenge binding");
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -136,11 +136,11 @@ export const POST: RequestHandler = async ({ request, params }) => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
if (!matchesPowDifficulty(digest, NEW_CLIENT_BOOTSTRAP_DIFFICULTY)) {
|
if (!matchesPowDifficulty(digest, NEW_CLIENT_BOOTSTRAP_DIFFICULTY)) {
|
||||||
await challengeThrottleService.recordFailedAttempt(binding, "passkey", tenantId);
|
await challengeThrottleService.recordFailedAttempt(binding, "passkey");
|
||||||
throw new ValidationError("Invalid bootstrap proof of work");
|
throw new ValidationError("Invalid bootstrap proof of work");
|
||||||
}
|
}
|
||||||
|
|
||||||
await challengeThrottleService.clearThrottle(binding, "passkey", tenantId);
|
await challengeThrottleService.clearThrottle(binding, "passkey");
|
||||||
|
|
||||||
const bookingAccessToken = await generateNewClientBootstrapToken({
|
const bookingAccessToken = await generateNewClientBootstrapToken({
|
||||||
tenantId,
|
tenantId,
|
||||||
|
|||||||
@@ -139,7 +139,7 @@ export const POST: RequestHandler = async ({ request, params }) => {
|
|||||||
const { emailHash } = requestSchema.parse(body);
|
const { emailHash } = requestSchema.parse(body);
|
||||||
|
|
||||||
// Check throttling
|
// Check throttling
|
||||||
const throttleResult = await challengeThrottleService.checkThrottle(emailHash, "pin", tenantId);
|
const throttleResult = await challengeThrottleService.checkThrottle(emailHash, "pin");
|
||||||
|
|
||||||
if (!throttleResult.allowed) {
|
if (!throttleResult.allowed) {
|
||||||
logger.warn("PIN challenge throttled", {
|
logger.warn("PIN challenge throttled", {
|
||||||
|
|||||||
@@ -169,11 +169,7 @@ export const POST: RequestHandler = async ({ request, params }) => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Record failed attempt for throttling
|
// Record failed attempt for throttling
|
||||||
await challengeThrottleService.recordFailedAttempt(
|
await challengeThrottleService.recordFailedAttempt(storedChallenge.emailHash, "pin");
|
||||||
storedChallenge.emailHash,
|
|
||||||
"pin",
|
|
||||||
tenantId,
|
|
||||||
);
|
|
||||||
|
|
||||||
throw new ValidationError("Invalid challenge response");
|
throw new ValidationError("Invalid challenge response");
|
||||||
}
|
}
|
||||||
@@ -213,7 +209,7 @@ export const POST: RequestHandler = async ({ request, params }) => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Clear throttle on successful verification
|
// Clear throttle on successful verification
|
||||||
await challengeThrottleService.clearThrottle(storedChallenge.emailHash, "pin", tenantId);
|
await challengeThrottleService.clearThrottle(storedChallenge.emailHash, "pin");
|
||||||
|
|
||||||
const response: ChallengeVerificationResponse = {
|
const response: ChallengeVerificationResponse = {
|
||||||
valid: true,
|
valid: true,
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
import { type Handle } from "@sveltejs/kit";
|
import { type Handle, type RequestEvent } from "@sveltejs/kit";
|
||||||
|
|
||||||
/** In-memory store for rate limiting records per client IP */
|
/** In-memory store for rate limiting records per client IP */
|
||||||
const rateLimitStore = new Map<string, { count: number; resetTime: number }>();
|
const rateLimitStore = new Map<string, { count: number; resetTime: number }>();
|
||||||
@@ -12,25 +12,20 @@ export const RATE_LIMIT_MAX_REQUESTS = 20;
|
|||||||
* Extracts the client IP address from request headers
|
* Extracts the client IP address from request headers
|
||||||
*
|
*
|
||||||
* Checks headers in order of preference:
|
* Checks headers in order of preference:
|
||||||
* 1. x-forwarded-for (takes first IP from comma-separated list)
|
* 1. Returns IP
|
||||||
* 2. x-real-ip
|
* 2. Returns 'unknown' if no IP found
|
||||||
* 3. Returns 'unknown' if no IP found
|
|
||||||
*
|
*
|
||||||
* @param {Request} request - The incoming HTTP request
|
* @param {RequestEvent} event - The incoming HTTP request event
|
||||||
* @returns {string} The client IP address or 'unknown'
|
* @returns {string} The client IP address or 'unknown'
|
||||||
*/
|
*/
|
||||||
function getClientIP(request: Request): string {
|
function getClientIP(event: RequestEvent): string {
|
||||||
const forwarded = request.headers.get("x-forwarded-for");
|
try {
|
||||||
if (forwarded) {
|
return event.getClientAddress();
|
||||||
return forwarded.split(",")[0].trim();
|
} catch {
|
||||||
|
// Thrown by adapter-node if behind a proxy without XFF_DEPTH configured,
|
||||||
|
// or if the platform can't determine an address.
|
||||||
|
return "unknown";
|
||||||
}
|
}
|
||||||
|
|
||||||
const realIP = request.headers.get("x-real-ip");
|
|
||||||
if (realIP) {
|
|
||||||
return realIP;
|
|
||||||
}
|
|
||||||
|
|
||||||
return "unknown";
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -83,8 +78,7 @@ setInterval(() => {
|
|||||||
* @returns {Promise<Response>} The response with applied headers and rate limiting
|
* @returns {Promise<Response>} The response with applied headers and rate limiting
|
||||||
*/
|
*/
|
||||||
export const rateLimitHandle: Handle = async ({ event, resolve }) => {
|
export const rateLimitHandle: Handle = async ({ event, resolve }) => {
|
||||||
const { request } = event;
|
const clientIP = getClientIP(event);
|
||||||
const clientIP = getClientIP(request);
|
|
||||||
|
|
||||||
if (isRateLimited(clientIP)) {
|
if (isRateLimited(clientIP)) {
|
||||||
return new Response("Too Many Requests", {
|
return new Response("Too Many Requests", {
|
||||||
|
|||||||
Reference in New Issue
Block a user