fix(auth): defensively inject response_type=code in OAuth URL (#695)#737
fix(auth): defensively inject response_type=code in OAuth URL (#695)#737derek3078 wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a defensive measure to ensure that OAuth authentication URLs are correctly formatted before being displayed to the user. By explicitly injecting the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces defensive logic to ensure the response_type=code parameter is present in the OAuth authentication URL, addressing reported issues where Google rejects requests due to a missing parameter. While the fix addresses the core issue, the current implementation of URL manipulation is fragile as it does not account for URL fragments (#) and uses simple substring matching which could lead to malformed URLs. A more robust approach for parsing and rebuilding the URL is recommended.
| let url_with_response_type = if url.contains("response_type=code") { | ||
| url.to_string() | ||
| } else if url.contains('?') { | ||
| format!("{url}&response_type=code") | ||
| } else { | ||
| format!("{url}?response_type=code") | ||
| }; | ||
|
|
||
| // 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}") | ||
| if url_with_response_type.contains('?') { | ||
| format!("{url_with_response_type}&login_hint={encoded}") | ||
| } else { | ||
| format!("{url}?login_hint={encoded}") | ||
| format!("{url_with_response_type}?login_hint={encoded}") | ||
| } | ||
| } else { | ||
| url.to_string() | ||
| url_with_response_type | ||
| }; |
There was a problem hiding this comment.
The current implementation of URL parameter injection is fragile and can produce malformed URLs if the input contains a fragment (#) or if the substring response_type=code appears elsewhere in the URL (e.g., as part of another parameter's value). Appending parameters directly to the end of the string will incorrectly place them inside the fragment if one is present, making them invisible to the server.
A more robust approach is to separate the fragment before manipulation and use a more precise check for the existence of the response_type parameter. Since the code ensures response_type=code is always present (either by finding it or adding it), any subsequent parameters like login_hint can safely be appended using &.
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(ref hint) = self.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);
}…workspace#695) Issue googleworkspace#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 googleworkspace#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 googleworkspace#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 googleworkspace#695
derek3078
force-pushed
the
fix/auth-login-response-type-695
branch
from
bd05713 to
d8be797
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a defensive build_display_url function to ensure the response_type=code parameter is present in OAuth authentication URLs, addressing issues where it might be missing on certain platforms. It also refactors the CliFlowDelegate to use this function for URL construction and adds a comprehensive suite of unit tests to verify correct handling of query parameters, fragments, and login hints. I have no feedback to provide.
2 participants
Summary
Addresses the symptom described in #695:
gws auth loginfails with"Error 400: invalid_request, Required parameter is missing:
response_type" on macOS Desktop OAuth clients. Adds a defensive
injection of
response_type=codeinCliFlowDelegate::present_user_urlso the URL presented to the user is always well-formed.
What this changes
crates/google-workspace-cli/src/auth_commands.rs: inCliFlowDelegate::present_user_url, if the URL received fromyup_oauth2 does not contain
response_type=code, append it beforepresenting to the user.
has_proxy_env),or any other code path.
Why not a deeper fix
yup_oauth2 v12.1.2's
build_authentication_request_urlincludes"&response_type=code"in its params vector. Reading the sourcesuggests the URL reaching the delegate should be correct. Source
inspection alone did not reproduce the missing-parameter condition.
The empirical report in #695, including @diegobartra's direct-URL
reproduction in a browser, nonetheless establishes that affected users
do see the parameter missing.
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.
Verification
Verified locally on macOS aarch64 with rustc 1.95.0 (stable):
cargo checkon the full workspace: clean, no warnings.cargo test --package google-workspace-cli auth_commands: 68 passed,0 failed, 0 ignored.
ask_auth_code_interactivelyandask_auth_code_via_httpcode paths.Credit
Diagnostic and root-cause narrative by @diegobartra in #695. This PR
implements a defensive workaround for the symptom they described.