Skip to content

Fix destination path validation in GCS-to-SFTP transfers #74186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
potiuk merged 1 commit into from
Oct 5, 2026
Merged

Fix destination path validation in GCS-to-SFTP transfers #74186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 14 additions & 15 deletions providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_sftp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 "
Expand Down
50 changes: 40 additions & 10 deletions providers/google/tests/unit/google/cloud/transfers/test_gcs_to_sftp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
[
Expand All @@ -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,
Expand All @@ -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"),
[
Expand All @@ -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,
Expand All @@ -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",
Expand Down Expand Up @@ -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,
Expand All @@ -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",
Expand Down Expand Up @@ -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,
Expand All @@ -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
]
)
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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(
Expand Down