diff --git a/loomem-server/src/oauth.rs b/loomem-server/src/oauth.rs index c419ddd..a8ab0d6 100644 --- a/loomem-server/src/oauth.rs +++ b/loomem-server/src/oauth.rs @@ -65,6 +65,11 @@ pub struct OAuthClient { /// exchange). Drives LRU eviction at capacity, so a still-active connector /// is not dropped in favor of a fresh flood entry. pub last_used: std::time::Instant, + /// RFC 7591 `application_type` ("web" or "native") as registered + /// (SEP-837). Recorded so redirect policy can differentiate native + /// clients (RFC 8252 loopback redirects) without a re-registration. + #[allow(dead_code)] // stored at registration; read once native-redirect policy lands + pub application_type: String, } /// Pending authorization: after user submits API key on /oauth/authorize. @@ -281,6 +286,8 @@ pub struct RegisterRequest { pub grant_types: Option>, pub response_types: Option>, pub token_endpoint_auth_method: Option, + /// RFC 7591 §2 / SEP-837: "web" (default) or "native". + pub application_type: Option, } #[derive(Serialize)] @@ -292,6 +299,7 @@ struct RegisterResponse { grant_types: Vec, response_types: Vec, token_endpoint_auth_method: String, + application_type: String, } #[derive(Deserialize)] @@ -359,6 +367,9 @@ pub async fn authorization_server_metadata( "token_endpoint_auth_methods_supported": ["client_secret_post", "none"], "code_challenge_methods_supported": ["S256"], "scopes_supported": ["mcp"], + // RFC 9207 (SEP-2468): authorization responses carry `iss`; + // advertising it here lets clients enforce mix-up protection. + "authorization_response_iss_parameter_supported": true, })) } @@ -427,6 +438,21 @@ pub async fn register( // client told it is confidential actually has its secret enforced at /token. let confidential = auth_method != "none"; + // SEP-837 / RFC 7591: accept and record `application_type`. Only the two + // registered values exist; reject anything else up front (same posture as + // the auth-method check above) instead of storing metadata we can't honor. + let application_type = req.application_type.as_deref().unwrap_or("web"); + if application_type != "web" && application_type != "native" { + return ( + StatusCode::BAD_REQUEST, + Json(json!({ + "error": "invalid_client_metadata", + "error_description": "unsupported application_type; use web or native", + })), + ) + .into_response(); + } + let client_id = Uuid::new_v4().to_string(); let client_secret = Uuid::new_v4().to_string(); @@ -435,6 +461,7 @@ pub async fn register( client_secret: Some(client_secret.clone()), redirect_uris: redirect_uris.clone(), confidential, + application_type: application_type.to_string(), last_used: std::time::Instant::now(), }; @@ -487,11 +514,74 @@ pub async fn register( .unwrap_or_else(|| vec!["authorization_code".into()]), response_types: req.response_types.unwrap_or_else(|| vec!["code".into()]), token_endpoint_auth_method: auth_method.to_string(), + application_type: application_type.to_string(), }), ) .into_response() } +/// Percent-decode one query component (used on the name position). Invalid +/// escape sequences pass through unchanged, matching lenient client parsers; +/// non-UTF-8 decodes lossily, which can never spell the pure-ASCII "iss". +fn percent_decode(component: &str) -> String { + fn hex_val(c: u8) -> Option { + match c { + b'0'..=b'9' => Some(c - b'0'), + b'a'..=b'f' => Some(c - b'a' + 10), + b'A'..=b'F' => Some(c - b'A' + 10), + _ => None, + } + } + let bytes = component.as_bytes(); + let mut out = Vec::with_capacity(bytes.len()); + let mut i = 0; + while i < bytes.len() { + if bytes[i] == b'%' { + if let (Some(&hi), Some(&lo)) = (bytes.get(i + 1), bytes.get(i + 2)) { + if let (Some(h), Some(l)) = (hex_val(hi), hex_val(lo)) { + out.push((h << 4) | l); + i += 3; + continue; + } + } + } + out.push(bytes[i]); + i += 1; + } + String::from_utf8_lossy(&out).into_owned() +} + +/// RFC 6749 §3.1.2 forbids fragments in the redirection endpoint URI — and a +/// fragment would swallow every appended response parameter (`code`, `state`, +/// `iss`) into the client-side fragment, so the callback request would never +/// see them (Greptile #62, verified repro). A raw `#` is the only fragment +/// delimiter: a percent-encoded `%23` stays an ordinary encoded octet in the +/// Location header and opens no fragment. +fn redirect_has_fragment(redirect_uri: &str) -> bool { + redirect_uri.contains('#') +} + +/// RFC 9207 hardening: a redirect URI that pre-embeds the reserved `iss` +/// response parameter would make the final redirect carry two `iss` values +/// (the embedded one and the one this server appends), so a compliant client +/// could reject the response — or a naive one could validate the wrong +/// issuer. Such URIs are refused at authorize time, inline like every other +/// authorize-time rejection. Parameter names are compared after +/// percent-decoding, because the client's query parser decodes them too — a +/// raw `%69ss` in the registered URI reaches that parser as a second `iss` +/// (Greptile #62, verified repro). +fn redirect_embeds_iss(redirect_uri: &str) -> bool { + redirect_uri + .split_once('?') + .map(|(_, query)| { + query.split('&').any(|pair| { + let name = pair.split('=').next().unwrap_or(""); + percent_decode(name) == "iss" + }) + }) + .unwrap_or(false) +} + /// GET /oauth/authorize — shows a minimal login page where user enters API key. pub async fn authorize_page( State(state): State>, @@ -514,6 +604,12 @@ pub async fn authorize_page( "PKCE required: send a code_challenge with code_challenge_method=S256", ); } + if redirect_has_fragment(&q.redirect_uri) { + return authorize_error("redirect_uri must not contain a fragment component"); + } + if redirect_embeds_iss(&q.redirect_uri) { + return authorize_error("redirect_uri must not embed the reserved iss response parameter"); + } authorize_page_html(q).into_response() } @@ -610,6 +706,12 @@ pub async fn authorize_submit( "PKCE required: send a code_challenge with code_challenge_method=S256", ); } + if redirect_has_fragment(&form.redirect_uri) { + return authorize_error("redirect_uri must not contain a fragment component"); + } + if redirect_embeds_iss(&form.redirect_uri) { + return authorize_error("redirect_uri must not embed the reserved iss response parameter"); + } let code = Uuid::new_v4().to_string(); @@ -637,7 +739,7 @@ pub async fn authorize_submit( .await .insert(code.clone(), pending); - // Build redirect URL with code + state + // Build redirect URL with code + state + iss let mut redirect_url = form.redirect_uri; let separator = if redirect_url.contains('?') { '&' } else { '?' }; redirect_url = format!("{}{}code={}", redirect_url, separator, urlencoding(&code)); @@ -646,6 +748,11 @@ pub async fn authorize_submit( redirect_url = format!("{}&state={}", redirect_url, urlencoding(&s)); } } + // RFC 9207 (SEP-2468): identify the issuer in the authorization response + // so the client can detect an authorization-server mix-up before redeeming + // the code. Mirrors `issuer` from the AS metadata, where + // `authorization_response_iss_parameter_supported` advertises this. + redirect_url = format!("{}&iss={}", redirect_url, urlencoding(&state.server_origin)); Redirect::to(&redirect_url).into_response() } @@ -925,6 +1032,7 @@ mod tests { client_secret: Some("shhh".to_string()), redirect_uris: vec![redirect.to_string()], confidential, + application_type: "web".to_string(), last_used: std::time::Instant::now(), }, ); @@ -1078,6 +1186,7 @@ mod tests { grant_types: None, response_types: None, token_endpoint_auth_method: Some("none".to_string()), + application_type: None, }), ) .await @@ -1100,6 +1209,7 @@ mod tests { grant_types: None, response_types: None, token_endpoint_auth_method: Some("client_secret_basic".to_string()), + application_type: None, }), ) .await @@ -1128,6 +1238,7 @@ mod tests { client_secret: Some("s".to_string()), redirect_uris: vec!["https://c/cb".to_string()], confidential: false, + application_type: "web".to_string(), last_used, }, ); @@ -1143,6 +1254,7 @@ mod tests { grant_types: None, response_types: None, token_endpoint_auth_method: Some("none".to_string()), + application_type: None, } } @@ -1318,6 +1430,7 @@ mod tests { client_secret: Some("s".to_string()), redirect_uris: vec!["https://c1/cb".to_string()], confidential: false, + application_type: "web".to_string(), last_used: stale, }, ); @@ -1355,6 +1468,7 @@ mod tests { client_secret: Some("topsecret".to_string()), redirect_uris: vec!["https://client.example/cb".to_string()], confidential: true, + application_type: "web".to_string(), last_used: std::time::Instant::now(), }, ); @@ -1379,4 +1493,197 @@ mod tests { .into_response(); assert_eq!(resp.status(), StatusCode::OK); } + + // RFC 9207 (SEP-2468): the success redirect carries `iss` identifying the + // issuer so the client can detect an authorization-server mix-up before + // redeeming the code. + #[tokio::test] + async fn authorize_redirect_includes_iss() { + let state = Arc::new(OAuthState::new("https://memory.example.com".to_string())); + seed_client(&state, "c1", "https://client.example/cb", false).await; + let resp = authorize_submit( + State(state.clone()), + axum::Form(AuthorizeSubmit { + client_id: "c1".to_string(), + redirect_uri: "https://client.example/cb".to_string(), + state: Some("xyz".to_string()), + api_key: "USER_KEY".to_string(), + code_challenge: Some("abc".to_string()), + code_challenge_method: Some("S256".to_string()), + }), + ) + .await + .into_response(); + assert!( + resp.status().is_redirection(), + "expected redirect, got {}", + resp.status() + ); + let location = resp + .headers() + .get(axum::http::header::LOCATION) + .and_then(|v| v.to_str().ok()) + .expect("Location header") + .to_string(); + assert!(location.starts_with("https://client.example/cb?code=")); + assert!(location.contains("&state=xyz")); + assert!( + location.contains("&iss=https%3A%2F%2Fmemory.example.com"), + "iss must identify the issuer, got: {location}" + ); + } + + // SEP-2468: AS metadata advertises RFC 9207 support so clients know to + // expect and validate the `iss` parameter. + #[tokio::test] + async fn metadata_advertises_iss_support() { + let state = Arc::new(OAuthState::new("https://memory.example.com".to_string())); + let resp = authorization_server_metadata(State(state)) + .await + .into_response(); + assert_eq!(resp.status(), StatusCode::OK); + let bytes = axum::body::to_bytes(resp.into_body(), 64 * 1024) + .await + .expect("metadata body"); + let v: serde_json::Value = serde_json::from_slice(&bytes).expect("metadata json"); + assert_eq!(v["authorization_response_iss_parameter_supported"], true); + assert_eq!(v["issuer"], "https://memory.example.com"); + } + + // SEP-837: `application_type` is accepted, recorded on the client and + // echoed in the registration response. + #[tokio::test] + async fn register_stores_and_echoes_application_type() { + let state = Arc::new(OAuthState::new("https://memory.example.com".to_string())); + let mut req = public_register_req(); + req.application_type = Some("native".to_string()); + let resp = register( + test_conn(), + State(state.clone()), + HeaderMap::new(), + Json(req), + ) + .await + .into_response(); + assert_eq!(resp.status(), StatusCode::CREATED); + let bytes = axum::body::to_bytes(resp.into_body(), 64 * 1024) + .await + .expect("register body"); + let v: serde_json::Value = serde_json::from_slice(&bytes).expect("register json"); + assert_eq!(v["application_type"], "native"); + let client_id = v["client_id"].as_str().expect("client_id").to_string(); + let clients = state.clients.read().await; + let stored = clients.get(&client_id).expect("stored client"); + assert_eq!(stored.application_type, "native"); + } + + // An `application_type` outside RFC 7591's two registered values is + // rejected at registration, mirroring the unsupported-auth-method posture. + #[tokio::test] + async fn register_rejects_unknown_application_type() { + let state = Arc::new(OAuthState::new("https://memory.example.com".to_string())); + let mut req = public_register_req(); + req.application_type = Some("spa".to_string()); + let resp = register(test_conn(), State(state), HeaderMap::new(), Json(req)) + .await + .into_response(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + } + + // Greptile #62 P1: a redirect URI that pre-embeds `iss` would produce a + // duplicate issuer parameter in the final redirect — refuse it inline. + #[test] + fn redirect_embeds_iss_detection() { + assert!(redirect_embeds_iss("https://c/cb?iss=x")); + assert!(redirect_embeds_iss("https://c/cb?a=b&iss=x")); + assert!(redirect_embeds_iss("https://c/cb?iss")); + // Percent-encoded names decode to `iss` in the client's parser and + // must be caught too (Greptile #62, verified repro). + assert!(redirect_embeds_iss("https://c/cb?%69ss=x")); + assert!(redirect_embeds_iss("https://c/cb?i%73s=x")); + assert!(redirect_embeds_iss("https://c/cb?a=b&%69ss=x")); + assert!(!redirect_embeds_iss("https://c/cb")); + assert!(!redirect_embeds_iss("https://c/cb?misses=x")); + assert!(!redirect_embeds_iss("https://c/cb?a=iss")); + // One decode only: a double-encoded name reaches the client's parser + // as `%69ss`, not `iss`. + assert!(!redirect_embeds_iss("https://c/cb?%2569ss=x")); + // Parameter names are case-sensitive; ISS is not iss. + assert!(!redirect_embeds_iss("https://c/cb?ISS=x")); + } + + // RFC 6749 §3.1.2: fragments are forbidden in the redirection endpoint — + // and would swallow code/state/iss into the client-side fragment + // (Greptile #62, verified repro). + #[test] + fn redirect_fragment_detection() { + assert!(redirect_has_fragment("https://c/cb#frag")); + assert!(redirect_has_fragment("https://c/cb?a=b#frag")); + assert!(!redirect_has_fragment("https://c/cb")); + assert!(!redirect_has_fragment("https://c/cb?a=b")); + // Percent-encoded %23 is an ordinary octet, not a fragment delimiter. + assert!(!redirect_has_fragment("https://c/cb?next=%23section")); + } + + #[tokio::test] + async fn authorize_rejects_redirect_uri_with_fragment() { + let state = Arc::new(OAuthState::new("https://memory.example.com".to_string())); + seed_client( + &state, + "c1", + "https://client.example/cb#client-state", + false, + ) + .await; + let resp = authorize_submit( + State(state.clone()), + axum::Form(AuthorizeSubmit { + client_id: "c1".to_string(), + redirect_uri: "https://client.example/cb#client-state".to_string(), + state: None, + api_key: "USER_KEY".to_string(), + code_challenge: Some("abc".to_string()), + code_challenge_method: Some("S256".to_string()), + }), + ) + .await + .into_response(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + } + + #[test] + fn percent_decode_handles_escapes_and_garbage() { + assert_eq!(percent_decode("%69ss"), "iss"); + assert_eq!(percent_decode("plain"), "plain"); + assert_eq!(percent_decode("%2569ss"), "%69ss"); + // Truncated/invalid escapes pass through unchanged. + assert_eq!(percent_decode("%6"), "%6"); + assert_eq!(percent_decode("%zz"), "%zz"); + } + + #[tokio::test] + async fn authorize_rejects_redirect_uri_with_embedded_iss() { + let state = Arc::new(OAuthState::new("https://memory.example.com".to_string())); + seed_client( + &state, + "c1", + "https://client.example/cb?iss=https://evil.example", + false, + ) + .await; + let resp = authorize_submit( + State(state.clone()), + axum::Form(AuthorizeSubmit { + client_id: "c1".to_string(), + redirect_uri: "https://client.example/cb?iss=https://evil.example".to_string(), + state: None, + api_key: "USER_KEY".to_string(), + code_challenge: Some("abc".to_string()), + code_challenge_method: Some("S256".to_string()), + }), + ) + .await + .into_response(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + } }