Skip to content

Fix: default an OpenID Connect scheme with no grantTypesSupported to the authorization-code grant - #865

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/oidc-default-authorization-code-grant
Open

Fix: default an OpenID Connect scheme with no grantTypesSupported to the authorization-code grant#865
AmaadMartin wants to merge 2 commits into
mainfrom
fix/oidc-default-authorization-code-grant

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

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

  2. Or, if no issue exists, describe the change:

Problem: determineGrantType() selected the OpenID Connect grant by sniffing the grantTypesSupported property. OpenID Connect Discovery 1.0 section 3 makes that field OPTIONAL and defaults it to ["authorization_code", "implicit"] (https://openid.net/specs/openid-connect-discovery-1_0.html), so a spec-compliant provider that omits it got undefined. exchange() then logged Unsupported OAuth2 grant type: undefined and returned the credential unexchanged, with no token and no error.

Solution: Gate the OpenID Connect branch on the type discriminant and default to the authorization-code grant. grantTypesSupported now only ever selects CLIENT_CREDENTIALS. This matches adk-python _determine_grant_type() in src/google/adk/auth/exchanger/oauth2_credential_exchanger.py, which branches on the scheme type and defaults the same way. Parity wins here because "does an OIDC tool acquire a token" is observable across the language boundary. The source diff is 2 changed lines and a comment; the flows branch above it is untouched.

Overlap with #773. That PR refactors the same function to use a new isOpenIdConnectScheme() guard, and it also narrows the flows branch. It does not make this behaviour change. I first stacked this PR on it, then rebased onto main so the diff shows only this change. Whichever lands second needs a one-function textual merge. Collision check run before writing any code: gh pr list --state open --limit 100 plus gh pr diff --name-only on every adjacent auth PR (#772, #774, #845, #848, #862, #864). Only #773 touches oauth2_credential_exchanger.ts.

Behaviour change, no API change. Nothing is added to core/src/index.ts or core/src/common.ts. Two directions:

  1. Widening: an OIDC scheme with no grantTypesSupported now attempts a real token exchange. A caller who passes an incomplete credential and ignores the result now sees CredentialExchangeError where it previously saw a silent no-op. That is the intended trade. No error message changes.
  2. Narrowing: an untyped object that carries only grantTypesSupported no longer resolves to a grant type. It is not a valid OpenIdConnectWithConfig, which requires type: 'openIdConnect', so this reaches untyped JavaScript callers and hand-built test doubles only.

Blast radius for the new throw: AuthHandler.parseAndStoreAuthResponse() and ToolAuthHandler.prepareAuthCredentials() via AutoAuthCredentialExchanger. Neither is modified here.

Two existing fixtures corrected, in their own commit (5557d11). Both determineGrantType OpenIdConnect cases named themselves OpenIdConnect but passed {grantTypesSupported: [...]} as AuthScheme, with no type. They only passed because the function sniffed a property. Both assertions are unchanged, and that commit is green on its own. No test is deleted, skipped, or weakened.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

Five tests added: three for determineGrantType (omitted grantTypesSupported, empty grantTypesSupported, and an apiKey scheme to prove the branch did not widen) and two for exchange() (the token request now happens, and an incomplete credential now raises CredentialExchangeError).

npx vitest run --project unit:core core/test/auth   # 12 files, 179 tests passed
npm run build          # exit 0
npm run lint           # exit 0
npm run format:check   # exit 0

CI is green on all four run-tests jobs. The first windows-latest attempt failed on two integration tests that this diff does not touch (tests/integration/a2a/basic/a2a_agent_test.ts server startup, and a 40s timeout in tests/integration/app_loader/app_loader_test.ts). Both passed on re-run, and ubuntu-latest and macos-latest passed on the first attempt.

npm run ts:check reports pre-existing errors in unrelated test files (core/test/plugins/logging_plugin_test.ts, core/test/agents/instructions_test.ts and others). #764 tracks that. Neither changed file appears in the output.

Coverage of core/src/auth/oauth2/oauth2_credential_exchanger.ts under core/test/auth: 100% statements, 100% lines, 100% functions, 96% branches. The two uncovered branches are in the pre-existing authResponseUri state-check try/catch, outside this diff.

Proof the tests can fail. I restored the old gate ((authScheme as OpenIdConnectWithConfig).grantTypesSupported) and reran the file. Three tests failed:

× determineGrantType > returns AUTHORIZATION_CODE for an OpenIdConnect scheme that omits grantTypesSupported
  → expected undefined to be 'authorization_code'
× exchange > exchanges an authorization code for an OpenIdConnect scheme that omits grantTypesSupported
  → expected false to be true
× exchange > throws CredentialExchangeError ... when the credential is incomplete
  → promise resolved "{ credential: { oauth2: {} }, …(1) }" instead of rejecting

The empty-array test passes either way by design: [] is truthy, so it pins behaviour the fix does not change.

Manual End-to-End (E2E) Tests:
No E2E test is added. A real check needs a live OpenID Provider that omits grant_types_supported, and fetchOAuth2Tokens() rejects loopback and non-HTTPS endpoints, so no local server can stand in for one.

I ran the built package directly, with no mocks, against an OIDC scheme with no grantTypesSupported and a loopback token endpoint:

determineGrantType => authorization_code
exchange threw => CredentialExchangeError | Failed to exchange tokens: SSRF protection: OAuth2 token endpoint 'https://127.0.0.1:9/token' is not allowed. ...
incomplete threw => CredentialExchangeError | clientId, clientSecret, and either authCode or authResponseUri are required for authorization code exchange.

The SSRF rejection proves the token request is now issued. Before this change the same call logged Unsupported OAuth2 grant type: undefined and resolved with wasExchanged: false. To repeat it against a real provider, build an OpenIdConnectWithConfig from a discovery document with grant_types_supported deleted, and confirm a POST reaches the token endpoint.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 9, 2026 06:34
…real scheme

Both cases named themselves OpenIdConnect but passed a bare object with no
type discriminant, so they were not valid OpenIdConnectWithConfig values.
Both assertions are unchanged.
…orization_code

OpenID Connect Discovery 1.0 section 3 makes grant_types_supported OPTIONAL
and defaults it to ["authorization_code", "implicit"]. determineGrantType()
sniffed that field instead of the scheme type, so a provider that omits it
returned undefined, exchange() logged "Unsupported OAuth2 grant type:
undefined" and no token was ever requested. Gate the branch on the type
discriminant, which also matches adk-python _determine_grant_type().
@AmaadMartin
AmaadMartin force-pushed the fix/oidc-default-authorization-code-grant branch from 342e46e to 9130d19 Compare August 9, 2026 13:35
@AmaadMartin
AmaadMartin changed the base branch from fix/oauth2-grant-type-scheme-guards to main August 9, 2026 13:35
@AmaadMartin AmaadMartin closed this Aug 9, 2026
@AmaadMartin AmaadMartin reopened this Aug 9, 2026
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.

1 participant