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 bc08bb6dc0bc4..af2304897baa4 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 @@ def execute(self, context: Context): 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 7955d3c2d4932..8caafa5d54543 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 @@ # 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 @@ def test_execute_copy_single_file( ) 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 @@ def test_execute_copy_single_file( @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 @@ def test_execute_move_single_file( ) 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 @@ def test_execute_copy_with_wildcard( 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 @@ def test_execute_copy_with_wildcard( ) 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 @@ def test_execute_move_with_wildcard( 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 @@ def test_execute_move_with_wildcard( ) 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 @@ def test_resolve_destination_path_allows_benign_nested(self): 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 @@ def test_resolve_destination_path_allows_relative_base(self, destination_path, s # 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(