This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new bfe84598a52 Fix destination path validation in GCS-to-SFTP transfers
(#74186)
bfe84598a52 is described below
commit bfe84598a526964be9d4ebc00ac65d6673bb3765
Author: Andrew Rukin <[email protected]>
AuthorDate: Mon Oct 5 13:31:45 2026 +0300
Fix destination path validation in GCS-to-SFTP transfers (#74186)
Check GCS-to-SFTP destination containment relative to the normalized remote
base. Allow configured parent-relative destinations while retaining rejection
of paths outside that base.
Co-authored-by: drewrukin <[email protected]>
---
.../google/cloud/transfers/gcs_to_sftp.py | 29 ++++++-------
.../google/cloud/transfers/test_gcs_to_sftp.py | 50 +++++++++++++++++-----
2 files changed, 54 insertions(+), 25 deletions(-)
diff --git
a/providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_sftp.py
b/providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_sftp.py
index bc08bb6dc0b..af2304897ba 100644
---
a/providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_sftp.py
+++
b/providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_sftp.py
@@ -20,8 +20,10 @@
from __future__ import annotations
import os
+import posixpath
from collections.abc import Sequence
from functools import cached_property
+from pathlib import PurePosixPath
from tempfile import NamedTemporaryFile
from typing import TYPE_CHECKING
@@ -177,25 +179,22 @@ class GCSToSFTPOperator(BaseOperator):
def _resolve_destination_path(self, source_object: str, prefix: str | None
= None) -> str:
if not self.keep_directory_structure:
if prefix:
- source_object = os.path.relpath(source_object, start=prefix)
+ source_object = posixpath.relpath(source_object, start=prefix)
else:
- source_object = os.path.basename(source_object)
+ source_object = posixpath.basename(source_object)
# GCS object names are arbitrary UTF-8 strings controlled by whoever
can
# write to the source bucket, so ``..`` segments or an absolute prefix
# could canonicalise outside ``destination_path`` on the SFTP server.
- # Resolve the join and require it to stay within the configured base.
- resolved = os.path.normpath(os.path.join(self.destination_path,
source_object))
- base = os.path.normpath(self.destination_path)
- escapes = (
- resolved == ".."
- or resolved.startswith(".." + os.sep)
- # An absolute source_object absorbed a relative base entirely.
- or (os.path.isabs(resolved) and not os.path.isabs(base))
- # A configured base directory must remain the prefix of the result.
- # ``base == "."`` is the SFTP login directory, where any
non-escaping
- # relative path is already in-bounds.
- or (base != "." and resolved != base and not
resolved.startswith(base.rstrip(os.sep) + os.sep))
- )
+ # The trusted base may contain ".." and refers to the remote server's
working directory.
+ base = posixpath.normpath(self.destination_path)
+ resolved = posixpath.normpath(posixpath.join(base, source_object))
+ try:
+ relative_path = PurePosixPath(resolved).relative_to(base)
+ except ValueError:
+ escapes = True
+ else:
+ # Pure paths retain ".."; an extra parent beyond a base such as
".." still escapes.
+ escapes = ".." in relative_path.parts
if escapes:
raise ValueError(
f"Refusing to copy GCS object {source_object!r}: resolved
destination "
diff --git
a/providers/google/tests/unit/google/cloud/transfers/test_gcs_to_sftp.py
b/providers/google/tests/unit/google/cloud/transfers/test_gcs_to_sftp.py
index 7955d3c2d49..8caafa5d545 100644
--- a/providers/google/tests/unit/google/cloud/transfers/test_gcs_to_sftp.py
+++ b/providers/google/tests/unit/google/cloud/transfers/test_gcs_to_sftp.py
@@ -36,6 +36,7 @@ DESTINATION_SFTP = "destination_path"
# TODO: After deprecating delimiter and wildcards in source objects,
# implement reverted changes from the first commit of PR #31261
class TestGoogleCloudStorageToSFTPOperator:
+ @pytest.mark.parametrize("destination_path", [DESTINATION_SFTP,
"../shared"])
@pytest.mark.parametrize(
("source_object", "target_object", "keep_directory_structure"),
[
@@ -48,13 +49,19 @@ class TestGoogleCloudStorageToSFTPOperator:
@mock.patch("airflow.providers.google.cloud.transfers.gcs_to_sftp.GCSHook")
@mock.patch("airflow.providers.google.cloud.transfers.gcs_to_sftp.SFTPHook")
def test_execute_copy_single_file(
- self, sftp_hook_mock, gcs_hook_mock, source_object, target_object,
keep_directory_structure
+ self,
+ sftp_hook_mock,
+ gcs_hook_mock,
+ source_object,
+ target_object,
+ keep_directory_structure,
+ destination_path,
):
task = GCSToSFTPOperator(
task_id=TASK_ID,
source_bucket=TEST_BUCKET,
source_object=source_object,
- destination_path=DESTINATION_SFTP,
+ destination_path=destination_path,
keep_directory_structure=keep_directory_structure,
move_object=False,
gcp_conn_id=GCP_CONN_ID,
@@ -73,11 +80,12 @@ class TestGoogleCloudStorageToSFTPOperator:
)
sftp_hook_mock.return_value.store_file.assert_called_with(
- os.path.join(DESTINATION_SFTP, target_object), mock.ANY
+ os.path.join(destination_path, target_object), mock.ANY
)
gcs_hook_mock.return_value.delete.assert_not_called()
+ @pytest.mark.parametrize("destination_path", [DESTINATION_SFTP,
"../shared"])
@pytest.mark.parametrize(
("source_object", "target_object", "keep_directory_structure"),
[
@@ -90,13 +98,19 @@ class TestGoogleCloudStorageToSFTPOperator:
@mock.patch("airflow.providers.google.cloud.transfers.gcs_to_sftp.GCSHook")
@mock.patch("airflow.providers.google.cloud.transfers.gcs_to_sftp.SFTPHook")
def test_execute_move_single_file(
- self, sftp_hook_mock, gcs_hook_mock, source_object, target_object,
keep_directory_structure
+ self,
+ sftp_hook_mock,
+ gcs_hook_mock,
+ source_object,
+ target_object,
+ keep_directory_structure,
+ destination_path,
):
task = GCSToSFTPOperator(
task_id=TASK_ID,
source_bucket=TEST_BUCKET,
source_object=source_object,
- destination_path=DESTINATION_SFTP,
+ destination_path=destination_path,
keep_directory_structure=keep_directory_structure,
move_object=True,
gcp_conn_id=GCP_CONN_ID,
@@ -115,11 +129,12 @@ class TestGoogleCloudStorageToSFTPOperator:
)
sftp_hook_mock.return_value.store_file.assert_called_with(
- os.path.join(DESTINATION_SFTP, target_object), mock.ANY
+ os.path.join(destination_path, target_object), mock.ANY
)
gcs_hook_mock.return_value.delete.assert_called_once_with(TEST_BUCKET,
source_object)
+ @pytest.mark.parametrize("destination_path", [DESTINATION_SFTP,
"../shared"])
@pytest.mark.parametrize(
(
"source_object",
@@ -188,13 +203,14 @@ class TestGoogleCloudStorageToSFTPOperator:
gcs_files_list,
target_objects,
keep_directory_structure,
+ destination_path,
):
gcs_hook_mock.return_value.list.return_value = gcs_files_list
operator = GCSToSFTPOperator(
task_id=TASK_ID,
source_bucket=TEST_BUCKET,
source_object=source_object,
- destination_path=DESTINATION_SFTP,
+ destination_path=destination_path,
keep_directory_structure=keep_directory_structure,
move_object=False,
gcp_conn_id=GCP_CONN_ID,
@@ -212,13 +228,14 @@ class TestGoogleCloudStorageToSFTPOperator:
)
sftp_hook_mock.return_value.store_file.assert_has_calls(
[
- mock.call(os.path.join(DESTINATION_SFTP, target_object),
mock.ANY)
+ mock.call(os.path.join(destination_path, target_object),
mock.ANY)
for target_object in target_objects
]
)
gcs_hook_mock.return_value.delete.assert_not_called()
+ @pytest.mark.parametrize("destination_path", [DESTINATION_SFTP,
"../shared"])
@pytest.mark.parametrize(
(
"source_object",
@@ -287,13 +304,14 @@ class TestGoogleCloudStorageToSFTPOperator:
gcs_files_list,
target_objects,
keep_directory_structure,
+ destination_path,
):
gcs_hook_mock.return_value.list.return_value = gcs_files_list
operator = GCSToSFTPOperator(
task_id=TASK_ID,
source_bucket=TEST_BUCKET,
source_object=source_object,
- destination_path=DESTINATION_SFTP,
+ destination_path=destination_path,
keep_directory_structure=keep_directory_structure,
move_object=True,
gcp_conn_id=GCP_CONN_ID,
@@ -311,7 +329,7 @@ class TestGoogleCloudStorageToSFTPOperator:
)
sftp_hook_mock.return_value.store_file.assert_has_calls(
[
- mock.call(os.path.join(DESTINATION_SFTP, target_object),
mock.ANY)
+ mock.call(os.path.join(destination_path, target_object),
mock.ANY)
for target_object in target_objects
]
)
@@ -588,6 +606,13 @@ class TestGoogleCloudStorageToSFTPOperator:
pytest.param(".", "file.txt", "file.txt", id="dot-base-benign"),
pytest.param("", "file.txt", "file.txt", id="empty-base-benign"),
pytest.param(".", "sub/dir/file.txt", "sub/dir/file.txt",
id="dot-base-nested"),
+ pytest.param("..", "file.txt", "../file.txt", id="parent-base"),
+ pytest.param("../shared", "file.txt", "../shared/file.txt",
id="parent-shared-base"),
+ pytest.param("../../shared", "sub/file.txt",
"../../shared/sub/file.txt", id="two-parents"),
+ pytest.param("./../shared//", "file.txt", "../shared/file.txt",
id="normalized-base"),
+ pytest.param("uploads/../../shared", "file.txt",
"../shared/file.txt", id="normalized-parents"),
+ pytest.param("../shared", "sub/../file.txt", "../shared/file.txt",
id="contained-normalization"),
+ pytest.param("/", "sub/file.txt", "/sub/file.txt", id="root-base"),
],
)
def test_resolve_destination_path_allows_relative_base(self,
destination_path, source_object, expected):
@@ -618,6 +643,11 @@ class TestGoogleCloudStorageToSFTPOperator:
# against the configured base must still reject them.
pytest.param("incoming", "../.ssh/authorized_keys",
id="dotdot-escape-from-nested-base"),
pytest.param("uploads/in", "../../etc/passwd",
id="dotdot-escape-from-deeper-base"),
+ pytest.param("../shared", "../other/file.txt",
id="sibling-of-parent-base"),
+ pytest.param("../shared", "../shared-other/file.txt",
id="similar-prefix-sibling"),
+ pytest.param("../shared", "/file.txt",
id="absolute-absorbs-parent-base"),
+ pytest.param("..", "../file.txt", id="additional-parent"),
+ pytest.param("../..", "../file.txt",
id="additional-parent-from-two-parents"),
],
)
def test_resolve_destination_path_rejects_escape_from_relative_base(
