kaxil commented on code in PR #73797:
URL: https://github.com/apache/airflow/pull/73797#discussion_r4262244842
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -54,24 +54,33 @@ def to_dict(self) -> dict[str, Any]: ...
"access_token",
"api_key",
"apikey",
+ "app_key",
"auth_header",
"authorization",
"bearer",
+ "certificate",
"connection_string",
+ "credential",
Review Comment:
These are substring matches with no scoping by connection type, so some
catch fields that aren't secrets. `kube_config` matches `kube_config_path` (a
plain string in the kubernetes provider.yaml), and `credential` matches
google's `credential_config_file`. When a key matches, `add_mask` adds the
value to the log patterns, so a kubeconfig path turns into `***` in task logs.
And users can't take a name back out through `sensitive_var_conn_names`.
I'd narrow these to exact field names. Using the `format: password` that
provider.yaml already declares per field would get there too, and would also
catch apprise `config`.
##########
airflow-core/tests/unit/always/test_providers_manager.py:
##########
@@ -263,6 +263,42 @@ def test_connection_form_widgets(self,
yaml_ui_metadata_counts):
# widgets loaded from YAML metadata only
assert len(connections_form_widgets) == yaml_widgets
+ def test_sensitive_connection_fields_are_redactable(self):
+ """
+ Every connection field a provider declares as sensitive must be in the
redaction allowlist.
+
+ Providers already state which fields are secret; this asserts
``DEFAULT_SENSITIVE_FIELDS``
+ never drifts apart from them.
+ """
+ pytest.importorskip("flask_appbuilder", reason="connection form
widgets need flask_appbuilder")
+
+ from airflow._shared.secrets_masker import DEFAULT_SENSITIVE_FIELDS
+ from airflow._shared.secrets_masker.secrets_masker import SecretsMasker
+
+ # Check the shipped defaults rather than the process-wide masker, whose
+ # sensitive_variables_fields is only populated once settings have been
configured.
+ masker = SecretsMasker()
+ masker.sensitive_variables_fields = list(DEFAULT_SENSITIVE_FIELDS)
+
+ # "config" is too generic a field name to add to the allowlist;
apprise's
+ # "config" field is handled separately.
Review Comment:
I couldn't find where apprise `config` is handled.
`should_hide_value_for_key("config")` is False, so the connections API returns
it unredacted inside `extra`, and the apprise docs put a webhook URL in it.
Could the comment say this is a known unredacted secret, so the exemption
doesn't read as already covered?
##########
airflow-core/tests/unit/always/test_providers_manager.py:
##########
@@ -263,6 +263,42 @@ def test_connection_form_widgets(self,
yaml_ui_metadata_counts):
# widgets loaded from YAML metadata only
assert len(connections_form_widgets) == yaml_widgets
+ def test_sensitive_connection_fields_are_redactable(self):
Review Comment:
`connection_form_widgets` goes through `_import_info_from_all_hooks()`, and
the comment on the test above says whichever test hits that first can exceed
the default 60s. Should this one get `@pytest.mark.execution_timeout(120)` as
well, since xdist ordering can make it first on its worker?
##########
airflow-core/src/airflow/providers_manager.py:
##########
@@ -270,6 +270,21 @@ class ConnectionFormWidgetInfo(NamedTuple):
is_sensitive: bool
+def _is_sensitive_form_field(field: Any) -> bool:
Review Comment:
I don't think this changes anything. `field` is an unbound field, so
`field.field_class.widget` is the class-level default widget, not the `widget=`
kwarg the provider passed. For any `PasswordField` that is `PasswordInput` with
`input_type == "password"`, so the asb `credential` was already
`is_sensitive=True` before this PR. The `__mro__` branch only matters for a
subclass, and subclasses are rejected earlier by the `widget.field_class not in
allowed_field_classes` check. The docstring is off too:
`StringField(widget=PasswordInput())` still comes out non-sensitive. Could we
keep the original one-line expression, and drop the "regardless of the widget"
sentence from the description?
##########
airflow-core/tests/unit/always/test_providers_manager.py:
##########
@@ -263,6 +263,42 @@ def test_connection_form_widgets(self,
yaml_ui_metadata_counts):
# widgets loaded from YAML metadata only
assert len(connections_form_widgets) == yaml_widgets
+ def test_sensitive_connection_fields_are_redactable(self):
+ """
+ Every connection field a provider declares as sensitive must be in the
redaction allowlist.
+
+ Providers already state which fields are secret; this asserts
``DEFAULT_SENSITIVE_FIELDS``
+ never drifts apart from them.
+ """
+ pytest.importorskip("flask_appbuilder", reason="connection form
widgets need flask_appbuilder")
+
+ from airflow._shared.secrets_masker import DEFAULT_SENSITIVE_FIELDS
Review Comment:
Nit: both are exported from `airflow._shared.secrets_masker`, so these can
be one import at the top of the file.
--
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]
