diff --git a/src/oauth/pds_write.rs b/src/oauth/pds_write.rs index 9724893..a104271 100644 --- a/src/oauth/pds_write.rs +++ b/src/oauth/pds_write.rs @@ -693,6 +693,77 @@ fn generate_dpop_proof_inner( Ok(format!("{}.{}.{}", header_b64, payload_b64, sig_b64)) } +/// Verify that a DPoP access token belongs to `did` by calling +/// `com.atproto.server.getSession` on the DID's own PDS. +/// +/// This is the trust anchor for session registration: the caller supplies a +/// `did` and an `access_token`, but nothing proves the token was issued for +/// that DID. We resolve the PDS **authoritatively from the DID document** (never +/// from a client-supplied PDS URL, which an attacker could point at a server +/// that lies), then present the token with a DPoP proof signed by the +/// provisioned key the token is bound to. The PDS reports which DID the token +/// actually belongs to; the caller compares it against the claimed `did`. +/// +/// Returns the DID the PDS reports for the token, or an auth error if the token +/// is rejected. +pub async fn verify_access_token_did( + http: &reqwest::Client, + plc_url: &str, + private_jwk: &serde_json::Value, + did: &str, + access_token: &str, +) -> Result { + let pds_url = resolve_pds_from_did(http, plc_url, did).await?; + let target_url = format!( + "{}/xrpc/com.atproto.server.getSession", + pds_url.trim_end_matches('/') + ); + + // The PDS may demand a DPoP nonce on the first attempt; retry once with it. + let mut nonce: Option = None; + for _ in 0..2 { + let proof = generate_dpop_proof( + private_jwk, + "GET", + &target_url, + access_token, + nonce.as_deref(), + )?; + let resp = http + .get(&target_url) + .header("Authorization", format!("DPoP {access_token}")) + .header("DPoP", proof) + .send() + .await + .map_err(|e| AppError::Internal(format!("getSession request failed: {e}")))?; + + if resp.status().is_success() { + let body: serde_json::Value = resp + .json() + .await + .map_err(|e| AppError::Internal(format!("invalid getSession response: {e}")))?; + return body["did"] + .as_str() + .map(|s| s.to_string()) + .ok_or_else(|| AppError::Auth("getSession response missing did".into())); + } + + if nonce.is_none() + && let Some(n) = extract_dpop_nonce(&resp) + { + nonce = Some(n); + continue; + } + + return Err(AppError::Auth(format!( + "access token verification failed ({})", + resp.status() + ))); + } + + Err(AppError::Auth("access token verification failed".into())) +} + /// Resolve a user's PDS URL from their DID document. async fn resolve_pds_from_did( http: &reqwest::Client, diff --git a/src/oauth/routes.rs b/src/oauth/routes.rs index d4580f5..75198c1 100644 --- a/src/oauth/routes.rs +++ b/src/oauth/routes.rs @@ -207,7 +207,7 @@ async fn register_session( .ok_or_else(|| AppError::Internal("TOKEN_ENCRYPTION_KEY not configured".into()))?; // Look up the DPoP key by provision_id - let (dpop_key_id, dpop_client_id, _private_jwk, _thumbprint, pkce_challenge) = + let (dpop_key_id, dpop_client_id, private_jwk, _thumbprint, pkce_challenge) = keys::get_dpop_key( &state.db, state.db_backend, @@ -258,6 +258,31 @@ async fn register_session( return Err(e); } + // Verify the access token actually belongs to the claimed DID. The `did` in + // the request body is client-supplied and untrusted; without this check any + // holder of a provisioned DPoP key could register a session for an arbitrary + // victim DID and be authenticated as them on every DPoP-accepting route. + let verified_did = super::pds_write::verify_access_token_did( + &state.http, + &state.config.plc_url, + &private_jwk, + &body.did, + &body.access_token, + ) + .await?; + + if verified_did != body.did { + tracing::warn!( + client_key = %client_key, + claimed_did = %body.did, + verified_did = %verified_did, + "session registration rejected: access token does not belong to claimed DID" + ); + return Err(AppError::Auth( + "access token does not belong to the claimed DID".into(), + )); + } + // Store the session let session_id = Uuid::new_v4().to_string(); sessions::store_dpop_session( diff --git a/tests/common/app.rs b/tests/common/app.rs index ff9d3d2..3ae5669 100644 --- a/tests/common/app.rs +++ b/tests/common/app.rs @@ -580,6 +580,51 @@ impl TestApp { format!("Bearer {}.{}.{}", header_b64, payload_b64, sig_b64) } + /// Mount mock PLC + PDS responses so that registering a DPoP session for + /// `did` passes access-token verification. + /// + /// The mock PLC (== `plc_url`) serves a DID document for `did` whose + /// `#atproto_pds` service points back at the mock server, and + /// `com.atproto.server.getSession` on the mock server reports `resolved_did` + /// for the presented token. For a legitimate session, pass the same value + /// for both arguments; for a spoofing attempt, make them differ. + pub async fn mock_session_verification(&self, did: &str, resolved_did: &str) { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, ResponseTemplate}; + + // Give each DID its own PDS path so several sessions can be mocked on the + // single shared mock server without their getSession mocks colliding + // (the getSession request itself carries no DID to disambiguate on). + let pds_path = format!("/pds/{did}"); + let pds_url = format!("{}{}", self.mock_server.uri(), pds_path); + + let did_doc = serde_json::json!({ + "id": did, + "service": [{ + "id": "#atproto_pds", + "type": "AtprotoPersonalDataServer", + "serviceEndpoint": pds_url, + }] + }); + + Mock::given(method("GET")) + .and(path(format!("/{did}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(did_doc)) + .mount(&self.mock_server) + .await; + + Mock::given(method("GET")) + .and(path(format!( + "{pds_path}/xrpc/com.atproto.server.getSession" + ))) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(serde_json::json!({ "did": resolved_did })), + ) + .mount(&self.mock_server) + .await; + } + /// Install a fake plugin directly into the registry at the given version. pub async fn install_fake_plugin(&self, id: &str, version: &str) { use happyview::plugin::{LoadedPlugin, PluginInfo, PluginSource}; diff --git a/tests/dev_happyview.rs b/tests/dev_happyview.rs index 3d25216..b5797fe 100644 --- a/tests/dev_happyview.rs +++ b/tests/dev_happyview.rs @@ -56,6 +56,7 @@ async fn setup_dpop_session(app: &common::app::TestApp, user_did: &str) -> (Stri // 2. Register session let access_token = format!("test-access-{}", uuid::Uuid::new_v4()); + app.mock_session_verification(user_did, user_did).await; let session_req = post_json_with_headers( "/oauth/sessions", &json!({ diff --git a/tests/dpop_auth.rs b/tests/dpop_auth.rs index 98aa721..ac0bf65 100644 --- a/tests/dpop_auth.rs +++ b/tests/dpop_auth.rs @@ -215,6 +215,118 @@ async fn test_register_session_requires_atproto_scope() { assert_eq!(resp.status(), StatusCode::BAD_REQUEST); } +#[tokio::test] +#[serial] +async fn test_register_session_rejects_did_not_matching_token() { + common::require_db!(); + let app = common::app::TestApp::new_with_encryption().await; + let (client_key, client_secret, _id) = app.create_api_client("confidential", None).await; + + // The attacker claims to be the victim, but the access token they present + // belongs to the attacker's own DID. getSession on the victim's PDS reports + // the attacker DID, so registration must be rejected. + app.mock_session_verification("did:plc:victim", "did:plc:attacker") + .await; + + let key_req = post_json_with_headers( + "/oauth/dpop-keys", + &json!({}), + vec![ + ("x-client-key", &client_key), + ("x-client-secret", &client_secret), + ], + ); + let key_resp = app.router.clone().oneshot(key_req).await.unwrap(); + let key_body = response_json(key_resp).await; + let provision_id = key_body["provision_id"].as_str().unwrap(); + + let req = post_json_with_headers( + "/oauth/sessions", + &json!({ + "provision_id": provision_id, + "did": "did:plc:victim", + "access_token": "attacker-token", + "scopes": "atproto", + }), + vec![ + ("x-client-key", &client_key), + ("x-client-secret", &client_secret), + ], + ); + + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert_eq!( + resp.status(), + StatusCode::UNAUTHORIZED, + "registering a session for a DID the token does not belong to must be rejected" + ); +} + +#[tokio::test] +#[serial] +async fn test_register_session_rejects_token_pds_refuses() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, ResponseTemplate}; + + common::require_db!(); + let app = common::app::TestApp::new_with_encryption().await; + let (client_key, client_secret, _id) = app.create_api_client("confidential", None).await; + + // The victim's DID document resolves to their real PDS, which rejects the + // attacker's token outright (401). Registration must fail. + let pds_url = app.mock_server.uri(); + Mock::given(method("GET")) + .and(path("/did:plc:victim2")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "id": "did:plc:victim2", + "service": [{ + "id": "#atproto_pds", + "type": "AtprotoPersonalDataServer", + "serviceEndpoint": pds_url, + }] + }))) + .mount(&app.mock_server) + .await; + Mock::given(method("GET")) + .and(path("/xrpc/com.atproto.server.getSession")) + .respond_with(ResponseTemplate::new(401).set_body_json(json!({ "error": "InvalidToken" }))) + .mount(&app.mock_server) + .await; + + let key_req = post_json_with_headers( + "/oauth/dpop-keys", + &json!({}), + vec![ + ("x-client-key", &client_key), + ("x-client-secret", &client_secret), + ], + ); + let key_resp = app.router.clone().oneshot(key_req).await.unwrap(); + let key_body = response_json(key_resp).await; + let provision_id = key_body["provision_id"].as_str().unwrap(); + + let req = post_json_with_headers( + "/oauth/sessions", + &json!({ + "provision_id": provision_id, + "did": "did:plc:victim2", + "access_token": "attacker-token", + "scopes": "atproto", + }), + vec![ + ("x-client-key", &client_key), + ("x-client-secret", &client_secret), + ], + ); + + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert_eq!( + resp.status(), + StatusCode::UNAUTHORIZED, + "registration must fail when the PDS refuses the presented access token" + ); +} + #[tokio::test] #[serial] async fn test_full_flow_provision_register_delete() { @@ -237,6 +349,8 @@ async fn test_full_flow_provision_register_delete() { let provision_id = key_body["provision_id"].as_str().unwrap(); // 2. Register session + app.mock_session_verification("did:plc:testuser", "did:plc:testuser") + .await; let session_req = post_json_with_headers( "/oauth/sessions", &json!({ @@ -381,6 +495,8 @@ async fn test_xrpc_dpop_auth_accepted() { // 2. Register session let access_token = "test-xrpc-access-token"; + app.mock_session_verification("did:plc:xrpcuser", "did:plc:xrpcuser") + .await; let session_req = post_json_with_headers( "/oauth/sessions", &json!({ @@ -446,6 +562,8 @@ async fn provision_and_register( let provision_id = key_body["provision_id"].as_str().unwrap().to_string(); let dpop_key = key_body["dpop_key"].clone(); + app.mock_session_verification(did, did).await; + let session_req = post_json_with_headers( "/oauth/sessions", &json!({ @@ -601,6 +719,8 @@ async fn test_session_upsert_same_device() { let key_body = response_json(key_resp).await; let provision_id = key_body["provision_id"].as_str().unwrap(); + app.mock_session_verification(did, did).await; + // Register session with token-v1 let reg1 = post_json_with_headers( "/oauth/sessions", @@ -738,6 +858,8 @@ async fn test_public_client_dpop_get_session() { let did = "did:plc:publicuser"; let access_token = "public-client-access-token"; + app.mock_session_verification(did, did).await; + // Register session with PKCE verifier let session_req = post_json_with_headers( "/oauth/sessions", @@ -806,6 +928,8 @@ async fn test_public_client_dpop_delete_session() { let did = "did:plc:publicdelete"; let access_token = "public-delete-token"; + app.mock_session_verification(did, did).await; + let session_req = post_json_with_headers( "/oauth/sessions", &json!({ diff --git a/tests/e2e_delegation.rs b/tests/e2e_delegation.rs index 2326e77..98424c1 100644 --- a/tests/e2e_delegation.rs +++ b/tests/e2e_delegation.rs @@ -64,6 +64,7 @@ async fn setup_dpop_session(app: &common::app::TestApp, user_did: &str) -> (Stri let dpop_key = key_body["dpop_key"].clone(); let access_token = format!("test-access-{}", uuid::Uuid::new_v4()); + app.mock_session_verification(user_did, user_did).await; let session_req = post_json_with_headers( "/oauth/sessions", &json!({ @@ -163,6 +164,7 @@ async fn register_target_session( let provision_id = key_body["provision_id"].as_str().unwrap().to_string(); let access_token = format!("test-target-access-{}", uuid::Uuid::new_v4()); + app.mock_session_verification(target_did, target_did).await; let session_req = post_json_with_headers( "/oauth/sessions", &json!({ @@ -211,6 +213,7 @@ async fn setup_linked_account( let dpop_key = key_body["dpop_key"].clone(); let access_token = format!("test-owner-access-{}", uuid::Uuid::new_v4()); + app.mock_session_verification(owner_did, owner_did).await; let session_req = post_json_with_headers( "/oauth/sessions", &json!({ @@ -269,6 +272,7 @@ async fn setup_session_for_client( let dpop_key = key_body["dpop_key"].clone(); let access_token = format!("test-access-{}", uuid::Uuid::new_v4()); + app.mock_session_verification(user_did, user_did).await; let session_req = post_json_with_headers( "/oauth/sessions", &json!({ -- 2.51.2 From 0ad78af842b96593da47a6c81ab1d7f3810fee29 Mon Sep 17 00:00:00 2001 From: Trezy Date: Tue, 7 Jul 2026 11:14:34 -0500 Subject: [PATCH 2/9] fix: prevent CORS from reflecting credentials to arbitrary origins Signed-off-by: Trezy --- src/domain.rs | 39 ++++++++++++ src/server.rs | 125 ++++++++++++++++++++++++++++++++----- tests/common/db.rs | 12 +++- tests/cors.rs | 151 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 309 insertions(+), 18 deletions(-) create mode 100644 tests/cors.rs diff --git a/src/domain.rs b/src/domain.rs index 4bfe539..bd9a1e0 100644 --- a/src/domain.rs +++ b/src/domain.rs @@ -99,6 +99,23 @@ impl DomainCache { let by_host = self.by_host.read().await; by_host.values().cloned().collect() } + + /// Return `true` if `origin` (a browser `Origin` header value, e.g. + /// `https://example.com` or `http://localhost:3000`) exactly matches a + /// registered domain's URL. + /// + /// This is the trusted-origin allowlist for credentialed (cookie-bearing) + /// CORS: only first-party domains HappyView actually serves may send + /// credentials cross-origin. The match is on the full origin + /// (scheme + host + port), not just the host, so `http` and `https` or a + /// different port are treated as distinct origins. + pub async fn is_allowed_origin(&self, origin: &str) -> bool { + let target = origin.trim_end_matches('/'); + let by_host = self.by_host.read().await; + by_host + .values() + .any(|d| d.url.trim_end_matches('/') == target) + } } impl Default for DomainCache { @@ -179,6 +196,28 @@ mod tests { assert!(cache.get("example.com").await.is_none()); } + #[tokio::test] + async fn is_allowed_origin_matches_full_origin() { + let cache = DomainCache::new(); + cache + .load(vec![ + make_domain("https://example.com", true), + make_domain("http://localhost:3000", false), + ]) + .await; + + // Exact matches (trailing slash tolerated). + assert!(cache.is_allowed_origin("https://example.com").await); + assert!(cache.is_allowed_origin("https://example.com/").await); + assert!(cache.is_allowed_origin("http://localhost:3000").await); + + // Scheme, port, and host must all match. + assert!(!cache.is_allowed_origin("http://example.com").await); + assert!(!cache.is_allowed_origin("https://example.com:8443").await); + assert!(!cache.is_allowed_origin("http://localhost:3001").await); + assert!(!cache.is_allowed_origin("https://evil.example").await); + } + #[tokio::test] async fn set_primary_updates() { let cache = DomainCache::new(); diff --git a/src/server.rs b/src/server.rs index 71d655f..f6732ce 100644 --- a/src/server.rs +++ b/src/server.rs @@ -7,7 +7,6 @@ use base64::Engine; use bytes::Bytes; use http_body_util::Full; use std::convert::Infallible; -use tower_http::cors::CorsLayer; use tower_http::services::ServeDir; use tower_http::trace::TraceLayer; @@ -179,25 +178,117 @@ pub fn router(state: AppState) -> Router { outer .layer(TraceLayer::new_for_http()) - .layer( - CorsLayer::new() - .allow_origin(tower_http::cors::AllowOrigin::mirror_request()) - .allow_methods([Method::GET, Method::POST, Method::DELETE, Method::OPTIONS]) - .allow_headers([ - header::CONTENT_TYPE, - header::AUTHORIZATION, - header::COOKIE, - axum::http::HeaderName::from_static("x-client-key"), - axum::http::HeaderName::from_static("x-client-secret"), - axum::http::HeaderName::from_static("dpop"), - axum::http::HeaderName::from_static("atproto-accept-labelers"), - axum::http::HeaderName::from_static("atproto-proxy"), - ]) - .allow_credentials(true), - ) + .layer(axum::middleware::from_fn_with_state(state.clone(), cors)) .with_state(state) } +/// Allowed request methods, shared by both CORS policies. +const CORS_ALLOW_METHODS: &str = "GET, POST, DELETE, OPTIONS"; + +/// Headers a credentialed (first-party, cookie-bearing) request may send. +const CORS_ALLOW_HEADERS_CREDENTIALED: &str = "content-type, authorization, cookie, x-client-key, x-client-secret, dpop, \ + atproto-accept-labelers, atproto-proxy"; + +/// Headers a credential-less (third-party, cookieless DPoP) request may send. +/// Identical to the credentialed set minus `cookie`. +const CORS_ALLOW_HEADERS_ANON: &str = "content-type, authorization, x-client-key, x-client-secret, dpop, \ + atproto-accept-labelers, atproto-proxy"; + +/// Cross-Origin Resource Sharing policy. +/// +/// This deliberately replaces a single permissive `CorsLayer`. The old policy +/// reflected *any* `Origin` **and** allowed credentials, which let a malicious +/// page drive the admin API with the victim's cookie and read the response +/// (finding C2). Instead we apply two policies keyed on trust: +/// +/// - **Trusted first-party origins** — those in the [`DomainCache`] (the domains +/// HappyView actually serves the dashboard/admin UI on) — get their origin +/// reflected *with* `Access-Control-Allow-Credentials: true`, so the +/// cookie-authenticated dashboard works cross-origin if ever hosted on a +/// second registered domain. +/// - **Any other origin** — e.g. a third-party app or an attacker page — gets a +/// credential-*less* grant: its origin is reflected but credentials are never +/// allowed. Third-party clients authenticate with explicit DPoP + client-key +/// headers (never ambient cookies), so they keep working; an attacker page +/// can neither ride the admin cookie nor read a credentialed response. +/// +/// The one rule that must never be violated: reflecting an arbitrary origin and +/// allowing credentials at the same time. +async fn cors( + State(state): State, + req: axum::extract::Request, + next: axum::middleware::Next, +) -> Response { + let origin = req + .headers() + .get(header::ORIGIN) + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + + // No `Origin` header → not a CORS request (same-origin navigation, + // server-to-server, curl). Emit no CORS headers at all. + let Some(origin) = origin else { + return next.run(req).await; + }; + + let credentialed = state.domain_cache.is_allowed_origin(&origin).await; + + let is_preflight = req.method() == Method::OPTIONS + && req + .headers() + .contains_key(header::ACCESS_CONTROL_REQUEST_METHOD); + + let mut cors_headers = header::HeaderMap::new(); + if let Ok(value) = header::HeaderValue::from_str(&origin) { + cors_headers.insert(header::ACCESS_CONTROL_ALLOW_ORIGIN, value); + } else { + // Malformed origin — refuse it entirely rather than emit a broken header. + if is_preflight { + return preflight_response(cors_headers); + } + return next.run(req).await; + } + cors_headers.insert(header::VARY, header::HeaderValue::from_static("origin")); + if credentialed { + cors_headers.insert( + header::ACCESS_CONTROL_ALLOW_CREDENTIALS, + header::HeaderValue::from_static("true"), + ); + } + + if is_preflight { + cors_headers.insert( + header::ACCESS_CONTROL_ALLOW_METHODS, + header::HeaderValue::from_static(CORS_ALLOW_METHODS), + ); + cors_headers.insert( + header::ACCESS_CONTROL_ALLOW_HEADERS, + header::HeaderValue::from_static(if credentialed { + CORS_ALLOW_HEADERS_CREDENTIALED + } else { + CORS_ALLOW_HEADERS_ANON + }), + ); + cors_headers.insert( + header::ACCESS_CONTROL_MAX_AGE, + header::HeaderValue::from_static("86400"), + ); + return preflight_response(cors_headers); + } + + let mut resp = next.run(req).await; + resp.headers_mut().extend(cors_headers); + resp +} + +/// Build a `204 No Content` preflight response carrying the given CORS headers. +fn preflight_response(cors_headers: header::HeaderMap) -> Response { + let mut resp = Response::new(axum::body::Body::empty()); + *resp.status_mut() = axum::http::StatusCode::NO_CONTENT; + resp.headers_mut().extend(cors_headers); + resp +} + async fn health() -> &'static str { "ok" } diff --git a/tests/common/db.rs b/tests/common/db.rs index 80f4968..1f5dcd0 100644 --- a/tests/common/db.rs +++ b/tests/common/db.rs @@ -48,7 +48,7 @@ pub async fn truncate_all(pool: &AnyPool) { match backend { DatabaseBackend::Postgres => { sqlx::query( - "TRUNCATE happyview_records, happyview_lexicons, happyview_backfill_jobs, happyview_users, happyview_user_permissions, happyview_api_keys, happyview_event_logs, happyview_script_variables, happyview_scripts, happyview_dead_letter_scripts, happyview_dead_letter_hooks, happyview_record_refs, happyview_labeler_subscriptions, happyview_labels, happyview_instance_settings, happyview_domains, happyview_dpop_sessions, happyview_dpop_keys, happyview_api_clients, happyview_delegated_accounts, happyview_account_delegates, happyview_service_identity, happyview_service_entries, happyview_service_entry_xrpcs, happyview_jobs RESTART IDENTITY CASCADE", + "TRUNCATE happyview_records, happyview_lexicons, happyview_backfill_jobs, happyview_users, happyview_user_permissions, happyview_api_keys, happyview_event_logs, happyview_script_variables, happyview_scripts, happyview_dead_letter_scripts, happyview_dead_letter_hooks, happyview_record_refs, happyview_labeler_subscriptions, happyview_labels, happyview_instance_settings, happyview_domains, happyview_dpop_sessions, happyview_dpop_keys, happyview_api_clients, happyview_delegated_accounts, happyview_account_delegates, happyview_service_identity, happyview_service_entries, happyview_service_entry_xrpcs, happyview_jobs, happyview_spaces, happyview_space_members, happyview_space_records, happyview_space_repo_state, happyview_space_record_oplog, happyview_space_notify_registrations, happyview_space_invites RESTART IDENTITY CASCADE", ) .execute(pool) .await @@ -56,6 +56,16 @@ pub async fn truncate_all(pool: &AnyPool) { } DatabaseBackend::Sqlite => { let tables = [ + // Spaces tables (children before parents — no cascade on SQLite). + "happyview_space_credentials", + "happyview_space_dids", + "happyview_space_invites", + "happyview_space_notify_registrations", + "happyview_space_record_oplog", + "happyview_space_repo_state", + "happyview_space_records", + "happyview_space_members", + "happyview_spaces", "happyview_service_entry_xrpcs", "happyview_service_entries", "happyview_service_identity", diff --git a/tests/cors.rs b/tests/cors.rs new file mode 100644 index 0000000..3da9772 --- /dev/null +++ b/tests/cors.rs @@ -0,0 +1,151 @@ +mod common; + +use axum::body::Body; +use axum::http::Request; +use serial_test::serial; +use tower::ServiceExt; + +/// The origin the TestApp registers in its DomainCache (a trusted first-party +/// domain — where the dashboard / admin UI is served). +const TRUSTED_ORIGIN: &str = "http://127.0.0.1:0"; +/// An arbitrary untrusted origin (e.g. a third-party app or a malicious page). +const UNTRUSTED_ORIGIN: &str = "https://evil.example"; + +fn preflight(origin: &str, request_method: &str) -> Request { + Request::builder() + .method("OPTIONS") + .uri("/xrpc/com.example.test") + .header("host", "127.0.0.1") + .header("origin", origin) + .header("access-control-request-method", request_method) + .header( + "access-control-request-headers", + "content-type, authorization", + ) + .body(Body::empty()) + .unwrap() +} + +fn get_with_origin(uri: &str, origin: &str) -> Request { + Request::builder() + .method("GET") + .uri(uri) + .header("host", "127.0.0.1") + .header("origin", origin) + .body(Body::empty()) + .unwrap() +} + +fn header<'a>(resp: &'a axum::http::Response, name: &str) -> Option<&'a str> { + resp.headers().get(name).and_then(|v| v.to_str().ok()) +} + +#[tokio::test] +#[serial] +async fn preflight_from_trusted_origin_allows_credentials() { + common::require_db!(); + let app = common::app::TestApp::new().await; + + let resp = app + .router + .clone() + .oneshot(preflight(TRUSTED_ORIGIN, "POST")) + .await + .unwrap(); + + assert!(resp.status().is_success()); + assert_eq!( + header(&resp, "access-control-allow-origin"), + Some(TRUSTED_ORIGIN) + ); + assert_eq!( + header(&resp, "access-control-allow-credentials"), + Some("true"), + "trusted first-party origins must be allowed to send credentials" + ); +} + +#[tokio::test] +#[serial] +async fn preflight_from_untrusted_origin_never_allows_credentials() { + common::require_db!(); + let app = common::app::TestApp::new().await; + + let resp = app + .router + .clone() + .oneshot(preflight(UNTRUSTED_ORIGIN, "POST")) + .await + .unwrap(); + + // The untrusted origin may still use credential-less (cookieless) CORS — + // e.g. a third-party DPoP client — but it must NEVER be granted credentials. + assert_eq!( + header(&resp, "access-control-allow-credentials"), + None, + "untrusted origins must never be allowed to send credentials" + ); +} + +#[tokio::test] +#[serial] +async fn actual_request_from_trusted_origin_allows_credentials() { + common::require_db!(); + let app = common::app::TestApp::new().await; + + let resp = app + .router + .clone() + .oneshot(get_with_origin("/health", TRUSTED_ORIGIN)) + .await + .unwrap(); + + assert_eq!( + header(&resp, "access-control-allow-origin"), + Some(TRUSTED_ORIGIN) + ); + assert_eq!( + header(&resp, "access-control-allow-credentials"), + Some("true") + ); +} + +#[tokio::test] +#[serial] +async fn actual_request_from_untrusted_origin_never_allows_credentials() { + common::require_db!(); + let app = common::app::TestApp::new().await; + + let resp = app + .router + .clone() + .oneshot(get_with_origin("/health", UNTRUSTED_ORIGIN)) + .await + .unwrap(); + + assert_eq!( + header(&resp, "access-control-allow-credentials"), + None, + "untrusted origins must never be allowed to send credentials" + ); +} + +#[tokio::test] +#[serial] +async fn request_without_origin_gets_no_cors_headers() { + common::require_db!(); + let app = common::app::TestApp::new().await; + + let req = Request::builder() + .method("GET") + .uri("/health") + .header("host", "127.0.0.1") + .body(Body::empty()) + .unwrap(); + + let resp = app.router.clone().oneshot(req).await.unwrap(); + + assert!(resp.status().is_success()); + assert_eq!(header(&resp, "access-control-allow-origin"), None); + assert_eq!(header(&resp, "access-control-allow-credentials"), None); +} -- 2.51.2 From 0d288df7a75377df7742a418fb3423a397c0b853 Mon Sep 17 00:00:00 2001 From: Trezy Date: Tue, 7 Jul 2026 11:49:49 -0500 Subject: [PATCH 3/9] fix: prevent use of default session secret Signed-off-by: Trezy --- .env.example | 5 +- src/auth/middleware.rs | 16 +++- src/auth/mod.rs | 6 ++ src/auth/routes.rs | 16 ++++ src/config.rs | 123 ++++++++++++++++++++++++++++- src/error.rs | 24 ++++++ src/main.rs | 33 ++++---- src/server.rs | 3 + tests/common/app.rs | 9 ++- tests/misconfigured.rs | 139 +++++++++++++++++++++++++++++++++ web/src/lib/config-context.tsx | 36 ++++++++- 11 files changed, 388 insertions(+), 22 deletions(-) create mode 100644 tests/misconfigured.rs 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} + ) } -- 2.51.2 From 815843f681736aeee628532e93fe31df8597f073 Mon Sep 17 00:00:00 2001 From: Trezy Date: Tue, 7 Jul 2026 12:32:43 -0500 Subject: [PATCH 4/9] fix: prevent db.raw access to sensitive tables Signed-off-by: Trezy --- Cargo.lock | 44 ++++ Cargo.toml | 1 + .../docs/api-reference/lua/database-api.md | 27 ++- src/lua/db_api.rs | 192 +++++++++++++++++- tests/e2e_scripts.rs | 16 +- tests/lua_db_api.rs | 35 +++- web/src/lib/lua-hover.ts | 2 +- 7 files changed, 294 insertions(+), 23 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e244f8b..c1db34b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1740,6 +1740,7 @@ dependencies = [ "serde_json", "serial_test", "sha2", + "sqlparser", "sqlx", "thiserror 2.0.18", "tokio", @@ -3173,6 +3174,26 @@ dependencies = [ "yasna", ] +[[package]] +name = "recursive" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0786a43debb760f491b1bc0269fe5e84155353c67482b9e60d0cfb596054b43e" +dependencies = [ + "recursive-proc-macro-impl", + "stacker", +] + +[[package]] +name = "recursive-proc-macro-impl" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76009fbe0614077fc1a2ce255e3a1881a2e3a3527097d5dc6d8212c585e7e38b" +dependencies = [ + "quote", + "syn", +] + [[package]] name = "redox_syscall" version = "0.5.18" @@ -3799,6 +3820,16 @@ version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3b9b39299b249ad65f3b7e96443bad61c02ca5cd3589f46cb6d610a0fd6c0d6a" +[[package]] +name = "sqlparser" +version = "0.62.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "13c6d1b651dc4edf07eead2a0c6c78016ce971bc2c10da5266861b13f25e7cec" +dependencies = [ + "log", + "recursive", +] + [[package]] name = "sqlx" version = "0.8.6" @@ -3999,6 +4030,19 @@ version = "1.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" +[[package]] +name = "stacker" +version = "0.1.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "640c8cdd92b6b12f5bcb1803ca3bbf5ab96e5e6b6b96b9ab77dabe9e880b3190" +dependencies = [ + "cc", + "cfg-if", + "libc", + "psm", + "windows-sys 0.61.2", +] + [[package]] name = "stringprep" version = "0.1.5" diff --git a/Cargo.toml b/Cargo.toml index 9bb091e..c4f3023 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -61,6 +61,7 @@ async-stream = "0.3.6" blake3 = "1" hkdf = "0.12" hmac = "0.12" +sqlparser = "0.62.0" [[bin]] name = "migrate-lua-sql" diff --git a/packages/docs/content/docs/api-reference/lua/database-api.md b/packages/docs/content/docs/api-reference/lua/database-api.md index 83ec672..cc6a9ce 100644 --- a/packages/docs/content/docs/api-reference/lua/database-api.md +++ b/packages/docs/content/docs/api-reference/lua/database-api.md @@ -148,10 +148,10 @@ local n = db.count("xyz.statusphere.status", "did:plc:abc") -- filter by DID ## db.raw -Run a raw SQL query against the database. Supports `SELECT`, `INSERT`, `UPDATE`, `DELETE`, and `CREATE TABLE` statements. +Run a raw SQL query against the database. Supports `SELECT`, `INSERT`, `UPDATE`, `DELETE`, and `CREATE TABLE` statements — use it for your own tables and to reach the record index directly. ```lua --- Read query +-- Read the record index local rows = db.raw( "SELECT uri, did, record FROM happyview_records WHERE collection = $1 AND did = $2 LIMIT $3", { "xyz.statusphere.status", "did:plc:abc", 10 } @@ -161,7 +161,7 @@ for _, row in ipairs(rows) do -- row.uri, row.did, row.record (JSONB is returned as a Lua table) end --- Write query (returns affected rows, if any) +-- Create and use your own tables db.raw("CREATE TABLE IF NOT EXISTS my_table (id TEXT PRIMARY KEY, value TEXT NOT NULL)") db.raw("INSERT INTO my_table (id, value) VALUES ($1, $2) ON CONFLICT (id) DO UPDATE SET value = $2", { "key1", "hello" }) @@ -169,6 +169,23 @@ db.raw("INSERT INTO my_table (id, value) VALUES ($1, $2) ON CONFLICT (id) DO UPD Parameters are passed as an array and bound to `$1`, `$2`, etc. Supported parameter types: strings, integers, numbers, booleans, and nil. +### Protected tables + +`db.raw` blocks HappyView's **sensitive internal tables** — a statement that references one is rejected before it runs. Blocked tables cover instance secrets and tokens (OAuth/DPoP keys and sessions, API keys/clients, `happyview_script_variables`), auth and privilege state (users, permissions, delegation), trust config (domains, instance settings), and cryptographic material (space credentials and repo state). Internal tables are blocked **by default**, so anything not on the allowlist below is protected. + +Available internal tables: + +| Table | Contents | +| --- | --- | +| `happyview_records` | indexed AT Protocol records | +| `happyview_record_refs` | backlink index | +| `happyview_labels` | applied labels | +| `happyview_lexicons` | uploaded lexicons | +| `happyview_jobs` | background job queue | +| `happyview_spaces`, `happyview_space_members`, `happyview_space_records`, `happyview_space_record_oplog`, `happyview_space_notify_registrations`, `happyview_space_dids` | space membership and data | + +Space data is available because a space defines *access*, not confidentiality; if you need record data without exposing internals, the structured accessors [`db.query`](#dbquery), [`db.get`](#dbget), and [`db.count`](#dbcount) are the backend-portable option. + ### SQL dialect Unlike the structured API methods (`db.query`, `db.get`, etc.), `db.raw` does **not** translate SQL between backends. Write native SQL for the database you're running against — `$1`/`$2` placeholders for Postgres, `?` for SQLite. Use `db.backend()` to branch when you need to support both. @@ -196,9 +213,9 @@ Returns `"sqlite"` or `"postgres"`. Useful when you need database-specific SQL t ```lua if db.backend() == "postgres" then - db.raw("SELECT * FROM happyview_records WHERE record @> $1::jsonb", { json.encode({ status = "active" }) }) + db.raw("SELECT * FROM my_events WHERE payload @> $1::jsonb", { json.encode({ status = "active" }) }) else -- SQLite fallback - db.raw("SELECT * FROM happyview_records WHERE json_extract(record, '$.status') = $1", { "active" }) + db.raw("SELECT * FROM my_events WHERE json_extract(payload, '$.status') = $1", { "active" }) end ``` diff --git a/src/lua/db_api.rs b/src/lua/db_api.rs index 6c4872f..2c44261 100644 --- a/src/lua/db_api.rs +++ b/src/lua/db_api.rs @@ -10,6 +10,88 @@ use crate::db::{DatabaseBackend, adapt_sql, decode_cursor, encode_cursor}; const MAX_FILTER_DEPTH: u8 = 5; const ALLOWED_OPS: &[&str] = &["=", "!=", "<", ">", "<=", ">=", "LIKE", "NOT LIKE"]; +/// Table-name prefix reserved for HappyView's own internal tables. `db.raw` +/// blocks these **by default** — so a table added in a future migration is +/// protected until it is deliberately allowed — except for the data tables in +/// [`ALLOWED_INTERNAL_TABLES`]. +const PROTECTED_TABLE_PREFIX: &str = "happyview_"; + +/// Internal tables that don't carry the `happyview_` prefix but are still +/// off-limits (SQLx's migration bookkeeping). +const PROTECTED_EXACT_TABLES: &[&str] = &["_sqlx_migrations"]; + +/// Internal tables `db.raw` is allowed to read and write despite the reserved +/// prefix: public AppView data and space *data*. Everything else `happyview_*` +/// stays blocked — secrets and tokens (`happyview_dpop_keys`/`_sessions`, +/// `happyview_api_keys`, `happyview_oauth_*`, `happyview_script_variables`), +/// auth/privilege state (`happyview_users`/`_user_permissions`, the delegation +/// tables), trust config (`happyview_domains`, `happyview_instance_settings`), +/// and cryptographic material (`happyview_space_credentials`, and +/// `happyview_space_repo_state` which holds commit-signature key material). +/// +/// Space membership/records are exposed because a space defines *access*, not +/// confidentiality — whether to expose otherwise-private space data through the +/// AppView is left to the admin. +const ALLOWED_INTERNAL_TABLES: &[&str] = &[ + // Public AppView data. + "happyview_records", + "happyview_record_refs", + "happyview_labels", + "happyview_lexicons", + // Background jobs. + "happyview_jobs", + // Space data (not the credential/key-material tables). + "happyview_spaces", + "happyview_space_members", + "happyview_space_records", + "happyview_space_record_oplog", + "happyview_space_notify_registrations", + "happyview_space_dids", +]; + +/// Reject a `db.raw` SQL string that references a protected internal table. +/// +/// Tokenizes the SQL (so string literals and comments containing the prefix are +/// ignored, and quoted / schema-qualified identifiers are still caught) and +/// blocks any `happyview_*` (or `_sqlx_migrations`) identifier that is not in +/// [`ALLOWED_INTERNAL_TABLES`]. Unicode-escaped identifiers (`U&"…"`) are refused +/// outright as an evasion vector, and SQL that cannot be tokenized fails closed. +fn check_raw_sql_tables(sql: &str) -> Result<(), String> { + use sqlparser::dialect::GenericDialect; + use sqlparser::tokenizer::{Token, Tokenizer}; + + // `U&'…'` / `U&"…"` unicode-escaped literals could smuggle a protected + // identifier past tokenization (the escapes decode to letters); there is no + // legitimate need for them in `db.raw`, so refuse them outright. + let lowered = sql.to_ascii_lowercase(); + if lowered.contains("u&\"") || lowered.contains("u&'") { + return Err("db.raw does not allow unicode-escaped identifiers".into()); + } + + // Tokenizing (rather than substring matching) means the prefix inside string + // literals or comments is ignored, while quoted and schema-qualified + // identifiers are still seen. SQL we cannot tokenize fails closed. + let tokens = Tokenizer::new(&GenericDialect {}, sql) + .tokenize() + .map_err(|e| format!("db.raw could not parse SQL: {e}"))?; + + for token in tokens { + if let Token::Word(word) = token { + let name = word.value.to_ascii_lowercase(); + let is_internal = name.starts_with(PROTECTED_TABLE_PREFIX) + || PROTECTED_EXACT_TABLES.contains(&name.as_str()); + if is_internal && !ALLOWED_INTERNAL_TABLES.contains(&name.as_str()) { + return Err(format!( + "db.raw cannot reference the protected internal HappyView table '{}'", + word.value + )); + } + } + } + + Ok(()) +} + fn is_valid_json_field_path(path: &str) -> bool { if path.is_empty() { return false; @@ -613,6 +695,10 @@ pub fn register_db_api(lua: &Lua, state: Arc) -> LuaResult<()> { lua.create_async_function(move |lua, (sql, params): (String, Option)| { let state = state_raw.clone(); async move { + // Protect HappyView's internal tables (secrets, auth, config, + // AppView bookkeeping) from raw access; own tables are fine. + check_raw_sql_tables(&sql).map_err(mlua::Error::runtime)?; + let mut query = sqlx::query(&sql); if let Some(ref params_table) = params { for value in params_table.sequence_values::() { @@ -809,19 +895,36 @@ mod tests { } #[tokio::test] - async fn raw_allows_non_select() { + async fn raw_allows_non_select_on_own_tables() { let state = test_state(); let lua = setup(&state); + // Non-SELECT statements are allowed against non-internal tables. Passes + // table validation; may then fail on the (empty in-memory) DB. let result: Result = lua - .load(r#"return db.raw("DELETE FROM happyview_records")"#) + .load(r#"return db.raw("DELETE FROM my_table")"#) .eval_async() .await; - // Should fail with a DB connection error, NOT a validation error - assert!(result.is_err()); - let err = result.unwrap_err().to_string(); + if let Err(e) = &result { + let err = e.to_string(); + assert!( + !err.contains("internal HappyView table"), + "should have passed table validation but got: {err}" + ); + } + } + + #[tokio::test] + async fn raw_blocks_internal_tables() { + let state = test_state(); + let lua = setup(&state); + let result: Result = lua + .load(r#"return db.raw("SELECT * FROM happyview_dpop_keys")"#) + .eval_async() + .await; + let err = result.expect_err("querying an internal table must be rejected"); assert!( - !err.contains("only supports SELECT"), - "should have passed validation but got: {err}" + err.to_string().contains("internal HappyView table"), + "expected an internal-table error, got: {err}" ); } @@ -842,6 +945,81 @@ mod tests { } } + #[test] + fn raw_sql_allows_non_protected_tables() { + // Admins can get wild with their own tables. + assert!(super::check_raw_sql_tables("SELECT * FROM my_table").is_ok()); + assert!(super::check_raw_sql_tables("CREATE TABLE analytics (id INT)").is_ok()); + assert!(super::check_raw_sql_tables("INSERT INTO analytics VALUES (1)").is_ok()); + assert!(super::check_raw_sql_tables("UPDATE analytics SET id = 2").is_ok()); + assert!(super::check_raw_sql_tables("DELETE FROM analytics WHERE id = 1").is_ok()); + assert!(super::check_raw_sql_tables("DROP TABLE analytics").is_ok()); + // A table that merely *contains* the prefix mid-name is fine. + assert!(super::check_raw_sql_tables("SELECT * FROM myhappyview_data").is_ok()); + // The prefix appearing inside a string literal is not a table reference. + assert!( + super::check_raw_sql_tables("INSERT INTO logs (msg) VALUES ('happyview_started')") + .is_ok() + ); + } + + #[test] + fn raw_sql_allows_allowlisted_internal_tables() { + // Public AppView data and background jobs are readable/writable. + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_records").is_ok()); + assert!( + super::check_raw_sql_tables("DELETE FROM happyview_records WHERE uri = $1").is_ok() + ); + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_record_refs").is_ok()); + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_labels").is_ok()); + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_lexicons").is_ok()); + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_jobs").is_ok()); + // Space data (access, not confidentiality). + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_space_records").is_ok()); + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_space_members").is_ok()); + } + + #[test] + fn raw_sql_blocks_protected_tables() { + // Secrets / tokens / keys. + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_dpop_keys").is_err()); + assert!(super::check_raw_sql_tables("DROP TABLE happyview_api_keys").is_err()); + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_script_variables").is_err()); + // Auth / privilege / trust config. + assert!(super::check_raw_sql_tables("UPDATE happyview_users SET is_super = true").is_err()); + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_domains").is_err()); + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_instance_settings").is_err()); + // Space credential / key-material tables stay blocked even though other + // space tables are allowed. + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_space_credentials").is_err()); + assert!(super::check_raw_sql_tables("SELECT * FROM happyview_space_repo_state").is_err()); + // The migration bookkeeping table is off-limits too. + assert!(super::check_raw_sql_tables("SELECT * FROM _sqlx_migrations").is_err()); + } + + #[test] + fn raw_sql_blocks_protected_tables_evasion() { + // Case-insensitive. + assert!(super::check_raw_sql_tables("SELECT * FROM HAPPYVIEW_USERS").is_err()); + // Double-quoted identifier. + assert!(super::check_raw_sql_tables(r#"SELECT * FROM "happyview_api_keys""#).is_err()); + // Schema-qualified. + assert!( + super::check_raw_sql_tables("SELECT * FROM public.happyview_dpop_sessions").is_err() + ); + // Second statement in a batch. + assert!(super::check_raw_sql_tables("SELECT 1; SELECT * FROM happyview_users").is_err()); + // JOIN / subquery position. + assert!( + super::check_raw_sql_tables( + "SELECT * FROM my_table JOIN happyview_api_clients USING (id)" + ) + .is_err() + ); + // Unicode-escaped identifier evasion is refused outright. + assert!(super::check_raw_sql_tables(r#"SELECT * FROM U&"happyview_dpop_keys""#).is_err()); + } + #[test] fn valid_json_field_paths() { assert!(super::is_valid_json_field_path("name")); diff --git a/tests/e2e_scripts.rs b/tests/e2e_scripts.rs index 2535f00..654821a 100644 --- a/tests/e2e_scripts.rs +++ b/tests/e2e_scripts.rs @@ -710,12 +710,11 @@ async fn label_script_uri_routes_actor_special_case() { create_script( &app, "labeler.apply:_actor", - // Sentinel: write a row into records-table-as-flag so we can - // detect that the script ran. + // Sentinel: write a row into a caller-owned table (db.raw cannot touch + // internal HappyView tables) so we can detect that the script ran. "function handle() \ - db.raw('INSERT INTO happyview_records (uri, did, collection, rkey, record, cid, created_at) \ - VALUES (?, ?, ?, ?, ?, ?, ?)', \ - {'at://did:plc:flag/flag.col/k', 'did:plc:flag', 'flag.col', 'k', '{}', 'b', '2026-05-01'}) \ + db.raw('CREATE TABLE IF NOT EXISTS script_sentinel (k TEXT)') \ + db.raw('INSERT INTO script_sentinel (k) VALUES (?)', {'fired'}) \ return event \ end", ) @@ -737,9 +736,12 @@ async fn label_script_uri_routes_actor_special_case() { assert!(matches!(outcome, LabelHookOutcome::Continue(_))); // Sentinel row should exist if the script ran. + let (count,): (i64,) = sqlx::query_as("SELECT COUNT(*) FROM script_sentinel WHERE k = 'fired'") + .fetch_one(&app.state.db) + .await + .unwrap(); assert_eq!( - count_records(&app, "at://did:plc:flag/flag.col/k").await, - 1, + count, 1, "labeler.apply:_actor should have fired for bare-DID label" ); } diff --git a/tests/lua_db_api.rs b/tests/lua_db_api.rs index f9045a2..d0d4cda 100644 --- a/tests/lua_db_api.rs +++ b/tests/lua_db_api.rs @@ -328,13 +328,21 @@ async fn db_raw_select_works() { let pool = db::test_pool().await; let backend = db::test_backend(); db::truncate_all(&pool).await; - seed_records(&pool, backend).await; let state = test_state_with_pool(pool, backend).await; let lua = setup_lua(&state); + // db.raw operates freely on the caller's own tables (internal HappyView + // tables are protected — see raw_blocks_internal_table). let result: mlua::Table = lua .load( - r#"return db.raw("SELECT COUNT(*) as cnt FROM happyview_records WHERE collection = $1", {"test.collection"})"#, + r#" + db.raw("DROP TABLE IF EXISTS raw_probe") + db.raw("CREATE TABLE raw_probe (n INT)") + db.raw("INSERT INTO raw_probe (n) VALUES (1), (2), (3)") + local rows = db.raw("SELECT COUNT(*) as cnt FROM raw_probe WHERE n >= $1", {2}) + db.raw("DROP TABLE raw_probe") + return rows + "#, ) .eval_async() .await @@ -343,7 +351,28 @@ async fn db_raw_select_works() { // Result is an array of row tables let first_row: mlua::Table = result.get(1).unwrap(); let cnt: i64 = first_row.get("cnt").unwrap(); - assert_eq!(cnt, 3); + assert_eq!(cnt, 2); +} + +#[tokio::test] +#[serial] +async fn db_raw_blocks_internal_table() { + common::require_db!(); + let pool = db::test_pool().await; + let backend = db::test_backend(); + db::truncate_all(&pool).await; + let state = test_state_with_pool(pool, backend).await; + let lua = setup_lua(&state); + + let result: Result = lua + .load(r#"return db.raw("SELECT * FROM happyview_dpop_keys")"#) + .eval_async() + .await; + let err = result.expect_err("db.raw must reject internal HappyView tables"); + assert!( + err.to_string().contains("internal HappyView table"), + "unexpected error: {err}" + ); } #[tokio::test] diff --git a/web/src/lib/lua-hover.ts b/web/src/lib/lua-hover.ts index 6f85916..b2cc7eb 100644 --- a/web/src/lib/lua-hover.ts +++ b/web/src/lib/lua-hover.ts @@ -157,7 +157,7 @@ export const HOVER_DOCS = new Map([ ["db.count", { signature: "db.count(collection [, did])", description: "Count records in a collection", module: "db" }], ["db.search", { signature: "db.search({collection, field, query, limit?})", description: "Search records by field value — returns {records}", module: "db" }], ["db.backlinks", { signature: "db.backlinks({collection, uri, did?, limit?, cursor?})", description: "Find records that reference a URI via record_refs — returns {records, cursor?}", module: "db" }], - ["db.raw", { signature: "db.raw(sql [, params])", description: "Execute a raw SQL query — returns array of row tables", module: "db" }], + ["db.raw", { signature: "db.raw(sql [, params])", description: "Execute a raw SQL query — returns array of row tables. Can reach your own tables plus the record index and space data; HappyView's sensitive internal tables (secrets, auth, config) are blocked.", module: "db" }], ["db.backend", { signature: "db.backend()", description: "Returns the database backend — \"sqlite\" or \"postgres\"", module: "db" }], // ── HappyView HTTP API ─────────────────────────────────────────────── -- 2.51.2 From 19f41990c14783f7ea1c4389cc1539aed9c4ed6b Mon Sep 17 00:00:00 2001 From: Trezy Date: Tue, 7 Jul 2026 12:48:21 -0500 Subject: [PATCH 5/9] fix: prevent `read_self` from escalating during access merge Signed-off-by: Trezy --- src/spaces/members.rs | 38 +++++++++++++++++++++++++++++++++++--- src/spaces/types.rs | 22 ++++++++++++++++++++++ tests/spaces_db.rs | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 3 deletions(-) diff --git a/src/spaces/members.rs b/src/spaces/members.rs index 2c3eceb..f355697 100644 --- a/src/spaces/members.rs +++ b/src/spaces/members.rs @@ -105,9 +105,12 @@ async fn resolve_delegation_target( } fn merge_access(resolved: &mut HashMap, did: &str, access: SpaceAccess) { - let entry = resolved.entry(did.to_string()).or_insert(SpaceAccess::Read); - if access.can_write() { - *entry = SpaceAccess::Write; + // Seed with the member's real level (so `read_self` survives), then only + // ever upgrade to a higher privilege — never downgrade. Seeding at `Read` + // here would silently promote `read_self` members to full `read`. + let entry = resolved.entry(did.to_string()).or_insert(access); + if access.rank() > entry.rank() { + *entry = access; } } @@ -129,6 +132,35 @@ mod tests { assert_eq!(map["did:plc:user1"], SpaceAccess::Write); } + #[test] + fn merge_access_preserves_read_self() { + // A read_self member must NOT be silently promoted to full read. + let mut map = HashMap::new(); + merge_access(&mut map, "did:plc:user", SpaceAccess::ReadSelf); + assert_eq!(map["did:plc:user"], SpaceAccess::ReadSelf); + } + + #[test] + fn merge_access_upgrades_but_never_downgrades() { + // read_self upgraded by a higher grant on another path. + let mut map = HashMap::new(); + merge_access(&mut map, "u", SpaceAccess::ReadSelf); + merge_access(&mut map, "u", SpaceAccess::Read); + assert_eq!(map["u"], SpaceAccess::Read); + merge_access(&mut map, "u", SpaceAccess::Write); + assert_eq!(map["u"], SpaceAccess::Write); + + // A lower grant on another path never downgrades. + let mut map2 = HashMap::new(); + merge_access(&mut map2, "v", SpaceAccess::Read); + merge_access(&mut map2, "v", SpaceAccess::ReadSelf); + assert_eq!(map2["v"], SpaceAccess::Read); + + merge_access(&mut map2, "v", SpaceAccess::Write); + merge_access(&mut map2, "v", SpaceAccess::ReadSelf); + assert_eq!(map2["v"], SpaceAccess::Write); + } + #[test] fn merge_access_multiple_users() { let mut map = HashMap::new(); diff --git a/src/spaces/types.rs b/src/spaces/types.rs index 622c536..a2c7d06 100644 --- a/src/spaces/types.rs +++ b/src/spaces/types.rs @@ -31,6 +31,20 @@ impl SpaceAccess { matches!(self, SpaceAccess::Write) } + /// Privilege rank used when merging memberships reached via multiple paths + /// (direct + delegation) — the highest rank wins. `read_self` is the most + /// restricted (own repo only), then `read`, then `write`. + /// + /// Note: the enum's declaration order (`Read`, `ReadSelf`, `Write`) does + /// **not** match privilege order, so `Ord` must not be derived — use this. + pub fn rank(&self) -> u8 { + match self { + SpaceAccess::ReadSelf => 0, + SpaceAccess::Read => 1, + SpaceAccess::Write => 2, + } + } + pub fn can_read(&self) -> bool { true } @@ -255,6 +269,14 @@ mod tests { assert!(SpaceAccess::Write.can_write()); } + #[test] + fn space_access_rank_orders_by_privilege() { + // read_self is the most restricted, then read, then write — regardless + // of enum declaration order. + assert!(SpaceAccess::ReadSelf.rank() < SpaceAccess::Read.rank()); + assert!(SpaceAccess::Read.rank() < SpaceAccess::Write.rank()); + } + #[test] fn mint_policy_roundtrip() { assert_eq!( diff --git a/tests/spaces_db.rs b/tests/spaces_db.rs index 345d923..7bdebd3 100644 --- a/tests/spaces_db.rs +++ b/tests/spaces_db.rs @@ -514,6 +514,49 @@ async fn add_and_get_member() { assert!(!fetched.is_delegation); } +#[tokio::test] +#[serial] +async fn resolve_members_preserves_read_self() { + common::require_db!(); + let pool = test_db::test_pool().await; + let backend = test_db::test_backend(); + test_db::truncate_all(&pool).await; + + let space_id = new_id(); + let space = make_space( + &space_id, + "did:plc:rs-owner", + "com.example.readself", + "rs-skey", + ); + spaces_db::create_space(&pool, backend, &space) + .await + .expect("create_space failed"); + + let member_did = "did:plc:rs-member"; + spaces_db::add_member( + &pool, + backend, + &SpaceMember { + id: new_id(), + space_id: space_id.clone(), + did: member_did.to_string(), + access: SpaceAccess::ReadSelf, + is_delegation: false, + granted_by: Some("did:plc:rs-owner".to_string()), + created_at: now_rfc3339(), + }, + ) + .await + .expect("add_member failed"); + + // Resolution must not promote read_self to full read. + let access = happyview::spaces::members::is_member(&pool, backend, &space_id, member_did) + .await + .expect("is_member failed"); + assert_eq!(access, Some(SpaceAccess::ReadSelf)); +} + #[tokio::test] #[serial] async fn remove_member() { -- 2.51.2 From 28cf563fa1b7adefafe30874f41fad88df83e59d Mon Sep 17 00:00:00 2001 From: Trezy Date: Tue, 7 Jul 2026 14:36:11 -0500 Subject: [PATCH 6/9] fix: prevent victim record overwrite Signed-off-by: Trezy --- src/xrpc/procedure.rs | 86 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 72 insertions(+), 14 deletions(-) diff --git a/src/xrpc/procedure.rs b/src/xrpc/procedure.rs index f06e99c..8dfdf3f 100644 --- a/src/xrpc/procedure.rs +++ b/src/xrpc/procedure.rs @@ -11,6 +11,29 @@ use crate::lexicon::ProcedureAction; use crate::record_refs::sync_refs; use crate::repo; +/// Resolve where a put/delete writes in the local index. +/// +/// The client supplies a full AT URI, but only the record key (last segment) is +/// trusted — the URI's DID is **ignored**. The returned index URI is always +/// scoped to the caller's own repo (`did`), so a caller cannot overwrite or +/// delete another user's indexed record by naming their URI. `collection` comes +/// from the server-side lexicon, not the client. +/// +/// Returns `(rkey, index_uri)`. +fn resolve_write_target( + did: &str, + collection: &str, + client_uri: &str, +) -> Result<(String, String), AppError> { + let rkey = client_uri + .split('/') + .next_back() + .filter(|s| !s.is_empty()) + .ok_or_else(|| AppError::BadRequest("invalid AT URI".into()))?; + let index_uri = format!("at://{did}/{collection}/{rkey}"); + Ok((rkey.to_string(), index_uri)) +} + pub(crate) async fn handle_procedure( state: &AppState, method: &str, @@ -287,15 +310,14 @@ async fn handle_put_record( collection: &str, session: &crate::HappyViewOAuthSession, ) -> Result { - let uri = input + let client_uri = input .get("uri") .and_then(|v| v.as_str()) .ok_or_else(|| AppError::BadRequest("missing uri field".into()))?; - let rkey = uri - .split('/') - .next_back() - .ok_or_else(|| AppError::Internal("invalid AT URI".into()))?; + // Trust only the record key from the client URI; scope the index write to + // the caller's own repo so it can't overwrite another user's indexed record. + let (rkey, uri) = resolve_write_target(claims.did(), collection, client_uri)?; // Build record from input, adding $type let mut record = input.clone(); @@ -344,10 +366,10 @@ async fn handle_put_record( backend, ); let _ = sqlx::query(&sql) - .bind(uri) + .bind(&uri) .bind(claims.did()) .bind(collection) - .bind(rkey) + .bind(&rkey) .bind(&record_str) .bind(cid) .bind(&now) @@ -355,7 +377,7 @@ async fn handle_put_record( .execute(&state.db) .await; - let _ = sync_refs(&state.db, uri, collection, &record, backend).await; + let _ = sync_refs(&state.db, &uri, collection, &record, backend).await; Ok(( StatusCode::OK, @@ -375,15 +397,14 @@ async fn handle_delete_record( collection: &str, session: &crate::HappyViewOAuthSession, ) -> Result { - let uri = input + let client_uri = input .get("uri") .and_then(|v| v.as_str()) .ok_or_else(|| AppError::BadRequest("missing uri field".into()))?; - let rkey = uri - .split('/') - .next_back() - .ok_or_else(|| AppError::Internal("invalid AT URI".into()))?; + // Trust only the record key from the client URI; scope the index delete to + // the caller's own repo so it can't delete another user's indexed record. + let (rkey, uri) = resolve_write_target(claims.did(), collection, client_uri)?; let pds_body = json!({ "repo": claims.did(), @@ -402,7 +423,7 @@ async fn handle_delete_record( let backend = state.db_backend; let sql = adapt_sql("DELETE FROM happyview_records WHERE uri = ?", backend); - let _ = sqlx::query(&sql).bind(uri).execute(&state.db).await; + let _ = sqlx::query(&sql).bind(&uri).execute(&state.db).await; Ok(( StatusCode::OK, @@ -606,3 +627,40 @@ async fn handle_dpop_procedure( repo::forward_pds_response(resp).await } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn write_target_scopes_index_uri_to_caller() { + // An authenticated caller names a *victim's* record. The record key is + // honored, but the index URI must be scoped to the caller's own repo — + // otherwise a put/delete would poison or remove the victim's row. + let (rkey, index_uri) = resolve_write_target( + "did:plc:attacker", + "app.some.post", + "at://did:plc:victim/app.some.post/rk1", + ) + .unwrap(); + assert_eq!(rkey, "rk1"); + assert_eq!(index_uri, "at://did:plc:attacker/app.some.post/rk1"); + } + + #[test] + fn write_target_uses_lexicon_collection_not_client_uri() { + // The collection comes from the server-side lexicon, not the client URI. + let (_rkey, index_uri) = resolve_write_target( + "did:plc:me", + "app.real.collection", + "at://did:plc:me/app.spoofed.collection/rk", + ) + .unwrap(); + assert_eq!(index_uri, "at://did:plc:me/app.real.collection/rk"); + } + + #[test] + fn write_target_rejects_empty_rkey() { + assert!(resolve_write_target("did:plc:me", "app.foo", "at://did:plc:me/app.foo/").is_err()); + } +} -- 2.51.2 From af9a4b18e7eea7d6545733564e1c273628dd4a84 Mon Sep 17 00:00:00 2001 From: Trezy Date: Tue, 7 Jul 2026 14:56:48 -0500 Subject: [PATCH 7/9] fix: ensure superadmin-created admin API keys respect scopes Signed-off-by: Trezy --- src/admin/auth.rs | 24 ++++++++++++++-- tests/e2e_admin.rs | 71 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/src/admin/auth.rs b/src/admin/auth.rs index bbd1d19..d6a1536 100644 --- a/src/admin/auth.rs +++ b/src/admin/auth.rs @@ -254,8 +254,13 @@ impl UserAuth { let key_permissions: Vec = serde_json::from_str(&permissions_json).unwrap_or_default(); + // An API key is bounded by its own stored permission list and never + // carries super privileges — otherwise a super admin's "read-only" key + // would grant full admin (H5). A super user implicitly holds every + // permission, so their key's list is used as-is; a non-super user's key + // is additionally intersected with what that user actually holds. let permissions = if is_super { - HashSet::new() + parse_permissions(&key_permissions) } else { Self::load_api_key_permissions(&state.db, &user_id, &key_permissions, backend).await? }; @@ -277,10 +282,25 @@ impl UserAuth { Ok(Some(UserAuth { did, user_id, - is_super, + // A key is never super, regardless of its owner: `require()` must + // consult the key's permissions, and super-only operations (user + // management, transfer_super) stay unavailable via API keys. + is_super: false, permissions, db: state.db.clone(), db_backend: backend, })) } } + +/// Parse a stored key permission list (JSON strings) into a permission set, +/// dropping any unrecognized entries. Used for super-user keys, whose owner +/// implicitly holds every permission so no intersection is needed. +fn parse_permissions(key_permissions: &[String]) -> HashSet { + key_permissions + .iter() + .filter_map(|s| { + serde_json::from_value::(serde_json::Value::String(s.clone())).ok() + }) + .collect() +} diff --git a/tests/e2e_admin.rs b/tests/e2e_admin.rs index cfdf83a..1e5ea6a 100644 --- a/tests/e2e_admin.rs +++ b/tests/e2e_admin.rs @@ -57,6 +57,77 @@ fn admin_delete( .unwrap() } +async fn response_json(resp: axum::http::Response) -> Value { + let body = resp.into_body().collect().await.unwrap().to_bytes(); + serde_json::from_slice(&body).unwrap_or(json!(null)) +} + +fn bearer_get(uri: &str, key: &str) -> Request { + Request::builder() + .uri(uri) + .header("authorization", format!("Bearer {key}")) + .body(Body::empty()) + .unwrap() +} + +/// A super admin's scoped API key must be limited to its stored permissions — +/// it must NOT inherit the owner's super privileges (H5). +#[tokio::test] +#[serial] +async fn super_user_api_key_is_bounded_by_its_permissions() { + common::require_db!(); + let app = TestApp::new().await; + + // The TestApp admin is a super user. Create a key scoped to stats:read only. + let create = app + .router + .clone() + .oneshot(admin_post( + "/admin/api-keys", + app.admin_cookie(), + &json!({ "name": "ci-monitor", "permissions": ["stats:read"] }), + )) + .await + .unwrap(); + assert_eq!(create.status(), StatusCode::CREATED); + let key = response_json(create).await["key"] + .as_str() + .expect("api key returned") + .to_string(); + + // It can reach the permission it was granted. + let allowed = app + .router + .clone() + .oneshot(bearer_get("/admin/stats", &key)) + .await + .unwrap(); + assert_eq!(allowed.status(), StatusCode::OK); + + // It must NOT reach a permission it wasn't granted, even though its owner + // is super. Before the fix this returned 200 (full super via the key). + let denied = app + .router + .clone() + .oneshot(bearer_get("/admin/lexicons", &key)) + .await + .unwrap(); + assert_eq!( + denied.status(), + StatusCode::FORBIDDEN, + "a super user's scoped key must not grant permissions outside its list" + ); + + // And it must not reach super-only operations (user management). + let users = app + .router + .clone() + .oneshot(bearer_get("/admin/users", &key)) + .await + .unwrap(); + assert_ne!(users.status(), StatusCode::OK); +} + // --------------------------------------------------------------------------- // Auth tests // --------------------------------------------------------------------------- -- 2.51.2 From 9e1709772d1017cdc37de4520bd12d768171a2a8 Mon Sep 17 00:00:00 2001 From: Trezy Date: Tue, 7 Jul 2026 15:15:36 -0500 Subject: [PATCH 8/9] fix: validate JWT aud in middleware Signed-off-by: Trezy --- src/auth/middleware.rs | 14 +++- src/auth/service_auth.rs | 23 +++++- tests/e2e_service_identity.rs | 129 ++++++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 5 deletions(-) diff --git a/src/auth/middleware.rs b/src/auth/middleware.rs index 2a5d672..aecc3df 100644 --- a/src/auth/middleware.rs +++ b/src/auth/middleware.rs @@ -123,10 +123,18 @@ impl FromRequestParts for Claims { }); } - // Otherwise, try service auth JWT - let service_auth = super::service_auth::ServiceAuth::from_bearer(token, state).await?; + // Otherwise, try service auth JWT. Route through the same helper the + // XRPC path uses so the token's `aud` is verified against this + // instance's service DID — otherwise a JWT the user minted for a + // different audience would authenticate here and impersonate them + // (H6). `from_bearer` alone checks only the signature and `exp`. + let host = parts + .headers + .get(axum::http::header::HOST) + .and_then(|v| v.to_str().ok()); + let service_claims = try_parse_service_auth(token, state, host).await?; return Ok(Claims { - did: service_auth.did, + did: service_claims.did, client_key: None, dpop_key_id: None, }); diff --git a/src/auth/service_auth.rs b/src/auth/service_auth.rs index fa9f0d9..7edaaa8 100644 --- a/src/auth/service_auth.rs +++ b/src/auth/service_auth.rs @@ -7,6 +7,11 @@ use serde::Deserialize; use crate::AppState; use crate::error::AppError; +/// Maximum accepted `exp - now` for a service-auth JWT (1 hour). atproto tokens +/// are typically valid for ~60s; this generous cap bounds the replay window of a +/// captured token without breaking well-behaved clients. +const MAX_SERVICE_JWT_LIFETIME_SECS: u64 = 3600; + /// Authenticated ATProto user identity extracted from a service auth JWT. /// /// Used for XRPC endpoints that receive proxied requests from PDSes. @@ -132,8 +137,22 @@ fn verify_service_jwt<'a>( return Err(AppError::Auth("JWT expired".into())); } - // Check audience if SERVICE_DID is configured (optional for HappyView). - // For now, accept any audience. + // Bound the acceptance window: atproto service-auth tokens are short-lived + // (~60s). Reject tokens valid absurdly far into the future so a captured + // token isn't replayable for months/years. + if payload.exp > now.saturating_add(MAX_SERVICE_JWT_LIFETIME_SECS) { + tracing::warn!( + exp = payload.exp, + now, + "service auth JWT lifetime exceeds maximum" + ); + return Err(AppError::Auth("JWT lifetime exceeds maximum".into())); + } + + // Audience is validated by the caller against this instance's service DID + // (see `try_parse_service_auth`) — every service-auth entry point routes + // through that check, so a token minted for a different audience is + // rejected rather than trusted here. let _ = &payload.aud; // Check lxm if present (optional validation). diff --git a/tests/e2e_service_identity.rs b/tests/e2e_service_identity.rs index 68af8d6..0941e98 100644 --- a/tests/e2e_service_identity.rs +++ b/tests/e2e_service_identity.rs @@ -2203,3 +2203,132 @@ async fn jwt_with_allowed_typ_accepted() { "JWT with typ=JWT should be accepted" ); } + +// --------------------------------------------------------------------------- +// Service auth — audience validation on the admin/UserAuth path (H6) +// --------------------------------------------------------------------------- + +/// Seed a super user directly, so an authenticated request reaches a 200 rather +/// than a permission error. +async fn seed_super_user(app: &TestApp, did: &str) { + let sql = happyview::db::adapt_sql( + "INSERT INTO happyview_users (id, did, is_super, created_at) VALUES (?, ?, ?, ?)", + app.state.db_backend, + ); + sqlx::query(&sql) + .bind(uuid::Uuid::new_v4().to_string()) + .bind(did) + .bind(1_i32) + .bind(happyview::db::now_rfc3339()) + .execute(&app.state.db) + .await + .expect("seed super user"); +} + +/// A service-auth JWT addressed to THIS instance authenticates on an admin route. +#[tokio::test] +#[serial] +async fn service_auth_correct_aud_accepted_on_admin_route() { + common::require_db!(); + let mut app = TestApp::new().await; + let plc_store = plc::setup_mock_plc(&app.mock_server).await; + let instance_did = app.setup_did_plc().await; + + let issuer = "did:plc:h6-good-issuer"; + seed_super_user(&app, issuer).await; + + let exp = chrono::Utc::now().timestamp() as u64 + 60; + let auth = app + .raw_service_auth_jwt(&plc_store, issuer, &format!("{instance_did}#appview"), exp) + .await; + + let resp = app + .router + .clone() + .oneshot( + Request::builder() + .uri("/admin/stats") + .header("authorization", &auth) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::OK); +} + +/// A service-auth JWT the user minted for a DIFFERENT audience must NOT +/// authenticate on the admin path — otherwise it impersonates the issuer. +#[tokio::test] +#[serial] +async fn service_auth_wrong_aud_rejected_on_admin_route() { + common::require_db!(); + let mut app = TestApp::new().await; + let plc_store = plc::setup_mock_plc(&app.mock_server).await; + let _instance_did = app.setup_did_plc().await; + + // The issuer is even a super user — so if the token authenticated, it would + // return 200. The point is that the wrong audience blocks it entirely. + let issuer = "did:plc:h6-bad-issuer"; + seed_super_user(&app, issuer).await; + + let exp = chrono::Utc::now().timestamp() as u64 + 60; + let auth = app + .raw_service_auth_jwt(&plc_store, issuer, "did:web:evil.example#appview", exp) + .await; + + let resp = app + .router + .clone() + .oneshot( + Request::builder() + .uri("/admin/stats") + .header("authorization", &auth) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::UNAUTHORIZED, + "a service-auth JWT for a different audience must not authenticate here" + ); +} + +/// A service-auth JWT valid absurdly far into the future is rejected even when +/// correctly addressed to this instance (bounds the replay window). +#[tokio::test] +#[serial] +async fn service_auth_excessive_lifetime_rejected() { + common::require_db!(); + let mut app = TestApp::new().await; + let plc_store = plc::setup_mock_plc(&app.mock_server).await; + let instance_did = app.setup_did_plc().await; + + let issuer = "did:plc:h6-longlived"; + seed_super_user(&app, issuer).await; + + // exp two hours out — well beyond the accepted max. + let exp = chrono::Utc::now().timestamp() as u64 + 7200; + let auth = app + .raw_service_auth_jwt(&plc_store, issuer, &format!("{instance_did}#appview"), exp) + .await; + + let resp = app + .router + .clone() + .oneshot( + Request::builder() + .uri("/admin/stats") + .header("authorization", &auth) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); +} -- 2.51.2 From c12a9e3c5ea16801094db41c87d5593d7bfcc300 Mon Sep 17 00:00:00 2001 From: Trezy Date: Tue, 7 Jul 2026 16:23:27 -0500 Subject: [PATCH 9/9] fix: require auth on space notification endpoints Signed-off-by: Trezy --- src/spaces/routes.rs | 29 +++++++- tests/spaces_notify_auth.rs | 140 ++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 tests/spaces_notify_auth.rs diff --git a/src/spaces/routes.rs b/src/spaces/routes.rs index 3ca762c..0e927ae 100644 --- a/src/spaces/routes.rs +++ b/src/spaces/routes.rs @@ -380,6 +380,22 @@ async fn require_auth_or_credential( )) } +/// Resolve the caller's DID from an authenticated identity for the inter-service +/// notify routes: a DPoP/cookie identity or a verified service-auth JWT. (Space +/// credentials are not accepted here — their subject is a space, not the +/// authority DID these routes gate on.) +fn require_notify_caller(claims: &XrpcClaims) -> Result { + if let Some(identity) = &claims.identity { + return Ok(identity.did().to_string()); + } + if let Some(service_auth) = &claims.service_auth { + return Ok(service_auth.did.clone()); + } + Err(AppError::Auth( + "This endpoint requires authentication".into(), + )) +} + async fn resolve_space(state: &AppState, space_uri: &str) -> Result { let uri = SpaceUri::parse(space_uri)?; db::get_space_by_address( @@ -1420,10 +1436,15 @@ async fn register_notify( async fn notify_write( State(state): State, - _claims: XrpcClaims, + claims: XrpcClaims, Json(input): Json, ) -> Result { + // Inter-service route: only the space authority (or a super admin) may fire + // write notifications. Previously this ignored the caller entirely, letting + // anyone spam/forge notifications to registered endpoints (M1). + let did = require_notify_caller(&claims)?; let space = resolve_space(&state, &input.space).await?; + require_space_admin(&state, &space, &did).await?; notifications::dispatch_write_notification( &state.db, @@ -1442,10 +1463,14 @@ async fn notify_write( async fn notify_space_deleted( State(state): State, - _claims: XrpcClaims, + claims: XrpcClaims, Json(input): Json, ) -> Result { + // Inter-service route: only the space authority (or a super admin) may fire + // "space deleted" events (which may cause consumers to purge cached data). + let did = require_notify_caller(&claims)?; let space = resolve_space(&state, &input.space).await?; + require_space_admin(&state, &space, &did).await?; notifications::dispatch_space_deleted(&state.db, state.db_backend, &state.http, &space.id) .await?; diff --git a/tests/spaces_notify_auth.rs b/tests/spaces_notify_auth.rs new file mode 100644 index 0000000..b090944 --- /dev/null +++ b/tests/spaces_notify_auth.rs @@ -0,0 +1,140 @@ +mod common; + +use axum::body::Body; +use axum::http::{Request, StatusCode}; +use happyview::db::now_rfc3339; +use happyview::spaces::db as spaces_db; +use happyview::spaces::types::*; +use serde_json::json; +use serial_test::serial; +use tower::ServiceExt; +use uuid::Uuid; + +use common::app::TestApp; + +const SPACE_DID: &str = "did:plc:spacehost"; +const SPACE_TYPE: &str = "com.example.notify"; +const SPACE_SKEY: &str = "main"; + +fn space_uri() -> String { + format!("at://{SPACE_DID}/space/{SPACE_TYPE}/{SPACE_SKEY}") +} + +async fn enable_spaces(app: &TestApp) { + let (name, value) = app.admin_cookie(); + let req = Request::builder() + .method("PUT") + .uri("/admin/settings/feature.spaces_enabled") + .header(name, value) + .header("content-type", "application/json") + .body(Body::from(json!({ "value": "true" }).to_string())) + .unwrap(); + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert!(resp.status().is_success(), "failed to enable spaces flag"); +} + +async fn create_space(app: &TestApp) { + let now = now_rfc3339(); + let space = Space { + id: Uuid::new_v4().to_string(), + did: SPACE_DID.to_string(), + authority_did: SPACE_DID.to_string(), + creator_did: SPACE_DID.to_string(), + type_nsid: SPACE_TYPE.to_string(), + skey: SPACE_SKEY.to_string(), + display_name: Some("Notify Space".to_string()), + description: None, + mint_policy: MintPolicy::MemberList, + app_access: AppAccess::Open, + managing_app_did: None, + config: SpaceConfig::default(), + revision: None, + created_at: now.clone(), + updated_at: now, + }; + spaces_db::create_space(&app.state.db, app.state.db_backend, &space) + .await + .expect("create_space failed"); +} + +fn notify_write_req( + auth_cookie: Option<(axum::http::HeaderName, axum::http::HeaderValue)>, +) -> Request { + let mut b = Request::builder() + .method("POST") + .uri("/xrpc/com.atproto.space.notifyWrite") + .header("content-type", "application/json"); + if let Some((name, value)) = auth_cookie { + b = b.header(name, value); + } + b.body(Body::from( + json!({ + "space": space_uri(), + "did": "did:plc:someauthor", + "collection": "com.example.post", + "rkey": "rk1", + }) + .to_string(), + )) + .unwrap() +} + +/// An unauthenticated caller must NOT be able to fire write notifications. +/// Before the fix this returned success (the caller was ignored entirely). +#[tokio::test] +#[serial] +async fn notify_write_rejects_unauthenticated() { + common::require_db!(); + let app = TestApp::new().await; + enable_spaces(&app).await; + create_space(&app).await; + + let resp = app + .router + .clone() + .oneshot(notify_write_req(None)) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::UNAUTHORIZED, + "unauthenticated notifyWrite must be rejected" + ); +} + +/// A super admin is allowed (require_space_admin accepts authority or super). +#[tokio::test] +#[serial] +async fn notify_write_allows_super_admin() { + common::require_db!(); + let app = TestApp::new().await; + enable_spaces(&app).await; + create_space(&app).await; + + let resp = app + .router + .clone() + .oneshot(notify_write_req(Some(app.admin_cookie()))) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); +} + +/// notifySpaceDeleted is likewise gated. +#[tokio::test] +#[serial] +async fn notify_space_deleted_rejects_unauthenticated() { + common::require_db!(); + let app = TestApp::new().await; + enable_spaces(&app).await; + create_space(&app).await; + + let req = Request::builder() + .method("POST") + .uri("/xrpc/com.atproto.space.notifySpaceDeleted") + .header("content-type", "application/json") + .body(Body::from(json!({ "space": space_uri() }).to_string())) + .unwrap(); + let resp = app.router.clone().oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); +}