SameerMesiah97 commented on code in PR #63161:
URL: https://github.com/apache/airflow/pull/63161#discussion_r2902740055
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##########
@@ -60,8 +60,7 @@ def redact_extra(cls, v: str | None) -> str | None:
redacted_dict = redact(extra_dict)
return json.dumps(redacted_dict)
except json.JSONDecodeError:
- # we can't redact fields in an unstructured `extra`
- return v
+ return "***"
Review Comment:
Just one tiny edge case: what about situations where the extra payload is
`""`? Won't this mislead the user into thinking that an extra is present in
your current implementation? Maybe, you could do something like after line 57:
```
if not v:
return v
```
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -197,6 +197,29 @@ def
test_get_should_not_overmask_short_password_value_in_extra(self, test_client
assert body["login"] == "a"
assert body["extra"] == '{"key": "value"}'
+ @pytest.mark.enable_redact
+ @pytest.mark.parametrize(
+ "non_json_extra",
+ [
+ "Bearer eyJhbGciOiJIUzI1NiJ9.secretpayload",
+ "plain-text-password",
+ "key=value&secret=token",
+ "",
Review Comment:
I think you should add another case for `None` to this test.
--
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]
