Skip to content

Commit 5ccefd3

Browse files
committed
fix(artifacts): validate user_id and session_id in FileArtifactService path construction
_base_root and _session_artifacts_dir used user_id and session_id directly in Path construction without checking for traversal segments. A crafted value (e.g. "../../x") could escape the storage root. Add _validate_path_segment using the same resolve()+relative_to() guard already applied to filenames, and wire it into both functions.
1 parent b3e9962 commit 5ccefd3

File tree

2 files changed

+91
-2
lines changed

2 files changed

+91
-2
lines changed

src/google/adk/artifacts/file_artifact_service.py

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,31 @@ def _resolve_scoped_artifact_path(
133133
return candidate, relative
134134

135135

136+
def _validate_path_segment(parent: Path, segment: str, label: str) -> Path:
137+
"""Validates that a path segment does not escape its parent directory.
138+
139+
Args:
140+
parent: The directory the segment must stay within.
141+
segment: Caller-supplied value used as a single path component.
142+
label: Human-readable name for error messages (e.g. ``"User ID"``).
143+
144+
Returns:
145+
The resolved path ``parent / segment``.
146+
147+
Raises:
148+
InputValidationError: If the resulting path escapes ``parent``.
149+
"""
150+
parent_resolved = parent.resolve(strict=False)
151+
candidate = (parent_resolved / segment).resolve(strict=False)
152+
try:
153+
candidate.relative_to(parent_resolved)
154+
except ValueError as exc:
155+
raise InputValidationError(
156+
f"{label} {segment!r} resolves outside storage directory."
157+
) from exc
158+
return candidate
159+
160+
136161
def _is_user_scoped(session_id: Optional[str], filename: str) -> bool:
137162
"""Determines whether artifacts should be stored in the user namespace."""
138163
return session_id is None or _file_has_user_namespace(filename)
@@ -145,7 +170,11 @@ def _user_artifacts_dir(base_root: Path) -> Path:
145170

146171
def _session_artifacts_dir(base_root: Path, session_id: str) -> Path:
147172
"""Returns the path that stores session-scoped artifacts."""
148-
return base_root / "sessions" / session_id / "artifacts"
173+
sessions_dir = base_root / "sessions"
174+
return (
175+
_validate_path_segment(sessions_dir, session_id, "Session ID")
176+
/ "artifacts"
177+
)
149178

150179

151180
def _versions_dir(artifact_dir: Path) -> Path:
@@ -220,7 +249,8 @@ def __init__(self, root_dir: Path | str):
220249

221250
def _base_root(self, user_id: str, /) -> Path:
222251
"""Returns the artifacts root directory for a user."""
223-
return self.root_dir / "users" / user_id
252+
users_dir = self.root_dir / "users"
253+
return _validate_path_segment(users_dir, user_id, "User ID")
224254

225255
def _scope_root(
226256
self,

tests/unittests/artifacts/test_artifact_service.py

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -896,3 +896,62 @@ async def test_save_artifact_with_snake_case_dict(
896896
assert loaded is not None
897897
assert loaded.inline_data is not None
898898
assert loaded.inline_data.mime_type == "text/plain"
899+
900+
901+
@pytest.mark.asyncio
902+
@pytest.mark.parametrize(
903+
"user_id",
904+
[
905+
"../../escape",
906+
"../sibling",
907+
"valid/../../../etc",
908+
],
909+
)
910+
async def test_file_save_rejects_traversal_user_id(tmp_path, user_id):
911+
"""FileArtifactService rejects user_id values that escape the storage root."""
912+
artifact_service = FileArtifactService(root_dir=tmp_path / "artifacts")
913+
part = types.Part(text="content")
914+
with pytest.raises(InputValidationError):
915+
await artifact_service.save_artifact(
916+
app_name="myapp",
917+
user_id=user_id,
918+
session_id="sess1",
919+
filename="file.txt",
920+
artifact=part,
921+
)
922+
923+
924+
@pytest.mark.asyncio
925+
@pytest.mark.parametrize(
926+
"session_id",
927+
[
928+
"../../escape",
929+
"../sibling",
930+
"valid/../../../tmp",
931+
],
932+
)
933+
async def test_file_save_rejects_traversal_session_id(tmp_path, session_id):
934+
"""FileArtifactService rejects session_id values that escape the storage root."""
935+
artifact_service = FileArtifactService(root_dir=tmp_path / "artifacts")
936+
part = types.Part(text="content")
937+
with pytest.raises(InputValidationError):
938+
await artifact_service.save_artifact(
939+
app_name="myapp",
940+
user_id="user1",
941+
session_id=session_id,
942+
filename="file.txt",
943+
artifact=part,
944+
)
945+
946+
947+
@pytest.mark.asyncio
948+
async def test_file_delete_rejects_traversal_session_id(tmp_path):
949+
"""Traversal session_id is caught before shutil.rmtree can be reached."""
950+
artifact_service = FileArtifactService(root_dir=tmp_path / "artifacts")
951+
with pytest.raises(InputValidationError):
952+
await artifact_service.delete_artifact(
953+
app_name="myapp",
954+
user_id="user1",
955+
session_id="../../escape",
956+
filename="file.txt",
957+
)

0 commit comments

Comments
 (0)