Skip to content

Fix: degrade instead of throwing when an OAuth2 credential exchange fails - #954

Open
AmaadMartin wants to merge 8 commits into
mainfrom
fix/oauth2-exchanger-degrade-on-failure
Open

Fix: degrade instead of throwing when an OAuth2 credential exchange fails#954
AmaadMartin wants to merge 8 commits into
mainfrom
fix/oauth2-exchanger-degrade-on-failure

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 11, 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):
    N/A
  2. Or, if no issue exists, describe the change:
    Problem: The OAuth2 credential exchanger rejected on every failure, so a 503 from the token endpoint aborted the whole invocation. None of its three callers catches. adk-python returns the original credential with was_exchanged=False and the run continues, and the sibling OAuth2CredentialRefresher in this repo already degrades the same way.

Solution: Each failure that depends on the remote server or on runtime credential data now logs and returns {credential, wasExchanged: false}. Two conditions still throw CredentialExchangeError: a missing authScheme, which is a programmer error, and a detected state mismatch, which is a tampering signal. ToolAuthHandler no longer caches a credential that still needs a token, otherwise one transient failure would strand a tool on a token-less credential for the whole session.

Notes for the reviewer:

  • Behaviour change for external callers. This aligns adk-js with adk-python. Code that wrapped exchange() in a try/catch to detect a failed exchange should check wasExchanged instead. No signature, type or export changed.
  • A rejected token endpoint still throws. fetchOAuth2Tokens() now throws OAuth2EndpointNotAllowedError when the SSRF guard rejects an endpoint, and both catches rethrow it. A tokenUrl pointing at a link-local or loopback address is a configuration or tampering signal, so it must not look like a 503 and let the tool proceed unauthenticated. This is a deliberate deviation from the spec, which asked for a log and a degrade there. Its type guard matches on the error name rather than instanceof, so it still holds when a runtime has loaded two copies of the package.
  • Nine existing tests were rewritten, in their own commit (34b99476). They asserted the rejection that this change deliberately replaces. The two cases that must keep rejecting, throws CredentialExchangeError if authScheme is missing and throws CredentialExchangeError if state in authResponseUri does not match expected state, are untouched.
  • The state comparison moved out of the try block. It previously sat inside, so its own catch rewrapped a detected CSRF attempt as Failed to parse authResponseUri for state validation.
  • Log safety. No degrade message interpolates the credential, the client secret, the authorization code or authResponseUri. Only the error's message is logged, and Node's URL error message is the constant Invalid URL.
  • Collision check. gh pr list --repo AmaadMartin/adk-js --state open --limit 300 plus gh pr diff --name-only on every adjacent PR. No open PR implements this change. Fix: discriminate auth schemes by type in determineGrantType #773 edits the same file but only determineGrantType(), which this change does not touch, so this branch starts from main.
  • Out of scope, as specified: the missing oauth2 branch in applyCredential(), and ToolAuthHandler not catching ServiceAccountCredentialExchanger throws.

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.

npx vitest run --project unit:core core/test/auth core/test/tools/openapi_tool — 21 files, 296 tests passed. This includes the untouched oauth2_credential_refresher_test.ts and auth_preprocessor_test.ts.

npm run lint, npm run format:check — clean. npm run ts:check reports 47 files with errors, the same 47 as on main; none of them is a file in this diff.

Coverage of the three changed source files, measured with --coverage.include on those files: oauth2_credential_exchanger.ts and tool_auth_handler.ts are at 100% of statements, lines and functions, with 98.18% and 95.65% of branches; oauth2_utils.ts is at 98.13%, with the new error class and its guard fully covered. Three branches fall short. One is the non-Error arm of the new URL() catch, which cannot run because URL only throws TypeError. The other two are pre-existing lines in getCredentialKey() and getTokenEndpoint() that this change does not touch.

Proof that the new tests fail against the unfixed code. Four mutations, each reverted afterwards:

  1. Restore throw new CredentialExchangeError in the fetchOAuth2Tokens catch of exchangeAuthorizationCode(). Six tests fail, including the end-to-end one: RestApiTool > resolves with the API response when the OAuth2 token exchange fails fails with Error: Failed to exchange tokens: Token request failed with status 503, and both AuthHandler cases fail with promise rejected "Error: Failed to exchange tokens: Token r…" instead of resolving.
  2. Move the state comparison back inside the try. Two tests fail with AssertionError: promise resolved "{ …(2) }" instead of rejecting.
  3. Remove the externalExchangeRequired() gate from the cache write. does not cache an OAuth2 credential whose exchange failed fails on expect(context.state.get('oauth2_existing_exchanged_credential')).toBeUndefined(). The other two cases still pass, which shows the gate does not widen into the paths it must not touch.
  4. Remove the OAuth2EndpointNotAllowedError rethrow from both catches. Both rethrows when the SSRF guard rejects the token endpoint cases fail with AssertionError: promise resolved "{ …(2) }" instead of rejecting.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

core/test/tools/openapi_tool/rest_api_tool_test.ts covers this without mocks of ADK code: a RestApiTool with an oauth2 scheme drives the real ToolAuthHandler and the real exchanger, globalThis.fetch answers 503 at the token endpoint and 200 at the API, and runAsync() resolves with the API response. The outgoing API request carries no Authorization header, which matches adk-python today.

To reproduce by hand, point an oauth2 tool's authorizationCode.tokenUrl at an unreachable host and run the tool. Before this change the call rejected. Now it logs Failed to fetch OAuth2 tokens: <message> once and returns the API's own response.

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 8 commits August 11, 2026 12:12
The OAuth2 exchanger rejected on every failure of the remote token endpoint,
so a 503 aborted the whole invocation. adk-python returns the original
credential with was_exchanged=False and lets the run continue.

Each remote or runtime failure now logs and returns
{credential, wasExchanged: false}. A missing authScheme and a detected state
mismatch still throw CredentialExchangeError. The state comparison moved out
of the try block so a mismatch is no longer rewrapped as a parse failure.
…ract

Nine cases asserted the rejection that the previous commit deliberately
replaced. Each now asserts the degrade contract: wasExchanged is false, the
result carries the same credential object, and it holds no access token. A
guard case also asserts that no token request went out.

The two cases that must keep rejecting -- a missing authScheme and a state
mismatch -- are unchanged.
…rade

Adds three cases: an unparseable authResponseUri degrades before the mismatch
comparison runs, a state mismatch carries the mismatch message and not the
parse-failure message, and exchange() still degrades through its delegation to
the authorization code path.
With the exchanger degrading, a failed exchange returned a credential with no
access token. ToolAuthHandler cached it, and the read path returns a cached
credential verbatim as 'done', so one transient token endpoint error stranded
the tool for the rest of the session.

The store is now gated on externalExchangeRequired(), ported from adk-python.
The predicate is false for apiKey, http and serviceAccount credentials, so
they are still cached as before.
AuthHandler.parseAndStoreAuthResponse() now resolves and stores the
unexchanged credential for both an oauth2 and an openIdConnect scheme, with
only fetchOAuth2Tokens stubbed so the real exchanger runs.

RestApiTool.runAsync() gets the end-to-end case: the token endpoint answers
503, the API answers 200, and the call resolves with the API response instead
of rejecting.
Closes the last uncovered branch in the state read: a response that carries no
state parameter must not be treated as matching.
The degrade swallowed the SSRF guard as well as remote failures, so a tokenUrl
pointing at a link-local address looked exactly like a 503 and the tool went on
unauthenticated. A disallowed endpoint is a configuration or tampering signal,
so it belongs with the state mismatch on the throwing side.

fetchOAuth2Tokens now throws OAuth2EndpointNotAllowedError, and both catches
rethrow it. Its type guard matches on the error name rather than instanceof, so
it still holds when a runtime loads two copies of the package.

Both catches also stop logging: fetchOAuth2Tokens already logs the failure with
its endpoint context, so every network error produced two lines for one event.
tool_auth_handler_test.ts already owns the AutoAuthCredentialExchanger mock, so
a second file re-established it for no reason. The three cases and the real
Context helper move across unchanged; the file's mock now shares one hoisted
exchange function, which lets a case set its own result without a cast.
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