From cbeccc6c1fc691ba92d7dae050dc58addf670cb7 Mon Sep 17 00:00:00 2001 From: Stefan Lazic Date: Sun, 6 Sep 2026 21:01:54 +0100 Subject: [PATCH] Auth Fix Fixed an issue where the user would be redirected to a localhost address because an auth url was not set in the env. --- .env.business.example | 5 + .../lib/__tests__/compose-template.test.ts | 14 ++ .../src/lib/__tests__/update-business.test.ts | 102 +++++++++ manager/src/lib/compose-template.ts | 8 + manager/src/lib/provisioning.ts | 10 +- src/auth.ts | 77 +------ .../auth-finalize.integration.test.ts | 194 ++++++++++++++++++ src/lib/auth/authorize-credentials.ts | 81 ++++++++ 8 files changed, 420 insertions(+), 71 deletions(-) create mode 100644 manager/src/lib/__tests__/update-business.test.ts create mode 100644 src/features/installation/__tests__/auth-finalize.integration.test.ts create mode 100644 src/lib/auth/authorize-credentials.ts diff --git a/.env.business.example b/.env.business.example index b0c8e1d..e8184c0 100644 --- a/.env.business.example +++ b/.env.business.example @@ -38,6 +38,11 @@ DATABASE_PASSWORD="replace-me" AUTH_SECRET="replace-me-with-a-32-byte-random-string" # Auth.js needs to trust the host it's served from when not on localhost:3000. AUTH_TRUST_HOST="true" +# Explicit, not just inferred from AUTH_TRUST_HOST + request headers — +# header-based detection (Host/X-Forwarded-Host) behaves inconsistently +# across reverse-proxy setups, and a wrong guess sends every post-sign-in +# redirect to the wrong origin. Must match DOMAIN above exactly. +AUTH_URL="https://example-business.example.com" NEXT_PUBLIC_APP_URL="https://example-business.example.com" # One-time setup token the installation wizard (/install) requires before # it will create the first business/administrator — generate with: diff --git a/manager/src/lib/__tests__/compose-template.test.ts b/manager/src/lib/__tests__/compose-template.test.ts index 2fb7dfe..54a45ed 100644 --- a/manager/src/lib/__tests__/compose-template.test.ts +++ b/manager/src/lib/__tests__/compose-template.test.ts @@ -35,3 +35,17 @@ describe("generateBusinessEnv — INSTALLATION_TOKEN", () => { expect(envB).not.toContain("token-a"); }); }); + +describe("generateBusinessEnv — AUTH_URL", () => { + it("sets AUTH_URL explicitly to the business's own https domain — not left to header-based detection", () => { + const env = generateBusinessEnv(baseValues); + expect(env).toContain('AUTH_URL="https://acme.example.com"'); + }); + + it("AUTH_URL always matches DOMAIN/NEXT_PUBLIC_APP_URL for a given business", () => { + const env = generateBusinessEnv({ ...baseValues, domain: "other-business.example.com" }); + expect(env).toContain('DOMAIN="other-business.example.com"'); + expect(env).toContain('AUTH_URL="https://other-business.example.com"'); + expect(env).toContain('NEXT_PUBLIC_APP_URL="https://other-business.example.com"'); + }); +}); diff --git a/manager/src/lib/__tests__/update-business.test.ts b/manager/src/lib/__tests__/update-business.test.ts new file mode 100644 index 0000000..a9ad5c8 --- /dev/null +++ b/manager/src/lib/__tests__/update-business.test.ts @@ -0,0 +1,102 @@ +import { mkdtemp, mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const composeUpMock = vi.fn().mockResolvedValue(undefined); +const reloadCaddyMock = vi.fn().mockResolvedValue(undefined); +vi.mock("@/lib/docker", () => ({ + composeUp: (...args: unknown[]) => composeUpMock(...args), + composeDown: vi.fn(), + composeStatus: vi.fn().mockResolvedValue("stopped"), + reloadCaddy: (...args: unknown[]) => reloadCaddyMock(...args), + runMigrations: vi.fn(), +})); + +/** + * Regression coverage for a real production bug: a business behind a + * reverse proxy redirected every post-sign-in navigation to + * http://localhost:3000 instead of its own domain, because AUTH_URL was + * never set and Auth.js's header-based host detection didn't behave as + * expected in that deployment. The fix makes AUTH_URL explicit at + * provisioning time (see compose-template.test.ts) — this file covers + * the other place a business's domain gets set: editing it later, where + * AUTH_URL must keep tracking DOMAIN exactly like NEXT_PUBLIC_APP_URL + * already did, or a business that changes domains would silently regress + * back into the same bug. + */ +describe("updateBusiness — AUTH_URL tracks domain changes", () => { + let stacksDir: string; + const originalStacksDir = process.env.STACKS_DIR; + const originalMysqlHost = process.env.MYSQL_HOST; + + beforeEach(async () => { + stacksDir = await mkdtemp(path.join(tmpdir(), "nc-manager-updatebiz-test-")); + process.env.STACKS_DIR = stacksDir; + // updateBusiness's own domain-availability check just scans other + // stacks' .env files via listBusinessStacks() — no real MySQL needed + // for this test since we never change the domain to something + // already in use. + composeUpMock.mockClear(); + reloadCaddyMock.mockClear(); + }); + + afterEach(async () => { + if (originalStacksDir === undefined) delete process.env.STACKS_DIR; + else process.env.STACKS_DIR = originalStacksDir; + if (originalMysqlHost === undefined) delete process.env.MYSQL_HOST; + else process.env.MYSQL_HOST = originalMysqlHost; + await rm(stacksDir, { recursive: true, force: true }); + }); + + async function createStack(slug: string, envContent: string) { + const dir = path.join(stacksDir, slug); + await mkdir(dir, { recursive: true }); + await writeFile(path.join(dir, "compose.yaml"), "name: test\n", "utf8"); + await writeFile(path.join(dir, ".env"), envContent, "utf8"); + return dir; + } + + it("updates AUTH_URL to the new domain, alongside DOMAIN and NEXT_PUBLIC_APP_URL", async () => { + const { updateBusiness } = await import("../provisioning"); + await createStack( + "acme", + [ + 'BUSINESS_NAME="Acme"', + 'DOMAIN="old-domain.example.com"', + 'AUTH_URL="https://old-domain.example.com"', + 'NEXT_PUBLIC_APP_URL="https://old-domain.example.com"', + 'AUTH_SECRET="unrelated-secret"', + ].join("\n") + "\n", + ); + + const result = await updateBusiness({ slug: "acme", name: "Acme", domain: "new-domain.example.com" }); + + expect(result.restarted).toBe(true); + const envAfter = await readFile(path.join(stacksDir, "acme", ".env"), "utf8"); + expect(envAfter).toContain('DOMAIN="new-domain.example.com"'); + expect(envAfter).toContain('AUTH_URL="https://new-domain.example.com"'); + expect(envAfter).toContain('NEXT_PUBLIC_APP_URL="https://new-domain.example.com"'); + expect(envAfter).not.toContain("old-domain.example.com"); + // Unrelated secrets untouched. + expect(envAfter).toContain('AUTH_SECRET="unrelated-secret"'); + + expect(composeUpMock).toHaveBeenCalledWith(path.join(stacksDir, "acme"), { forceRecreate: true }); + }); + + it("leaves AUTH_URL alone (no restart) when only the business name changes, not the domain", async () => { + const { updateBusiness } = await import("../provisioning"); + await createStack( + "acme", + ['BUSINESS_NAME="Acme"', 'DOMAIN="acme.example.com"', 'AUTH_URL="https://acme.example.com"'].join("\n") + "\n", + ); + + const result = await updateBusiness({ slug: "acme", name: "Acme Barbers Ltd", domain: "acme.example.com" }); + + expect(result.restarted).toBe(false); + expect(composeUpMock).not.toHaveBeenCalled(); + const envAfter = await readFile(path.join(stacksDir, "acme", ".env"), "utf8"); + expect(envAfter).toContain('AUTH_URL="https://acme.example.com"'); + expect(envAfter).toContain('BUSINESS_NAME="Acme Barbers Ltd"'); + }); +}); diff --git a/manager/src/lib/compose-template.ts b/manager/src/lib/compose-template.ts index 10f5c44..7c0b357 100644 --- a/manager/src/lib/compose-template.ts +++ b/manager/src/lib/compose-template.ts @@ -134,6 +134,14 @@ DATABASE_USER="${values.databaseUser}" DATABASE_PASSWORD="${values.databasePassword}" AUTH_SECRET="${values.authSecret}" AUTH_TRUST_HOST="true" +# Explicit, not just inferred from AUTH_TRUST_HOST + request headers — +# header-based detection (Host/X-Forwarded-Host) is exactly the kind of +# thing that behaves inconsistently across reverse-proxy setups, and a +# wrong guess here sends every post-sign-in/post-install redirect to the +# wrong origin (seen for real: a business behind Caddy redirecting to +# http://localhost:3000 instead of its own domain). Auth.js's own docs +# recommend setting this explicitly for exactly this reason. +AUTH_URL="https://${values.domain}" NEXT_PUBLIC_APP_URL="https://${values.domain}" INSTALLATION_TOKEN="${values.installationToken}" CRON_SECRET="${values.cronSecret}" diff --git a/manager/src/lib/provisioning.ts b/manager/src/lib/provisioning.ts index 0730e09..ee61526 100644 --- a/manager/src/lib/provisioning.ts +++ b/manager/src/lib/provisioning.ts @@ -254,14 +254,16 @@ export async function updateBusiness(input: UpdateBusinessInput): Promise { - const email = typeof raw?.email === "string" ? raw.email.trim().toLowerCase() : ""; - if (!email) return null; - - // Keyed by the attempted email, not IP — this is what actually - // stops credential stuffing against one account regardless of how - // many source addresses it's spread across. 30 attempts per 15 - // minutes is well above anything a real forgetful user would ever - // hit, while still making online guessing impractical. - const { allowed } = checkRateLimit(`signin:${email}`, { maxAttempts: 30, windowMs: 15 * 60 * 1000 }); - if (!allowed) return null; - - if (raw?.finalize === "true") { - const token = typeof raw?.token === "string" ? raw.token : ""; - if (!verifyFinalizeToken(token)) return null; - if (!(await finalizeOtp(prisma, email))) return null; - } else { - const code = typeof raw?.code === "string" ? raw.code : ""; - if (!code) return null; - const verified = await verifyOtp(prisma, email, code); - if (!verified.ok) return null; - if (!(await finalizeOtp(prisma, email))) return null; - } - - const user = await prisma.user.findUnique({ where: { email } }); - if (!user) return null; - - return { id: user.id, email: user.email, name: user.name, image: user.image }; - }, + // Kept in its own module (not inlined here) so it's importable from + // a plain Vitest test without dragging in this file's NextAuth(...) + // construction, which needs the Next.js runtime — see + // authorize-credentials.ts's own header. + authorize: authorizeCredentials, }), ], callbacks: { diff --git a/src/features/installation/__tests__/auth-finalize.integration.test.ts b/src/features/installation/__tests__/auth-finalize.integration.test.ts new file mode 100644 index 0000000..3297111 --- /dev/null +++ b/src/features/installation/__tests__/auth-finalize.integration.test.ts @@ -0,0 +1,194 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { prisma } from "@/services/scheduling/__tests__/fixtures"; +import { authorizeCredentials } from "@/lib/auth/authorize-credentials"; +import { requestOtp, verifyOtp } from "@/lib/auth/otp.service"; +import { MockEmailProvider } from "@/services/notifications/providers/mock-email-provider"; +import { installApplication } from "../installer.service"; +import type { InstallInput } from "../schema"; + +/** + * Regression coverage for a real production bug: installApplication's own + * transaction consumes the administrator's OTP row via finalizeOtp (see + * installer.service.ts) — but src/auth.ts's Credentials provider used to + * call finalizeOtp a *second* time on its `{finalize:"true"}` sign-in + * path, which always fails (finalizeOtp is one-time-use) and turned every + * successful install into a CredentialsSignin error on the very last + * step. installer.service.integration.test.ts and + * otp.service.integration.test.ts each separately proved their own half + * correctly; neither exercised the interaction between them, which is + * exactly where this broke. This file exercises that interaction, end to + * end, against a real MySQL database. + */ + +const INSTALLATION_TOKEN = "auth-finalize-test-install-token"; + +function buildInput(overrides: Partial = {}): InstallInput { + return { + installationToken: INSTALLATION_TOKEN, + administrator: { + firstName: "Test", + lastName: "Owner", + email: `auth-finalize-test-${Date.now()}-${Math.random().toString(36).slice(2, 8)}@example.com`, + phone: "", + }, + business: { name: "Auth Finalize Test Barbershop", shortName: "", description: "", email: "", phone: "", website: "" }, + location: { + addressLine1: "1 Test Street", + addressLine2: "", + city: "Testville", + region: "", + postalCode: "T1 1TT", + country: "GB", + timezone: "Europe/London", + currency: "GBP", + locale: "en-GB", + }, + hours: (["MONDAY", "TUESDAY", "WEDNESDAY", "THURSDAY", "FRIDAY", "SATURDAY", "SUNDAY"] as const).map((dayOfWeek) => ({ + dayOfWeek, + isClosed: dayOfWeek === "SUNDAY", + opensAtMinutes: dayOfWeek === "SUNDAY" ? undefined : 540, + closesAtMinutes: dayOfWeek === "SUNDAY" ? undefined : 1080, + })), + bookingRules: { + allowGuestBooking: false, + minNoticeMinutes: 120, + maxAdvanceDays: 60, + cancellationWindowHours: 24, + rescheduleWindowHours: 24, + slotIntervalMinutes: 30, + bufferBeforeMinutes: 0, + bufferAfterMinutes: 5, + maxAppointmentsPerDay: null, + maxAppointmentsPerStaffPerDay: null, + customerCanChooseStaff: true, + requireApproval: false, + }, + branding: { logoUrl: "", coverImageUrl: "", themeColor: "", backgroundColor: "" }, + starterServices: [ + { categoryName: "Haircuts", name: "Classic Cut", description: "", price: "20.00", durationMinutes: 30, active: true, allowOnlineBooking: true }, + ], + ...overrides, + }; +} + +async function verifyAdministratorEmail(email: string): Promise { + const mockProvider = new MockEmailProvider(); + await requestOtp(prisma, { email, purpose: "install", emailProvider: mockProvider }); + const sent = mockProvider.sentEmails[0]; + const code = /verification code is (\d{6})/.exec(sent?.text ?? "")?.[1]; + if (!code) throw new Error("Test setup failed: couldn't extract the OTP code from the captured email."); + const result = await verifyOtp(prisma, email, code); + if (!result.ok) throw new Error(`Test setup failed: OTP verification failed (${result.reason}).`); +} + +async function buildVerifiedInput(overrides: Partial = {}): Promise { + const input = buildInput(overrides); + await verifyAdministratorEmail(input.administrator.email); + return input; +} + +async function resetInstallationSingleton(): Promise { + await prisma.applicationInstallation.deleteMany({ where: { id: "singleton" } }); +} + +async function cleanupInstalledBusiness(businessId: string | undefined): Promise { + if (!businessId) return; + const staff = await prisma.staffProfile.findMany({ where: { businessId }, select: { id: true, userId: true } }); + const userIds = staff.map((s) => s.userId); + await prisma.staffService.deleteMany({ where: { staffId: { in: staff.map((s) => s.id) } } }); + await prisma.staffProfile.deleteMany({ where: { businessId } }); + await prisma.service.deleteMany({ where: { businessId } }); + await prisma.serviceCategory.deleteMany({ where: { businessId } }); + await prisma.rolePermission.deleteMany({ where: { role: { businessId } } }); + await prisma.role.deleteMany({ where: { businessId } }); + await prisma.auditLog.deleteMany({ where: { entityId: businessId } }); + await prisma.loyaltyProgramme.deleteMany({ where: { businessId } }); + await prisma.bookingRule.deleteMany({ where: { businessId } }); + await prisma.businessSettings.deleteMany({ where: { businessId } }); + await prisma.businessHours.deleteMany({ where: { businessId } }); + await prisma.businessLocation.deleteMany({ where: { businessId } }); + await prisma.business.deleteMany({ where: { id: businessId } }); + await prisma.user.deleteMany({ where: { id: { in: userIds } } }); + await resetInstallationSingleton(); +} + +describe("authorizeCredentials — post-install finalize sign-in (integration)", () => { + let createdBusinessId: string | undefined; + let originalToken: string | undefined; + + beforeEach(async () => { + await resetInstallationSingleton(); + createdBusinessId = undefined; + originalToken = process.env.INSTALLATION_TOKEN; + process.env.INSTALLATION_TOKEN = INSTALLATION_TOKEN; + }); + + afterEach(async () => { + await cleanupInstalledBusiness(createdBusinessId); + process.env.INSTALLATION_TOKEN = originalToken; + }); + + it("signs the administrator in immediately after a real installApplication() success — the exact sequence submitInstallationAction runs", async () => { + const input = await buildVerifiedInput(); + const result = await installApplication(prisma, { input, installedVersion: "test" }); + expect(result.success).toBe(true); + if (!result.success) return; + createdBusinessId = result.businessId; + + // This is the reported bug: before the fix, this call always returned + // null (CredentialsSignin) because installApplication already + // consumed the OTP row and this path tried to finalize it again. + const user = await authorizeCredentials({ + email: input.administrator.email, + finalize: "true", + token: INSTALLATION_TOKEN, + }); + + expect(user).not.toBeNull(); + expect(user?.id).toBe(result.userId); + expect(user?.email).toBe(input.administrator.email); + }); + + it("rejects the finalize path with a wrong installation token, even right after a real install", async () => { + const input = await buildVerifiedInput(); + const result = await installApplication(prisma, { input, installedVersion: "test" }); + expect(result.success).toBe(true); + if (!result.success) return; + createdBusinessId = result.businessId; + + const user = await authorizeCredentials({ email: input.administrator.email, finalize: "true", token: "wrong-token" }); + expect(user).toBeNull(); + }); + + it("rejects the finalize path entirely if no INSTALLATION_TOKEN is configured", async () => { + const input = await buildVerifiedInput(); + const result = await installApplication(prisma, { input, installedVersion: "test" }); + expect(result.success).toBe(true); + if (!result.success) return; + createdBusinessId = result.businessId; + + delete process.env.INSTALLATION_TOKEN; + const user = await authorizeCredentials({ email: input.administrator.email, finalize: "true", token: INSTALLATION_TOKEN }); + expect(user).toBeNull(); + }); + + it("the ordinary {email, code} sign-in path is unaffected by this fix — still verifies and consumes its own fresh code", async () => { + const email = `auth-finalize-normal-${Date.now()}@example.com`; + const mockProvider = new MockEmailProvider(); + await requestOtp(prisma, { email, purpose: "login", emailProvider: mockProvider }); + const code = /verification code is (\d{6})/.exec(mockProvider.sentEmails[0]?.text ?? "")?.[1]; + if (!code) throw new Error("Test setup failed: couldn't extract the OTP code."); + + await prisma.user.upsert({ + where: { email }, + update: {}, + create: { email, name: "Normal Login User" }, + }); + + const user = await authorizeCredentials({ email, code }); + expect(user).not.toBeNull(); + expect(user?.email).toBe(email); + + await prisma.user.deleteMany({ where: { email } }); + }); +}); diff --git a/src/lib/auth/authorize-credentials.ts b/src/lib/auth/authorize-credentials.ts new file mode 100644 index 0000000..e268bb7 --- /dev/null +++ b/src/lib/auth/authorize-credentials.ts @@ -0,0 +1,81 @@ +import { finalizeOtp, verifyOtp } from "@/lib/auth/otp.service"; +import { prisma } from "@/lib/db/prisma"; +import { checkRateLimit } from "@/lib/security/rate-limit"; +import { secretsMatch } from "@/lib/security/secrets-match"; + +/** + * Two credential shapes — see src/lib/auth/otp.service.ts's own header + * for the full verify/finalize design this builds on. + * + * `{ email, code }` — the normal path, used by sign-in, registration, and + * (since the "administrator invite verifies via OTP too" work) invitation + * acceptance as well: verifyOtp then finalizeOtp back to back in this one + * request (functionally "verify and consume in one shot" from the + * caller's point of view). + * + * `{ email, finalize: "true", token }` — used only by the installer's own + * immediate post-completion sign-in. Deliberately does NOT call + * finalizeOtp itself (unlike the branch above) — installApplication's own + * transaction already did, consuming the row as part of creating the + * administrator (see installer.service.ts). finalizeOtp is one-time-use + * (it sets consumedAt and refuses a second call on the same row), so + * calling it again here would always fail and turn every successful + * install into a CredentialsSignin error on this very last step — this + * was exactly that bug, caught because it reproduces on every install. + * `token` matching the live INSTALLATION_TOKEN is this branch's entire + * proof of legitimacy instead — required because + * /api/auth/callback/credentials is a public route regardless of this + * change (true of any Credentials provider), so this path must + * independently prove the caller already holds a secret an attacker + * can't have, not just trust that it's being called "from the right + * place". (Invitation acceptance doesn't use this path — it has its own + * real OTP code to re-enter here instead.) + */ +function verifyFinalizeToken(token: string): boolean { + if (!token) return false; + + const installToken = process.env.INSTALLATION_TOKEN; + return !!installToken && secretsMatch(token, installToken); +} + +/** + * The Credentials provider's `authorize` logic, pulled out of src/auth.ts + * into this module specifically so it's importable in a plain Vitest + * test without dragging in `next-auth`'s `NextAuth(...)` construction + * (which needs the Next.js runtime and fails to resolve under Vitest — + * see auth-finalize.integration.test.ts's own header for why this + * function's correctness matters enough to need that). src/auth.ts wires + * this in verbatim as its Credentials provider's `authorize` option. + */ +export async function authorizeCredentials( + raw: Partial> | undefined, +): Promise<{ id: string; email: string | null; name: string | null; image: string | null } | null> { + const email = typeof raw?.email === "string" ? raw.email.trim().toLowerCase() : ""; + if (!email) return null; + + // Keyed by the attempted email, not IP — this is what actually + // stops credential stuffing against one account regardless of how + // many source addresses it's spread across. 30 attempts per 15 + // minutes is well above anything a real forgetful user would ever + // hit, while still making online guessing impractical. + const { allowed } = checkRateLimit(`signin:${email}`, { maxAttempts: 30, windowMs: 15 * 60 * 1000 }); + if (!allowed) return null; + + if (raw?.finalize === "true") { + const token = typeof raw?.token === "string" ? raw.token : ""; + if (!verifyFinalizeToken(token)) return null; + // No finalizeOtp call here — installApplication's own + // transaction already consumed it. See this file's own header. + } else { + const code = typeof raw?.code === "string" ? raw.code : ""; + if (!code) return null; + const verified = await verifyOtp(prisma, email, code); + if (!verified.ok) return null; + if (!(await finalizeOtp(prisma, email))) return null; + } + + const user = await prisma.user.findUnique({ where: { email } }); + if (!user) return null; + + return { id: user.id, email: user.email, name: user.name, image: user.image }; +}