From e4c14acb8db50e2099e9c6d8f8db99d67cf2aab9 Mon Sep 17 00:00:00 2001 From: Nant361 Date: Thu, 7 May 2026 09:11:15 +0700 Subject: [PATCH] fix file artifact app namespace --- .../adk/artifacts/file_artifact_service.py | 65 +++++++--- .../artifacts/test_artifact_service.py | 111 ++++++++++++++++++ 2 files changed, 160 insertions(+), 16 deletions(-) diff --git a/src/google/adk/artifacts/file_artifact_service.py b/src/google/adk/artifacts/file_artifact_service.py index 9c3870b6e3..ab5cfe53c9 100644 --- a/src/google/adk/artifacts/file_artifact_service.py +++ b/src/google/adk/artifacts/file_artifact_service.py @@ -217,18 +217,20 @@ class FileArtifactService(BaseArtifactService): # Storage layout matches the cloud and in-memory services: # root/ - # └── users/ - # └── {user_id}/ - # ├── sessions/ - # │ └── {session_id}/ - # │ └── artifacts/ - # │ └── {artifact_path}/ # derived from filename - # │ └── versions/ - # │ └── {version}/ - # │ ├── {original_filename} - # │ └── metadata.json - # └── artifacts/ - # └── {artifact_path}/... + # └── apps/ + # └── {app_name}/ + # └── users/ + # └── {user_id}/ + # ├── sessions/ + # │ └── {session_id}/ + # │ └── artifacts/ + # │ └── {artifact_path}/ # derived from filename + # │ └── versions/ + # │ └── {version}/ + # │ ├── {original_filename} + # │ └── metadata.json + # └── artifacts/ + # └── {artifact_path}/... # # Artifact paths are derived from the provided filenames: separators create # nested directories, and path traversal is rejected to keep the layout @@ -244,19 +246,21 @@ def __init__(self, root_dir: Path | str): self.root_dir = Path(root_dir).expanduser().resolve() self.root_dir.mkdir(parents=True, exist_ok=True) - def _base_root(self, user_id: str, /) -> Path: + def _base_root(self, app_name: str, user_id: str, /) -> Path: """Returns the artifacts root directory for a user.""" + _validate_path_segment(app_name, "app_name") _validate_path_segment(user_id, "user_id") - return self.root_dir / "users" / user_id + return self.root_dir / "apps" / app_name / "users" / user_id def _scope_root( self, + app_name: str, user_id: str, session_id: Optional[str], filename: str, ) -> Path: """Returns the directory that represents the artifact scope.""" - base = self._base_root(user_id) + base = self._base_root(app_name, user_id) if _is_user_scoped(session_id, filename): return _user_artifacts_dir(base) if not session_id: @@ -267,12 +271,14 @@ def _scope_root( def _artifact_dir( self, + app_name: str, user_id: str, session_id: Optional[str], filename: str, ) -> Path: """Builds the directory path for an artifact.""" scope_root = self._scope_root( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -283,6 +289,7 @@ def _artifact_dir( def _build_artifact_version( self, *, + app_name: str, user_id: str, session_id: Optional[str], filename: str, @@ -294,6 +301,7 @@ def _build_artifact_version( metadata.canonical_uri if metadata and metadata.canonical_uri else self._canonical_uri( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -312,6 +320,7 @@ def _build_artifact_version( def _canonical_uri( self, *, + app_name: str, user_id: str, session_id: Optional[str], filename: str, @@ -319,6 +328,7 @@ def _canonical_uri( ) -> str: """Builds the canonical file:// URI for an artifact payload.""" artifact_dir = self._artifact_dir( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -357,6 +367,7 @@ async def save_artifact( """ return await asyncio.to_thread( self._save_artifact_sync, + app_name, user_id, filename, artifact, @@ -366,6 +377,7 @@ async def save_artifact( def _save_artifact_sync( self, + app_name: str, user_id: str, filename: str, artifact: Union[types.Part, dict[str, Any]], @@ -375,6 +387,7 @@ def _save_artifact_sync( """Saves an artifact to disk and returns its version.""" artifact = ensure_part(artifact) artifact_dir = self._artifact_dir( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -407,6 +420,7 @@ def _save_artifact_sync( ) canonical_uri = self._canonical_uri( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -441,6 +455,7 @@ async def load_artifact( ) -> Optional[types.Part]: return await asyncio.to_thread( self._load_artifact_sync, + app_name, user_id, filename, session_id, @@ -449,6 +464,7 @@ async def load_artifact( def _load_artifact_sync( self, + app_name: str, user_id: str, filename: str, session_id: Optional[str], @@ -456,6 +472,7 @@ def _load_artifact_sync( ) -> Optional[types.Part]: """Loads an artifact from disk.""" artifact_dir = self._artifact_dir( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -510,19 +527,21 @@ async def list_artifact_keys( ) -> list[str]: return await asyncio.to_thread( self._list_artifact_keys_sync, + app_name, user_id, session_id, ) def _list_artifact_keys_sync( self, + app_name: str, user_id: str, session_id: Optional[str], ) -> list[str]: """Lists artifact filenames for the given session/user.""" filenames: set[str] = set() - base_root = self._base_root(user_id) + base_root = self._base_root(app_name, user_id) if session_id: session_root = _session_artifacts_dir(base_root, session_id) @@ -565,6 +584,7 @@ async def delete_artifact( """ await asyncio.to_thread( self._delete_artifact_sync, + app_name, user_id, filename, session_id, @@ -572,11 +592,13 @@ async def delete_artifact( def _delete_artifact_sync( self, + app_name: str, user_id: str, filename: str, session_id: Optional[str], ) -> None: artifact_dir = self._artifact_dir( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -597,6 +619,7 @@ async def list_versions( """Lists all versions stored for an artifact.""" return await asyncio.to_thread( self._list_versions_sync, + app_name, user_id, filename, session_id, @@ -604,11 +627,13 @@ async def list_versions( def _list_versions_sync( self, + app_name: str, user_id: str, filename: str, session_id: Optional[str], ) -> list[int]: artifact_dir = self._artifact_dir( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -627,6 +652,7 @@ async def list_artifact_versions( """Lists metadata for each artifact version on disk.""" return await asyncio.to_thread( self._list_artifact_versions_sync, + app_name, user_id, filename, session_id, @@ -634,11 +660,13 @@ async def list_artifact_versions( def _list_artifact_versions_sync( self, + app_name: str, user_id: str, filename: str, session_id: Optional[str], ) -> list[ArtifactVersion]: artifact_dir = self._artifact_dir( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -650,6 +678,7 @@ def _list_artifact_versions_sync( metadata = _read_metadata(metadata_path) artifact_versions.append( self._build_artifact_version( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -672,6 +701,7 @@ async def get_artifact_version( """Gets metadata for a specific artifact version.""" return await asyncio.to_thread( self._get_artifact_version_sync, + app_name, user_id, filename, session_id, @@ -680,12 +710,14 @@ async def get_artifact_version( def _get_artifact_version_sync( self, + app_name: str, user_id: str, filename: str, session_id: Optional[str], version: Optional[int], ) -> Optional[ArtifactVersion]: artifact_dir = self._artifact_dir( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, @@ -703,6 +735,7 @@ def _get_artifact_version_sync( metadata_path = _metadata_path(artifact_dir, version_to_read) metadata = _read_metadata(metadata_path) return self._build_artifact_version( + app_name=app_name, user_id=user_id, session_id=session_id, filename=filename, diff --git a/tests/unittests/artifacts/test_artifact_service.py b/tests/unittests/artifacts/test_artifact_service.py index 8b82397097..26b7cd04ee 100644 --- a/tests/unittests/artifacts/test_artifact_service.py +++ b/tests/unittests/artifacts/test_artifact_service.py @@ -616,6 +616,8 @@ async def test_file_metadata_camelcase(tmp_path, artifact_service_factory): metadata_path = ( tmp_path / "artifacts" + / "apps" + / "myapp" / "users" / "user123" / "sessions" @@ -677,6 +679,8 @@ async def test_file_list_artifact_versions(tmp_path, artifact_service_factory): version_payload_path = ( tmp_path / "artifacts" + / "apps" + / "myapp" / "users" / "user123" / "sessions" @@ -718,6 +722,81 @@ async def test_file_list_artifact_versions(tmp_path, artifact_service_factory): assert latest.custom_metadata == version_meta.custom_metadata +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("filename", "session_id"), + [ + ("docs/report.txt", "sess789"), + ("user:shared.txt", None), + ], +) +async def test_file_artifacts_are_isolated_by_app_name( + tmp_path, filename, session_id +): + """FileArtifactService does not share artifacts across app names.""" + artifact_service = FileArtifactService(root_dir=tmp_path / "artifacts") + + await artifact_service.save_artifact( + app_name="victim_app", + user_id="user123", + session_id=session_id, + filename=filename, + artifact=types.Part(text="victim data"), + ) + + assert ( + await artifact_service.load_artifact( + app_name="attacker_app", + user_id="user123", + session_id=session_id, + filename=filename, + ) + is None + ) + assert ( + await artifact_service.list_artifact_keys( + app_name="attacker_app", + user_id="user123", + session_id=session_id, + ) + == [] + ) + assert ( + await artifact_service.list_versions( + app_name="attacker_app", + user_id="user123", + session_id=session_id, + filename=filename, + ) + == [] + ) + assert ( + await artifact_service.get_artifact_version( + app_name="attacker_app", + user_id="user123", + session_id=session_id, + filename=filename, + ) + is None + ) + + await artifact_service.delete_artifact( + app_name="attacker_app", + user_id="user123", + session_id=session_id, + filename=filename, + ) + + loaded = await artifact_service.load_artifact( + app_name="victim_app", + user_id="user123", + session_id=session_id, + filename=filename, + ) + assert loaded is not None + assert loaded.text == "victim data" + + @pytest.mark.asyncio @pytest.mark.parametrize( ("filename", "session_id"), @@ -744,6 +823,38 @@ async def test_file_save_artifact_rejects_out_of_scope_paths( ) +@pytest.mark.asyncio +@pytest.mark.parametrize( + "app_name", + [ + "../escape", + "../../etc", + "foo/../../bar", + "valid/../..", + "..", + ".", + "has/slash", + "back\\slash", + "null\x00byte", + "", + ], +) +async def test_file_save_artifact_rejects_traversal_in_app_name( + tmp_path, app_name +): + """FileArtifactService rejects app_name values that escape root_dir.""" + artifact_service = FileArtifactService(root_dir=tmp_path / "artifacts") + part = types.Part(text="content") + with pytest.raises(InputValidationError): + await artifact_service.save_artifact( + app_name=app_name, + user_id="user123", + session_id="sess123", + filename="safe.txt", + artifact=part, + ) + + @pytest.mark.asyncio @pytest.mark.parametrize( "user_id",