#2576 oncall. SAT: test spec against exposed secrets

airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog
 
+## 0.2.17
+Test connector specification against exposed secret fields. [#19124](https://github.com/airbytehq/airbyte/pull/19124).
+
 ## 0.2.16
 Run `basic_read` on the discovered catalog in `high` `test_strictness_level`. [#18937](https://github.com/airbytehq/airbyte/pull/18937).
 

airbyte-integrations/bases/source-acceptance-test/Dockerfile

@@ -33,7 +33,7 @@ COPY pytest.ini setup.py ./
 COPY source_acceptance_test ./source_acceptance_test
 RUN pip install .
 
-LABEL io.airbyte.version=0.2.16
+LABEL io.airbyte.version=0.2.17
 LABEL io.airbyte.name=airbyte/source-acceptance-test
 
 ENTRYPOINT ["python", "-m", "pytest", "-p", "source_acceptance_test.plugin", "-r", "fEsx"]

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py

@@ -8,7 +8,7 @@
 from collections import Counter, defaultdict
 from functools import reduce
 from logging import Logger
-from typing import Any, Dict, List, Mapping, MutableMapping, Optional, Set
+from typing import Any, Dict, List, Mapping, MutableMapping, Optional, Set, Tuple
 from xmlrpc.client import Boolean
 
 import dpath.util
@@ -150,6 +150,84 @@ def test_has_secret(self):
     def test_secret_never_in_the_output(self):
         """This test should be injected into any docker command it needs to know current config and spec"""
 
+    def test_secret_is_properly_marked(self, actual_connector_spec: ConnectorSpecification, detailed_logger):
+        def is_property_name_secret(path: str) -> Tuple[str, bool]:
+            reserved_keywords = ("anyOf", "oneOf", "allOf", "not", "properties", "items")
+            name = None
+            for part in reversed(path.split("/")[:-1]):
+                if part.isdigit() or part in reserved_keywords:
+                    continue
+                name = part
+                break
+            return name, name.lower() in (
+                "client_token",
+                "access_token",
+                "api_token",
+                "token",
+                "secret",
+                "client_secret",
+                "password",
+                "key",
+                "service_account_info",
+                "service_account",
+                "tenant_id",
+                "certificate",
+                "jwt",
+                "credentials",
+                "app_id",
+                "appid",
+                "refresh_token",
+            )
+
+        def can_hide_a_secret(prop: dict) -> bool:
+            # Null type as well as boolean can not hold a secret value.
+            # A string, a number or an integer type can always hide secrets.
+            # Objects and arrays can hide a secret in case they are generic.
+            unsecure_types = {"string", "integer", "number"}
+            type_ = prop["type"]
+            # type_ is a scalar value
+            return any(
+                [
+                    isinstance(type_, str) and type_ in unsecure_types,
+                    type_ == "object" and "properties" not in prop,
+                    type_ == "array" and "items" not in prop,
+                    isinstance(type_, list) and (set(type_) & unsecure_types),
+                ]
+            )
+
+        def is_marked_secret(prop: dict):
+            return prop.get("airbyte_secret", False)
+
+        secrets_exposed = []
+        non_secrets_hidden = []
+        spec_properties = actual_connector_spec.connectionSpecification["properties"]
+        for type_path, value in dpath.util.search(spec_properties, "**/type", yielded=True):
+            name, is_secret_name = is_property_name_secret(type_path)
+            if not is_secret_name:
+                continue
+            absolute_path = f"/{type_path}"
+            property_path, _ = absolute_path.rsplit(sep="/", maxsplit=1)
+            property_definition = dpath.util.get(spec_properties, property_path)
+            marked_as_secret, possibly_a_secret = is_marked_secret(property_definition), can_hide_a_secret(property_definition)
+            if marked_as_secret and not possibly_a_secret:
+                non_secrets_hidden.append(property_path)
+            if not marked_as_secret and possibly_a_secret:
+                secrets_exposed.append(property_path)
+
+        if non_secrets_hidden:
+            properties = "\n".join(non_secrets_hidden)
+            detailed_logger.warning(
+                f"""Some properties are marked with `airbyte_secret` although they probably should not be.
+                Please double check them. If they're okay, please fix this test.
+                {properties}"""
+            )
+        if secrets_exposed:
+            properties = "\n".join(secrets_exposed)
+            pytest.fail(
+                f"""The following properties should be marked with `airbyte_secret!`
+                    {properties}"""
+            )
+
     def test_defined_refs_exist_in_json_spec_file(self, connector_spec_dict: dict):
         """Checking for the presence of unresolved `$ref`s values within each json spec file"""
         check_result = list(find_all_values_for_key_in_schema(connector_spec_dict, "$ref"))

airbyte-integrations/bases/source-acceptance-test/unit_tests/test_spec.py

@@ -6,6 +6,7 @@
 
 import pytest
 from airbyte_cdk.models import ConnectorSpecification
+from source_acceptance_test import conftest
 from source_acceptance_test.tests.test_core import TestSpec as _TestSpec
 
 from .conftest import does_not_raise
@@ -605,3 +606,55 @@ def test_additional_properties_is_true(connector_spec, expectation):
     t = _TestSpec()
     with expectation:
         t.test_additional_properties_is_true(connector_spec)
+
+
+@pytest.mark.parametrize(
+    "connector_spec, should_fail",
+    (
+        (
+            ConnectorSpecification(
+                connectionSpecification={"type": "object", "properties": {"api_token": {"type": "string", "airbyte_secret": True}}}
+            ),
+            False,
+        ),
+        (
+            ConnectorSpecification(connectionSpecification={"type": "object", "properties": {"api_token": {"type": "null"}}}),
+            False,
+        ),
+        (
+            ConnectorSpecification(connectionSpecification={"type": "object", "properties": {"jwt": {"type": "object"}}}),
+            True,
+        ),
+        (
+            ConnectorSpecification(
+                connectionSpecification={"type": "object", "properties": {"refresh_token": {"type": ["null", "string"]}}}
+            ),
+            True,
+        ),
+        (
+            ConnectorSpecification(connectionSpecification={"type": "object", "properties": {"credentials": {"type": "array"}}}),
+            True,
+        ),
+        (
+            ConnectorSpecification(
+                connectionSpecification={"type": "object", "properties": {"credentials": {"type": "array", "items": {"type": "string"}}}}
+            ),
+            True,
+        ),
+        (
+            ConnectorSpecification(
+                connectionSpecification={"type": "object", "properties": {"auth": {"oneOf": [{"api_token": {"type": "string"}}]}}}
+            ),
+            True,
+        ),
+    ),
+)
+def test_airbyte_secret(mocker, connector_spec, should_fail):
+    mocker.patch.object(conftest.pytest, "fail")
+    t = _TestSpec()
+    logger = mocker.Mock()
+    t.test_secret_is_properly_marked(connector_spec, logger)
+    if should_fail:
+        conftest.pytest.fail.assert_called_once()
+    else:
+        conftest.pytest.fail.assert_not_called()