diff --git a/src/google/adk/auth/auth_credential.py b/src/google/adk/auth/auth_credential.py index d4ffa47b69b..ac2e1dd7ae8 100644 --- a/src/google/adk/auth/auth_credential.py +++ b/src/google/adk/auth/auth_credential.py @@ -30,6 +30,133 @@ _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", +}) + + +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. + + `_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) + 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 diff --git a/src/google/adk/cli/api_server.py b/src/google/adk/cli/api_server.py index 34ad0a824b5..0fe2772c221 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,8 @@ 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 ..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 @@ -574,6 +577,35 @@ def _invalid_event_error(event_index: int, disallowed: str) -> HTTPException: ) +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. @@ -1452,36 +1484,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( @@ -1493,25 +1528,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) @@ -1527,7 +1565,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( @@ -1539,6 +1577,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( @@ -1546,7 +1585,7 @@ async def update_session( user_id: str, session_id: str, req: UpdateSessionRequest, - ) -> Session: + ) -> Response: """Updates session state without running the agent. Args: @@ -1585,7 +1624,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", @@ -1825,8 +1864,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 +1924,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 +2009,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 +2154,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/src/google/adk/cli/dev_server.py b/src/google/adk/cli/dev_server.py index fa3807df394..599fc6b5a69 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 @@ -62,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 @@ -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/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 265eaf086e0..73b545d451c 100644 --- a/src/google/adk/telemetry/tracing.py +++ b/src/google/adk/telemetry/tracing.py @@ -68,9 +68,11 @@ 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 +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 @@ -531,9 +533,30 @@ 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. 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" + ) + ) + ).decode() except Exception: # pylint: disable=broad-exception-caught function_response_event_json = "" @@ -878,11 +901,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/auth/test_auth_credential.py b/tests/unittests/auth/test_auth_credential.py index 732a6ae6a51..7fa5ce61388 100644 --- a/tests/unittests/auth/test_auth_credential.py +++ b/tests/unittests/auth/test_auth_credential.py @@ -16,13 +16,18 @@ 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 +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 +128,44 @@ 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. + + 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 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, ( + '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}' + ) + + 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..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 @@ -45,7 +48,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 @@ -1635,6 +1640,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 @@ -1721,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") @@ -1929,6 +2080,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 ): @@ -4732,6 +4999,461 @@ 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 + + +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, 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 + + 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" + ) + + +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", @@ -4772,6 +5494,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 ): diff --git a/tests/unittests/telemetry/test_spans.py b/tests/unittests/telemetry/test_spans.py index 57a842f2885..99225a258c5 100644 --- a/tests/unittests/telemetry/test_spans.py +++ b/tests/unittests/telemetry/test_spans.py @@ -18,12 +18,21 @@ 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 +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 @@ -238,6 +247,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') @@ -1073,6 +1170,218 @@ 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 + + +@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 ):