kaxil commented on code in PR #76012:
URL: https://github.com/apache/airflow/pull/76012#discussion_r4437477202
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/variables.py:
##########
@@ -40,6 +42,24 @@
from airflow.models.variable import Variable
+def _restore_redacted_value(new_value: str, old_value: str | None, key: str)
-> str:
+ """
+ Put the stored secret back wherever ``new_value`` still holds the mask
``VariableResponse`` sent out.
+
+ The UI edit form is filled from the redacted response, so saving it
unchanged would otherwise
+ write ``***`` over every sensitive field.
+ """
+ if old_value is None:
+ return new_value
+ try:
+ new_obj, old_obj = json.loads(new_value), json.loads(old_value)
+ except json.JSONDecodeError:
+ return merge(new_value, old_value, key)
+ merged = merge(new_obj, old_obj, key)
+ # Keep the caller's formatting when there was nothing to restore.
+ return new_value if merged == new_obj else json.dumps(merged)
Review Comment:
When a secret was restored this stores compact JSON, while the UI form sends
2-space JSON and `VariableBody` / `Variable.set(serialize_json=True)` write
`indent=2`. So the stored text changes layout depending on whether anything was
restored. `json.dumps(merged, indent=2)` would keep it consistent.
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/variables.py:
##########
@@ -40,6 +42,24 @@
from airflow.models.variable import Variable
+def _restore_redacted_value(new_value: str, old_value: str | None, key: str)
-> str:
+ """
+ Put the stored secret back wherever ``new_value`` still holds the mask
``VariableResponse`` sent out.
+
+ The UI edit form is filled from the redacted response, so saving it
unchanged would otherwise
+ write ``***`` over every sensitive field.
+ """
+ if old_value is None:
+ return new_value
+ try:
+ new_obj, old_obj = json.loads(new_value), json.loads(old_value)
+ except json.JSONDecodeError:
+ return merge(new_value, old_value, key)
+ merged = merge(new_obj, old_obj, key)
Review Comment:
`merge()` still stops at `MAX_RECURSION_DEPTH` (5), but `redact()` has
walked to any depth since #73793, so a deep enough secret is still saved as
`***`. I ran `{"clusters": [{"nodes": [{"conn": {"auth": {"password":
"s"}}}]}]}` and `{"a": {"b": {"c": {"d": {"e": {"f": {"token": "s"}}}}}}}`
through redact then merge and both come back masked. Could `_merge` walk
containers the way `_redact` does now? That would also cover the Connections
extra merge, which has the same limit.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_variables.py:
##########
@@ -622,6 +622,53 @@ def test_patch_variable_rejects_blank_key(self,
test_client, session, key):
assert response.status_code == 422
assert Variable.get(key, session=session) == "old value"
+ @pytest.mark.enable_redact
+ @pytest.mark.parametrize(
+ ("key", "stored_value"),
+ [
+ (TEST_VARIABLE_KEY2, TEST_VARIABLE_VALUE2),
+ (TEST_VARIABLE_KEY3, TEST_VARIABLE_VALUE3),
+ (TEST_VARIABLE_KEY5, TEST_VARIABLE_VALUE5),
+ (TEST_VARIABLE_KEY6, TEST_VARIABLE_VALUE6),
+ ],
+ ids=["sensitive-key", "sensitive-json-field",
"nested-sensitive-json-fields", "sensitive-key-json"],
+ )
+ def test_patch_with_redacted_value_keeps_stored_secrets(self, test_client,
key, stored_value):
Review Comment:
The edit form only sends `value` when it changed, and then always with
`update_mask=["value"]` (`useEditVariable.ts`), so that's the request that
actually hits this bug. None of the new tests pass a mask, so only the `not
update_mask` half of the condition is covered, and changing it to `and not
update_mask` would keep them all green. Could these run with
`params={"update_mask": ["value"]}` as well? Also, an unchanged form doesn't
send `value` at all, so "saving the edit form unchanged" in the docstring isn't
quite the scenario.
--
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]
