fix: full headless auth support (logging + manual flow) #92
Conversation
Summary of ChangesHello @allenhutchison, 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 comprehensive support for authentication in headless environments. It addresses potential stdout corruption by directing authentication-related messages to stderr and implements a robust manual authentication flow. This allows users on headless servers (e.g., via SSH or Linux without a GUI) to authenticate by opening a URL on another machine, copying the generated credentials, and pasting them directly into their terminal, ensuring a seamless experience where automated browser launch is not feasible. 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. 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
|
![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 effectively adds support for headless authentication by introducing a manual copy-paste flow and ensuring auth-related messages are sent to stderr. The changes are well-structured. I've identified a couple of areas for improvement in the new authManual function to enhance its security and robustness, specifically regarding CSRF token validation and adding a timeout to prevent the process from hanging.
- Update Cloud Function to include CSRF token in credentials JSON. - Update AuthManager to validate CSRF token in manual flow. - Add 10-minute timeout to manual authentication to prevent hanging.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully implements full support for authentication in headless environments. The changes are well-structured, correctly replacing console.log with console.error for authentication messages to prevent stdout corruption, and introducing a robust manual authentication flow. The use of a CSRF token in the manual flow is a great security measure. My review includes a minor suggestion to improve the validation of pasted credentials for increased robustness. Overall, this is a solid and well-thought-out enhancement.
| const tokens = JSON.parse(answer.trim()); | ||
|
|
||
| if (tokens.csrf_token_for_validation !== csrfToken) { | ||
| reject(new Error('CSRF token mismatch. Authentication aborted.')); | ||
| return; | ||
| } | ||
|
|
||
| if (tokens.access_token) { | ||
| client.setCredentials(tokens); | ||
| logToFile('Manual authentication successful'); | ||
| resolve(); | ||
| } else { | ||
| reject(new Error('Invalid credentials JSON: missing access_token')); | ||
| } |
There was a problem hiding this comment.
The current validation for the pasted credentials only checks for the presence of access_token. This could lead to issues if other essential fields like expiry_date are missing or have an incorrect type. Additionally, the entire tokens object, which includes the csrf_token_for_validation property, is passed to client.setCredentials(). While the library might ignore unknown properties, it's cleaner and safer to pass only the expected credential fields.
I suggest adding more robust validation for essential fields and cleaning the object before passing it to setCredentials to make the manual flow more robust.
const tokens = JSON.parse(answer.trim());
if (tokens.csrf_token_for_validation !== csrfToken) {
reject(new Error('CSRF token mismatch. Authentication aborted.'));
return;
}
if (tokens.access_token && typeof tokens.access_token === 'string' && tokens.expiry_date && typeof tokens.expiry_date === 'number') {
// Exclude the CSRF validation token before setting credentials.
const { csrf_token_for_validation, ...validTokens } = tokens;
client.setCredentials(validTokens);
logToFile('Manual authentication successful');
resolve();
} else {
reject(new Error('Invalid credentials JSON: missing or invalid `access_token` or `expiry_date`.'));
}
1 participant
Fixes #63.
This PR implements full support for authentication in headless environments.
console.logwithconsole.errorfor authentication messages. This prevents stdout corruption in the MCP stdio transport, ensuring messages are visible in logs.AuthManager.ts): If a browser cannot be launched, the client now prints the auth URL to stderr and waits for the user to paste the credentials JSON from stdin.cloud_function/index.js): Updated the manual fallback page to provide clear instructions for copying the credentials JSON and pasting it back into the terminal.This allows users on headless servers (SSH, Linux without GUI) to authenticate by opening the URL on another machine and copying the token back.