Skip to content

Commit 8718aef

Browse files
Ashutosh0xjazhang00
authored andcommitted
fix: Validate path segments in GcsArtifactService and InMemoryArtifactService to prevent cross-user artifact access
InMemoryArtifactService and GcsArtifactService currently construct GCS/In-Memory paths by directly interpolating user-supplied identifiers (user_id, app_name, and session_id) without validation. A malicious identifier containing path separators or traversal segments (e.g., ../other-user) could escape the intended scope and allow cross-user artifact access. This patch brings GcsArtifactService, InMemoryArtifactService, and FileArtifactService to parity and unifies their validation logic: - Refactored all path segment validation logic into a shared helper in `artifact_util.py`. - Validate `user_id`, `app_name`, and `session_id` in `GcsArtifactService`, `InMemoryArtifactService`, and `FileArtifactService` using the shared helper. - Consolidated path traversal tests into a parameterized suite in `test_artifact_service.py` and `test_artifact_util.py`. Closes #6115 Closes #6116 Co-authored-by: Jason Zhang <jasoncz@google.com> PiperOrigin-RevId: 943976800
1 parent c291821 commit 8718aef

6 files changed

Lines changed: 357 additions & 67 deletions

File tree

src/google/adk/artifacts/artifact_util.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,3 +134,32 @@ def validate_artifact_reference_scope(
134134
"Session-scoped artifact references must stay within the same"
135135
" session scope."
136136
)
137+
138+
139+
def validate_path_segment(value: str, field_name: str) -> None:
140+
"""Rejects values that could alter the constructed path.
141+
142+
Args:
143+
value: The caller-supplied identifier (e.g. user_id or session_id).
144+
field_name: Human-readable name used in the error message.
145+
146+
Raises:
147+
InputValidationError: If the value contains path separators, traversal
148+
segments, or null bytes.
149+
"""
150+
if not value:
151+
raise input_validation_error.InputValidationError(
152+
f"{field_name} must not be empty."
153+
)
154+
if "\x00" in value:
155+
raise input_validation_error.InputValidationError(
156+
f"{field_name} must not contain null bytes."
157+
)
158+
if "/" in value or "\\" in value:
159+
raise input_validation_error.InputValidationError(
160+
f"{field_name} {value!r} must not contain path separators."
161+
)
162+
if value in (".", "..") or ".." in value.split("/"):
163+
raise input_validation_error.InputValidationError(
164+
f"{field_name} {value!r} must not contain traversal segments."
165+
)

src/google/adk/artifacts/file_artifact_service.py

Lines changed: 5 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
from pydantic import ValidationError
3535
from typing_extensions import override
3636

37+
from . import artifact_util
3738
from ..errors.input_validation_error import InputValidationError
3839
from .base_artifact_service import ArtifactVersion
3940
from .base_artifact_service import BaseArtifactService
@@ -142,39 +143,14 @@ def _is_user_scoped(session_id: Optional[str], filename: str) -> bool:
142143
return session_id is None or _file_has_user_namespace(filename)
143144

144145

145-
def _validate_path_segment(value: str, field_name: str) -> None:
146-
"""Rejects values that could alter the constructed filesystem path.
147-
148-
Args:
149-
value: The caller-supplied identifier (e.g. user_id or session_id).
150-
field_name: Human-readable name used in the error message.
151-
152-
Raises:
153-
InputValidationError: If the value contains path separators, traversal
154-
segments, or null bytes.
155-
"""
156-
if not value:
157-
raise InputValidationError(f"{field_name} must not be empty.")
158-
if "\x00" in value:
159-
raise InputValidationError(f"{field_name} must not contain null bytes.")
160-
if "/" in value or "\\" in value:
161-
raise InputValidationError(
162-
f"{field_name} {value!r} must not contain path separators."
163-
)
164-
if value in (".", "..") or ".." in value.split("/"):
165-
raise InputValidationError(
166-
f"{field_name} {value!r} must not contain traversal segments."
167-
)
168-
169-
170146
def _user_artifacts_dir(base_root: Path) -> Path:
171147
"""Returns the path that stores user-scoped artifacts."""
172148
return base_root / "artifacts"
173149

174150

175151
def _session_artifacts_dir(base_root: Path, session_id: str) -> Path:
176152
"""Returns the path that stores session-scoped artifacts."""
177-
_validate_path_segment(session_id, "session_id")
153+
artifact_util.validate_path_segment(session_id, "session_id")
178154
return base_root / "sessions" / session_id / "artifacts"
179155

180156

@@ -256,7 +232,7 @@ def __init__(self, root_dir: Path | str):
256232

257233
def _base_root(self, user_id: str, /) -> Path:
258234
"""Returns the artifacts root directory for a user."""
259-
_validate_path_segment(user_id, "user_id")
235+
artifact_util.validate_path_segment(user_id, "user_id")
260236
return self.root_dir / "users" / user_id
261237

262238
def _scope_root(
@@ -269,7 +245,7 @@ def _scope_root(
269245
base = self._base_root(user_id)
270246
if _is_user_scoped(session_id, filename):
271247
return _user_artifacts_dir(base)
272-
if not session_id:
248+
if session_id is None:
273249
raise InputValidationError(
274250
"Session ID must be provided for session-scoped artifacts."
275251
)
@@ -543,7 +519,7 @@ def _list_artifact_keys_sync(
543519

544520
base_root = self._base_root(user_id)
545521

546-
if session_id:
522+
if session_id is not None:
547523
session_root = _session_artifacts_dir(base_root, session_id)
548524
for artifact_dir in _iter_artifact_dirs(session_root):
549525
metadata = self._latest_metadata(artifact_dir)

src/google/adk/artifacts/gcs_artifact_service.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,13 +167,16 @@ def _get_blob_prefix(
167167
session_id: Optional[str] = None,
168168
) -> str:
169169
"""Constructs the blob name prefix in GCS for a given artifact."""
170+
artifact_util.validate_path_segment(app_name, "app_name")
171+
artifact_util.validate_path_segment(user_id, "user_id")
170172
if self._file_has_user_namespace(filename):
171173
return f"{app_name}/{user_id}/user/{filename}"
172174

173175
if session_id is None:
174176
raise InputValidationError(
175177
"Session ID must be provided for session-scoped artifacts."
176178
)
179+
artifact_util.validate_path_segment(session_id, "session_id")
177180
return f"{app_name}/{user_id}/{session_id}/{filename}"
178181

179182
def _get_blob_name(
@@ -368,6 +371,10 @@ def _load_artifact(
368371
def _list_artifact_keys(
369372
self, app_name: str, user_id: str, session_id: Optional[str]
370373
) -> list[str]:
374+
artifact_util.validate_path_segment(app_name, "app_name")
375+
artifact_util.validate_path_segment(user_id, "user_id")
376+
if session_id is not None:
377+
artifact_util.validate_path_segment(session_id, "session_id")
371378
filenames = set()
372379

373380
if session_id:

src/google/adk/artifacts/in_memory_artifact_service.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,13 +85,16 @@ def _artifact_path(
8585
Returns:
8686
The constructed artifact path.
8787
"""
88+
artifact_util.validate_path_segment(app_name, "app_name")
89+
artifact_util.validate_path_segment(user_id, "user_id")
8890
if self._file_has_user_namespace(filename):
8991
return f"{app_name}/{user_id}/user/{filename}"
9092

9193
if session_id is None:
9294
raise InputValidationError(
9395
"Session ID must be provided for session-scoped artifacts."
9496
)
97+
artifact_util.validate_path_segment(session_id, "session_id")
9598
return f"{app_name}/{user_id}/{session_id}/{filename}"
9699

97100
@override
@@ -215,6 +218,10 @@ async def load_artifact(
215218
async def list_artifact_keys(
216219
self, *, app_name: str, user_id: str, session_id: Optional[str] = None
217220
) -> list[str]:
221+
artifact_util.validate_path_segment(app_name, "app_name")
222+
artifact_util.validate_path_segment(user_id, "user_id")
223+
if session_id is not None:
224+
artifact_util.validate_path_segment(session_id, "session_id")
218225
usernamespace_prefix = f"{app_name}/{user_id}/user/"
219226
session_prefix = (
220227
f"{app_name}/{user_id}/{session_id}/" if session_id else None

0 commit comments

Comments
 (0)