fix: Port migration unpickle allowlist and database URL redaction to v1 - #6791
Merged
Conversation
Port of the upstream fix (PR #5866) to the v1 branch. The v0 session schema stored event actions as a pickled blob. Migration read that column straight out of the source database and handed the raw bytes to pickle.loads, so migrating a database whose contents the operator did not control could run code from that database inside the migrating process. Migration now loads those blobs through a pickle.Unpickler subclass whose find_class accepts only an explicit allowlist: builtin containers and primitives, datetime types, the ADK action models (EventActions, EventCompaction, AuthConfig, ToolConfirmation, UiWidget and the auth credential and scheme models) and the google.genai.types classes that compaction payloads need. Anything else raises pickle.UnpicklingError, which the existing handler turns into a warning and an empty EventActions() for that one event, leaving the rest of the migration to proceed. An operator who trusts the source database and needs the old behaviour for custom objects can opt back in: migrate() and migration_runner.upgrade() take allow_unsafe_unpickling, and both `adk migrate session` and the migration script take --allow_unsafe_unpickling / --allow-unsafe-unpickling. The same commit tightens the two JSON helpers in the migration path so that they accept only JSON objects. _safe_json_load returns None and _get_state_dict returns {} when the decoded value is something else. Behaviour changes for existing users: - A v0 database containing a pickled custom Python object in state_delta or another Any-typed field now migrates that event with empty actions and a logged warning, instead of reconstructing the object. Passing --allow_unsafe_unpickling restores the old behaviour. - An event whose content, metadata or transcription column holds valid JSON that is not an object used to fail model validation, which dropped the whole event with a warning. That event now migrates with the field left unset. - A state column holding valid JSON that is not an object used to be stored as-is; it is now stored as an empty dict with a warning.
Port of the upstream fix (PR #6485) to the v1 branch. A database URL carries its password in the userinfo component, and the session code interpolated that URL as-is into places that routinely end up in application logs and tracebacks. DatabaseSessionService put it in all three of its engine-creation ValueError messages, the schema-version check logged it in a warning, and the migration entry points logged it on every run, not just on failure: both connect lines in the pickle migration, the connect line in the sqlite migration, and the already-up-to-date and per-step lines in the migration runner. _schema_check_utils now has a _redact_db_url helper, and those call sites go through it. It parses the URL with SQLAlchemy's make_url, replaces every query-parameter value with REDACTED, and renders the result with hide_password=True. Query values are masked wholesale because drivers accept secrets as query parameters under names ADK cannot enumerate. The helper runs while an error is already being reported, so it catches everything and returns the fixed string "<unparseable database URL>" rather than raising or echoing a URL it could not parse. Behaviour change for existing users: these error messages and log lines no longer contain the full connection string. The password shows as ***, each query-parameter value shows as REDACTED, and a URL that make_url cannot parse is replaced by the placeholder. Anyone grepping logs for a connection string, or parsing the ValueError text, will see different output.
GWeale
changed the title
DeanChensj
approved these changes
Aug 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR ports two session-migration commits to the
v1branch:Migration unpickle allowlist (
9db48ce9, fix(migration): restrict unpickling of v0 actions blobs #5866)(module, name)pairs; a blob naming anything else migrates with empty actions and a logged warning.--allow_unsafe_unpicklingonadk migrate sessionand the migration script uses plainpickle.loadsinstead.content, metadata or transcription column holding non-object JSON migrates with that field unset. Astatecolumn holding non-object JSON migrates as{}with a warning.Database URL redaction (
4ad3eccc, Redact password from DatabaseSessionService engine-creation errors #6485)DatabaseSessionServiceand the migration entry points report a redacted URL:***for the password,REDACTEDfor every query-parameter value, and<unparseable database URL>for a URL that cannot be parsed.