rjgoyln commented on code in PR #74322:
URL: https://github.com/apache/airflow/pull/74322#discussion_r4194869303
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -1823,7 +1823,10 @@ def sync_to_local_dir(self, bucket_name: str, local_dir:
Path, s3_prefix="", del
if obj.key.endswith("/"):
continue
obj_path = Path(obj.key)
- local_target_path =
local_dir.joinpath(obj_path.relative_to(s3_prefix))
+ relative_path = obj_path.relative_to(s3_prefix)
+ if relative_path == Path("."):
Review Comment:
One small robustness thought: a key like `dags/../<bundle_name>` gets past
this check because its relative path is `../<bundle_name>`, but it still
resolves to the sync directory and could hit the same `IsADirectoryError` on
refresh.
The containment check below already resolves the target, and
`relative_to(local_dir_resolved)` returns `.` for this case. Would it make
sense to reuse that result and skip when it is `.` instead, so both cases are
covered?
```python
local_target_path = local_dir.joinpath(obj_path.relative_to(s3_prefix))
try:
resolved_relative_path =
local_target_path.resolve().relative_to(local_dir_resolved)
except ValueError:
raise S3HookPathTraversalError(
f"S3 object key {obj.key!r} resolves outside local directory
{local_dir}"
) from None
if resolved_relative_path == Path("."):
continue
```
I tried this locally, and the 21 sync/bundle tests still pass, with
`dags/../<bundle_name>` skipped as well.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
