feat(auth): open the OAuth URL in the default browser - #888
Conversation
🦋 Changeset detectedLatest commit: af57b34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 enhances the user experience for 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 the 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 counterproductive. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces automatic browser opening for OAuth login URLs across Linux, macOS, and Windows, with a fallback to copy-pasting. It also fixes an environment variable leak in tests and refactors a match arm in script.rs. The feedback suggests using std::thread::Builder::new().spawn() instead of std::thread::spawn to handle potential thread spawning failures gracefully and prevent application panics.
| Ok(mut child) => { | ||
| std::thread::spawn(move || { | ||
| let _ = child.wait(); | ||
| }); | ||
| true | ||
| } |
There was a problem hiding this comment.
Using std::thread::spawn can panic and crash the entire CLI application if the operating system fails to spawn a new thread (e.g., due to resource exhaustion or thread limits). Since this is a CLI tool that may run in resource-constrained environments (like containers or remote servers), it is safer to use std::thread::Builder::new().spawn() which returns a Result and handle any failure gracefully without panicking.
Ok(mut child) => {
let _ = std::thread::Builder::new()
.name("gws-browser-reaper".to_string())
.spawn(move || {
let _ = child.wait();
});
true
}There was a problem hiding this comment.
Done in af57b34 — thread::Builder::spawn with the error discarded: if the OS refuses the reaper thread, the child just goes unreaped until this short-lived CLI exits, and the login is unaffected.
gws auth login now attempts to open the authorization URL - $BROWSER first, otherwise the platform opener - while keeping the printed URL as the copy-paste fallback. Applies to both the proxy-aware flow and the yup-oauth2 flow delegate. Supersedes googleworkspace#875. Also unbreaks CI on current stable (clippy 1.97 rejects a nested if in helpers/script.rs on unmodified main) and fixes a 1-in-6 test flake (a bare set_var leaked GOOGLE_WORKSPACE_CLI_CONFIG_DIR into later tests; now uses the module's own EnvVarGuard).
7332621 to
af57b34
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to automatically open OAuth login URLs in the system's default browser with a copy-paste fallback. It implements OS-specific browser commands (using xdg-open on Linux, open on macOS, and explorer on Windows) and includes URL validation to prevent shell injection or other security issues. Additionally, it improves test isolation by using an environment variable guard and refactors a match statement in the script helper. There are no review comments, and I have no feedback to provide.
Description
This supersedes #875, which went stale before its review feedback was addressed.
gws auth logincurrently prints the OAuth URL and waits — on a remote or headless machine that means manually copying the URL and manually forwarding the ephemeral localhost callback port. This PR makes both login flows attempt to open the URL in a browser, keeping the printed URL as the copy-paste fallback:$BROWSERfirst when set (treated as a single command, not a shell line).xdg-openon Linux,openon macOS,exploreron Windows.Opening in your browser...only when an opener actually spawned; the existing URL output is unchanged either way (tools and wrappers parse it).login_with_proxy_support) and theyup-oauth2InstalledFlowDelegate.httpsschemes, control/whitespace/dangerous-Unicode characters, quotes, backslashes, and shell metacharacters — addressing the Gemini review feedback on feat(auth): open OAuth URL in default browser automatically #875.explorerrather thanrundll32on Windows addresses the EDR concern raised there. No new dependency.Two small riders needed for a green CI run, called out so they don't surprise anyone:
helpers/script.rs: clippy 1.97 (current stable) rejects a nestedifinside amatchon unmodifiedmain, socargo clippy --workspace -- -D warningsfails before this PR changes anything. Collapsed into a guarded match arm, behavior-preserving and covered by the existing tests.auth.rstests:test_load_credentials_encrypted_filesetGOOGLE_WORKSPACE_CLI_CONFIG_DIRwith a bareset_varand never restored it, leaking a dropped tempdir path into later tests —config_dir_returns_gws_subdirfailed roughly 1 in 6 local runs on unmodifiedmain. Now uses the test module's ownEnvVarGuard; 10 consecutive clean runs after.Dry Run Output:
Test evidence
New unit tests cover opener selection (
$BROWSERprecedence, per-OS defaults, unknown platform) and URL validation (accepts https OAuth URLs; rejects whitespace, control characters, RTL-override/zero-width Unicode, quotes, backslashes, and shell metacharacters).Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.