From 8ce6eb619dec41078f48df76f338db609b2b503d Mon Sep 17 00:00:00 2001 From: Nicolas Meienberger Date: Fri, 27 Feb 2026 21:10:11 +0100 Subject: [PATCH] fix: prevent oidc provider to be named "credential" --- app/client/api-client/types.gen.ts | 4 + .../migration.sql | 6 +- .../snapshot.json | 51 +---- app/server/db/schema.ts | 48 ++--- app/server/lib/auth.ts | 2 + .../validate-sso-provider-id.test.ts | 42 +++++ .../middlewares/validate-sso-provider-id.ts | 25 +++ app/server/lib/auth/utils/sso-provider-id.ts | 9 + .../auth.service.sso-provider.test.ts | 177 ++++++++++++++++++ app/server/modules/auth/auth.controller.ts | 6 +- app/server/modules/auth/auth.dto.ts | 3 + app/server/modules/auth/auth.service.ts | 25 ++- playwright/dex-config.yaml | 12 +- 13 files changed, 323 insertions(+), 87 deletions(-) rename app/drizzle/{20260224193555_brief_magik => 20260227200731_sad_luke_cage}/migration.sql (65%) rename app/drizzle/{20260224193555_brief_magik => 20260227200731_sad_luke_cage}/snapshot.json (98%) create mode 100644 app/server/lib/auth/middlewares/__tests__/validate-sso-provider-id.test.ts create mode 100644 app/server/lib/auth/middlewares/validate-sso-provider-id.ts create mode 100644 app/server/lib/auth/utils/sso-provider-id.ts create mode 100644 app/server/modules/auth/__tests__/auth.service.sso-provider.test.ts diff --git a/app/client/api-client/types.gen.ts b/app/client/api-client/types.gen.ts index 7268f6db..c3bb01dc 100644 --- a/app/client/api-client/types.gen.ts +++ b/app/client/api-client/types.gen.ts @@ -90,6 +90,10 @@ export type DeleteSsoProviderErrors = { * Forbidden */ 403: unknown; + /** + * Provider not found + */ + 404: unknown; }; export type DeleteSsoProviderResponses = { diff --git a/app/drizzle/20260224193555_brief_magik/migration.sql b/app/drizzle/20260227200731_sad_luke_cage/migration.sql similarity index 65% rename from app/drizzle/20260224193555_brief_magik/migration.sql rename to app/drizzle/20260227200731_sad_luke_cage/migration.sql index bed46c74..8a0600cb 100644 --- a/app/drizzle/20260224193555_brief_magik/migration.sql +++ b/app/drizzle/20260227200731_sad_luke_cage/migration.sql @@ -1,6 +1,6 @@ CREATE TABLE `sso_provider` ( `id` text PRIMARY KEY, - `provider_id` text NOT NULL, + `provider_id` text NOT NULL UNIQUE, `organization_id` text NOT NULL, `user_id` text, `issuer` text NOT NULL, @@ -13,7 +13,3 @@ CREATE TABLE `sso_provider` ( CONSTRAINT `fk_sso_provider_organization_id_organization_id_fk` FOREIGN KEY (`organization_id`) REFERENCES `organization`(`id`) ON DELETE CASCADE, CONSTRAINT `fk_sso_provider_user_id_users_table_id_fk` FOREIGN KEY (`user_id`) REFERENCES `users_table`(`id`) ON DELETE SET NULL ); ---> statement-breakpoint -CREATE UNIQUE INDEX `sso_provider_provider_id_uidx` ON `sso_provider` (`provider_id`);--> statement-breakpoint -CREATE INDEX `sso_provider_organization_id_idx` ON `sso_provider` (`organization_id`);--> statement-breakpoint -CREATE INDEX `sso_provider_domain_idx` ON `sso_provider` (`domain`); diff --git a/app/drizzle/20260224193555_brief_magik/snapshot.json b/app/drizzle/20260227200731_sad_luke_cage/snapshot.json similarity index 98% rename from app/drizzle/20260224193555_brief_magik/snapshot.json rename to app/drizzle/20260227200731_sad_luke_cage/snapshot.json index 23e0f3bd..fda9380d 100644 --- a/app/drizzle/20260224193555_brief_magik/snapshot.json +++ b/app/drizzle/20260227200731_sad_luke_cage/snapshot.json @@ -1,7 +1,7 @@ { "version": "7", "dialect": "sqlite", - "id": "9fe017f6-0277-40aa-adbc-e165e20796fd", + "id": "60a53b0c-395a-4960-9b48-1740388bd2dc", "prevIds": ["3a308c54-d950-464f-9490-fee06985fbeb"], "ddl": [ { @@ -2075,48 +2075,6 @@ "entityType": "indexes", "table": "sessions_table" }, - { - "columns": [ - { - "value": "provider_id", - "isExpression": false - } - ], - "isUnique": true, - "where": null, - "origin": "manual", - "name": "sso_provider_provider_id_uidx", - "entityType": "indexes", - "table": "sso_provider" - }, - { - "columns": [ - { - "value": "organization_id", - "isExpression": false - } - ], - "isUnique": false, - "where": null, - "origin": "manual", - "name": "sso_provider_organization_id_idx", - "entityType": "indexes", - "table": "sso_provider" - }, - { - "columns": [ - { - "value": "domain", - "isExpression": false - } - ], - "isUnique": false, - "where": null, - "origin": "manual", - "name": "sso_provider_domain_idx", - "entityType": "indexes", - "table": "sso_provider" - }, { "columns": [ { @@ -2194,6 +2152,13 @@ "entityType": "uniques", "table": "sessions_table" }, + { + "columns": ["provider_id"], + "nameExplicit": false, + "name": "sso_provider_provider_id_unique", + "entityType": "uniques", + "table": "sso_provider" + }, { "columns": ["username"], "nameExplicit": false, diff --git a/app/server/db/schema.ts b/app/server/db/schema.ts index 29b605fb..da5873f5 100644 --- a/app/server/db/schema.ts +++ b/app/server/db/schema.ts @@ -173,34 +173,26 @@ export const invitation = sqliteTable( ], ); -export const ssoProvider = sqliteTable( - "sso_provider", - { - id: text("id").primaryKey(), - providerId: text("provider_id").notNull(), - organizationId: text("organization_id") - .notNull() - .references(() => organization.id, { onDelete: "cascade" }), - 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(false), - oidcConfig: text("oidc_config", { mode: "json" }).$type | null>(), - samlConfig: text("saml_config", { mode: "json" }).$type | null>(), - createdAt: integer("created_at", { mode: "timestamp_ms" }) - .notNull() - .default(sql`(unixepoch() * 1000)`), - updatedAt: integer("updated_at", { mode: "timestamp_ms" }) - .notNull() - .$onUpdate(() => new Date()) - .default(sql`(unixepoch() * 1000)`), - }, - (table) => [ - uniqueIndex("sso_provider_provider_id_uidx").on(table.providerId), - index("sso_provider_organization_id_idx").on(table.organizationId), - index("sso_provider_domain_idx").on(table.domain), - ], -); +export const ssoProvider = sqliteTable("sso_provider", { + id: text("id").primaryKey(), + providerId: text("provider_id").notNull().unique(), + organizationId: text("organization_id") + .notNull() + .references(() => organization.id, { onDelete: "cascade" }), + 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(false), + oidcConfig: text("oidc_config", { mode: "json" }).$type | null>(), + samlConfig: text("saml_config", { mode: "json" }).$type | null>(), + createdAt: integer("created_at", { mode: "timestamp_ms" }) + .notNull() + .default(sql`(unixepoch() * 1000)`), + updatedAt: integer("updated_at", { mode: "timestamp_ms" }) + .notNull() + .$onUpdate(() => new Date()) + .default(sql`(unixepoch() * 1000)`), +}); /** * Volumes Table diff --git a/app/server/lib/auth.ts b/app/server/lib/auth.ts index 343425d2..5fe675b3 100644 --- a/app/server/lib/auth.ts +++ b/app/server/lib/auth.ts @@ -18,6 +18,7 @@ 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 { validateSsoProviderId } from "./auth/middlewares/validate-sso-provider-id"; 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"; @@ -37,6 +38,7 @@ export const auth = betterAuth({ }, hooks: { before: createAuthMiddleware(async (ctx) => { + await validateSsoProviderId(ctx); await validateSsoCallbackUrls(ctx); await ensureOnlyOneUser(ctx); await convertLegacyUserOnFirstLogin(ctx); diff --git a/app/server/lib/auth/middlewares/__tests__/validate-sso-provider-id.test.ts b/app/server/lib/auth/middlewares/__tests__/validate-sso-provider-id.test.ts new file mode 100644 index 00000000..da5bbc4d --- /dev/null +++ b/app/server/lib/auth/middlewares/__tests__/validate-sso-provider-id.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, test } from "bun:test"; +import type { AuthMiddlewareContext } from "~/server/lib/auth"; +import { validateSsoProviderId } from "../validate-sso-provider-id"; + +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("validateSsoProviderId", () => { + test("allows non-reserved provider id", async () => { + const ctx = createContext("/sso/register", { providerId: "acme-oidc" }); + + expect(validateSsoProviderId(ctx)).resolves.toBeUndefined(); + }); + + test("rejects reserved credential provider id", async () => { + const ctx = createContext("/sso/register", { providerId: "credential" }); + + expect(validateSsoProviderId(ctx)).rejects.toThrow("reserved"); + }); + + test("rejects reserved credentials provider id case-insensitively", async () => { + const ctx = createContext("/sso/register", { providerId: " Credential " }); + + expect(validateSsoProviderId(ctx)).rejects.toThrow("reserved"); + }); + + test("skips validation outside register endpoint", async () => { + const ctx = createContext("/sign-in/sso", { providerId: "credential" }); + + expect(validateSsoProviderId(ctx)).resolves.toBeUndefined(); + }); +}); diff --git a/app/server/lib/auth/middlewares/validate-sso-provider-id.ts b/app/server/lib/auth/middlewares/validate-sso-provider-id.ts new file mode 100644 index 00000000..903a972c --- /dev/null +++ b/app/server/lib/auth/middlewares/validate-sso-provider-id.ts @@ -0,0 +1,25 @@ +import { APIError } from "better-auth/api"; +import type { AuthMiddlewareContext } from "~/server/lib/auth"; +import { isReservedSsoProviderId } from "../utils/sso-provider-id"; + +export const validateSsoProviderId = async (ctx: AuthMiddlewareContext) => { + if (ctx.path !== "/sso/register") { + return; + } + + if (!ctx.body || typeof ctx.body !== "object") { + return; + } + + const providerId = (ctx.body as Record).providerId; + + if (typeof providerId !== "string") { + return; + } + + if (isReservedSsoProviderId(providerId)) { + throw new APIError("BAD_REQUEST", { + message: `Invalid providerId. '${providerId}' is reserved and cannot be used for SSO providers.`, + }); + } +}; diff --git a/app/server/lib/auth/utils/sso-provider-id.ts b/app/server/lib/auth/utils/sso-provider-id.ts new file mode 100644 index 00000000..b76f8903 --- /dev/null +++ b/app/server/lib/auth/utils/sso-provider-id.ts @@ -0,0 +1,9 @@ +const RESERVED_SSO_PROVIDER_IDS = new Set(["credential"]); + +export function normalizeSsoProviderId(providerId: string): string { + return providerId.trim().toLowerCase(); +} + +export function isReservedSsoProviderId(providerId: string): boolean { + return RESERVED_SSO_PROVIDER_IDS.has(normalizeSsoProviderId(providerId)); +} diff --git a/app/server/modules/auth/__tests__/auth.service.sso-provider.test.ts b/app/server/modules/auth/__tests__/auth.service.sso-provider.test.ts new file mode 100644 index 00000000..cb7f6c8b --- /dev/null +++ b/app/server/modules/auth/__tests__/auth.service.sso-provider.test.ts @@ -0,0 +1,177 @@ +import { beforeEach, describe, expect, test } from "bun:test"; +import { db } from "~/server/db/db"; +import { account, invitation, member, organization, ssoProvider, usersTable } from "~/server/db/schema"; +import { authService } from "../auth.service"; + +function randomId() { + return Bun.randomUUIDv7(); +} + +function randomSlug(prefix: string) { + return `${prefix}-${Math.random().toString(36).slice(2, 8)}`; +} + +async function createUser(email: string) { + const id = randomId(); + + await db.insert(usersTable).values({ + id, + email, + name: email.split("@")[0], + username: randomSlug("user"), + }); + + return id; +} + +async function createOrganization(name: string) { + const id = randomId(); + + await db.insert(organization).values({ + id, + name, + slug: randomSlug("org"), + createdAt: new Date(), + }); + + return id; +} + +describe("authService.deleteSsoProvider", () => { + 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("does not delete accounts when provider belongs to another organization", async () => { + const orgA = await createOrganization("Org A"); + const orgB = await createOrganization("Org B"); + const providerOwner = await createUser(`${randomSlug("owner")}@example.com`); + const accountUser = await createUser(`${randomSlug("member")}@example.com`); + + const providerId = `oidc-${randomSlug("provider")}`; + + await db.insert(ssoProvider).values({ + id: randomId(), + providerId, + organizationId: orgB, + userId: providerOwner, + issuer: "https://issuer.example.com", + domain: "example.com", + }); + + await db.insert(account).values({ + id: randomId(), + accountId: randomSlug("acct"), + providerId, + userId: accountUser, + }); + + const deleted = await authService.deleteSsoProvider(providerId, orgA); + + expect(deleted).toBe(false); + + const remainingProvider = await db.query.ssoProvider.findFirst({ + where: { providerId }, + columns: { id: true }, + }); + const remainingAccounts = await db.query.account.findMany({ + where: { providerId }, + columns: { id: true }, + }); + + expect(remainingProvider).not.toBeUndefined(); + expect(remainingAccounts).toHaveLength(1); + }); + + test("deletes provider and linked accounts in the active organization", async () => { + const org = await createOrganization("Org A"); + const providerOwner = await createUser(`${randomSlug("owner")}@example.com`); + const accountUserA = await createUser(`${randomSlug("member-a")}@example.com`); + const accountUserB = await createUser(`${randomSlug("member-b")}@example.com`); + + const providerId = `oidc-${randomSlug("provider")}`; + + await db.insert(ssoProvider).values({ + id: randomId(), + providerId, + organizationId: org, + userId: providerOwner, + issuer: "https://issuer.example.com", + domain: "example.com", + }); + + await db.insert(account).values([ + { + id: randomId(), + accountId: randomSlug("acct-a"), + providerId, + userId: accountUserA, + }, + { + id: randomId(), + accountId: randomSlug("acct-b"), + providerId, + userId: accountUserB, + }, + ]); + + const deleted = await authService.deleteSsoProvider(providerId, org); + + expect(deleted).toBe(true); + + const remainingProvider = await db.query.ssoProvider.findFirst({ + where: { providerId }, + columns: { id: true }, + }); + const remainingAccounts = await db.query.account.findMany({ + where: { providerId }, + columns: { id: true }, + }); + + expect(remainingProvider).toBeUndefined(); + expect(remainingAccounts).toHaveLength(0); + }); + + test("deleting a reserved provider id never deletes credential accounts", async () => { + const org = await createOrganization("Org A"); + const providerOwner = await createUser(`${randomSlug("owner")}@example.com`); + const credentialUser = await createUser(`${randomSlug("credential")}@example.com`); + + await db.insert(ssoProvider).values({ + id: randomId(), + providerId: "credential", + organizationId: org, + userId: providerOwner, + issuer: "https://issuer.example.com", + domain: "example.com", + }); + + await db.insert(account).values({ + id: randomId(), + accountId: randomSlug("credential-acct"), + providerId: "credential", + userId: credentialUser, + }); + + const deleted = await authService.deleteSsoProvider("credential", org); + + expect(deleted).toBe(true); + + const remainingProvider = await db.query.ssoProvider.findFirst({ + where: { providerId: "credential" }, + columns: { id: true }, + }); + const remainingAccounts = await db.query.account.findMany({ + where: { providerId: "credential" }, + columns: { id: true }, + }); + + expect(remainingProvider).toBeUndefined(); + expect(remainingAccounts).toHaveLength(1); + }); +}); diff --git a/app/server/modules/auth/auth.controller.ts b/app/server/modules/auth/auth.controller.ts index 09a45f3e..b3f9c675 100644 --- a/app/server/modules/auth/auth.controller.ts +++ b/app/server/modules/auth/auth.controller.ts @@ -66,7 +66,11 @@ export const authController = new Hono() const providerId = c.req.param("providerId"); const organizationId = c.get("organizationId"); - await authService.deleteSsoProvider(providerId, organizationId); + const deleted = await authService.deleteSsoProvider(providerId, organizationId); + + if (!deleted) { + return c.json({ message: "Provider not found" }, 404); + } return c.json({ success: true }); }) diff --git a/app/server/modules/auth/auth.dto.ts b/app/server/modules/auth/auth.dto.ts index 3a0cd33f..de2f274e 100644 --- a/app/server/modules/auth/auth.dto.ts +++ b/app/server/modules/auth/auth.dto.ts @@ -157,6 +157,9 @@ export const deleteSsoProviderDto = describeRoute({ 200: { description: "SSO provider deleted successfully", }, + 404: { + description: "Provider not found", + }, 403: { description: "Forbidden", }, diff --git a/app/server/modules/auth/auth.service.ts b/app/server/modules/auth/auth.service.ts index c3d01a41..6e32d024 100644 --- a/app/server/modules/auth/auth.service.ts +++ b/app/server/modules/auth/auth.service.ts @@ -12,6 +12,7 @@ import { } from "../../db/schema"; import { eq, ne, and, count, inArray } from "drizzle-orm"; import type { UserDeletionImpactDto } from "./auth.dto"; +import { isReservedSsoProviderId } from "~/server/lib/auth/utils/sso-provider-id"; export class AuthService { /** @@ -108,11 +109,25 @@ export class AuthService { * Delete an SSO provider and its associated accounts */ async deleteSsoProvider(providerId: string, organizationId: string) { - await db.transaction(async (tx) => { - await tx.delete(account).where(eq(account.providerId, providerId)); - await tx - .delete(ssoProvider) - .where(and(eq(ssoProvider.providerId, providerId), eq(ssoProvider.organizationId, organizationId))); + return db.transaction(async (tx) => { + const provider = await tx.query.ssoProvider.findFirst({ + where: { AND: [{ providerId }, { organizationId }] }, + columns: { id: true, providerId: true }, + }); + + if (!provider) { + return false; + } + + if (isReservedSsoProviderId(provider.providerId)) { + await tx.delete(ssoProvider).where(eq(ssoProvider.id, provider.id)); + return true; + } + + await tx.delete(account).where(eq(account.providerId, provider.providerId)); + await tx.delete(ssoProvider).where(eq(ssoProvider.id, provider.id)); + + return true; }); } diff --git a/playwright/dex-config.yaml b/playwright/dex-config.yaml index d2e43267..15bed466 100644 --- a/playwright/dex-config.yaml +++ b/playwright/dex-config.yaml @@ -28,11 +28,8 @@ staticClients: - id: zerobyte-test name: Zerobyte Test redirectURIs: - - "http://localhost:3000/api/auth/sso/callback/test-oidc" - - "http://localhost:3000/api/auth/sso/callback/test-oidc-register" - - "http://localhost:3000/api/auth/sso/callback/test-oidc-uninvited" - - "http://localhost:3000/api/auth/sso/callback/test-oidc-invited" - - "http://localhost:3000/api/auth/sso/callback/test-oidc-autolink" + - "http://localhost:3000/api/auth/sso/callback/credential" + - "http://localhost:3000/api/auth/sso/callback/dex" - "http://localhost:4096/api/auth/sso/callback/test-oidc" - "http://localhost:4096/api/auth/sso/callback/test-oidc-register" - "http://localhost:4096/api/auth/sso/callback/test-oidc-uninvited" @@ -46,3 +43,8 @@ oauth2: logger: level: debug format: json + +# Issuer URL: http://dex:5557/dex +# Discovery URL: http://dex:5557/dex/.well-known/openid-configuration +# Client ID: zerobyte-test +# Client Secret: test-secret-12345