feat(auth): Add oauth callback port#682
feat(auth): Add oauth callback port#682ilteoood wants to merge 8 commits intogoogleworkspace:mainfrom
Conversation
Agent-Logs-Url: https://github.com/ilteoood/gws-cli/sessions/a3a0d939-aa16-4501-995a-73774619e4e9 Co-authored-by: ilteoood <6383527+ilteoood@users.noreply.github.com>
|
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. |
🦋 Changeset detectedLatest commit: e472f0b 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 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 --callback-host and --callback-port flags, along with corresponding environment variables, to the gws auth login command. These additions allow users to configure the OAuth callback server host and port, facilitating authentication in environments with fixed redirect URIs or port-forwarding. The review feedback highlights several areas for improvement: the current implementation is restricted to IPv4 bindings, which lacks support for IPv6; the redirect_uri construction fails to properly format IPv6 addresses with square brackets; and the callback-host input requires validation to prevent the injection of dangerous characters into the generated URL.
Agent-Logs-Url: https://github.com/ilteoood/gws-cli/sessions/4f0920fe-4e99-4de9-90c1-1c0a65dc3ab5 Co-authored-by: ilteoood <6383527+ilteoood@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces --callback-host and --callback-port flags to the gws auth login command, allowing users to specify the host and port for the OAuth callback server. These settings can also be configured via environment variables. The implementation includes logic to handle different binding addresses and construct the appropriate redirect URI. Feedback was provided regarding the handling of IPv6 loopback addresses in the binding logic, which currently defaults to IPv4 and could cause authentication failures.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
ilteoood
changed the title
There was a problem hiding this comment.
Code Review
This pull request introduces the --callback-host and --callback-port flags (and corresponding environment variables) to the gws auth login command, allowing users to configure the OAuth callback server host and port. This is particularly useful for environments with fixed redirect URIs or when running in Docker/CI with port-forwarding. The implementation updates the internal login logic to dynamically determine the binding address and redirect URI based on these inputs. A review comment identified a bug in the binding logic where custom IPv6 hosts would incorrectly cause the server to bind to an IPv4 address (0.0.0.0), which would prevent successful callbacks; a suggestion was provided to bind to [::] for IPv6 hosts.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces --callback-host and --callback-port flags, along with corresponding environment variables, to the gws auth login command. These changes allow users to configure the OAuth callback server's host and port, facilitating authentication in Docker or CI environments where port-forwarding is used. The implementation includes logic to determine the appropriate network binding address and format the redirect URI, including support for IPv6. Review feedback suggests an improvement to the binding logic: when localhost is used with a custom port, the server should bind to all interfaces (0.0.0.0 or [::]) to ensure accessibility from outside a container while maintaining a valid Google OAuth redirect URI.
…ogic Agent-Logs-Url: https://github.com/ilteoood/gws-cli/sessions/603b728d-ac26-4682-ab45-3e6118d0aa91 Co-authored-by: ilteoood <6383527+ilteoood@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --callback-port flag and a corresponding GOOGLE_WORKSPACE_CLI_CALLBACK_PORT environment variable to the gws auth login command, allowing users to configure the OAuth callback server's port. This is particularly useful for environments like Docker or CI. The implementation updates the internal login logic to support this custom port and switches the binding address to 0.0.0.0. Feedback highlights a security concern regarding the default binding to 0.0.0.0, which exposes the callback server to the network; it is recommended to add a --callback-host flag that defaults to 127.0.0.1 to maintain security while allowing flexibility for containerized environments.
ilteoood
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --callback-port flag and a corresponding GOOGLE_WORKSPACE_CLI_CALLBACK_PORT environment variable to allow users to configure the OAuth callback server's port, which is particularly useful for Docker or CI environments. The authentication logic was updated to use the proxy-aware flow when a custom port is provided. A security concern was raised regarding the change to bind the callback server to 0.0.0.0 by default, as this exposes the temporary server to the network and represents a regression from the previous 127.0.0.1 binding. It is recommended to only bind to all interfaces if a non-zero port is explicitly requested.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the --callback-port flag and the GOOGLE_WORKSPACE_CLI_CALLBACK_PORT environment variable to the gws auth login command, enabling manual configuration of the OAuth callback server port. Review feedback highlights a security concern regarding the default binding to 0.0.0.0 when a custom port is used, suggesting a move back to 127.0.0.1. Furthermore, it is recommended to utilize yup_oauth2's native port configuration to ensure better reliability compared to the custom listener implementation.
| let host = if callback_port == 0 { "127.0.0.1" } else { "0.0.0.0" }; | ||
| let listener = TcpListener::bind(format!("{host}:{callback_port}")) |
There was a problem hiding this comment.
Binding to 0.0.0.0 by default when a custom port is specified is a security risk, as it exposes the temporary OAuth callback server to the entire network. While this is intended to support Docker/CI environments, it should not be forced on all users. To address this security concern while avoiding scope creep, consider defaulting to 127.0.0.1 and only binding to 0.0.0.0 if explicitly necessary for the environment.
References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
| // use proxy-aware OAuth flow (reqwest). Otherwise use yup-oauth2 (faster, | ||
| // but doesn't support proxy or custom callback configuration). | ||
| let (access_token, refresh_token) = | ||
| if crate::auth::has_proxy_env() || callback_port != 0 { |
There was a problem hiding this comment.
Forcing the use of login_with_proxy_support whenever a custom port is specified is a regression in reliability. The custom one-shot TCP listener fails on any non-OAuth request (like a favicon request). To maintain the primary goal of supporting custom ports without introducing unnecessary complexity or maintenance burden, it is recommended to use yup_oauth2::InstalledFlowAuthenticatorBuilder::with_port() instead of implementing custom listener logic.
References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
Description
Please include a summary of the change and which issue is fixed. If adding a new feature or command, please include the output of running it with
--dry-runto prove the JSON request body matches the Discovery Document schema.Dry Run Output:
// Paste --dry-run output here if applicableChecklist:
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.