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.

[gemini-code-assist]

This block has a critical security flaw. If exchanger.exchange() fails, the client_secret on the credential object will remain un-redacted, leading to a secret leak. You must use a try...finally block to ensure the secret is always re-redacted. Additionally, the restored flag (set earlier) is unused and should be part of the re-redaction logic.

      try:
        exchanged_credential = await exchanger.exchange(
            credential, self._auth_config.auth_scheme
        )
      finally:
        # Always re-redact the secret on the original credential object if it was restored.
        if restored and credential.oauth2:
          credential.oauth2.client_secret = "<redacted>"

      # Redact client secret again after exchange to prevent leakage
      if exchanged_credential.oauth2:
        exchanged_credential.oauth2.client_secret = "<redacted>"

[gemini-code-assist]

Writing a sensitive access token directly to sys.stderr is a security risk, as stderr is often logged or monitored. This debug statement should be removed. If logging is necessary for debugging, use the logging module at the DEBUG level and avoid logging the token value itself.

[gemini-code-assist]

The original_secret variable is initialized here and assigned a value on line 58, but it is never used. It should be removed to improve code clarity and avoid confusion.

[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.

[gemini-code-assist]

The tests in this module modify the state of CredentialManager._CLIENT_SECRETS, which is a class-level variable. Since there is no setup/teardown logic to clear this dictionary between tests, state from one test can leak into another, making the test suite flaky and order-dependent. Please add a pytest fixture (e.g., an autouse fixture) to clear CredentialManager._CLIENT_SECRETS before each test runs.

[gemini-code-assist]

This block for restoring and re-redacting the client secret is very similar to the logic in CredentialManager._exchange_credential. This code duplication can lead to maintenance challenges and potential inconsistencies (in fact, the CredentialManager version has a bug that this version doesn't). Consider centralizing this logic into a single helper function or a context manager within CredentialManager that can be used in both places. This would improve consistency and robustness.

[gemini-code-assist]

The setUp method will not be automatically called by pytest for a class that does not inherit from unittest.TestCase. This can lead to state from CredentialManager._CLIENT_SECRETS being shared between tests, causing them to be flaky. Please use a pytest fixture for test setup to ensure the secret store is cleared before each test. You could, for example, define a module-level autouse fixture.

[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.

[gemini-code-assist]

This fallback logic introduces a critical security vulnerability. If the credential object being exchanged has a client_id that is different from the client_id in self._auth_config.raw_auth_credential, this code will proceed to use the secret associated with raw_auth_credential for the token exchange request of the credential object. Using a secret for one client with another client's ID is a severe security risk and will likely cause the token exchange to fail, or worse, lead to unintended access if the authentication server has lax validation.

It is much safer to rely only on the client_id from the credential being processed. The restore_client_secret context manager already correctly looks up the secret based on the client_id of the credential passed to it. If a secret is not found for that client_id, it is better to fail the operation than to silently use a potentially incorrect secret from another credential.

Suggested change

	# Determine if we need to fallback/lookup secret from raw credential
	secret_to_use = None
	if (
	credential.oauth2
	and credential.oauth2.client_id
	and credential.oauth2.client_id not in self._CLIENT_SECRETS
	and self._auth_config.raw_auth_credential
	and self._auth_config.raw_auth_credential.oauth2
	and self._auth_config.raw_auth_credential.oauth2.client_id
	in self._CLIENT_SECRETS
	):
	# Fallback to look up using raw credential client id
	secret_to_use = self._CLIENT_SECRETS[
	self._auth_config.raw_auth_credential.oauth2.client_id
	]
	with self.restore_client_secret(credential, secret=secret_to_use):
	with self.restore_client_secret(credential):