From bc135cdc9fb07799e929423c507d3840405e479a Mon Sep 17 00:00:00 2001 From: Trezy Date: Sat, 27 Jun 2026 06:25:05 +0000 Subject: [PATCH] fix: address copilot feedback Signed-off-by: Trezy Signed-off-by: Trezy --- migrations/sqlite/20260627000000_proposal_0016_alignment.sql | 2 +- packages/docs/content/docs/guides/lua-scripting.md | 2 +- src/admin/verification_methods.rs | 17 +++++++++++++++++ src/lua/context.rs | 6 +++--- src/spaces/auth.rs | 10 +++++++++- src/spaces/credential.rs | 39 +++++++++++++++++++++++++++++++++++---- src/spaces/integration_tests.rs | 4 ++-- src/spaces/routes.rs | 39 +++++++++++++++++++++++++++++++++++---- src/spaces/simplespace.rs | 30 ++++-------------------------- tests/e2e_feature_flags.rs | 6 +++--- 10 file(s) changed, 110 insertion(s)(+), 45 deletion(s)(-) diff --git a/migrations/sqlite/20260627000000_proposal_0016_alignment.sql b/migrations/sqlite/20260627000000_proposal_0016_alignment.sql --- a/migrations/sqlite/20260627000000_proposal_0016_alignment.sql +++ b/migrations/sqlite/20260627000000_proposal_0016_alignment.sql @@ -39,7 +39,7 @@ CREATE TABLE happyview_space_repo_state ( id TEXT PRIMARY KEY, space_id TEXT NOT NULL REFERENCES happyview_spaces(id) ON DELETE CASCADE, author_did TEXT NOT NULL, - lthash_state BLOB NOT NULL DEFAULT X'', + lthash_state BLOB NOT NULL DEFAULT (zeroblob(2048)), rev TEXT, hash BLOB, ikm BLOB, diff --git a/packages/docs/content/docs/guides/lua-scripting.md b/packages/docs/content/docs/guides/lua-scripting.md --- a/packages/docs/content/docs/guides/lua-scripting.md +++ b/packages/docs/content/docs/guides/lua-scripting.md @@ -74,7 +74,7 @@ | ----------- | ------ | -------------------------------------------------------- | | `space` | string | The full `ats://` space URI | | `space_id` | string | Internal space identifier | | `did` | string | The space's DID | -| `owner_did` | string | The space authority's DID | +| `authority_did` | string | The space authority's DID | | `type_nsid` | string | Space type NSID | | `skey` | string | Space key | diff --git a/src/admin/verification_methods.rs b/src/admin/verification_methods.rs --- a/src/admin/verification_methods.rs +++ b/src/admin/verification_methods.rs @@ -32,6 +32,23 @@ Json(body): Json, ) -> Result<(StatusCode, Json), AppError> { auth.require(Permission::SettingsManage).await?; + if !body.fragment_id.starts_with('#') { + return Err(AppError::BadRequest( + "fragment_id must start with '#'".into(), + )); + } + let frag_body = &body.fragment_id[1..]; + if frag_body.is_empty() + || !frag_body + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_') + { + return Err(AppError::BadRequest( + "fragment_id must contain only alphanumeric characters and underscores after '#'" + .into(), + )); + } + let encryption_key = state .config .token_encryption_key diff --git a/src/lua/context.rs b/src/lua/context.rs --- a/src/lua/context.rs +++ b/src/lua/context.rs @@ -8,7 +8,7 @@ pub struct SpaceContext { pub space: String, pub space_id: String, pub did: String, - pub owner_did: String, + pub authority_did: String, pub type_nsid: String, pub skey: String, } @@ -21,7 +21,7 @@ let table = lua.create_table()?; table.set("space", ctx.space.as_str())?; table.set("space_id", ctx.space_id.as_str())?; table.set("did", ctx.did.as_str())?; - table.set("owner_did", ctx.owner_did.as_str())?; + table.set("authority_did", ctx.authority_did.as_str())?; table.set("type_nsid", ctx.type_nsid.as_str())?; table.set("skey", ctx.skey.as_str())?; globals.set("space", table)?; @@ -274,7 +274,7 @@ let space = SpaceContext { space: "ats://did:plc:owner/com.example.forum/main".into(), space_id: "space-123".into(), did: "did:plc:owner".into(), - owner_did: "did:plc:owner".into(), + authority_did: "did:plc:owner".into(), type_nsid: "com.example.forum".into(), skey: "main".into(), }; diff --git a/src/spaces/auth.rs b/src/spaces/auth.rs --- a/src/spaces/auth.rs +++ b/src/spaces/auth.rs @@ -111,11 +111,19 @@ authority_did: &str, ) -> Result { // Parse DID#fragment — the fragment identifies the service endpoint in the DID doc. // For outbound callback we derive the endpoint from the DID. - let (did, _fragment) = if let Some(pos) = managing_app.find('#') { + let (did, fragment) = if let Some(pos) = managing_app.find('#') { (&managing_app[..pos], Some(&managing_app[pos + 1..])) } else { (managing_app, None) }; + + if let Some(frag) = fragment + && frag != "atproto_pds" + { + return Err(AppError::BadRequest(format!( + "unsupported service fragment '#{frag}' for managing app" + ))); + } // Resolve the managing app's PDS/service endpoint from its DID document. let endpoint = resolve_did_service_endpoint(http, did).await?; diff --git a/src/spaces/credential.rs b/src/spaces/credential.rs --- a/src/spaces/credential.rs +++ b/src/spaces/credential.rs @@ -25,6 +25,17 @@ let header: serde_json::Value = serde_json::from_slice(&header_bytes).ok()?; header["typ"].as_str().map(|s| s.to_string()) } +/// Peek at a delegation token's payload to extract the `sub` (space URI) without verifying. +pub fn peek_delegation_sub(token: &str) -> Option { + let parts: Vec<&str> = token.split('.').collect(); + if parts.len() != 3 { + return None; + } + let payload_bytes = URL_SAFE_NO_PAD.decode(parts[1]).ok()?; + let claims: DelegationTokenClaims = serde_json::from_slice(&payload_bytes).ok()?; + Some(claims.sub) +} + /// Peek at a space credential JWT's payload to extract the `sub` (space URI) without verifying. pub fn peek_credential_sub(token: &str) -> Option { let parts: Vec<&str> = token.split('.').collect(); @@ -69,6 +80,7 @@ pub fn verify_delegation_token( token: &str, verifying_key: &K256VerifyingKey, + expected_aud: &str, ) -> Result { let parts: Vec<&str> = token.split('.').collect(); if parts.len() != 3 { @@ -130,6 +142,12 @@ .as_secs(); if now >= claims.exp { return Err(AppError::Auth("delegation token has expired".into())); + } + + if claims.aud != expected_aud { + return Err(AppError::Auth( + "delegation token audience does not match this host".into(), + )); } Ok(claims) @@ -448,7 +466,7 @@ let verifying_key = K256VerifyingKey::from(&signing_key); let claims = make_delegation_claims(); let token = sign_delegation_token(&claims, &signing_key).unwrap(); - let verified = verify_delegation_token(&token, &verifying_key).unwrap(); + let verified = verify_delegation_token(&token, &verifying_key, &claims.aud).unwrap(); assert_eq!(verified.iss, claims.iss); assert_eq!(verified.sub, claims.sub); @@ -464,8 +482,21 @@ let verifying_key = K256VerifyingKey::from(&other_key); let claims = make_delegation_claims(); let token = sign_delegation_token(&claims, &signing_key).unwrap(); - let result = verify_delegation_token(&token, &verifying_key); + let result = verify_delegation_token(&token, &verifying_key, &claims.aud); + assert!(result.is_err()); + } + + #[test] + fn delegation_rejects_wrong_aud() { + let signing_key = make_k256_signing_key(); + let verifying_key = K256VerifyingKey::from(&signing_key); + let claims = make_delegation_claims(); + + let token = sign_delegation_token(&claims, &signing_key).unwrap(); + let result = + verify_delegation_token(&token, &verifying_key, "did:plc:wrong#atproto_space_host"); assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("audience")); } #[test] @@ -486,7 +517,7 @@ jti: make_jti(), }; let token = sign_delegation_token(&claims, &signing_key).unwrap(); - let result = verify_delegation_token(&token, &verifying_key); + let result = verify_delegation_token(&token, &verifying_key, &claims.aud); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("expired")); } @@ -510,7 +541,7 @@ payload_b64, URL_SAFE_NO_PAD.encode(sig.to_bytes()) ); - let result = verify_delegation_token(&token, &verifying_key); + let result = verify_delegation_token(&token, &verifying_key, &claims.aud); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("typ")); } diff --git a/src/spaces/integration_tests.rs b/src/spaces/integration_tests.rs --- a/src/spaces/integration_tests.rs +++ b/src/spaces/integration_tests.rs @@ -150,7 +150,7 @@ // Peek must return the correct typ before verification assert_eq!(peek_jwt_typ(&token).as_deref(), Some(DELEGATION_TOKEN_TYP)); // Step 2: space host verifies delegation token - let verified_delegation = verify_delegation_token(&token, &vk).unwrap(); + let verified_delegation = verify_delegation_token(&token, &vk, &delegation.aud).unwrap(); assert_eq!(verified_delegation.iss, "did:plc:member"); assert_eq!( verified_delegation.sub, @@ -203,7 +203,7 @@ exp: now - 60, // already expired jti: make_jti(), }; let token = sign_delegation_token(&delegation, &sk).unwrap(); - let result = verify_delegation_token(&token, &vk); + let result = verify_delegation_token(&token, &vk, &delegation.aud); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("expired")); } diff --git a/src/spaces/routes.rs b/src/spaces/routes.rs --- a/src/spaces/routes.rs +++ b/src/spaces/routes.rs @@ -397,7 +397,7 @@ if row.is_some_and(|(is_super,)| is_super != 0) { return Ok(()); } Err(AppError::Forbidden( - "Only the space owner can perform this action".into(), + "Only the space authority can perform this action".into(), )) } @@ -450,6 +450,22 @@ fn content_cid(record: &serde_json::Value) -> String { let bytes = serde_json::to_vec(record).unwrap_or_default(); let hash = Sha256::digest(&bytes); format!("bafyrei{}", hex::encode(&hash[..20])) +} + +async fn resolve_client_id_url( + state: &AppState, + client_key: &str, +) -> Result, AppError> { + let sql = adapt_sql( + "SELECT client_id_url FROM happyview_api_clients WHERE client_key = ?", + state.db_backend, + ); + let row: Option<(String,)> = sqlx::query_as(&sql) + .bind(client_key) + .fetch_optional(&state.db) + .await + .map_err(|e| AppError::Internal(format!("failed to look up API client: {e}")))?; + Ok(row.map(|(url,)| url)) } // --------------------------------------------------------------------------- @@ -1337,12 +1353,27 @@ })?; k256::ecdsa::VerifyingKey::from(&signing_key) }; - let delegation_claims = - crate::spaces::credential::verify_delegation_token(&input.grant, &verifying_key)?; + let delegation_claims = { + let unverified_sub = crate::spaces::credential::peek_delegation_sub(&input.grant) + .ok_or_else(|| AppError::Auth("invalid delegation token".into()))?; + let space_did = crate::spaces::SpaceUri::parse(&unverified_sub) + .map(|u| u.did.clone()) + .unwrap_or_default(); + let expected_aud = format!("{space_did}#atproto_space_host"); + crate::spaces::credential::verify_delegation_token( + &input.grant, + &verifying_key, + &expected_aud, + )? + }; let space = resolve_space(&state, &delegation_claims.sub).await?; - let client_id = claims.client_key().map(|k| k.to_string()); + let client_id = if let Some(key) = claims.client_key() { + resolve_client_id_url(&state, key).await? + } else { + None + }; let issued = crate::spaces::auth::issue_credential( &state.db, state.db_backend, diff --git a/src/spaces/simplespace.rs b/src/spaces/simplespace.rs --- a/src/spaces/simplespace.rs +++ b/src/spaces/simplespace.rs @@ -160,29 +160,6 @@ .as_ref() .ok_or_else(|| AppError::Auth("This endpoint requires authentication".into())) } -async fn require_auth_or_credential( - state: &AppState, - claims: &XrpcClaims, -) -> Result { - if let Some(identity) = &claims.identity { - return Ok(identity.did().to_string()); - } - - if let Some(token) = &claims.space_credential { - let verified = crate::spaces::credential::verify_external_credential( - token, - &state.http, - &state.config.plc_url, - ) - .await?; - return Ok(verified.sub); - } - - 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( @@ -214,7 +191,7 @@ if row.is_some_and(|(is_super,)| is_super != 0) { return Ok(()); } Err(AppError::Forbidden( - "Only the space owner can perform this action".into(), + "Only the space authority can perform this action".into(), )) } @@ -380,8 +357,9 @@ ) -> Result, AppError> { let space = resolve_space(&state, &query.space).await?; if !space.config.membership_public { - let did = require_auth_or_credential(&state, &xrpc_claims).await?; - let member = members::is_member(&state.db, state.db_backend, &space.id, &did).await?; + let claims = require_auth(&xrpc_claims)?; + let member = + members::is_member(&state.db, state.db_backend, &space.id, claims.did()).await?; member.ok_or_else(|| AppError::Forbidden("You are not a member of this space".into()))?; } diff --git a/tests/e2e_feature_flags.rs b/tests/e2e_feature_flags.rs --- a/tests/e2e_feature_flags.rs +++ b/tests/e2e_feature_flags.rs @@ -62,7 +62,7 @@ .router .clone() .oneshot( Request::builder() - .uri("/xrpc/dev.happyview.space.list") + .uri("/xrpc/com.atproto.space.listSpaces") .body(Body::empty()) .unwrap(), ) @@ -99,7 +99,7 @@ .router .clone() .oneshot( Request::builder() - .uri("/xrpc/dev.happyview.space.list") + .uri("/xrpc/com.atproto.space.listSpaces") .body(Body::empty()) .unwrap(), ) @@ -151,7 +151,7 @@ .router .clone() .oneshot( Request::builder() - .uri("/xrpc/dev.happyview.space.list") + .uri("/xrpc/com.atproto.space.listSpaces") .body(Body::empty()) .unwrap(), ) -- tangled.sh