potiuk commented on code in PR #70890:
URL: https://github.com/apache/airflow/pull/70890#discussion_r3708255175
##########
airflow-core/src/airflow/api_fastapi/logging/decorators.py:
##########
Review Comment:
Good catch, and it is worth noting it is newly reachable rather than merely
pre-existing: before this change a bulk body never got as far as the `extra`
branch, because the only top-level key it presented was `actions`. Now it does.
Widened the except to `(json.JSONDecodeError, TypeError)`. Added
`test_non_string_extra_does_not_raise_and_does_not_leak`, parametrized over an
already-decoded object, an already-decoded array, a number and a bool — all
four raise `TypeError` out of the audit-log decorator without the fix, and all
four assert the secret does not survive into the serialized entry with it.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/tests/unit/api_fastapi/logging/test_decorators.py:
##########
@@ -130,6 +130,141 @@ def test_value_without_key_is_still_masked(self):
assert result == {"value": "***"}
+class TestMaskBulkFields:
+ """The bulk endpoints nest their entities, and the masking has to reach
them.
+
+ A ``BulkBody`` has exactly one top-level field, ``actions``; the entities
carrying the
+ secrets sit two levels down, in ``actions[].entities[]``. Both maskers
dispatch on
+ top-level key names, so a bulk body previously presented them with the
single key
+ ``actions`` -- neither ``val``/``value`` for variables, nor ``extra`` for
connections --
+ and the payload was written to the audit log as supplied.
+ """
Review Comment:
Right — the class docstring was restating `_mask_bulk_entities`'s docstring
almost sentence for sentence. Cut down to a single line; the mechanism stays
documented at the helper.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/api_fastapi/logging/decorators.py:
##########
@@ -45,8 +45,51 @@ def _sanitize_for_stdlib_log(value: str) -> str:
return value.replace("\r", " ").replace("\n", " ")
+def _mask_bulk_entities(extra_fields, mask_entity):
+ """
+ Apply per-entity masking to a bulk request body.
+
+ A ``BulkBody`` has exactly one top-level field, ``actions``; the entities
carrying the
+ secrets sit two levels down, in ``actions[].entities[]``. The per-entity
maskers below
+ inspect top-level key names, so handing them a bulk body means they see
only the key
+ ``actions`` and pass its whole payload through untouched. Reach the
entities first.
+
+ Returns ``None`` when the body is not bulk-shaped, so callers fall back to
flat masking.
+ """
+ actions = extra_fields.get("actions")
+ if not isinstance(actions, list):
+ return None
+
+ masked_actions = []
+ for action in actions:
+ if not isinstance(action, dict):
Review Comment:
Not adding this one, because `None` here does not mean "unrecognised shape"
— it means "not a bulk body", which is the ordinary case.
Every single-entity endpoint routed through this decorator sends a flat body
with no `actions` key: `POST /connections`, `PATCH /connections/{id}`, `POST
/variables`, `PATCH /variables/{key}` and so on. All of them take this return
path on every request, and the caller then does exactly the right thing with it
— falls back to flat masking. A warning here would fire on the majority of
audited write requests and say nothing useful.
The narrower case — a body that *is* bulk-shaped but malformed, say
`actions` present but an entity list that is not a list — is a request FastAPI
rejects with 422 immediately after this runs. Nothing is written to the audit
log for it, so there is no unmasked payload to warn about.
If the worry is a bulk shape changing under us and masking silently going
quiet, the guard I would trust is a test rather than a log line, and
`test_no_secret_survives_in_the_serialized_entry` is that test: it asserts on
the serialized entry rather than on one field, so a shape change that stops
reaching the entities fails it.
---
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]
