Skip to content

fix(artifacts): keep in-memory scopes separate when identifiers contain / - #7707

Closed
xxwtiancai wants to merge 2 commits into
google:mainfrom
xxwtiancai:fix/inmemory-artifact-scope-7706
Closed

xxwtiancai wants to merge 2 commits into
google:mainfrom
xxwtiancai:fix/inmemory-artifact-scope-7706

Conversation

@xxwtiancai

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

Fixes #7706.

InMemoryArtifactService joined raw app_name/user_id/session_id/filename values into one flat storage key. The shared validator (artifact_util.validate_path_segment) deliberately allows embedded / in identifiers, so distinct (app_name, user_id) pairs — e.g. ("team/project", "alice") vs ("team", "project/alice") — addressed the same stored entry: the second scope could read the first's artifacts, allocate colliding version numbers, delete the first's artifacts, and see them in list_artifact_keys.

Key the store by a namespace tag plus percent-encoded segments instead:

  • u/{enc(app)}/{enc(user)}/{enc(filename)} for user-namespace artifacts, s/{enc(app)}/{enc(user)}/{enc(session)}/{enc(filename)} for session artifacts, where enc percent-encodes with safe="" so an embedded / (%2F) can never shift a segment boundary.
  • The leading u/s tag structurally separates the two namespaces (the existing reserved-segment guard on session ids stays as-is; the tag is defense-in-depth, not a replacement).
  • list_artifact_keys matches these exact encoded prefixes and decodes filenames back, so its output format is unchanged; with no / in any encoded segment, a trailing-slash prefix now matches exactly one scope.
  • The public artifacts field keeps its dict[str, list[_ArtifactEntry]] type; only the internal key encoding changes (existing tests already compute keys through _artifact_path, so nothing observable outside the service changes). The shared validator, canonical URIs, and every other backend are untouched — rejecting ambiguous identifiers would have been a behavior change for all backends and for users legitimately using / in app names.

Testing Plan

Unit tests and checks:

  • Two new cross-scope regression cases in tests/unittests/artifacts/test_artifact_service.py, parametrized over both collision directions (("team/project", "alice") ↔ ("team", "project/alice")): the second scope sees an empty history (load/list/list_keys), its first save allocates its own version 0, the first scope's value and version history are untouched, and deleting through the second scope leaves the first intact.
  • Revert control on unmodified main: both cases fail with the original flat key (the second scope reads the first's value); all pass with the fix.
  • Full artifacts suite (all backends, including the shared conformance matrix): 1124 passed with the fix.
  • Pre-commit on both changed files and mypy on the source file: clean.
  • During analysis I also probed a potential second collision (session id user vs the user namespace): unreachable — artifact_util._validate_session_id_for_flat_storage already rejects such session ids at save time, so no test claims it; the namespace tag makes that invariant structural as well.

Manual end-to-end tests

Not needed beyond the suite: the service is entirely local, and the new cases exercise the public API exactly as the issue's repro does.

…in /

The in-memory service joined raw app, user, session and filename values
into one flat storage key, and the shared validator allows embedded '/', so
distinct (app_name, user_id) pairs addressed the same stored artifact for
reads, version allocation and deletion. Key each scope by a namespace tag
plus percent-encoded segments so embedded slashes cannot shift a segment
boundary, and list keys by exact encoded prefix.

Fixes google#7706
…vior

Regression guards requested in google#7706 review: a filename containing '/'
round-trips exactly through save and list_artifact_keys in both
namespaces, and a hierarchical app_name lists only its own scope's
files (the latter fails on the unfixed resolver, where the aliased
scope's files leak into the listing).
copybara-service Bot pushed a commit that referenced this pull request Oct 26, 2026
Merge #7707

InMemoryArtifactService joined app_name, user_id and session_id raw into
its storage keys, so distinct scopes such as ("team/project", "alice") and
("team", "project/alice") read, versioned and deleted each other's
artifacts. Those identifiers now have `%` and `/` escaped inside the key;
keys for identifiers containing neither character are unchanged.

Fixes #7706

PiperOrigin-RevId: 1007619819
@adk-bot

adk-bot commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Thank you @xxwtiancai for your contribution! 🎉

Your changes have been successfully imported and merged via Copybara in commit 27cdf7f.

Closing this PR as the changes are now in the main branch.

@adk-bot adk-bot added the merged [Status] This PR is merged label Oct 26, 2026
@adk-bot adk-bot closed this Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged [Status] This PR is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InMemoryArtifactService aliases distinct app/user scopes when identifiers contain slashes

3 participants