Skip to content

fix(artifacts): reject the reserved segment 'user' as a session_id - #7064

Open
MUHAMMEDHAFEEZ wants to merge 1 commit into
google:mainfrom
MUHAMMEDHAFEEZ:fix/artifact-reject-reserved-user-session-id
Open

fix(artifacts): reject the reserved segment 'user' as a session_id#7064
MUHAMMEDHAFEEZ wants to merge 1 commit into
google:mainfrom
MUHAMMEDHAFEEZ:fix/artifact-reject-reserved-user-session-id

Conversation

@MUHAMMEDHAFEEZ

Copy link
Copy Markdown
Contributor

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

Link to Issue or Description of Change

Problem:

InMemoryArtifactService and GcsArtifactService lay session-scoped and
user-scoped artifacts out in the same flat namespace, using the literal
segment "user" to mark user-scoped artifacts:
_artifact_path/_get_blob_prefix return
f"{app_name}/{user_id}/user/{filename}" for user-namespaced files, and
f"{app_name}/{user_id}/{session_id}/{filename}" otherwise. The reserved
literal "user" is never validated against, so a session actually named
"user" writes its artifacts into the same prefix reserved for
user-scoped ones.

This isn't just a naming collision: list_artifact_keys() for an
unrelated session incorrectly returns filenames that belong to the
"user"-named session, and loading one of those filenames then returns
None — the listing and the load disagree.

Solution:

Adds artifact_util.validate_session_id_segment(), which runs the existing
validate_path_segment() checks and additionally rejects the literal value
"user". Applied at the two places each service already ran
validate_path_segment on session_id:
_artifact_path/_get_blob_prefix and each service's list_artifact_keys.

FileArtifactService is not touched: it lays session-scoped artifacts out
under their own sessions/<id>/ subtree, distinct from the user-scoped
artifacts/ subtree, so a session named "user" cannot collide with it
there (confirmed with a positive test below).

Version Bump?

  • Yes
  • No

No pyproject.toml version field exists for this package.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Testing Plan

Unit Tests:

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

Added to tests/unittests/artifacts/test_artifact_service.py:

  • test_save_artifact_rejects_reserved_user_as_session_id (IN_MEMORY,
    GCS) — session_id="user" raises InputValidationError.
  • test_file_allows_reserved_user_as_session_idFILE is unaffected and
    a session named "user" saves/loads normally there.
  • test_list_artifact_keys_rejects_reserved_user_as_session_id (IN_MEMORY,
    GCS) — the same rejection applies to listing, not just save/load.

Added to tests/unittests/artifacts/test_artifact_util.py:

  • test_validate_session_id_segment_rejects_reserved_user
  • test_validate_session_id_segment_allows_ordinary_values
  • test_validate_session_id_segment_still_runs_path_segment_checks

Confirmed all 5 backend-affecting new tests fail against the unfixed code
(temporarily reverted just the three source files, keeping the new tests)
and pass with the fix restored.

$ pytest tests/unittests/artifacts tests/unittests/sessions
1184 passed, 3 xfailed, 10 warnings

Also ran pyink --check, ruff check, isort --check, codespell, and
mypy on all five changed files. Two pre-existing, unrelated findings
confirmed present on unmodified main (not introduced by this change, left
alone): an unused SimpleNamespace import in test_artifact_service.py,
and a mypy attr-defined error on google.cloud.storage in
gcs_artifact_service.py (missing type stub in this environment).

Manual End-to-End (E2E) Tests:

Ran the reproduction from the issue directly against
InMemoryArtifactService:

Before the fix:

list_artifact_keys(session_id="s2"): ['from_session_user.txt', 'user:shared.txt']
load_artifact(session_id="s2", filename="from_session_user.txt"): None

After the fix: save_artifact(session_id="user", ...) raises
InputValidationError up front, so this state can no longer arise.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • 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.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

Surfaced during review of #7030 (whitespace normalization, fixed by
#6958) — a related but distinct root cause (a reserved literal value, not
padding), independent of that fix and not blocked by it.

InMemoryArtifactService and GcsArtifactService lay session-scoped and
user-scoped artifacts out in the same flat namespace, using the literal
path/blob-prefix segment "user" to mark user-scoped artifacts. A session
actually named "user" collided with that reserved segment: its artifacts
were stored under the same prefix as genuinely user-scoped ones, so they
leaked into an unrelated session's list_artifact_keys() result while still
being unloadable there (listing and load disagreed).

Adds artifact_util.validate_session_id_segment(), which runs the existing
validate_path_segment() checks and additionally rejects the literal value
"user". Applied at the two places that already ran validate_path_segment
on session_id: _artifact_path/_get_blob_prefix and the list_artifact_keys
entry points of both services.

FileArtifactService is not affected and is not touched: it lays
session-scoped artifacts out under their own sessions/<id>/ subtree,
distinct from the user-scoped artifacts/ subtree, so a session named
"user" cannot collide with it.
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.

InMemoryArtifactService/GcsArtifactService collide with the reserved "user" segment when session_id is literally "user"

2 participants