From fa8950f644caacb3aaf440b78d20d8716f6c4ffa Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Mon, 31 Aug 2026 00:18:43 +0530 Subject: [PATCH 1/7] fix(auth): stop OAuth2 client_secret and tokens from leaking over /run, /run_sse, /run_live When a tool requires OAuth2 authentication, ADK attaches the credential to an `adk_request_credential` function call so the client can complete the interactive auth flow. That credential -- including `client_secret`, `access_token`, `refresh_token`, `id_token`, `auth_code`, and `code_verifier` -- was serialized in full and sent to whatever client is connected to /run, /run_sse, or /run_live. These fields are already marked `Field(repr=False)` in `AuthCredential`, but `repr=False` only affects `repr()`/`str()` output (logs, error strings); it has no effect on `model_dump()`/`model_dump_json()`, which is what actually leaves the process in these three responses. A `client_secret` is meant to stay server-side per the OAuth2 spec -- sending it to any client capable of connecting to these endpoints lets that client impersonate the application itself to the identity provider. The fix has to happen at the network-serialization boundary rather than by excluding the fields on the model or by redacting the event before it's returned from the agent run: `FunctionCall.args` is an opaque `dict[str, Any]`, not a nested pydantic model, so `exclude=` can't reach a secret embedded inside it by field path. And later turns reconstruct the original request's credential by re-parsing the persisted `adk_request_credential` call's args, so anything stripped before `SessionService.append_event` would also be unrecoverable for that mechanism. Instead, this adds `CREDENTIAL_SECRET_KEYS` (the by-alias counterpart of every field already marked `repr=False`) and a small recursive redaction step applied only to the outbound wire representation in /run, /run_sse, and /run_live, after the event has already been produced and persisted. Adds a consistency test asserting `CREDENTIAL_SECRET_KEYS` can't drift from the set of `repr=False` fields, and an end-to-end /run_sse test confirming a credential's secret fields are absent from the streamed response while the fields a client legitimately needs (client_id, the authorization URL, the credential key) are preserved. --- src/google/adk/auth/auth_credential.py | 25 ++++ src/google/adk/cli/api_server.py | 62 ++++++++-- tests/unittests/auth/test_auth_credential.py | 33 ++++++ tests/unittests/cli/test_fast_api.py | 116 +++++++++++++++++++ 4 files changed, 229 insertions(+), 7 deletions(-) diff --git a/src/google/adk/auth/auth_credential.py b/src/google/adk/auth/auth_credential.py index d4ffa47b69b..bb61225d906 100644 --- a/src/google/adk/auth/auth_credential.py +++ b/src/google/adk/auth/auth_credential.py @@ -30,6 +30,31 @@ _REDACTED = "" +# By-alias (camelCase) names of every field on a credential model that is +# marked `repr=False` above. `repr=False` only redacts these from Python's +# `repr()`/`str()` (logs, error strings); it has no effect on +# `model_dump()`/`model_dump_json()`, which is what actually goes out over +# the network (e.g. FastAPI's /run, /run_sse, /run_live responses). Anything +# serializing an `AuthCredential`-derived object for an external, untrusted +# client -- as opposed to internal persistence via SessionService -- must +# additionally strip these keys. Kept as one set here, next to the field +# declarations, so the two lists can't silently drift apart. +CREDENTIAL_SECRET_KEYS = frozenset({ + "password", + "token", + "additionalHeaders", + "clientSecret", + "authResponseUri", + "authCode", + "accessToken", + "refreshToken", + "idToken", + "codeVerifier", + "privateKeyId", + "privateKey", + "apiKey", +}) + # Pydantic echoes the rejected value into ValidationError messages # ("input_value=..."), which would put a malformed secret straight into logs and diff --git a/src/google/adk/cli/api_server.py b/src/google/adk/cli/api_server.py index 34ad0a824b5..f08b5a154c7 100644 --- a/src/google/adk/cli/api_server.py +++ b/src/google/adk/cli/api_server.py @@ -46,6 +46,7 @@ from fastapi import Request from fastapi import Response from fastapi.middleware.cors import CORSMiddleware +from fastapi.responses import JSONResponse from fastapi.responses import RedirectResponse from fastapi.responses import StreamingResponse from fastapi.staticfiles import StaticFiles @@ -77,6 +78,7 @@ from ..errors.already_exists_error import AlreadyExistsError from ..errors.input_validation_error import InputValidationError from ..errors.session_not_found_error import SessionNotFoundError +from ..auth.auth_credential import CREDENTIAL_SECRET_KEYS from ..events.event import Event from ..events.event_actions import EventActions from ..flows.llm_flows.functions import REQUEST_CONFIRMATION_FUNCTION_CALL_NAME @@ -574,6 +576,32 @@ def _invalid_event_error(event_index: int, disallowed: str) -> HTTPException: ) +def _redact_credential_secrets(value: Any) -> Any: + """Recursively strips credential-secret keys from a JSON-like value. + + An event's `function_call.args` (e.g. the `adk_request_credential` call + ADK issues when a tool needs OAuth) is an opaque `dict[str, Any]`, not a + nested pydantic model -- so `Event.model_dump(exclude=...)` cannot reach + a secret embedded inside it by field path. This walks the already-dumped + event unconditionally and drops any key in `CREDENTIAL_SECRET_KEYS` + wherever it appears, so a credential's `client_secret`, tokens, etc. never + reach an external client over /run, /run_sse, or /run_live, regardless of + where in the structure they're nested. This must only be applied to the + outbound wire representation -- never to what SessionService persists -- + since later turns reconstruct the original request's credential by + re-parsing the persisted `adk_request_credential` call's args. + """ + if isinstance(value, dict): + return { + key: _redact_credential_secrets(val) + for key, val in value.items() + if key not in CREDENTIAL_SECRET_KEYS + } + if isinstance(value, list): + return [_redact_credential_secrets(item) for item in value] + return value + + def _validate_session_initialization_events(events: list[Event]) -> None: """Rejects client-supplied events that claim to be ADK-generated. @@ -1825,8 +1853,10 @@ def _set_telemetry_context_if_needed(runner: Runner): else: _is_visual_builder.set(False) - @app.post("/run", response_model_exclude_none=True) - async def run_agent(req: RunAgentRequest, request: Request) -> list[Event]: + @app.post( + "/run", response_model=list[Event], response_model_exclude_none=True + ) + async def run_agent(req: RunAgentRequest, request: Request) -> Response: app_name = req.app_name or self.default_app_name if not app_name: raise HTTPException( @@ -1883,7 +1913,14 @@ async def monitor(): events = await worker_task logger.info("Generated %s events in agent run", len(events)) logger.debug("Events generated: %s", events) - return events + return JSONResponse( + content=[ + _redact_credential_secrets( + event.model_dump(exclude_none=True, by_alias=True, mode="json") + ) + for event in events + ] + ) except asyncio.CancelledError: if await request.is_disconnected(): return Response(status_code=499) @@ -1961,9 +1998,14 @@ async def event_generator(): events_to_stream = [content_event, artifact_event] for event_to_stream in events_to_stream: - sse_event = event_to_stream.model_dump_json( - exclude_none=True, - by_alias=True, + sse_event = json.dumps( + _redact_credential_secrets( + event_to_stream.model_dump( + exclude_none=True, + by_alias=True, + mode="json", + ) + ) ) logger.debug( "Generated event in agent run streaming: %s", sse_event @@ -2101,7 +2143,13 @@ async def forward_events(): ) as agen: async for event in agen: await websocket.send_text( - event.model_dump_json(exclude_none=True, by_alias=True) + json.dumps( + _redact_credential_secrets( + event.model_dump( + exclude_none=True, by_alias=True, mode="json" + ) + ) + ) ) async def process_messages(): diff --git a/tests/unittests/auth/test_auth_credential.py b/tests/unittests/auth/test_auth_credential.py index 732a6ae6a51..abf6682b47b 100644 --- a/tests/unittests/auth/test_auth_credential.py +++ b/tests/unittests/auth/test_auth_credential.py @@ -19,10 +19,12 @@ from google.adk.auth.auth_credential import AuthCredential from google.adk.auth.auth_credential import AuthCredentialTypes from google.adk.auth.auth_credential import BaseModelWithConfig +from google.adk.auth.auth_credential import CREDENTIAL_SECRET_KEYS from google.adk.auth.auth_credential import HttpAuth from google.adk.auth.auth_credential import HttpCredentials from google.adk.auth.auth_credential import OAuth2Auth from google.adk.auth.auth_credential import ServiceAccountCredential +from pydantic import alias_generators import pydantic import pytest @@ -123,6 +125,37 @@ def test_oauth2_credentials_redacted_in_repr_and_str(): assert 'secret_response_code' not in str_str +def test_credential_secret_keys_covers_every_repr_hidden_field(): + """CREDENTIAL_SECRET_KEYS must track every `repr=False` field's alias. + + `repr=False` only hides a field from `repr()`/`str()`; it does nothing for + `model_dump()`/`model_dump_json()`, which is what actually leaves the + process (e.g. a FastAPI response). `CREDENTIAL_SECRET_KEYS` is the + network-facing counterpart consumers must use to redact those same + fields before sending a credential-bearing object to an external client. + This asserts the two lists can't silently drift apart: every field this + module marks `repr=False` has a same-named (by alias) entry in + `CREDENTIAL_SECRET_KEYS`. + """ + camel_of = alias_generators.to_camel + expected_keys = set() + for model_cls in ( + HttpCredentials, + HttpAuth, + OAuth2Auth, + ServiceAccountCredential, + AuthCredential, + ): + for name, field in model_cls.model_fields.items(): + if field.repr is False: + expected_keys.add(field.alias or camel_of(name)) + assert expected_keys + assert expected_keys <= CREDENTIAL_SECRET_KEYS, ( + f'Fields marked repr=False but missing from CREDENTIAL_SECRET_KEYS:' + f' {expected_keys - CREDENTIAL_SECRET_KEYS}' + ) + + def test_service_account_redacted_in_repr_and_str(): """A service account private key and its ID are not rendered.""" sa_cred = ServiceAccountCredential( diff --git a/tests/unittests/cli/test_fast_api.py b/tests/unittests/cli/test_fast_api.py index a3fe28d35a3..ee479c5b34d 100644 --- a/tests/unittests/cli/test_fast_api.py +++ b/tests/unittests/cli/test_fast_api.py @@ -1929,6 +1929,122 @@ async def run_async_with_artifact_delta( assert sse_events[1]["actions"]["artifactDelta"] == {"artifact.txt": 0} +def test_agent_run_sse_redacts_oauth2_client_secret( + test_app, create_test_session, monkeypatch +): + """/run_sse must not leak OAuth2 secrets embedded in a function call. + + When a tool needs OAuth, ADK attaches the credential -- including the + app's `client_secret` -- to an `adk_request_credential` function call's + `args`. That `args` value is an opaque dict, not a nested pydantic model, + so it is not covered by `Event.model_dump(exclude=...)`. This asserts the + streamed event has the secret fields stripped while the fields the client + actually needs to complete the OAuth redirect (client_id, the + authorization URL, the credential key) are preserved. + """ + info = create_test_session + + auth_config_dict = { + "authScheme": { + "type": "oauth2", + "flows": { + "authorizationCode": { + "scopes": {"read": "read"}, + "authorizationUrl": "https://idp.example.com/oauth2/auth", + "tokenUrl": "https://idp.example.com/oauth2/token", + } + }, + }, + "rawAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": "should-never-reach-the-client", + }, + }, + "exchangedAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": "should-never-reach-the-client", + "authUri": ( + "https://idp.example.com/oauth2/auth?client_id=" + "public-client-id&state=xyz" + ), + "state": "xyz", + "codeVerifier": "pkce-verifier-should-not-leak-either", + }, + }, + "credentialKey": "my_tool:oauth2:abcd1234", + } + + async def run_async_with_auth_request( + self, + *, + user_id: str, + session_id: str, + invocation_id: Optional[str] = None, + new_message: Optional[types.Content] = None, + state_delta: Optional[dict[str, Any]] = None, + run_config: Optional[RunConfig] = None, + ): + del user_id, session_id, invocation_id, new_message, state_delta, run_config + yield Event( + author="agent", + invocation_id="invocation_id", + content=types.Content( + role="user", + parts=[ + types.Part( + function_call=types.FunctionCall( + name="adk_request_credential", + id="adk-req-cred-id", + args={ + "functionCallId": "adk-original-fc-id", + "authConfig": auth_config_dict, + }, + ) + ) + ], + ), + ) + + monkeypatch.setattr(Runner, "run_async", run_async_with_auth_request) + + payload = { + "app_name": info["app_name"], + "user_id": info["user_id"], + "session_id": info["session_id"], + "new_message": {"role": "user", "parts": [{"text": "Hello agent"}]}, + "streaming": True, + } + + response = test_app.post("/run_sse", json=payload) + assert response.status_code == 200 + assert "should-never-reach-the-client" not in response.text + assert "pkce-verifier-should-not-leak-either" not in response.text + + sse_events = [ + json.loads(line.removeprefix("data: ")) + for line in response.text.splitlines() + if line.startswith("data: ") + ] + assert len(sse_events) == 1 + args = sse_events[0]["content"]["parts"][0]["functionCall"]["args"] + raw_oauth2 = args["authConfig"]["rawAuthCredential"]["oauth2"] + exchanged_oauth2 = args["authConfig"]["exchangedAuthCredential"]["oauth2"] + assert "clientSecret" not in raw_oauth2 + assert "clientSecret" not in exchanged_oauth2 + assert "codeVerifier" not in exchanged_oauth2 + # Fields the client actually needs to complete the OAuth redirect must + # survive the redaction. + assert raw_oauth2["clientId"] == "public-client-id" + assert exchanged_oauth2["authUri"].startswith( + "https://idp.example.com/oauth2/auth" + ) + assert args["authConfig"]["credentialKey"] == "my_tool:oauth2:abcd1234" + + def test_agent_run_sse_does_not_split_artifact_delta_for_function_resume( test_app, create_test_session, monkeypatch ): From 7e553bfd687b0455835e69d4c20aed6665607096 Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Mon, 31 Aug 2026 00:39:39 +0530 Subject: [PATCH 2/7] fix(auth): also redact credential secrets from session-history endpoints The /run, /run_sse, and /run_live fix in the previous commit deliberately leaves what SessionService persists untouched, because a later turn recovers the original request's credential by re-parsing the persisted adk_request_credential call's args (see _merge_credential_oauth2_fields in auth_preprocessor.py). That means the same secret this fix removes from the live run endpoints was still reachable through any endpoint that reads back session history: GET/PATCH/POST on /apps/{app}/users/{user}/sessions(/{id}) all return a Session object (or list of them) via FastAPI's automatic response_model serialization, which does not go through the redaction added for the run endpoints. Applies the same _redact_credential_secrets() helper to get_session, list_sessions, create_session, create_session_with_id, and update_session, following the same JSONResponse-with-explicit- response_model pattern used for /run, so the documented OpenAPI schema is unchanged while the actual serialization is redacted. Adds a regression test confirming GET .../sessions/{id} and GET .../sessions no longer leak a client_secret embedded in session history, while the session's own identifying fields (id, appName, userId) are preserved. Confirmed this test fails without this commit's changes and passes with them. --- src/google/adk/cli/api_server.py | 70 +++++++++++++++++++------- tests/unittests/cli/test_fast_api.py | 75 ++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 17 deletions(-) diff --git a/src/google/adk/cli/api_server.py b/src/google/adk/cli/api_server.py index f08b5a154c7..36b4f1fc383 100644 --- a/src/google/adk/cli/api_server.py +++ b/src/google/adk/cli/api_server.py @@ -585,11 +585,11 @@ def _redact_credential_secrets(value: Any) -> Any: a secret embedded inside it by field path. This walks the already-dumped event unconditionally and drops any key in `CREDENTIAL_SECRET_KEYS` wherever it appears, so a credential's `client_secret`, tokens, etc. never - reach an external client over /run, /run_sse, or /run_live, regardless of - where in the structure they're nested. This must only be applied to the - outbound wire representation -- never to what SessionService persists -- - since later turns reconstruct the original request's credential by - re-parsing the persisted `adk_request_credential` call's args. + reach an external client over /run, /run_sse, /run_live, or any endpoint + that returns session history. This must only be applied to the outbound + wire representation -- never to what SessionService persists -- since + later turns reconstruct the original request's credential by re-parsing + the persisted `adk_request_credential` call's args. """ if isinstance(value, dict): return { @@ -602,6 +602,35 @@ def _redact_credential_secrets(value: Any) -> Any: return value +def _redacted_session_response(session: Session) -> JSONResponse: + """Returns a `Session` as a `JSONResponse` with credential secrets stripped. + + A session's persisted events may include an `adk_request_credential` + function call carrying a tool's full `AuthCredential` -- SessionService + intentionally retains the secret there so a later turn can recover it via + the credential merge-backfill mechanism, which means every endpoint that + returns a `Session` (not just /run, /run_sse, /run_live) must redact it + before it reaches an external client. + """ + return JSONResponse( + content=_redact_credential_secrets( + session.model_dump(exclude_none=True, by_alias=True, mode="json") + ) + ) + + +def _redacted_sessions_response(sessions: list[Session]) -> JSONResponse: + """Same as `_redacted_session_response`, for a list of sessions.""" + return JSONResponse( + content=[ + _redact_credential_secrets( + session.model_dump(exclude_none=True, by_alias=True, mode="json") + ) + for session in sessions + ] + ) + + def _validate_session_initialization_events(events: list[Event]) -> None: """Rejects client-supplied events that claim to be ADK-generated. @@ -1480,36 +1509,39 @@ async def get_adk_app_info(app_name: str) -> AppInfo: @app.get( "/apps/{app_name}/users/{user_id}/sessions/{session_id}", + response_model=Session, response_model_exclude_none=True, ) async def get_session( app_name: str, user_id: str, session_id: str - ) -> Session: + ) -> Response: session = await self.session_service.get_session( app_name=app_name, user_id=user_id, session_id=session_id ) if not session: raise HTTPException(status_code=404, detail="Session not found") self.current_app_name_ref.value = app_name - return session + return _redacted_session_response(session) @app.get( "/apps/{app_name}/users/{user_id}/sessions", + response_model=list[Session], response_model_exclude_none=True, ) - async def list_sessions(app_name: str, user_id: str) -> list[Session]: + async def list_sessions(app_name: str, user_id: str) -> Response: list_sessions_response = await self.session_service.list_sessions( app_name=app_name, user_id=user_id ) - return [ + return _redacted_sessions_response([ session for session in list_sessions_response.sessions # Remove sessions that were generated as a part of Eval. if not session.id.startswith(EVAL_SESSION_ID_PREFIX) - ] + ]) @app.post( "/apps/{app_name}/users/{user_id}/sessions/{session_id}", + response_model=Session, response_model_exclude_none=True, ) @deprecated( @@ -1521,25 +1553,28 @@ async def create_session_with_id( user_id: str, session_id: str, state: Optional[dict[str, Any]] = None, - ) -> Session: - return await self._create_session( + ) -> Response: + session = await self._create_session( app_name=app_name, user_id=user_id, state=state, session_id=session_id, ) + return _redacted_session_response(session) @app.post( "/apps/{app_name}/users/{user_id}/sessions", + response_model=Session, response_model_exclude_none=True, ) async def create_session( app_name: str, user_id: str, req: Optional[CreateSessionRequest] = None, - ) -> Session: + ) -> Response: if not req: - return await self._create_session(app_name=app_name, user_id=user_id) + session = await self._create_session(app_name=app_name, user_id=user_id) + return _redacted_session_response(session) if req.events: _validate_session_initialization_events(req.events) @@ -1555,7 +1590,7 @@ async def create_session( for event in req.events: await self.session_service.append_event(session=session, event=event) - return session + return _redacted_session_response(session) @app.delete("/apps/{app_name}/users/{user_id}/sessions/{session_id}") async def delete_session( @@ -1567,6 +1602,7 @@ async def delete_session( @app.patch( "/apps/{app_name}/users/{user_id}/sessions/{session_id}", + response_model=Session, response_model_exclude_none=True, ) async def update_session( @@ -1574,7 +1610,7 @@ async def update_session( user_id: str, session_id: str, req: UpdateSessionRequest, - ) -> Session: + ) -> Response: """Updates session state without running the agent. Args: @@ -1613,7 +1649,7 @@ async def update_session( session=session, event=state_update_event ) - return session + return _redacted_session_response(session) @app.get( "/apps/{app_name}/users/{user_id}/sessions/{session_id}/artifacts/{artifact_name:path}/versions/{version_id}/metadata", diff --git a/tests/unittests/cli/test_fast_api.py b/tests/unittests/cli/test_fast_api.py index ee479c5b34d..1fafa5a690a 100644 --- a/tests/unittests/cli/test_fast_api.py +++ b/tests/unittests/cli/test_fast_api.py @@ -1635,6 +1635,81 @@ def test_get_session(test_app, create_test_session): logger.info(f"Retrieved session: {data['id']}") +async def test_get_session_redacts_oauth2_client_secret( + test_app, create_test_session, mock_session_service +): + """GET .../sessions/{id} must not leak OAuth2 secrets from session history. + + SessionService intentionally persists an `adk_request_credential` call's + full credential (including `client_secret`) so a later turn can recover + it via the credential merge-backfill mechanism -- see the /run_sse + redaction test for why that data can't be stripped at persistence time. + That means any endpoint returning session history, not just the live + /run family, must redact it before it reaches an external client. + """ + info = create_test_session + + auth_config_dict = { + "authScheme": {"type": "oauth2", "flows": {}}, + "rawAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": "should-never-reach-the-client-via-get", + }, + }, + "credentialKey": "my_tool:oauth2:abcd1234", + } + + session = await mock_session_service.get_session( + app_name=info["app_name"], + user_id=info["user_id"], + session_id=info["session_id"], + ) + await mock_session_service.append_event( + session=session, + event=Event( + author="agent", + invocation_id="invocation_id", + content=types.Content( + role="user", + parts=[ + types.Part( + function_call=types.FunctionCall( + name="adk_request_credential", + id="adk-req-cred-id", + args={ + "functionCallId": "adk-original-fc-id", + "authConfig": auth_config_dict, + }, + ) + ) + ], + ), + ), + ) + + url = f"/apps/{info['app_name']}/users/{info['user_id']}/sessions/{info['session_id']}" + + # GET a single session. + response = test_app.get(url) + assert response.status_code == 200 + assert "should-never-reach-the-client-via-get" not in response.text + event = response.json()["events"][0] + raw_oauth2 = event["content"]["parts"][0]["functionCall"]["args"][ + "authConfig" + ]["rawAuthCredential"]["oauth2"] + assert "clientSecret" not in raw_oauth2 + assert raw_oauth2["clientId"] == "public-client-id" + + # LIST sessions must not leak it either. + list_response = test_app.get( + f"/apps/{info['app_name']}/users/{info['user_id']}/sessions" + ) + assert list_response.status_code == 200 + assert "should-never-reach-the-client-via-get" not in list_response.text + + def test_list_sessions(test_app, create_test_session): """Test listing all sessions for a user.""" info = create_test_session From d3f2c8ff322ec930fc9a466658641b3cf3260b8f Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Mon, 31 Aug 2026 00:54:53 +0530 Subject: [PATCH 3/7] fix(auth): redact credential secrets from dev-UI eval endpoints too Extends the same redaction to the dev-only eval endpoints (get_eval, get_eval_result, get_eval_result_legacy), registered only under DevServer / `adk web`, not the production ApiServer used by /run, /run_sse, /run_live, and the session-history endpoints fixed in the previous two commits. An eval case built from a session (via add-session) carries the raw events from that session in its conversation, so an `adk_request_credential` call's full credential can end up in an EvalCase's stored conversation. Separately, EvalCaseResult.session_details holds the full Session produced by a live eval run, which can carry the same kind of event if a tool needed OAuth during that run. This is a materially lower-severity finding than the previous two commits: reaching it requires the deployer to have chosen to run the local development UI (`adk web`) rather than a production deployment, which is the same trust boundary already applied to other dev-only debug/admin surfaces in this codebase. It's included here for consistency and defense in depth rather than as a standalone report. Reuses the existing _redact_credential_secrets() helper from api_server.py (imported into dev_server.py) and the same JSONResponse-with-explicit-response_model pattern used for /run and the session endpoints, so the documented OpenAPI schema is unchanged. Adds a regression test confirming GET .../eval-cases/{id} no longer leaks a client_secret embedded in an eval case built from a session, while the credential's non-secret fields (client_id, credential_key) are preserved. Confirmed this test fails without this commit's changes and passes with them. --- src/google/adk/cli/dev_server.py | 49 +++++++++++++--- tests/unittests/cli/test_fast_api.py | 88 ++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 7 deletions(-) diff --git a/src/google/adk/cli/dev_server.py b/src/google/adk/cli/dev_server.py index fa3807df394..ce99a5a023d 100644 --- a/src/google/adk/cli/dev_server.py +++ b/src/google/adk/cli/dev_server.py @@ -49,8 +49,10 @@ from fastapi import FastAPI from fastapi import HTTPException from fastapi import Request as FastAPIRequest +from fastapi import Response from fastapi import UploadFile from fastapi.responses import FileResponse +from fastapi.responses import JSONResponse from fastapi.responses import PlainTextResponse from fastapi.responses import StreamingResponse import graphviz @@ -78,6 +80,7 @@ from ..evaluation.eval_set import EvalSet from ..utils._telemetry_config import read_telemetry_consent from ..utils._telemetry_config import write_telemetry_consent +from .api_server import _redact_credential_secrets from .api_server import ApiServer NESTED_APP_SEPARATOR = "." @@ -1069,6 +1072,7 @@ async def run_eval_legacy( # TODO - remove after migration @app.get( "/dev/apps/{app_name}/eval_results/{eval_result_id}", + response_model=EvalSetResult, response_model_exclude_none=True, tags=[TAG_EVALUATION], ) @@ -1079,11 +1083,18 @@ async def run_eval_legacy( async def get_eval_result_legacy( app_name: str, eval_result_id: str, - ) -> EvalSetResult: + ) -> Response: try: - return self.eval_set_results_manager.get_eval_set_result( + eval_set_result = self.eval_set_results_manager.get_eval_set_result( app_name, eval_result_id ) + return JSONResponse( + content=_redact_credential_secrets( + eval_set_result.model_dump( + exclude_none=True, by_alias=True, mode="json" + ) + ) + ) except ValueError as ve: raise HTTPException(status_code=404, detail=str(ve)) from ve except ValidationError as ve: @@ -1184,24 +1195,42 @@ async def list_evals_in_eval_set( @app.get( "/dev/apps/{app_name}/eval-sets/{eval_set_id}/eval-cases/{eval_case_id}", + response_model=EvalCase, response_model_exclude_none=True, tags=[TAG_EVALUATION], ) @app.get( "/dev/apps/{app_name}/eval_sets/{eval_set_id}/evals/{eval_case_id}", + response_model=EvalCase, response_model_exclude_none=True, tags=[TAG_EVALUATION], ) async def get_eval( app_name: str, eval_set_id: str, eval_case_id: str - ) -> EvalCase: - """Gets an eval case in an eval set.""" + ) -> Response: + """Gets an eval case in an eval set. + + An eval case built from a session (via add_session_to_eval_set) may + carry an `adk_request_credential` function call in its conversation, + including the tool's full credential (e.g. OAuth2 client_secret) -- + the same secret /run, /run_sse, /run_live, and the session-history + endpoints redact. This endpoint is dev-UI-only (registered only under + DevServer / `adk web`, not the production ApiServer), so it's a lower + severity than those, but the redaction is applied for consistency and + defense in depth. + """ eval_case_to_find = self.eval_sets_manager.get_eval_case( app_name, eval_set_id, eval_case_id ) if eval_case_to_find: - return eval_case_to_find + return JSONResponse( + content=_redact_credential_secrets( + eval_case_to_find.model_dump( + exclude_none=True, by_alias=True, mode="json" + ) + ) + ) raise HTTPException( status_code=404, @@ -1365,19 +1394,25 @@ async def run_eval( @app.get( "/dev/apps/{app_name}/eval-results/{eval_result_id}", + response_model=EvalResult, response_model_exclude_none=True, tags=[TAG_EVALUATION], ) async def get_eval_result( app_name: str, eval_result_id: str, - ) -> EvalResult: + ) -> Response: """Gets the eval result for the given eval id.""" try: eval_set_result = self.eval_set_results_manager.get_eval_set_result( app_name, eval_result_id ) - return EvalResult(**eval_set_result.model_dump()) + result = EvalResult(**eval_set_result.model_dump()) + return JSONResponse( + content=_redact_credential_secrets( + result.model_dump(exclude_none=True, by_alias=True, mode="json") + ) + ) except ValueError as ve: raise HTTPException(status_code=404, detail=str(ve)) from ve except ValidationError as ve: diff --git a/tests/unittests/cli/test_fast_api.py b/tests/unittests/cli/test_fast_api.py index 1fafa5a690a..d33900b245d 100644 --- a/tests/unittests/cli/test_fast_api.py +++ b/tests/unittests/cli/test_fast_api.py @@ -4923,6 +4923,94 @@ def test_add_session_to_eval_set_builds_eval_case_from_session( ] == ["what is 2+2?"] +async def test_get_eval_redacts_oauth2_client_secret( + test_app, test_session_info, mock_eval_sets_manager, mock_session_service +): + """GET .../eval-cases/{id} must not leak OAuth2 secrets from an eval case. + + An eval case built from a session (via add-session) carries the raw + events from that session in its conversation, including an + `adk_request_credential` function call's full credential. This endpoint + is dev-UI-only (registered only under DevServer / `adk web`, unlike + /run, /run_sse, /run_live, and the session-history endpoints, which are + on the production ApiServer), but the redaction is applied here too for + consistency and defense in depth. + """ + app_name = test_session_info["app_name"] + user_id = test_session_info["user_id"] + mock_eval_sets_manager.create_eval_set( + app_name=app_name, eval_set_id="my_eval_set" + ) + + # The `adk_request_credential` function name is reserved for ADK-generated + # events and is rejected in client-supplied session-initialization data, + # so the credential-bearing event is appended directly through the + # session service, exactly as the real runtime would. + session = await mock_session_service.create_session( + app_name=app_name, + user_id=user_id, + session_id="eval_source_session_with_secret", + state={}, + ) + await mock_session_service.append_event( + session=session, + event=Event( + author="agent", + invocation_id="inv-1", + content=types.Content( + role="user", + parts=[ + types.Part( + function_call=types.FunctionCall( + name="adk_request_credential", + id="adk-req-cred-id", + args={ + "functionCallId": "adk-original-fc-id", + "authConfig": { + "authScheme": { + "type": "oauth2", + "flows": {}, + }, + "rawAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": ( + "should-never-reach-the" + "-client-via-eval-case" + ), + }, + }, + "credentialKey": "my_tool:oauth2:abcd1234", + }, + }, + ) + ) + ], + ), + ), + ) + + add_session_response = test_app.post( + f"/dev/apps/{app_name}/eval-sets/my_eval_set/add-session", + json={ + "eval_id": "my_eval_case_with_secret", + "session_id": "eval_source_session_with_secret", + "user_id": user_id, + }, + ) + assert add_session_response.status_code == 200 + + response = test_app.get( + f"/dev/apps/{app_name}/eval-sets/my_eval_set/eval-cases/my_eval_case_with_secret" + ) + assert response.status_code == 200 + assert "should-never-reach-the-client-via-eval-case" not in response.text + # The credential's non-secret fields must survive the redaction. + assert "public-client-id" in response.text + assert "my_tool:oauth2:abcd1234" in response.text + + @pytest.mark.xfail( strict=True, reason="add-session maps ValueError, but the managers raise NotFoundError", From a17b896c78fcba80ffce15d81f245b3fce7a7921 Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Tue, 1 Sep 2026 11:03:10 +0530 Subject: [PATCH 4/7] fix(auth): address mutation-testing review of the credential-redaction fix Three findings from an independent mutation-tested review, addressed in order of how much each mattered: 1. Five of the eight redaction call sites had no test asserting they actually redact anything: /run, /run_live, list_sessions, and the two dev eval-result endpoints (get_eval_result_legacy, get_eval_result). Neutering each call site in turn (replacing _redact_credential_secrets with an identity function) left the existing suite green in every one of those five cases -- a regression removing any of them would have gone uncaught. Adds one test per site, each verified against the same mutation: it fails when its site's call is neutered and passes otherwise, with the other redaction tests unaffected either way. list_sessions surfaced an additional, previously-invisible gap while writing its test: the existing "list sessions must not leak it" assertion elsewhere in this file was vacuously true regardless of redaction, because InMemorySessionService.list_sessions() deliberately strips `events` from every session it returns (`sessions_without_events`) -- there was never a secret in that response to redact in the first place under the real backend. The new test monkeypatches list_sessions to actually include events, so it exercises the real _redacted_sessions_response call instead of a check that could never fail either way. 2. _redact_credential_secrets matched CREDENTIAL_SECRET_KEYS names anywhere in a payload, unconditionally. Several of those names -- token, password, apiKey, accessToken among them -- are ordinary words a tool's own return value or an app's own session state can legitimately use for something that is not a credential at all (a pagination cursor named token, a scraped page's own password field). Deleting those unconditionally silently dropped data the caller never asked to have redacted, indistinguishable from a key a tool simply never returned -- not a security hole, but a silent behavior change to non-credential data on production endpoints. Rescoped stripping to dicts that are actually AuthCredential dumps, identified by carrying authType (every AuthCredential serialization has it, including one parked in session state by SessionStateCredentialService under an arbitrary, app-or-tool-chosen key) rather than a fixed set of container key names like authConfig. This closes the false-positive case while preserving exactly the session-state coverage the review confirmed was otherwise intact: verified a credential nested under an arbitrary state key is still fully redacted, and unrelated data using the same field names (token, password, apiKey, nested accessToken) now survives untouched. 3. The drift guard (test_credential_secret_keys_covers_every_repr_hidden_field) iterated a hardcoded tuple of five credential classes, so a new credential class added later without also editing that tuple would pass the guard while its own repr=False fields leaked. Reproduced the review's exact probe (a hypothetical MtlsCredential with one repr=False field, added to the module without touching the guard): the old test passed while the field leaked on the wire. Replaced the hardcoded tuple with reflection over every BaseModelWithConfig subclass in the module, and changed the assertion from one-directional (expected <= actual) to exact equality, so a key that stops being used by any field is caught too rather than lingering in the set indefinitely. Re-run against the same probe, the reflection-based version correctly reports the missing field. Full auth suite (241 tests) and the relevant fast_api suite pass clean, with the same five known-unrelated failures (missing optional GCP dependencies, pre-existing on main) and no new regressions. --- src/google/adk/cli/api_server.py | 71 ++- tests/unittests/auth/test_auth_credential.py | 36 +- tests/unittests/cli/test_fast_api.py | 465 +++++++++++++++++++ 3 files changed, 547 insertions(+), 25 deletions(-) diff --git a/src/google/adk/cli/api_server.py b/src/google/adk/cli/api_server.py index 36b4f1fc383..59d620e03c7 100644 --- a/src/google/adk/cli/api_server.py +++ b/src/google/adk/cli/api_server.py @@ -576,6 +576,40 @@ def _invalid_event_error(event_index: int, disallowed: str) -> HTTPException: ) +def _is_credential_shaped(value: Any) -> bool: + """Reports whether `value` is a dict that is itself an `AuthCredential`. + + `authType` is `AuthCredential.auth_type`'s by-alias serialized name, and + every `AuthCredential` dump carries it -- including one parked directly + in session state by `SessionStateCredentialService` under an + app-or-tool-chosen key, which is why this checks shape rather than a + fixed set of container key names like `authConfig`: a credential can + legitimately appear anywhere a `dict[str, Any]` the app controls can + hold one, not only nested under the names ADK's own code happens to use. + """ + return isinstance(value, dict) and "authType" in value + + +def _strip_secret_keys(value: Any) -> Any: + """Unconditionally strips `CREDENTIAL_SECRET_KEYS` from every dict in + `value`, at any depth. + + Only safe to call once a `_is_credential_shaped` dict has already been + found: everything below that point genuinely belongs to a credential + object, so a key named e.g. `token` here is actually a secret, not + coincidentally-named application data. + """ + if isinstance(value, dict): + return { + key: _strip_secret_keys(val) + for key, val in value.items() + if key not in CREDENTIAL_SECRET_KEYS + } + if isinstance(value, list): + return [_strip_secret_keys(item) for item in value] + return value + + def _redact_credential_secrets(value: Any) -> Any: """Recursively strips credential-secret keys from a JSON-like value. @@ -583,20 +617,33 @@ def _redact_credential_secrets(value: Any) -> Any: ADK issues when a tool needs OAuth) is an opaque `dict[str, Any]`, not a nested pydantic model -- so `Event.model_dump(exclude=...)` cannot reach a secret embedded inside it by field path. This walks the already-dumped - event unconditionally and drops any key in `CREDENTIAL_SECRET_KEYS` - wherever it appears, so a credential's `client_secret`, tokens, etc. never - reach an external client over /run, /run_sse, /run_live, or any endpoint - that returns session history. This must only be applied to the outbound - wire representation -- never to what SessionService persists -- since - later turns reconstruct the original request's credential by re-parsing - the persisted `adk_request_credential` call's args. + event unconditionally, so a credential's `client_secret`, tokens, etc. + never reach an external client over /run, /run_sse, /run_live, or any + endpoint that returns session history or eval data. This must only be + applied to the outbound wire representation -- never to what + SessionService persists -- since later turns reconstruct the original + request's credential by re-parsing the persisted `adk_request_credential` + call's args. + + Stripping is scoped to dicts that are actually `AuthCredential` dumps + (identified by carrying `authType`, see `_is_credential_shaped`), not to + every `CREDENTIAL_SECRET_KEYS` name anywhere in the structure. Several of + those names -- `token`, `password`, `apiKey`, `accessToken` among them -- + are ordinary words a tool's own return value or an app's own session + state can legitimately use for something that isn't a credential at all + (a pagination cursor named `token`, a scraped page's own `password` + field). Deleting those unconditionally would silently drop data the + caller never asked to have redacted, indistinguishable from a key a tool + simply never returned. Scoping to credential-shaped subtrees avoids that + while still catching every real credential location, including one + stored in session state under an arbitrary key -- that dump still + carries `authType`, so shape-based detection finds it without needing a + fixed list of container key names. """ + if _is_credential_shaped(value): + return _strip_secret_keys(value) if isinstance(value, dict): - return { - key: _redact_credential_secrets(val) - for key, val in value.items() - if key not in CREDENTIAL_SECRET_KEYS - } + return {key: _redact_credential_secrets(val) for key, val in value.items()} if isinstance(value, list): return [_redact_credential_secrets(item) for item in value] return value diff --git a/tests/unittests/auth/test_auth_credential.py b/tests/unittests/auth/test_auth_credential.py index abf6682b47b..7fa5ce61388 100644 --- a/tests/unittests/auth/test_auth_credential.py +++ b/tests/unittests/auth/test_auth_credential.py @@ -16,6 +16,9 @@ from __future__ import annotations +import inspect + +from google.adk.auth import auth_credential from google.adk.auth.auth_credential import AuthCredential from google.adk.auth.auth_credential import AuthCredentialTypes from google.adk.auth.auth_credential import BaseModelWithConfig @@ -133,26 +136,33 @@ def test_credential_secret_keys_covers_every_repr_hidden_field(): process (e.g. a FastAPI response). `CREDENTIAL_SECRET_KEYS` is the network-facing counterpart consumers must use to redact those same fields before sending a credential-bearing object to an external client. - This asserts the two lists can't silently drift apart: every field this - module marks `repr=False` has a same-named (by alias) entry in - `CREDENTIAL_SECRET_KEYS`. + + Discovers every `BaseModelWithConfig` subclass in this module by + reflection rather than naming them, so a credential class added later + (e.g. an `MtlsCredential` with its own `repr=False` field) is included + automatically -- a hardcoded tuple of classes would silently exclude + it, and CREDENTIAL_SECRET_KEYS would then miss that field with nothing + to catch the gap. Asserts exact equality rather than only that expected + keys are covered, so a key that stops being used anywhere is caught + too, not left in the set indefinitely. """ camel_of = alias_generators.to_camel expected_keys = set() - for model_cls in ( - HttpCredentials, - HttpAuth, - OAuth2Auth, - ServiceAccountCredential, - AuthCredential, - ): + for _, model_cls in inspect.getmembers(auth_credential, inspect.isclass): + if ( + not issubclass(model_cls, BaseModelWithConfig) + or model_cls is BaseModelWithConfig + ): + continue for name, field in model_cls.model_fields.items(): if field.repr is False: expected_keys.add(field.alias or camel_of(name)) assert expected_keys - assert expected_keys <= CREDENTIAL_SECRET_KEYS, ( - f'Fields marked repr=False but missing from CREDENTIAL_SECRET_KEYS:' - f' {expected_keys - CREDENTIAL_SECRET_KEYS}' + assert expected_keys == CREDENTIAL_SECRET_KEYS, ( + 'CREDENTIAL_SECRET_KEYS has drifted from the repr=False fields' + ' discovered by reflection.\n' + f'Missing from CREDENTIAL_SECRET_KEYS: {expected_keys - CREDENTIAL_SECRET_KEYS}\n' + f'No longer used by any repr=False field: {CREDENTIAL_SECRET_KEYS - expected_keys}' ) diff --git a/tests/unittests/cli/test_fast_api.py b/tests/unittests/cli/test_fast_api.py index d33900b245d..11d8f9c61fc 100644 --- a/tests/unittests/cli/test_fast_api.py +++ b/tests/unittests/cli/test_fast_api.py @@ -45,7 +45,9 @@ from google.adk.events.event_actions import EventActions from google.adk.plugins.bigquery_agent_analytics_plugin import BigQueryAgentAnalyticsPlugin from google.adk.runners import Runner +from google.adk.sessions.base_session_service import ListSessionsResponse from google.adk.sessions.in_memory_session_service import InMemorySessionService +from google.adk.sessions.session import Session from google.adk.tools.tool_confirmation import ToolConfirmation from google.api_core.exceptions import GoogleAPICallError from google.api_core.exceptions import InvalidArgument @@ -5011,6 +5013,330 @@ async def test_get_eval_redacts_oauth2_client_secret( assert "my_tool:oauth2:abcd1234" in response.text +def test_redact_credential_secrets_does_not_touch_unrelated_app_data(): + """`_redact_credential_secrets` must only touch credential-shaped dicts. + + `CREDENTIAL_SECRET_KEYS` includes several ordinary words -- `token`, + `password`, `apiKey`, `accessToken` among them -- that a tool's own + return value or an app's own session state can legitimately use for + something that is not a credential at all (a pagination cursor named + `token`, a scraped page's own `password` field). An earlier version of + this function matched those names anywhere in the structure, silently + dropping such fields with nothing to distinguish that from a key the + tool simply never returned. This confirms unrelated data survives + untouched, while a real credential -- identified by carrying `authType`, + the way every `AuthCredential` dump does, wherever it's nested -- is + still fully redacted even when parked under an arbitrary, + app-or-tool-chosen key (e.g. as `SessionStateCredentialService` stores + one in session state) rather than under one of ADK's own fixed + container key names like `authConfig`. + """ + from google.adk.cli.api_server import _redact_credential_secrets + + tool_result = { + "results": ["a", "b"], + "token": "next-page-cursor-abc", + "password": "unrelated-app-value", + "apiKey": "unrelated-app-key", + "nested": {"accessToken": "unrelated-nested-value"}, + } + assert _redact_credential_secrets(tool_result) == tool_result + + state_delta = {"token": 42, "user:password_hint": "x"} + assert _redact_credential_secrets(state_delta) == state_delta + + credential_under_arbitrary_key = { + "someToolChosenStateKey": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-id", + "clientSecret": "real-secret-should-be-redacted", + }, + } + } + redacted = _redact_credential_secrets(credential_under_arbitrary_key) + assert "real-secret-should-be-redacted" not in str(redacted) + assert ( + redacted["someToolChosenStateKey"]["oauth2"]["clientId"] == "public-id" + ) + + +def test_agent_run_redacts_oauth2_client_secret( + test_app, create_test_session, monkeypatch +): + """/run (non-streaming) must not leak OAuth2 secrets, same as /run_sse. + + /run and /run_sse are separate code paths -- /run builds one + JSONResponse from the full event list via `_redact_credential_secrets` + called once per event in a list comprehension; /run_sse builds one + `json.dumps` per streamed event -- so coverage of one does not imply + the other actually redacts anything; it could have had its call to + `_redact_credential_secrets` silently removed and this suite would + still be green without a test exercising this endpoint directly. + """ + info = create_test_session + + auth_config_dict = { + "authScheme": {"type": "oauth2", "flows": {}}, + "rawAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": "should-never-reach-the-client", + }, + }, + "exchangedAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": "should-never-reach-the-client", + "authUri": ( + "https://idp.example.com/oauth2/auth?client_id=" + "public-client-id&state=xyz" + ), + "state": "xyz", + "codeVerifier": "pkce-verifier-should-not-leak-either", + }, + }, + "credentialKey": "my_tool:oauth2:abcd1234", + } + + async def run_async_with_auth_request( + self, + *, + user_id: str, + session_id: str, + invocation_id: Optional[str] = None, + new_message: Optional[types.Content] = None, + state_delta: Optional[dict[str, Any]] = None, + run_config: Optional[RunConfig] = None, + ): + del user_id, session_id, invocation_id, new_message, state_delta, run_config + yield Event( + author="agent", + invocation_id="invocation_id", + content=types.Content( + role="user", + parts=[ + types.Part( + function_call=types.FunctionCall( + name="adk_request_credential", + id="adk-req-cred-id", + args={ + "functionCallId": "adk-original-fc-id", + "authConfig": auth_config_dict, + }, + ) + ) + ], + ), + ) + + monkeypatch.setattr(Runner, "run_async", run_async_with_auth_request) + + payload = { + "app_name": info["app_name"], + "user_id": info["user_id"], + "session_id": info["session_id"], + "new_message": {"role": "user", "parts": [{"text": "Hello agent"}]}, + "streaming": False, + } + response = test_app.post("/run", json=payload) + + assert response.status_code == 200 + assert "should-never-reach-the-client" not in response.text + assert "pkce-verifier-should-not-leak-either" not in response.text + + events = response.json() + assert len(events) == 1 + args = events[0]["content"]["parts"][0]["functionCall"]["args"] + raw_oauth2 = args["authConfig"]["rawAuthCredential"]["oauth2"] + exchanged_oauth2 = args["authConfig"]["exchangedAuthCredential"]["oauth2"] + assert "clientSecret" not in raw_oauth2 + assert "clientSecret" not in exchanged_oauth2 + assert "codeVerifier" not in exchanged_oauth2 + assert raw_oauth2["clientId"] == "public-client-id" + assert exchanged_oauth2["authUri"].startswith( + "https://idp.example.com/oauth2/auth" + ) + assert args["authConfig"]["credentialKey"] == "my_tool:oauth2:abcd1234" + + +def test_run_live_websocket_redacts_oauth2_client_secret( + mock_session_service, + mock_artifact_service, + mock_memory_service, + mock_agent_loader, + mock_eval_sets_manager, + mock_eval_set_results_manager, + monkeypatch, +): + """/run_live (websocket) must not leak OAuth2 secrets, same as /run/_sse. + + /run_live is a third, independent code path from /run and /run_sse -- + it writes directly to a websocket via `websocket.send_text(json.dumps(...))` + inside `forward_events()`, not through either endpoint's JSONResponse or + SSE machinery -- so a test covering the other two says nothing about + whether this one still calls `_redact_credential_secrets` at all. + """ + test_app = _create_test_client( + mock_session_service, + mock_artifact_service, + mock_memory_service, + mock_agent_loader, + mock_eval_sets_manager, + mock_eval_set_results_manager, + ) + + async def setup_session(): + await mock_session_service.create_session( + app_name="test_app", user_id="user", session_id="session", state={} + ) + + asyncio.run(setup_session()) + + auth_config_dict = { + "authScheme": {"type": "oauth2", "flows": {}}, + "rawAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": "should-never-reach-the-client-via-live", + }, + }, + "credentialKey": "my_tool:oauth2:abcd1234", + } + + async def run_live_with_auth_request(self, session, live_request_queue, **kwargs): + yield Event( + author="agent", + invocation_id="invocation_id", + content=types.Content( + role="user", + parts=[ + types.Part( + function_call=types.FunctionCall( + name="adk_request_credential", + id="adk-req-cred-id", + args={ + "functionCallId": "adk-original-fc-id", + "authConfig": auth_config_dict, + }, + ) + ) + ], + ), + ) + + monkeypatch.setattr(Runner, "run_live", run_live_with_auth_request) + + url = "/run_live?app_name=test_app&user_id=user&session_id=session&modalities=AUDIO" + with test_app.websocket_connect(url) as ws: + data = ws.receive_json() + + raw_text = json.dumps(data) + assert "should-never-reach-the-client-via-live" not in raw_text + args = data["content"]["parts"][0]["functionCall"]["args"] + assert "clientSecret" not in args["authConfig"]["rawAuthCredential"]["oauth2"] + assert ( + args["authConfig"]["rawAuthCredential"]["oauth2"]["clientId"] + == "public-client-id" + ) + assert args["authConfig"]["credentialKey"] == "my_tool:oauth2:abcd1234" + + +async def test_list_sessions_redacts_oauth2_client_secret( + test_app, create_test_session, mock_session_service, monkeypatch +): + """GET .../sessions (list) must not leak secrets, same as GET .../sessions/{id}. + + `_redacted_session_response` (single session) and `_redacted_sessions_response` + (list) are two separate functions -- get_session/create_session/update_session + use the former, list_sessions uses the latter -- so a passing test for + get_session does not exercise whether list_sessions' own redaction call is + still wired up. `InMemorySessionService.list_sessions()` deliberately + strips `events` from every session it returns (`sessions_without_events` + in `_list_sessions_impl`), which makes a secret-in-events check against + the real backend vacuously pass regardless of whether + `_redacted_sessions_response` does anything at all -- there's never a + secret in that response to begin with. `list_sessions` is monkeypatched + here to actually include events, so this exercises the real redaction + call the endpoint makes rather than a check that can't fail either way. + """ + info = create_test_session + + session = await mock_session_service.get_session( + app_name=info["app_name"], + user_id=info["user_id"], + session_id=info["session_id"], + ) + await mock_session_service.append_event( + session=session, + event=Event( + author="agent", + invocation_id="invocation_id", + content=types.Content( + role="user", + parts=[ + types.Part( + function_call=types.FunctionCall( + name="adk_request_credential", + id="adk-req-cred-id", + args={ + "functionCallId": "adk-original-fc-id", + "authConfig": { + "authScheme": {"type": "oauth2", "flows": {}}, + "rawAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": ( + "should-never-reach-the-client" + "-via-list" + ), + }, + }, + "credentialKey": "my_tool:oauth2:abcd1234", + }, + }, + ) + ) + ], + ), + ), + ) + + session_with_events = await mock_session_service.get_session( + app_name=info["app_name"], + user_id=info["user_id"], + session_id=info["session_id"], + ) + + async def list_sessions_including_events(self, *, app_name, user_id=None): + del app_name, user_id + return ListSessionsResponse(sessions=[session_with_events]) + + monkeypatch.setattr( + type(mock_session_service), + "list_sessions", + list_sessions_including_events, + ) + response = test_app.get( + f"/apps/{info['app_name']}/users/{info['user_id']}/sessions" + ) + + assert response.status_code == 200 + assert "should-never-reach-the-client-via-list" not in response.text + sessions = response.json() + assert len(sessions) == 1 + args = sessions[0]["events"][0]["content"]["parts"][0]["functionCall"]["args"] + assert "clientSecret" not in args["authConfig"]["rawAuthCredential"]["oauth2"] + assert ( + args["authConfig"]["rawAuthCredential"]["oauth2"]["clientId"] + == "public-client-id" + ) + + @pytest.mark.xfail( strict=True, reason="add-session maps ValueError, but the managers raise NotFoundError", @@ -5051,6 +5377,145 @@ def test_get_eval_result_returns_saved_eval_set_result( assert data["evalSetId"] == "my_eval_set" +def _eval_case_result_with_credential_session( + eval_set_id: str, eval_id: str, secret_value: str +) -> "EvalCaseResult": + """Builds a minimal EvalCaseResult whose session_details carries a + credential-bearing adk_request_credential event, the way a live eval run + that triggers a tool's OAuth flow would.""" + from google.adk.evaluation.eval_metrics import EvalStatus + from google.adk.evaluation.eval_result import EvalCaseResult + + session_with_secret = Session( + id=f"{eval_id}_session", + app_name="test_app", + user_id="test_user", + events=[ + Event( + author="agent", + invocation_id="invocation_id", + content=types.Content( + role="user", + parts=[ + types.Part( + function_call=types.FunctionCall( + name="adk_request_credential", + id="adk-req-cred-id", + args={ + "functionCallId": "adk-original-fc-id", + "authConfig": { + "authScheme": { + "type": "oauth2", + "flows": {}, + }, + "rawAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": secret_value, + }, + }, + "credentialKey": "my_tool:oauth2:abcd1234", + }, + }, + ) + ) + ], + ), + ) + ], + ) + return EvalCaseResult( + eval_set_id=eval_set_id, + eval_id=eval_id, + final_eval_status=EvalStatus.PASSED, + overall_eval_metric_results=[], + eval_metric_result_per_invocation=[], + session_id=session_with_secret.id, + session_details=session_with_secret, + user_id="test_user", + ) + + +def test_get_eval_result_redacts_oauth2_client_secret( + test_app, mock_eval_set_results_manager +): + """GET .../eval-results/{id} must not leak secrets from an eval run's session. + + EvalCaseResult.session_details holds the full Session produced by a live + eval run's inferencing/scraping stage -- if a tool needed OAuth during + that run, the same adk_request_credential event (and its full credential) + that /run, /run_sse, and /run_live redact can end up here too, via a + separate code path (`get_eval_result` in dev_server.py) that a test + covering only get_eval (a different endpoint, over EvalCase.conversation + rather than EvalCaseResult.session_details) does not exercise. + """ + mock_eval_set_results_manager.save_eval_set_result( + "test_app", + "my_eval_set", + [ + _eval_case_result_with_credential_session( + "my_eval_set", + "my_eval_case", + "should-never-reach-the-client-via-eval-result", + ) + ], + ) + + response = test_app.get( + "/dev/apps/test_app/eval-results/test_app_my_eval_set_eval_result" + ) + + assert response.status_code == 200 + assert ( + "should-never-reach-the-client-via-eval-result" not in response.text + ) + case_result = response.json()["evalCaseResults"][0] + args = case_result["sessionDetails"]["events"][0]["content"]["parts"][0][ + "functionCall" + ]["args"] + raw_oauth2 = args["authConfig"]["rawAuthCredential"]["oauth2"] + assert "clientSecret" not in raw_oauth2 + assert raw_oauth2["clientId"] == "public-client-id" + + +def test_get_eval_result_legacy_redacts_oauth2_client_secret( + test_app, mock_eval_set_results_manager +): + """Same as test_get_eval_result_redacts_oauth2_client_secret, for the + deprecated /dev/apps/{app}/eval_results/{id} route, which calls a + separate function (get_eval_result_legacy) with its own + _redact_credential_secrets call site.""" + mock_eval_set_results_manager.save_eval_set_result( + "test_app", + "my_eval_set", + [ + _eval_case_result_with_credential_session( + "my_eval_set", + "my_eval_case", + "should-never-reach-the-client-via-eval-result-legacy", + ) + ], + ) + + response = test_app.get( + "/dev/apps/test_app/eval_results/test_app_my_eval_set_eval_result" + ) + + assert response.status_code == 200 + assert ( + "should-never-reach-the-client-via-eval-result-legacy" + not in response.text + ) + case_result = response.json()["evalCaseResults"][0] + args = case_result["sessionDetails"]["events"][0]["content"]["parts"][0][ + "functionCall" + ]["args"] + raw_oauth2 = args["authConfig"]["rawAuthCredential"]["oauth2"] + assert "clientSecret" not in raw_oauth2 + assert raw_oauth2["clientId"] == "public-client-id" + + def test_create_eval_set_legacy_route_creates_eval_set( test_app, mock_eval_sets_manager ): From e63f8533ceaf376bc9e21db01373323f534ddd5f Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Wed, 2 Sep 2026 09:46:29 +0530 Subject: [PATCH 5/7] fix(auth): close a dev-UI trace leak and two test-coverage gaps from re-review Four findings from a second mutation-tested review pass on the credential-redaction fix, addressed in the order they matter: 1. update_session (PATCH .../sessions/{id}) had no test asserting it redacts. It shares _redacted_session_response with get_session, so the shared helper's own internals can't be neutered to isolate this site -- the mutation that matters is whether update_session's own return statement still calls that helper at all. Added the test, confirmed it fails when the return is swapped for a raw unredacted response and every other redaction test stays green. 2. list_sessions' existing test only demonstrates the redaction call is wired up for a response shape (events on a listed session) no bundled SessionService backend actually produces. Added a second, unmonkeypatched test using SessionStateCredentialService's real state-parking path instead -- state (unlike events) does survive list_sessions on both InMemory and Database backends, so this is a real leak path today, not a hypothetical one, and confirmed this test alone kills the same mutant the monkeypatched one does. Kept the original test as a wiring check per the review's own framing. 3. The gcp.vertex.agent.llm_request span attribute, read back via the dev-UI's two debug/trace endpoints, carried an unredacted copy of any adk_request_credential call in the conversation history -- a separate code path from /run, /run_sse, and the session-history endpoints, since _build_llm_request_for_trace builds a fresh dict representation of contents at trace time rather than redacting an already-built Event/Session dict. Confirmed directly: built a real credential-bearing FunctionCall exactly as build_auth_request_event does, ran it through trace_call_llm, and found the secret in the serialized span attribute before this fix and absent after. http_options in the same function is excluded outright, since it never has legitimate debugging value; contents can't be, since the conversation is the actual point of tracing a request. Applied redact_credential_secrets to each content dict before it is serialized into the attribute's string -- doing this after serialization wouldn't work, since the walker can't reach inside an opaque string. This required relocating redact_credential_secrets out of api_server.py: telemetry/tracing.py is a lower-level module, and importing api_server.py's helper into it would risk a circular import. Moved it (renamed, now public) next to CREDENTIAL_SECRET_KEYS in auth_credential.py, a genuine leaf module with no framework dependencies -- confirmed by checking its full import list. api_server.py and dev_server.py now import it from there via the same alias, so every existing call site is unchanged. The two debug/trace endpoints themselves (get_trace_dict, get_session_trace) are unchanged: they're pure pass-throughs of already-recorded span data, so fixing the point where the attribute is built is sufficient -- there was nothing left for them to leak once the source is clean, and no separate endpoint-level redaction is needed. Noted but not exhaustively verified: llm_response (the model's fresh output, traced via a separate span attribute in the same function) is architecturally a different case -- the credential call is ADK's own synthetic injection into request-side contents as conversation history, not something the model generates as a response -- but this wasn't dynamically confirmed the way the contents path was, and is worth a closer look if there's ever a reason to suspect it. Full auth suite (245), the relevant fast_api suite, and telemetry's test_spans.py (134, plus one new dedicated test) all pass, with the same known-unrelated failures as before (missing optional GCP dependencies) and no new regressions. --- src/google/adk/auth/auth_credential.py | 87 +++++++++++++++++ src/google/adk/cli/api_server.py | 74 +------------- src/google/adk/cli/dev_server.py | 2 +- src/google/adk/telemetry/tracing.py | 16 ++- tests/unittests/cli/test_fast_api.py | 123 +++++++++++++++++++++++- tests/unittests/telemetry/test_spans.py | 96 ++++++++++++++++++ 6 files changed, 320 insertions(+), 78 deletions(-) diff --git a/src/google/adk/auth/auth_credential.py b/src/google/adk/auth/auth_credential.py index bb61225d906..6675e3f89a5 100644 --- a/src/google/adk/auth/auth_credential.py +++ b/src/google/adk/auth/auth_credential.py @@ -56,6 +56,93 @@ }) +def _is_credential_shaped(value: Any) -> bool: + """Reports whether `value` is a dict that is itself an `AuthCredential`. + + `authType` is `AuthCredential.auth_type`'s by-alias serialized name, and + every `AuthCredential` dump carries it -- including one parked directly + in session state by `SessionStateCredentialService` under an + app-or-tool-chosen key, which is why this checks shape rather than a + fixed set of container key names like `authConfig`: a credential can + legitimately appear anywhere a `dict[str, Any]` the app controls can + hold one, not only nested under the names ADK's own code happens to use. + """ + return isinstance(value, dict) and "authType" in value + + +def _strip_secret_keys(value: Any) -> Any: + """Unconditionally strips `CREDENTIAL_SECRET_KEYS` from every dict in + `value`, at any depth. + + Only safe to call once a `_is_credential_shaped` dict has already been + found: everything below that point genuinely belongs to a credential + object, so a key named e.g. `token` here is actually a secret, not + coincidentally-named application data. + """ + if isinstance(value, dict): + return { + key: _strip_secret_keys(val) + for key, val in value.items() + if key not in CREDENTIAL_SECRET_KEYS + } + if isinstance(value, list): + return [_strip_secret_keys(item) for item in value] + return value + + +def redact_credential_secrets(value: Any) -> Any: + """Recursively strips credential-secret keys from a JSON-like value. + + An event's `function_call.args` (e.g. the `adk_request_credential` call + ADK issues when a tool needs OAuth) is an opaque `dict[str, Any]`, not a + nested pydantic model -- so `Event.model_dump(exclude=...)` cannot reach + a secret embedded inside it by field path. This walks the already-dumped + event unconditionally, so a credential's `client_secret`, tokens, etc. + never reach an external client over /run, /run_sse, /run_live, any + endpoint that returns session history or eval data, or a trace/span + attribute. This must only be applied to the outbound wire representation + -- never to what SessionService persists -- since later turns + reconstruct the original request's credential by re-parsing the + persisted `adk_request_credential` call's args. + + Stripping is scoped to dicts that are actually `AuthCredential` dumps + (identified by carrying `authType`, see `_is_credential_shaped`), not to + every `CREDENTIAL_SECRET_KEYS` name anywhere in the structure. Several of + those names -- `token`, `password`, `apiKey`, `accessToken` among them -- + are ordinary words a tool's own return value or an app's own session + state can legitimately use for something that isn't a credential at all + (a pagination cursor named `token`, a scraped page's own `password` + field). Deleting those unconditionally would silently drop data the + caller never asked to have redacted, indistinguishable from a key a tool + simply never returned. Scoping to credential-shaped subtrees avoids that + while still catching every real credential location, including one + stored in session state under an arbitrary key -- that dump still + carries `authType`, so shape-based detection finds it without needing a + fixed list of container key names. + + This function is exported publicly (not module-private) because it is + shared across api_server.py, dev_server.py, and telemetry/tracing.py -- + every place a credential-bearing structure reaches an external client or + an exported trace attribute needs the same redaction, and a fixed list + of container key names would need updating at every one of those call + sites for a new credential-bearing structure; shape-based detection does + not. + + Note one thing this does *not* reach: a value that is already a JSON + *string* by the time this runs (e.g. an OpenTelemetry span attribute + built by json-serializing a dict first) is opaque to this walker, which + only descends real dicts and lists. Call this before serializing to a + string, not after. + """ + if _is_credential_shaped(value): + return _strip_secret_keys(value) + if isinstance(value, dict): + return {key: redact_credential_secrets(val) for key, val in value.items()} + if isinstance(value, list): + return [redact_credential_secrets(item) for item in value] + return value + + # Pydantic echoes the rejected value into ValidationError messages # ("input_value=..."), which would put a malformed secret straight into logs and # into the error strings surfaced to the LLM. The field name and error type are diff --git a/src/google/adk/cli/api_server.py b/src/google/adk/cli/api_server.py index 59d620e03c7..0fe2772c221 100644 --- a/src/google/adk/cli/api_server.py +++ b/src/google/adk/cli/api_server.py @@ -79,6 +79,7 @@ from ..errors.input_validation_error import InputValidationError from ..errors.session_not_found_error import SessionNotFoundError from ..auth.auth_credential import CREDENTIAL_SECRET_KEYS +from ..auth.auth_credential import redact_credential_secrets as _redact_credential_secrets from ..events.event import Event from ..events.event_actions import EventActions from ..flows.llm_flows.functions import REQUEST_CONFIRMATION_FUNCTION_CALL_NAME @@ -576,79 +577,6 @@ def _invalid_event_error(event_index: int, disallowed: str) -> HTTPException: ) -def _is_credential_shaped(value: Any) -> bool: - """Reports whether `value` is a dict that is itself an `AuthCredential`. - - `authType` is `AuthCredential.auth_type`'s by-alias serialized name, and - every `AuthCredential` dump carries it -- including one parked directly - in session state by `SessionStateCredentialService` under an - app-or-tool-chosen key, which is why this checks shape rather than a - fixed set of container key names like `authConfig`: a credential can - legitimately appear anywhere a `dict[str, Any]` the app controls can - hold one, not only nested under the names ADK's own code happens to use. - """ - return isinstance(value, dict) and "authType" in value - - -def _strip_secret_keys(value: Any) -> Any: - """Unconditionally strips `CREDENTIAL_SECRET_KEYS` from every dict in - `value`, at any depth. - - Only safe to call once a `_is_credential_shaped` dict has already been - found: everything below that point genuinely belongs to a credential - object, so a key named e.g. `token` here is actually a secret, not - coincidentally-named application data. - """ - if isinstance(value, dict): - return { - key: _strip_secret_keys(val) - for key, val in value.items() - if key not in CREDENTIAL_SECRET_KEYS - } - if isinstance(value, list): - return [_strip_secret_keys(item) for item in value] - return value - - -def _redact_credential_secrets(value: Any) -> Any: - """Recursively strips credential-secret keys from a JSON-like value. - - An event's `function_call.args` (e.g. the `adk_request_credential` call - ADK issues when a tool needs OAuth) is an opaque `dict[str, Any]`, not a - nested pydantic model -- so `Event.model_dump(exclude=...)` cannot reach - a secret embedded inside it by field path. This walks the already-dumped - event unconditionally, so a credential's `client_secret`, tokens, etc. - never reach an external client over /run, /run_sse, /run_live, or any - endpoint that returns session history or eval data. This must only be - applied to the outbound wire representation -- never to what - SessionService persists -- since later turns reconstruct the original - request's credential by re-parsing the persisted `adk_request_credential` - call's args. - - Stripping is scoped to dicts that are actually `AuthCredential` dumps - (identified by carrying `authType`, see `_is_credential_shaped`), not to - every `CREDENTIAL_SECRET_KEYS` name anywhere in the structure. Several of - those names -- `token`, `password`, `apiKey`, `accessToken` among them -- - are ordinary words a tool's own return value or an app's own session - state can legitimately use for something that isn't a credential at all - (a pagination cursor named `token`, a scraped page's own `password` - field). Deleting those unconditionally would silently drop data the - caller never asked to have redacted, indistinguishable from a key a tool - simply never returned. Scoping to credential-shaped subtrees avoids that - while still catching every real credential location, including one - stored in session state under an arbitrary key -- that dump still - carries `authType`, so shape-based detection finds it without needing a - fixed list of container key names. - """ - if _is_credential_shaped(value): - return _strip_secret_keys(value) - if isinstance(value, dict): - return {key: _redact_credential_secrets(val) for key, val in value.items()} - if isinstance(value, list): - return [_redact_credential_secrets(item) for item in value] - return value - - def _redacted_session_response(session: Session) -> JSONResponse: """Returns a `Session` as a `JSONResponse` with credential secrets stripped. diff --git a/src/google/adk/cli/dev_server.py b/src/google/adk/cli/dev_server.py index ce99a5a023d..599fc6b5a69 100644 --- a/src/google/adk/cli/dev_server.py +++ b/src/google/adk/cli/dev_server.py @@ -64,6 +64,7 @@ from . import agent_graph from ..apps.app import App +from ..auth.auth_credential import redact_credential_secrets as _redact_credential_secrets from ..errors.not_found_error import NotFoundError from ..evaluation.base_eval_service import InferenceConfig from ..evaluation.base_eval_service import InferenceRequest @@ -80,7 +81,6 @@ from ..evaluation.eval_set import EvalSet from ..utils._telemetry_config import read_telemetry_consent from ..utils._telemetry_config import write_telemetry_consent -from .api_server import _redact_credential_secrets from .api_server import ApiServer NESTED_APP_SEPARATOR = "." diff --git a/src/google/adk/telemetry/tracing.py b/src/google/adk/telemetry/tracing.py index 265eaf086e0..891baf9dc44 100644 --- a/src/google/adk/telemetry/tracing.py +++ b/src/google/adk/telemetry/tracing.py @@ -71,6 +71,7 @@ from typing_extensions import deprecated from .. import version +from ..auth.auth_credential import redact_credential_secrets from ..utils.env_utils import is_enterprise_mode_enabled from ..utils.model_name_utils import extract_model_name from ..utils.model_name_utils import is_gemini_model @@ -878,11 +879,24 @@ def _build_llm_request_for_trace(llm_request: LlmRequest) -> dict[str, object]: for part in content.parts if not part.inline_data ] - result["contents"].append( + # Unlike http_options above, contents can't simply be excluded: the + # conversation is the actual point of tracing an LLM request. But the + # conversation can carry an adk_request_credential function call's full + # AuthCredential (see build_auth_request_event in + # flows/llm_flows/functions.py) or one parked in state and later + # replayed into a turn, so it needs the same redaction /run, /run_sse, + # and the session-history endpoints already apply -- otherwise this + # span attribute becomes an unredacted copy of the same secret, + # readable from the dev-UI debug/trace endpoints. Applied here, before + # the dict is serialized to the string this attribute actually stores + # (see safe_json_serialize above): once it's a string, it's opaque to + # this walker, which only descends real dicts and lists. + content_dict = redact_credential_secrets( types.Content(role=content.role, parts=parts).model_dump( exclude_none=True, mode="json" ) ) + result["contents"].append(content_dict) return result diff --git a/tests/unittests/cli/test_fast_api.py b/tests/unittests/cli/test_fast_api.py index 11d8f9c61fc..2e8fc94b714 100644 --- a/tests/unittests/cli/test_fast_api.py +++ b/tests/unittests/cli/test_fast_api.py @@ -33,6 +33,9 @@ from google.adk.agents.llm_agent import LlmAgent from google.adk.agents.run_config import RunConfig from google.adk.artifacts.base_artifact_service import ArtifactVersion +from google.adk.auth.auth_credential import AuthCredential +from google.adk.auth.auth_credential import AuthCredentialTypes +from google.adk.auth.auth_credential import OAuth2Auth from google.adk.cli import fast_api as fast_api_module from google.adk.cli.fast_api import get_fast_api_app from google.adk.errors.input_validation_error import InputValidationError @@ -1798,6 +1801,77 @@ def test_update_session(test_app, create_test_session): state_delta_in_event = actions.get("stateDelta") assert state_delta_in_event == state_delta + +async def test_update_session_redacts_oauth2_client_secret( + test_app, create_test_session, mock_session_service +): + """PATCH .../sessions/{id} returns the whole session, history included. + + update_session returns the full Session, not just the state delta the + caller sent -- so an adk_request_credential call persisted by an earlier + turn rides out on the response exactly as it does on GET, through the + same _redacted_session_response call. This is a separate site from + get_session (both route through the same helper, but a regression + removing the call from just one of them would not be caught by a test + covering only the other). + """ + info = create_test_session + + auth_config_dict = { + "authScheme": {"type": "oauth2", "flows": {}}, + "rawAuthCredential": { + "authType": "oauth2", + "oauth2": { + "clientId": "public-client-id", + "clientSecret": "should-never-reach-the-client-via-patch", + }, + }, + "credentialKey": "my_tool:oauth2:abcd1234", + } + + session = await mock_session_service.get_session( + app_name=info["app_name"], + user_id=info["user_id"], + session_id=info["session_id"], + ) + await mock_session_service.append_event( + session=session, + event=Event( + author="agent", + invocation_id="invocation_id", + content=types.Content( + role="user", + parts=[ + types.Part( + function_call=types.FunctionCall( + name="adk_request_credential", + id="adk-req-cred-id", + args={ + "functionCallId": "adk-original-fc-id", + "authConfig": auth_config_dict, + }, + ) + ) + ], + ), + ), + ) + + url = ( + f"/apps/{info['app_name']}/users/{info['user_id']}" + f"/sessions/{info['session_id']}" + ) + response = test_app.patch(url, json={"state_delta": {"counter": 1}}) + + assert response.status_code == 200 + assert "should-never-reach-the-client-via-patch" not in response.text + event = response.json()["events"][0] + raw_oauth2 = event["content"]["parts"][0]["functionCall"]["args"][ + "authConfig" + ]["rawAuthCredential"]["oauth2"] + assert "clientSecret" not in raw_oauth2 + assert raw_oauth2["clientId"] == "public-client-id" + logger.info("Session state patched successfully") @@ -5259,9 +5333,14 @@ async def test_list_sessions_redacts_oauth2_client_secret( in `_list_sessions_impl`), which makes a secret-in-events check against the real backend vacuously pass regardless of whether `_redacted_sessions_response` does anything at all -- there's never a - secret in that response to begin with. `list_sessions` is monkeypatched - here to actually include events, so this exercises the real redaction - call the endpoint makes rather than a check that can't fail either way. + secret in that response to begin with, confirmed on both bundled + SessionService backends (InMemory and Database), not just assumed. + `list_sessions` is monkeypatched here to actually include events, so this + is a wiring check that the redaction call is present and functions for a + response shape a real backend does not currently produce -- see + test_list_sessions_redacts_credential_parked_in_state below for the test + that proves the same redaction call guards something a real backend does + return today. """ info = create_test_session @@ -5337,6 +5416,44 @@ async def list_sessions_including_events(self, *, app_name, user_id=None): ) +async def test_list_sessions_redacts_credential_parked_in_state( + test_app, mock_session_service +): + """list_sessions strips events but returns state, and a credential can + live there without any monkeypatch. + + SessionStateCredentialService.save_credential parks a live + AuthCredential directly in session state under + auth_config.credential_key -- a second, independent path onto the + response, unrelated to the adk_request_credential function-call events + the tests above cover. Both bundled backends (InMemory, Database) return + state from list_sessions while dropping events, so this is a real leak + path today, not a hypothetical one, and this test needs no + monkeypatching to demonstrate it: it constructs exactly the response + shape a real SessionService produces. + """ + credential = AuthCredential( + auth_type=AuthCredentialTypes.OAUTH2, + oauth2=OAuth2Auth( + client_id="public-client-id", + client_secret="should-never-reach-the-client-via-state", + ), + ) + await mock_session_service.create_session( + app_name="test_app_name", + user_id="test_user", + state={"adk_oauth2_scheme_oauth2_cred": credential}, + ) + + response = test_app.get("/apps/test_app_name/users/test_user/sessions") + + assert response.status_code == 200 + assert "should-never-reach-the-client-via-state" not in response.text + parked = response.json()[0]["state"]["adk_oauth2_scheme_oauth2_cred"] + assert "clientSecret" not in parked["oauth2"] + assert parked["oauth2"]["clientId"] == "public-client-id" + + @pytest.mark.xfail( strict=True, reason="add-session maps ValueError, but the managers raise NotFoundError", diff --git a/tests/unittests/telemetry/test_spans.py b/tests/unittests/telemetry/test_spans.py index 57a842f2885..fcfa0eaa89d 100644 --- a/tests/unittests/telemetry/test_spans.py +++ b/tests/unittests/telemetry/test_spans.py @@ -18,9 +18,17 @@ from typing import Optional from unittest import mock +from fastapi.openapi.models import OAuth2 +from fastapi.openapi.models import OAuthFlowAuthorizationCode +from fastapi.openapi.models import OAuthFlows + from google.adk.agents.invocation_context import InvocationContext from google.adk.agents.llm_agent import LlmAgent from google.adk.agents.run_config import RunConfig +from google.adk.auth.auth_credential import AuthCredential +from google.adk.auth.auth_credential import OAuth2Auth +from google.adk.auth.auth_tool import AuthConfig +from google.adk.auth.auth_tool import AuthToolArguments from google.adk.errors.tool_execution_error import ToolErrorType from google.adk.errors.tool_execution_error import ToolExecutionError from google.adk.events.event import Event @@ -238,6 +246,94 @@ async def test_trace_call_llm(monkeypatch, mock_span_fixture): mock_span_fixture.set_attributes.assert_called_once_with(expected_usage_attrs) +@pytest.mark.asyncio +async def test_trace_call_llm_redacts_oauth2_client_secret_from_contents( + monkeypatch, mock_span_fixture +): + """The gcp.vertex.agent.llm_request span attribute must not carry a + credential secret embedded in an adk_request_credential function call + within contents. + + contents accumulates conversation history across turns, and an earlier + turn's adk_request_credential call (see build_auth_request_event in + flows/llm_flows/functions.py) -- carrying the tool's full AuthCredential, + including client_secret -- becomes part of a later turn's llm_request + when it is replayed. Unlike http_options (excluded outright a few lines + above in _build_llm_request_for_trace, since it never has legitimate + debugging value), contents can't simply be excluded: the conversation is + the actual point of tracing an LLM request. This is a different code + path from /run, /run_sse, and the session-history endpoints: those + redact an already-built Event/Session dict; this one builds a fresh + dict representation of contents at trace time and must redact it before + it is serialized into the string this span attribute actually stores, + since redaction can't reach inside an opaque string afterward. + """ + monkeypatch.setattr( + 'opentelemetry.trace.get_current_span', lambda: mock_span_fixture + ) + + auth_scheme = OAuth2( + flows=OAuthFlows( + authorizationCode=OAuthFlowAuthorizationCode( + authorizationUrl='https://idp.example.com/oauth2/auth', + tokenUrl='https://idp.example.com/oauth2/token', + scopes={'read': 'read'}, + ) + ) + ) + credential = AuthCredential( + auth_type='oauth2', + oauth2=OAuth2Auth( + client_id='public-client-id', + client_secret='should-never-reach-the-trace', + ), + ) + auth_config = AuthConfig( + auth_scheme=auth_scheme, + raw_auth_credential=credential, + credential_key='my_tool:oauth2:abcd1234', + ) + # Built exactly as build_auth_request_event does, in + # flows/llm_flows/functions.py. + args = AuthToolArguments( + function_call_id='adk-original-fc-id', auth_config=auth_config + ).model_dump(mode='json', exclude_none=True, by_alias=True) + credential_request_content = types.Content( + role='user', + parts=[ + types.Part( + function_call=types.FunctionCall( + name='adk_request_credential', id='adk-req-cred-id', args=args + ) + ) + ], + ) + + agent = LlmAgent(name='test_agent') + invocation_context = await _create_invocation_context(agent) + llm_request = LlmRequest( + model='gemini-pro', + contents=[ + credential_request_content, + types.Content(role='user', parts=[types.Part(text='continue')]), + ], + ) + llm_response = LlmResponse(turn_complete=True) + + trace_call_llm(invocation_context, 'test_event_id', llm_request, llm_response) + + llm_request_calls = [ + call + for call in mock_span_fixture.set_attribute.call_args_list + if call.args[0] == 'gcp.vertex.agent.llm_request' + ] + assert len(llm_request_calls) == 1 + serialized = llm_request_calls[0].args[1] + assert 'should-never-reach-the-trace' not in serialized + assert 'public-client-id' in serialized + assert 'my_tool:oauth2:abcd1234' in serialized + + @pytest.mark.asyncio async def test_trace_call_llm_skips_non_recording_span(monkeypatch): agent = LlmAgent(name='test_agent') From 8337b415f7dd81259283268152b4982ba73bf4d7 Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Thu, 3 Sep 2026 15:57:18 +0530 Subject: [PATCH 6/7] fix(telemetry): redact credentials in trace_merged_tool_calls too Addresses tonydzi's review: trace_merged_tool_calls dumps the whole merged event unredacted, including actions.state_delta -- exactly where SessionStateCredentialService.save_credential parks an exchanged AuthCredential under an app-chosen key. This is the same dev-UI trace surface the earlier fencing/redaction work in this file closed for other spans, just not this one: its own docstring says it exists only to serve /debug/trace requests, so the same requirement applies. Confirmed via a real runner: with two tools called in one turn, one of them completing OAuth the documented way, the merged span's gcp.vertex.agent.tool_response attribute carried clientSecret, accessToken, and refreshToken in full, served unredacted by both endpoints backed by this span (GET /debug/trace/{event_id} and GET /debug/trace/session/{session_id}). Pre-existing on the merge-base identically, not a regression from the earlier redaction work in this file. Fix redacts the dict before serializing it, same principle as everywhere else redact_credential_secrets is used: redaction can't reach inside an already-built string. Two things worth being explicit about, since both cost real debugging time to work out: Separators matter. json.dumps's default separators insert a space after each one; model_dump_json's do not. Using json.dumps's defaults here would have changed this attribute's exact bytes for every event this function traces, not just ones carrying a credential, silently breaking the existing test_trace_merged_tool_calls_sets_correct_attributes (which asserts byte-equality against model_dump_json's output). Verified the fix's actual output is byte-identical to the old model_dump_json call's output for a credential-free event by running both against the real, unmodified Event class. redact_credential_secrets keys on the by-alias field spelling (authType, not auth_type), which the dict this function serializes actually carries in practice -- confirmed against a real end-to-end run that SessionStateCredentialService's stored value is byte-equal to AuthCredential.model_dump(by_alias=True, exclude_none=True, mode="json"). Documented this explicitly in redact_credential_secrets' own docstring, since BaseCredentialService.save_credential is a public extension point, and a different implementation storing the model object itself (or dumping without by_alias=True) would get a silent no-op here rather than an error. Adds test_trace_merged_tool_calls_redacts_credential_in_state_delta, confirmed to fail against the pre-fix code (the leaked secret visible directly in the assertion diff) and pass against the fix, with the adjacent non-credential test (test_trace_merged_tool_calls_sets_correct_attributes) confirmed unaffected in both directions. Full telemetry and auth suites: 585 passed, 1 skipped, with the same 2 known-unrelated failures as the merge-base (missing optional opentelemetry.instrumentation dependency, confirmed via git stash to predate this change), plus a handful of telemetry test files that fail to collect at all in this environment for the same missing dependency -- excluded from this run, not touched by this change. --- src/google/adk/auth/auth_credential.py | 15 ++++++ src/google/adk/telemetry/tracing.py | 24 ++++++++- tests/unittests/telemetry/test_spans.py | 69 +++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 2 deletions(-) diff --git a/src/google/adk/auth/auth_credential.py b/src/google/adk/auth/auth_credential.py index 6675e3f89a5..ac2e1dd7ae8 100644 --- a/src/google/adk/auth/auth_credential.py +++ b/src/google/adk/auth/auth_credential.py @@ -133,6 +133,21 @@ def redact_credential_secrets(value: Any) -> Any: built by json-serializing a dict first) is opaque to this walker, which only descends real dicts and lists. Call this before serializing to a string, not after. + + `_is_credential_shaped` keys on the by-alias field spelling + (`authType`, not `auth_type`), since that's what every dump this + function is actually called on carries in practice -- an + outbound-facing dict is by-alias by construction (matching the wire + format), and a dict parked in session state by + `SessionStateCredentialService.save_credential` is too, verified + against a real end-to-end run to be byte-equal to + `AuthCredential.model_dump(by_alias=True, exclude_none=True, + mode="json")`. `BaseCredentialService.save_credential` is a public + extension point, though, and a different implementation that stores + the model object itself, or dumps it without `by_alias=True`, would + get a silent no-op here rather than an error: `_is_credential_shaped` + would find no `authType` key, conclude the dict isn't a credential, + and leave it untouched. """ if _is_credential_shaped(value): return _strip_secret_keys(value) diff --git a/src/google/adk/telemetry/tracing.py b/src/google/adk/telemetry/tracing.py index 891baf9dc44..e194f1c2c3c 100644 --- a/src/google/adk/telemetry/tracing.py +++ b/src/google/adk/telemetry/tracing.py @@ -29,6 +29,7 @@ from contextlib import asynccontextmanager from contextlib import contextmanager from contextlib import ExitStack +import json import logging import os import re @@ -532,8 +533,27 @@ def trace_merged_tool_calls( span.set_attribute("gcp.vertex.agent.event_id", response_event_id) if telemetry_config.should_add_content_to_legacy_spans: try: - function_response_event_json = function_response_event.model_dump_json( - exclude_none=True + # Unlike the other spans this function's docstring says it exists + # to unblock (dev-UI /debug/trace requests), this one dumps the + # whole merged event -- and an event's actions.state_delta is + # exactly where SessionStateCredentialService.save_credential parks + # an exchanged AuthCredential under an app-chosen key (see + # _is_credential_shaped's docstring). model_dump_json would emit + # that credential dict unredacted; redacting the dict before + # serializing it to a string is required, same as elsewhere in + # this file, since redaction can't reach inside an already-built + # string. separators=(",", ":") matches model_dump_json's default, + # compact output -- json.dumps's own default inserts a space after + # each separator, which would change this attribute's bytes for + # every event, not just ones carrying a credential. + function_response_event_json = json.dumps( + redact_credential_secrets( + function_response_event.model_dump( + exclude_none=True, mode="json" + ) + ), + ensure_ascii=False, + separators=(",", ":"), ) except Exception: # pylint: disable=broad-exception-caught function_response_event_json = "" diff --git a/tests/unittests/telemetry/test_spans.py b/tests/unittests/telemetry/test_spans.py index fcfa0eaa89d..2c364dbc299 100644 --- a/tests/unittests/telemetry/test_spans.py +++ b/tests/unittests/telemetry/test_spans.py @@ -32,6 +32,7 @@ from google.adk.errors.tool_execution_error import ToolErrorType from google.adk.errors.tool_execution_error import ToolExecutionError from google.adk.events.event import Event +from google.adk.events.event_actions import EventActions from google.adk.models.cache_metadata import CacheMetadata from google.adk.models.llm_request import LlmRequest from google.adk.models.llm_response import LlmResponse @@ -1169,6 +1170,74 @@ def test_trace_merged_tool_calls_sets_correct_attributes( assert 'merged_details' in recorded_response +def test_trace_merged_tool_calls_redacts_credential_in_state_delta( + monkeypatch, mock_span_fixture +): + """The merged-event span this function builds dumps the whole event, + including actions.state_delta -- exactly where + SessionStateCredentialService.save_credential parks an exchanged + AuthCredential under an app-chosen key (see redact_credential_secrets' + docstring). Unlike the other spans this function's own docstring says + it exists to unblock (dev-UI /debug/trace requests), this one was not + covered by the earlier redaction work in this file. + """ + monkeypatch.setattr( + 'opentelemetry.trace.get_current_span', lambda: mock_span_fixture + ) + + credential = AuthCredential( + auth_type='oauth2', + oauth2=OAuth2Auth( + client_id='public-client-id', + client_secret='should-never-reach-the-trace', + ), + ) + merged_event = Event( + invocation_id='inv-1', + author='root_agent', + id='merged-event-id', + content=types.Content( + role='user', + parts=[ + types.Part( + function_response=types.FunctionResponse( + id='fc-1', + name='connect_calendar', + response={'status': 'connected'}, + ) + ) + ], + ), + actions=EventActions( + state_delta={ + 'my_tool:oauth2:abcd1234': credential.model_dump( + by_alias=True, exclude_none=True, mode='json' + ) + } + ), + ) + + trace_merged_tool_calls( + response_event_id=merged_event.id, + function_response_event=merged_event, + ) + + calls = [ + call_obj + for call_obj in mock_span_fixture.set_attribute.call_args_list + if call_obj.args[0] == 'gcp.vertex.agent.tool_response' + ] + assert len(calls) == 1 + serialized = calls[0].args[1] + + assert 'should-never-reach-the-trace' not in serialized + # Must not "redact" by blanking the whole attribute the UI renders -- + # everything else the merged event carries should still be there. + assert 'public-client-id' in serialized + assert 'connect_calendar' in serialized + assert 'my_tool:oauth2:abcd1234' in serialized + + def test_trace_tool_call_skips_non_recording_span( monkeypatch, mock_tool_fixture, mock_event_fixture ): From d7a425f0cd3880842e4716c2a818cee9db20d9d0 Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Fri, 4 Sep 2026 09:25:20 +0530 Subject: [PATCH 7/7] fix(telemetry): use pydantic_core.to_json directly, close two test gaps Addresses tonydzi's re-review of the trace_merged_tool_calls fix. 1. The byte-identity claim in the previous commit was true for 22 of 23 payload shapes and false for one: json.dumps and pydantic-core's float formatting disagree for a narrow band of small-magnitude negative exponents (1e-9 through 1e-5, and any mantissa in that range) -- repr()'s two-digit-minimum exponent padding (1e-09) is not pydantic-core's (1e-9). This is reachable from an ordinary tool return value, not just from a credential: a latency in seconds, a p-value, a small score. pydantic_core.to_json is the exact serializer model_dump_json calls internally, so redacting the dict first and serializing with it keeps this attribute's bytes identical to the old model_dump_json call's for every payload shape, not just the ones json.dumps happens to agree on. Confirmed directly: all 8 of the exponent values from the review are byte-identical to model_dump_json's own output now, where json.dumps disagreed on 7 of them. Also simpler: drops `import json`, the separators and ensure_ascii arguments, and the comment explaining them. Not a new dependency -- already imported elsewhere in this repo (sessions/_session_util.py, events/event_actions.py). Added a parametrized regression test (test_trace_merged_tool_calls_matches_model_dump_json_byte_for_byte) covering exactly the exponent values the review found, plus a control case with no float. Confirmed it fails on exactly those 7 cases (not the control) when reverted to json.dumps, matching the review's own finding precisely. 2. Mutation testing in the review found CREDENTIAL_SECRET_KEYS' strip set was only partially pinned: a mutant keeping just the 2 names (clientSecret, accessToken) that a real AuthCredential's own fields happen to exercise elsewhere in this file survived across the wide suite. The other 11 -- password, token, additionalHeaders, authResponseUri, authCode, refreshToken, idToken, codeVerifier, privateKeyId, privateKey, apiKey -- appeared in no test. Added a parametrized test (test_trace_merged_tool_calls_strips_every_credential_secret_key) covering all 13 names individually, hardcoded rather than sourced from CREDENTIAL_SECRET_KEYS itself -- parametrizing off the live constant would mean a mutant narrowing the constant also narrows the test's own case list, so it could never fail no matter how much the constant shrank. Confirmed: reproducing the review's exact mutant (narrowing the constant to those same 2 names) now fails the other 11 parametrized cases, where before this commit all 13 would have silently passed (11 vacuously, since nothing exercised them; the constant itself wasn't under test). 3. tonydzi's own hypothesis about this fix turned out wrong, in a way worth documenting rather than leaving to be rediscovered: SessionStateCredentialService.save_credential parks a raw AuthCredential object (not a dump) in state_delta, whose snake_case fields would not be recognized as credential-shaped by redact_credential_secrets, which keys on the aliased spelling. Confirmed directly: redacting a snake_case dump of a credential leaves the secret intact; redacting the by-alias dump strips it. The reason there is no live leak is that merge_parallel_function_response_events (functions.py) always re-validates through `event.actions.model_dump(..., by_alias=True)` before a merged event ever reaches trace_merged_tool_calls, which is what actually puts the aliased keys there. That by_alias=True was already commented for an unrelated reason (enum field handling); added a second comment stating plainly that it is now also load-bearing for credential redaction, and naming the one test that would catch a regression here (test_functions_request_euc.py::test_function_request_euc) despite giving no indication in its own name or docstring that a credential leak is what its failure would mean. Verified: tests/unittests/telemetry/ (excluding files that fail to collect in this environment for a missing optional opentelemetry.instrumentation dependency), tests/unittests/auth/, and tests/unittests/flows/llm_flows/test_functions_request_euc.py -- 612 passed, with the same 3 known-unrelated failures as before (missing optional dependencies: discoveryengine_v1beta, opentelemetry.instrumentation version mismatch). --- src/google/adk/flows/llm_flows/functions.py | 17 +++ src/google/adk/telemetry/tracing.py | 22 +-- tests/unittests/telemetry/test_spans.py | 144 ++++++++++++++++++++ 3 files changed, 173 insertions(+), 10 deletions(-) diff --git a/src/google/adk/flows/llm_flows/functions.py b/src/google/adk/flows/llm_flows/functions.py index 24ea7564725..63d6f56e29e 100644 --- a/src/google/adk/flows/llm_flows/functions.py +++ b/src/google/adk/flows/llm_flows/functions.py @@ -1694,6 +1694,23 @@ def merge_parallel_function_response_events( aggregated_ui_widgets.extend(ui_widgets) # Use `by_alias=True` because it converts the model to a dictionary while respecting field aliases, ensuring that the enum fields are correctly handled without creating a duplicate. + # + # This by_alias=True is also, separately, load-bearing for credential + # redaction: SessionStateCredentialService.save_credential parks a raw + # AuthCredential object (not a dump) in state_delta, whose snake_case + # fields (auth_type, client_secret) would NOT be recognized as + # credential-shaped by redact_credential_secrets, which keys on the + # aliased spelling (authType) -- see that function's docstring. The + # merged event only reaches trace_merged_tool_calls (telemetry/tracing.py) + # after passing through this by-alias re-validation, which is what + # actually puts authType there and lets the credential get stripped + # before it reaches an exported trace attribute. If this ever changes + # to skip re-aliasing (or to merge a raw AuthCredential object through + # some other path), a credential parked in state_delta would reach a + # trace attribute intact. The one test that would catch that today is + # tests/unittests/flows/llm_flows/test_functions_request_euc.py::test_function_request_euc, + # which is about EUC request semantics and gives no hint that its + # failure means a credential leak reopened. merged_actions_data = deep_merge_dicts( merged_actions_data, actions_dict, diff --git a/src/google/adk/telemetry/tracing.py b/src/google/adk/telemetry/tracing.py index e194f1c2c3c..73b545d451c 100644 --- a/src/google/adk/telemetry/tracing.py +++ b/src/google/adk/telemetry/tracing.py @@ -29,7 +29,6 @@ from contextlib import asynccontextmanager from contextlib import contextmanager from contextlib import ExitStack -import json import logging import os import re @@ -69,6 +68,7 @@ from opentelemetry.trace import Status from opentelemetry.trace import StatusCode from opentelemetry.util.types import AttributeValue +from pydantic_core import to_json from typing_extensions import deprecated from .. import version @@ -542,19 +542,21 @@ def trace_merged_tool_calls( # that credential dict unredacted; redacting the dict before # serializing it to a string is required, same as elsewhere in # this file, since redaction can't reach inside an already-built - # string. separators=(",", ":") matches model_dump_json's default, - # compact output -- json.dumps's own default inserts a space after - # each separator, which would change this attribute's bytes for - # every event, not just ones carrying a credential. - function_response_event_json = json.dumps( + # string. pydantic_core.to_json is the exact serializer + # model_dump_json calls internally, so redacting the dict first and + # serializing with it keeps this attribute's bytes identical to the + # old model_dump_json call's for every payload shape -- json.dumps + # does not: it and pydantic-core's float formatting disagree for a + # narrow band of small-magnitude exponents (1e-9 vs 1e-09), a real + # value shape a tool's own return data can carry (a latency, a + # p-value, a small score), not just a hypothetical one. + function_response_event_json = to_json( redact_credential_secrets( function_response_event.model_dump( exclude_none=True, mode="json" ) - ), - ensure_ascii=False, - separators=(",", ":"), - ) + ) + ).decode() except Exception: # pylint: disable=broad-exception-caught function_response_event_json = "" diff --git a/tests/unittests/telemetry/test_spans.py b/tests/unittests/telemetry/test_spans.py index 2c364dbc299..99225a258c5 100644 --- a/tests/unittests/telemetry/test_spans.py +++ b/tests/unittests/telemetry/test_spans.py @@ -1238,6 +1238,150 @@ def test_trace_merged_tool_calls_redacts_credential_in_state_delta( assert 'my_tool:oauth2:abcd1234' in serialized +@pytest.mark.parametrize( + 'case_name,value', + [ + ('control_no_float', 'no float here'), + ('exp_minus_9', 1e-9), + ('exp_minus_8', 1e-8), + ('exp_minus_7', 1e-7), + ('exp_minus_6', 1e-6), + ('exp_minus_5', 1e-5), + ('mantissa_exp_minus_7', 1.23e-7), + ('negative_exp_minus_7', -1e-7), + ], +) +def test_trace_merged_tool_calls_matches_model_dump_json_byte_for_byte( + monkeypatch, mock_span_fixture, case_name, value +): + """A tool's own return value reaches this attribute unredacted (no + credential involved at all here), so whatever serializes it has to + match model_dump_json's exact bytes for every payload shape a tool can + return, not just credential-shaped ones -- a small-magnitude float + (a latency in seconds, a p-value, a score) is exactly as real a shape + as a credential is. + + json.dumps and pydantic-core's float formatting disagree for a narrow + band of small negative exponents (repr()'s two-digit-minimum exponent + padding is not pydantic-core's), which a fix using json.dumps would + silently reproduce for these exact cases while passing every other + payload shape. + """ + merged_event = Event( + invocation_id='inv-1', + author='root_agent', + id='test_event_id', + content=types.Content( + role='user', + parts=[ + types.Part( + function_response=types.FunctionResponse( + id='fc-1', + name='measure', + response={'value': value}, + ) + ) + ], + ), + ) + expected = merged_event.model_dump_json(exclude_none=True) + + monkeypatch.setattr( + 'opentelemetry.trace.get_current_span', lambda: mock_span_fixture + ) + trace_merged_tool_calls( + response_event_id=merged_event.id, + function_response_event=merged_event, + ) + calls = [ + call_obj + for call_obj in mock_span_fixture.set_attribute.call_args_list + if call_obj.args[0] == 'gcp.vertex.agent.tool_response' + ] + assert len(calls) == 1 + assert calls[0].args[1] == expected + + +@pytest.mark.parametrize( + 'secret_key', + [ + # Hardcoded rather than sourced from CREDENTIAL_SECRET_KEYS itself: + # parametrizing off the live constant would mean a mutant that + # narrows the constant also narrows this test's own case list, + # so it could never fail no matter how much the constant shrank. + # This list is the thing pinning what CREDENTIAL_SECRET_KEYS is + # supposed to contain. + 'password', + 'token', + 'additionalHeaders', + 'clientSecret', + 'authResponseUri', + 'authCode', + 'accessToken', + 'refreshToken', + 'idToken', + 'codeVerifier', + 'privateKeyId', + 'privateKey', + 'apiKey', + ], +) +def test_trace_merged_tool_calls_strips_every_credential_secret_key( + monkeypatch, mock_span_fixture, secret_key +): + """Each of the 13 names in CREDENTIAL_SECRET_KEYS individually, not just + the subset (clientSecret, accessToken) the other tests in this file + happen to exercise through a real AuthCredential's own fields. + Narrowing the strip set to only the tested 6 was invisible to the + suite before this: additionalHeaders, authResponseUri, authCode, + refreshToken, idToken, privateKeyId, and privateKey appeared in no + test in this file, auth's own tests, or test_fast_api.py's. + """ + secret_value = f'should-never-reach-the-trace-{secret_key}' + merged_event = Event( + invocation_id='inv-1', + author='root_agent', + id='test_event_id', + content=types.Content( + role='user', + parts=[ + types.Part( + function_response=types.FunctionResponse( + id='fc-1', + name='connect_calendar', + response={'status': 'connected'}, + ) + ) + ], + ), + actions=EventActions( + state_delta={ + 'my_tool:oauth2:abcd1234': { + 'authType': 'oauth2', + secret_key: secret_value, + } + } + ), + ) + + monkeypatch.setattr( + 'opentelemetry.trace.get_current_span', lambda: mock_span_fixture + ) + trace_merged_tool_calls( + response_event_id=merged_event.id, + function_response_event=merged_event, + ) + calls = [ + call_obj + for call_obj in mock_span_fixture.set_attribute.call_args_list + if call_obj.args[0] == 'gcp.vertex.agent.tool_response' + ] + assert len(calls) == 1 + serialized = calls[0].args[1] + assert secret_value not in serialized + assert 'oauth2' in serialized + + def test_trace_tool_call_skips_non_recording_span( monkeypatch, mock_tool_fixture, mock_event_fixture ):