potiuk opened a new pull request, #72162:
URL: https://github.com/apache/airflow/pull/72162
## Why
`S3RemoteLogIO.upload`, `GCSRemoteLogIO.upload` and `WasbRemoteLogIO.upload`
build the local log path as `base_log_folder.joinpath(path)`, then read that
file and — when `delete_local_copy` is set —
`shutil.rmtree(os.path.dirname(local_loc))`.
`joinpath` and `PurePath.relative_to` are purely **lexical**: neither
normalises `..`. A relative log path containing `..` therefore resolves outside
`base_log_folder`, so its contents get uploaded to the remote log store and its
parent directory gets removed.
`CloudWatchRemoteLogIO.upload`, in the same package, already guards exactly
this operation:
```python
base = self.base_log_folder.resolve()
local_path = (raw if raw.is_absolute() else base / raw).resolve()
try:
local_path.relative_to(base)
except ValueError:
self.log.warning("Skipping deletion: path %s is outside base_log_folder
%s", local_path, base)
return
```
So the correct behaviour is already established here — three of the four
remote log handlers just don't have it. This makes them consistent.
## What
- Add a `resolve()` + `relative_to(base_log_folder)` containment check to
`upload()` in the S3, GCS and WASB remote log handlers, skipping with a warning
when the path escapes — mirroring the CloudWatch handler.
- The check runs **before** the file is read, so it covers both the
read/upload and the `delete_local_copy` deletion.
- Add a test to each of the three provider test suites covering the
traversing path and asserting the file and its parent survive.
## Compatibility
No behaviour change for paths that resolve inside `base_log_folder`. That
explicitly includes relative paths with internal `..` segments that stay within
the folder (e.g. `dag_id=a/../1.log`), and absolute paths inside the folder —
both verified.
## Testing
`moto` and the other provider test dependencies aren't available in my local
environment, so I could not execute the three provider suites — **CI needs to
run them.** What I did verify locally:
- `py_compile` and `ruff check` clean on all six changed files.
- The containment arithmetic itself, exercised against real temporary
directories across six cases: normal relative path (allowed), `..` traversal
(blocked), deep traversal (blocked), absolute inside base (allowed), absolute
outside base (blocked), and internal `..` staying inside (allowed — confirming
no false positives on legitimate paths).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
