diff --git a/apps/dashboard/src/app.d.ts b/apps/dashboard/src/app.d.ts index fb4ea81..96f0814 100644 --- a/apps/dashboard/src/app.d.ts +++ b/apps/dashboard/src/app.d.ts @@ -4,6 +4,7 @@ declare module '@auth/sveltekit' { interface Session { accessToken?: string; idToken?: string; + error?: 'RefreshAccessTokenError'; } } @@ -13,6 +14,7 @@ declare module '@auth/core/jwt' { refreshToken?: string; idToken?: string; expiresAt?: number; + error?: 'RefreshAccessTokenError'; } } diff --git a/apps/dashboard/src/auth.ts b/apps/dashboard/src/auth.ts index cf9f4cb..a1a9d64 100644 --- a/apps/dashboard/src/auth.ts +++ b/apps/dashboard/src/auth.ts @@ -1,6 +1,7 @@ import { SvelteKitAuth } from '@auth/sveltekit'; import type { Provider } from '@auth/core/providers'; import { env } from '$env/dynamic/private'; +import { dev } from '$app/environment'; function zitadelProvider(): Provider { return { @@ -22,6 +23,46 @@ function zitadelProvider(): Provider { }; } +async function refreshAccessToken(token: import('@auth/core/jwt').JWT): Promise { + const issuer = env.AUTH_ZITADEL_ISSUER; + const clientId = env.AUTH_ZITADEL_CLIENT_ID; + const clientSecret = env.AUTH_ZITADEL_CLIENT_SECRET; + + if (!issuer || !clientId || !clientSecret || !token.refreshToken) { + return { ...token, error: 'RefreshAccessTokenError' }; + } + + const credentials = Buffer.from(`${clientId}:${clientSecret}`).toString('base64'); + + const response = await fetch(`${issuer}/oauth/v2/token`, { + method: 'POST', + headers: { + 'Authorization': `Basic ${credentials}`, + 'Content-Type': 'application/x-www-form-urlencoded' + }, + body: new URLSearchParams({ + grant_type: 'refresh_token', + refresh_token: token.refreshToken + }) + }); + + if (!response.ok) { + console.error('Token refresh failed:', response.status, await response.text()); + return { ...token, error: 'RefreshAccessTokenError' }; + } + + const data = await response.json(); + + return { + ...token, + accessToken: data.access_token, + idToken: data.id_token ?? token.idToken, + expiresAt: Math.floor(Date.now() / 1000) + (data.expires_in as number), + refreshToken: data.refresh_token ?? token.refreshToken, + error: undefined + }; +} + export const { handle, signIn, signOut } = SvelteKitAuth({ providers: [zitadelProvider()], callbacks: { @@ -31,17 +72,28 @@ export const { handle, signIn, signOut } = SvelteKitAuth({ token.refreshToken = account.refresh_token; token.idToken = account.id_token; token.expiresAt = account.expires_at; + return token; } - return token; + + // Return token if it hasn't expired yet (with 60s buffer) + if (token.expiresAt && Date.now() / 1000 < token.expiresAt - 60) { + return token; + } + + // Token has expired, attempt refresh + return refreshAccessToken(token); }, async session({ session, token }) { session.accessToken = token.accessToken as string; session.idToken = token.idToken as string; + if (token.error) { + session.error = token.error; + } return session; } }, pages: { signIn: '/login' }, - trustHost: true + trustHost: dev }); diff --git a/crates/pvm-auth/src/jwks.rs b/crates/pvm-auth/src/jwks.rs index 5e2855a..c132a53 100644 --- a/crates/pvm-auth/src/jwks.rs +++ b/crates/pvm-auth/src/jwks.rs @@ -1,7 +1,7 @@ use jsonwebtoken::jwk::JwkSet; use std::sync::Arc; use std::time::{Duration, Instant}; -use tokio::sync::RwLock; +use tokio::sync::{Mutex, RwLock}; use tracing::{info, warn}; const REFRESH_INTERVAL: Duration = Duration::from_secs(3600); // 1 hour @@ -9,6 +9,7 @@ const REFRESH_INTERVAL: Duration = Duration::from_secs(3600); // 1 hour #[derive(Clone)] pub struct JwksCache { inner: Arc>, + refresh_lock: Arc>, jwks_url: String, client: reqwest::Client, } @@ -30,13 +31,14 @@ impl JwksCache { jwks: None, last_refresh: None, })), + refresh_lock: Arc::new(Mutex::new(())), jwks_url, client: reqwest::Client::new(), } } pub async fn get_jwks(&self) -> Result { - // Check if we have a cached, non-stale JWKS + // Fast path: return cached JWKS if still fresh { let cache = self.inner.read().await; if let (Some(jwks), Some(last_refresh)) = (&cache.jwks, &cache.last_refresh) { @@ -46,7 +48,21 @@ impl JwksCache { } } - // Need to refresh + // Cache is stale or empty. Acquire the refresh lock so only one + // task performs the HTTP fetch; all others wait here. + let _refresh_guard = self.refresh_lock.lock().await; + + // Double-check: another task may have refreshed while we waited + { + let cache = self.inner.read().await; + if let (Some(jwks), Some(last_refresh)) = (&cache.jwks, &cache.last_refresh) { + if last_refresh.elapsed() < REFRESH_INTERVAL { + return Ok(jwks.clone()); + } + } + } + + // We won the race -- actually perform the refresh self.refresh().await } diff --git a/docker/setup-zitadel.sh b/docker/setup-zitadel.sh index bdedc30..927218b 100755 --- a/docker/setup-zitadel.sh +++ b/docker/setup-zitadel.sh @@ -21,6 +21,10 @@ DASHBOARD_ENV_FILE="${SCRIPT_DIR}/../apps/dashboard/.env" log() { printf '[setup-zitadel] %s\n' "$*" >&2; } fail() { log "ERROR: $*" >&2; exit 1; } +# ---------- preflight ---------- + +command -v jq >/dev/null 2>&1 || fail "jq is required but not installed. Install it with: apt install jq" + wait_for_zitadel() { log "Waiting for Zitadel to be healthy..." for _ in $(seq 1 30); do @@ -53,7 +57,7 @@ find_project() { -H "Content-Type: application/json" \ -d '{"queries":[{"nameQuery":{"name":"PVM","method":"TEXT_QUERY_METHOD_EQUALS"}}]}') || return 1 - PROJECT_ID=$(echo "$RESPONSE" | grep -o '"id":"[^"]*"' | head -1 | cut -d'"' -f4) + PROJECT_ID=$(printf '%s' "$RESPONSE" | jq -r '.result[0].id // empty') if [ -n "$PROJECT_ID" ]; then echo "$PROJECT_ID" return 0 @@ -68,13 +72,11 @@ find_oidc_app() { -H "Content-Type: application/json" \ -d '{}') || return 1 - # Look for "PVM Dashboard" in the app list - if echo "$RESPONSE" | grep -q '"name":"PVM Dashboard"'; then - CLIENT_ID=$(echo "$RESPONSE" | grep -o '"clientId":"[^"]*"' | head -1 | cut -d'"' -f4) - if [ -n "$CLIENT_ID" ]; then - echo "$CLIENT_ID" - return 0 - fi + # Look for "PVM Dashboard" in the app list and extract its clientId + CLIENT_ID=$(printf '%s' "$RESPONSE" | jq -r '.result[] | select(.name == "PVM Dashboard") | .oidcConfig.clientId // empty' 2>/dev/null) + if [ -n "$CLIENT_ID" ]; then + echo "$CLIENT_ID" + return 0 fi return 1 } @@ -87,7 +89,7 @@ create_project() { -d '{"name": "PVM"}') \ || fail "Failed to create project." - PROJECT_ID=$(echo "$RESPONSE" | grep -o '"id":"[^"]*"' | head -1 | cut -d'"' -f4) + PROJECT_ID=$(printf '%s' "$RESPONSE" | jq -r '.id // empty') [ -n "$PROJECT_ID" ] || fail "Could not extract project ID from response: $RESPONSE" log "Project created with ID: $PROJECT_ID" echo "$PROJECT_ID" @@ -95,28 +97,36 @@ create_project() { create_oidc_app() { PROJECT_ID="$1" - log "Creating OIDC application 'PVM Dashboard' in project $PROJECT_ID..." + + # devMode=true skips redirect URI validation (allows http://). Disable in production. + if [ "${ZITADEL_PRODUCTION:-}" = "true" ]; then + DEV_MODE="false" + else + DEV_MODE="true" + fi + + log "Creating OIDC application 'PVM Dashboard' in project $PROJECT_ID (devMode=$DEV_MODE)..." RESPONSE=$(curl -sf -X POST "${ZITADEL_URL}/management/v1/projects/${PROJECT_ID}/apps/oidc" \ -H "Authorization: Bearer ${PAT}" \ -H "Content-Type: application/json" \ - -d '{ - "name": "PVM Dashboard", - "redirectUris": ["http://localhost:5173/auth/callback/zitadel"], - "postLogoutRedirectUris": ["http://localhost:5173", "http://localhost:5173/login"], - "responseTypes": ["OIDC_RESPONSE_TYPE_CODE"], - "grantTypes": ["OIDC_GRANT_TYPE_AUTHORIZATION_CODE"], - "appType": "OIDC_APP_TYPE_WEB", - "authMethodType": "OIDC_AUTH_METHOD_TYPE_BASIC", - "devMode": true, - "accessTokenType": "OIDC_TOKEN_TYPE_BEARER", - "accessTokenRoleAssertion": true, - "idTokenRoleAssertion": true, - "idTokenUserinfoAssertion": true - }') \ + -d "{ + \"name\": \"PVM Dashboard\", + \"redirectUris\": [\"http://localhost:5173/auth/callback/zitadel\"], + \"postLogoutRedirectUris\": [\"http://localhost:5173\", \"http://localhost:5173/login\"], + \"responseTypes\": [\"OIDC_RESPONSE_TYPE_CODE\"], + \"grantTypes\": [\"OIDC_GRANT_TYPE_AUTHORIZATION_CODE\", \"OIDC_GRANT_TYPE_REFRESH_TOKEN\"], + \"appType\": \"OIDC_APP_TYPE_WEB\", + \"authMethodType\": \"OIDC_AUTH_METHOD_TYPE_BASIC\", + \"devMode\": ${DEV_MODE}, + \"accessTokenType\": \"OIDC_TOKEN_TYPE_BEARER\", + \"accessTokenRoleAssertion\": true, + \"idTokenRoleAssertion\": true, + \"idTokenUserinfoAssertion\": true + }") \ || fail "Failed to create OIDC app." - CLIENT_ID=$(echo "$RESPONSE" | grep -o '"clientId":"[^"]*"' | head -1 | cut -d'"' -f4) - CLIENT_SECRET=$(echo "$RESPONSE" | grep -o '"clientSecret":"[^"]*"' | head -1 | cut -d'"' -f4) + CLIENT_ID=$(printf '%s' "$RESPONSE" | jq -r '.clientId // empty') + CLIENT_SECRET=$(printf '%s' "$RESPONSE" | jq -r '.clientSecret // empty') [ -n "$CLIENT_ID" ] || fail "Could not extract client ID from response: $RESPONSE" log "OIDC app created." diff --git a/docs/TODO_SECURITY.md b/docs/TODO_SECURITY.md index b136578..ec0ec61 100644 --- a/docs/TODO_SECURITY.md +++ b/docs/TODO_SECURITY.md @@ -4,16 +4,6 @@ Items identified during code review that should be addressed before production. ## High Priority -### Token refresh logic in SvelteKit (`apps/dashboard/src/auth.ts`) -The JWT callback stores `expiresAt` and `refreshToken` but never checks expiry or initiates a refresh. Auth.js does not auto-refresh tokens. Without this, users get silently logged out when their access token expires (typically 5-15 minutes for Zitadel). - -**Fix:** Add expiry check in the `jwt` callback and use `refreshToken` to obtain a new access token when expired. - -### JWKS cache thundering herd (`crates/pvm-auth/src/jwks.rs`) -When the cache expires, every concurrent request sees stale cache and calls `refresh()` simultaneously. The `RwLock` serializes writes but each request still makes an HTTP call before acquiring the lock. - -**Fix:** Add a "refresh-in-progress" flag or use double-checked locking so only one request triggers the refresh while others wait. - ### Social login providers (Zitadel IDP configuration) Configure external identity providers in Zitadel so users can sign in with social accounts. Each requires creating OAuth2 credentials with the respective provider and registering them in Zitadel Console → Settings → Identity Providers. @@ -25,22 +15,33 @@ No code changes needed in the SvelteKit app — Zitadel's login page shows socia ## Medium Priority -### `trustHost: true` in auth.ts -Disables CSRF origin check in Auth.js. Required for local dev behind localhost, but must be removed or made conditional for production. - -### `devMode: true` in OIDC app config (`docker/setup-zitadel.sh`) -Disables redirect URI validation in Zitadel. The setup script is dev-only, but if a similar script is used for production, this must be `false`. - ### Custom login UI Replace the default Zitadel login v1 UI with a fully custom login/signup flow built into the SvelteKit dashboard using Zitadel's Session API. Includes: login, signup, password reset, 2FA flows. Must match PVM visual design. -## Low Priority +### Frontend handling of RefreshAccessTokenError +The session now exposes `session.error` when token refresh fails, but no frontend component checks this to redirect the user to `/login`. Add a check in the dashboard layout or a client-side hook. -### Shell script JSON parsing (`docker/setup-zitadel.sh`) -Uses `grep -o` and `cut` to extract JSON fields. Fragile if JSON format changes. Consider using `jq` with a fallback. +## Low Priority ### PAT expiration Machine user PAT expires 2030-01-01. Fine for dev, but production should use shorter-lived credentials. ### Running Zitadel as root (`docker-compose.dev.yml`) `user: "0"` is required for `start-from-init` to write the PAT file. Dev-only concern — production deployment should use proper volume permissions. + +## Resolved + +### ~~Token refresh logic~~ (done) +Added expiry check + refresh in `auth.ts` JWT callback. Uses `refreshToken` to call Zitadel's token endpoint with `client_secret_basic`. + +### ~~JWKS cache thundering herd~~ (done) +Added `tokio::sync::Mutex` refresh lock with double-checked locking in `jwks.rs`. Only one request triggers HTTP refresh; others wait. + +### ~~`trustHost: true`~~ (done) +Changed to `trustHost: dev` — uses SvelteKit's `$app/environment` to enable only in dev mode. + +### ~~`devMode: true`~~ (done) +Made conditional on `ZITADEL_PRODUCTION` env var in `setup-zitadel.sh`. + +### ~~Shell script JSON parsing~~ (done) +Replaced all `grep -o`/`cut` with `jq -r` in `setup-zitadel.sh`. Added jq preflight check.