diff --git a/api/src/api/http/auth.rs b/api/src/api/http/auth.rs index f2c98b0b..ef297788 100644 --- a/api/src/api/http/auth.rs +++ b/api/src/api/http/auth.rs @@ -2693,6 +2693,35 @@ pub async fn get_authorization_for_origin( } } +async fn load_policy_permissions_or_deny( + pool: &PgPool, + policy_id: i32, + user_pubkey: &str, + tenant_id: i64, +) -> Result, AuthError> { + let policy_repo = PolicyRepository::new(pool.clone()); + match policy_repo.find(policy_id).await { + Ok(_) => {} + Err(keycast_core::repositories::RepositoryError::NotFound(_)) => { + tracing::warn!( + "Denying OAuth request for user {} in tenant {} due to dangling policy_id {}", + user_pubkey, + tenant_id, + policy_id + ); + return Err(AuthError::Forbidden( + "Authorization policy is missing. Re-authorize this app.".to_string(), + )); + } + Err(e) => return Err(e.into()), + } + + policy_repo + .get_permissions(policy_id) + .await + .map_err(Into::into) +} + /// Validate that the user has permission to sign this event /// Returns () if successful, or an error if unauthorized pub async fn validate_signing_permissions( @@ -2721,9 +2750,8 @@ pub async fn validate_signing_permissions( } }; - // Load permissions for this policy - let policy_repo = PolicyRepository::new(pool.clone()); - let permissions = policy_repo.get_permissions(policy_id).await?; + let permissions = + load_policy_permissions_or_deny(pool, policy_id, user_pubkey, tenant_id).await?; // Convert to custom permissions let custom_permissions: Result>, _> = permissions @@ -2802,9 +2830,8 @@ pub async fn validate_encrypt_permissions( } }; - // Load permissions for this policy - let policy_repo = PolicyRepository::new(pool.clone()); - let permissions = policy_repo.get_permissions(policy_id).await?; + let permissions = + load_policy_permissions_or_deny(pool, policy_id, user_pubkey, tenant_id).await?; let custom_permissions: Result>, _> = permissions .iter() @@ -2883,9 +2910,8 @@ pub async fn validate_decrypt_permissions( } }; - // Load permissions for this policy - let policy_repo = PolicyRepository::new(pool.clone()); - let permissions = policy_repo.get_permissions(policy_id).await?; + let permissions = + load_policy_permissions_or_deny(pool, policy_id, user_pubkey, tenant_id).await?; let custom_permissions: Result>, _> = permissions .iter() diff --git a/api/src/api/http/nostr_rpc.rs b/api/src/api/http/nostr_rpc.rs index 22564840..518693ca 100644 --- a/api/src/api/http/nostr_rpc.rs +++ b/api/src/api/http/nostr_rpc.rs @@ -263,6 +263,20 @@ async fn load_handler_on_demand( // Load permissions for this authorization's policy (if any) let permissions: Vec> = if let Some(pid) = policy_id { let policy_repo = PolicyRepository::new(pool.clone()); + match policy_repo.find(pid).await { + Ok(_) => {} + Err(keycast_core::repositories::RepositoryError::NotFound(_)) => { + return Err(RpcError::Auth(AuthError::Forbidden( + "Authorization policy is missing. Re-authorize this app.".to_string(), + ))); + } + Err(e) => { + return Err(RpcError::Internal(format!( + "Database error loading policy: {}", + e + ))); + } + } let db_permissions = policy_repo.get_permissions(pid).await.map_err(|e| { RpcError::Internal(format!("Database error loading permissions: {}", e)) })?; diff --git a/api/tests/rpc_permission_edge_cases_test.rs b/api/tests/rpc_permission_edge_cases_test.rs index 8f78e42d..53f4405e 100644 --- a/api/tests/rpc_permission_edge_cases_test.rs +++ b/api/tests/rpc_permission_edge_cases_test.rs @@ -9,6 +9,7 @@ use chrono::{Duration, Utc}; use keycast_api::ucan_auth::{nostr_pubkey_to_did, validate_ucan_token, NostrKeyMaterial}; use keycast_core::encryption::file_key_manager::FileKeyManager; use keycast_core::encryption::KeyManager; +use keycast_core::repositories::RepositoryError; use nostr_sdk::prelude::*; use serde_json::json; use sqlx::PgPool; @@ -337,7 +338,94 @@ async fn test_null_policy_grants_full_access() { } // ============================================================================ -// Test 4: Policy Enforces Kind Restrictions +// Test 4: Valid policy with zero permissions remains permissive (backward compatibility) +// ============================================================================ + +#[tokio::test] +async fn test_valid_empty_policy_grants_access() { + let pool = setup_db().await; + let tenant_id = create_test_tenant(&pool).await; + let (keys, pubkey) = create_test_user(); + let key_manager = FileKeyManager::new().expect("Failed to create key manager"); + + insert_user(&pool, tenant_id, &pubkey).await; + create_personal_key(&pool, tenant_id, &pubkey, &keys, &key_manager).await; + + // Create a real policy row but link no permissions. + let policy_id = create_test_policy(&pool, tenant_id, vec![]).await; + let redirect_origin = format!("https://empty-policy-{}.example.com", Uuid::new_v4()); + + create_test_authorization( + &pool, + tenant_id, + &pubkey, + &redirect_origin, + Some(policy_id), + None, + &key_manager, + ) + .await; + + let unsigned_event = + EventBuilder::new(Kind::EncryptedDirectMessage, "No restrictions").build(keys.public_key()); + + let result = keycast_api::api::http::auth::validate_signing_permissions( + &pool, + tenant_id, + &pubkey, + &redirect_origin, + &unsigned_event, + ) + .await; + + assert!( + result.is_ok(), + "Valid policy with zero linked permissions should stay permissive: {:?}", + result + ); +} + +// ============================================================================ +// Test 5: Referenced OAuth policy cannot be deleted (prevents dangling policy_id) +// ============================================================================ + +#[tokio::test] +async fn test_referenced_oauth_policy_delete_is_restricted() { + let pool = setup_db().await; + let tenant_id = create_test_tenant(&pool).await; + let (keys, pubkey) = create_test_user(); + let key_manager = FileKeyManager::new().expect("Failed to create key manager"); + + insert_user(&pool, tenant_id, &pubkey).await; + create_personal_key(&pool, tenant_id, &pubkey, &keys, &key_manager).await; + + let policy_id = create_test_policy(&pool, tenant_id, vec![]).await; + let redirect_origin = format!("https://fk-policy-{}.example.com", Uuid::new_v4()); + + create_test_authorization( + &pool, + tenant_id, + &pubkey, + &redirect_origin, + Some(policy_id), + None, + &key_manager, + ) + .await; + + let delete_result = sqlx::query("DELETE FROM policies WHERE id = $1") + .bind(policy_id) + .execute(&pool) + .await; + + assert!( + delete_result.is_err(), + "Deleting a policy referenced by oauth_authorizations should be restricted" + ); +} + +// ============================================================================ +// Test 6: Policy Enforces Kind Restrictions // ============================================================================ #[tokio::test] @@ -402,7 +490,7 @@ async fn test_policy_enforces_kind_restrictions() { } // ============================================================================ -// Test 5: Expired Authorization Rejected +// Test 7: Expired Authorization Rejected // ============================================================================ #[tokio::test] @@ -454,7 +542,7 @@ async fn test_expired_authorization_rejected() { } // ============================================================================ -// Test 6: Encrypt Requires Authorization +// Test 8: Encrypt Requires Authorization // ============================================================================ #[tokio::test] @@ -608,3 +696,18 @@ async fn test_ucan_with_bunker_pubkey_returns_some() { "bunker_pubkey should match" ); } + +#[test] +fn test_repository_database_error_maps_to_internal_not_forbidden() { + let auth_err: keycast_api::api::http::auth::AuthError = + RepositoryError::Database("temporary db outage".to_string()).into(); + + assert!( + matches!( + auth_err, + keycast_api::api::http::auth::AuthError::Internal(msg) + if msg.contains("temporary db outage") + ), + "Repository database errors must not be reclassified as Forbidden" + ); +} diff --git a/core/src/types/authorization.rs b/core/src/types/authorization.rs index 46d4d6e8..4e75864a 100644 --- a/core/src/types/authorization.rs +++ b/core/src/types/authorization.rs @@ -24,6 +24,8 @@ pub enum AuthorizationError { InvalidSecret, #[error("Unauthorized by permission")] Unauthorized, + #[error("OAuth authorization references missing policy_id {0}")] + DanglingPolicy(i32), #[error("Unsupported request")] UnsupportedRequest, } diff --git a/core/src/types/oauth_authorization.rs b/core/src/types/oauth_authorization.rs index 99cac6a1..d04ccc35 100644 --- a/core/src/types/oauth_authorization.rs +++ b/core/src/types/oauth_authorization.rs @@ -65,6 +65,17 @@ impl OAuthAuthorization { None => return Ok(vec![]), }; + // Distinguish "valid policy with no permissions" from "dangling policy_id". + let policy_exists: bool = + sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM policies WHERE id = $1)") + .bind(policy_id) + .fetch_one(pool) + .await + .map_err(AuthorizationError::Database)?; + if !policy_exists { + return Err(AuthorizationError::DanglingPolicy(policy_id)); + } + // Load permissions from database // Tenant isolation is enforced at authorization lookup level let permissions = sqlx::query_as::<_, crate::types::permission::Permission>( diff --git a/database/migrations/20260504124000_add_oauth_policy_fk.sql b/database/migrations/20260504124000_add_oauth_policy_fk.sql new file mode 100644 index 00000000..21e82d99 --- /dev/null +++ b/database/migrations/20260504124000_add_oauth_policy_fk.sql @@ -0,0 +1,22 @@ +-- Fail closed for dangling oauth_authorizations.policy_id references. +-- 1) Revoke active OAuth authorizations pointing at missing policies. +-- 2) Clear dangling policy_id for all affected rows so FK can be added safely. +-- 3) Add FK to prevent future dangling references. + +UPDATE oauth_authorizations oa +SET revoked_at = CASE + WHEN oa.revoked_at IS NULL THEN NOW() + ELSE oa.revoked_at + END, + policy_id = NULL, + updated_at = NOW() +WHERE oa.policy_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 + FROM policies p + WHERE p.id = oa.policy_id + ); + +ALTER TABLE ONLY public.oauth_authorizations + ADD CONSTRAINT oauth_authorizations_policy_id_fkey + FOREIGN KEY (policy_id) REFERENCES public.policies(id) ON DELETE RESTRICT; diff --git a/signer/src/signer_daemon.rs b/signer/src/signer_daemon.rs index c049df63..247a95c8 100644 --- a/signer/src/signer_daemon.rs +++ b/signer/src/signer_daemon.rs @@ -10,8 +10,9 @@ use keycast_core::encryption::KeyManager; use keycast_core::metrics::METRICS; use keycast_core::signing_handler::SigningHandler; use keycast_core::signing_session::canonicalize_event_author; -use keycast_core::types::authorization::Authorization; +use keycast_core::types::authorization::{Authorization, AuthorizationError}; use keycast_core::types::oauth_authorization::OAuthAuthorization; +use keycast_core::types::permission::Permission; use moka::future::Cache; use nostr_sdk::prelude::*; use secrecy::{ExposeSecret, SecretString}; @@ -63,6 +64,28 @@ pub struct Nip46Handler { } impl Nip46Handler { + async fn load_permissions_for_validation(&self) -> SignerResult> { + if self.is_oauth { + let oauth_auth = + OAuthAuthorization::find(&self.pool, self.tenant_id, self.authorization_id).await?; + return match oauth_auth.permissions(&self.pool, self.tenant_id).await { + Ok(permissions) => Ok(permissions), + Err(AuthorizationError::DanglingPolicy(policy_id)) => { + Err(SignerError::permission_denied(format!( + "OAuth authorization {} references missing policy_id {}", + self.authorization_id, policy_id + ))) + } + Err(e) => Err(e.into()), + }; + } + + let auth = Authorization::find(&self.pool, self.tenant_id, self.authorization_id).await?; + auth.permissions(&self.pool, self.tenant_id) + .await + .map_err(Into::into) + } + /// Constructor for testing only - do not use in production code #[doc(hidden)] pub fn new_for_test( @@ -128,16 +151,7 @@ impl Nip46Handler { &self, unsigned_event: &UnsignedEvent, ) -> SignerResult<()> { - // Load permissions based on authorization type - let permissions = if self.is_oauth { - let oauth_auth = - OAuthAuthorization::find(&self.pool, self.tenant_id, self.authorization_id).await?; - oauth_auth.permissions(&self.pool, self.tenant_id).await? - } else { - let auth = - Authorization::find(&self.pool, self.tenant_id, self.authorization_id).await?; - auth.permissions(&self.pool, self.tenant_id).await? - }; + let permissions = self.load_permissions_for_validation().await?; // If no permissions configured, allow all (backward compatibility) if permissions.is_empty() { @@ -171,16 +185,7 @@ impl Nip46Handler { plaintext: &str, recipient_pubkey: &PublicKey, ) -> SignerResult<()> { - // Load permissions based on authorization type - let permissions = if self.is_oauth { - let oauth_auth = - OAuthAuthorization::find(&self.pool, self.tenant_id, self.authorization_id).await?; - oauth_auth.permissions(&self.pool, self.tenant_id).await? - } else { - let auth = - Authorization::find(&self.pool, self.tenant_id, self.authorization_id).await?; - auth.permissions(&self.pool, self.tenant_id).await? - }; + let permissions = self.load_permissions_for_validation().await?; // If no permissions configured, allow all (backward compatibility) if permissions.is_empty() { @@ -216,16 +221,7 @@ impl Nip46Handler { ciphertext: &str, sender_pubkey: &PublicKey, ) -> SignerResult<()> { - // Load permissions based on authorization type - let permissions = if self.is_oauth { - let oauth_auth = - OAuthAuthorization::find(&self.pool, self.tenant_id, self.authorization_id).await?; - oauth_auth.permissions(&self.pool, self.tenant_id).await? - } else { - let auth = - Authorization::find(&self.pool, self.tenant_id, self.authorization_id).await?; - auth.permissions(&self.pool, self.tenant_id).await? - }; + let permissions = self.load_permissions_for_validation().await?; // If no permissions configured, allow all (backward compatibility) if permissions.is_empty() { diff --git a/signer/tests/permission_validation_tests.rs b/signer/tests/permission_validation_tests.rs index ae915542..def3eea4 100644 --- a/signer/tests/permission_validation_tests.rs +++ b/signer/tests/permission_validation_tests.rs @@ -6,7 +6,7 @@ use chrono::{Duration, Utc}; use keycast_core::encryption::{file_key_manager::FileKeyManager, KeyManager}; use keycast_core::signing_handler::SigningHandler; -use keycast_core::types::authorization::Authorization; +use keycast_core::types::authorization::{Authorization, AuthorizationError}; use keycast_core::types::oauth_authorization::OAuthAuthorization; use keycast_signer::Nip46Handler; use nostr_sdk::prelude::*; @@ -1173,36 +1173,26 @@ async fn test_19_content_filter_blocks_encrypt_of_blocked_plaintext() { ); } -/// Regression: an OAuth authorization with a dangling `policy_id` (no matching row in -/// `policies`) currently degrades to "no permissions = allow". This test pins that behavior. -/// See divinevideo/keycast#141 for the open question of whether this should fail closed. +/// OAuth authorization permissions must fail closed when `policy_id` does not exist. #[tokio::test] -async fn test_20_oauth_invalid_policy_id_allows_signing() { +async fn test_20_oauth_invalid_policy_id_is_denied() { let pool = setup_test_db().await; let key_manager = FileKeyManager::new().expect("Failed to create key manager"); - // OAuth authorizations can reference a non-existent policy_id. - // Current behavior treats missing policy permissions as unrestricted access. + // Create a valid OAuth authorization first, then simulate a dangling policy reference + // in-memory to avoid violating DB-level foreign key constraints. let invalid_policy_id = i32::MAX; - let (oauth_auth, user_keys) = - create_oauth_authorization(&pool, 1, Some(invalid_policy_id), &key_manager).await; - - let handler = Nip46Handler::new_for_test( - user_keys.clone(), - user_keys.clone(), - oauth_auth.secret_hash.clone(), - oauth_auth.id, - 1, - true, - pool.clone(), - ); - - let unsigned = EventBuilder::new(Kind::EncryptedDirectMessage, "Test message") - .build(user_keys.public_key()); - let result = handler.sign_event_direct(unsigned).await; + let (mut oauth_auth, _user_keys) = + create_oauth_authorization(&pool, 1, None, &key_manager).await; + oauth_auth.policy_id = Some(invalid_policy_id); + let result = oauth_auth.permissions(&pool, 1).await; assert!( - result.is_ok(), - "OAuth authorization with invalid policy_id should follow current permissive behavior" + matches!( + result, + Err(AuthorizationError::DanglingPolicy(policy_id)) if policy_id == invalid_policy_id + ), + "OAuth authorization with dangling policy_id must fail closed, got: {:?}", + result ); }