From d8be79706ef7420152a719ce4aace43a4dbf0010 Mon Sep 17 00:00:00 2001 From: Derek Lacey <266581052+derek3078@users.noreply.github.com> Date: Mon, 20 Apr 2026 21:38:59 +1000 Subject: [PATCH] fix(auth): defensively inject response_type=code in OAuth URL (#695) Issue #695 reports that `gws auth login` produces an authorization URL missing `response_type=code`, causing Google to reject with "Error 400: invalid_request, Required parameter is missing: response_type". Reproduces on macOS (Darwin arm64) with Desktop OAuth clients on v0.22.4 and v0.22.5. yup_oauth2 v12.1.2's `build_authentication_request_url` does include `&response_type=code` in its params vector, so code review of the URL builder alone suggests the URL reaching the delegate should be correct. The empirical report in #695 nonetheless shows the parameter missing at the point Google receives the request, verified by @diegobartra by constructing a correctly-formed URL by hand and watching the consent screen render against the same Desktop OAuth client. This change makes `CliFlowDelegate::present_user_url` inject `response_type=code` defensively when it is absent from the URL. When the upstream URL is correct, this is a no-op. When it is not, this unblocks the user. This PR is intentionally narrow: it fixes the symptom reliably without changing flow selection or dependencies. The deeper root cause (why yup_oauth2's params fold does not always reach the delegate) is worth investigation but does not block this fix. Credit to @diegobartra for the diagnostic in #695. Updated per gemini-code-assist review: handle URL fragments by splitting on '#' before manipulation, and use exact-parameter equality check instead of substring match. URL-rewriting logic extracted into a pure helper function with unit tests covering the edge cases Gemini's review identified. Verified locally on macOS aarch64 with rustc 1.95.0: - cargo check: clean, no warnings - cargo test --package google-workspace-cli auth_commands: 76 passed, 0 failed, 0 ignored Addresses #695 --- .../google-workspace-cli/src/auth_commands.rs | 151 ++++++++++++++++-- 1 file changed, 136 insertions(+), 15 deletions(-) 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" + ); + } }