Skip to content

Allow GCSToSambaOperator to write to the share root - #73863

Open
MohammadHijjawi97 wants to merge 1 commit into
apache:mainfrom
MohammadHijjawi97:fix-gcs-to-samba-share-root
Open

MohammadHijjawi97 wants to merge 1 commit into
apache:mainfrom
MohammadHijjawi97:fix-gcs-to-samba-share-root

Conversation

@MohammadHijjawi97

Copy link
Copy Markdown

GCSToSambaOperator rejects every object copied to the root of the SMB share. The destination containment check added in #67857 compares the resolved path with destination_path + os.sep, so:

  • destination_path="/" builds the prefix "//", and "/dir/file.txt" does not start with it;
  • destination_path="" or "." normalises to ".", and the resolved relative path ("dir/file.txt") has no "./" prefix.

In both cases the task fails with ValueError: Resolved destination path ... is outside the configured destination_path, although the file would land inside the share. This has affected the operator since samba provider 4.12.6.

This PR handles the share root the same way GCSToSFTPOperator._resolve_destination_path and the Amazon validate_destination_path helper already do:

  • for a relative root, it rejects only paths that climb out with .. or that are absolute;
  • for any other base, it strips a trailing separator before building the prefix.

Object names containing .. are still refused.

Tests: added parametrized cases for /, "" and . as the destination, plus traversal cases from a relative share root. pytest providers/samba/tests/unit/samba/transfers/test_gcs_to_samba.py: 32 passed; the 3 share-root cases fail without the change. ruff format / ruff check are clean.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code following the guidelines

I used an AI coding assistant (Claude Code) while writing the fix and tests; I reviewed the changes and ran the checks listed above.

@boring-cyborg

boring-cyborg Bot commented Sep 28, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@yuseok89 yuseok89 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. The share-root handling looks good to me.

@eladkal
eladkal requested review from potiuk and shahar1 September 30, 2026 11:52
@MohammadHijjawi97

Copy link
Copy Markdown
Author

The failing Non-DB tests: providers job is not related to this change. Every failure and error in it comes from one SQLAlchemy error, first raised while collecting providers/google/tests/unit/google/cloud/hooks/test_stackdriver.py:

When initializing mapper Mapper[AssetWatcherModel(asset_watcher)], expression 'Trigger' failed to locate a name ('Trigger')

The samba hook, hive and common.compat failures are the follow-on "One or more mappers failed to initialize" error in the same pytest session (a samba change pulls in the google tests, so they share it). None of the failures are in test_gcs_to_samba.py.

This is the collection issue fixed on main by #74066, which was merged on Oct 2, after this run. I can rebase onto main so the job runs with that fix.

The destination containment check added in apache#67857 compares the resolved
path against destination_path + os.sep. For destination_path="/" that
prefix becomes "//", and for "" or "." the resolved path is relative
without a "./" prefix, so every object copied to the root of the share
was rejected as being outside destination_path. Handle both forms the
same way GCSToSFTPOperator and the S3-to-SFTP/FTP transfers already do,
while still refusing ".." and absolute object names.
@MohammadHijjawi97
MohammadHijjawi97 force-pushed the fix-gcs-to-samba-share-root branch from 2ff6175 to 96e8b03 Compare October 4, 2026 13:59
@MohammadHijjawi97

Copy link
Copy Markdown
Author

Rebased onto main to pick up #74066; no change to the PR's own code.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants