diff --git a/.env.example b/.env.example index 83b3e72..a589415 100644 --- a/.env.example +++ b/.env.example @@ -8,7 +8,10 @@ DATABASE_URL=sqlite://data/happyview.db?mode=rwc # HappyView PUBLIC_URL=http://127.0.0.1:3000 -SESSION_SECRET=change-me-in-production +# REQUIRED: signs the dashboard/admin session cookie. Generate a random value of +# at least 32 bytes, e.g. `openssl rand -base64 48`. If unset or insecure, the +# server still starts but cookie-based login is disabled until you fix it. +SESSION_SECRET= RELAY_URL=https://relay1.us-east.bsky.network PORT=3000 diff --git a/src/auth/middleware.rs b/src/auth/middleware.rs index 93fa2b8..2a5d672 100644 --- a/src/auth/middleware.rs +++ b/src/auth/middleware.rs @@ -79,6 +79,14 @@ impl FromRequestParts for Claims { .map_err(|_| AppError::Auth("failed to read cookies".into()))?; if let Some(cookie) = jar.get(COOKIE_NAME) { + // Cookie auth relies on the SESSION_SECRET-derived signing key. If + // that secret is insecure the key is forgeable, so we refuse cookie + // auth outright with a clear error rather than trust it. + if !state.config.session_secret_secure() { + return Err(AppError::ServerMisconfigured( + crate::auth::COOKIE_AUTH_DISABLED_MSG.into(), + )); + } let value = cookie.value().to_string(); let (did, client_key) = if let Some((d, k)) = value.split_once(COOKIE_SEP) { (d.to_string(), Some(k.to_string())) @@ -319,7 +327,13 @@ impl FromRequestParts for XrpcClaims { .await .map_err(|_| AppError::Auth("failed to read cookies".into()))?; - if let Some(cookie) = jar.get(COOKIE_NAME) { + // Only trust the session cookie when the signing key is secure. + // When SESSION_SECRET is insecure we ignore the cookie and treat + // the request as anonymous, so public/DPoP reads keep working for + // clients that happen to carry a stale cookie. + if state.config.session_secret_secure() + && let Some(cookie) = jar.get(COOKIE_NAME) + { let value = cookie.value().to_string(); let (did, client_key) = if let Some((d, k)) = value.split_once(COOKIE_SEP) { (d.to_string(), Some(k.to_string())) diff --git a/src/auth/mod.rs b/src/auth/mod.rs index 2e9be32..15e6d20 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -12,3 +12,9 @@ pub use routes::parse_scope_string; pub use service_auth::ServiceAuth; pub const COOKIE_NAME: &str = "happyview_session"; + +/// Error message returned when cookie-based auth (dashboard login) is disabled +/// because `SESSION_SECRET` is not configured securely. Other auth mechanisms +/// (DPoP, service auth, API keys) are unaffected. +pub const COOKIE_AUTH_DISABLED_MSG: &str = "Cookie-based login is disabled because SESSION_SECRET is not configured securely. \ + Set SESSION_SECRET to a random value of at least 32 bytes and restart the server."; diff --git a/src/auth/routes.rs b/src/auth/routes.rs index 2ad8c01..5d2ad8c 100644 --- a/src/auth/routes.rs +++ b/src/auth/routes.rs @@ -64,6 +64,14 @@ async fn login( domain: Option>>, Query(query): Query, ) -> Result<(SignedCookieJar, Json), AppError> { + // Refuse to start a login flow we cannot finish securely: the session cookie + // set by the callback is signed with the SESSION_SECRET-derived key. + if !state.config.session_secret_secure() { + return Err(AppError::ServerMisconfigured( + crate::auth::COOKIE_AUTH_DISABLED_MSG.into(), + )); + } + tracing::debug!(handle = %query.handle, redirect_uri = ?query.redirect_uri, scope = ?query.scope, "login request"); // Use scopes from the query param if provided, otherwise fall back to the @@ -153,6 +161,14 @@ async fn callback( jar: SignedCookieJar, Query(query): Query, ) -> Result<(SignedCookieJar, Redirect), AppError> { + // The callback sets the session cookie; refuse when its signing key is not + // secure (mirrors the guard in `login`). + if !state.config.session_secret_secure() { + return Err(AppError::ServerMisconfigured( + crate::auth::COOKIE_AUTH_DISABLED_MSG.into(), + )); + } + tracing::debug!(state = ?query.state, "callback received"); // Look up the redirect URI and client_id from the database before the OAuth library consumes the state diff --git a/src/config.rs b/src/config.rs index cabd5d2..dfa2150 100644 --- a/src/config.rs +++ b/src/config.rs @@ -3,6 +3,47 @@ use std::net::SocketAddr; use crate::db::DatabaseBackend; +/// Placeholder session secrets that have shipped in this repo's docs/examples. +/// Booting with any of them is refused — the cookie signing key is derived from +/// `SESSION_SECRET`, so a known value lets anyone forge a validly-signed admin +/// session cookie. +const INSECURE_SESSION_SECRETS: &[&str] = &[ + "change-me-in-production-not-secure", + "change-me-in-production", +]; + +/// Minimum acceptable `SESSION_SECRET` length in bytes. `Key::derive_from` also +/// requires at least 32 bytes; enforcing it here yields a clear error instead of +/// a downstream panic. +const MIN_SESSION_SECRET_BYTES: usize = 32; + +/// Validate a session secret, rejecting known placeholder values and anything +/// too short to be secure. Returns a human-readable reason on failure. +fn validate_session_secret(secret: &str) -> Result<(), String> { + if secret.is_empty() { + return Err( + "SESSION_SECRET is not set. Generate a random value of at least 32 bytes \ + (e.g. `openssl rand -base64 48`) and set SESSION_SECRET." + .into(), + ); + } + if INSECURE_SESSION_SECRETS.contains(&secret) { + return Err( + "SESSION_SECRET is set to a known insecure default. Generate a random \ + value of at least 32 bytes (e.g. `openssl rand -base64 48`)." + .into(), + ); + } + if secret.len() < MIN_SESSION_SECRET_BYTES { + return Err(format!( + "SESSION_SECRET must be at least {MIN_SESSION_SECRET_BYTES} bytes (got {}). \ + Generate a random value (e.g. `openssl rand -base64 48`).", + secret.len() + )); + } + Ok(()) +} + #[derive(Clone, Debug)] pub struct Config { pub host: String, @@ -43,8 +84,10 @@ impl Config { database_url, database_backend, public_url: env::var("PUBLIC_URL").expect("PUBLIC_URL must be set"), - session_secret: env::var("SESSION_SECRET") - .unwrap_or_else(|_| "change-me-in-production-not-secure".into()), + // Not required and never defaulted to a placeholder: an unset, + // insecure, or too-short value is surfaced via `config_errors()` and + // disables cookie auth rather than aborting boot. See C3. + session_secret: env::var("SESSION_SECRET").unwrap_or_default(), jetstream_url: env::var("JETSTREAM_URL") .unwrap_or_else(|_| "wss://jetstream1.us-east.bsky.network".into()), relay_url: env::var("RELAY_URL").unwrap_or_else(|_| "https://bsky.network".into()), @@ -86,6 +129,25 @@ impl Config { } } + /// Whether the configured `SESSION_SECRET` is safe to derive the cookie + /// signing key from. When `false`, cookie-based auth is disabled (see the + /// auth extractors and login handlers) because the signing key would be + /// forgeable. + pub fn session_secret_secure(&self) -> bool { + validate_session_secret(&self.session_secret).is_ok() + } + + /// Human-readable configuration problems detected at startup, surfaced to + /// the dashboard (via `/config`) so an operator can fix them. Empty when the + /// instance is configured correctly. + pub fn config_errors(&self) -> Vec { + let mut errors = Vec::new(); + if let Err(e) = validate_session_secret(&self.session_secret) { + errors.push(e); + } + errors + } + pub fn listen_addr(&self) -> SocketAddr { format!("{}:{}", self.host, self.port) .parse() @@ -231,6 +293,63 @@ mod tests { Config::from_env(); } + #[test] + fn validate_session_secret_accepts_strong_secret() { + assert!(validate_session_secret("a-securely-generated-32plus-byte-secret!!").is_ok()); + // Exactly 32 bytes is accepted. + assert!(validate_session_secret(&"x".repeat(32)).is_ok()); + } + + #[test] + fn validate_session_secret_rejects_empty() { + let err = validate_session_secret("").unwrap_err(); + assert!(err.contains("not set"), "got: {err}"); + } + + #[test] + fn validate_session_secret_rejects_known_defaults() { + // The code's historical sentinel is 34 bytes, so length alone would not + // catch it — the explicit default list must. + assert!(validate_session_secret("change-me-in-production-not-secure").is_err()); + assert!(validate_session_secret("change-me-in-production").is_err()); + } + + #[test] + fn validate_session_secret_rejects_too_short() { + let err = validate_session_secret(&"x".repeat(31)).unwrap_err(); + assert!(err.contains("at least 32 bytes"), "got: {err}"); + } + + #[test] + #[serial] + fn from_env_does_not_panic_without_session_secret() { + unsafe { + clear_env(); + set_required_env(); + } + // Boot must succeed even with no SESSION_SECRET; the problem is surfaced + // via config_errors() and disables cookie auth instead of aborting. + let config = Config::from_env(); + assert!(!config.session_secret_secure()); + assert!(!config.config_errors().is_empty()); + } + + #[test] + #[serial] + fn from_env_with_strong_session_secret_is_secure() { + unsafe { + clear_env(); + set_required_env(); + env::set_var( + "SESSION_SECRET", + "a-securely-generated-32plus-byte-secret!!", + ); + } + let config = Config::from_env(); + assert!(config.session_secret_secure()); + assert!(config.config_errors().is_empty()); + } + #[test] #[serial] #[should_panic(expected = "PUBLIC_URL must be set")] diff --git a/src/error.rs b/src/error.rs index caf4b39..7f722ee 100644 --- a/src/error.rs +++ b/src/error.rs @@ -63,6 +63,10 @@ pub enum AppError { Internal(String), NotFound(String), PdsError(StatusCode, Bytes), + /// The instance is misconfigured (e.g. an insecure `SESSION_SECRET`); the + /// requested auth path is disabled until an operator fixes it. Renders as + /// 503 so clients and the dashboard can distinguish it from a normal 401. + ServerMisconfigured(String), RateLimited { retry_after: u64, limit: u32, @@ -90,6 +94,7 @@ impl std::fmt::Display for AppError { AppError::Internal(msg) => write!(f, "internal error: {msg}"), AppError::NotFound(msg) => write!(f, "not found: {msg}"), AppError::PdsError(status, _) => write!(f, "PDS error: {status}"), + AppError::ServerMisconfigured(msg) => write!(f, "server misconfigured: {msg}"), AppError::RateLimited { retry_after, .. } => { write!(f, "rate limited: retry after {retry_after}s") } @@ -163,6 +168,13 @@ impl IntoResponse for AppError { }); (StatusCode::FORBIDDEN, axum::Json(body)).into_response() } + AppError::ServerMisconfigured(msg) => { + let body = serde_json::json!({ + "error": "ServerMisconfigured", + "message": msg, + }); + (StatusCode::SERVICE_UNAVAILABLE, axum::Json(body)).into_response() + } AppError::RateLimited { retry_after, limit, @@ -197,6 +209,7 @@ impl IntoResponse for AppError { | AppError::AuthDpopNonce(..) | AppError::FeatureDisabled(..) | AppError::InsufficientPermissions(..) + | AppError::ServerMisconfigured(..) | AppError::RateLimited { .. } | AppError::ScriptError { .. } => unreachable!(), }; @@ -222,6 +235,17 @@ mod tests { (status, json) } + #[tokio::test] + async fn server_misconfigured_returns_503() { + let (status, body) = response_parts(AppError::ServerMisconfigured( + "SESSION_SECRET is not set".into(), + )) + .await; + assert_eq!(status, StatusCode::SERVICE_UNAVAILABLE); + assert_eq!(body["error"], "ServerMisconfigured"); + assert_eq!(body["message"], "SESSION_SECRET is not set"); + } + #[tokio::test] async fn auth_error_returns_401() { let (status, body) = response_parts(AppError::Auth("bad token".into())).await; diff --git a/src/main.rs b/src/main.rs index 6a1f814..c5baa94 100644 --- a/src/main.rs +++ b/src/main.rs @@ -484,23 +484,24 @@ async fn main() { .expect("Failed to create OAuth client") }; - if config.session_secret == "change-me-in-production-not-secure" { - if db_backend == happyview::db::DatabaseBackend::Postgres { - tracing::error!( - "INSECURE SESSION SECRET — You are using the default session secret with a \ - Postgres backend, which likely indicates a production deployment. \ - Set SESSION_SECRET to a random string of at least 64 characters." - ); - } else { - warn!( - "Using the default session secret. Set SESSION_SECRET to a random \ - string in production." - ); + // Derive the cookie signing key from SESSION_SECRET when it is secure. When + // it is not, log the problem loudly and fall back to an ephemeral random key + // so no attacker can forge cookies with a known/weak key. Cookie-based auth + // is disabled in this state (see the auth extractors and login handlers); + // DPoP, service auth, and API-key auth are unaffected. The server still boots + // so the dashboard can surface the misconfiguration to an operator. + let cookie_key = if config.session_secret_secure() { + axum_extra::extract::cookie::Key::derive_from(config.session_secret.as_bytes()) + } else { + for err in config.config_errors() { + tracing::error!("INSECURE CONFIGURATION: {err}"); } - } - - let cookie_key = - axum_extra::extract::cookie::Key::derive_from(config.session_secret.as_bytes()); + tracing::error!( + "Cookie-based login is DISABLED until SESSION_SECRET is set securely. \ + Other auth (DPoP, service auth, API keys) continues to work." + ); + axum_extra::extract::cookie::Key::generate() + }; let initial_collections = lexicons.get_record_collections().await; let (collections_tx, collections_rx) = watch::channel(initial_collections); diff --git a/src/server.rs b/src/server.rs index f6732ce..2bab835 100644 --- a/src/server.rs +++ b/src/server.rs @@ -348,6 +348,9 @@ async fn config_endpoint( "features": { "spaces": spaces_enabled, }, + // Startup configuration problems (e.g. an insecure SESSION_SECRET) so the + // dashboard can surface them to an operator. Empty when healthy. + "configErrors": state.config.config_errors(), })) } diff --git a/tests/common/app.rs b/tests/common/app.rs index 3ae5669..a5b9bfd 100644 --- a/tests/common/app.rs +++ b/tests/common/app.rs @@ -53,7 +53,7 @@ impl TestApp { database_url: String::new(), database_backend: backend, public_url: "http://127.0.0.1:0".into(), - session_secret: "test-secret".into(), + session_secret: "test-session-secret-0123456789abcdef".into(), jetstream_url: "wss://jetstream1.us-east.bsky.network".into(), relay_url: mock_url.clone(), plc_url: mock_url.clone(), @@ -218,6 +218,13 @@ impl TestApp { app } + /// Put the instance into the "insecure SESSION_SECRET" state, in which + /// cookie-based auth is disabled (C3). Other auth is unaffected. + pub fn set_insecure_session_secret(&mut self) { + self.state.config.session_secret = String::new(); + self.rebuild_router(); + } + /// Create an API client in the database for testing. /// Returns (client_key, client_secret, api_client_id). pub async fn create_api_client( diff --git a/tests/misconfigured.rs b/tests/misconfigured.rs new file mode 100644 index 0000000..f656227 --- /dev/null +++ b/tests/misconfigured.rs @@ -0,0 +1,139 @@ +mod common; + +use axum::body::Body; +use axum::http::{Request, StatusCode}; +use http_body_util::BodyExt; +use serde_json::json; +use serial_test::serial; +use tower::ServiceExt; + +async fn response_json(resp: axum::http::Response) -> serde_json::Value { + let body = resp.into_body().collect().await.unwrap().to_bytes(); + serde_json::from_slice(&body).unwrap_or(json!(null)) +} + +/// With an insecure SESSION_SECRET, starting a cookie login must fail loudly +/// (503 ServerMisconfigured) rather than mint a forgeable cookie session. +#[tokio::test] +#[serial] +async fn insecure_secret_login_returns_503() { + common::require_db!(); + let mut app = common::app::TestApp::new().await; + app.set_insecure_session_secret(); + + let req = Request::builder() + .method("GET") + .uri("/auth/login?handle=alice.test") + .header("host", "127.0.0.1") + .body(Body::empty()) + .unwrap(); + + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); + let body = response_json(resp).await; + assert_eq!(body["error"], "ServerMisconfigured"); +} + +/// A cookie-authenticated admin request must be rejected with a clear 503 when +/// the session secret is insecure — the cookie signature can't be trusted. +#[tokio::test] +#[serial] +async fn insecure_secret_admin_cookie_returns_503() { + common::require_db!(); + let mut app = common::app::TestApp::new().await; + app.set_insecure_session_secret(); + + let (name, value) = app.admin_cookie(); + let req = Request::builder() + .method("GET") + .uri("/admin/lexicons") + .header("host", "127.0.0.1") + .header(name, value) + .body(Body::empty()) + .unwrap(); + + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); + let body = response_json(resp).await; + assert_eq!(body["error"], "ServerMisconfigured"); +} + +/// Anonymous / public XRPC traffic must keep working when the session secret is +/// insecure — even for a client that happens to carry a stale session cookie. +/// The cookie is ignored (treated as anonymous), so the response is never the +/// misconfiguration 503. +#[tokio::test] +#[serial] +async fn insecure_secret_xrpc_ignores_cookie() { + common::require_db!(); + let mut app = common::app::TestApp::new().await; + + // Capture a validly-signed cookie *before* flipping to the insecure state. + let (name, value) = app.admin_cookie(); + app.set_insecure_session_secret(); + + let req = Request::builder() + .method("POST") + .uri("/xrpc/com.example.test.procedure") + .header("host", "127.0.0.1") + .header("content-type", "application/json") + .header(name, value) + .body(Body::from("{}")) + .unwrap(); + + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert_ne!( + resp.status(), + StatusCode::SERVICE_UNAVAILABLE, + "the misconfiguration gate must not block XRPC traffic; the cookie should be ignored" + ); +} + +/// `/config` surfaces the misconfiguration so the dashboard can explain it. +#[tokio::test] +#[serial] +async fn config_endpoint_reports_errors_when_insecure() { + common::require_db!(); + let mut app = common::app::TestApp::new().await; + app.set_insecure_session_secret(); + + let req = Request::builder() + .method("GET") + .uri("/config") + .header("host", "127.0.0.1") + .body(Body::empty()) + .unwrap(); + + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body = response_json(resp).await; + let errors = body["configErrors"].as_array().expect("configErrors array"); + assert!( + !errors.is_empty(), + "configErrors should list the insecure SESSION_SECRET" + ); +} + +/// A correctly configured instance reports no config errors and serves login. +#[tokio::test] +#[serial] +async fn config_endpoint_reports_no_errors_when_healthy() { + common::require_db!(); + let app = common::app::TestApp::new().await; + + let req = Request::builder() + .method("GET") + .uri("/config") + .header("host", "127.0.0.1") + .body(Body::empty()) + .unwrap(); + + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body = response_json(resp).await; + let errors = body["configErrors"].as_array().expect("configErrors array"); + assert!( + errors.is_empty(), + "a healthy instance should report no config errors" + ); +} diff --git a/web/src/lib/config-context.tsx b/web/src/lib/config-context.tsx index 875627e..21ef409 100644 --- a/web/src/lib/config-context.tsx +++ b/web/src/lib/config-context.tsx @@ -1,6 +1,7 @@ "use client" import { createContext, useContext, useEffect, useState } from "react" +import { TriangleAlert } from "lucide-react" interface ConfigContextType { public_url: string @@ -8,6 +9,7 @@ interface ConfigContextType { default_rate_limit_refill_rate: number app_name: string | null logo_url: string | null + configErrors: string[] } const ConfigContext = createContext({ @@ -16,8 +18,34 @@ const ConfigContext = createContext({ default_rate_limit_refill_rate: 2.0, app_name: null, logo_url: null, + configErrors: [], }) +function ConfigErrorBanner({ errors }: { errors: string[] }) { + return ( +
+
+
+
+ ) +} + export function ConfigProvider({ children }: { children: React.ReactNode }) { const [config, setConfig] = useState(null) const [error, setError] = useState(null) @@ -35,6 +63,7 @@ export function ConfigProvider({ children }: { children: React.ReactNode }) { default_rate_limit_refill_rate: data.default_rate_limit_refill_rate, app_name: data.app_name ?? null, logo_url: data.logo_url ?? null, + configErrors: Array.isArray(data.configErrors) ? data.configErrors : [], }) }) .catch((e) => setError(e.message)) @@ -47,7 +76,12 @@ export function ConfigProvider({ children }: { children: React.ReactNode }) { if (!config) return null return ( - {children} + + {config.configErrors.length > 0 && ( + + )} + {children} + ) }