diff --git a/crates/google-workspace-cli/src/auth_commands.rs b/crates/google-workspace-cli/src/auth_commands.rs index d7571e74..167b532a 100644 --- a/crates/google-workspace-cli/src/auth_commands.rs +++ b/crates/google-workspace-cli/src/auth_commands.rs @@ -542,6 +542,48 @@ struct CliFlowDelegate { login_hint: Option, } +/// Defensive: ensure `response_type=code` is present before the user +/// opens the URL. The URL arrives from yup_oauth2's +/// `build_authentication_request_url`, which should already include it, +/// but issue #695 reports cases on macOS with Desktop OAuth clients where +/// the URL reaching the browser is missing `response_type`, and Google +/// rejects with "Error 400: invalid_request, Required parameter is +/// missing: response_type". Injecting here is a no-op when the upstream +/// URL is correct and a targeted fix when it is not. +/// +/// Also appends `login_hint` if provided, and preserves any URL fragment +/// by splitting it off before query-string manipulation and reattaching +/// it at the end. +fn build_display_url(url: &str, login_hint: Option<&str>) -> String { + let (base, fragment) = match url.split_once('#') { + Some((b, f)) => (b, Some(f)), + None => (url, None), + }; + + let mut display_url = if base.split(['?', '&']).any(|p| p == "response_type=code") { + base.to_string() + } else if base.contains('?') { + format!("{base}&response_type=code") + } else { + format!("{base}?response_type=code") + }; + + if let Some(hint) = login_hint { + let encoded = percent_encoding::percent_encode( + hint.as_bytes(), + percent_encoding::NON_ALPHANUMERIC, + ); + display_url = format!("{display_url}&login_hint={encoded}"); + } + + if let Some(f) = fragment { + display_url.push('#'); + display_url.push_str(f); + } + + display_url +} + impl yup_oauth2::authenticator_delegate::InstalledFlowDelegate for CliFlowDelegate { fn present_user_url<'a>( &'a self, @@ -550,21 +592,7 @@ impl yup_oauth2::authenticator_delegate::InstalledFlowDelegate for CliFlowDelega ) -> std::pin::Pin> + Send + 'a>> { Box::pin(async move { - // Inject login_hint into the OAuth URL if we have one - let display_url = if let Some(ref hint) = self.login_hint { - let encoded: String = percent_encoding::percent_encode( - hint.as_bytes(), - percent_encoding::NON_ALPHANUMERIC, - ) - .to_string(); - if url.contains('?') { - format!("{url}&login_hint={encoded}") - } else { - format!("{url}?login_hint={encoded}") - } - } else { - url.to_string() - }; + let display_url = build_display_url(url, self.login_hint.as_deref()); eprintln!("Open this URL in your browser to authenticate:\n"); eprintln!(" {display_url}\n"); Ok(String::new()) @@ -2532,4 +2560,97 @@ mod tests { let err = read_refresh_token_from_cache(file.path()).unwrap_err(); assert!(err.to_string().contains("no refresh token was returned")); } + + #[test] + fn url_without_query_gets_response_type() { + let out = build_display_url("https://accounts.google.com/o/oauth2/v2/auth", None); + assert_eq!(out, "https://accounts.google.com/o/oauth2/v2/auth?response_type=code"); + } + + #[test] + fn url_with_query_gets_response_type_appended() { + let out = build_display_url( + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc", + None, + ); + assert_eq!( + out, + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc&response_type=code" + ); + } + + #[test] + fn url_with_exact_response_type_param_not_duplicated() { + let input = + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc&response_type=code&scope=openid"; + let out = build_display_url(input, None); + assert_eq!(out, input); + } + + #[test] + fn url_with_response_type_in_other_param_value_still_injects() { + // `state=response_type=code_fake` contains the substring + // "response_type=code" but is not itself the response_type param. + // The exact-equality check must see through this and still inject + // the real response_type=code. + let out = build_display_url( + "https://accounts.google.com/o/oauth2/v2/auth?state=response_type=code_fake", + None, + ); + assert_eq!( + out, + "https://accounts.google.com/o/oauth2/v2/auth?state=response_type=code_fake&response_type=code" + ); + } + + #[test] + fn url_with_fragment_preserves_fragment() { + let out = build_display_url( + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc#section", + None, + ); + assert_eq!( + out, + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc&response_type=code#section" + ); + } + + #[test] + fn url_with_response_type_only_in_fragment_still_injects() { + // Fragments are client-side only and do not satisfy the OAuth + // requirement. The fragment-split must happen before the param + // check so response_type=code lands in the query string. + let out = build_display_url( + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc#response_type=code", + None, + ); + assert_eq!( + out, + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc&response_type=code#response_type=code" + ); + } + + #[test] + fn login_hint_appended_after_response_type_with_encoding() { + let out = build_display_url( + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc", + Some("foo+bar@example.com"), + ); + assert_eq!( + out, + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc&response_type=code&login_hint=foo%2Bbar%40example%2Ecom" + ); + } + + #[test] + fn fragment_reattached_after_login_hint() { + let out = build_display_url( + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc#section", + Some("user@example.com"), + ); + assert_eq!( + out, + "https://accounts.google.com/o/oauth2/v2/auth?client_id=abc&response_type=code&login_hint=user%40example%2Ecom#section" + ); + } }