From 5e4f3fca62ba1c4697b0c764ab667dc73789a0b8 Mon Sep 17 00:00:00 2001 From: Nicolas Meienberger Date: Mon, 23 Feb 2026 20:23:54 +0100 Subject: [PATCH] refactor: restrict provider configuration to super admins only --- .../settings/components/user-management.tsx | 2 +- .../settings/routes/create-sso-provider.tsx | 2 +- app/routes/(dashboard)/settings/sso/new.tsx | 12 ++++- app/routes/api.$.ts | 6 +-- app/server/db/schema.ts | 2 +- app/server/lib/auth.ts | 15 ++++-- .../__tests__/require-sso-invitation.test.ts | 14 ++--- .../trust-sso-provider-for-linking.test.ts | 18 +++++++ .../validate-sso-callback-urls.test.ts | 48 +++++++++++++++++ .../trust-sso-provider-for-linking.ts | 18 ++++--- .../middlewares/validate-sso-callback-urls.ts | 36 +++++++++++++ .../auth/__tests__/auth.sso-security.test.ts | 54 +++++++++++++++++++ 12 files changed, 202 insertions(+), 25 deletions(-) create mode 100644 app/server/lib/auth/middlewares/__tests__/validate-sso-callback-urls.test.ts create mode 100644 app/server/lib/auth/middlewares/validate-sso-callback-urls.ts create mode 100644 app/server/modules/auth/__tests__/auth.sso-security.test.ts diff --git a/app/client/modules/settings/components/user-management.tsx b/app/client/modules/settings/components/user-management.tsx index f40640b4..bdfaf5e7 100644 --- a/app/client/modules/settings/components/user-management.tsx +++ b/app/client/modules/settings/components/user-management.tsx @@ -134,7 +134,7 @@ export function UserManagement({ currentUser }: { currentUser: { id: string } | - + 0 })}> No users found. diff --git a/app/client/modules/settings/routes/create-sso-provider.tsx b/app/client/modules/settings/routes/create-sso-provider.tsx index ccb95b83..d36b6973 100644 --- a/app/client/modules/settings/routes/create-sso-provider.tsx +++ b/app/client/modules/settings/routes/create-sso-provider.tsx @@ -50,7 +50,7 @@ export function CreateSsoProviderPage() { clientId: "", clientSecret: "", discoveryEndpoint: "", - linkMatchingEmails: true, + linkMatchingEmails: false, }, }); diff --git a/app/routes/(dashboard)/settings/sso/new.tsx b/app/routes/(dashboard)/settings/sso/new.tsx index b11ad6af..2ec2907e 100644 --- a/app/routes/(dashboard)/settings/sso/new.tsx +++ b/app/routes/(dashboard)/settings/sso/new.tsx @@ -1,8 +1,18 @@ -import { createFileRoute } from "@tanstack/react-router"; +import { createFileRoute, redirect } from "@tanstack/react-router"; import { CreateSsoProviderPage } from "~/client/modules/settings/routes/create-sso-provider"; +import { fetchUser } from "../../route"; export const Route = createFileRoute("/(dashboard)/settings/sso/new")({ component: RouteComponent, + loader: async () => { + const authContext = await fetchUser(); + + if (authContext.user?.role !== "admin") { + throw redirect({ to: "/settings" }); + } + + return authContext; + }, staticData: { breadcrumb: () => [{ label: "Settings", href: "/settings" }, { label: "Register SSO Provider" }], }, diff --git a/app/routes/api.$.ts b/app/routes/api.$.ts index 751a2784..6a88864e 100644 --- a/app/routes/api.$.ts +++ b/app/routes/api.$.ts @@ -8,11 +8,7 @@ const handle = ({ request }: { request: Request }) => app.fetch(request.clone()) export const Route = createFileRoute("/api/$")({ server: { handlers: { - GET: handle, - POST: handle, - DELETE: handle, - PUT: handle, - PATCH: handle, + ANY: handle, }, }, }); diff --git a/app/server/db/schema.ts b/app/server/db/schema.ts index 2e542f04..20071336 100644 --- a/app/server/db/schema.ts +++ b/app/server/db/schema.ts @@ -184,7 +184,7 @@ export const ssoProvider = sqliteTable( userId: text("user_id").references(() => usersTable.id, { onDelete: "set null" }), issuer: text("issuer").notNull(), domain: text("domain").notNull(), - autoLinkMatchingEmails: int("auto_link_matching_emails", { mode: "boolean" }).notNull().default(true), + autoLinkMatchingEmails: int("auto_link_matching_emails", { mode: "boolean" }).notNull().default(false), oidcConfig: text("oidc_config", { mode: "json" }).$type | null>(), samlConfig: text("saml_config", { mode: "json" }).$type | null>(), createdAt: integer("created_at", { mode: "timestamp_ms" }) diff --git a/app/server/lib/auth.ts b/app/server/lib/auth.ts index 9c187224..343425d2 100644 --- a/app/server/lib/auth.ts +++ b/app/server/lib/auth.ts @@ -4,6 +4,7 @@ import { type BetterAuthOptions, type MiddlewareContext, type MiddlewareOptions, + type User, } from "better-auth"; import { drizzleAdapter } from "better-auth/adapters/drizzle"; import { admin, createAuthMiddleware, twoFactor, username, organization } from "better-auth/plugins"; @@ -16,10 +17,10 @@ import { tanstackStartCookies } from "better-auth/tanstack-start"; import { isValidUsername, normalizeUsername } from "~/lib/username"; import { ensureOnlyOneUser } from "./auth/middlewares/only-one-user"; import { convertLegacyUserOnFirstLogin } from "./auth/middlewares/convert-legacy-user"; +import { validateSsoCallbackUrls } from "./auth/middlewares/validate-sso-callback-urls"; import { createUserDefaultOrg } from "./auth/helpers/create-default-org"; import { isSsoCallbackRequest, requireSsoInvitation } from "./auth/middlewares/require-sso-invitation"; import { ssoTrustedProviderLinkingPlugin } from "./auth/plugins/sso-trusted-provider-linking"; -import { logger } from "../utils/logger"; export type AuthMiddlewareContext = MiddlewareContext>; @@ -36,6 +37,7 @@ export const auth = betterAuth({ }, hooks: { before: createAuthMiddleware(async (ctx) => { + await validateSsoCallbackUrls(ctx); await ensureOnlyOneUser(ctx); await convertLegacyUserOnFirstLogin(ctx); }), @@ -53,7 +55,6 @@ export const auth = betterAuth({ create: { before: async (user, ctx) => { if (isSsoCallbackRequest(ctx)) { - logger.debug("SSO callback detected, checking for pending invitations for user", user.email); await requireSsoInvitation(user.email, ctx); user.hasDownloadedResticPassword = true; } @@ -119,7 +120,15 @@ export const auth = betterAuth({ allowUserToCreateOrganization: false, }), sso({ - trustEmailVerified: true, + trustEmailVerified: false, + providersLimit: async (user: User) => { + const existingUser = await db.query.usersTable.findFirst({ + columns: { role: true }, + where: { id: user.id }, + }); + + return existingUser?.role === "admin" ? 10 : 0; + }, organizationProvisioning: { disabled: false, defaultRole: "member", diff --git a/app/server/lib/auth/middlewares/__tests__/require-sso-invitation.test.ts b/app/server/lib/auth/middlewares/__tests__/require-sso-invitation.test.ts index b95f6e8e..a7e78f0c 100644 --- a/app/server/lib/auth/middlewares/__tests__/require-sso-invitation.test.ts +++ b/app/server/lib/auth/middlewares/__tests__/require-sso-invitation.test.ts @@ -69,17 +69,17 @@ describe("requireSsoInvitation", () => { await db.delete(usersTable); }); - test("returns early when context is null", async () => { + test("throws when context is null", async () => { await createSsoProvider("oidc-acme"); - expect(requireSsoInvitation("user@example.com", null)).resolves.toBeUndefined(); + expect(requireSsoInvitation("user@example.com", null)).rejects.toThrow("Missing SSO context"); }); - test("returns early when request is not an SSO callback", async () => { + test("throws when request is not an SSO callback", async () => { await createSsoProvider("oidc-acme"); const ctx = createMockContext("/sign-up/email"); - expect(requireSsoInvitation("user@example.com", ctx)).resolves.toBeUndefined(); + expect(requireSsoInvitation("user@example.com", ctx)).rejects.toThrow("Missing providerId"); }); test("detects whether current request is an SSO callback", async () => { @@ -105,7 +105,7 @@ describe("requireSsoInvitation", () => { await db.insert(invitation).values({ id: randomId(), organizationId, - email: "User@Example.com", + email: "user@example.com", role: "member", status: "pending", expiresAt: new Date(Date.now() + 60 * 60 * 1000), @@ -117,8 +117,8 @@ describe("requireSsoInvitation", () => { expect(requireSsoInvitation("user@example.com", ctx)).resolves.toBeUndefined(); }); - test("returns early when provider is not registered", async () => { + test("throws when provider is not registered", async () => { const ctx = createMockSsoCallbackContext("missing-provider"); - expect(requireSsoInvitation("user@example.com", ctx)).resolves.toBeUndefined(); + expect(requireSsoInvitation("user@example.com", ctx)).rejects.toThrow("SSO provider not found"); }); }); diff --git a/app/server/lib/auth/middlewares/__tests__/trust-sso-provider-for-linking.test.ts b/app/server/lib/auth/middlewares/__tests__/trust-sso-provider-for-linking.test.ts index d4741e47..7b274f00 100644 --- a/app/server/lib/auth/middlewares/__tests__/trust-sso-provider-for-linking.test.ts +++ b/app/server/lib/auth/middlewares/__tests__/trust-sso-provider-for-linking.test.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, test } from "bun:test"; import type { GenericEndpointContext } from "@better-auth/core"; +import { eq } from "drizzle-orm"; import { db } from "~/server/db/db"; import { account, member, organization, ssoProvider, usersTable } from "~/server/db/schema"; import { isSsoCallbackPath, trustSsoProviderForLinking } from "../trust-sso-provider-for-linking"; @@ -146,6 +147,23 @@ describe("trustSsoProviderForLinking", () => { expect(ctx.context.options.account?.accountLinking?.trustedProviders).toEqual(["pocket-id"]); }); + test("removes provider from trusted providers when auto-linking is disabled", async () => { + await createSsoProviderRecord("pocket-id", true); + + const ctx = createMockContext({ + path: "/sso/callback/pocket-id", + params: { providerId: "pocket-id" }, + }); + + await trustSsoProviderForLinking(ctx); + expect(ctx.context.options.account?.accountLinking?.trustedProviders).toEqual(["pocket-id"]); + + await db.update(ssoProvider).set({ autoLinkMatchingEmails: false }).where(eq(ssoProvider.providerId, "pocket-id")); + + await trustSsoProviderForLinking(ctx); + expect(ctx.context.options.account?.accountLinking?.trustedProviders).toEqual([]); + }); + test("does nothing when account linking is disabled", async () => { await createSsoProviderRecord("pocket-id", true); diff --git a/app/server/lib/auth/middlewares/__tests__/validate-sso-callback-urls.test.ts b/app/server/lib/auth/middlewares/__tests__/validate-sso-callback-urls.test.ts new file mode 100644 index 00000000..ae488527 --- /dev/null +++ b/app/server/lib/auth/middlewares/__tests__/validate-sso-callback-urls.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, test } from "bun:test"; +import type { AuthMiddlewareContext } from "~/server/lib/auth"; +import { validateSsoCallbackUrls } from "../validate-sso-callback-urls"; + +function createContext(path: string, body: Record): AuthMiddlewareContext { + return { + path, + body, + query: {}, + headers: new Headers(), + request: new Request(`http://localhost:3000${path}`), + params: {}, + method: "POST", + context: {} as AuthMiddlewareContext["context"], + } as AuthMiddlewareContext; +} + +describe("validateSsoCallbackUrls", () => { + test("accepts /login", async () => { + const ctx = createContext("/sign-in/sso", { callbackURL: "/login" }); + + expect(validateSsoCallbackUrls(ctx)).resolves.toBeUndefined(); + }); + + test("rejects https://evil.example", async () => { + const ctx = createContext("/sign-in/sso", { callbackURL: "https://evil.example" }); + + expect(validateSsoCallbackUrls(ctx)).rejects.toThrow("callbackURL"); + }); + + test("rejects //evil.example", async () => { + const ctx = createContext("/sign-in/sso", { callbackURL: "//evil.example" }); + + expect(validateSsoCallbackUrls(ctx)).rejects.toThrow("callbackURL"); + }); + + test("rejects /sso/callback/foo", async () => { + const ctx = createContext("/sign-in/sso", { callbackURL: "/sso/callback/foo" }); + + expect(validateSsoCallbackUrls(ctx)).rejects.toThrow("callbackURL"); + }); + + test("rejects /sso/saml2/foo", async () => { + const ctx = createContext("/sign-in/sso", { callbackURL: "/sso/saml2/foo" }); + + expect(validateSsoCallbackUrls(ctx)).rejects.toThrow("callbackURL"); + }); +}); diff --git a/app/server/lib/auth/middlewares/trust-sso-provider-for-linking.ts b/app/server/lib/auth/middlewares/trust-sso-provider-for-linking.ts index e12f3c31..233dda2f 100644 --- a/app/server/lib/auth/middlewares/trust-sso-provider-for-linking.ts +++ b/app/server/lib/auth/middlewares/trust-sso-provider-for-linking.ts @@ -23,15 +23,21 @@ export async function trustSsoProviderForLinking(ctx: GenericEndpointContext): P return; } - const provider = await db.query.ssoProvider.findFirst({ where: { providerId, autoLinkMatchingEmails: true } }); + const provider = await db.query.ssoProvider.findFirst({ + columns: { organizationId: true }, + where: { providerId }, + }); if (!provider) { return; } - const trustedProviders = accountLinking.trustedProviders ?? []; - if (trustedProviders.includes(providerId)) { - return; - } + const autoLinkingProviders = await db.query.ssoProvider.findMany({ + columns: { providerId: true }, + where: { + organizationId: provider.organizationId, + autoLinkMatchingEmails: true, + }, + }); - accountLinking.trustedProviders = [...trustedProviders, providerId]; + accountLinking.trustedProviders = autoLinkingProviders.map((entry) => entry.providerId); } diff --git a/app/server/lib/auth/middlewares/validate-sso-callback-urls.ts b/app/server/lib/auth/middlewares/validate-sso-callback-urls.ts new file mode 100644 index 00000000..873f3cdc --- /dev/null +++ b/app/server/lib/auth/middlewares/validate-sso-callback-urls.ts @@ -0,0 +1,36 @@ +import { APIError } from "better-auth/api"; +import type { AuthMiddlewareContext } from "~/server/lib/auth"; + +function isValidCallbackPath(value: string): boolean { + if (!value.startsWith("/") || value.startsWith("//") || value.includes("\\")) { + return false; + } + + if (value.startsWith("/sso/callback/") || value.startsWith("/sso/saml2/")) { + return false; + } + + return true; +} + +export const validateSsoCallbackUrls = async (ctx: AuthMiddlewareContext) => { + if (ctx.path !== "/sign-in/sso") { + return; + } + + const sources = [ctx.body, ctx.query].filter((s) => s && typeof s === "object"); + + for (const source of sources) { + const payload = source as Record; + + for (const field of ["callbackURL", "errorCallbackURL", "newUserCallbackURL"]) { + const value = payload[field]; + + if (value !== undefined && (typeof value !== "string" || !isValidCallbackPath(value))) { + throw new APIError("BAD_REQUEST", { + message: `Invalid ${field}. Only relative paths like /login are allowed.`, + }); + } + } + } +}; diff --git a/app/server/modules/auth/__tests__/auth.sso-security.test.ts b/app/server/modules/auth/__tests__/auth.sso-security.test.ts new file mode 100644 index 00000000..eb15e105 --- /dev/null +++ b/app/server/modules/auth/__tests__/auth.sso-security.test.ts @@ -0,0 +1,54 @@ +import { beforeEach, describe, expect, test } from "bun:test"; +import { createApp } from "~/server/app"; +import { account, invitation, member, organization, ssoProvider, usersTable } from "~/server/db/schema"; +import { db } from "~/server/db/db"; + +const app = createApp(); + +describe("auth SSO sign-in security", () => { + beforeEach(async () => { + await db.delete(member); + await db.delete(account); + await db.delete(invitation); + await db.delete(ssoProvider); + await db.delete(organization); + await db.delete(usersTable); + }); + + test("rejects malicious callback URL", async () => { + const response = await app.request("/api/auth/sign-in/sso", { + method: "POST", + headers: { + "Content-Type": "application/json", + }, + body: JSON.stringify({ + providerId: "missing-provider", + callbackURL: "https://evil.example", + }), + }); + + expect(response.status).toBe(400); + + const body = await response.json(); + expect(body.code).toContain("CALLBACKURL"); + expect(body.message).toContain("callbackURL"); + }); + + test("allows relative callback URL to continue normal flow", async () => { + const response = await app.request("/api/auth/sign-in/sso", { + method: "POST", + headers: { + "Content-Type": "application/json", + }, + body: JSON.stringify({ + providerId: "missing-provider", + callbackURL: "/login", + }), + }); + + expect(response.status).toBe(404); + + const body = await response.json(); + expect(body.code).toBe("NO_PROVIDER_FOUND_FOR_THE_ISSUER"); + }); +});