Fix: key the OpenAPI tool credential cache on scheme and credential identity - #864
Open
AmaadMartin wants to merge 4 commits into
Open
Fix: key the OpenAPI tool credential cache on scheme and credential identity#864AmaadMartin wants to merge 4 commits into
AmaadMartin wants to merge 4 commits into
Conversation
added 3 commits
August 9, 2026 05:39
The OpenAPI credential cache needs a digest it can put in a session-state key. node:crypto is aliased out of the browser bundle by core/build.js and crypto.subtle.digest is async and secure-context only, so SHA-256 is written out here over 32-bit arithmetic. canonicalJson sorts object keys and drops null and undefined members, so two structurally equal values digest the same.
…identity ToolContextCredentialStore keyed the cached credential on the scheme type alone, so a session had at most four slots. Two toolsets that both declare an oauth2 scheme shared one slot: whichever tool authenticated first won, and the second tool then sent the first tool's token to a different API. The key now carries a digest of the scheme and of the configured credential. Volatile OAuth2 fields such as redirectUri are stripped first, so a credential survives a re-consent and a change of callback URL. A credential stored under the old key is still found and is copied forward.
The new cases asserted on a spy for getAuthResponse. A real Context over a shared session state shows the same thing through the state it writes, and it removes the hand-built context doubles from the new cases.
…via Web Crypto Reading the pre-upgrade key re-created the leak this change closes. That key names only the scheme type, so on a miss two oauth2 tools both resolved to the one pre-upgrade slot and the migration then made the wrong owner permanent. A miss now costs one re-exchange, which is the right price for a cached credential whose owner cannot be established. accessToken and refreshToken are no longer stripped before digesting. A token the tool was configured with is the credential, so two tools holding different tokens need different slots. The PKCE fields codeVerifier and nonce are stripped instead: a consent round trip produces them. stableDigest now awaits crypto.subtle.digest instead of a hand-rolled SHA-256. The whole call chain was already async, so this costs three awaits and removes 115 lines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem:
ToolContextCredentialStorederives its session-state key from the security scheme type alone, so a session has at most four credential slots. Two OpenAPI toolsets that both declare anoauth2scheme share one slot: whichever tool authenticates first wins, and the second tool then sends the first tool's access token to a different API. TwoapiKeytools leak each other's key the same way. The collision is silent and it persists for the rest of the session.Solution: The key now carries a SHA-256 digest of the scheme and of the configured credential, so distinct
(scheme, credential)pairs never share a slot. Fields that a consent round trip produces, such asauthCodeandcodeVerifier, and the per-deploymentredirectUri, are stripped before digesting, so a credential survives a re-consent and a change of callback URL. A credential written under the old type-only key is not read, because that key cannot say which tool owns it.Design notes:
oauth2tools both resolve to the one pre-upgrade slot, and copying that value forward would make the wrong owner permanent. Ignoring it costs one re-exchange per in-flight session, which is the right price for a cached credential whose owner cannot be established. This deviates from adk-python, whose legacy key is itself digest-based and therefore unambiguous. Here the legacy key is the bug.accessTokenandrefreshTokencontribute to the key. A token the tool was configured with is the credential, not round-trip state, so two tools holding different tokens need different slots.codeVerifierandnonceare stripped instead, because a consent round trip produces them. This is a deliberate divergence from adk-python's field list.stableDigestawaitscrypto.subtle.digest. The call chain (prepareAuthCredentials->getCredential->getCredentialKey) is alreadyasync, so this costs threeawaits and no hand-rolled cryptography.ToolContextCredentialStoretakes a requiredauthScheme. The store is module-private and its only caller returns early when the scheme is absent, so an optional parameter would only add an unreachable, uncoverable branch.as unknown as InvocationContextcasts in the tests build the minimal invocation context aContextneeds, matchingtests/integration/tools/run_skill_inline_script_tool_test.ts:29. Noany, no@ts-expect-errorand no lint suppression is added anywhere in this change.Collision check:
gh pr list --repo AmaadMartin/adk-js --state open --limit 100returned three open PRs that touchtool_auth_handler.ts(#862 user-consent gating, #772 refresh on read, #774 widening the scheme parameter type). None of them changes the cache key, so this is not a duplicate. This PR is based onmainrather than stacked on one of them, because all three are concurrent and only one base can be chosen. The diff is confined to the key function and its two call sites, so it rebases cleanly whichever lands first.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:
Coverage of the new code, measured with
--coverage:core/src/utils/hash_utils.tsis at 100% of statements, branches, functions and lines.tool_auth_handler.tsreaches 100% branch coverage; its one uncovered line range is the pre-existingfromToolContextfactory, which this change does not touch.Every new test was run against unfixed code and failed:
git checkout main -- tool_auth_handler.tsexpected 'pre-upgrade-token' to be 'exchanged-token'andexpected [] to have a length of 2'accessToken'back on the strip listgives two tools configured with different access tokens their own cache slot:expected [ Array(1) ] to have a length of 2'redirectUri'from the strip listreuses the cached credential when only the redirect URI differs:expected [ …(2) ] to have a length of 1'codeVerifier'from the strip list.sort()incanonicalizestableDigest is equal for two key-order-permuted twinsnullmembers incanonicalizedrops undefined and null members at every depthfails'SHA-256'->'SHA-1'DIGEST_BYTES8 -> 6Three assertions encoded behaviour this change deliberately removes, so they were rewritten rather than added alongside:
should store exchanged credential in state and record it in the deltaandcaches a static credential that did require an exchangeasserted the literal old key ('apiKey_existing_exchanged_credential'/'oauth2_existing_exchanged_credential') as the write target. That literal is the defect. They now read the single key instate.toRecord()and assert its shape by regex, which pins both the scheme segment and the credential segment. The token assertions around them are unchanged.should return cached credential if availableseeded the pre-upgrade key and asserted a cache hit, which is the behaviour that has been removed. It is nowignores a credential stored under the pre-upgrade keyand asserts the opposite. Cache hits are still pinned byre-uses a credential persisted by a previous tool call instead of re-exchanging(expect(secondContext.getAuthResponse).not.toHaveBeenCalled()) and byreuses the cached credential when only the redirect URI differs.No test was skipped, disabled or weakened.
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
tests/integration/tools/openapi_credential_cache_test.tsautomates the reproduction with no mocks. It starts two local HTTP servers that echo the headers they receive, builds anOpenAPIToolsetagainst each, and calls both tools over real HTTP in one session. Againstmainthe second tool sends the first tool's credential (expected 'key-a' to be 'key-b'andexpected 'Bearer token-a' to be 'Bearer token-b'). With this change each tool sends its own.The OAuth2 client-credentials exchange itself is covered at unit level rather than end to end, because
fetchOAuth2Tokensblocks loopback and plain-HTTP token endpoints as SSRF protection, so a local token server cannot be reached.To reproduce by hand: mount two
OPENAPIToolsets in one agent, both with anoauth2client-credentials scheme but differenttokenUrland differentclientId/clientSecret. Invoke tool A, then tool B, and inspect theAuthorizationheader each API receives.Checklist
CI note: two pre-existing flaky tests failed before this branch went green on all three operating systems.
tests/integration/app_loader/app_loader_test.tstimed out on macOS, andcore/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdouttimed out on Windows. Neither touches this change, and the shell-executor test fails the same way on other branches of this fork.