Skip to content

fix(auth): strip redirect_uri from credential_key#5692

Open
doughayden wants to merge 2 commits into
google:mainfrom
doughayden:fix/credential-key-strip-redirect-uri
Open

fix(auth): strip redirect_uri from credential_key#5692
doughayden wants to merge 2 commits into
google:mainfrom
doughayden:fix/credential-key-strip-redirect-uri

Conversation

@doughayden
Copy link
Copy Markdown

Link to Issue or Description of Change

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

This change adds auth_credential.oauth2.redirect_uri = None to the OAuth2 strip block at the three call sites where credential hashing happens. redirect_uri is deployment configuration (which callback URL the auth server should redirect to), not part of the credential identity, so it should be excluded from the hash just as access_token, refresh_token, expires_at, and the other transient OAuth2 fields already are. Without this change, a credential minted under one deployment URL cannot be retrieved when the deployment moves to another.

Testing Plan

Unit Tests:

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

Three new tests, one per affected method. Each constructs two AuthCredential instances that differ only in redirect_uri and asserts the computed key is identical:

  • tests/unittests/tools/openapi_tool/openapi_spec_parser/test_tool_auth_handler.py::test_credential_key_is_stable_across_redirect_uri
  • tests/unittests/tools/openapi_tool/openapi_spec_parser/test_tool_auth_handler.py::test_legacy_credential_key_is_stable_across_redirect_uri
  • tests/unittests/auth/test_auth_config.py::test_credential_key_is_stable_across_redirect_uri
$ pytest tests/unittests/tools/openapi_tool/openapi_spec_parser/test_tool_auth_handler.py tests/unittests/auth/test_auth_config.py
======================== 14 passed, 8 warnings in 2.90s ========================

$ pytest tests/unittests/
================ 5740 passed, 2340 warnings in 86.64s (0:01:26) ================

Manual End-to-End (E2E) Tests:

A self-contained Runner-based reproduction is at https://github.com/doughayden/adk-issue-examples/tree/main/05-redirect_uri_in_credential_hash. The agent definition (agent.py) wires up an OpenAPIToolset against a local OAuth2 test server, configured with one redirect_uri value. main.py constructs an InMemoryRunner, seeds a real (non-expired) OAuth2 credential into session state under a different redirect_uri's hash, and runs the agent. The --apply-fix flag monkey-patches the proposed fix to demonstrate the resolution end-to-end.

Without the fix:

🌤️  WeatherAssistant Agent — redirect_uri-in-hash repro
============================================================
Proposed fix applied:      False
STORED_REDIRECT_URI:       http://localhost:8080/callback
CURRENT_REDIRECT_URI:      http://localhost:8081/callback

Hash keys produced by ToolContextCredentialStore.get_credential_key:
    STORED   → oauth2_55f666541ad22e39_oauth2_8ba0457897522d9d_existing_exchanged_credential
    CURRENT  → oauth2_55f666541ad22e39_oauth2_ae16199243c358df_existing_exchanged_credential
    ❌ Keys differ — credentials minted under STORED are not retrievable.

👤 User: What's the weather in San Francisco?
🌤️  Weather Assistant event stream:

    [function_call] get_weather by WeatherAssistant
    [auth_event] adk_request_credential by WeatherAssistant
    [function_response] get_weather by WeatherAssistant
    [text] WeatherAssistant: 'It seems I need your authorization to access weather data. Could you please g...'

Event counts:
    function_calls: 1
    auth_events: 1
    function_responses: 1
    text_events: 1

✅ Bug reproduced: agent emitted 1 adk_request_credential event(s) despite a valid seeded credential being present in state.

With the fix:

🌤️  WeatherAssistant Agent — redirect_uri-in-hash repro
============================================================
Proposed fix applied:      True
STORED_REDIRECT_URI:       http://localhost:8080/callback
CURRENT_REDIRECT_URI:      http://localhost:8081/callback

Hash keys produced by ToolContextCredentialStore.get_credential_key:
    STORED   → oauth2_55f666541ad22e39_oauth2_c2ad46dffd26cd87_existing_exchanged_credential
    CURRENT  → oauth2_55f666541ad22e39_oauth2_c2ad46dffd26cd87_existing_exchanged_credential
    ✅ Keys match — fix is taking effect at the hash level.

👤 User: What's the weather in San Francisco?
🌤️  Weather Assistant event stream:

    [function_call] get_weather by WeatherAssistant
    [function_response] get_weather by WeatherAssistant
    [text] WeatherAssistant: 'The weather in San Francisco is Clear with a temperature of 30 degrees Celsiu...'

Event counts:
    function_calls: 1
    auth_events: 0
    function_responses: 1
    text_events: 1

✅ Fix verified: tool call succeeded against the seeded credential without an adk_request_credential prompt.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

Scope:

The same strip block exists at three call sites and has the same gap at all three. This PR patches all three. Patching only the tool-level pair (tool_auth_handler.py) and leaving the framework-level path (auth_tool.py:AuthConfig.get_credential_key) would leave the bug reachable for any consumer that does not work around #5327 with get_auth_config = lambda: None. Patching only AuthConfig.get_credential_key and leaving the tool-level pair would leave the bug reachable on the standard tool-level credential lookup path.

Related:

- Add `redirect_uri = None` to OAuth2 strip block at 3 call sites
- tool_auth_handler.py: get_credential_key + legacy variant
- auth_tool.py: AuthConfig.get_credential_key
- redirect_uri is deployment config, not credential identity
- Add hash-stability tests asserting redirect_uri invariance
@rohityan rohityan added agent config [Component] This issue is related to the Agent Config interface and implementation request clarification [Status] The maintainer need clarification or more information from the author labels May 18, 2026
@rohityan
Copy link
Copy Markdown
Collaborator

Hi @doughayden , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please fix the failing mypy-diff tests before we can proceed with the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent config [Component] This issue is related to the Agent Config interface and implementation request clarification [Status] The maintainer need clarification or more information from the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth2 credential_key includes redirect_uri, breaking credential lookup across deployment URLs

2 participants