Skip to content

fix(artifacts): reject interior path separators in validate_path_segment - #6975

Open
chelsealong wants to merge 1 commit into
google:mainfrom
chelsealong:fix/artifact-path-segment-separator
Open

fix(artifacts): reject interior path separators in validate_path_segment#6975
chelsealong wants to merge 1 commit into
google:mainfrom
chelsealong:fix/artifact-path-segment-separator

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

Problem:

artifact_util.validate_path_segment only rejects a value that starts
with / or \, not one that contains a separator anywhere. FileArtifactService
builds its user-scoped and session-scoped directories so that they differ by
an infix (.../users/<user_id>/artifacts/... vs.
.../users/<user_id>/sessions/<session_id>/artifacts/...), so a user_id
containing /sessions/<id> now resolves to the same directory as another
user's session scope. Concretely: a user-scoped save with
user_id="victim/sessions/s1" lands in exactly the directory that
user_id="victim", session_id="s1" uses for its session-scoped artifacts,
so one caller's artifact overwrites/reads another's.

This is a regression from 45a77dc5 ("fix: Validate path segments in
GcsArtifactService and InMemoryArtifactService to prevent cross-user
artifact access"), which consolidated per-service path validation into a
single artifact_util.validate_path_segment helper but weakened the check
from "must not contain a separator" to "must not start with a separator" in
the process.

Solution:

Restore the full separator check ("/" in value or "\\" in value), matching
the check FileArtifactService had before the consolidation. This subsumes
the old "must not start with a slash" branch, since any leading separator is
also an interior one.

One existing test, test_save_and_load_namespaced_user_id_succeeds, asserted
that a user_id like "group/user123" round-trips successfully across all
three backends. That test was added in the same commit that introduced the
regression, and is itself a symptom of the same weakened check rather than an
independently designed feature — permitting arbitrary separators in user_id
is exactly what makes the FileArtifactService collision possible, so it
can't be preserved without leaving the vulnerability open. I converted it into
a negative test (test_save_artifact_rejects_slash_in_user_id) asserting the
separator is now rejected, and added a new test,
test_file_rejects_user_id_that_collides_with_session_scope, that reproduces
the exact scope-collision scenario from the issue end-to-end against
FileArtifactService.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Added/updated:

  • tests/unittests/artifacts/test_artifact_util.py: moved "group/user123",
    "has/slash", "back\\slash" from the valid-segment cases to the
    invalid-segment cases, and added "victim/sessions/s1" as an invalid case.
  • tests/unittests/artifacts/test_artifact_service.py: added
    test_file_rejects_user_id_that_collides_with_session_scope, which
    reproduces the issue's exact repro (save a session-scoped artifact for
    user_id="victim", then confirm a user-scoped save with
    user_id="victim/sessions/s1" is rejected instead of overwriting it, then
    confirms the original artifact still reads back intact); converted
    test_save_and_load_namespaced_user_id_succeeds into
    test_save_artifact_rejects_slash_in_user_id; updated the expected error
    messages in INVALID_PATH_SEGMENT_CASES to match the restored check.

I confirmed the new/updated tests fail without the fix by reverting just
src/google/adk/artifacts/artifact_util.py to the main version
(git checkout HEAD~1 -- src/google/adk/artifacts/artifact_util.py) and
re-running:

$ python3 -m pytest tests/unittests/artifacts/test_artifact_service.py -k "test_file_rejects_user_id_that_collides_with_session_scope or test_save_artifact_rejects_slash_in_user_id" -q
...
FAILED tests/unittests/artifacts/test_artifact_service.py::test_file_rejects_user_id_that_collides_with_session_scope - Failed: DID NOT RAISE InputValidationError
FAILED tests/unittests/artifacts/test_artifact_service.py::test_save_artifact_rejects_slash_in_user_id[ArtifactServiceType.IN_MEMORY] - Failed: DID NOT RAISE InputValidationError
FAILED tests/unittests/artifacts/test_artifact_service.py::test_save_artifact_rejects_slash_in_user_id[ArtifactServiceType.GCS] - Failed: DID NOT RAISE InputValidationError
FAILED tests/unittests/artifacts/test_artifact_service.py::test_save_artifact_rejects_slash_in_user_id[ArtifactServiceType.FILE] - Failed: DID NOT RAISE InputValidationError
4 failed, 605 deselected in 1.18s

With the fix restored, the full artifact suite passes:

$ python3 -m pytest tests/unittests/artifacts -q
715 passed in 2.84s

Also ran pyink --check, ruff check, isort --check, and codespell on
the three changed files — clean, aside from one pre-existing, unrelated
ruff finding (SimpleNamespace imported but unused in
test_artifact_service.py) that is already present on main and untouched
by this change.

Manual End-to-End (E2E) Tests:

Not applicable — this is a pure library fix to a synchronous validation
helper with no I/O or model/network involvement; the added unit test exercises
the real FileArtifactService against a temp directory end-to-end (save,
rejected collision attempt, load), which is the scenario from the issue.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end. (not applicable, see above)
  • Any dependent changes have been merged and published in downstream modules.

Additional context

GcsArtifactService and InMemoryArtifactService share the same validator
and are also covered by the new/updated tests, even though the issue's
analysis is that their {app}/{user}/user/{file} layout doesn't have this
particular collision — the shared helper doesn't distinguish between
backends, so the fix (and the regression test) applies uniformly across all
three.

This change was developed with AI assistance (Claude Code). The bug was
verified against the current main branch, the fix and tests were written
and reviewed by me, and all test output quoted above was produced by running
the suite in this checkout.

artifact_util.validate_path_segment only rejected a leading '/' or '\',
not one appearing anywhere in the value. FileArtifactService builds its
user-scoped and session-scoped directories so that they differ by an
infix ("sessions/<id>"), so a user_id containing that infix collapses
onto another caller's session-scoped directory, letting artifacts saved
under one scope be read back under the other.

Restores the full separator check that existed before the validator was
consolidated into artifact_util (and inadvertently weakened) in 45a77dc.

Fixes google#6973
@chelsealong
chelsealong force-pushed the fix/artifact-path-segment-separator branch from 830d69d to ba7ec04 Compare September 3, 2026 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate_path_segment no longer rejects path separators, collapsing FileArtifactService user and session scopes

2 participants