Skip to content

Fix: reject whitespace-padded artifact filenames in all backends - #224

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/artifact-filename-whitespace-padding
Open

Fix: reject whitespace-padded artifact filenames in all backends#224
AmaadMartin wants to merge 3 commits into
mainfrom
fix/artifact-filename-whitespace-padding

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 11, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:
    Problem: FileArtifactService trimmed a filename before it mapped the name onto a directory, so ' a.txt' and 'a.txt' addressed the same artifact. Saving the padded name appended a version to the unpadded artifact, loading it returned the other artifact's content, and deleting it destroyed the other artifact. InMemoryArtifactService and GcsArtifactService do not trim, so the three backends disagreed about what a filename means.

Solution: All three backends now reject a whitespace-padded filename on save with one InputValidationError, and the file backend reports such a name as absent on every read path. Rejection is the only rule all backends can honour: Windows path normalization strips trailing spaces and periods from a path component (reference), so 'a.txt ' cannot be stored apart from 'a.txt' there. The check runs before the path join, not merely in place of the trim, because ' ../../secret.txt' otherwise resolves back inside the scope root and silently aliases 'secret.txt'.

This narrows the accepted input contract. A caller that saved ' a.txt' and relied on it landing on 'a.txt' now gets an error; that reliance was the bug. Existing data needs no migration, because a padded name was already written to the trimmed directory and stays readable under its unpadded form.

Collision check: I scanned all 223 pull requests on this fork (gh pr list --state all --limit 400) and diffed every open branch against main. No other pull request touches src/google/adk/artifacts/, so this does not duplicate or overlap with one.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

pytest tests/unittests/artifacts tests/unittests/tools/test_forwarding_artifact_service.py tests/unittests/cli/utils/test_local_storage.py -q -> 636 passed. The 544 pre-existing artifact tests are unchanged and still pass, including test_file_save_artifact_rejects_out_of_scope_paths and the INVALID_PATH_SEGMENT_CASES families.

I also ran the consumers of the narrowed contract: pytest tests/unittests/plugins/test_save_files_as_artifacts.py tests/unittests/flows/llm_flows/test_code_execution.py tests/unittests/flows/llm_flows/test_audio_cache_manager.py tests/unittests/agents/test_context.py -q -> 87 passed.

Coverage: every executable line and branch this branch adds to src/ is covered (--cov=google.adk.artifacts --cov-branch). artifact_util.py reports 100% line and branch coverage.

Mutation checks. I ran the 65 new tests against broken code four times to prove they can fail:

  1. Whole source change reverted -> 56 of 65 fail. The 9 survivors are the over-rejection guards ("", ".", "my report.txt"), which must pass both before and after.
  2. Pre-join guard deleted from _resolve_scoped_artifact_path, leaving only the removed .strip() -> 12 fail, including test_file_padded_traversal_filename_does_not_alias_scope_root with Failed: DID NOT RAISE InputValidationError. Under this mutation the reproduction script below prints load 'secret.txt' -> overwritten and versions 'secret.txt' -> [0, 1]: the alias is real, and the containment check does not catch it.
  3. Padding guard deleted from _read_artifact_dir -> 5 fail, so the read paths are pinned separately from the save path.
  4. is_whitespace_padded_filename changed to ignore the user: prefix -> 8 fail, so the after-the-prefix case is pinned.

CI coverage gap, stated plainly. .github/workflows/continuous-integration.yml passes --ignore=tests/unittests/artifacts/test_artifact_service.py to the unit-test job. That exclusion predates this change and I did not touch it, but it means CI does not run the cross-backend conformance tests added here; only test_artifact_util.py runs. I ran the excluded file locally on the pushed commit, as recorded above.

Static checks: ruff check src/google/adk/artifacts/, isort --check-only, pyink --check and scripts/compliance_checks.py all pass. mypy src/google/adk/artifacts/ reports the same single pre-existing error as main (google.cloud has no attribute "storage"), and no new one.

Manual End-to-End (E2E) Tests:
Run this against a real temporary directory and a real in-memory service, with no mocks:

import asyncio, tempfile
from google.adk.artifacts.file_artifact_service import FileArtifactService
from google.adk.errors.input_validation_error import InputValidationError
from google.genai import types

async def main():
  with tempfile.TemporaryDirectory() as root:
    svc = FileArtifactService(root_dir=root)
    kw = dict(app_name="app", user_id="u", session_id="s")
    print(await svc.save_artifact(**kw, filename="a.txt",
                                  artifact=types.Part(text="first")))
    try:
      await svc.save_artifact(**kw, filename=" a.txt",
                              artifact=types.Part(text="second"))
    except InputValidationError as err:
      print(err)
    print((await svc.load_artifact(**kw, filename="a.txt")).text)
    print(await svc.load_artifact(**kw, filename=" a.txt"))
    print(await svc.list_versions(**kw, filename="a.txt"))

asyncio.run(main())

Output, identical for FileArtifactService and InMemoryArtifactService:

0
Artifact filename ' a.txt' must not have leading or trailing whitespace.
first
None
[0]

The traversal trap, on FileArtifactService: after saving 'secret.txt', saving ' ../../secret.txt' raises the whitespace error, load_artifact(' ../../secret.txt') returns None, and 'secret.txt' still holds "top secret" at version 0.

GcsArtifactService needs a real bucket and credentials, so I could not exercise it without mocks. Its rejection is the first statement of _save_artifact, before any bucket call, and it is covered by the mocked-bucket conformance tests.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

CI result on this commit

All 14 test jobs pass on the reviewed commit: Unit Tests on Python 3.10-3.14, Mypy Check on 3.10-3.13, and A2A v0.3 Tests on 3.10-3.14.

Two notes, neither caused by this change.

Unit Tests (Python 3.13) failed once on test_unsafe_local_code_executor.py::test_kill_execution_kills_what_the_code_spawned (assert not True where True = _is_alive(3293)), then passed on re-run. That test polls process liveness after a teardown and is timing-sensitive; PR #181 on this fork tracks it. My diff touches no file under code_executors/, and the same job passed on the previous commit, which differs only in docstring text.

The Pre-commit Linter fails on one hook, update-constraints. The hook regenerates constraints-3.1x.txt, and the only change it produces is the snapshot date in the header comment (--exclude-newer 2026-07-24 -> 2026-08-07), so it drifts with the calendar. PR #223, which touches no constraints file either, fails the same hook the same way. My diff touches no pyproject.toml and no constraints-*.txt, and committing regenerated constraints here would be exactly the unrelated churn the contribution guide asks us to keep out of a fix. Every other pre-commit hook passes, both in CI and locally over main..HEAD.

Amaad Martin added 3 commits August 11, 2026 11:00
A filename is a storage key, but FileArtifactService trimmed it before
mapping it onto a directory name, so ' a.txt' and 'a.txt' addressed the
same artifact while the in-memory and GCS backends kept them apart.
Saving the padded name appended a version to the unpadded artifact, and
loading it returned the other artifact's content.

Every backend now rejects a padded filename on save with one
InputValidationError, and the file backend reports it as absent on reads
instead of resolving onto the unpadded artifact. The rejection runs
before the path join, because ' ../../secret.txt' otherwise resolves
back inside the scope root and aliases 'secret.txt'.
…kends

Replays the padded-filename cases against all three backends so they
cannot drift apart again, and adds a regression test for the
' ../../secret.txt' alias that removing the strip would otherwise open.
The 'cannot be stored apart from its unpadded form' explanation appeared
in five places. The constraint stays at each public entry point, but the
reason now lives only beside the Windows-trimming link in artifact_util.
_read_artifact_dir loses an Args block that restated its own parameters.
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.

1 participant