potiuk commented on code in PR #72162:
URL: https://github.com/apache/airflow/pull/72162#discussion_r4044124004
##########
providers/amazon/src/airflow/providers/amazon/aws/log/s3_task_handler.py:
##########
@@ -84,6 +84,18 @@ def upload(self, path: os.PathLike | str, ti: RuntimeTI |
None = None) -> None:
local_loc = self.base_log_folder.joinpath(path)
remote_loc = os.path.join(self.remote_base, path)
+ # The log path is supplied by the caller and is not guaranteed to stay
within
+ # ``base_log_folder``: ``joinpath`` and ``PurePath.relative_to`` are
purely lexical and
+ # do not normalise ``..``. Without this check a traversing path would
have its contents
+ # uploaded to the remote log store and, with ``delete_local_copy``,
its parent directory
+ # removed. Mirrors the containment check in
``CloudWatchRemoteLogIO.upload``.
Review Comment:
I'd rather keep this one, though you have a point on length.
The lines below say *what* — resolve the path, compare it to the base, skip
if it escapes. What they can't say is *why* the check has to exist: that the
path comes from the caller, and that `joinpath` and `PurePath.relative_to` are
purely lexical and will carry a `..` through without normalising it. Nothing in
the code below hints at that, and it is the entire reason a lexical-looking
comparison is not enough here.
`AGENTS.md` draws the line in the same place — "comment sparingly, code says
*what*, comments say *why*" — and then names this exact carve-out: *"Reserve
longer explanation for genuinely complex or non-obvious logic (e.g. a security
check whose threat model isn't apparent)."*
Five lines was more than it needed, so I've cut the prose restatement of the
failure mode and kept the threat and the cross-reference, in all three handlers:
```python
# ``joinpath``/``relative_to`` are lexical and do not normalise ``..``, so a
caller-supplied
# path can escape ``base_log_folder``. Mirrors the check in
``CloudWatchRemoteLogIO.upload``.
```
Pushed as `baa3ac4abe`. Does that work for you?
---
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
