fix(auth): propagate error when token directory cannot be created#542
fix(auth): propagate error when token directory cannot be created#542dumko2001 wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: ec2b687 The changes in this PR will be included in the next version bump. 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 robustness of the authentication system by addressing a critical flaw where failures in creating the token storage directory were previously ignored. By explicitly propagating these I/O errors, the system now provides clearer feedback on credential persistence issues, improving reliability and preventing confusing authentication behavior. 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 correctly propagates an error that was previously ignored when creating the token storage directory. My review identifies a similar issue where an error from setting directory permissions is also ignored, which could lead to a security risk. I've suggested a fix to make the error handling in this function fully robust.
| tokio::fs::create_dir_all(parent).await.map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "Failed to create token directory '{}': {}", | ||
| parent.display(), | ||
| e | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
While this change correctly propagates errors from create_dir_all, a similar issue exists a few lines below that should also be addressed for robust error handling.
The result of std::fs::set_permissions on line 95 is currently ignored. If setting the directory permissions fails, it could leave the token storage directory with incorrect, possibly insecure, permissions. This error should also be propagated to make the error handling in this block complete.
std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700))
.map_err(|e| anyhow::anyhow!("Failed to set permissions on token directory '{}': {}", parent.display(), e))?;
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where errors during the creation of the token storage directory were silently ignored. The change ensures that any I/O error is now properly propagated, which is a great improvement. The implementation is correct and robust.
As a note for future improvement, I observed a related pattern on line 95 where the result of std::fs::set_permissions is ignored. This could lead to the token directory having insecure permissions. To improve robustness, this error should also be propagated. Since this is outside the direct changes of this PR, it can be addressed in a follow-up to avoid scope creep.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where an I/O error was silently ignored when creating the token directory. The change now properly propagates the error, improving the robustness of the authentication flow. I've added one comment regarding sanitizing the path in the new error message to prevent potential terminal escape sequence injection, which is a good practice for security.
| tokio::fs::create_dir_all(parent).await.map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "Failed to create token directory '{}': {}", | ||
| parent.display(), |
There was a problem hiding this comment.
The path parent.display() is included in the error message without sanitization. If a path contains terminal escape sequences, this could lead to escape sequence injection when the error is printed to a terminal. It's safer to sanitize external inputs like file paths before including them in messages that will be displayed.
| parent.display(), | |
| sanitize_for_terminal(&parent.display().to_string()), |
References
- Sanitize error strings printed to the terminal to prevent escape sequence injection.
2 participants
Description
Previously, errors encountered during the creation of the token storage directory were silently discarded. This could lead to confusing behavior where authentication appears to succeed but credentials are not persisted to disk. This PR ensures that IO errors during directory creation are properly propagated.
Fixes #493
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.