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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
