You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Samin061 (via GitHub)" <gi...@apache.org> on 2026/07/14 06:07:34 UTC
[PR] prevent path traversal in ObjectStoragePath recursive copy to local [airflow]
Samin061 opened a new pull request, #69850:
URL: https://github.com/apache/airflow/pull/69850
ObjectStoragePath.copy() delegates a recursive remote-to-local transfer straight to fsspec's fs.get, so object keys containing .. segments (which anyone able to write to the source prefix controls) are followed verbatim and land outside the destination directory. Add a containment check on the recursive remote-to-local branch that expands the source keys and refuses any whose resolved local path escapes the destination, matching the guards already in the S3 and GCS sync_to_local_dir helpers. Non-recursive and same-store copies are untouched, so valid transfers behave exactly as before.
---
##### Was generative AI tooling used to co-author this PR?
- [ ] Yes (please specify the tool below)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] prevent path traversal in ObjectStoragePath recursive copy to local [airflow]
Posted by "steveahnahn (via GitHub)" <gi...@apache.org>.
steveahnahn commented on code in PR #69850:
URL: https://github.com/apache/airflow/pull/69850#discussion_r3693165926
##########
task-sdk/src/airflow/sdk/io/path.py:
##########
@@ -339,6 +340,25 @@ def size(self) -> int:
"""Size in bytes of the file at this path."""
return self.fs.size(self.path)
+ def _raise_if_remote_keys_escape(self, local_dir: str, **kwargs) -> None:
+ """
+ Refuse a recursive download when a remote object key resolves outside ``local_dir``.
+
+ Object-store keys are arbitrary strings and may contain ``..`` segments written by
+ anyone who can put objects in the source prefix; ``fs.get`` follows them verbatim and
+ would write outside the destination directory.
+ """
+ dst_root = os.path.realpath(local_dir)
+ for src_key in self.fs.expand_path(self.path, recursive=True, **kwargs):
+ if self.fs.isdir(src_key):
+ continue
Review Comment:
This adds one isdir round trip per object, on top of a second full expand_path traversal. Measured against main with a fake remote fs: 10 objects goes from 17 isdir calls to 34, 50 from 57 to 114, 200 from 207 to 414. It exactly doubles them, so the cost grows with object count on a path that is already doing a bulk transfer.
self.fs.find(self.path, **kwargs) returns files only and gives an identical file set to the expand_path plus per-key isdir filter, so swapping it in would drop the per-key calls entirely and make the guard close to free.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] prevent path traversal in ObjectStoragePath recursive copy to local [airflow]
Posted by "Samin061 (via GitHub)" <gi...@apache.org>.
Samin061 commented on code in PR #69850:
URL: https://github.com/apache/airflow/pull/69850#discussion_r3756256383
##########
task-sdk/src/airflow/sdk/io/path.py:
##########
@@ -339,6 +340,25 @@ def size(self) -> int:
"""Size in bytes of the file at this path."""
return self.fs.size(self.path)
+ def _raise_if_remote_keys_escape(self, local_dir: str, **kwargs) -> None:
+ """
+ Refuse a recursive download when a remote object key resolves outside ``local_dir``.
+
+ Object-store keys are arbitrary strings and may contain ``..`` segments written by
+ anyone who can put objects in the source prefix; ``fs.get`` follows them verbatim and
+ would write outside the destination directory.
+ """
+ dst_root = os.path.realpath(local_dir)
+ for src_key in self.fs.expand_path(self.path, recursive=True, **kwargs):
+ if self.fs.isdir(src_key):
+ continue
Review Comment:
Good point, the doubled traversal was wasteful. Switched to fs.find(self.path), which returns files only, so the per-key isdir calls are gone and the guard walks the tree just once. Tests still green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] prevent path traversal in ObjectStoragePath recursive copy to local [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #69850:
URL: https://github.com/apache/airflow/pull/69850#issuecomment-5839375006
Hello @Samin061 - thank you for your contributions to Apache Airflow!
The Airflow community has introduced a limit of **5 open pull requests at a time** for contributors without write access to the repository. You currently have 7 open pull requests, so - as a one-time step of introducing the limit - we closed the ones where maintainers have not engaged yet:
* #69585 - Fix execution API 404 for connection IDs containing a slash
* #69644 - validate log server hostname before building the log fetch url
* #69850 - prevent path traversal in ObjectStoragePath recursive copy to local
* #71700 - escape connection type and field name in registry connection builder
* #71766 - restrict permissions on teradata credential files written for ssh runs
These pull requests stay open because maintainers are already engaged in them - they count towards your limit:
* #70000 - Strip connection headers on cross-host redirect in HttpHook
* #71423 - disable external entities in SAML idp response parsing
This is **not** a judgement of you or of your changes. We never told contributors before that opening many pull requests at once was a problem, so there is nothing to feel bad about - and nothing is lost: your branches, commits and the review history stay where they are.
What we ask you to do is to make your **first prioritization decision**: choose which of the pull requests above matter most to you, and reopen them (up to 5 open at a time, including the ones still open) with the "Reopen pull request" button or `gh pr reopen <PR_NUMBER> --repo apache/airflow`. Reopen the ones you are ready to follow through - keep them rebased, respond to review comments and fix failing checks.
While your pull requests are waiting for review, the most valuable thing you can do is help in other ways - reviewing other contributors' pull requests, helping with issues, and taking part in the discussions on the devlist and Slack.
Why we introduced the limit, what it means for you and how to reopen or restore a pull request is explained in https://github.com/apache/airflow/blob/main/contributing-docs/32_open_pull_request_limit.rst.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] prevent path traversal in ObjectStoragePath recursive copy to local [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #69850: prevent path traversal in ObjectStoragePath recursive copy to local
URL: https://github.com/apache/airflow/pull/69850
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org