fix #3845

[guillaumeblaquiere]

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

• Closes: Security Critical: Client_secret is shared in OAuth2 flow with authenticated function tool #3845

Problem:
The client_secret was leaked and shared with the client

Solution:
I was unable to solve the issue myself. I vibe coded it with Antigravity.
I checked the code, it looks good to me.

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

Manual End-to-End (E2E) Tests:

Manual test is OK

[gemini-code-assist]

Summary of Changes

Hello @guillaumeblaquiere, I'm Gemini Code Assist¹! 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 addresses a critical security concern by implementing a robust system to prevent the leakage of client secrets. It centralizes the secure, in-memory storage of these secrets, redacts them from configuration objects, and only exposes them transiently during necessary authentication flows, significantly enhancing the overall security posture of the system's authentication handling.

Highlights

• Client Secret Redaction and Secure In-Memory Storage: Implemented a new mechanism to prevent client secret leakage by redacting secrets from AuthConfig objects and storing them securely in an in-memory map within the CredentialManager.
• On-Demand Client Secret Restoration: Modified the OAuth2 credential exchange and URI generation processes to temporarily retrieve the client secret from secure storage only when needed for authentication operations, ensuring it is re-redacted immediately after use.
• Credential Key Normalization: Updated the logic for generating credential keys to normalize the client secret field to None, which ensures that the key remains stable and consistent regardless of whether the secret is currently redacted or not.
• Test Suite Enhancements: Refactored mock creation in unit tests for CredentialManager to use a new helper function, create_auth_config_mock, and ensured proper mocking of oauth2 attributes on AuthCredential mocks to align with the new secret handling logic.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[adk-bot]

Response from ADK Triaging Agent

Hello @guillaumeblaquiere, thank you for your contribution!

To help the reviewers better understand and verify this important security fix, could you please provide logs or a screenshot demonstrating that the client_secret is no longer leaked after your changes are applied?

This will help speed up the review process. Thanks!

[gemini-code-assist]

Code Review

This pull request aims to prevent a client secret leak by redacting it and storing it in memory. While the approach is generally correct, the implementation introduces a critical security vulnerability where a secret could be left un-redacted if an error occurs during an OAuth token exchange. Additionally, there's a high-severity issue where sensitive access tokens are leaked to stderr for debugging. I've also included several medium-severity comments to improve code quality, efficiency, and maintainability. It is crucial to address the security flaws before merging.

[guillaumeblaquiere]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses a critical security vulnerability by preventing client secrets from being leaked. The approach of redacting the secret and storing it in memory on the server-side is sound. The changes are extensive and include new logic in CredentialManager and AuthHandler, as well as new tests to cover the secret handling.

My review focuses on ensuring the new logic is correct, robust, and maintainable. I've identified a critical bug in the secret re-redaction logic, some code duplication that should be addressed, and issues in the new tests that could lead to flakiness. Please see my detailed comments below.

[guillaumeblaquiere]

/gemini review

[gemini-code-assist]

Code Review

This pull request aims to fix a critical security issue where the client_secret was being leaked. The approach of redacting the secret upon CredentialManager initialization and using a context manager (restore_client_secret) to temporarily restore it for API calls is excellent. The addition of new unit tests to cover this new secret handling logic is also a great improvement. However, I've identified a critical security vulnerability in the new fallback logic within the _exchange_credential method. This logic could lead to using a secret for one client with another client's ID, which must be fixed. My review includes a specific comment with a suggested fix for this issue.