Address security/tech debt: token refresh, JWKS thundering herd, config safety, jq migration
- Add token refresh logic in Auth.js JWT callback with 60s expiry buffer - Fix JWKS cache thundering herd with Mutex + double-checked locking - Make trustHost conditional (dev-only) via SvelteKit's $app/environment - Make devMode conditional on ZITADEL_PRODUCTION env var in setup script - Replace fragile grep/cut JSON parsing with jq in setup-zitadel.sh - Add OIDC_GRANT_TYPE_REFRESH_TOKEN to Zitadel OIDC app grant types - Update TODO_SECURITY.md: mark resolved items, add RefreshAccessTokenError frontend handling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
76489a53a6
commit
ed0578cd07
5 changed files with 131 additions and 50 deletions
2
apps/dashboard/src/app.d.ts
vendored
2
apps/dashboard/src/app.d.ts
vendored
|
|
@ -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';
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<import('@auth/core/jwt').JWT> {
|
||||
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
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<RwLock<CacheInner>>,
|
||||
refresh_lock: Arc<Mutex<()>>,
|
||||
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<JwkSet, JwksError> {
|
||||
// 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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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."
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue