Copilot commented on code in PR #62616:
URL: https://github.com/apache/airflow/pull/62616#discussion_r2899179199
##########
airflow-core/src/airflow/executors/workloads/callback.py:
##########
@@ -90,7 +90,7 @@ def make(
name=dag_run.dag_model.bundle_name,
version=dag_run.bundle_version,
)
- fname = f"executor_callbacks/{callback.id}" # TODO: better log file
template
+ fname =
f"executor_callbacks/{dag_run.dag_id}/{dag_run.run_id}/{callback.id}"
Review Comment:
`dag_run.run_id` is user-influenced (and can be made more permissive via
`scheduler.allowed_run_id_pattern`). Embedding it directly into `log_path` can
enable path traversal (e.g. `../`) and writing callback logs outside
`base_log_folder` when the path is later joined to the base log directory.
Please sanitize/encode the `dag_id` and `run_id` path components (and
explicitly reject `..` / path separators) before building the relative log path.
##########
airflow-core/src/airflow/executors/workloads/callback.py:
##########
@@ -90,7 +90,7 @@ def make(
name=dag_run.dag_model.bundle_name,
version=dag_run.bundle_version,
)
- fname = f"executor_callbacks/{callback.id}" # TODO: better log file
template
+ fname =
f"executor_callbacks/{dag_run.dag_id}/{dag_run.run_id}/{callback.id}"
Review Comment:
This change alters the callback log file layout. To avoid regressions (and
to document the intended template), add a unit test for
`ExecuteCallback.make()` that asserts the rendered `log_path` includes the
expected dag_id/run_id/callback.id hierarchy.
--
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]
