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) <noreply@anthropic.com>
This commit is contained in:
@@ -53,7 +53,7 @@ export default function ProfileClient({ user, enabledProviders, apiTokens }: Pro
|
|||||||
const [copied, setCopied] = useState(false);
|
const [copied, setCopied] = useState(false);
|
||||||
|
|
||||||
const hasPassword = !!user.passwordHash;
|
const hasPassword = !!user.passwordHash;
|
||||||
const hasOAuth = user.provider !== "credentials";
|
const hasOAuth = !!user.provider && user.provider !== "credentials";
|
||||||
|
|
||||||
const handlePasswordChange = async () => {
|
const handlePasswordChange = async () => {
|
||||||
setError(null);
|
setError(null);
|
||||||
|
|||||||
@@ -676,10 +676,20 @@ export async function suppressWafRuleGloballyAction(ruleId: number): Promise<Act
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function redactProviderSecrets<T extends { clientId: string; clientSecret: string }>(provider: T): T {
|
||||||
|
const clientId = provider.clientId;
|
||||||
|
return {
|
||||||
|
...provider,
|
||||||
|
clientId: clientId.length > 4 ? "••••" + clientId.slice(-4) : "••••",
|
||||||
|
clientSecret: "••••••••",
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
export async function getOAuthProvidersAction() {
|
export async function getOAuthProvidersAction() {
|
||||||
await requireAdmin();
|
await requireAdmin();
|
||||||
const { listOAuthProviders } = await import("@/src/lib/models/oauth-providers");
|
const { listOAuthProviders } = await import("@/src/lib/models/oauth-providers");
|
||||||
return listOAuthProviders();
|
const providers = await listOAuthProviders();
|
||||||
|
return providers.map(redactProviderSecrets);
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function createOAuthProviderAction(data: {
|
export async function createOAuthProviderAction(data: {
|
||||||
@@ -709,7 +719,7 @@ export async function createOAuthProviderAction(data: {
|
|||||||
data: JSON.stringify({ providerId: provider.id }),
|
data: JSON.stringify({ providerId: provider.id }),
|
||||||
});
|
});
|
||||||
revalidatePath("/settings");
|
revalidatePath("/settings");
|
||||||
return provider;
|
return redactProviderSecrets(provider);
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function updateOAuthProviderAction(
|
export async function updateOAuthProviderAction(
|
||||||
@@ -728,21 +738,40 @@ export async function updateOAuthProviderAction(
|
|||||||
enabled: boolean;
|
enabled: boolean;
|
||||||
}>
|
}>
|
||||||
) {
|
) {
|
||||||
await requireAdmin();
|
const session = await requireAdmin();
|
||||||
const { updateOAuthProvider } = await import("@/src/lib/models/oauth-providers");
|
const { updateOAuthProvider } = await import("@/src/lib/models/oauth-providers");
|
||||||
const { invalidateProviderCache } = await import("@/src/lib/auth-server");
|
const { invalidateProviderCache } = await import("@/src/lib/auth-server");
|
||||||
const updated = await updateOAuthProvider(id, data);
|
const updated = await updateOAuthProvider(id, data);
|
||||||
invalidateProviderCache();
|
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");
|
revalidatePath("/settings");
|
||||||
return updated;
|
return updated ? redactProviderSecrets(updated) : null;
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function deleteOAuthProviderAction(id: string) {
|
export async function deleteOAuthProviderAction(id: string) {
|
||||||
await requireAdmin();
|
const session = await requireAdmin();
|
||||||
const { deleteOAuthProvider } = await import("@/src/lib/models/oauth-providers");
|
const { getOAuthProvider, deleteOAuthProvider } = await import("@/src/lib/models/oauth-providers");
|
||||||
const { invalidateProviderCache } = await import("@/src/lib/auth-server");
|
const { invalidateProviderCache } = await import("@/src/lib/auth-server");
|
||||||
|
const existing = await getOAuthProvider(id);
|
||||||
await deleteOAuthProvider(id);
|
await deleteOAuthProvider(id);
|
||||||
invalidateProviderCache();
|
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");
|
revalidatePath("/settings");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -5,7 +5,7 @@ import db, { sqlite } from "./db";
|
|||||||
import * as schema from "./db/schema";
|
import * as schema from "./db/schema";
|
||||||
import { eq } from "drizzle-orm";
|
import { eq } from "drizzle-orm";
|
||||||
import { config } from "./config";
|
import { config } from "./config";
|
||||||
import { decryptSecret } from "./secret";
|
import { decryptSecret, encryptSecret, isEncryptedSecret } from "./secret";
|
||||||
import type { OAuthProvider } from "./models/oauth-providers";
|
import type { OAuthProvider } from "./models/oauth-providers";
|
||||||
import type { GenericOAuthConfig } from "better-auth/plugins";
|
import type { GenericOAuthConfig } from "better-auth/plugins";
|
||||||
|
|
||||||
@@ -82,6 +82,10 @@ function createAuth(): any {
|
|||||||
secret: config.sessionSecret,
|
secret: config.sessionSecret,
|
||||||
baseURL: config.baseUrl,
|
baseURL: config.baseUrl,
|
||||||
basePath: "/api/auth",
|
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],
|
trustedOrigins: [config.baseUrl],
|
||||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||||
advanced: {
|
advanced: {
|
||||||
@@ -91,8 +95,8 @@ function createAuth(): any {
|
|||||||
} as any,
|
} as any,
|
||||||
rateLimit: {
|
rateLimit: {
|
||||||
enabled: process.env.AUTH_RATE_LIMIT_ENABLED !== "false",
|
enabled: process.env.AUTH_RATE_LIMIT_ENABLED !== "false",
|
||||||
window: Number(process.env.AUTH_RATE_LIMIT_WINDOW ?? 10),
|
window: Number(process.env.AUTH_RATE_LIMIT_WINDOW ?? 60),
|
||||||
max: Number(process.env.AUTH_RATE_LIMIT_MAX ?? 200),
|
max: Number(process.env.AUTH_RATE_LIMIT_MAX ?? 5),
|
||||||
},
|
},
|
||||||
user: {
|
user: {
|
||||||
modelName: "users",
|
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: [
|
plugins: [
|
||||||
username(),
|
username(),
|
||||||
genericOAuth({ config: oauthConfigs }),
|
genericOAuth({ config: oauthConfigs }),
|
||||||
|
|||||||
@@ -209,13 +209,17 @@ function runEnvProviderSync() {
|
|||||||
let encryptSecret: (v: string) => string;
|
let encryptSecret: (v: string) => string;
|
||||||
try {
|
try {
|
||||||
encryptSecret = require("./secret").encryptSecret;
|
encryptSecret = require("./secret").encryptSecret;
|
||||||
} catch {
|
} catch (e) {
|
||||||
encryptSecret = (v) => v;
|
console.error("CRITICAL: Failed to load encryption module, refusing to store plaintext secrets:", e);
|
||||||
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
const name = config.oauth.providerName;
|
const name = config.oauth.providerName;
|
||||||
// Use a slug-based ID so the OAuth callback URL is predictable
|
// Use a slug-based ID so the OAuth callback URL is predictable, with hash suffix to avoid collisions
|
||||||
const providerId = name.toLowerCase().replace(/[^a-z0-9]+/g, "-").replace(/^-|-$/g, "") || "oauth";
|
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 existing = db.select().from(oauthProviders).where(eq(oauthProviders.name, name)).get();
|
||||||
|
|
||||||
const now = new Date().toISOString();
|
const now = new Date().toISOString();
|
||||||
|
|||||||
@@ -130,6 +130,15 @@ export async function updateUserPassword(userId: number, passwordHash: string):
|
|||||||
updatedAt: now
|
updatedAt: now
|
||||||
})
|
})
|
||||||
.where(eq(users.id, userId));
|
.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<User[]> {
|
export async function listUsers(): Promise<User[]> {
|
||||||
|
|||||||
Reference in New Issue
Block a user