Skip to content

Commit 48b28de

Browse files
GWealeadk-bot
authored andcommitted
fix(gcs): judge a local path by where it lands, not by whether it is absolute
Co-authored-by: George Weale <gweale@google.com> PiperOrigin-RevId: 979339666
1 parent fb45e24 commit 48b28de

2 files changed

Lines changed: 86 additions & 16 deletions

File tree

‎src/google/adk/integrations/gcs/storage_tool.py‎

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,8 @@ def _resolve_local_path(path: str, settings: GCSToolSettings | None) -> str:
3030
"""Resolves a model-supplied local file path under the configured root.
3131
3232
Raises:
33-
ValueError: If no root is configured, if the path is absolute, if it names
34-
the root itself, or if it resolves outside the root once symlinks are
35-
followed.
33+
ValueError: If no root is configured, if it names the root itself, or if
34+
it resolves outside the root once symlinks are followed.
3635
"""
3736
root = settings.local_file_root if settings else None
3837
if not root:
@@ -41,10 +40,16 @@ def _resolve_local_path(path: str, settings: GCSToolSettings | None) -> str:
4140
" settings to let these tools read from or write to the local"
4241
" filesystem."
4342
)
44-
if Path(path).is_absolute():
45-
raise ValueError(f"Local file path must be relative: {path}")
4643
resolved_root = Path(root).resolve()
47-
resolved = (resolved_root / path).resolve()
44+
# An absolute path is judged by where it lands, like a relative one: the
45+
# containment check below is what confines either, and it runs after symlinks
46+
# are followed.
47+
candidate = Path(path)
48+
if not candidate.is_absolute():
49+
candidate = resolved_root / candidate
50+
resolved = candidate.resolve()
51+
# The root itself stays out of the message: it reaches the model, and the
52+
# deployment's filesystem layout is not the model's business.
4853
if not resolved.is_relative_to(resolved_root):
4954
raise ValueError(f"Local file path escapes the configured root: {path}")
5055
if resolved == resolved_root:
@@ -188,8 +193,9 @@ def create_object(
188193
credentials (Credentials): The credentials to use for the request.
189194
data (str, optional): The content to write to the object.
190195
source_file_path (str, optional): The path of a local file to upload,
191-
relative to the directory the tools are configured to use. Rejected
192-
when no such directory is configured.
196+
either absolute or relative to the directory the tools are configured
197+
to use. Must name a file inside that directory. Rejected when no such
198+
directory is configured.
193199
194200
Returns:
195201
dict: Dictionary indicating success or error.
@@ -242,9 +248,10 @@ def get_object_data(
242248
generation (int, optional): If present, selects a specific generation of
243249
this object.
244250
destination_file_path (str, optional): The path to save the downloaded
245-
file to, relative to the directory the tools are configured to use.
246-
Rejected when no such directory is configured. Missing parent
247-
directories are created.
251+
file to, either absolute or relative to the directory the tools are
252+
configured to use. Must name a file inside that directory. Rejected
253+
when no such directory is configured. Missing parent directories are
254+
created.
248255
249256
Returns:
250257
dict: Dictionary containing the object data as a string or confirming file

‎tests/unittests/integrations/gcs/test_gcs_storage_tool.py‎

Lines changed: 68 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@ def test_resolve_local_path_without_local_file_root():
4242
@pytest.mark.parametrize(
4343
"path, expected_error",
4444
[
45-
pytest.param("/etc/passwd", "must be relative", id="absolute"),
45+
pytest.param(
46+
"/etc/passwd", "escapes the configured root", id="absolute-outside"
47+
),
4648
pytest.param(
4749
"../outside.txt", "escapes the configured root", id="dotdot"
4850
),
@@ -68,6 +70,31 @@ def test_resolve_local_path_rejects(tmp_path, path, expected_error):
6870
)
6971

7072

73+
def test_resolve_local_path_accepts_an_absolute_path_inside_the_root(tmp_path):
74+
"""Test _resolve_local_path accepts an absolute path that lands in the root."""
75+
root = tmp_path / "root"
76+
root.mkdir()
77+
78+
resolved = storage_tool._resolve_local_path(
79+
str(root / "report.csv"), GCSToolSettings(local_file_root=str(root))
80+
)
81+
82+
assert resolved == str(root.resolve() / "report.csv")
83+
84+
85+
def test_resolve_local_path_keeps_the_root_out_of_the_message(tmp_path):
86+
"""Test the rejection does not disclose the configured root, which the model reads."""
87+
root = tmp_path / "root"
88+
root.mkdir()
89+
90+
with pytest.raises(ValueError) as excinfo:
91+
storage_tool._resolve_local_path(
92+
"/etc/passwd", GCSToolSettings(local_file_root=str(root))
93+
)
94+
95+
assert str(root.resolve()) not in str(excinfo.value)
96+
97+
7198
def test_list_objects():
7299
"""Test list_objects function."""
73100
with mock.patch.object(
@@ -259,8 +286,10 @@ def test_create_object_from_file_without_local_file_root():
259286
mock_blob.upload_from_filename.assert_not_called()
260287

261288

262-
def test_create_object_from_file_rejects_absolute_path(tmp_path):
263-
"""Test create_object refuses an absolute source_file_path."""
289+
def test_create_object_from_file_rejects_absolute_path_outside_the_root(
290+
tmp_path,
291+
):
292+
"""Test create_object refuses a source_file_path outside the configured root."""
264293
with mock.patch.object(
265294
client, "get_gcs_client", autospec=True
266295
) as mock_get_client:
@@ -271,19 +300,53 @@ def test_create_object_from_file_rejects_absolute_path(tmp_path):
271300
mock_blob = mock.MagicMock()
272301
mock_bucket.blob.return_value = mock_blob
273302

303+
root = tmp_path / "root"
304+
root.mkdir()
305+
274306
creds = mock.create_autospec(Credentials, instance=True)
275307
result = storage_tool.create_object(
276308
bucket_name="test-bucket",
277309
object_name="test-object",
278310
source_file_path=str(tmp_path / "secrets.json"),
279311
credentials=creds,
280-
settings=GCSToolSettings(local_file_root=str(tmp_path)),
312+
settings=GCSToolSettings(local_file_root=str(root)),
281313
)
282314
assert result["status"] == "ERROR"
283-
assert "must be relative" in result["error_details"]
315+
assert "escapes the configured root" in result["error_details"]
284316
mock_blob.upload_from_filename.assert_not_called()
285317

286318

319+
def test_create_object_from_file_accepts_absolute_path_inside_the_root(
320+
tmp_path,
321+
):
322+
"""Test create_object uploads from an absolute path under the configured root."""
323+
with mock.patch.object(
324+
client, "get_gcs_client", autospec=True
325+
) as mock_get_client:
326+
mock_client = mock.MagicMock()
327+
mock_get_client.return_value = mock_client
328+
mock_bucket = mock.MagicMock()
329+
mock_client.get_bucket.return_value = mock_bucket
330+
mock_blob = mock.MagicMock()
331+
mock_bucket.blob.return_value = mock_blob
332+
333+
source = tmp_path / "report.csv"
334+
source.write_text("a,b\n1,2\n")
335+
336+
creds = mock.create_autospec(Credentials, instance=True)
337+
result = storage_tool.create_object(
338+
bucket_name="test-bucket",
339+
object_name="test-object",
340+
source_file_path=str(source),
341+
credentials=creds,
342+
settings=GCSToolSettings(local_file_root=str(tmp_path)),
343+
)
344+
assert result["status"] == "SUCCESS"
345+
mock_blob.upload_from_filename.assert_called_once_with(
346+
str(source.resolve())
347+
)
348+
349+
287350
def test_create_object_no_data():
288351
"""Test create_object function when neither data nor source_file_path is provided."""
289352
with mock.patch.object(

0 commit comments

Comments
 (0)