Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 35 additions & 9 deletions api/src/api/http/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<keycast_core::types::permission::Permission>, 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(
Expand Down Expand Up @@ -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<Vec<Box<dyn CustomPermission>>, _> = permissions
Expand Down Expand Up @@ -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<Vec<Box<dyn CustomPermission>>, _> = permissions
.iter()
Expand Down Expand Up @@ -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<Vec<Box<dyn CustomPermission>>, _> = permissions
.iter()
Expand Down
14 changes: 14 additions & 0 deletions api/src/api/http/nostr_rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,20 @@ async fn load_handler_on_demand(
// Load permissions for this authorization's policy (if any)
let permissions: Vec<Box<dyn CustomPermission>> = 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))
})?;
Expand Down
109 changes: 106 additions & 3 deletions api/tests/rpc_permission_edge_cases_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -402,7 +490,7 @@ async fn test_policy_enforces_kind_restrictions() {
}

// ============================================================================
// Test 5: Expired Authorization Rejected
// Test 7: Expired Authorization Rejected
// ============================================================================

#[tokio::test]
Expand Down Expand Up @@ -454,7 +542,7 @@ async fn test_expired_authorization_rejected() {
}

// ============================================================================
// Test 6: Encrypt Requires Authorization
// Test 8: Encrypt Requires Authorization
// ============================================================================

#[tokio::test]
Expand Down Expand Up @@ -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"
);
}
2 changes: 2 additions & 0 deletions core/src/types/authorization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand Down
11 changes: 11 additions & 0 deletions core/src/types/oauth_authorization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>(
Expand Down
22 changes: 22 additions & 0 deletions database/migrations/20260504124000_add_oauth_policy_fk.sql
Original file line number Diff line number Diff line change
@@ -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,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep revoked OAuth rows out before any NULL policy branch can allow access.
This migration sets revoked_at and clears dangling policy_id to NULL, but the later origin lookup and signer reload can still evaluate that row as unrestricted access.
Add revoked_at IS NULL to the active OAuth authorization lookup and make signer permission validation reject any refetched OAuth authorization with revoked_at set.
Please cover both the origin-based HTTP validator and signer validation for a revoked policy_id = NULL row, while preserving active NULL-policy access and valid empty-policy access.

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;
58 changes: 27 additions & 31 deletions signer/src/signer_daemon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -63,6 +64,28 @@ pub struct Nip46Handler {
}

impl Nip46Handler {
async fn load_permissions_for_validation(&self) -> SignerResult<Vec<Permission>> {
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(
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down
Loading
Loading