Skip to content

Fix: discriminate auth schemes by type in determineGrantType - #773

Open
AmaadMartin wants to merge 4 commits into
mainfrom
fix/oauth2-grant-type-scheme-guards
Open

Fix: discriminate auth schemes by type in determineGrantType#773
AmaadMartin wants to merge 4 commits into
mainfrom
fix/oauth2-grant-type-scheme-guards

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 7, 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):

None.

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

Problem: determineGrantType() decided which OAuth2 grant type a scheme
describes by looking for a flows or a grantTypesSupported property, then
asserting the value to OpenIdConnectWithConfig. An http or apiKey scheme
that carries either property name was classified as OAuth2 or OpenID Connect,
and OAuth2CredentialExchanger.exchange() then ran a real token exchange
against it. getTokenEndpoint() in the same feature reads tokenEndpoint
through the same kind of cast. The assertions are what hid all of this from the
compiler.

Solution: determineGrantType() now branches on the OpenAPI type
discriminant. The OAuth2 arm needs no helper, because SecuritySchemeObject is
a discriminated union and authScheme.type === 'oauth2' narrows on its own.
The OIDC arm does need one, since plain narrowing does not reach the
configuration fields, so this adds isOpenIdConnectScheme narrowing to
OpenIdSecurityScheme & Partial<OpenIdConnectWithConfig> — what the check
actually proves. getTokenEndpoint() uses the same guard, which removes the
last two casts.

Why the guard is Partial

OpenIdConnectWithConfig declares authorizationEndpoint and tokenEndpoint
as required, but a type === 'openIdConnect' check proves neither. A predicate
of scheme is OpenIdConnectWithConfig would therefore be an as in disguise:
it hands the caller a tokenEndpoint: string that is absent at runtime. The
Partial intersection keeps type and openIdConnectUrl required and makes
the configuration fields optional, so the compiler enforces the caveat instead
of a doc comment asking the reader to remember it.

Verified, with the guard applied and the field read unchecked:

core/test/auth/soundness_probe.ts(5,12): error TS18048: 'scheme.tokenEndpoint' is possibly 'undefined'.

That line compiled clean under a scheme is OpenIdConnectWithConfig predicate
and threw at runtime.

Behaviour change

Every value that satisfies the AuthScheme union keeps its old result. No
union member can reach a changed row without an as assertion, so no
correctly-typed caller in the repository can regress. getTokenEndpoint() is
unchanged in behaviour; only its casts are gone.

input before after
{type: 'oauth2', flows: {clientCredentials: …}} CLIENT_CREDENTIALS same
{type: 'oauth2', flows: {authorizationCode: …}} AUTHORIZATION_CODE same
{type: 'oauth2', flows: {implicit: …}} IMPLICIT same
{type: 'oauth2', flows: {password: …}} PASSWORD same
{type: 'oauth2', flows: {}} undefined same
{type: 'openIdConnect', …, grantTypesSupported: ['client_credentials']} CLIENT_CREDENTIALS same
{type: 'openIdConnect', …, grantTypesSupported: ['authorization_code']} AUTHORIZATION_CODE same
{type: 'openIdConnect', …} with no grantTypesSupported undefined same
{type: 'apiKey' | 'http' | …} with no stray fields undefined same
{type: 'http' | 'apiKey' | …} carrying flows grant from flows undefined
{type: 'http' | 'apiKey' | …} carrying grantTypesSupported OIDC grant undefined
object with no type, carrying either field grant undefined

The last three rows are the fix. A caller that passes an untyped scheme object
today now gets undefined and the existing Unsupported OAuth2 grant type
warning from logger.warn. That is deliberate.

Scope

&& authScheme.flows and && authScheme.grantTypesSupported are both kept, so
the classifier moves one thing only: the discriminant. The resulting divergence
from adk-python, which defaults an OIDC scheme with no advertised grant types
to AUTHORIZATION_CODE, is left alone. A new test pins the current undefined
so a later change to it has to be deliberate.

getTokenEndpoint() was going to be a follow-up. I folded it in because it is
two lines, it carries the same defect, and it is what gives the exported guard
a second caller. auth_handler.ts also discriminates structurally and is
untouched; PR #691 is already in that file.

The guard is internal. It is not added to common.ts or index.ts.

Collision check

I listed every open PR on this fork and read the changed-file list of all of
them. None touches auth_schemes.ts, oauth2_credential_exchanger.ts or
oauth2_utils.ts. The nearest neighbours are #741 (types in
core/test/auth/exchanger/credential_exchanger_test.ts), #772
(tool_auth_handler.ts) and #691 (auth_handler.ts).

Testing Plan

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

Why existing fixtures were edited

The repository convention is to add a test rather than rewrite one. Eight
fixtures in oauth2_credential_exchanger_test.ts are the documented exception,
so I am naming them rather than slipping them in. They used as AuthScheme to
compile objects that no member of the AuthScheme union accepts: a flows or
grantTypesSupported field and no type. They encoded the bug as the
contract.

No assertion was touched. Only the fixture literals changed, and four of the
eight cannot pass at all once the discriminant is checked. The commit split is
the proof: b8c06286 retypes the fixtures and touches no source file, and the
suite stays green against the unfixed determineGrantType; 223f26a2 makes
the behaviour change. No test was deleted, skipped or weakened.

6903ae99 puts back the one case the retyping did drop. The old
{} as AuthScheme fixture became a valid apiKey scheme, which left the
untyped-object path — precisely the input whose behaviour changed — uncovered.
Three cases now cover it, alongside the apiKey scheme rather than instead of
it. They use as AuthScheme deliberately, because the input under test is data
that does not satisfy the union.

The {} as AuthScheme fixtures in the exchangeClientCredentials and
exchangeAuthorizationCode describes are untouched. Those functions never call
determineGrantType and getTokenEndpoint is mocked for them.

Proof that the new tests can fail

Mutation 1 — restore the original structural body of determineGrantType:

× returns undefined for a non-OAuth2 scheme that happens to carry flows
  → expected 'client_credentials' to be undefined
× returns undefined for a non-OIDC scheme that happens to carry grantTypesSupported
  → expected 'client_credentials' to be undefined
× returns undefined for an untyped object that carries flows
  → expected 'client_credentials' to be undefined
× returns undefined for an untyped object that carries grantTypesSupported
  → expected 'client_credentials' to be undefined

Tests  4 failed | 28 passed (32)

The other 28 pass under the mutation, which is the same evidence from the other
side: the change is behaviour-preserving for every well-typed scheme.

Mutation 2 — make the guard structural again
(return 'grantTypesSupported' in scheme):

× isOpenIdConnectScheme > accepts a scheme whose type is openIdConnect
× isOpenIdConnectScheme > accepts an openIdConnect scheme that carries no configuration fields
× isOpenIdConnectScheme > rejects an http scheme that carries a grantTypesSupported property
× getTokenEndpoint > returns tokenEndpoint from OpenIdConnectWithConfig

Tests  4 failed | 30 passed (34)

Mutation 3 — make getTokenEndpoint return the wrong OIDC field
(return authScheme.openIdConnectUrl):

× getTokenEndpoint > returns tokenEndpoint from OpenIdConnectWithConfig
  → expected undefined to be 'https://example.com/token'
× getTokenEndpoint > returns undefined for an openIdConnect scheme with no tokenEndpoint
  → expected 'https://example.com/.well-known/openi…' to be undefined

Tests  2 failed | 53 passed (55)

Coverage

Measured on the three changed source files:

File                        | % Stmts | % Branch | % Funcs | % Lines | Uncovered
auth_schemes.ts             |     100 |      100 |     100 |     100 |
oauth2_credential_exchanger |     100 |    95.91 |     100 |     100 | 169,177
oauth2_utils.ts             |   97.84 |     93.1 |     100 |   97.84 | 26-27

Both gaps are pre-existing and outside this change. Lines 169 and 177 are in
exchangeAuthorizationCode. Lines 26-27 are the flows.password || flows.implicit fallbacks in getTokenEndpoint; on main the same file
measures 97.89% statements and the same 93.1% branch with the same two lines
uncovered, so this change is coverage-neutral there.

One branch in the new code is not reachable from a valid input: the false arm
of && authScheme.flows, because flows is statically required on
OAuth2SecurityScheme. The guard is deliberate. It protects
getOAuthGrantTypeFromFlow from a deserialized {"type": "oauth2"} arriving
from an OpenAPI document or a YAML agent config, and the three untyped-input
tests exercise the same defensive posture one level up. I did not write a
cast-based fixture to reach it and I did not add a coverage suppression.

Commands

Run on the pushed commit:

npx vitest run --project unit:core \
  core/test/auth/auth_schemes_test.ts \
  core/test/auth/oauth2/oauth2_credential_exchanger_test.ts \
  core/test/auth/oauth2/oauth2_utils_test.ts \
  core/test/auth/auth_handler_test.ts
  → Test Files 4 passed (4), Tests 83 passed (83)

npm run build         → OK
npm run lint          → clean
npm run format:check  → clean
npm run ts:check      → 287 errors repo-wide, identical to the count before
                        this branch, and none in the files I changed

auth_handler_test.ts is a regression check on the other auth/ consumer of
AuthScheme. It needed no edit.

One CI note

run-tests is green on ubuntu, macOS and Windows. Getting there took two
re-runs, both lost to
tests/integration/app_loader/app_loader_test.ts > should discover apps vs agents across directories and standalone files, which timed out at 40000ms.
That test is unrelated to this change and it is already borderline: on the
Windows leg of #772, where it passed, it took 37358ms of its 40000ms budget.
PRs #610, #652 and #664 are fixing it. I did not touch it.

Manual End-to-End (E2E) Tests:

Not applicable. determineGrantType is a pure classifier with no CLI surface,
no server route, and no user-visible change for a correctly-typed scheme. To
reproduce the bug on main instead:

import {determineGrantType} from './core/src/auth/oauth2/oauth2_credential_exchanger.js';

determineGrantType({
  type: 'http',
  scheme: 'bearer',
  flows: {
    clientCredentials: {tokenUrl: 'https://example.com/token', scopes: {}},
  },
});
// main:      OAuthGrantType.CLIENT_CREDENTIALS
// this PR:   undefined

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 4 commits August 7, 2026 05:51
Eight fixtures in the OAuth2 credential exchanger test used `as AuthScheme`
to compile objects that no member of the AuthScheme union accepts: they
carried a `flows` or `grantTypesSupported` field and no `type` discriminant.
Retyping them to `const authScheme: AuthScheme = {...}` removes the
assertion and drops the compiler's approval back in.

The suite stays green against the unchanged source, which shows the fixtures
were only mistyped.

The `{} as AuthScheme` fixtures in the exchangeClientCredentials and
exchangeAuthorizationCode describes are left alone. Those functions never
call determineGrantType and getTokenEndpoint is mocked for them.
determineGrantType() decided which OAuth2 grant type a scheme describes by
looking for a `flows` or a `grantTypesSupported` property, then asserting the
value to OpenIdConnectWithConfig. An `http` or `apiKey` scheme that carries
either property name was classified as OAuth2 or OpenID Connect, and the
exchanger ran a real token exchange against it.

The function now branches on the OpenAPI `type` discriminant through two new
type guards, isOAuth2Scheme and isOpenIdConnectWithConfigScheme. This matches
adk-python, which branches on isinstance. Both unchecked assertions are gone.

Every scheme that satisfies the AuthScheme union keeps its old result, so no
correctly-typed caller changes behaviour. A scheme with a foreign `type` now
returns undefined, and the exchanger logs the existing warning.

The guards are internal. They are not added to common.ts or index.ts.
Three changes to the guard surface added by the previous commit.

isOAuth2Scheme is gone. SecuritySchemeObject is a union discriminated on a
literal `type`, so `authScheme.type === 'oauth2'` already narrows to
OAuth2SecurityScheme with no predicate. getTokenEndpoint in the same feature
has always relied on that. The helper bought nothing.

The OIDC predicate was unsound. OpenIdConnectWithConfig declares
authorizationEndpoint and tokenEndpoint as required, but the check is on
`type` alone, so the predicate handed callers a `tokenEndpoint: string` that
can be absent at runtime, and a JSDoc paragraph asked them to remember. The
narrowed type is now
`OpenIdSecurityScheme & Partial<OpenIdConnectWithConfig>`, which is what the
check actually proves, so the compiler enforces the caveat and the prose is
deleted. Reading `scheme.tokenEndpoint.length` after the guard is now
TS18048 instead of a runtime TypeError. The name loses `WithConfig` because
the guard no longer asserts one.

getTokenEndpoint now uses the guard. Its two
`(authScheme as OpenIdConnectWithConfig)` casts read a field the type never
verified, which is the defect this branch exists to remove, and they sat one
function away from it. Removing them also gives the exported guard a second
caller.
Retyping the fixtures replaced the `{} as AuthScheme` case with a valid
apiKey scheme, which left the untyped-object path untested. That path is
exactly what the `type` discriminant changed: before the fix an object with
no `type` but a `flows` or `grantTypesSupported` field was classified as
OAuth2 or OpenID Connect.

Three cases cover it now, alongside the apiKey scheme rather than instead of
it. They use `as AuthScheme` on purpose, because the input under test is data
that does not satisfy the union.
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