Mask list-shaped Variable values on deserialization - #70891
Conversation
SameerMesiah97
left a comment
There was a problem hiding this comment.
Looks good. Just a few stylistic nits (for e.g. I found one comment a bit hard to understand).
_mask_and_deserialize_variable dispatched on the top-level type of the deserialized value and handled only str and dict, so a Variable whose JSON is a list was returned with no masking applied to anything inside it. add_mask walks iterables itself, so the same list nested one level inside a dict was already masked -- only a top-level one was skipped. The list is passed under the variable's key rather than anonymously: elements have no key names of their own, so they follow the variable key's sensitivity, and a list of ordinary values such as region names is not added to the global pattern set. Dicts inside the list are still masked by their own key names.
The rationale was spread across three sites saying the same thing; keep it at the branch where the decision is made.
07d2d16 to
c7e188e
Compare
|
All threads addressed and resolved. Comments trimmed as suggested. I did not take the branch collapse — the equivalence relies on Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
amoghrajesh
left a comment
There was a problem hiding this comment.
Thanks, simple enough now.
Backport successfully created: v3-3-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
…he#70891) * Mask list-shaped Variable values on deserialization _mask_and_deserialize_variable dispatched on the top-level type of the deserialized value and handled only str and dict, so a Variable whose JSON is a list was returned with no masking applied to anything inside it. add_mask walks iterables itself, so the same list nested one level inside a dict was already masked -- only a top-level one was skipped. The list is passed under the variable's key rather than anonymously: elements have no key names of their own, so they follow the variable key's sensitivity, and a list of ordinary values such as region names is not added to the global pattern set. Dicts inside the list are still masked by their own key names. * Tighten the comments around Variable masking dispatch The rationale was spread across three sites saying the same thing; keep it at the branch where the decision is made. (cherry picked from commit b968192) Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
…he#70891) * Mask list-shaped Variable values on deserialization _mask_and_deserialize_variable dispatched on the top-level type of the deserialized value and handled only str and dict, so a Variable whose JSON is a list was returned with no masking applied to anything inside it. add_mask walks iterables itself, so the same list nested one level inside a dict was already masked -- only a top-level one was skipped. The list is passed under the variable's key rather than anonymously: elements have no key names of their own, so they follow the variable key's sensitivity, and a list of ordinary values such as region names is not added to the global pattern set. Dicts inside the list are still masked by their own key names. * Tighten the comments around Variable masking dispatch The rationale was spread across three sites saying the same thing; keep it at the branch where the decision is made. (cherry picked from commit b968192) Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
…) (#71069) * Mask list-shaped Variable values on deserialization _mask_and_deserialize_variable dispatched on the top-level type of the deserialized value and handled only str and dict, so a Variable whose JSON is a list was returned with no masking applied to anything inside it. add_mask walks iterables itself, so the same list nested one level inside a dict was already masked -- only a top-level one was skipped. The list is passed under the variable's key rather than anonymously: elements have no key names of their own, so they follow the variable key's sensitivity, and a list of ordinary values such as region names is not added to the global pattern set. Dicts inside the list are still masked by their own key names. * Tighten the comments around Variable masking dispatch The rationale was spread across three sites saying the same thing; keep it at the branch where the decision is made. (cherry picked from commit b968192) Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
…) (#71069) * Mask list-shaped Variable values on deserialization _mask_and_deserialize_variable dispatched on the top-level type of the deserialized value and handled only str and dict, so a Variable whose JSON is a list was returned with no masking applied to anything inside it. add_mask walks iterables itself, so the same list nested one level inside a dict was already masked -- only a top-level one was skipped. The list is passed under the variable's key rather than anonymously: elements have no key names of their own, so they follow the variable key's sensitivity, and a list of ordinary values such as region names is not added to the global pattern set. Dicts inside the list are still masked by their own key names. * Tighten the comments around Variable masking dispatch The rationale was spread across three sites saying the same thing; keep it at the branch where the decision is made. (cherry picked from commit b968192) Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
* Mask list-shaped Variable values on deserialization _mask_and_deserialize_variable dispatched on the top-level type of the deserialized value and handled only str and dict, so a Variable whose JSON is a list was returned with no masking applied to anything inside it. add_mask walks iterables itself, so the same list nested one level inside a dict was already masked -- only a top-level one was skipped. The list is passed under the variable's key rather than anonymously: elements have no key names of their own, so they follow the variable key's sensitivity, and a list of ordinary values such as region names is not added to the global pattern set. Dicts inside the list are still masked by their own key names. * Tighten the comments around Variable masking dispatch The rationale was spread across three sites saying the same thing; keep it at the branch where the decision is made.
_mask_and_deserialize_variabledispatched on the top-level type of thedeserialized value and handled only
stranddict. A Variable whose JSON is alist therefore matched no branch and was returned with nothing inside it masked.
add_maskwalks dicts and iterables itself, so the same list nested one levelinside 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
listbranch, passing the value under the variable's key ratherthan 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 causeus-east-1to beredacted everywhere it appeared in any log.
test_var_json_list_value_does_not_over_maskalready guarded that, and this keeps it true. Dicts inside the list are still
masked by their own key names.
Verified against the real masker:
[{"password": "…"}]db_configs[{"password": "***"}]["us-east-1", "eu-west-1"]aws_regions["…"]my_password***{"password": "…"}db_config{"password": "***"}(unchanged path)test_var_json_list_value_does_not_over_maskis updated: it assertedmask_secretwas called exactly once, and there are now two calls. The propertyit guards is unchanged and the assertion is now stated directly -- the list is
passed under the variable's key, and never anonymously.
Test plan
test_var_json_masks_list_values-- list handed to the masker under thevariable's key
test_var_json_list_value_does_not_over_mask-- updated to assert theno-over-masking property rather than the call count
test_var_json_scalar_values_pass_through-- int/bool/null/float returnedunchanged
test_context.py-- 170 passedruff check/ruff formatcleanWas generative AI tooling used to co-author this PR?
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