From 7faec624d12618e09ea915753c87c1b123e418ed Mon Sep 17 00:00:00 2001 From: lewis Date: Mon, 05 Jan 2026 21:08:25 +0000 Subject: [PATCH] General linting, document react-native-streamplace-oauth-problem --- KNOWN_ISSUES.md | 23 +++++++++++++++++++++++ src/lib.rs | 4 ++++ src/util.rs | 36 +++++++++++++++++++++++++++++++++++- frontend/src/routes/Admin.svelte | 18 ------------------ frontend/src/routes/AppPasswords.svelte | 9 --------- frontend/src/routes/Comms.svelte | 6 ------ frontend/src/routes/Controllers.svelte | 19 ------------------- frontend/src/routes/DelegationAudit.svelte | 10 ---------- frontend/src/routes/DidDocumentEditor.svelte | 6 ------ frontend/src/routes/InviteCodes.svelte | 9 --------- frontend/src/routes/Migration.svelte | 18 ++++++++++-------- frontend/src/routes/OAuthConsent.svelte | 4 ++-- frontend/src/routes/Register.svelte | 6 ------ frontend/src/routes/RepoExplorer.svelte | 6 ------ frontend/src/routes/Security.svelte | 6 ------ frontend/src/routes/Settings.svelte | 3 +-- frontend/src/routes/TrustedDevices.svelte | 6 ------ frontend/src/styles/migration.css | 8 ++++++++ src/oauth/endpoints/authorize.rs | 193 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------- frontend/src/components/migration/ChooseHandleStep.svelte | 2 +- 20 file(s) changed, 214 insertion(s)(+), 178 deletion(s)(-) diff --git a/KNOWN_ISSUES.md b/KNOWN_ISSUES.md new file mode 100644 --- /dev/null +++ b/KNOWN_ISSUES.md @@ -0,0 +1,23 @@ +# Known Issues + +## stream.place iOS app OAuth flow fails + +OAuth flow with stream.place's iOS app (using expo-web-browser's ASWebAuthenticationSession) does not complete. After user approves consent, the redirect from our PDS to stream.place's callback URL is not followed by ASWebAuthenticationSession. + +What does work with stream.place: everything else :P +- Desktop browsers +- ios safari (regular browser) +- ASWebAuthenticationSession using the reference pds + +What fails: +- ASWebAuthenticationSession with this pds + +Attempted fixes (all failed): +- HTTP 302/303/307 redirects +- JavaScript navigation +- Meta refresh +- Form auto-submit +- Removing CORS headers +- HTTP/1.1 instead of HTTP/2 +- Minimal response headers + diff --git a/src/lib.rs b/src/lib.rs --- a/src/lib.rs +++ b/src/lib.rs @@ -550,6 +550,10 @@ .route("/authorize/deny", post(oauth::endpoints::authorize_deny)) .route("/authorize/consent", get(oauth::endpoints::consent_get)) .route("/authorize/consent", post(oauth::endpoints::consent_post)) + .route( + "/authorize/redirect", + get(oauth::endpoints::authorize_redirect), + ) .route("/delegation/auth", post(oauth::endpoints::delegation_auth)) .route( "/delegation/totp", diff --git a/src/util.rs b/src/util.rs --- a/src/util.rs +++ b/src/util.rs @@ -154,7 +154,16 @@ } pub fn build_full_url(path: &str) -> String { - format!("{}{}", pds_public_url(), path) + let normalized_path = if !path.starts_with("/xrpc/") + && (path.starts_with("/com.atproto.") + || path.starts_with("/app.bsky.") + || path.starts_with("/_")) + { + format!("/xrpc{}", path) + } else { + path.to_string() + }; + format!("{}{}", pds_public_url(), normalized_path) } pub fn json_to_ipld(value: &JsonValue) -> Ipld { @@ -354,5 +363,30 @@ return; } panic!("Failed to find CID link in parsed CBOR"); + } + + #[test] + fn test_build_full_url_adds_xrpc_prefix_for_atproto_paths() { + unsafe { std::env::set_var("PDS_HOSTNAME", "example.com") }; + assert_eq!( + build_full_url("/com.atproto.server.getSession"), + "https://example.com/xrpc/com.atproto.server.getSession" + ); + assert_eq!( + build_full_url("/app.bsky.feed.getTimeline"), + "https://example.com/xrpc/app.bsky.feed.getTimeline" + ); + assert_eq!( + build_full_url("/_health"), + "https://example.com/xrpc/_health" + ); + assert_eq!( + build_full_url("/xrpc/com.atproto.server.getSession"), + "https://example.com/xrpc/com.atproto.server.getSession" + ); + assert_eq!( + build_full_url("/oauth/token"), + "https://example.com/oauth/token" + ); } } diff --git a/frontend/src/routes/Admin.svelte b/frontend/src/routes/Admin.svelte --- a/frontend/src/routes/Admin.svelte +++ b/frontend/src/routes/Admin.svelte @@ -674,24 +674,6 @@ padding: var(--space-7); } - .message { - padding: var(--space-3); - border-radius: var(--radius-md); - margin-bottom: var(--space-4); - } - - .message.error { - background: var(--error-bg); - border: 1px solid var(--error-border); - color: var(--error-text); - } - - .message.success { - background: var(--success-bg); - border: 1px solid var(--success-border); - color: var(--success-text); - } - .config-form { max-width: 500px; } diff --git a/frontend/src/routes/AppPasswords.svelte b/frontend/src/routes/AppPasswords.svelte --- a/frontend/src/routes/AppPasswords.svelte +++ b/frontend/src/routes/AppPasswords.svelte @@ -234,15 +234,6 @@ margin-bottom: var(--space-7); } - .error { - padding: var(--space-3); - background: var(--error-bg); - border: 1px solid var(--error-border); - border-radius: var(--radius-md); - color: var(--error-text); - margin-bottom: var(--space-4); - } - .created-password { display: flex; flex-direction: column; diff --git a/frontend/src/routes/Comms.svelte b/frontend/src/routes/Comms.svelte --- a/frontend/src/routes/Comms.svelte +++ b/frontend/src/routes/Comms.svelte @@ -412,12 +412,6 @@ margin: var(--space-2) 0 0 0; } - .loading { - text-align: center; - color: var(--text-secondary); - padding: var(--space-7); - } - .split-layout { display: grid; grid-template-columns: 1fr; diff --git a/frontend/src/routes/Controllers.svelte b/frontend/src/routes/Controllers.svelte --- a/frontend/src/routes/Controllers.svelte +++ b/frontend/src/routes/Controllers.svelte @@ -453,29 +453,10 @@ margin: var(--space-2) 0 0 0; } - .loading, .empty { text-align: center; color: var(--text-secondary); padding: var(--space-4); - } - - .message { - padding: var(--space-3); - border-radius: var(--radius-md); - margin-bottom: var(--space-4); - } - - .message.error { - background: var(--error-bg); - border: 1px solid var(--error-border); - color: var(--error-text); - } - - .message.success { - background: var(--success-bg); - border: 1px solid var(--success-border); - color: var(--success-text); } .constraint-notice { diff --git a/frontend/src/routes/DelegationAudit.svelte b/frontend/src/routes/DelegationAudit.svelte --- a/frontend/src/routes/DelegationAudit.svelte +++ b/frontend/src/routes/DelegationAudit.svelte @@ -216,20 +216,10 @@ margin: var(--space-2) 0 0 0; } - .loading, .empty { text-align: center; color: var(--text-secondary); padding: var(--space-7); - } - - .message.error { - padding: var(--space-3); - background: var(--error-bg); - border: 1px solid var(--error-border); - border-radius: var(--radius-md); - color: var(--error-text); - margin-bottom: var(--space-4); } .audit-list { diff --git a/frontend/src/routes/DidDocumentEditor.svelte b/frontend/src/routes/DidDocumentEditor.svelte --- a/frontend/src/routes/DidDocumentEditor.svelte +++ b/frontend/src/routes/DidDocumentEditor.svelte @@ -439,12 +439,6 @@ margin-top: var(--space-6); } - .loading { - text-align: center; - padding: var(--space-9); - color: var(--text-secondary); - } - @media (max-width: 600px) { .field-row { flex-direction: column; diff --git a/frontend/src/routes/InviteCodes.svelte b/frontend/src/routes/InviteCodes.svelte --- a/frontend/src/routes/InviteCodes.svelte +++ b/frontend/src/routes/InviteCodes.svelte @@ -192,15 +192,6 @@ margin-bottom: var(--space-7); } - .error { - padding: var(--space-3); - background: var(--error-bg); - border: 1px solid var(--error-border); - border-radius: var(--radius-md); - color: var(--error-text); - margin-bottom: var(--space-4); - } - .created-code { padding: var(--space-6); background: var(--success-bg); diff --git a/frontend/src/routes/Migration.svelte b/frontend/src/routes/Migration.svelte --- a/frontend/src/routes/Migration.svelte +++ b/frontend/src/routes/Migration.svelte @@ -74,17 +74,18 @@ if (!hasOAuthCallback) { if (hasPendingMigration()) { - resumeInfo = getResumeInfo() - if (resumeInfo) { - if (resumeInfo.step === 'success') { + const info = getResumeInfo() + if (info) { + if (info.step === 'success') { clearMigrationState() - resumeInfo = null } else { + resumeInfo = info const stored = loadMigrationState() if (stored && stored.direction === 'inbound') { direction = 'inbound' - inboundFlow = createInboundMigrationFlow() - inboundFlow.resumeFromState(stored) + const flow = createInboundMigrationFlow() + flow.resumeFromState(stored) + inboundFlow = flow } } } @@ -94,8 +95,9 @@ clearOfflineState() } else { direction = 'offline-inbound' - offlineFlow = createOfflineInboundMigrationFlow() - offlineFlow.tryResume() + const flow = createOfflineInboundMigrationFlow() + flow.tryResume() + offlineFlow = flow } } } diff --git a/frontend/src/routes/OAuthConsent.svelte b/frontend/src/routes/OAuthConsent.svelte --- a/frontend/src/routes/OAuthConsent.svelte +++ b/frontend/src/routes/OAuthConsent.svelte @@ -93,8 +93,8 @@ body: JSON.stringify({ request_uri: consentData.request_uri, approved_scopes: approvedScopes, - remember: rememberChoice - }) + remember: rememberChoice, + }), }) if (!response.ok) { diff --git a/frontend/src/routes/Register.svelte b/frontend/src/routes/Register.svelte --- a/frontend/src/routes/Register.svelte +++ b/frontend/src/routes/Register.svelte @@ -516,12 +516,6 @@ color: var(--error-text); } - .section-hint { - font-size: var(--text-sm); - color: var(--text-secondary); - margin: 0 0 var(--space-5) 0; - } - .radio-group { display: flex; flex-direction: column; diff --git a/frontend/src/routes/RepoExplorer.svelte b/frontend/src/routes/RepoExplorer.svelte --- a/frontend/src/routes/RepoExplorer.svelte +++ b/frontend/src/routes/RepoExplorer.svelte @@ -599,12 +599,6 @@ color: var(--success-text); } - .loading-text { - text-align: center; - color: var(--text-secondary); - padding: var(--space-7); - } - .toolbar { display: flex; gap: var(--space-2); diff --git a/frontend/src/routes/Security.svelte b/frontend/src/routes/Security.svelte --- a/frontend/src/routes/Security.svelte +++ b/frontend/src/routes/Security.svelte @@ -795,12 +795,6 @@ margin: var(--space-2) 0 0 0; } - .loading { - text-align: center; - color: var(--text-secondary); - padding: var(--space-7); - } - section { padding: var(--space-6); background: var(--bg-secondary); diff --git a/frontend/src/routes/Settings.svelte b/frontend/src/routes/Settings.svelte --- a/frontend/src/routes/Settings.svelte +++ b/frontend/src/routes/Settings.svelte @@ -960,8 +960,7 @@ font-size: var(--text-xs); } - .empty, - .loading { + .empty { color: var(--text-secondary); font-size: var(--text-sm); margin-bottom: var(--space-4); diff --git a/frontend/src/routes/TrustedDevices.svelte b/frontend/src/routes/TrustedDevices.svelte --- a/frontend/src/routes/TrustedDevices.svelte +++ b/frontend/src/routes/TrustedDevices.svelte @@ -244,12 +244,6 @@ font-size: var(--text-sm); } - .loading { - text-align: center; - padding: var(--space-7); - color: var(--text-secondary); - } - .empty-state { text-align: center; padding: var(--space-8) var(--space-4); diff --git a/frontend/src/styles/migration.css b/frontend/src/styles/migration.css --- a/frontend/src/styles/migration.css +++ b/frontend/src/styles/migration.css @@ -3,6 +3,14 @@ margin: 0 auto; } +.field-label { + display: block; + font-size: var(--text-sm); + font-weight: var(--font-medium); + color: var(--text-primary); + margin-bottom: var(--space-2); +} + .step-indicator { display: flex; align-items: center; diff --git a/src/oauth/endpoints/authorize.rs b/src/oauth/endpoints/authorize.rs --- a/src/oauth/endpoints/authorize.rs +++ b/src/oauth/endpoints/authorize.rs @@ -22,7 +22,18 @@ const DEVICE_COOKIE_NAME: &str = "oauth_device_id"; fn redirect_see_other(uri: &str) -> Response { - (StatusCode::SEE_OTHER, [(LOCATION, uri.to_string())]).into_response() + ( + StatusCode::SEE_OTHER, + [ + (LOCATION, uri.to_string()), + (axum::http::header::CACHE_CONTROL, "no-store".to_string()), + ( + SET_COOKIE, + "bfCacheBypass=foo; max-age=1; SameSite=Lax".to_string(), + ), + ], + ) + .into_response() } fn redirect_to_frontend_error(error: &str, description: &str) -> Response { @@ -783,13 +794,13 @@ { return show_login_error("An error occurred. Please try again.", json_response); } - let redirect_url = build_success_redirect( - &request_data.parameters.redirect_uri, - &code.0, - request_data.parameters.state.as_deref(), - request_data.parameters.response_mode.as_deref(), - ); if json_response { + let redirect_url = build_intermediate_redirect_url( + &request_data.parameters.redirect_uri, + &code.0, + request_data.parameters.state.as_deref(), + request_data.parameters.response_mode.as_deref(), + ); if let Some(cookie) = new_cookie { ( StatusCode::OK, @@ -800,14 +811,22 @@ } else { Json(serde_json::json!({"redirect_uri": redirect_url})).into_response() } - } else if let Some(cookie) = new_cookie { - ( - StatusCode::SEE_OTHER, - [(SET_COOKIE, cookie), (LOCATION, redirect_url)], - ) - .into_response() } else { - redirect_see_other(&redirect_url) + let redirect_url = build_success_redirect( + &request_data.parameters.redirect_uri, + &code.0, + request_data.parameters.state.as_deref(), + request_data.parameters.response_mode.as_deref(), + ); + if let Some(cookie) = new_cookie { + ( + StatusCode::SEE_OTHER, + [(SET_COOKIE, cookie), (LOCATION, redirect_url)], + ) + .into_response() + } else { + redirect_see_other(&redirect_url) + } } } @@ -984,7 +1003,7 @@ "An error occurred. Please try again.", ); } - let redirect_url = build_success_redirect( + let redirect_url = build_intermediate_redirect_url( &request_data.parameters.redirect_uri, &code.0, request_data.parameters.state.as_deref(), @@ -1012,16 +1031,68 @@ '?' }; redirect_url.push(separator); - redirect_url.push_str(&format!("code={}", url_encode(code))); + let pds_hostname = std::env::var("PDS_HOSTNAME").unwrap_or_else(|_| "localhost".to_string()); + redirect_url.push_str(&format!( + "iss={}", + url_encode(&format!("https://{}", pds_hostname)) + )); if let Some(req_state) = state { redirect_url.push_str(&format!("&state={}", url_encode(req_state))); } - let pds_hostname = std::env::var("PDS_HOSTNAME").unwrap_or_else(|_| "localhost".to_string()); - redirect_url.push_str(&format!( - "&iss={}", - url_encode(&format!("https://{}", pds_hostname)) - )); + redirect_url.push_str(&format!("&code={}", url_encode(code))); redirect_url +} + +fn build_intermediate_redirect_url( + redirect_uri: &str, + code: &str, + state: Option<&str>, + response_mode: Option<&str>, +) -> String { + let pds_hostname = std::env::var("PDS_HOSTNAME").unwrap_or_else(|_| "localhost".to_string()); + let mut url = format!( + "https://{}/oauth/authorize/redirect?redirect_uri={}&code={}", + pds_hostname, + url_encode(redirect_uri), + url_encode(code) + ); + if let Some(s) = state { + url.push_str(&format!("&state={}", url_encode(s))); + } + if let Some(rm) = response_mode { + url.push_str(&format!("&response_mode={}", url_encode(rm))); + } + url +} + +#[derive(Debug, Deserialize)] +pub struct AuthorizeRedirectParams { + redirect_uri: String, + code: String, + state: Option, + response_mode: Option, +} + +pub async fn authorize_redirect(Query(params): Query) -> Response { + let final_url = build_success_redirect( + ¶ms.redirect_uri, + ¶ms.code, + params.state.as_deref(), + params.response_mode.as_deref(), + ); + tracing::info!( + final_url = %final_url, + client_redirect = %params.redirect_uri, + "authorize_redirect performing 303 redirect" + ); + ( + StatusCode::SEE_OTHER, + [ + (axum::http::header::LOCATION, final_url), + (axum::http::header::CACHE_CONTROL, "no-store".to_string()), + ], + ) + .into_response() } #[derive(Debug, Serialize)] @@ -1367,11 +1438,16 @@ } }; - if let Some(err_response) = validate_auth_flow_state(&flow_state, true) { - if flow_state.is_expired() { - let _ = db::delete_authorization_request(&state.db, &form.request_uri).await; - } - return err_response; + if flow_state.is_expired() { + let _ = db::delete_authorization_request(&state.db, &form.request_uri).await; + return json_error( + StatusCode::BAD_REQUEST, + "invalid_request", + "Authorization request has expired", + ); + } + if flow_state.is_pending() { + return json_error(StatusCode::FORBIDDEN, "access_denied", "Not authenticated"); } let did = flow_state.did().unwrap().to_string(); @@ -1420,14 +1496,11 @@ && !has_granular_scopes && !form.approved_scopes.contains(&"atproto".to_string()) { - return ( + return json_error( StatusCode::BAD_REQUEST, - Json(serde_json::json!({ - "error": "invalid_request", - "error_description": "The atproto scope was requested and must be approved" - })), - ) - .into_response(); + "invalid_request", + "The atproto scope was requested and must be approved", + ); } let final_approved: Vec = if user_denied_some_granular { form.approved_scopes @@ -1439,14 +1512,11 @@ form.approved_scopes.clone() }; if final_approved.is_empty() { - return ( + return json_error( StatusCode::BAD_REQUEST, - Json(serde_json::json!({ - "error": "invalid_request", - "error_description": "At least one scope must be approved" - })), - ) - .into_response(); + "invalid_request", + "At least one scope must be approved", + ); } let approved_scope_str = final_approved.join(" "); let has_valid_scope = final_approved.iter().all(|s| { @@ -1462,14 +1532,11 @@ || s.starts_with("include:") }); if !has_valid_scope { - return ( + return json_error( StatusCode::BAD_REQUEST, - Json(serde_json::json!({ - "error": "invalid_request", - "error_description": "Invalid scope format" - })), - ) - .into_response(); + "invalid_request", + "Invalid scope format", + ); } if form.remember { let preferences: Vec = requested_scopes @@ -1503,25 +1570,25 @@ .await .is_err() { - return ( + return json_error( StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "server_error", - "error_description": "Failed to complete authorization" - })), - ) - .into_response(); + "server_error", + "Failed to complete authorization", + ); } - let redirect_url = build_success_redirect( - &request_data.parameters.redirect_uri, + let redirect_uri = &request_data.parameters.redirect_uri; + let intermediate_url = build_intermediate_redirect_url( + redirect_uri, &code.0, request_data.parameters.state.as_deref(), request_data.parameters.response_mode.as_deref(), ); - Json(serde_json::json!({ - "redirect_uri": redirect_url - })) - .into_response() + tracing::info!( + intermediate_url = %intermediate_url, + client_redirect = %redirect_uri, + "consent_post returning JSON with intermediate URL (for 303 redirect)" + ); + Json(serde_json::json!({ "redirect_uri": intermediate_url })).into_response() } pub async fn authorize_2fa_post( @@ -1630,7 +1697,7 @@ "An error occurred. Please try again.", ); } - let redirect_url = build_success_redirect( + let redirect_url = build_intermediate_redirect_url( &request_data.parameters.redirect_uri, &code.0, request_data.parameters.state.as_deref(), @@ -1725,7 +1792,7 @@ "An error occurred. Please try again.", ); } - let redirect_url = build_success_redirect( + let redirect_url = build_intermediate_redirect_url( &request_data.parameters.redirect_uri, &code.0, request_data.parameters.state.as_deref(), @@ -2367,7 +2434,7 @@ .into_response(); } - let redirect_url = build_success_redirect( + let redirect_url = build_intermediate_redirect_url( &request_data.parameters.redirect_uri, &code.0, request_data.parameters.state.as_deref(), diff --git a/frontend/src/components/migration/ChooseHandleStep.svelte b/frontend/src/components/migration/ChooseHandleStep.svelte --- a/frontend/src/components/migration/ChooseHandleStep.svelte +++ b/frontend/src/components/migration/ChooseHandleStep.svelte @@ -111,7 +111,7 @@
- + {$_('migration.inbound.chooseHandle.authMethod')}