fix(artifacts): reject the reserved segment 'user' as a session_id - #7064
Open
MUHAMMEDHAFEEZ wants to merge 1 commit into
Open
fix(artifacts): reject the reserved segment 'user' as a session_id#7064MUHAMMEDHAFEEZ wants to merge 1 commit into
MUHAMMEDHAFEEZ wants to merge 1 commit into
Conversation
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.
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
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Problem:
InMemoryArtifactServiceandGcsArtifactServicelay session-scoped anduser-scoped artifacts out in the same flat namespace, using the literal
segment
"user"to mark user-scoped artifacts:_artifact_path/_get_blob_prefixreturnf"{app_name}/{user_id}/user/{filename}"for user-namespaced files, andf"{app_name}/{user_id}/{session_id}/{filename}"otherwise. The reservedliteral
"user"is never validated against, so a session actually named"user"writes its artifacts into the same prefix reserved foruser-scoped ones.
This isn't just a naming collision:
list_artifact_keys()for anunrelated session incorrectly returns filenames that belong to the
"user"-named session, and loading one of those filenames then returnsNone— the listing and the load disagree.Solution:
Adds
artifact_util.validate_session_id_segment(), which runs the existingvalidate_path_segment()checks and additionally rejects the literal value"user". Applied at the two places each service already ranvalidate_path_segmentonsession_id:_artifact_path/_get_blob_prefixand each service'slist_artifact_keys.FileArtifactServiceis not touched: it lays session-scoped artifacts outunder their own
sessions/<id>/subtree, distinct from the user-scopedartifacts/subtree, so a session named"user"cannot collide with itthere (confirmed with a positive test below).
Version Bump?
No
pyproject.tomlversion field exists for this package.Type of Change
Testing Plan
Unit Tests:
Added to
tests/unittests/artifacts/test_artifact_service.py:test_save_artifact_rejects_reserved_user_as_session_id(IN_MEMORY,GCS) —session_id="user"raisesInputValidationError.test_file_allows_reserved_user_as_session_id—FILEis unaffected anda 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_usertest_validate_session_id_segment_allows_ordinary_valuestest_validate_session_id_segment_still_runs_path_segment_checksConfirmed 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.
Also ran
pyink --check,ruff check,isort --check,codespell, andmypyon all five changed files. Two pre-existing, unrelated findingsconfirmed present on unmodified
main(not introduced by this change, leftalone): an unused
SimpleNamespaceimport intest_artifact_service.py,and a
mypyattr-definederror ongoogle.cloud.storageingcs_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:
After the fix:
save_artifact(session_id="user", ...)raisesInputValidationErrorup front, so this state can no longer arise.Checklist
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.