fix(artifacts): Port app-scoped file artifact storage and layout-derived payload paths to v1 - #6795
Open
GWeale wants to merge 2 commits into
Open
fix(artifacts): Port app-scoped file artifact storage and layout-derived payload paths to v1#6795GWeale wants to merge 2 commits into
GWeale wants to merge 2 commits into
Conversation
FileArtifactService stored every artifact under `root/users/{user_id}`,
dropping app_name from the path entirely. Two apps served from one root
therefore shared a single artifact namespace: saving `report.txt` from one
app overwrote the other app's `report.txt`, and load, list, delete and both
version APIs returned the other app's data. The shipped CLI reaches this
without any attacker capability, because `adk web` and `adk api_server`
point one FileArtifactService at `<agents_dir>/.adk/artifacts` for every
agent in that directory. The in-memory and GCS services already key on
app_name, so the file service was the odd one out.
Artifacts now live under `root/apps/{app_name}/users/{user_id}/...`, and
app_name is validated as a path segment the way user_id and session_id
already were. That validation was meaningless before this change and is
meaningful now, because app_name only reaches the path from here on. All
seven public method signatures already accepted app_name, so no caller
changes.
Breaking change for existing 1.x users. Artifacts written by earlier 1.x
releases live under `root/users/{user_id}/...` and are no longer reachable
through the API. They are not deleted and stay on disk. To keep them
reachable, move `root/users/` to `root/apps/<app_name>/users/` for the app
that owns them. That root is `<agents_dir>/.adk/artifacts` for `adk web`
and `adk api_server`, which point one service at the agents directory. It
is `<agents_dir>/<agent_folder>/.adk/artifacts` for `adk run`, which gives
each agent a root of its own.
Upstream, "fix(artifacts): namespace file artifacts by app" landed a
transitional read of the pre-app-scoped tree, and the follow-up "fix: scope
file artifact reads and deletes to the requesting app" removed it again,
because a root shared by several apps cannot attribute that tree to any one
of them. Both are ported here as a single change with the transitional read
never written, which leaves this branch where main already is: main's
FileArtifactService reads `root/users` for no root at all, per-agent roots
included.
…and partial writes (v1) Port of the upstream "Secure and harden FileArtifactService against tampered metadata and partial writes". Four defects, all in the file service: The payload location was taken from `canonicalUri` in the on-disk metadata document whenever the payload file itself was absent. That document lives inside the artifact tree, so anything able to write there, or to win the race between a delete and a load, could redirect a read to any file the process could open. The payload location is now derived only from the storage layout, and the `canonical_uri` returned to callers is recomputed from that layout rather than read back from the document. Saving an artifact named `metadata.json` destroyed it. The payload is stored under the artifact directory's own name, so it was written first and then overwritten by the metadata document, leaving a version directory holding only metadata. Filenames are model-supplied, so this needed no attacker. The name is now rejected at save time, caselessly, because a case-insensitive filesystem resolves `Metadata.json` to the same file. The rejection is on the save path only, so an artifact already stored under that name stays readable and deletable. A save that failed partway left the version directory behind, and a version with a payload but no metadata reads as valid. Serializing `custom_metadata` is caller-driven and can fail, which was enough to produce one. The whole version directory is now removed if any step fails. The metadata document was written in place with `write_text`, so a reader could see a truncated document. It is now written to a temporary file in the same directory and renamed over the destination. `tempfile.mkstemp` hardcodes mode 0600 and `os.replace` carries that mode across, so the mode a normally created file would get from the umask is restored first; otherwise the metadata document and the payload beside it end up readable by different principals. Behaviour changes an existing 1.x user would notice: - Saving an artifact named `metadata.json` in any casing now raises InputValidationError. It previously succeeded and silently destroyed the artifact it had just written. - A save that fails partway now leaves nothing behind, where it previously left a version directory that `list_versions` reported. - A metadata document naming a `canonicalUri` outside the artifact tree is ignored rather than followed, so an artifact whose payload is missing now loads as None. `_umask_derived_file_mode()` calls `os.umask` twice at import time, which is a process-global mutation. It is momentary, and sampling per write would race against concurrent writers instead. The upstream commit also threads `inline_data.display_name` through the save and load paths and guards `inline_data.data is None`. Neither is ported: both come from separate upstream changes that are not on this branch.
GWeale
changed the title
xuanyang15
approved these changes
Aug 19, 2026
| """Returns the artifacts root directory for a user.""" | ||
| def _base_root(self, app_name: str, user_id: str) -> Path: | ||
| """Returns the app-scoped root holding a user's artifacts.""" | ||
| _validate_path_segment(app_name, "app_name") |
Collaborator
There was a problem hiding this comment.
app_name starts reaching the filesystem path here, and it is validated by the module-private _validate_path_segment (215), which rejects / and \ anywhere in the value. Upstream validates the same argument with artifact_util.validate_path_segment (main:file_artifact_service.py:367), which rejects only a leading separator — main's own suite pins "group/user123" and "has/slash" as valid (tests/unittests/artifacts/test_artifact_util.py:153-154). So an app_name that main accepts (and stores as a nested directory) raises here.
That is not hypothetical: VertexAiSessionService._get_reasoning_engine_id (sessions/vertex_ai_session_service.py:376-392) accepts a full projects/{p}/locations/{l}/reasoningEngines/{id} as the app name, and that pairing worked on v1 before this change precisely because the file service ignored app_name altogether. After this it fails on every artifact call.
Keeping the stricter private validator is well argued in d68d89cf2, but that argument is about user_id and session_id, which this branch already guarded that way — app_name is new here, and the strictness is inherited rather than chosen. There is a closer-to-upstream option once #6794 has landed artifact_util.validate_path_segment on this branch: move just this one call onto the shared helper and leave user_id/session_id on the private one. Would that be worth doing as the follow-up, so v1 and main agree on which app names are addressable?
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
3 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.
Ports three
FileArtifactServicecommits to thev1branch. The on-disk layout changes — read the migration note first.Migration
Artifacts live under
root/apps/{app_name}/users/{user_id}/.... Move an existingroot/users/tree toroot/apps/<app_name>/users/for the app that owns it to keep it reachable. Roots by entry point:adk web,adk api_server<agents_dir>/.adk/artifactsadk run<agents_dir>/<agent_folder>/.adk/artifacts1. Namespace file artifacts by app
f72f0db5, with follow-upc27d8688.app_name.app_namemust be a path segment: non-empty, no null byte, no/or\, not.or..; otherwiseInputValidationError.2. Derive payload paths from the layout
c5672030.canonical_uriare derived from the storage layout.metadata.json, in any casing, is rejected as an artifact name at save time.None.