malpercio.dev / ezpds
An easy-to-host PDS on the ATProtocol, MacOS. Grandma-approved.
fork atom

fix(relay): address OAuth server metadata PR review

Critical:
- Add scopes_supported: ["atproto", "transition:generic"] per oauth-integration-spec.md §5.1
- Add public_url HTTPS validation to config.rs validate_and_build (RFC 8414 requires HTTPS issuer)

Important:
- Fix jwks_uri path: /oauth/jwks.json → /oauth/jwks (spec §5 table)
- Add private_key_jwt to token_endpoint_auth_methods_supported (spec §1.2 and §5.1)
- Fix db/oauth.rs FCIS annotation: Functional Core → Imperative Shell
- Remove ticket-reference language from db/oauth.rs comments
- Refactor trailing slash test: use ..base struct syntax, assert all four URL fields
- Add accessible_without_auth_headers test to lock in public access contract

Minor:
- Make OAuthServerMetadata struct private (no external callers)
- Add require_pushed_authorization_requests: true (PAR is mandatory per AT Protocol OAuth spec)
- Tighten pkce and response_types tests to exact-match (not .any()) to catch accidental additions
- Add par_is_required test

authored by malpercio.dev and committed by

Tangled 27e5f8f0 6637ca9e

+120 -40
+31
crates/common/src/config.rs
··· 286 286 let public_url = raw.public_url.ok_or(ConfigError::MissingField { 287 287 field: "public_url", 288 288 })?; 289 + if !public_url.starts_with("https://") { 290 + return Err(ConfigError::Invalid(format!( 291 + "public_url must start with https:// (RFC 8414 requires HTTPS for the OAuth issuer), got: {public_url:?}" 292 + ))); 293 + } 289 294 let available_user_domains = raw 290 295 .available_user_domains 291 296 .ok_or(ConfigError::MissingField { ··· 553 558 Some("https://example.com/tos") 554 559 ); 555 560 assert_eq!(config.contact.email.as_deref(), Some("admin@example.com")); 561 + } 562 + 563 + #[test] 564 + fn public_url_without_https_scheme_returns_error() { 565 + for bad_url in &[ 566 + "pds.example.com", 567 + "http://pds.example.com", 568 + "ftp://pds.example.com", 569 + "", 570 + ] { 571 + let raw = RawConfig { 572 + data_dir: Some("/var/pds".to_string()), 573 + public_url: Some(bad_url.to_string()), 574 + available_user_domains: Some(vec!["example.com".to_string()]), 575 + ..Default::default() 576 + }; 577 + let err = validate_and_build(raw).unwrap_err(); 578 + assert!( 579 + matches!(err, ConfigError::Invalid(_)), 580 + "expected Invalid error for public_url={bad_url:?}, got: {err}" 581 + ); 582 + assert!( 583 + err.to_string().contains("https://"), 584 + "error message should mention https:// for public_url={bad_url:?}" 585 + ); 586 + } 556 587 } 557 588 558 589 #[test]
+5 -5
crates/relay/src/db/oauth.rs
··· 1 - // pattern: Functional Core (pure data access — no business logic) 1 + // pattern: Imperative Shell 2 2 // 3 3 // Storage adapter for OAuth server-side state in the `oauth_clients` table. 4 - // Future tickets add authorization code and token functions as the full OAuth 4 + // Authorization code and token functions will be added when the full OAuth 5 5 // flow is implemented. 6 6 7 7 use sqlx::SqlitePool; ··· 10 10 /// 11 11 /// `client_metadata` is stored as a raw JSON string (RFC 7591 client metadata). 12 12 /// Callers are responsible for serializing/deserializing the JSON. 13 - // Not yet wired to a handler — will be used when the OAuth authorization flow is implemented. 13 + // Wired to handlers when the OAuth authorization flow is implemented. 14 14 #[allow(dead_code)] 15 15 pub struct OAuthClientRow { 16 16 pub client_id: String, ··· 25 25 /// 26 26 /// Returns `sqlx::Error` on failure. Callers should use `crate::db::is_unique_violation` 27 27 /// to detect duplicate `client_id` conflicts. 28 - // Not yet wired to a handler — will be used when the OAuth authorization flow is implemented. 28 + // Wired to handlers when the OAuth authorization flow is implemented. 29 29 #[allow(dead_code)] 30 30 pub async fn register_oauth_client( 31 31 pool: &SqlitePool, ··· 44 44 } 45 45 46 46 /// Look up a registered OAuth client by `client_id`. Returns `None` if not found. 47 - // Not yet wired to a handler — will be used when the OAuth authorization flow is implemented. 47 + // Wired to handlers when the OAuth authorization flow is implemented. 48 48 #[allow(dead_code)] 49 49 pub async fn get_oauth_client( 50 50 pool: &SqlitePool,
+84 -35
crates/relay/src/routes/oauth_server_metadata.rs
··· 18 18 /// camelCase used by XRPC/AT Protocol Lexicon endpoints in this codebase. 19 19 /// 20 20 /// AT Protocol OAuth extensions: 21 + /// - `scopes_supported`: the AT Protocol scopes this server recognises. 21 22 /// - `dpop_signing_alg_values_supported`: signals that DPoP (RFC 9449) is required. 22 - /// - `token_endpoint_auth_methods_supported: ["none"]`: public clients only — no client secrets. 23 + /// - `token_endpoint_auth_methods_supported`: public clients + private_key_jwt per spec §1.2. 24 + /// - `require_pushed_authorization_requests`: PAR is mandatory per AT Protocol OAuth spec. 23 25 #[derive(Serialize)] 24 - pub struct OAuthServerMetadata { 26 + struct OAuthServerMetadata { 25 27 issuer: String, 26 28 authorization_endpoint: String, 27 29 token_endpoint: String, 28 30 pushed_authorization_request_endpoint: String, 29 31 jwks_uri: String, 32 + scopes_supported: Vec<String>, 30 33 response_types_supported: Vec<String>, 31 34 grant_types_supported: Vec<String>, 35 + token_endpoint_auth_methods_supported: Vec<String>, 32 36 code_challenge_methods_supported: Vec<String>, 33 37 dpop_signing_alg_values_supported: Vec<String>, 34 - token_endpoint_auth_methods_supported: Vec<String>, 38 + require_pushed_authorization_requests: bool, 35 39 } 36 40 37 41 pub async fn oauth_server_metadata(State(state): State<AppState>) -> impl IntoResponse { ··· 41 45 authorization_endpoint: format!("{base}/oauth/authorize"), 42 46 token_endpoint: format!("{base}/oauth/token"), 43 47 pushed_authorization_request_endpoint: format!("{base}/oauth/par"), 44 - jwks_uri: format!("{base}/oauth/jwks.json"), 48 + jwks_uri: format!("{base}/oauth/jwks"), 49 + scopes_supported: vec!["atproto".to_string(), "transition:generic".to_string()], 45 50 response_types_supported: vec!["code".to_string()], 46 51 grant_types_supported: vec![ 47 52 "authorization_code".to_string(), 48 53 "refresh_token".to_string(), 49 54 ], 55 + token_endpoint_auth_methods_supported: vec![ 56 + "none".to_string(), 57 + "private_key_jwt".to_string(), 58 + ], 50 59 code_challenge_methods_supported: vec!["S256".to_string()], 51 60 dpop_signing_alg_values_supported: vec!["ES256".to_string()], 52 - token_endpoint_auth_methods_supported: vec!["none".to_string()], 61 + require_pushed_authorization_requests: true, 53 62 }) 54 63 } 55 64 56 65 #[cfg(test)] 57 66 mod tests { 67 + use std::sync::Arc; 68 + 58 69 use axum::{ 59 70 body::Body, 60 71 http::{Request, StatusCode}, 61 72 }; 62 73 use tower::ServiceExt; 63 74 64 - use crate::app::{app, test_state}; 75 + use crate::app::{app, test_state, AppState}; 65 76 66 77 async fn metadata_json() -> serde_json::Value { 67 78 let response = app(test_state().await) ··· 100 111 } 101 112 102 113 #[tokio::test] 114 + async fn accessible_without_auth_headers() { 115 + // Lock in that the discovery endpoint requires no credentials. 116 + // A future global auth middleware must not inadvertently protect this route. 117 + let response = app(test_state().await) 118 + .oneshot( 119 + Request::builder() 120 + .uri("/.well-known/oauth-authorization-server") 121 + .body(Body::empty()) 122 + .unwrap(), 123 + ) 124 + .await 125 + .unwrap(); 126 + assert_eq!(response.status(), StatusCode::OK); 127 + } 128 + 129 + #[tokio::test] 103 130 async fn issuer_matches_public_url() { 104 131 let json = metadata_json().await; 105 132 assert_eq!(json["issuer"], "https://test.example.com"); ··· 120 147 json["pushed_authorization_request_endpoint"], 121 148 "https://test.example.com/oauth/par" 122 149 ); 123 - assert_eq!(json["jwks_uri"], "https://test.example.com/oauth/jwks.json"); 150 + assert_eq!(json["jwks_uri"], "https://test.example.com/oauth/jwks"); 124 151 } 125 152 126 153 #[tokio::test] 127 - async fn response_types_contains_code() { 154 + async fn scopes_supported_are_atproto_scopes() { 128 155 let json = metadata_json().await; 129 - assert!(json["response_types_supported"] 130 - .as_array() 131 - .unwrap() 132 - .iter() 133 - .any(|v| v == "code")); 156 + assert_eq!( 157 + json["scopes_supported"], 158 + serde_json::json!(["atproto", "transition:generic"]) 159 + ); 160 + } 161 + 162 + #[tokio::test] 163 + async fn response_types_is_exactly_code() { 164 + let json = metadata_json().await; 165 + assert_eq!( 166 + json["response_types_supported"], 167 + serde_json::json!(["code"]) 168 + ); 134 169 } 135 170 136 171 #[tokio::test] ··· 142 177 } 143 178 144 179 #[tokio::test] 145 - async fn dpop_signing_alg_includes_es256() { 180 + async fn token_endpoint_auth_methods_are_none_and_private_key_jwt() { 146 181 let json = metadata_json().await; 147 - assert!(json["dpop_signing_alg_values_supported"] 182 + let methods: Vec<&str> = json["token_endpoint_auth_methods_supported"] 148 183 .as_array() 149 184 .unwrap() 150 185 .iter() 151 - .any(|v| v == "ES256")); 186 + .map(|v| v.as_str().unwrap()) 187 + .collect(); 188 + assert!(methods.contains(&"none"), "must support public clients"); 189 + assert!( 190 + methods.contains(&"private_key_jwt"), 191 + "must support private_key_jwt per AT Protocol OAuth spec §1.2" 192 + ); 152 193 } 153 194 154 195 #[tokio::test] 155 - async fn token_endpoint_auth_method_is_none() { 196 + async fn dpop_signing_alg_includes_es256() { 156 197 let json = metadata_json().await; 157 - assert!(json["token_endpoint_auth_methods_supported"] 198 + assert!(json["dpop_signing_alg_values_supported"] 158 199 .as_array() 159 200 .unwrap() 160 201 .iter() 161 - .any(|v| v == "none")); 202 + .any(|v| v == "ES256")); 203 + } 204 + 205 + #[tokio::test] 206 + async fn pkce_method_is_exactly_s256() { 207 + // AT Protocol OAuth prohibits plain — assert the exact set, not just contains. 208 + let json = metadata_json().await; 209 + assert_eq!( 210 + json["code_challenge_methods_supported"], 211 + serde_json::json!(["S256"]) 212 + ); 162 213 } 163 214 164 215 #[tokio::test] 165 - async fn pkce_method_is_s256() { 216 + async fn par_is_required() { 166 217 let json = metadata_json().await; 167 - assert!(json["code_challenge_methods_supported"] 168 - .as_array() 169 - .unwrap() 170 - .iter() 171 - .any(|v| v == "S256")); 218 + assert_eq!(json["require_pushed_authorization_requests"], true); 172 219 } 173 220 174 221 #[tokio::test] 175 222 async fn trailing_slash_in_public_url_does_not_double_slash_endpoints() { 176 - use crate::app::AppState; 177 - use std::sync::Arc; 178 - 179 223 let base = test_state().await; 180 224 let mut config = (*base.config).clone(); 181 225 config.public_url = "https://pds.example.com/".to_string(); 182 226 let state = AppState { 183 227 config: Arc::new(config), 184 - db: base.db, 185 - http_client: base.http_client, 186 - dns_provider: base.dns_provider, 187 - txt_resolver: base.txt_resolver, 188 - well_known_resolver: base.well_known_resolver, 189 - jwt_secret: base.jwt_secret, 228 + ..base 190 229 }; 191 230 192 231 let response = app(state) ··· 204 243 .unwrap(); 205 244 let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); 206 245 207 - // public_url with trailing slash must not produce "...com//oauth/..." 246 + // All four URL-bearing fields must not produce "...com//oauth/..." when 247 + // public_url has a trailing slash. 208 248 assert_eq!( 209 249 json["authorization_endpoint"], 210 250 "https://pds.example.com/oauth/authorize" 211 251 ); 252 + assert_eq!( 253 + json["token_endpoint"], 254 + "https://pds.example.com/oauth/token" 255 + ); 256 + assert_eq!( 257 + json["pushed_authorization_request_endpoint"], 258 + "https://pds.example.com/oauth/par" 259 + ); 260 + assert_eq!(json["jwks_uri"], "https://pds.example.com/oauth/jwks"); 212 261 } 213 262 }