Skip to content

fix(mcp): keep empty OAuth2 token on its own branch in McpTool - #7647

Open
CorgiBoyG wants to merge 1 commit into
google:mainfrom
CorgiBoyG:fix/mcp-oauth-branch-consistency
Open

CorgiBoyG wants to merge 1 commit into
google:mainfrom
CorgiBoyG:fix/mcp-oauth-branch-consistency

Conversation

@CorgiBoyG

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

Problem:

McpTool._get_headers() uses if credential.oauth2 and credential.oauth2.access_token:. When an AuthCredential carries an OAuth2 object with an empty/None access_token and an HTTP credential on the same object, that condition is falsy, so control falls through to elif credential.http: and emits an Authorization header from the HTTP field.

The canonical discovery helper build_auth_headers() (used by McpToolset) keeps the credential on the OAuth2 branch and emits no header for an empty token. The two paths therefore resolve the same credential to different Authorization headers. Both feed _merge_auth_and_provider_headers, and the MCP session pool key is derived from the merged headers, so discovery and invocation can bind to different pooled sessions — the path-consistency concern behind #7548 / #7590.

This mixed shape is reachable: a custom auth provider may return such a credential (the non-consent path in CredentialManager returns it without clearing fields). It is a narrow edge, not the default OAuth2/HTTP flow.

Solution:

Restore the nested structure so an OAuth2 credential stays on its own branch and an empty token simply emits no header, mirroring build_auth_headers():

if credential.oauth2:
    if credential.oauth2.access_token:
        headers = {"Authorization": f"Bearer {credential.oauth2.access_token}"}
elif credential.http:
    ...

The change is limited to the OAuth2 guard plus one regression test for the mixed-credential case.

Testing Plan

Unit Tests:

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

The new test fails on current main with:

Expected: create_session(headers={'Authorization': 'Bearer provider-token'})
Actual:   create_session(headers={'Authorization': 'Bearer stale-http-token'})

After the fix:

pytest -q -p no:cacheprovider tests/unittests/tools/mcp_tool
442 passed

pytest -q -p no:cacheprovider tests/unittests/tools tests/unittests/auth
3556 passed, 1 skipped

I also compared the invocation path (McpTool._get_headers) against the discovery path (build_auth_headers) across 15 OAuth2/HTTP credential combinations: 0 mismatches after the fix (4 before).

Static checks:

pyink --check: passed
ruff check: passed
git diff --check: passed

The full repository test suite was not run.

Manual End-to-End (E2E) Tests:

Not run. The regression test asserts the final headers passed to create_session().

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.
  • Relevant MCP and auth unit tests pass locally.
  • I have manually tested my changes end-to-end.
  • No downstream dependency changes are required.

@Somay-kousis Somay-kousis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran this locally. No blockers found.

What I ran (PR head d1712a3, merge base 1daa0ec)

  • New test test_run_async_impl_empty_oauth_does_not_fall_through_to_http: passes on head. With only mcp_tool.py reverted to the merge base it fails with Expected: create_session(headers={'Authorization': 'Bearer provider-token'}), Actual: create_session(headers={'Authorization': 'Bearer stale-http-token'}).
  • tests/unittests/tools/mcp_tool: 442 passed on head.
  • My own comparison of McpTool._get_headers against build_auth_headers across 23 credential shapes: OAuth2 in {absent, access_token=None, access_token="", token set} crossed with HTTP in {absent, bearer, empty bearer, basic, bearer plus additional_headers, custom scheme}. Head: 0 mismatches. Merge base: 8 mismatches, all of them OAuth2 with a None or empty token plus any usable HTTP credential, where invocation sent the HTTP header and discovery sent none.

Checked, no issues

  • src/google/adk/tools/mcp_tool/mcp_tool.py:862-869 now has the same branch shape as src/google/adk/auth/_auth_headers.py (if credential.oauth2: then the token check inside).
  • Existing callers: the fall-through only came in with #7590 (dc78d0a). No tag in my clone contains that commit, so no released version relied on falling back to the HTTP field. Before #7590 this branch sent Bearer None instead.
  • With a valid OAuth2 token, OAuth2 still wins over any HTTP field, same as before.

Nit

  • The test covers access_token=None (tests/unittests/tools/mcp_tool/test_mcp_tool.py:1952) but not access_token="". Both go down the same path today (my comparison above covers it), so a parametrize over [None, ""] would just pin it.

Not checked

  • End to end against a real MCP server or the session pool.
  • The full tests/unittests/tools and tests/unittests/auth suites (the PR reports 3556 passed there).

McpTool._get_headers used `if credential.oauth2 and
credential.oauth2.access_token`, so an OAuth2 credential with an empty
token fell through to the `elif credential.http` branch and emitted an
Authorization header from an unrelated HTTP field on the same object.

The discovery helper build_auth_headers keeps the credential on the
OAuth2 branch and emits no header for an empty token. Restore the nested
structure here so both paths resolve the same credential to the same
header and therefore the same pooled MCP session.

Fixes google#7646
@CorgiBoyG
CorgiBoyG force-pushed the fix/mcp-oauth-branch-consistency branch from d1712a3 to 14f104b Compare October 22, 2026 06:06
@CorgiBoyG

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review and the independent 23-shape comparison, @Somay-kousis — much appreciated.

I took the nit: the regression test is now parametrized over access_token in [None, ""] so the empty-string case is pinned alongside None. Production code is unchanged (the OAuth2 guard in mcp_tool.py is identical); only the test gained the "" parameter.

Re-verified on the updated commit:

  • tests/unittests/tools/mcp_tool: 443 passed
  • pyink / ruff / git diff --check: passed

The earlier triage check failure was an infra-side 503 UNAVAILABLE from the model provider, unrelated to the change; re-running it now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP invocation falls through empty OAuth2 token to HTTP field, diverging from discovery

3 participants