fix: address copilot feedback

trezy · trezy · commit 6d20b2c11b80 · 2026-06-27T01:26:35.000-05:00
Signed-off-by: Trezy &lt;tre@trezy.com&gt;
diff --git 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
@@ -74,7 +74,7 @@ When a script handles a space-scoped request, the `space` global is set to a tab
 | `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
@@ -32,6 +32,23 @@ pub(super) async fn create(
 ) -> Result<(StatusCode, Json<VerificationMethod>), 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
@@ -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 @@ fn set_space_context(lua: &Lua, space: Option<&SpaceContext>) -> LuaResult<()> {
             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 @@ mod tests {
             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
@@ -111,12 +111,20 @@ async fn check_user_access_with_managing_app(
 ) -> Result<bool, AppError> {
     // 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
@@ -25,6 +25,17 @@ pub fn peek_jwt_typ(token: &str) -> Option<String> {
     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<String> {
+    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<String> {
     let parts: Vec<&str> = token.split('.').collect();
@@ -69,6 +80,7 @@ pub fn sign_delegation_token(
 pub fn verify_delegation_token(
     token: &str,
     verifying_key: &K256VerifyingKey,
+    expected_aud: &str,
 ) -> Result<DelegationTokenClaims, AppError> {
     let parts: Vec<&str> = token.split('.').collect();
     if parts.len() != 3 {
@@ -132,6 +144,12 @@ pub fn verify_delegation_token(
         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 @@ mod tests {
         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 @@ mod tests {
         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 @@ mod tests {
         };
 
         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 @@ mod tests {
             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
@@ -150,7 +150,7 @@ mod tests {
         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 @@ mod tests {
             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
@@ -397,7 +397,7 @@ async fn require_space_admin(state: &AppState, space: &Space, did: &str) -> Resu
         return Ok(());
     }
     Err(AppError::Forbidden(
-        "Only the space owner can perform this action".into(),
+        "Only the space authority can perform this action".into(),
     ))
 }
 
@@ -452,6 +452,22 @@ fn content_cid(record: &serde_json::Value) -> String {
     format!("bafyrei{}", hex::encode(&hash[..20]))
 }
 
+async fn resolve_client_id_url(
+    state: &AppState,
+    client_key: &str,
+) -> Result<Option<String>, 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))
+}
+
 // ---------------------------------------------------------------------------
 // Space read handlers
 // ---------------------------------------------------------------------------
@@ -1337,12 +1353,27 @@ async fn get_space_credential(
         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
@@ -160,29 +160,6 @@ fn require_auth(claims: &XrpcClaims) -> Result<&crate::auth::Claims, AppError> {
         .ok_or_else(|| AppError::Auth("This endpoint requires authentication".into()))
 }
 
-async fn require_auth_or_credential(
-    state: &AppState,
-    claims: &XrpcClaims,
-) -> Result<String, AppError> {
-    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<Space, AppError> {
     let uri = SpaceUri::parse(space_uri)?;
     db::get_space_by_address(
@@ -214,7 +191,7 @@ async fn require_space_admin(state: &AppState, space: &Space, did: &str) -> Resu
         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 @@ async fn list_members(
     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
@@ -62,7 +62,7 @@ async fn space_routes_blocked_when_flag_disabled() {
         .clone()
         .oneshot(
             Request::builder()
-                .uri("/xrpc/dev.happyview.space.list")
+                .uri("/xrpc/com.atproto.space.listSpaces")
                 .body(Body::empty())
                 .unwrap(),
         )
@@ -99,7 +99,7 @@ async fn space_routes_allowed_after_enabling_flag() {
         .clone()
         .oneshot(
             Request::builder()
-                .uri("/xrpc/dev.happyview.space.list")
+                .uri("/xrpc/com.atproto.space.listSpaces")
                 .body(Body::empty())
                 .unwrap(),
         )
@@ -151,7 +151,7 @@ async fn space_routes_blocked_again_after_disabling_flag() {
         .clone()
         .oneshot(
             Request::builder()
-                .uri("/xrpc/dev.happyview.space.list")
+                .uri("/xrpc/com.atproto.space.listSpaces")
                 .body(Body::empty())
                 .unwrap(),
         )
