fix(core): restore auth consent in headless mode and add unit tests#19689
fix(core): restore auth consent in headless mode and add unit tests#19689ehedlund merged 2 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @ehedlund, 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 fixes a critical regression in the CLI's authentication consent mechanism for headless environments, ensuring that users can properly authenticate even without an interactive UI. Concurrently, it introduces a significant security enhancement by adding deceptive URL detection to tool confirmation messages, providing users with warnings about potentially malicious links. These changes improve both the functionality and security posture of the application. Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
0063405 to
8d65d08
Compare
There was a problem hiding this comment.
Code Review
The pull request addresses a critical bug where authentication consent in headless mode was broken and adds comprehensive unit tests to prevent future regressions. It also introduces URL security utilities to detect and warn about deceptive URLs in tool confirmation messages, enhancing user security. The changes are well-tested and improve the robustness and security of the CLI.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx (561-564)
This block correctly renders the securityWarnings component when present. Placing it before the main question ensures that critical security information is presented to the user prominently and early in the confirmation flow. This is vital for user awareness and decision-making.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx (37-42)
The introduction of WarningMessage, getDeceptiveUrlDetails, toUnicodeUrl, and DeceptiveUrlDetails is a significant security enhancement. This allows the UI to proactively warn users about potentially deceptive URLs, which is crucial for preventing phishing and other social engineering attacks. This directly addresses a potential security vulnerability.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx (108-137)
The deceptiveUrlWarnings and deceptiveUrlWarningText memos correctly identify and format warnings for deceptive URLs. This logic is well-placed to ensure that all relevant URLs, whether from info or exec type commands, are checked for deceptive characteristics. This is a critical security feature.
packages/core/src/utils/authConsent.ts (20-29)
This revised logic for getConsentForOauth correctly prioritizes headless mode consent via getOauthConsentNonInteractive and then checks for interactive UI listeners. The previous implementation incorrectly threw an error in headless mode when no UI listeners were present, which is now fixed. The FatalAuthenticationError is now correctly thrown only when no suitable consent mechanism (headless or interactive UI) is available, ensuring robust error handling in critical authentication paths. This is a critical fix for the authentication flow.
packages/cli/src/ui/utils/urlSecurityUtils.ts (1-90)
The new urlSecurityUtils.ts file introduces essential functions for detecting and handling deceptive URLs. toUnicodeUrl correctly converts Punycode to Unicode for display, and getDeceptiveUrlDetails identifies URLs that might be used in homograph attacks. This is a critical addition to the CLI's security features, protecting users from malicious links. The manual reconstruction of the URL in toUnicodeUrl is a clever and necessary workaround for the WHATWG URL class's automatic Punycode conversion, ensuring that the visually deceptive Unicode version is correctly presented to the user.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx (488-491)
Using toUnicodeUrl(urlString) when displaying URLs to fetch is a crucial step in mitigating homograph attacks. By converting Punycode URLs back to their Unicode representation, users can visually inspect the actual domain, preventing them from being tricked by deceptive look-alike domains. This is a critical security improvement.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx (90-204)
The new tests for deceptive URLs are a great addition, significantly improving the security posture of the application by ensuring that potential phishing attempts are flagged to the user. This is a critical improvement for user safety.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx (510)
Returning securityWarnings from the useMemo hook is essential for the ToolConfirmationMessage component to render any detected security issues. This ensures that the warnings are passed up to the rendering logic.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx (517)
Including deceptiveUrlWarningText in the dependency array for the useMemo hook ensures that the question, bodyContent, options, and securityWarnings are re-calculated whenever the deceptive URL warning text changes. This is important for keeping the UI up-to-date with security alerts.
packages/cli/src/ui/utils/urlSecurityUtils.test.ts (1-65)
The new urlSecurityUtils.test.ts file provides comprehensive unit tests for toUnicodeUrl and getDeceptiveUrlDetails. These tests cover various scenarios, including Punycode conversion, complex URLs, and handling of invalid URLs, ensuring the reliability and correctness of the URL security utilities. This is crucial for maintaining the integrity of the security features.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx (310-312)
This conditional rendering of WarningMessage based on deceptiveUrlWarningText ensures that security warnings are displayed prominently when detected. This is a direct implementation of the new security feature.
packages/core/src/utils/authConsent.test.ts (30-142)
The refactoring of getConsentForOauth tests into interactive mode and non-interactive mode describes blocks, along with the addition of new test cases, significantly improves test coverage and clarity. This ensures that both interactive UI-based consent and headless readline-based consent mechanisms are thoroughly validated, preventing regressions in a critical authentication flow. This is a high-quality improvement for maintainability and correctness.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx (300-304)
Adding securityWarnings to the useMemo return type ensures that the UI can display security-related messages, such as deceptive URL warnings. This is a necessary structural change to integrate the new security features into the existing component.
Summary
This PR fixes the authentication consent logic for headless/non-interactive mode which was inadvertently broken in a recent commit. It also adds a comprehensive set of unit tests to
packages/core/src/utils/authConsent.test.tsto prevent future regressions.Details
getConsentForOauthinpackages/core/src/utils/authConsent.tsto correctly handle headless mode usingreadlineinstead of throwing an error when UI listeners are absent.packages/core/src/utils/authConsent.test.tsfor better readability and organization.Related Issues
Fixes #19677
How to Validate
npm test -w @google/gemini-cli-core -- src/utils/authConsent.test.ts-pand trigger an OAuth flow to confirm it prompts correctly on stdin.Pre-Merge Checklist