You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2026/08/01 02:39:37 UTC
[PR] Mask nested entities when writing bulk audit-log entries [airflow]
potiuk opened a new pull request, #70890:
URL: https://github.com/apache/airflow/pull/70890
The audit-log maskers for Variables and Connections dispatch on top-level key
names -- `val`/`value` for variables, `extra` for connections. A bulk request
body has exactly one top-level key, `actions`, and the entities sit two levels
down in `actions[].entities[]`, so neither masker ever saw a key it recognised
and the payload was recorded as supplied. The single-entity endpoints mask the
same fields correctly; only the bulk paths diverged.
### Approach
`_mask_bulk_entities` walks `actions[].entities[]` and applies the existing
per-entity masking to each entry, so the two shapes now agree. The per-entity
logic is unchanged and simply factored out (`_mask_connection_entity`,
`_mask_variable_entity`) so both call sites share it.
- A `delete` action may list bare id/key strings rather than entity objects;
those carry nothing to mask and are passed through.
- The helper returns `None` for a non-bulk body so the flat path is untouched.
- It runs on the request body *before* validation, so malformed shapes
(`actions` not a list, missing `entities`, non-dict actions) must not raise --
each is covered by a test.
Worth noting for connections: a nested `password` was already covered, but only
while `hide_sensitive_var_conn_fields` is enabled, since that is key-name
redaction. `extra` was never covered by it -- the name is not a recognised
sensitive field and the value is a JSON *string*, which `redact` returns
unchanged. Masking `extra` here is structural and so does not depend on that
setting.
### Test plan
- [x] `TestMaskBulkFields` -- bulk variables, bulk connection `extra`, multiple
actions/entities, delete-by-key, and five malformed shapes
- [x] Verified against unmodified code: 5 of the new tests **fail**, and the
guard tests correctly pass either way
- [x] `test_decorators.py` -- 30 passed
- [x] `test_variables.py` + `test_connections.py` -- 230 passed
- [x] `ruff check` / `ruff format` clean
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Opus 5 (1M context)
Generated-by: Claude Opus 5 (1M context) following the guidelines at
https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mask nested entities when writing bulk audit-log entries [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mask nested entities when writing bulk audit-log entries [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mask nested entities when writing bulk audit-log entries [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #70890:
URL: https://github.com/apache/airflow/pull/70890#issuecomment-5172959114
### Backport failed to create: v3-3-test. View the failure log <a href='https://github.com/apache/airflow/actions/runs/30863620783'> Run details </a>
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>❌</td>
<td>v3-3-test</td>
<td><a href="https://github.com/apache/airflow/commit/fc8d6d8b6194674f4043fb4f632e7700876c0fd7"><img src='https://img.shields.io/badge/Commit-fc8d6d8-red' alt='Commit Link'></a></td>
</tr>
</table>
You can attempt to backport this manually by running:
```bash
cherry_picker fc8d6d8 v3-3-test
```
This should apply the commit to the v3-3-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.
After you have resolved the conflicts, you can continue the backport process by running:
```bash
cherry_picker --continue
```
If you don't have cherry-picker installed, see the [installation guide](https://github.com/apache/airflow/blob/main/dev/README.md#how-to-backport-pr-with-cherry-picker-cli).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mask nested entities when writing bulk audit-log entries [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #70890:
URL: https://github.com/apache/airflow/pull/70890
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org