fix(auth): refresh OAuth2 tokens in long-running watch/subscribe loops#407
Conversation
🦋 Changeset detectedLatest commit: f40d037 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 |
|
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. |
googleworkspace-bot
added
area: auth
area: core
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 significantly enhances the reliability of long-running operations, specifically Gmail watch and Workspace Events subscribe, by implementing a robust mechanism for refreshing OAuth2 access tokens. It introduces a new 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
|
![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 a robust mechanism for refreshing OAuth2 tokens in long-running processes using an AccessTokenProvider trait, improving reliability and correctly refactoring watch and subscribe helpers. However, a critical security concern has been identified: potential URL injection vulnerabilities in both the Gmail watch and Workspace Events subscribe helpers, where user-supplied subscription names and event types are used to construct API URLs without sufficient validation. Additionally, the current implementation fetches tokens excessively within loops, leading to unnecessary performance overhead. It is recommended to address the URL injection by applying strict resource name validation consistently across all helpers, and to optimize token usage by reusing tokens within a single loop iteration where they are still valid.
Remove redundant access_token() calls for acknowledge requests (both loops) and per-message Gmail fetches. The token obtained at the start of the iteration / function is still valid and is now reused for all HTTP calls within the same pass. Addresses review feedback from gemini-code-assist.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an AccessTokenProvider trait and ScopedTokenProvider to enable long-running helpers, specifically for Gmail watch and Workspace Events subscribe, to refresh OAuth access tokens before each API call, preventing token expiration issues. The changes involve refactoring token acquisition in src/auth.rs, src/helpers/events/subscribe.rs, and src/helpers/gmail/watch.rs to use these new providers, along with adding new test utilities and tests. Review comments highlight several issues: potential URL path traversal vulnerabilities in both the Pub/Sub subscription and Gmail watch cleanup operations due to unvalidated user input, code duplication of the FakeTokenProvider test utility, and a re-introduced bug in the Gmail watch helper where the gmail_token might still expire during long fetch_and_output_messages operations.
…r message - Move FakeTokenProvider to auth.rs behind #[cfg(test)] so both helper test modules share one definition. - Refresh the Gmail token per message inside fetch_and_output_messages to guard against expiry during large batches. - Update test expectations accordingly.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for refreshing OAuth2 tokens in long-running processes by using an AccessTokenProvider trait. The refactoring of the gmail +watch and events +subscribe helpers to use this provider is well-executed and addresses the critical issue of token expiration. The addition of constants for API base URLs and comprehensive tests, including mock servers, significantly improves the code's testability and reliability.
I've identified a couple of high-severity issues in the cleanup logic where a failure to refresh a token is not handled correctly, potentially misleading the user. Please see the detailed comments.
Move the success message inside the token-refresh guard and add an else branch that warns about potential orphaned resources.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an AccessTokenProvider trait and its implementation, ScopedTokenProvider, to enable refreshing OAuth access tokens for long-running operations. The Gmail watch and Workspace Events subscribe helpers are refactored to use these new token providers, ensuring that fresh access tokens are obtained before each Pub/Sub and Gmail API request, including during cleanup and for individual message fetches in Gmail. This prevents token expiration issues in long-running processes. New test helpers and integration tests were added to validate this token refreshing behavior.
Note: Security Review did not run due to the size of the PR.
A single history batch won't approach the token's 1-hour lifetime, so per-message refresh is unnecessary overhead. The outer watch_pull_loop already refreshes tokens each iteration for long-running resilience.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces an AccessTokenProvider trait and ScopedTokenProvider to enable long-running helpers (Gmail watch and Workspace Events subscribe) to refresh OAuth access tokens before each API request, addressing potential token expiration. The subscribe.rs and watch.rs helpers are updated to utilize this new token refresh mechanism for Pub/Sub and Gmail API calls, including cleanup operations. New tests were added to verify this behavior. However, the review comments highlight that the newly added tests for token refreshing in both subscribe.rs and watch.rs are insufficient. They currently use single tokens for multiple requests, failing to properly validate that tokens are indeed refreshed between calls. Additionally, a potential issue was identified in watch.rs where the Gmail token for fetching individual messages is only refreshed once per fetch_and_output_messages call, which could still lead to expiration if many messages are processed. This token should be refreshed before each individual message fetch for improved robustness.
etanase
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #407 +/- ##
==========================================
+ Coverage 64.40% 65.67% +1.26%
==========================================
Files 38 38
Lines 15584 15843 +259
==========================================
+ Hits 10037 10405 +368
+ Misses 5547 5438 -109 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Summary
AccessTokenProvidertrait andScopedTokenProviderinauth.rsso long-running helpers can request fresh tokens before each API call instead of holding a single token string that expires after ~1 hourgmail +watch(watch_pull_loop) andevents +subscribe(pull_loop) to accept a&dyn AccessTokenProviderand refresh tokens per-requestPUBSUB_API_BASE/GMAIL_API_BASEURL constants to enable test injectionTest plan
cargo test— 553 tests pass (3 new)cargo clippy -- -D warnings— cleantest_scoped_token_provider_uses_get_token— verifiesScopedTokenProviderdelegates toget_tokentest_pull_loop_refreshes_pubsub_token_between_requests— fake TCP server +FakeTokenProviderverify each Pub/Sub request uses a distinct fresh tokentest_watch_pull_loop_refreshes_tokens_for_each_request— same pattern for Gmail watch, verifying all 4 HTTP calls (pull, history, message fetch, ack) each get a fresh tokenCloses #392
🤖 Generated with Claude Code