fix(migration): restrict unpickling of v0 actions blobs#5866
fix(migration): restrict unpickling of v0 actions blobs#5866White-Mouse wants to merge 4 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Response from ADK Triaging Agent Hello @White-Mouse, thank you for submitting this pull request! To help us review and process your contribution more efficiently, please address the following items in accordance with our contribution guidelines:
Thank you for your cooperation! |
d09e22e to
71c4053
Compare
|
Hi @White-Mouse , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please fix the failing mypy-diff tests before we can proceed with the review. |
|
Hi @White-Mouse, thanks for the update! I appreciate the effort to move towards a "Secure by Default" approach using a restricted unpickler. To ensure this doesn't negatively impact existing users, I'd like to suggest the following improvements to the PR:
With these two changes, we can achieve strong security for the majority of users while maintaining full functionality and flexibility. What do you think? |
|
Thanks for the review. I updated the PR to address both points: the restricted allowlist now covers standard nested ADK action types ( I also fixed the |
What
The v0 session schema stored event actions as pickled blobs. The migration helper reads raw bytes via
SELECT * FROM eventsand previously usedpickle.loads(...)directly.This PR replaces the default load path with a restricted unpickler allowlist for builtin containers/primitives, standard ADK
EventActionspayloads, nested ADK core action types (AuthConfig,ToolConfirmation,EventCompaction), and thegoogle.genai.types.Content/Partdependency classes that normal compaction payloads require.It also adds an explicit trust toggle for legacy databases that contain custom Python objects in
state_deltaor otherAnyfields:migrate(..., allow_unsafe_unpickling=True)upgrade(..., allow_unsafe_unpickling=True)adk migrate session --allow_unsafe_unpickling ...--allow_unsafe_unpickling/--allow-unsafe-unpicklingWhy
pickleis not safe for untrusted inputs. Migration tooling often runs against restored/backed-up DB files or shared storage; failing closed by default reduces the blast radius if the source DB contents are compromised.The opt-in flag keeps compatibility for users who trust their source database and need the original unsafe pickle behavior for custom legacy objects.
Associated Issue / Background
No existing GitHub issue is linked. This was found while reviewing the v0-to-v1 migration path for unsafe deserialization risks in legacy session data.
Compatibility / fail-closed boundary
Normal v0
EventActionspayloads made from primitive/container fields continue to migrate. The allowlist now also covers common nested ADK action models requested during review, including requested auth configs, requested tool confirmations, and event compaction content.Payloads that require globals outside the explicit allowlist still fail closed by default: the migration logs a warning and falls back to empty
EventActions()for that event. Users can opt into the previous unsafe pickle behavior only when they trust the source database.Verification
uv run pytest tests/unittests/sessions/migration/test_migration.py22 passed, 4 warningsuv run mypy src/google/adk/sessions/migration/migrate_from_sqlalchemy_pickle.py src/google/adk/sessions/migration/migration_runner.pySuccess: no issues found in 2 source filesuv run pre-commit run --files src/google/adk/sessions/migration/migrate_from_sqlalchemy_pickle.py src/google/adk/sessions/migration/migration_runner.py src/google/adk/cli/cli_tools_click.py tests/unittests/sessions/migration/test_migration.pygit diff --check