fat-catTW commented on code in PR #72499:
URL: https://github.com/apache/airflow/pull/72499#discussion_r3947286024
##########
task-sdk/src/airflow/sdk/definitions/param.py:
##########
@@ -382,6 +383,27 @@ def process_params(
if task.params:
params.update(task.params)
if conf.getboolean("core", "dag_run_conf_overrides_params") and
dagrun_conf:
+ # Mask before logging: dag.params/task.params are already merged into
`params` at this
+ # point, so the declared format="password" schema for a key is
available here even though
+ # dagrun_conf hasn't been merged in yet. Registering first ensures the
debug log below is
+ # redacted by the SecretsMasker logging filter rather than emitting
the raw value.
+ _mask_password_values(params, dagrun_conf)
logger.debug("Updating task params (%s) with DagRun.conf (%s)",
params, dagrun_conf)
Review Comment:
This can still leak the password Param's default value.
`_mask_password_values(params, dagrun_conf)` only registers values from
`dagrun_conf`, while this log statement also renders `params`, which may still
contain an unregistered password default.
Example:
if `api_token` defaults to `"default-secret"` and `dagrun_conf` contains an
unrelated key or overrides it with another value, `"default-secret"` can be
logged before the final masking call below. Could we either register the
current password values in `params` before this log statement or avoid logging
the raw dictionaries?
Please also add a regression test asserting that the password default itself
is registered before `logger.debug()`, rather than checking only that some
`mask_secret()` call occurred first.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py:
##########
@@ -456,7 +496,7 @@ def clear_dag_runs(
)
)
return DAGRunCollectionResponse(
- dag_runs=cleared_runs,
+ dag_runs=_build_masked_dag_run_responses(cleared_runs, dag_bag,
session),
Review Comment:
The single-run clear endpoint still returns `perform_clear_dag_run()`
directly, so a non-dry-run response may expose the raw password value in
`conf`. Could we apply the same masking there and add a regression test?
##########
task-sdk/src/airflow/sdk/definitions/param.py:
##########
@@ -382,6 +383,27 @@ def process_params(
if task.params:
params.update(task.params)
if conf.getboolean("core", "dag_run_conf_overrides_params") and
dagrun_conf:
Review Comment:
When `dag_run_conf_overrides_params=False`, this branch is skipped, so
password values in `dagrun_conf` are never registered with `mask_secret()` and
may leak through logs or rendered templates. Could we mask `dagrun_conf`
independently of the override setting and add a regression test?
--
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]
