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:42 UTC
[PR] Mask list-shaped Variable values on deserialization [airflow]
potiuk opened a new pull request, #70891:
URL: https://github.com/apache/airflow/pull/70891
`_mask_and_deserialize_variable` dispatched on the top-level type of the
deserialized value and handled only `str` and `dict`. A Variable whose JSON is a
list therefore matched no branch and was returned with nothing inside it masked.
`add_mask` walks dicts and iterables itself, so the same list nested one level
inside a dict was already masked -- the top-level dispatch was the only thing
deciding whether the nested values were reached at all.
### Approach
Add the `list` branch, passing the value **under the variable's key** rather
than anonymously.
That distinction is the whole design of the change. Elements of a list have no
key names of their own, so they follow the variable key's sensitivity -- the same
rule already applied to a plain string value. Passing the list anonymously would
mask every element unconditionally and add each to the global pattern set, so a
Variable holding `["us-east-1", "eu-west-1"]` would cause `us-east-1` to be
redacted everywhere it appeared in any log. `test_var_json_list_value_does_not_over_mask`
already guarded that, and this keeps it true. Dicts *inside* the list are still
masked by their own key names.
Verified against the real masker:
| variable | key | result |
|---|---|---|
| `[{"password": "…"}]` | `db_configs` | `[{"password": "***"}]` |
| `["us-east-1", "eu-west-1"]` | `aws_regions` | unchanged |
| `["…"]` | `my_password` | `***` |
| `{"password": "…"}` | `db_config` | `{"password": "***"}` (unchanged path) |
`test_var_json_list_value_does_not_over_mask` is updated: it asserted
`mask_secret` was called exactly once, and there are now two calls. The property
it guards is unchanged and the assertion is now stated directly -- the list is
passed under the variable's key, and never anonymously.
### Test plan
- [x] `test_var_json_masks_list_values` -- list handed to the masker under the
variable's key
- [x] `test_var_json_list_value_does_not_over_mask` -- updated to assert the
no-over-masking property rather than the call count
- [x] `test_var_json_scalar_values_pass_through` -- int/bool/null/float returned
unchanged
- [x] Verified against unmodified code: both list tests **fail**
- [x] `test_context.py` -- 170 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 list-shaped Variable values on deserialization [airflow]
Posted by "SameerMesiah97 (via GitHub)" <gi...@apache.org>.
SameerMesiah97 commented on code in PR #70891:
URL: https://github.com/apache/airflow/pull/70891#discussion_r3696967650
##########
task-sdk/src/airflow/sdk/execution_time/context.py:
##########
@@ -323,9 +323,22 @@ def _mask_and_deserialize_variable(raw: str, key: str, deserialize_json: bool) -
return raw
val = json.loads(raw)
if isinstance(val, str):
+ # No key names of its own, so the variable's key decides whether it is masked.
mask_secret(val, key)
elif isinstance(val, dict):
+ # ``add_mask`` walks a dict per key and masks by the *inner* key name, ignoring the
+ # name passed here.
mask_secret(val)
+ elif isinstance(val, list):
+ # A list was previously skipped entirely, even though ``add_mask`` walks iterables --
+ # the same list nested one level inside a dict was masked, only a top-level one was not.
+ #
+ # It is passed under the variable's key, not anonymously: a list has no key names of
+ # its own, so bare values inside follow the same rule as the plain-string case above.
+ # Masking them unconditionally would add every element to the global pattern set, so a
+ # list of ordinary values such as region names would be redacted everywhere it appeared.
+ # Dicts *inside* the list are still masked by their own key names.
Review Comment:
I had to read this a few times to understand what it was trying to convey. I think the comment can be more concise and clear. Please see below:
```
# Pass the Variable's key so list elements inherit the Variable's sensitivity
# instead of being added to the global mask patterns.
```
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -428,6 +428,41 @@ def test_var_json_masks_raw_string_and_dict_values(self, mock_mask_secret, mock_
# Second call: deserialized dict so internal sensitive fields like "password" get masked
mock_mask_secret.assert_any_call({"password": "s3cr3t", "host": "db.example.com"})
+ @mock.patch("airflow.sdk.execution_time.context.mask_secret")
+ def test_var_json_masks_list_values(self, mock_mask_secret, mock_supervisor_comms):
+ """A JSON list is handed to the masker whole, exactly as a dict is.
+
Review Comment:
I would keep the first line for the docstring.
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -456,15 +491,25 @@ def test_var_json_string_value_masks_both_forms(self, mock_mask_secret, mock_sup
@mock.patch("airflow.sdk.execution_time.context.mask_secret")
def test_var_json_list_value_does_not_over_mask(self, mock_mask_secret, mock_supervisor_comms):
- """var.json with a non-sensitive list variable does not mask individual list elements."""
+ """var.json with a non-sensitive list variable does not mask individual list elements.
+
+ The list is handed to the masker **under the variable's key**, which is what preserves
+ this: bare values inside a list have no key names of their own, so they follow the
+ variable key's sensitivity. Passing the list anonymously instead would add every element
+ to the global pattern set, and an ordinary value such as a region name would then be
+ redacted everywhere it appeared in the logs.
Review Comment:
Same here. The first line of the docstring can be kept and extra context can be retained in the comments.
--
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 list-shaped Variable values on deserialization [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #70891:
URL: https://github.com/apache/airflow/pull/70891#discussion_r3701976429
##########
task-sdk/src/airflow/sdk/execution_time/context.py:
##########
@@ -323,9 +323,22 @@ def _mask_and_deserialize_variable(raw: str, key: str, deserialize_json: bool) -
return raw
val = json.loads(raw)
if isinstance(val, str):
+ # No key names of its own, so the variable's key decides whether it is masked.
mask_secret(val, key)
elif isinstance(val, dict):
+ # ``add_mask`` walks a dict per key and masks by the *inner* key name, ignoring the
+ # name passed here.
mask_secret(val)
+ elif isinstance(val, list):
+ # A list was previously skipped entirely, even though ``add_mask`` walks iterables --
+ # the same list nested one level inside a dict was masked, only a top-level one was not.
+ #
+ # It is passed under the variable's key, not anonymously: a list has no key names of
+ # its own, so bare values inside follow the same rule as the plain-string case above.
+ # Masking them unconditionally would add every element to the global pattern set, so a
+ # list of ordinary values such as region names would be redacted everywhere it appeared.
+ # Dicts *inside* the list are still masked by their own key names.
Review Comment:
+1 agreed.
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -428,6 +428,41 @@ def test_var_json_masks_raw_string_and_dict_values(self, mock_mask_secret, mock_
# Second call: deserialized dict so internal sensitive fields like "password" get masked
mock_mask_secret.assert_any_call({"password": "s3cr3t", "host": "db.example.com"})
+ @mock.patch("airflow.sdk.execution_time.context.mask_secret")
+ def test_var_json_masks_list_values(self, mock_mask_secret, mock_supervisor_comms):
+ """A JSON list is handed to the masker whole, exactly as a dict is.
+
+ ``add_mask`` walks dicts and iterables itself, so the top-level type dispatch was the
+ only thing deciding whether nested values were reached: a list at the top level matched
+ no branch and was skipped, while the same list nested inside a dict was masked.
+ """
Review Comment:
```suggestion
```
Since its already there in the code path.
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -456,15 +491,25 @@ def test_var_json_string_value_masks_both_forms(self, mock_mask_secret, mock_sup
@mock.patch("airflow.sdk.execution_time.context.mask_secret")
def test_var_json_list_value_does_not_over_mask(self, mock_mask_secret, mock_supervisor_comms):
- """var.json with a non-sensitive list variable does not mask individual list elements."""
+ """var.json with a non-sensitive list variable does not mask individual list elements.
+
+ The list is handed to the masker **under the variable's key**, which is what preserves
+ this: bare values inside a list have no key names of their own, so they follow the
+ variable key's sensitivity. Passing the list anonymously instead would add every element
+ to the global pattern set, and an ordinary value such as a region name would then be
+ redacted everywhere it appeared in the logs.
Review Comment:
Agree with @SameerMesiah97, either that or remove docstring too.
##########
task-sdk/src/airflow/sdk/execution_time/context.py:
##########
@@ -323,9 +323,22 @@ def _mask_and_deserialize_variable(raw: str, key: str, deserialize_json: bool) -
return raw
val = json.loads(raw)
if isinstance(val, str):
+ # No key names of its own, so the variable's key decides whether it is masked.
mask_secret(val, key)
elif isinstance(val, dict):
+ # ``add_mask`` walks a dict per key and masks by the *inner* key name, ignoring the
+ # name passed here.
mask_secret(val)
+ elif isinstance(val, list):
Review Comment:
In dict branch, we dont pass key. Inside `add_mask` dict is handled this way:
```python
if isinstance(secret, dict):
for k, v in secret.items():
self.add_mask(v, k)
```
Therefore,
- `mask_secret({"password": "x"})` -> calls `add_mask("x", "password")`
- `mask_secret({"password": "x"}, "some_key")` -> calls `add_mask("x", "password")`
So, you could just do this to simplify it:
```python
if isinstance(val, (str, dict, list)):
mask_secret(val, key)
```
--
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 list-shaped Variable values on deserialization [airflow]
Posted by "vatsrahul1001 (via GitHub)" <gi...@apache.org>.
vatsrahul1001 merged PR #70891:
URL: https://github.com/apache/airflow/pull/70891
--
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 list-shaped Variable values on deserialization [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #70891:
URL: https://github.com/apache/airflow/pull/70891#issuecomment-5176566416
### Backport successfully created: v3-3-test
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/pull/71069"><img src="https://img.shields.io/badge/PR-71069-blue" alt="PR Link"></a></td>
</tr>
</table>
--
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 list-shaped Variable values on deserialization [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #70891:
URL: https://github.com/apache/airflow/pull/70891#discussion_r3708321625
##########
task-sdk/src/airflow/sdk/execution_time/context.py:
##########
@@ -323,9 +323,22 @@ def _mask_and_deserialize_variable(raw: str, key: str, deserialize_json: bool) -
return raw
val = json.loads(raw)
if isinstance(val, str):
+ # No key names of its own, so the variable's key decides whether it is masked.
mask_secret(val, key)
elif isinstance(val, dict):
+ # ``add_mask`` walks a dict per key and masks by the *inner* key name, ignoring the
+ # name passed here.
mask_secret(val)
+ elif isinstance(val, list):
+ # A list was previously skipped entirely, even though ``add_mask`` walks iterables --
+ # the same list nested one level inside a dict was masked, only a top-level one was not.
+ #
+ # It is passed under the variable's key, not anonymously: a list has no key names of
+ # its own, so bare values inside follow the same rule as the plain-string case above.
+ # Masking them unconditionally would add every element to the global pattern set, so a
+ # list of ordinary values such as region names would be redacted everywhere it appeared.
+ # Dicts *inside* the list are still masked by their own key names.
Review Comment:
Taken, with your wording verbatim. The long version was trying to justify the choice and the surrounding history at the same time; yours says the thing that actually matters at the point of decision.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -428,6 +428,41 @@ def test_var_json_masks_raw_string_and_dict_values(self, mock_mask_secret, mock_
# Second call: deserialized dict so internal sensitive fields like "password" get masked
mock_mask_secret.assert_any_call({"password": "s3cr3t", "host": "db.example.com"})
+ @mock.patch("airflow.sdk.execution_time.context.mask_secret")
+ def test_var_json_masks_list_values(self, mock_mask_secret, mock_supervisor_comms):
+ """A JSON list is handed to the masker whole, exactly as a dict is.
+
Review Comment:
Done — first line kept, the rest dropped. The detail it carried is now in the one comment at the dispatch branch rather than repeated in each test.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -456,15 +491,25 @@ def test_var_json_string_value_masks_both_forms(self, mock_mask_secret, mock_sup
@mock.patch("airflow.sdk.execution_time.context.mask_secret")
def test_var_json_list_value_does_not_over_mask(self, mock_mask_secret, mock_supervisor_comms):
- """var.json with a non-sensitive list variable does not mask individual list elements."""
+ """var.json with a non-sensitive list variable does not mask individual list elements.
+
+ The list is handed to the masker **under the variable's key**, which is what preserves
+ this: bare values inside a list have no key names of their own, so they follow the
+ variable key's sensitivity. Passing the list anonymously instead would add every element
+ to the global pattern set, and an ordinary value such as a region name would then be
+ redacted everywhere it appeared in the logs.
Review Comment:
Done — first line kept, the rest dropped. The detail it carried is now in the one comment at the dispatch branch rather than repeated in each test.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -428,6 +428,41 @@ def test_var_json_masks_raw_string_and_dict_values(self, mock_mask_secret, mock_
# Second call: deserialized dict so internal sensitive fields like "password" get masked
mock_mask_secret.assert_any_call({"password": "s3cr3t", "host": "db.example.com"})
+ @mock.patch("airflow.sdk.execution_time.context.mask_secret")
+ def test_var_json_masks_list_values(self, mock_mask_secret, mock_supervisor_comms):
+ """A JSON list is handed to the masker whole, exactly as a dict is.
+
+ ``add_mask`` walks dicts and iterables itself, so the top-level type dispatch was the
+ only thing deciding whether nested values were reached: a list at the top level matched
+ no branch and was skipped, while the same list nested inside a dict was masked.
+ """
Review Comment:
Done — first line kept, the rest dropped. The detail it carried is now in the one comment at the dispatch branch rather than repeated in each test.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
task-sdk/tests/task_sdk/execution_time/test_context.py:
##########
@@ -456,15 +491,25 @@ def test_var_json_string_value_masks_both_forms(self, mock_mask_secret, mock_sup
@mock.patch("airflow.sdk.execution_time.context.mask_secret")
def test_var_json_list_value_does_not_over_mask(self, mock_mask_secret, mock_supervisor_comms):
- """var.json with a non-sensitive list variable does not mask individual list elements."""
+ """var.json with a non-sensitive list variable does not mask individual list elements.
+
+ The list is handed to the masker **under the variable's key**, which is what preserves
+ this: bare values inside a list have no key names of their own, so they follow the
+ variable key's sensitivity. Passing the list anonymously instead would add every element
+ to the global pattern set, and an ordinary value such as a region name would then be
+ redacted everywhere it appeared in the logs.
Review Comment:
Done — first line kept, the rest dropped. The detail it carried is now in the one comment at the dispatch branch rather than repeated in each test.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
task-sdk/src/airflow/sdk/execution_time/context.py:
##########
@@ -323,9 +323,22 @@ def _mask_and_deserialize_variable(raw: str, key: str, deserialize_json: bool) -
return raw
val = json.loads(raw)
if isinstance(val, str):
+ # No key names of its own, so the variable's key decides whether it is masked.
mask_secret(val, key)
elif isinstance(val, dict):
+ # ``add_mask`` walks a dict per key and masks by the *inner* key name, ignoring the
+ # name passed here.
mask_secret(val)
+ elif isinstance(val, list):
Review Comment:
Your reading of `add_mask` is right — for a dict it re-enters per inner key and the `name` it was handed is never used, so `mask_secret(val, key)` and `mask_secret(val)` are indistinguishable for a dict today. I checked the source rather than taking it on trust.
Still not merging the branches, for two reasons.
The equivalence is incidental, not designed. `key` would be passed for the dict case and then silently ignored, so the code would read as though the Variable's key participates in masking a dict when it does not. If `add_mask` ever did use the outer name for dicts — masking the whole dict when the Variable key is sensitive would be a reasonable thing to want — the merged form changes what gets masked, with nothing at this call site to signal it. The separate branches say what each type actually needs, which is different things.
The second reason is scope. This is milestoned for 3.3.1. The dict branch is pre-existing behaviour and the bug is narrower than it looks: a top-level list matched no branch at all and was skipped entirely, while the same list nested one level inside a dict was masked. Rewriting the dict line as well widens what a backport touches for no behavioural gain.
The comments are trimmed per the other threads, which is what made the dispatch look bulkier than it is — it is now three short branches.
---
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 list-shaped Variable values on deserialization [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #70891:
URL: https://github.com/apache/airflow/pull/70891#issuecomment-5173304205
All threads addressed and resolved. Comments trimmed as suggested.
I did not take the branch collapse — the equivalence relies on `add_mask` ignoring the `name` it is handed for a dict, so the key would be passed and silently ignored, and the dict branch is pre-existing behaviour I would rather not rewrite inside a 3.3.1 backport. Reasoning is in that thread if you disagree.
@amoghrajesh @SameerMesiah97
---
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