From e202a0c8468de80918115a54659a8c4cd07b4d33 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 29 Oct 2026 16:38:00 +0100 Subject: [PATCH 1/3] feat: add envUrl helper to env-schema Validates an http or https URL from an environment variable, treating a blank value as unset. Used by the Trino and OIDC settings in the following commits (#402). --- src/lib/env-schema.test.ts | 21 ++++++++++++++++++++- src/lib/env-schema.ts | 3 +++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/lib/env-schema.test.ts b/src/lib/env-schema.test.ts index df99abe0..703197e0 100644 --- a/src/lib/env-schema.test.ts +++ b/src/lib/env-schema.test.ts @@ -1,6 +1,13 @@ import { describe, it, expect } from 'vitest'; import { z } from 'zod'; -import { envBoolean, envInteger, envList, envOptionalInteger, parseEnv } from './env-schema.js'; +import { + envBoolean, + envInteger, + envList, + envOptionalInteger, + envUrl, + parseEnv +} from './env-schema.js'; describe('envBoolean', () => { const schema = envBoolean(false); @@ -42,6 +49,18 @@ describe('envOptionalInteger', () => { }); }); +describe('envUrl', () => { + it('accepts http and https URLs and is undefined when blank', () => { + expect(envUrl.parse(' https://trino.example.com:8443 ')).toBe('https://trino.example.com:8443'); + expect(envUrl.parse('')).toBeUndefined(); + }); + + it('rejects values without an http or https scheme', () => { + expect(envUrl.safeParse('trino.example.com:8443').success).toBe(false); + expect(envUrl.safeParse('ftp://trino.example.com').success).toBe(false); + }); +}); + describe('envList', () => { it('splits, trims and drops empty entries', () => { expect(envList(['a']).parse(' x , ,y ')).toEqual(['x', 'y']); diff --git a/src/lib/env-schema.ts b/src/lib/env-schema.ts index 4a40f54d..d36de482 100644 --- a/src/lib/env-schema.ts +++ b/src/lib/env-schema.ts @@ -40,6 +40,9 @@ export const envOptionalInteger = z.preprocess( /** A trimmed string, or `undefined` when unset. */ export const envString = z.preprocess(blankAsUnset, z.string().optional()); +/** An `http` or `https` URL, or `undefined` when unset. */ +export const envUrl = z.preprocess(blankAsUnset, z.url({ protocol: /^https?$/ }).optional()); + /** A list of non-empty entries separated by `separator`. */ export function envList(defaultValue: string[], separator: RegExp | string = ',') { return z.preprocess( From cc1eb0d2ad7f99bc35772b0226abe89305736f7d Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 29 Oct 2026 16:38:13 +0100 Subject: [PATCH 2/3] fix: validate Trino and query settings with zod (#402) STACKABLE_COCKPIT_TRINO_AUTH_TYPE, _TRINO_TLS_INSECURE, _TRINO_USER_IMPERSONATION, _QUERY_TTL and the Trino URLs fell back to defaults on invalid input, so a typo could silently disable TLS verification or impersonation. They are now parsed once at startup, and invalid values stop the server with an error naming each variable. --- .env.example | 1 + src/hooks.server.ts | 6 ++-- src/lib/server/trino/client.ts | 26 +++++++-------- src/lib/server/trino/config.test.ts | 46 ++++++++++++++++++++++++++ src/lib/server/trino/config.ts | 51 +++++++++++++++++++++++++++++ src/lib/server/trino/queries.ts | 4 +-- 6 files changed, 115 insertions(+), 19 deletions(-) create mode 100644 src/lib/server/trino/config.test.ts create mode 100644 src/lib/server/trino/config.ts diff --git a/.env.example b/.env.example index 2bd6f7f4..2829a94b 100644 --- a/.env.example +++ b/.env.example @@ -42,6 +42,7 @@ DATABASE_PASSWORD=change-me # When STACKABLE_COCKPIT_TRINO_URL is set, the connection form is hidden and # the specified Trino endpoint is used automatically. By default the logged-in # user is forwarded to Trino as X-Trino-User (impersonation via the OIDC username). +# Invalid values stop the server at startup. # STACKABLE_COCKPIT_TRINO_URL=https://trino.example.com:8443 # STACKABLE_COCKPIT_TRINO_PUBLIC_URL=https://trino.example.com # Browser-facing Trino URL for "View in Trino" links (default: the Trino URL used for queries) # STACKABLE_COCKPIT_TRINO_AUTH_TYPE=basic # "none" (default) or "basic" diff --git a/src/hooks.server.ts b/src/hooks.server.ts index 6df0fe73..29787893 100644 --- a/src/hooks.server.ts +++ b/src/hooks.server.ts @@ -10,9 +10,11 @@ import { requestLogger, logger } from '$lib/server/logging'; import { getConnectionFromHeader } from '$lib/server/storage/connection.js'; import { STORAGE_CONNECTION_ID_HEADER } from '$lib/storage/connection-id-header.js'; import { storageBrowserEnabled } from '$lib/server/feature-flags.js'; -// Imported for its side effect: parses the storage and PUBLIC_ client config at -// startup, so invalid values stop the server instead of failing the first request. +// Imported for their side effect: parse the storage, PUBLIC_ client and Trino +// config at startup, so invalid values stop the server instead of failing the +// first request. import '$lib/server/storage/config.js'; +import '$lib/server/trino/config.js'; import { storageEncryptionKey } from '$lib/server/storage/encryption-key.js'; import { notConnected } from '$lib/server/storage/not-connected.js'; import { runMigrations } from '$lib/server/migrate.js'; diff --git a/src/lib/server/trino/client.ts b/src/lib/server/trino/client.ts index 0fb801ab..04549c2b 100644 --- a/src/lib/server/trino/client.ts +++ b/src/lib/server/trino/client.ts @@ -1,7 +1,7 @@ -import { env } from '$env/dynamic/private'; import { readFileSync } from 'node:fs'; import { Agent } from 'undici'; import { logger } from '$lib/server/logging'; +import { trinoConfig } from './config.js'; import { getUserTrinoClient, getUserTrinoUrl } from './user-clients.js'; const log = logger.child({ module: 'trino-client' }); @@ -168,15 +168,17 @@ export function buildBasicAuthHeader(username: string, password: string): string // --- Singleton & env config --- -const trinoUrl = env.STACKABLE_COCKPIT_TRINO_URL; -const trinoPublicUrl = env.STACKABLE_COCKPIT_TRINO_PUBLIC_URL; -const authType = (env.STACKABLE_COCKPIT_TRINO_AUTH_TYPE ?? 'none') as 'none' | 'basic'; -const authUsername = env.STACKABLE_COCKPIT_TRINO_AUTH_USERNAME; -const authPassword = env.STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD; -const tlsInsecure = env.STACKABLE_COCKPIT_TRINO_TLS_INSECURE === 'true'; -const tlsCaCertPath = env.STACKABLE_COCKPIT_TRINO_TLS_CA_CERT; +const { + url: trinoUrl, + publicUrl: trinoPublicUrl, + authType, + authUsername, + authPassword, + tlsInsecure, + tlsCaCertPath +} = trinoConfig; -export const trinoUserImpersonation = env.STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION !== 'false'; +export const trinoUserImpersonation = trinoConfig.userImpersonation; /** True when the Trino connection is pre-configured via environment variables. */ export const trinoConfigured = !!trinoUrl; @@ -184,12 +186,6 @@ export const trinoConfigured = !!trinoUrl; let singleton: TrinoClient | undefined; if (trinoConfigured) { - if (authType === 'basic' && (!authUsername || !authPassword)) { - throw new Error( - 'STACKABLE_COCKPIT_TRINO_AUTH_TYPE is "basic" but STACKABLE_COCKPIT_TRINO_AUTH_USERNAME or STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD is missing' - ); - } - let dispatcher: Agent | undefined; if (tlsInsecure || tlsCaCertPath) { dispatcher = new Agent({ diff --git a/src/lib/server/trino/config.test.ts b/src/lib/server/trino/config.test.ts new file mode 100644 index 00000000..522d6305 --- /dev/null +++ b/src/lib/server/trino/config.test.ts @@ -0,0 +1,46 @@ +import { describe, it, expect, vi } from 'vitest'; + +vi.mock('$env/dynamic/private', () => ({ env: {} })); + +import { parseTrinoConfig } from './config.js'; + +const TRINO_URL = 'https://trino.example.com:8443'; + +describe('parseTrinoConfig', () => { + it('uses the defaults when nothing is set', () => { + expect(parseTrinoConfig({})).toEqual({ + url: undefined, + publicUrl: undefined, + authType: 'none', + authUsername: undefined, + authPassword: undefined, + tlsInsecure: false, + tlsCaCertPath: undefined, + userImpersonation: true, + queryTtlSeconds: 1800 + }); + }); + + it.each([ + ['STACKABLE_COCKPIT_TRINO_URL', 'trino.example.com:8443'], + ['STACKABLE_COCKPIT_TRINO_AUTH_TYPE', 'oauth2'], + ['STACKABLE_COCKPIT_TRINO_TLS_INSECURE', 'yes'], + ['STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION', 'off'], + ['STACKABLE_COCKPIT_QUERY_TTL', '30m'] + ])('rejects an invalid %s', (name, value) => { + expect(() => + parseTrinoConfig({ STACKABLE_COCKPIT_TRINO_URL: TRINO_URL, [name]: value }) + ).toThrow(`${name}:`); + }); + + it('requires credentials for basic auth when a Trino URL is set', () => { + const basic = { + STACKABLE_COCKPIT_TRINO_AUTH_TYPE: 'basic', + STACKABLE_COCKPIT_TRINO_AUTH_USERNAME: 'svc' + }; + expect(() => parseTrinoConfig({ ...basic, STACKABLE_COCKPIT_TRINO_URL: TRINO_URL })).toThrow( + /STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD is missing/ + ); + expect(parseTrinoConfig(basic).authType).toBe('basic'); + }); +}); diff --git a/src/lib/server/trino/config.ts b/src/lib/server/trino/config.ts new file mode 100644 index 00000000..6bd79f1c --- /dev/null +++ b/src/lib/server/trino/config.ts @@ -0,0 +1,51 @@ +// Trino connection and query settings. Parsed once at module init so a typo +// fails at startup instead of silently changing TLS or impersonation behaviour. + +import { z } from 'zod'; +import { env } from '$env/dynamic/private'; +import { envBoolean, envInteger, envString, envUrl, parseEnv } from '$lib/env-schema.js'; + +const schema = z + .object({ + STACKABLE_COCKPIT_TRINO_URL: envUrl, + STACKABLE_COCKPIT_TRINO_PUBLIC_URL: envUrl, + STACKABLE_COCKPIT_TRINO_AUTH_TYPE: envString.pipe(z.enum(['none', 'basic']).default('none')), + STACKABLE_COCKPIT_TRINO_AUTH_USERNAME: envString, + STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD: envString, + STACKABLE_COCKPIT_TRINO_TLS_INSECURE: envBoolean(false), + STACKABLE_COCKPIT_TRINO_TLS_CA_CERT: envString, + STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION: envBoolean(true), + STACKABLE_COCKPIT_QUERY_TTL: envInteger(1800) + }) + .refine( + (config) => + !config.STACKABLE_COCKPIT_TRINO_URL || + config.STACKABLE_COCKPIT_TRINO_AUTH_TYPE !== 'basic' || + (config.STACKABLE_COCKPIT_TRINO_AUTH_USERNAME && + config.STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD), + { + path: ['STACKABLE_COCKPIT_TRINO_AUTH_TYPE'], + message: + 'is "basic" but STACKABLE_COCKPIT_TRINO_AUTH_USERNAME or STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD is missing' + } + ); + +export function parseTrinoConfig(env: Record) { + const config = parseEnv(schema, env); + return { + url: config.STACKABLE_COCKPIT_TRINO_URL, + publicUrl: config.STACKABLE_COCKPIT_TRINO_PUBLIC_URL, + authType: config.STACKABLE_COCKPIT_TRINO_AUTH_TYPE, + authUsername: config.STACKABLE_COCKPIT_TRINO_AUTH_USERNAME, + authPassword: config.STACKABLE_COCKPIT_TRINO_AUTH_PASSWORD, + tlsInsecure: config.STACKABLE_COCKPIT_TRINO_TLS_INSECURE, + tlsCaCertPath: config.STACKABLE_COCKPIT_TRINO_TLS_CA_CERT, + userImpersonation: config.STACKABLE_COCKPIT_TRINO_USER_IMPERSONATION, + queryTtlSeconds: config.STACKABLE_COCKPIT_QUERY_TTL + }; +} + +/** Trino settings from the environment. The connection fields are only used + * when `STACKABLE_COCKPIT_TRINO_URL` is set; otherwise users configure their + * own connection. Impersonation also applies to those per-user connections. */ +export const trinoConfig = parseTrinoConfig(env); diff --git a/src/lib/server/trino/queries.ts b/src/lib/server/trino/queries.ts index 6161be8c..51d91671 100644 --- a/src/lib/server/trino/queries.ts +++ b/src/lib/server/trino/queries.ts @@ -1,4 +1,3 @@ -import { env } from '$env/dynamic/private'; import { logger } from '$lib/server/logging'; import { trinoActiveQueries, trinoQueryTotal } from '$lib/server/metrics.js'; import { @@ -10,6 +9,7 @@ import { type QueryState } from '$lib/types/query.js'; import { type TrinoClient, type TrinoQueryStats, resolveTrinoPublicUrl } from './client.js'; +import { trinoConfig } from './config.js'; import { collectResults } from './result-collector.js'; const log = logger.child({ module: 'trino-queries' }); @@ -17,7 +17,7 @@ const log = logger.child({ module: 'trino-queries' }); // --- TTL & eviction configuration --- /** Time (seconds) before completed, unaccessed tab queries are evicted. */ -const QUERY_TTL = Number(env.STACKABLE_COCKPIT_QUERY_TTL) || 1800; // 30 min +const QUERY_TTL = trinoConfig.queryTtlSeconds; const SWEEP_INTERVAL_MS = 60_000; // --- TrinoQuery model --- From ac765678b393227ecba8a2d1472209e2aa1aa8eb Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 29 Oct 2026 16:38:17 +0100 Subject: [PATCH 3/3] fix: validate OIDC settings and reject partial OIDC config (#402) OIDC was silently disabled when only some of STACKABLE_COCKPIT_OIDC_DISCOVERY_URL, _CLIENT_ID and _CLIENT_SECRET were set. Setting only some of them now stops the server at startup, and the discovery URL must be an http or https URL. --- .env.example | 3 ++- src/lib/server/auth.test.ts | 17 ++++++++++++- src/lib/server/auth.ts | 51 +++++++++++++++++++++++++------------ 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/.env.example b/.env.example index 2829a94b..815c3004 100644 --- a/.env.example +++ b/.env.example @@ -7,7 +7,8 @@ STACKABLE_COCKPIT_BASE_URL=http://localhost:5173 # OIDC discovery URL (Keycloak, Entra ID, or any compliant OIDC provider) STACKABLE_COCKPIT_OIDC_DISCOVERY_URL=https://your-idp.example.com/realms/your-realm/.well-known/openid-configuration -# The client ID and secret registered in your OIDC provider +# The client ID and secret registered in your OIDC provider. Set all three OIDC +# variables or none; setting only some of them stops the server at startup. STACKABLE_COCKPIT_OIDC_CLIENT_ID=stackable-cockpit STACKABLE_COCKPIT_OIDC_CLIENT_SECRET=your-client-secret diff --git a/src/lib/server/auth.test.ts b/src/lib/server/auth.test.ts index b188d8cf..01c1cd60 100644 --- a/src/lib/server/auth.test.ts +++ b/src/lib/server/auth.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { parseAdditionalUserInputFromProviderProfile } from 'better-auth/db'; import { auth } from './auth.js'; @@ -37,3 +37,18 @@ describe('user.additionalFields.trinoUser', () => { await expect(update({ emailVerified: true }, '/update-user')).resolves.toBeUndefined(); }); }); + +describe('OIDC configuration', () => { + it('fails at startup when only some of the OIDC variables are set', async () => { + vi.resetModules(); + vi.doMock('$env/dynamic/private', () => ({ + env: { STACKABLE_COCKPIT_OIDC_CLIENT_ID: 'cockpit' } + })); + try { + await expect(import('./auth.js')).rejects.toThrow(/set all or none of/); + } finally { + vi.doUnmock('$env/dynamic/private'); + vi.resetModules(); + } + }); +}); diff --git a/src/lib/server/auth.ts b/src/lib/server/auth.ts index 8564a0ed..ee980fdf 100644 --- a/src/lib/server/auth.ts +++ b/src/lib/server/auth.ts @@ -4,8 +4,10 @@ import { drizzleAdapter } from '@better-auth/drizzle-adapter'; import { genericOAuth, openAPI } from 'better-auth/plugins'; import { sveltekitCookies } from 'better-auth/svelte-kit'; import type { RequestEvent } from '@sveltejs/kit'; +import { z } from 'zod'; import { db } from './db.js'; import * as schema from './auth-schema.js'; +import { envList, envString, envUrl, parseEnv } from '../env-schema.js'; // Dynamic imports with fallbacks for non-SvelteKit contexts. const envModule = await import('$env/dynamic/private').catch(() => null); @@ -28,23 +30,40 @@ function guardTrinoUserWrite(data: Record, context: { path?: st throw new APIError('FORBIDDEN', { message: 'trinoUser is not allowed to be set' }); } -const trinoUserClaim = env.STACKABLE_COCKPIT_TRINO_USER_CLAIM ?? 'preferred_username'; +const OIDC_REQUIRED = [ + 'STACKABLE_COCKPIT_OIDC_DISCOVERY_URL', + 'STACKABLE_COCKPIT_OIDC_CLIENT_ID', + 'STACKABLE_COCKPIT_OIDC_CLIENT_SECRET' +] as const; -const defaultScopes = ['openid', 'profile', 'email']; +// OIDC is enabled only when all required variables are set. Setting only some +// of them is a startup error, not a silently disabled login. +const config = parseEnv( + z + .object({ + STACKABLE_COCKPIT_OIDC_DISCOVERY_URL: envUrl, + STACKABLE_COCKPIT_OIDC_CLIENT_ID: envString, + STACKABLE_COCKPIT_OIDC_CLIENT_SECRET: envString, + STACKABLE_COCKPIT_OIDC_EXTRA_SCOPES: envList([], /[\s,]+/), + STACKABLE_COCKPIT_TRINO_USER_CLAIM: envString.pipe(z.string().default('preferred_username')) + }) + .refine( + (values) => { + const set = OIDC_REQUIRED.filter((name) => values[name]).length; + return set === 0 || set === OIDC_REQUIRED.length; + }, + { message: `set all or none of ${OIDC_REQUIRED.join(', ')}` } + ), + env +); -const extraScopes = (env.STACKABLE_COCKPIT_OIDC_EXTRA_SCOPES ?? '') - .split(/[\s,]+/) - .map((scope) => scope.trim()) - .filter(Boolean); +const trinoUserClaim = config.STACKABLE_COCKPIT_TRINO_USER_CLAIM; -const oidcScopes = [...new Set([...defaultScopes, ...extraScopes])]; +const defaultScopes = ['openid', 'profile', 'email']; -// OIDC is enabled only when all required OIDC env vars are present. -export const oidcEnabled = !!( - env.STACKABLE_COCKPIT_OIDC_DISCOVERY_URL && - env.STACKABLE_COCKPIT_OIDC_CLIENT_ID && - env.STACKABLE_COCKPIT_OIDC_CLIENT_SECRET -); +const oidcScopes = [...new Set([...defaultScopes, ...config.STACKABLE_COCKPIT_OIDC_EXTRA_SCOPES])]; + +export const oidcEnabled = OIDC_REQUIRED.every((name) => config[name]); export const auth = betterAuth({ secret: env.STACKABLE_COCKPIT_SESSION_SECRET, @@ -89,9 +108,9 @@ export const auth = betterAuth({ config: [ { providerId: 'oidc', - discoveryUrl: env.STACKABLE_COCKPIT_OIDC_DISCOVERY_URL, - clientId: env.STACKABLE_COCKPIT_OIDC_CLIENT_ID!, - clientSecret: env.STACKABLE_COCKPIT_OIDC_CLIENT_SECRET!, + discoveryUrl: config.STACKABLE_COCKPIT_OIDC_DISCOVERY_URL!, + clientId: config.STACKABLE_COCKPIT_OIDC_CLIENT_ID!, + clientSecret: config.STACKABLE_COCKPIT_OIDC_CLIENT_SECRET!, scopes: oidcScopes, pkce: true, // Refresh the user from the IdP on every login, so changes to the