From 66f8e32df59cc3764ece4a58d55af36f9e3e84fa Mon Sep 17 00:00:00 2001 From: fuomag9 <1580624+fuomag9@users.noreply.github.com> Date: Sun, 12 Apr 2026 21:50:48 +0200 Subject: [PATCH] Fix security issues in Better Auth migration - Tighten login rate limit from 200/10s to 5/60s to prevent brute-force - Encrypt OAuth tokens (access/refresh/id) in accounts table via databaseHooks - Sync password changes to accounts.password so old passwords stop working - Redact OAuth client secrets in server actions before returning to client - Add trustHost config (default false) to prevent Host header poisoning - Add audit logging for successful logins via session create hook - Add audit logging to OAuth provider update/delete server actions - Fix provider ID collision by appending name hash suffix to slug - Fix nullable provider field causing incorrect hasOAuth detection - Refuse to store plaintext secrets if encryption module fails to load Co-Authored-By: Claude Opus 4.6 (1M context) --- app/(dashboard)/profile/ProfileClient.tsx | 2 +- app/(dashboard)/settings/actions.ts | 41 ++++++++++++++++--- src/lib/auth-server.ts | 50 +++++++++++++++++++++-- src/lib/db.ts | 12 ++++-- src/lib/models/user.ts | 9 ++++ 5 files changed, 100 insertions(+), 14 deletions(-) diff --git a/app/(dashboard)/profile/ProfileClient.tsx b/app/(dashboard)/profile/ProfileClient.tsx index a576f2bd..f110c2a1 100644 --- a/app/(dashboard)/profile/ProfileClient.tsx +++ b/app/(dashboard)/profile/ProfileClient.tsx @@ -53,7 +53,7 @@ export default function ProfileClient({ user, enabledProviders, apiTokens }: Pro const [copied, setCopied] = useState(false); const hasPassword = !!user.passwordHash; - const hasOAuth = user.provider !== "credentials"; + const hasOAuth = !!user.provider && user.provider !== "credentials"; const handlePasswordChange = async () => { setError(null); diff --git a/app/(dashboard)/settings/actions.ts b/app/(dashboard)/settings/actions.ts index c1ecd65b..a51d0871 100644 --- a/app/(dashboard)/settings/actions.ts +++ b/app/(dashboard)/settings/actions.ts @@ -676,10 +676,20 @@ export async function suppressWafRuleGloballyAction(ruleId: number): Promise(provider: T): T { + const clientId = provider.clientId; + return { + ...provider, + clientId: clientId.length > 4 ? "••••" + clientId.slice(-4) : "••••", + clientSecret: "••••••••", + }; +} + export async function getOAuthProvidersAction() { await requireAdmin(); const { listOAuthProviders } = await import("@/src/lib/models/oauth-providers"); - return listOAuthProviders(); + const providers = await listOAuthProviders(); + return providers.map(redactProviderSecrets); } export async function createOAuthProviderAction(data: { @@ -709,7 +719,7 @@ export async function createOAuthProviderAction(data: { data: JSON.stringify({ providerId: provider.id }), }); revalidatePath("/settings"); - return provider; + return redactProviderSecrets(provider); } export async function updateOAuthProviderAction( @@ -728,21 +738,40 @@ export async function updateOAuthProviderAction( enabled: boolean; }> ) { - await requireAdmin(); + const session = await requireAdmin(); const { updateOAuthProvider } = await import("@/src/lib/models/oauth-providers"); const { invalidateProviderCache } = await import("@/src/lib/auth-server"); const updated = await updateOAuthProvider(id, data); invalidateProviderCache(); + const { createAuditEvent } = await import("@/src/lib/models/audit"); + await createAuditEvent({ + userId: Number(session.user.id), + action: "oauth_provider_updated", + entityType: "oauth_provider", + entityId: null, + summary: `Updated OAuth provider "${id}"`, + data: JSON.stringify({ providerId: id, fields: Object.keys(data) }), + }); revalidatePath("/settings"); - return updated; + return updated ? redactProviderSecrets(updated) : null; } export async function deleteOAuthProviderAction(id: string) { - await requireAdmin(); - const { deleteOAuthProvider } = await import("@/src/lib/models/oauth-providers"); + const session = await requireAdmin(); + const { getOAuthProvider, deleteOAuthProvider } = await import("@/src/lib/models/oauth-providers"); const { invalidateProviderCache } = await import("@/src/lib/auth-server"); + const existing = await getOAuthProvider(id); await deleteOAuthProvider(id); invalidateProviderCache(); + const { createAuditEvent } = await import("@/src/lib/models/audit"); + await createAuditEvent({ + userId: Number(session.user.id), + action: "oauth_provider_deleted", + entityType: "oauth_provider", + entityId: null, + summary: `Deleted OAuth provider "${existing?.name ?? id}"`, + data: JSON.stringify({ providerId: id }), + }); revalidatePath("/settings"); } diff --git a/src/lib/auth-server.ts b/src/lib/auth-server.ts index 25c9d91f..29326f15 100644 --- a/src/lib/auth-server.ts +++ b/src/lib/auth-server.ts @@ -5,7 +5,7 @@ import db, { sqlite } from "./db"; import * as schema from "./db/schema"; import { eq } from "drizzle-orm"; import { config } from "./config"; -import { decryptSecret } from "./secret"; +import { decryptSecret, encryptSecret, isEncryptedSecret } from "./secret"; import type { OAuthProvider } from "./models/oauth-providers"; import type { GenericOAuthConfig } from "better-auth/plugins"; @@ -82,6 +82,10 @@ function createAuth(): any { secret: config.sessionSecret, baseURL: config.baseUrl, basePath: "/api/auth", + // Only trust the Host header when the operator explicitly opts in. + // baseURL already pins the canonical origin; trustHost is only needed + // behind reverse proxies that rewrite Host without setting X-Forwarded-Host. + trustHost: process.env.AUTH_TRUST_HOST === "true", trustedOrigins: [config.baseUrl], // eslint-disable-next-line @typescript-eslint/no-explicit-any advanced: { @@ -91,8 +95,8 @@ function createAuth(): any { } as any, rateLimit: { enabled: process.env.AUTH_RATE_LIMIT_ENABLED !== "false", - window: Number(process.env.AUTH_RATE_LIMIT_WINDOW ?? 10), - max: Number(process.env.AUTH_RATE_LIMIT_MAX ?? 200), + window: Number(process.env.AUTH_RATE_LIMIT_WINDOW ?? 60), + max: Number(process.env.AUTH_RATE_LIMIT_MAX ?? 5), }, user: { modelName: "users", @@ -126,6 +130,46 @@ function createAuth(): any { }, }, }, + databaseHooks: { + account: { + create: { + before: async (account) => { + const data = { ...account }; + if (data.accessToken) data.accessToken = encryptSecret(data.accessToken); + if (data.refreshToken) data.refreshToken = encryptSecret(data.refreshToken); + if (data.idToken) data.idToken = encryptSecret(data.idToken); + return { data }; + }, + }, + update: { + before: async (account) => { + const data = { ...account }; + if (data.accessToken && !isEncryptedSecret(data.accessToken)) data.accessToken = encryptSecret(data.accessToken); + if (data.refreshToken && !isEncryptedSecret(data.refreshToken)) data.refreshToken = encryptSecret(data.refreshToken); + if (data.idToken && !isEncryptedSecret(data.idToken)) data.idToken = encryptSecret(data.idToken); + return { data }; + }, + }, + }, + session: { + create: { + after: async (session) => { + try { + const { createAuditEvent } = await import("./models/audit"); + await createAuditEvent({ + userId: typeof session.userId === "string" ? Number(session.userId) : session.userId, + action: "login_success", + entityType: "session", + entityId: null, + summary: "User signed in", + }); + } catch { + // Don't break auth flow if audit logging fails + } + }, + }, + }, + }, plugins: [ username(), genericOAuth({ config: oauthConfigs }), diff --git a/src/lib/db.ts b/src/lib/db.ts index 8c4c0ee4..c0fdf310 100644 --- a/src/lib/db.ts +++ b/src/lib/db.ts @@ -209,13 +209,17 @@ function runEnvProviderSync() { let encryptSecret: (v: string) => string; try { encryptSecret = require("./secret").encryptSecret; - } catch { - encryptSecret = (v) => v; + } catch (e) { + console.error("CRITICAL: Failed to load encryption module, refusing to store plaintext secrets:", e); + return; } const name = config.oauth.providerName; - // Use a slug-based ID so the OAuth callback URL is predictable - const providerId = name.toLowerCase().replace(/[^a-z0-9]+/g, "-").replace(/^-|-$/g, "") || "oauth"; + // Use a slug-based ID so the OAuth callback URL is predictable, with hash suffix to avoid collisions + const slug = name.toLowerCase().replace(/[^a-z0-9]+/g, "-").replace(/^-|-$/g, "") || "oauth"; + // Append a short hash of the exact name to avoid collisions (e.g. "Google!" vs "Google?") + const nameHash = Buffer.from(name).toString("base64url").slice(0, 6); + const providerId = `${slug}-${nameHash}`; const existing = db.select().from(oauthProviders).where(eq(oauthProviders.name, name)).get(); const now = new Date().toISOString(); diff --git a/src/lib/models/user.ts b/src/lib/models/user.ts index abca03a5..ea6bbc5b 100644 --- a/src/lib/models/user.ts +++ b/src/lib/models/user.ts @@ -130,6 +130,15 @@ export async function updateUserPassword(userId: number, passwordHash: string): updatedAt: now }) .where(eq(users.id, userId)); + + // Also update the Better Auth credential account so the new password takes effect there too + await db + .update(accounts) + .set({ + password: passwordHash, + updatedAt: now, + }) + .where(and(eq(accounts.userId, userId), eq(accounts.providerId, "credential"))); } export async function listUsers(): Promise {