amoghrajesh commented on code in PR #70890:
URL: https://github.com/apache/airflow/pull/70890#discussion_r3701952385
##########
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:
This passes an unrecognized bulk shape through unmasked with no warning,
maybe log a warning here instead of silently passing it through untouched.
##########
airflow-core/src/airflow/api_fastapi/logging/decorators.py:
##########
Review Comment:
`json.loads(v)` for the extra field only catches JsonDecodeError but
`json.loads()` can raise TypeError, not JSONDecodeError, when given a
non-string value (e.g. 123, {"already": "a dict"}).
Since this masking function runs on the raw, pre validation request body -
explicitly to tolerate malformed input, and "extra" set to anything other than
a string would raise an uncaught `TypeError` here.
Worth a fix while this is open.
##########
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:
Remove this since already mentioned in the code helper.
```suggestion
```
--
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]
