diff --git a/.agents/skills/baserow-registry/SKILL.md b/.agents/skills/baserow-registry/SKILL.md index 3c55ec33c8..2c48df0feb 100644 --- a/.agents/skills/baserow-registry/SKILL.md +++ b/.agents/skills/baserow-registry/SKILL.md @@ -135,10 +135,27 @@ Use `EasyImportExportMixin` for model-backed types with direct property serializ - `parent_property_name`: the parent relation to set during import. - `id_mapping_name`: optional mapping key to populate. - `model_class`: the model class to create. -- `sensitive_fields`: fields omitted when `exclude_sensitive_data` is enabled. +- `sensitive_fields`: fields omitted when `exclude_sensitive_data` is enabled. Integration and user source updates also keep these fields out of the undo action log. Neither hides a field from API responses. Override `serialize_property`, `deserialize_property`, or `create_instance_from_serialized` for file handling, ID remapping, compatibility, or custom creation. +## Integration Secrets + +Credentials on an `IntegrationType` (passwords, tokens, API keys) must be write-only. Declare them on the type: + +- `secret_fields`: fields that can be set and overwritten but are never returned by the API, the creator included. The response serializer replaces each one with a `has_` boolean. Omitting the field on update keeps the stored value. Document that contract in the field's `help_text` through `serializer_field_extra_kwargs`. +- `secret_field_dependencies`: maps a secret to the fields that decide where it is sent. Changing any of those fields requires the secret to be supplied again in the same request, otherwise the stored credential could be sent to a destination its owner never chose. +- Also list every secret in `sensitive_fields` so it stays out of exports. + +```python +class SMTPIntegrationType(IntegrationType): + sensitive_fields = ["host", "port", "use_tls", "username", "password"] + secret_fields = ["password"] + secret_field_dependencies = {"password": ["host", "port", "use_tls"]} +``` + +`DataSyncType.secret_field_dependencies` is the equivalent for data syncs. + ## API URLs And Exceptions Use `APIUrlsInstanceMixin` on instances that contribute API routes and `APIUrlsRegistryMixin` on the registry. Include `registry.api_urls` in the owning URL module. diff --git a/backend/src/baserow/api/extensions.py b/backend/src/baserow/api/extensions.py index ffa5299e98..1fc8020650 100644 --- a/backend/src/baserow/api/extensions.py +++ b/backend/src/baserow/api/extensions.py @@ -31,11 +31,13 @@ def get_name(self): def map_serializer(self, auto_schema, direction): return self._map_serializer(auto_schema, direction, self.target.mapping) - def _map_serializer(self, auto_schema, direction, mapping): + def _map_serializer(self, auto_schema, direction, mapping, partial=False): sub_components = [] for key, serializer_class in mapping.items(): sub_serializer = force_instance(serializer_class) + if partial: + sub_serializer.partial = True resolved_sub_serializer = auto_schema.resolve_serializer( sub_serializer, direction ) @@ -87,7 +89,8 @@ def map_serializer(self, auto_schema, direction): for types in self.target.registry.registry.values() } - return self._map_serializer(auto_schema, direction, mapping) + partial = getattr(self.target, "partial_request", False) + return self._map_serializer(auto_schema, direction, mapping, partial=partial) class DiscriminatorMappingSerializerExtension(OpenApiSerializerExtension): diff --git a/backend/src/baserow/api/integrations/errors.py b/backend/src/baserow/api/integrations/errors.py index fd2d4762ad..051426c258 100644 --- a/backend/src/baserow/api/integrations/errors.py +++ b/backend/src/baserow/api/integrations/errors.py @@ -11,3 +11,9 @@ HTTP_400_BAD_REQUEST, "The given integrations do not belong to the same application.", ) + +ERROR_INTEGRATION_CREDENTIAL_REQUIRED = ( + "ERROR_INTEGRATION_CREDENTIAL_REQUIRED", + HTTP_400_BAD_REQUEST, + "The credential must be provided again when the connection target changes.", +) diff --git a/backend/src/baserow/api/integrations/fields.py b/backend/src/baserow/api/integrations/fields.py new file mode 100644 index 0000000000..c36dd18d1c --- /dev/null +++ b/backend/src/baserow/api/integrations/fields.py @@ -0,0 +1,22 @@ +from rest_framework import serializers + + +class HasSecretField(serializers.BooleanField): + """ + A read-only boolean that reports whether a write-only credential is set, + without disclosing its value. Used in place of the credential itself in + integration response serializers. + """ + + def __init__(self, secret_field_name: str, **kwargs): + self.secret_field_name = secret_field_name + kwargs["read_only"] = True + kwargs.setdefault( + "help_text", + f"Whether the `{secret_field_name}` is set. The value itself is " + f"write-only and is never returned.", + ) + super().__init__(**kwargs) + + def get_attribute(self, instance): + return bool(getattr(instance, self.secret_field_name, None)) diff --git a/backend/src/baserow/api/integrations/views.py b/backend/src/baserow/api/integrations/views.py index 49d177dd9c..fd07bcd96f 100644 --- a/backend/src/baserow/api/integrations/views.py +++ b/backend/src/baserow/api/integrations/views.py @@ -19,6 +19,7 @@ validate_body_custom_fields, ) from baserow.api.integrations.errors import ( + ERROR_INTEGRATION_CREDENTIAL_REQUIRED, ERROR_INTEGRATION_DOES_NOT_EXIST, ERROR_INTEGRATION_NOT_IN_SAME_APPLICATION, ) @@ -47,6 +48,7 @@ UpdateIntegrationActionType, ) from baserow.core.integrations.exceptions import ( + IntegrationCredentialRequired, IntegrationDoesNotExist, IntegrationNotInSameApplication, ) @@ -130,6 +132,7 @@ def get(self, request, application_id): request=DiscriminatorCustomFieldsMappingSerializer( integration_type_registry, CreateIntegrationSerializer, + request=True, ), responses={ 200: DiscriminatorCustomFieldsMappingSerializer( @@ -191,6 +194,8 @@ class IntegrationView(APIView): request=CustomFieldRegistryMappingSerializer( integration_type_registry, UpdateIntegrationSerializer, + request=True, + partial_request=True, ), responses={ 200: DiscriminatorCustomFieldsMappingSerializer( @@ -199,6 +204,7 @@ class IntegrationView(APIView): 400: get_error_schema( [ "ERROR_REQUEST_BODY_VALIDATION", + "ERROR_INTEGRATION_CREDENTIAL_REQUIRED", ] ), 404: get_error_schema( @@ -212,6 +218,7 @@ class IntegrationView(APIView): @map_exceptions( { IntegrationDoesNotExist: ERROR_INTEGRATION_DOES_NOT_EXIST, + IntegrationCredentialRequired: ERROR_INTEGRATION_CREDENTIAL_REQUIRED, } ) @require_request_data_type(dict) @@ -229,6 +236,8 @@ def patch(self, request, integration_id: int): integration_type_registry, request.data, base_serializer_class=UpdateIntegrationSerializer, + partial=True, + return_validated=True, ) integration_updated = UpdateIntegrationActionType.do( diff --git a/backend/src/baserow/api/utils.py b/backend/src/baserow/api/utils.py index ea32596a9e..98691933a9 100644 --- a/backend/src/baserow/api/utils.py +++ b/backend/src/baserow/api/utils.py @@ -374,7 +374,8 @@ def get_serializer_class( base_class = ModelSerializer extends_meta = object - meta_extra_kwargs = meta_extra_kwargs or {} + # Copy it: the base class kwargs are merged in below, and the dict is shared. + meta_extra_kwargs = {**(meta_extra_kwargs or {})} if hasattr(base_class, "Meta"): extends_meta = base_class.Meta @@ -449,13 +450,20 @@ def __init__( base_class, many=False, request=False, + partial_request=False, ): + """ + :param partial_request: Documents every type as a partial update, for a + view that validates the request with `partial=True`. + """ + self.read_only = False self.registry = registry self.base_class = base_class self.many = many self.partial = False self.request = request + self.partial_request = partial_request class DiscriminatorCustomFieldsMappingSerializer: diff --git a/backend/src/baserow/contrib/builder/preview/__init__.py b/backend/src/baserow/contrib/builder/preview/__init__.py index 9d2bdd2287..d3a0d45991 100644 --- a/backend/src/baserow/contrib/builder/preview/__init__.py +++ b/backend/src/baserow/contrib/builder/preview/__init__.py @@ -43,6 +43,7 @@ class BuilderPreviewActor: is_authenticated = True is_anonymous = False + is_staff = False user_source_authentication_header = "Authorization" @property diff --git a/backend/src/baserow/contrib/database/data_sync/registries.py b/backend/src/baserow/contrib/database/data_sync/registries.py index 31d144c6ed..2b91108024 100644 --- a/backend/src/baserow/contrib/database/data_sync/registries.py +++ b/backend/src/baserow/contrib/database/data_sync/registries.py @@ -123,6 +123,9 @@ class DataSyncType( request. Example: {"postgresql_password": ["postgresql_host", "postgresql_port"]} + + `IntegrationType.secret_field_dependencies` is the same concept for + integrations. """ sensitive_fields: List[str] = [] diff --git a/backend/src/baserow/contrib/integrations/core/integration_types.py b/backend/src/baserow/contrib/integrations/core/integration_types.py index ffe90aa745..0d0867eb8d 100644 --- a/backend/src/baserow/contrib/integrations/core/integration_types.py +++ b/backend/src/baserow/contrib/integrations/core/integration_types.py @@ -19,9 +19,20 @@ class SerializedDict(IntegrationDict): serializer_field_names = ["host", "port", "use_tls", "username", "password"] allowed_fields = ["host", "port", "use_tls", "username", "password"] sensitive_fields = ["host", "port", "use_tls", "username", "password"] + secret_fields = ["password"] + # Changing where the request goes, or downgrading it to plaintext, would + # send the stored password somewhere its owner never agreed to. + secret_field_dependencies = {"password": ["host", "port", "use_tls"]} request_serializer_field_names = ["host", "port", "use_tls", "username", "password"] request_serializer_field_overrides = {} + serializer_field_extra_kwargs = { + "password": { + "help_text": "The SMTP password. Write-only: it is never returned, " + "see `has_password`. Omit it to keep the stored password; send an " + "empty string or null to clear it." + } + } def deserialize_property( self, diff --git a/backend/src/baserow/contrib/integrations/core/service_types.py b/backend/src/baserow/contrib/integrations/core/service_types.py index 94a89eb245..8fc03232aa 100644 --- a/backend/src/baserow/contrib/integrations/core/service_types.py +++ b/backend/src/baserow/contrib/integrations/core/service_types.py @@ -1044,8 +1044,11 @@ def dispatch_data( backend="django.core.mail.backends.smtp.EmailBackend", host=smtp_integration.host, port=smtp_integration.port, - username=smtp_integration.username, - password=smtp_integration.password, + # Django's backend replaces a None username or password with the + # instance's EMAIL_HOST_USER / EMAIL_HOST_PASSWORD, which would + # authenticate to a host the builder chose. + username=smtp_integration.username or "", + password=smtp_integration.password or "", use_tls=smtp_integration.use_tls, timeout=SMTP_EMAIL_TIMEOUT, ) diff --git a/backend/src/baserow/contrib/integrations/slack/integration_types.py b/backend/src/baserow/contrib/integrations/slack/integration_types.py index 672b843d51..2f71c4b3df 100644 --- a/backend/src/baserow/contrib/integrations/slack/integration_types.py +++ b/backend/src/baserow/contrib/integrations/slack/integration_types.py @@ -16,9 +16,17 @@ class SerializedDict(IntegrationDict): serializer_field_names = ["token"] allowed_fields = ["token"] sensitive_fields = ["token"] + secret_fields = ["token"] request_serializer_field_names = ["token"] request_serializer_field_overrides = {} + serializer_field_extra_kwargs = { + "token": { + "help_text": "The Bot User OAuth Token. Write-only: it is never " + "returned, see `has_token`. Omit it on an update to keep the stored " + "token." + } + } def import_serialized( self, diff --git a/backend/src/baserow/core/integrations/actions.py b/backend/src/baserow/core/integrations/actions.py index 78d05a238a..25bbd5575c 100644 --- a/backend/src/baserow/core/integrations/actions.py +++ b/backend/src/baserow/core/integrations/actions.py @@ -82,6 +82,14 @@ def redo(cls, user: AbstractUser, params: Params, action_to_redo: Action): class UpdateIntegrationActionType(UndoableActionType): + """ + Undo and redo replay the logged values without any credential. The fields + in `IntegrationType.get_action_log_excluded_fields`, which include every + target a credential protects, are never logged, so a replay cannot re-point + a stored password. If a replay does trip `IntegrationCredentialRequired`, + the action handler rolls it back and records the error on the action. + """ + type = "update_integration" description = ActionTypeDescription( _("Update integration"), diff --git a/backend/src/baserow/core/integrations/exceptions.py b/backend/src/baserow/core/integrations/exceptions.py index bd27415289..a4a05514da 100644 --- a/backend/src/baserow/core/integrations/exceptions.py +++ b/backend/src/baserow/core/integrations/exceptions.py @@ -7,3 +7,10 @@ class IntegrationNotInSameApplication(Exception): Raised when trying to order integrations that that don't belong to the same application. """ + + +class IntegrationCredentialRequired(Exception): + """ + Raised when a field that controls where a request goes is changed without the + credential it protects being re-supplied in the same request. + """ diff --git a/backend/src/baserow/core/integrations/registries.py b/backend/src/baserow/core/integrations/registries.py index 42ca5fb907..b20187e59c 100644 --- a/backend/src/baserow/core/integrations/registries.py +++ b/backend/src/baserow/core/integrations/registries.py @@ -1,8 +1,9 @@ from abc import ABC -from typing import Any, Dict, Optional, Type, TypeVar +from typing import Any, Dict, List, Optional, Type, TypeVar from django.contrib.auth.models import AbstractUser +from baserow.api.integrations.fields import HasSecretField from baserow.core.registry import ( CustomFieldsInstanceMixin, CustomFieldsRegistryMixin, @@ -32,6 +33,30 @@ class IntegrationType( An integration type define a specific integration with a given external service. """ + secret_fields: List[str] = [] + """ + Credentials that are write-only: they can be set and overwritten, but are + never serialized back to any user, the creator included. + + This is deliberately narrower than `sensitive_fields`, which is stripped + from workspace exports and kept out of the action log, and which for some + types covers ordinary configuration such as the host and port. + """ + + secret_field_dependencies: Dict[str, List[str]] = {} + """ + Maps a secret field to the request-target fields it protects. If any target + field changes value, the secret must be re-supplied in the same request, + otherwise the stored credential would be sent to a destination its owner + never chose. + + Example: {"password": ["host", "port", "use_tls"]} + + `DataSyncType.secret_field_dependencies` is the same concept for data syncs. + Unlike that check, this one skips a secret that is not stored, because an + integration may authenticate anonymously. + """ + def enhance_queryset(self, queryset): """ Allow to enhance the queryset when querying the integration mainly to improve @@ -57,6 +82,74 @@ def prepare_values( return values + def get_action_log_excluded_fields(self) -> List[str]: + """ + Returns the fields kept out of the update action log. Undo and redo replay + the log without any credential, so a secret and every target it protects + stay out: replaying a host change would send a password stored later to a + host someone else chose. A type that declares no secrets keeps all its + sensitive fields out, because they may hold credentials of their own, + such as the AI integration's `ai_settings`. + """ + + if not self.secret_fields: + return self.sensitive_fields + + targets = [ + target + for targets in self.secret_field_dependencies.values() + for target in targets + ] + return [*self.secret_fields, *targets] + + def get_field_names( + self, request_serializer: bool, extra_params=None, **kwargs + ) -> List[str]: + """ + Removes the secret fields from the response serializer and replaces each + with a `has_` boolean. The request serializer keeps them, because + setting a credential is the only thing a user may do with it. A secret + the serializer does not list gets no flag. + """ + + field_names = super().get_field_names( + request_serializer, extra_params, **kwargs + ) + + if request_serializer or not self.secret_fields: + return field_names + + serialised_secrets = [n for n in field_names if n in self.secret_fields] + + return [name for name in field_names if name not in self.secret_fields] + [ + f"has_{name}" for name in serialised_secrets + ] + + def get_field_overrides( + self, request_serializer: bool, extra_params=None, **kwargs + ) -> Dict: + """ + Declares the `has_` booleans on the response serializer. + """ + + overrides = super().get_field_overrides( + request_serializer, extra_params, **kwargs + ) + + if request_serializer or not self.secret_fields: + return overrides + + # DRF asserts if a declared field is missing from the name list. + field_names = self.get_field_names(request_serializer, extra_params, **kwargs) + return { + **overrides, + **{ + f"has_{name}": HasSecretField(name) + for name in self.secret_fields + if f"has_{name}" in field_names + }, + } + def serialize_property( self, integration: Integration, diff --git a/backend/src/baserow/core/integrations/service.py b/backend/src/baserow/core/integrations/service.py index 524e4283f6..a5d007fb42 100644 --- a/backend/src/baserow/core/integrations/service.py +++ b/backend/src/baserow/core/integrations/service.py @@ -1,10 +1,13 @@ -from typing import List, Optional +from typing import Any, Dict, List, Optional from django.contrib.auth.models import AbstractUser from baserow.core.exceptions import CannotCalculateIntermediateOrder from baserow.core.handler import CoreHandler -from baserow.core.integrations.exceptions import IntegrationNotInSameApplication +from baserow.core.integrations.exceptions import ( + IntegrationCredentialRequired, + IntegrationNotInSameApplication, +) from baserow.core.integrations.handler import IntegrationHandler from baserow.core.integrations.models import Integration from baserow.core.integrations.operations import ( @@ -134,8 +137,38 @@ def create_integration( return new_integration + def _check_secret_dependencies( + self, + integration: IntegrationForUpdate, + integration_type: IntegrationType, + values: Dict[str, Any], + ): + """ + Raises if a request-target field is being changed without the credential + it protects being supplied in the same request. A secret that is not + stored is skipped, so an integration that authenticates anonymously can + still change its host. + """ + + for secret, targets in integration_type.secret_field_dependencies.items(): + if not getattr(integration, secret, None): + continue + + target_changed = any( + target in values and values[target] != getattr(integration, target) + for target in targets + ) + if target_changed and secret not in values: + raise IntegrationCredentialRequired( + f"The `{secret}` must be supplied again when the connection " + f"target changes." + ) + def update_integration( - self, user: AbstractUser, integration: IntegrationForUpdate, **kwargs + self, + user: AbstractUser, + integration: IntegrationForUpdate, + **kwargs, ) -> UpdatedIntegration: """ Updates and integration with values. Will also check if the values are allowed @@ -143,9 +176,10 @@ def update_integration( :param user: The user trying to update the integration. :param integration: The integration that should be updated. - :param values: The values that should be set on the integration. :param kwargs: Additional attributes of the integration. :return: The updated integration together with the values that changed. + :raises IntegrationCredentialRequired: When a request-target field changes + without its credential. """ CoreHandler().check_permissions( @@ -157,11 +191,13 @@ def update_integration( integration_type = integration.get_type() + self._check_secret_dependencies(integration, integration_type, kwargs) + # Capture the original and new values (in the service-level vocabulary, so # FK fields are stored as their ids) before `prepare_values` mutates them, so # the update can be undone/redone. original_values, new_values = extract_undo_redo_values( - integration, kwargs, integration_type.sensitive_fields + integration, kwargs, integration_type.get_action_log_excluded_fields() ) prepared_values = integration_type.prepare_values(kwargs, user) diff --git a/backend/src/baserow/core/registry.py b/backend/src/baserow/core/registry.py index a3ba9e7a86..3c622549c2 100644 --- a/backend/src/baserow/core/registry.py +++ b/backend/src/baserow/core/registry.py @@ -538,7 +538,9 @@ class EasyImportExportMixin(Generic[T], ABC): SerializedDict: Type[TypedDict] # List of fields that are potentially sensitive and shouldn't be included - # when exporting the application. + # when exporting the application. This does not hide them from API responses: + # integration credentials that must be write-only belong in + # `IntegrationType.secret_fields` as well. sensitive_fields: List[str] = [] # The parent property name for the model diff --git a/backend/src/baserow/test_utils/fixtures/integration.py b/backend/src/baserow/test_utils/fixtures/integration.py index 6221991537..02e05124c3 100644 --- a/backend/src/baserow/test_utils/fixtures/integration.py +++ b/backend/src/baserow/test_utils/fixtures/integration.py @@ -1,5 +1,6 @@ from baserow.contrib.integrations.core.models import SMTPIntegration from baserow.contrib.integrations.local_baserow.models import LocalBaserowIntegration +from baserow.contrib.integrations.slack.models import SlackBotIntegration from baserow.core.integrations.registries import integration_type_registry @@ -25,6 +26,13 @@ def create_smtp_integration(self, **kwargs): integration = self.create_integration(SMTPIntegration, **kwargs) return integration + def create_slack_bot_integration(self, **kwargs): + if "token" not in kwargs: + kwargs["token"] = "xoxb-test-token" # nosec B105 + + integration = self.create_integration(SlackBotIntegration, **kwargs) + return integration + def create_integration_with_first_type(self, **kwargs): first_type = list(integration_type_registry.get_all())[0] return self.create_integration(first_type.model_class, **kwargs) diff --git a/backend/tests/baserow/api/integrations/test_integration_views.py b/backend/tests/baserow/api/integrations/test_integration_views.py index 40756515cb..b02a4a5d89 100644 --- a/backend/tests/baserow/api/integrations/test_integration_views.py +++ b/backend/tests/baserow/api/integrations/test_integration_views.py @@ -11,7 +11,12 @@ HTTP_404_NOT_FOUND, ) +from baserow.api.integrations.serializers import ( + CreateIntegrationSerializer, + UpdateIntegrationSerializer, +) from baserow.core.integrations.models import Integration +from baserow.core.integrations.registries import integration_type_registry from baserow.core.registries import application_type_registry @@ -409,3 +414,299 @@ def test_get_integrations_context_data_present_when_databases_raises( assert response.status_code == HTTP_200_OK response_json = response.json() assert response_json[0]["context_data"] == {"databases": []} + + +@pytest.mark.django_db +def test_get_integrations_does_not_return_smtp_password(api_client, data_fixture): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + data_fixture.create_smtp_integration( + application=application, password="supersecret" + ) + + url = reverse("api:integrations:list", kwargs={"application_id": application.id}) + response = api_client.get(url, format="json", HTTP_AUTHORIZATION=f"JWT {token}") + + assert response.status_code == HTTP_200_OK + integration_json = response.json()[0] + assert "password" not in integration_json + assert integration_json["has_password"] is True + assert integration_json["host"] == "smtp.example.com" + + +@pytest.mark.django_db +def test_get_integrations_has_password_false_when_unset(api_client, data_fixture): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + data_fixture.create_smtp_integration(application=application, password="") + + url = reverse("api:integrations:list", kwargs={"application_id": application.id}) + response = api_client.get(url, format="json", HTTP_AUTHORIZATION=f"JWT {token}") + + assert response.status_code == HTTP_200_OK + assert response.json()[0]["has_password"] is False + + +@pytest.mark.django_db +def test_get_integrations_does_not_return_slack_token(api_client, data_fixture): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + data_fixture.create_slack_bot_integration( + application=application, token="xoxb-secret" + ) + + url = reverse("api:integrations:list", kwargs={"application_id": application.id}) + response = api_client.get(url, format="json", HTTP_AUTHORIZATION=f"JWT {token}") + + assert response.status_code == HTTP_200_OK + integration_json = response.json()[0] + assert "token" not in integration_json + assert integration_json["has_token"] is True + + +@pytest.mark.django_db +def test_create_slack_integration_without_a_token_is_rejected(api_client, data_fixture): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + + url = reverse("api:integrations:list", kwargs={"application_id": application.id}) + response = api_client.post( + url, + {"type": "slack_bot", "name": "No token"}, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_400_BAD_REQUEST + assert response.json()["error"] == "ERROR_REQUEST_BODY_VALIDATION" + assert response.json()["detail"]["token"][0]["code"] == "required" + + # The published create schema must agree with the server. + serializer_class = integration_type_registry.get("slack_bot").get_serializer_class( + request_serializer=True, base_class=CreateIntegrationSerializer + ) + assert serializer_class().fields["token"].required is True + + +@pytest.mark.django_db +def test_create_slack_integration_with_a_blank_token_is_rejected( + api_client, data_fixture +): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + + url = reverse("api:integrations:list", kwargs={"application_id": application.id}) + response = api_client.post( + url, + {"type": "slack_bot", "name": "Blank token", "token": ""}, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_400_BAD_REQUEST + assert response.json()["detail"]["token"][0]["code"] == "blank" + + +@pytest.mark.django_db +def test_create_smtp_integration_without_a_password_is_allowed( + api_client, data_fixture +): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + + # Unlike the Slack token, the SMTP password is blankable: a relay that + # accepts anonymous mail needs no credential. + url = reverse("api:integrations:list", kwargs={"application_id": application.id}) + response = api_client.post( + url, + {"type": "smtp", "name": "Mailer", "host": "smtp.example.com"}, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_200_OK + assert response.json()["has_password"] is False + + +@pytest.mark.django_db +def test_update_smtp_integration_response_omits_password(api_client, data_fixture): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + integration = data_fixture.create_smtp_integration(application=application) + + url = reverse("api:integrations:item", kwargs={"integration_id": integration.id}) + response = api_client.patch( + url, + {"password": "newsecret"}, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_200_OK + assert "password" not in response.json() + assert response.json()["has_password"] is True + integration.refresh_from_db() + assert integration.password == "newsecret" + + +@pytest.mark.django_db +def test_update_slack_integration_without_token_succeeds(api_client, data_fixture): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + integration = data_fixture.create_slack_bot_integration( + application=application, token="xoxb-secret" + ) + + url = reverse("api:integrations:item", kwargs={"integration_id": integration.id}) + response = api_client.patch( + url, + {"name": "Renamed"}, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_200_OK + integration.refresh_from_db() + assert integration.name == "Renamed" + assert integration.token == "xoxb-secret" + + +@pytest.mark.django_db +def test_update_slack_integration_with_empty_token_is_rejected( + api_client, data_fixture +): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + integration = data_fixture.create_slack_bot_integration( + application=application, token="xoxb-secret" + ) + + url = reverse("api:integrations:item", kwargs={"integration_id": integration.id}) + response = api_client.patch( + url, + {"token": ""}, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_400_BAD_REQUEST + assert response.json()["detail"]["token"][0]["code"] == "blank" + integration.refresh_from_db() + assert integration.token == "xoxb-secret" + + +@pytest.mark.django_db +def test_update_smtp_integration_host_without_password_returns_400( + api_client, data_fixture +): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + integration = data_fixture.create_smtp_integration( + application=application, host="smtp.original.com", password="secret" + ) + + url = reverse("api:integrations:item", kwargs={"integration_id": integration.id}) + response = api_client.patch( + url, + {"host": "smtp.attacker.com"}, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_400_BAD_REQUEST + assert response.json()["error"] == "ERROR_INTEGRATION_CREDENTIAL_REQUIRED" + integration.refresh_from_db() + assert integration.host == "smtp.original.com" + assert integration.password == "secret" + + +@pytest.mark.django_db +def test_update_smtp_integration_without_password_keeps_the_stored_one( + api_client, data_fixture +): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + integration = data_fixture.create_smtp_integration( + application=application, host="smtp.original.com", password="secret" + ) + + url = reverse("api:integrations:item", kwargs={"integration_id": integration.id}) + response = api_client.patch( + url, + {"host": "smtp.original.com", "username": "mailer"}, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_200_OK + integration.refresh_from_db() + assert integration.username == "mailer" + assert integration.password == "secret" + + +@pytest.mark.django_db +def test_update_smtp_integration_with_empty_password_clears_it( + api_client, data_fixture +): + user, token = data_fixture.create_user_and_token() + application = data_fixture.create_builder_application(user=user) + integration = data_fixture.create_smtp_integration( + application=application, host="smtp.original.com", password="secret" + ) + + url = reverse("api:integrations:item", kwargs={"integration_id": integration.id}) + response = api_client.patch( + url, + {"host": "smtp.original.com", "password": ""}, + format="json", + HTTP_AUTHORIZATION=f"JWT {token}", + ) + + assert response.status_code == HTTP_200_OK + assert response.json()["has_password"] is False + integration.refresh_from_db() + assert integration.password == "" + + +@pytest.mark.django_db +def test_secret_request_fields_document_the_write_only_contract(): + for type_name, secret in [("smtp", "password"), ("slack_bot", "token")]: + serializer_class = integration_type_registry.get( + type_name + ).get_serializer_class( + request_serializer=True, base_class=UpdateIntegrationSerializer + ) + assert "keep the stored" in serializer_class().fields[secret].help_text + + # The update base class makes `name` optional. That must not leak into the + # create serializer through the type's shared extra kwargs. + serializer_class = integration_type_registry.get("smtp").get_serializer_class( + request_serializer=True, base_class=CreateIntegrationSerializer + ) + assert serializer_class().fields["name"].required is True + + +@pytest.mark.django_db +def test_integration_request_schemas_match_the_validation(api_client): + schema = api_client.get(reverse("api:json_schema")).json() + components = schema["components"]["schemas"] + + def request_components(path, method): + body = schema["paths"][path][method]["requestBody"]["content"] + ref = body["application/json"]["schema"]["$ref"].split("/")[-1] + return { + sub["$ref"].split("/")[-1]: components[sub["$ref"].split("/")[-1]] + for sub in components[ref].get("anyOf", components[ref].get("oneOf", [])) + } + + update = request_components("/api/integration/{integration_id}/", "patch") + assert update + # The update validates with `partial=True`, so nothing may be documented as + # required, the Slack token included. + assert all(not component.get("required") for component in update.values()) + + create = request_components( + "/api/application/{application_id}/integrations/", "post" + ) + slack_create = next(c for name, c in create.items() if name.startswith("SlackBot")) + assert "token" in slack_create["required"] diff --git a/backend/tests/baserow/contrib/integrations/core/test_smtp_email_service_type.py b/backend/tests/baserow/contrib/integrations/core/test_smtp_email_service_type.py index 64918e9a7d..9d9dcdc241 100644 --- a/backend/tests/baserow/contrib/integrations/core/test_smtp_email_service_type.py +++ b/backend/tests/baserow/contrib/integrations/core/test_smtp_email_service_type.py @@ -107,6 +107,42 @@ def test_send_smtp_email_basic(data_fixture): assert result.data == {"success": True} +@pytest.mark.django_db +@override_settings( + CELERY_EMAIL_BACKEND="django.core.mail.backends.smtp.EmailBackend", + EMAIL_HOST_USER="instance-user", + EMAIL_HOST_PASSWORD="instance-password", +) +def test_send_smtp_email_without_credentials_does_not_use_the_instance_account( + data_fixture, +): + # Django's SMTP backend replaces a None username or password with the + # instance's own, so an integration with none stored would otherwise + # authenticate to its own host with the instance mail account. + smtp_integration = data_fixture.create_smtp_integration( + host="smtp.example.com", + username=None, + password=None, + ) + service = data_fixture.create_core_smtp_email_service( + integration=smtp_integration, + from_email="'sender@example.com'", + to_emails="'recipient@example.com'", + subject="'Subject'", + body="'Body'", + ) + + with patch( + "baserow.contrib.integrations.core.service_types.EmailMultiAlternatives", + ) as mock_email: + service.get_type().dispatch(service, FakeDispatchContext()) + + connection = mock_email.call_args.kwargs["connection"] + assert connection.host == "smtp.example.com" + assert connection.username == "" + assert connection.password == "" + + @pytest.mark.django_db @override_settings( CELERY_EMAIL_BACKEND="anymail.backends.mailgun.EmailBackend", diff --git a/backend/tests/baserow/contrib/integrations/core/test_smtp_integration_type.py b/backend/tests/baserow/contrib/integrations/core/test_smtp_integration_type.py index 8cf981f06b..6b87de4bef 100644 --- a/backend/tests/baserow/contrib/integrations/core/test_smtp_integration_type.py +++ b/backend/tests/baserow/contrib/integrations/core/test_smtp_integration_type.py @@ -98,18 +98,21 @@ def test_smtp_integration_partial_update(data_fixture): password="password123", ) + # Changing the host and port re-points where the password is sent, so it + # has to be supplied again in the same call. updated_integration = IntegrationService().update_integration( user, integration, host="smtp.newhost.com", port=465, + password="password123", ) assert updated_integration.integration.host == "smtp.newhost.com" assert updated_integration.integration.port == 465 assert updated_integration.integration.use_tls is True # unchanged assert updated_integration.integration.username == "user@example.com" # unchanged - assert updated_integration.integration.password == "password123" # unchanged + assert updated_integration.integration.password == "password123" @pytest.mark.django_db diff --git a/backend/tests/baserow/core/integrations/test_integration_actions.py b/backend/tests/baserow/core/integrations/test_integration_actions.py index d92a0e28e7..0b91616e2a 100644 --- a/backend/tests/baserow/core/integrations/test_integration_actions.py +++ b/backend/tests/baserow/core/integrations/test_integration_actions.py @@ -3,6 +3,7 @@ import pytest from baserow.core.action.handler import ActionHandler +from baserow.core.action.models import Action from baserow.core.action.scopes import ApplicationActionScopeType from baserow.core.integrations.actions import ( CreateIntegrationActionType, @@ -214,3 +215,52 @@ def test_integration_grouped_create_configure_undo_redo(data_fixture): assert Integration.objects.filter(id=integration.id).exists() integration.refresh_from_db() assert integration.name == "Configured" + + +@pytest.mark.django_db +@pytest.mark.undo_redo +def test_update_smtp_integration_action_does_not_log_the_password(data_fixture): + """ + The credential and the connection targets it protects stay out of the + action log. A replay carries no credential, so a recorded target change + could re-point whatever password happens to be stored at the time. + """ + + session_id = str(uuid.uuid4()) + user = data_fixture.create_user(session_id=session_id) + application = data_fixture.create_builder_application(user=user) + integration = data_fixture.create_smtp_integration( + user=user, application=application, password="secret" + ) + + UpdateIntegrationActionType.do( + user, integration, host="smtp.changed.com", password="newsecret" + ) + + logged = Action.objects.filter(type=UpdateIntegrationActionType.type).last() + for params in ("integration_original_params", "integration_new_params"): + assert "password" not in logged.params[params] + assert "host" not in logged.params[params] + + +@pytest.mark.django_db +@pytest.mark.undo_redo +def test_update_smtp_integration_username_can_be_undone_and_redone(data_fixture): + session_id = str(uuid.uuid4()) + user = data_fixture.create_user(session_id=session_id) + application = data_fixture.create_builder_application(user=user) + integration = data_fixture.create_smtp_integration( + user=user, application=application, username="old", password="secret" + ) + + UpdateIntegrationActionType.do(user, integration, username="new") + + ActionHandler.undo(user, _scope(application), session_id) + integration.refresh_from_db() + assert integration.username == "old" + assert integration.password == "secret" + + ActionHandler.redo(user, _scope(application), session_id) + integration.refresh_from_db() + assert integration.username == "new" + assert integration.password == "secret" diff --git a/backend/tests/baserow/core/integrations/test_integration_service.py b/backend/tests/baserow/core/integrations/test_integration_service.py index 4ca45993d2..10491ce5fd 100644 --- a/backend/tests/baserow/core/integrations/test_integration_service.py +++ b/backend/tests/baserow/core/integrations/test_integration_service.py @@ -5,9 +5,11 @@ from baserow.core.exceptions import PermissionException from baserow.core.integrations.exceptions import ( + IntegrationCredentialRequired, IntegrationDoesNotExist, IntegrationNotInSameApplication, ) +from baserow.core.integrations.handler import IntegrationHandler from baserow.core.integrations.models import Integration from baserow.core.integrations.registries import integration_type_registry from baserow.core.integrations.service import IntegrationService @@ -369,3 +371,164 @@ def test_move_integration_trigger_order_recalculated( integration_orders_recalculated_mock.send.assert_called_once_with( service, application=application ) + + +@pytest.mark.django_db +def test_update_smtp_integration_host_without_password_is_rejected(data_fixture): + user = data_fixture.create_user() + integration = data_fixture.create_smtp_integration( + user=user, host="smtp.original.com", password="secret" + ) + + with pytest.raises(IntegrationCredentialRequired): + IntegrationService().update_integration( + user, integration, host="smtp.attacker.com" + ) + + integration.refresh_from_db() + assert integration.host == "smtp.original.com" + assert integration.password == "secret" + + +@pytest.mark.django_db +def test_update_smtp_integration_port_without_password_is_rejected(data_fixture): + user = data_fixture.create_user() + integration = data_fixture.create_smtp_integration( + user=user, port=587, password="secret" + ) + + with pytest.raises(IntegrationCredentialRequired): + IntegrationService().update_integration(user, integration, port=2525) + + +@pytest.mark.django_db +def test_update_smtp_integration_disabling_tls_without_password_is_rejected( + data_fixture, +): + user = data_fixture.create_user() + integration = data_fixture.create_smtp_integration( + user=user, use_tls=True, password="secret" + ) + + with pytest.raises(IntegrationCredentialRequired): + IntegrationService().update_integration(user, integration, use_tls=False) + + +@pytest.mark.django_db +def test_update_smtp_integration_host_with_password_succeeds(data_fixture): + user = data_fixture.create_user() + integration = data_fixture.create_smtp_integration( + user=user, host="smtp.original.com", password="secret" + ) + + IntegrationService().update_integration( + user, integration, host="smtp.new.com", password="newsecret" + ) + + integration.refresh_from_db() + assert integration.host == "smtp.new.com" + assert integration.password == "newsecret" + + +@pytest.mark.django_db +def test_update_smtp_integration_unchanged_host_does_not_require_password( + data_fixture, +): + user = data_fixture.create_user() + integration = data_fixture.create_smtp_integration( + user=user, host="smtp.original.com", password="secret" + ) + + IntegrationService().update_integration( + user, integration, host="smtp.original.com", username="mailer" + ) + + integration.refresh_from_db() + assert integration.username == "mailer" + assert integration.password == "secret" + + +@pytest.mark.django_db +def test_update_smtp_integration_name_only_keeps_the_password(data_fixture): + user = data_fixture.create_user() + integration = data_fixture.create_smtp_integration(user=user, password="secret") + + IntegrationService().update_integration(user, integration, name="Renamed") + + integration.refresh_from_db() + assert integration.name == "Renamed" + assert integration.password == "secret" + + +@pytest.mark.django_db +def test_update_smtp_integration_empty_password_clears_it(data_fixture): + user = data_fixture.create_user() + integration = data_fixture.create_smtp_integration(user=user, password="secret") + + IntegrationService().update_integration(user, integration, password="") + + integration.refresh_from_db() + assert integration.password == "" + + +@pytest.mark.django_db +def test_update_smtp_integration_without_a_stored_password_allows_host_change( + data_fixture, +): + """ + An integration that authenticates anonymously has no credential to + redirect, so the check must not demand one that does not exist. + """ + + user = data_fixture.create_user() + integration = data_fixture.create_smtp_integration( + user=user, host="smtp.original.com", password="" + ) + + IntegrationService().update_integration(user, integration, host="smtp.other.com") + + integration.refresh_from_db() + assert integration.host == "smtp.other.com" + + +@pytest.mark.django_db +def test_update_slack_integration_has_no_target_dependency(data_fixture): + user = data_fixture.create_user() + integration = data_fixture.create_slack_bot_integration( + user=user, token="xoxb-secret" + ) + + IntegrationService().update_integration(user, integration, name="Renamed") + + integration.refresh_from_db() + assert integration.name == "Renamed" + assert integration.token == "xoxb-secret" + + +@pytest.mark.django_db +def test_ai_integration_settings_are_kept_out_of_the_action_log(data_fixture): + """ + The AI integration's provider API keys live inside `ai_settings`, a + sensitive field, and must not reach the action log, which the audit log + copies verbatim. + """ + + user = data_fixture.create_user() + application = data_fixture.create_builder_application(user=user) + integration_type = integration_type_registry.get("ai") + created = IntegrationService().create_integration( + user, + integration_type, + application=application, + ai_settings={"openai": {"api_key": "OLDKEY", "models": ["gpt-4"]}}, + ) + for_update = IntegrationHandler().get_integration_for_update(created.id) + + updated = IntegrationService().update_integration( + user, + for_update, + ai_settings={"openai": {"api_key": "NEWKEY", "models": ["gpt-4"]}}, + ) + + assert "ai_settings" not in updated.original_values + assert "ai_settings" not in updated.new_values diff --git a/backend/tests/baserow/core/integrations/test_ws_integrations_signals.py b/backend/tests/baserow/core/integrations/test_ws_integrations_signals.py index e1dc90fc18..c94e1af80a 100644 --- a/backend/tests/baserow/core/integrations/test_ws_integrations_signals.py +++ b/backend/tests/baserow/core/integrations/test_ws_integrations_signals.py @@ -50,3 +50,19 @@ def test_integration_deleted(mock_broadcast, data_fixture): args = mock_broadcast.delay.call_args assert args[0][4]["type"] == "integration_deleted" assert args[0][4]["integration_id"] == integration_id + + +@pytest.mark.django_db(transaction=True) +@patch("baserow.core.integrations.ws.signals.broadcast_to_permitted_users") +def test_integration_updated_does_not_broadcast_secret(mock_broadcast, data_fixture): + user = data_fixture.create_user() + integration = data_fixture.create_smtp_integration( + user=user, password="supersecret" + ) + + IntegrationService().update_integration(user, integration, name="Updated") + + mock_broadcast.delay.assert_called_once() + payload = mock_broadcast.delay.call_args[0][4]["integration"] + assert "password" not in payload + assert payload["has_password"] is True diff --git a/backend/tests/baserow/throttling/test_builder_preview_throttle.py b/backend/tests/baserow/throttling/test_builder_preview_throttle.py new file mode 100644 index 0000000000..0f5718416d --- /dev/null +++ b/backend/tests/baserow/throttling/test_builder_preview_throttle.py @@ -0,0 +1,32 @@ +import pytest +from rest_framework.request import Request +from rest_framework.test import APIRequestFactory + +from baserow.api.exceptions import ThrottledAPIException +from baserow.contrib.builder.preview import BuilderPreviewActor +from baserow.throttling.handler import ConcurrentUserRequestsThrottle + + +def test_builder_preview_requests_are_throttled_and_release_their_slot( + settings, monkeypatch +): + """Preview actors obey the concurrency limit and free their slot after a response.""" + + settings.BASEROW_THROTTLE_BLACKLIST_TTL_SECONDS = 0 + monkeypatch.setattr(ConcurrentUserRequestsThrottle, "rate", 1, raising=False) + actor = BuilderPreviewActor( + builder_id=1, workspace_id=1, grant_id=12345, issued_by_user_id=1 + ) + request = Request(APIRequestFactory().get("/api/builder/preview/1/current/")) + request.user = actor + + throttle = ConcurrentUserRequestsThrottle() + assert throttle.allow_request(request, None) + try: + with pytest.raises(ThrottledAPIException): + ConcurrentUserRequestsThrottle().allow_request(request, None) + finally: + ConcurrentUserRequestsThrottle.on_request_processed(request._request) + + assert ConcurrentUserRequestsThrottle().allow_request(request, None) + ConcurrentUserRequestsThrottle.on_request_processed(request._request) diff --git a/changelog/entries/unreleased/breaking_change/integration_credentials_are_now_writeonly_and_are_never_retu.json b/changelog/entries/unreleased/breaking_change/integration_credentials_are_now_writeonly_and_are_never_retu.json new file mode 100644 index 0000000000..672401e8b2 --- /dev/null +++ b/changelog/entries/unreleased/breaking_change/integration_credentials_are_now_writeonly_and_are_never_retu.json @@ -0,0 +1,13 @@ +{ + "type": "breaking_change", + "message": "SMTP integration passwords and Slack bot tokens are now write-only and are never returned by the API", + "issue_origin": "github", + "issue_number": null, + "domain": "integration", + "bullet_points": [ + "Breaking change: the SMTP integration's `password` and the Slack bot integration's `token` are no longer returned by the integration API. Each is replaced by a read-only `has_password` / `has_token` boolean. Any integration that reads those fields must be updated.", + "Changing an SMTP integration's host, port or TLS setting now requires the password to be supplied again in the same request, so a stored password cannot be redirected to another server or sent over an unencrypted connection.", + "During a rolling deploy, a browser tab still running the previous frontend sends an empty SMTP password when the integration is saved, which clears the stored one. Re-entering the password restores it." + ], + "created_at": "2026-09-10" +} diff --git a/changelog/entries/unreleased/bug/fix_builder_preview_with_concurrency_throttling.json b/changelog/entries/unreleased/bug/fix_builder_preview_with_concurrency_throttling.json new file mode 100644 index 0000000000..31764aa76a --- /dev/null +++ b/changelog/entries/unreleased/bug/fix_builder_preview_with_concurrency_throttling.json @@ -0,0 +1,9 @@ +{ + "type": "bug", + "message": "Fixed Application Builder previews failing when concurrent request throttling is enabled.", + "issue_origin": "github", + "issue_number": null, + "domain": "builder", + "bullet_points": [], + "created_at": "2026-09-14" +} diff --git a/e2e-tests/fixtures/builder/integration.ts b/e2e-tests/fixtures/builder/integration.ts index cf43edbaa8..fb2fa465aa 100644 --- a/e2e-tests/fixtures/builder/integration.ts +++ b/e2e-tests/fixtures/builder/integration.ts @@ -5,17 +5,48 @@ export class Integration { constructor( public id: number, public type: string, - public builder: Builder, + public builder: Builder ) {} } export async function createLocalBaserowIntegration( builder: Builder, - name = "Local Baserow", + name = "Local Baserow" ): Promise { const response: any = await getClient(builder.workspace.user).post( `application/${builder.id}/integrations/`, - { type: "local_baserow", name }, + { type: "local_baserow", name } ); return new Integration(response.data.id, response.data.type, builder); } + +/** + * An SMTP integration on the builder. The password is write-only: the API + * never returns it, so a test that needs to know it has to remember what it + * sent here. + */ +export async function createSMTPIntegration( + builder: Builder, + options: { name?: string; host?: string; password: string } +): Promise { + const response: any = await getClient(builder.workspace.user).post( + `application/${builder.id}/integrations/`, + { + type: "smtp", + name: options.name ?? "Mailer", + host: options.host ?? "smtp.example.com", + port: 587, + use_tls: true, + username: "mailer", + password: options.password, + } + ); + return new Integration(response.data.id, response.data.type, builder); +} + +export async function listIntegrations(builder: Builder): Promise { + const response: any = await getClient(builder.workspace.user).get( + `application/${builder.id}/integrations/` + ); + return response.data; +} diff --git a/e2e-tests/tests/builder/integrationSecrets.spec.ts b/e2e-tests/tests/builder/integrationSecrets.spec.ts new file mode 100644 index 0000000000..397feb0081 --- /dev/null +++ b/e2e-tests/tests/builder/integrationSecrets.spec.ts @@ -0,0 +1,187 @@ +import { + createSMTPIntegration, + listIntegrations, +} from "../../fixtures/builder/integration"; +import { expect, test } from "../baserowTest"; + +/** + * The SMTP password is write-only: it can be set and overwritten, but the API + * never returns it to anyone, the creator included. + * + * These cover the seam between the two halves of that rule, which unit tests + * on either side cannot see. The form omits an untouched password from the + * request; the backend reads an omitted password as "keep what is stored". If + * either side changes its mind about what an untouched field looks like, a + * save that has nothing to do with the password silently destroys it, and + * nothing tells the user. + */ + +const PASSWORD = "e2e-top-secret"; + +/** Open the builder application's Integrations settings. */ +async function openIntegrationSettings(page, builderName: string) { + // The application's own row. Scope to its direct `.tree__action` child: the + // pages nested underneath carry a `.tree__options` of their own. + const sidebarRow = page + .locator("li.tree__item", { hasText: builderName }) + .locator("> .tree__action"); + await sidebarRow.hover(); + + // The Context self-hides when it doesn't fit the viewport, and the first + // click can land before hydration settles, so retry until the item shows. + const settingsItem = page.locator( + '.context__menu-item-link:visible:has-text("Settings")' + ); + await expect(async () => { + await sidebarRow.locator(".tree__options").click(); + await expect(settingsItem).toBeVisible({ timeout: 1000 }); + }).toPass({ timeout: 15000 }); + await settingsItem.click(); + + await page + .locator(".modal-sidebar__nav-link", { hasText: "Integrations" }) + .click(); + await expect( + page.locator(".box__title", { hasText: "Integrations" }) + ).toBeVisible(); +} + +/** Open the edit modal of the integration with the given name. */ +async function openIntegrationEditModal(page, name: string) { + const row = page.locator(".integration-settings__integration", { + hasText: name, + }); + await row + .locator(".integration-settings__integration-actions .button-icon") + .first() + .click(); + + const modal = page.locator(".modal__box:visible", { + hasText: "Edit integration", + }); + await expect(modal).toBeVisible(); + return modal; +} + +test.describe("Write-only integration secrets", () => { + test("the password is never sent to the browser, and a rename keeps it", async ({ + page, + builderPagePage, + }) => { + const builder = builderPagePage.builder; + await createSMTPIntegration(builder, { + name: "Mailer", + password: PASSWORD, + }); + + // Every response the browser receives, from the moment it loads the + // builder. A single one carrying the password is the bug this guards. + const leaked: string[] = []; + // Playwright does not await a listener's promise, so every body read has + // to be collected and awaited before asserting, or a leaking response + // still in flight would slip past and the test would pass on nothing. + const reads: Promise[] = []; + page.on("response", (response) => { + if (!response.url().includes("/api/")) return; + reads.push( + response + .text() + .then((body) => { + if (body.includes(PASSWORD)) leaked.push(response.url()); + }) + // Redirects and responses without a body cannot leak anything. + .catch(() => {}) + ); + }); + + await builderPagePage.goto(); + await openIntegrationSettings(page, builder.name); + const modal = await openIntegrationEditModal(page, "Mailer"); + + // The field renders empty, and says so rather than looking unset. + const password = modal.getByPlaceholder( + "Leave blank to keep the saved password" + ); + await expect(password).toHaveValue(""); + await expect( + modal.getByText("A password is saved. Leave blank to keep it.") + ).toBeVisible(); + + // A save that has nothing to do with the password. + await modal + .getByPlaceholder("Enter integration name...") + .fill("Renamed mailer"); + await modal.getByRole("button", { name: "Save" }).click(); + await expect(modal).toBeHidden(); + + const [integration] = await listIntegrations(builder); + expect(integration.name).toBe("Renamed mailer"); + // The password survived the save, and the API still refuses to show it. + expect(integration.has_password).toBe(true); + expect(integration).not.toHaveProperty("password"); + + await Promise.all(reads); + expect(leaked, "the password was sent to the browser").toEqual([]); + }); + + test("changing the host without retyping the password is refused", async ({ + page, + builderPagePage, + }) => { + const builder = builderPagePage.builder; + await createSMTPIntegration(builder, { + name: "Mailer", + host: "smtp.example.com", + password: PASSWORD, + }); + + await builderPagePage.goto(); + await openIntegrationSettings(page, builder.name); + const modal = await openIntegrationEditModal(page, "Mailer"); + + await modal + .getByPlaceholder("smtp.gmail.com") + .fill("smtp.attacker.example"); + await modal.getByRole("button", { name: "Save" }).click(); + + await expect(modal.getByText("Credential required")).toBeVisible(); + await expect( + modal.getByText( + "Enter the credential again to change where this integration connects." + ) + ).toBeVisible(); + + // Nothing was written: the host is unchanged and the password is intact. + const [integration] = await listIntegrations(builder); + expect(integration.host).toBe("smtp.example.com"); + expect(integration.has_password).toBe(true); + }); + + test("retyping the password lets the host change go through", async ({ + page, + builderPagePage, + }) => { + const builder = builderPagePage.builder; + await createSMTPIntegration(builder, { + name: "Mailer", + host: "smtp.example.com", + password: PASSWORD, + }); + + await builderPagePage.goto(); + await openIntegrationSettings(page, builder.name); + const modal = await openIntegrationEditModal(page, "Mailer"); + + await modal.getByPlaceholder("smtp.gmail.com").fill("smtp.newhost.example"); + await modal + .getByPlaceholder("Leave blank to keep the saved password") + .fill("a-new-secret"); + await modal.getByRole("button", { name: "Save" }).click(); + await expect(modal).toBeHidden(); + + const [integration] = await listIntegrations(builder); + expect(integration.host).toBe("smtp.newhost.example"); + expect(integration.has_password).toBe(true); + expect(integration).not.toHaveProperty("password"); + }); +}); diff --git a/web-frontend/modules/builder/components/workflowAction/WorkflowActionWithService.vue b/web-frontend/modules/builder/components/workflowAction/WorkflowActionWithService.vue index 4b45e3b05b..b6f6f0fd56 100644 --- a/web-frontend/modules/builder/components/workflowAction/WorkflowActionWithService.vue +++ b/web-frontend/modules/builder/components/workflowAction/WorkflowActionWithService.vue @@ -16,13 +16,7 @@ :loading="workflowActionLoading" :databases="databases" :default-values="defaultValues.service" - @values-changed=" - values.service = { - ...workflowAction.service, - ...values.service, - ...$event, - } - " + @values-changed="bufferServiceChange($event)" > @@ -51,6 +45,12 @@ export default { values: { service: {}, }, + // The service changes the user has made but not yet had saved, + // accumulated from the wrapped form's `values-changed` events. The form + // only emits editable fields, so this never carries the read-only, + // backend-computed ones (`schema`, sample data, …); those always come + // fresh from `workflowAction.service` when we rebuild `values.service`. + pendingServiceChanges: {}, } }, computed: { @@ -78,11 +78,7 @@ export default { ) }, set(newValue) { - this.values.service = { - ...this.workflowAction?.service, - ...this.values.service, - integration_id: newValue, - } + this.bufferServiceChange({ integration_id: newValue }) }, }, databases() { @@ -93,5 +89,24 @@ export default { ) }, }, + methods: { + /** + * Records an editable service change and rebuilds `values.service` from the + * latest server service plus the accumulated changes. Buffering the changes + * keeps a follow-up edit (or a freshly picked integration) that lands before + * the debounced save completes, while the read-only fields are always taken + * fresh from `workflowAction.service` instead of a stale buffered copy. + */ + bufferServiceChange(changes) { + this.pendingServiceChanges = { + ...this.pendingServiceChanges, + ...changes, + } + this.values.service = { + ...this.workflowAction?.service, + ...this.pendingServiceChanges, + } + }, + }, } diff --git a/web-frontend/modules/core/components/integrations/IntegrationCreateEditModal.vue b/web-frontend/modules/core/components/integrations/IntegrationCreateEditModal.vue index 98625dba03..1446b9b845 100644 --- a/web-frontend/modules/core/components/integrations/IntegrationCreateEditModal.vue +++ b/web-frontend/modules/core/components/integrations/IntegrationCreateEditModal.vue @@ -45,6 +45,7 @@ import error from '@baserow/modules/core/mixins/error' import modal from '@baserow/modules/core/mixins/modal' import IntegrationEditForm from '@baserow/modules/core/components/integrations/IntegrationEditForm' import { getNextAvailableNameInSequence } from '@baserow/modules/core/utils/string' +import { ResponseErrorMessage } from '@baserow/modules/core/plugins/clientHandler' export default { components: { IntegrationEditForm }, @@ -124,7 +125,12 @@ export default { } this.hide() } catch (error) { - this.handleError(error) + this.handleError(error, 'integration', { + ERROR_INTEGRATION_CREDENTIAL_REQUIRED: new ResponseErrorMessage( + this.$t('integrationCreateEditModal.credentialRequiredTitle'), + this.$t('integrationCreateEditModal.credentialRequiredMessage') + ), + }) } this.loading = false }, diff --git a/web-frontend/modules/core/locales/en.json b/web-frontend/modules/core/locales/en.json index d2f2f97898..77d5c0ddd3 100644 --- a/web-frontend/modules/core/locales/en.json +++ b/web-frontend/modules/core/locales/en.json @@ -858,7 +858,9 @@ "integrationCreateEditModal": { "createTitle": "New integration", "editTitle": "Edit integration", - "warningTitle": "Warning" + "warningTitle": "Warning", + "credentialRequiredTitle": "Credential required", + "credentialRequiredMessage": "Enter the credential again to change where this integration connects." }, "integrationEditForm": { "name": "Name", diff --git a/web-frontend/modules/core/store/integration.js b/web-frontend/modules/core/store/integration.js index 4d24d60150..dd7b58d8ff 100644 --- a/web-frontend/modules/core/store/integration.js +++ b/web-frontend/modules/core/store/integration.js @@ -134,7 +134,16 @@ const actions = { }) try { - await IntegrationService($client).update(integration.id, values) + const { data } = await IntegrationService($client).update( + integration.id, + values + ) + // Only the response carries the updated `has_*` credential flags. + await dispatch('forceUpdate', { + application, + integration, + values: data, + }) } catch (error) { await dispatch('forceUpdate', { application, diff --git a/web-frontend/modules/database/workflowActionTypes.js b/web-frontend/modules/database/workflowActionTypes.js index 0728cb34e6..7d7cf01cc3 100644 --- a/web-frontend/modules/database/workflowActionTypes.js +++ b/web-frontend/modules/database/workflowActionTypes.js @@ -699,7 +699,8 @@ export class SlackWriteMessageWorkflowActionType extends DatabaseExternalWorkflo if (!bot) { return null } - if (!bot.token) { + // The token is write-only: the list only says whether one is set. + if (!bot.has_token) { return this.app.$i18n.t('databaseWorkflowActionType.slackTokenMissing') } return null diff --git a/web-frontend/modules/integrations/core/components/integrations/SMTPForm.vue b/web-frontend/modules/integrations/core/components/integrations/SMTPForm.vue index 037924fa1c..37d96da686 100644 --- a/web-frontend/modules/integrations/core/components/integrations/SMTPForm.vue +++ b/web-frontend/modules/integrations/core/components/integrations/SMTPForm.vue @@ -49,11 +49,20 @@ /> - + @@ -83,11 +92,31 @@ export default { port: 587, use_tls: true, username: '', - password: '', + // Untouched (`null`) is dropped on submit to keep the stored password; + // an empty string clears it. + password: null, }, allowedValues: ['host', 'port', 'use_tls', 'username', 'password'], } }, + computed: { + hasPassword() { + return this.defaultValues.has_password === true + }, + }, + methods: { + getFormValues(deep = false) { + const values = Object.assign( + {}, + this.values, + this.getChildFormsValues(deep) + ) + if (values.password === null) { + delete values.password + } + return values + }, + }, validations() { return { values: { diff --git a/web-frontend/modules/integrations/core/integrationTypes.js b/web-frontend/modules/integrations/core/integrationTypes.js index f96ca78fe7..1ffb137851 100644 --- a/web-frontend/modules/integrations/core/integrationTypes.js +++ b/web-frontend/modules/integrations/core/integrationTypes.js @@ -30,12 +30,13 @@ export class SMTPIntegrationType extends IntegrationType { } getDefaultValues() { + // No `password`: the form starts it at null to mean "untouched", and a + // default here would overwrite that sentinel on the create path. return { host: '', port: 587, use_tls: true, username: '', - password: '', } } diff --git a/web-frontend/modules/integrations/locales/en.json b/web-frontend/modules/integrations/locales/en.json index 9bc7b3b2df..8951b94529 100644 --- a/web-frontend/modules/integrations/locales/en.json +++ b/web-frontend/modules/integrations/locales/en.json @@ -25,7 +25,7 @@ "slackBotIntegrationType": { "slackBotSummary": "Slack Bot", "slackBotNoToken": "Slack Bot - Not configured", - "slackBotWarning": "Anyone who can build in this application can send messages through this bot, and can read its token through the API. Use a bot whose access you are happy to share." + "slackBotWarning": "Anyone who can build in this application can send messages through this bot, and can replace its token. Use a bot whose access you are happy to share." }, "serviceType": { "localBaserowGetRow": "Get a single row", @@ -277,7 +277,9 @@ "supportPairingHeading": "2. Pairing with your Slack app", "supportPairingStep1": "If your app is new: navigate to 'Settings' > 'Install App'. Click the green button to install the app to your workspace.", "supportPairingStep2": "Copy your 'Bot User OAuth Token' and store it in the 'Bot User Token' field in this form.", - "supportPairingStep3": "Finally, if your app is new: in Slack, invite your app to your chosen channel with {command}" + "supportPairingStep3": "Finally, if your app is new: in Slack, invite your app to your chosen channel with {command}", + "tokenKeepPlaceholder": "Leave blank to keep the saved token", + "tokenConfigured": "A token is saved. Leave blank to keep it." }, "slackWriteMessageServiceForm": { "alertMessage": "This action must be paired with a Slack app. Please follow the guide in the integrations popup to get started.", @@ -308,7 +310,9 @@ "username": "Username", "usernamePlaceholder": "your-email{'@'}example.com", "password": "Password", - "passwordPlaceholder": "your-password" + "passwordPlaceholder": "your-password", + "passwordConfigured": "A password is saved. Leave blank to keep it.", + "passwordKeepPlaceholder": "Leave blank to keep the saved password" }, "smtpEmailForm": { "smtpConfigurationMode": "SMTP configuration", diff --git a/web-frontend/modules/integrations/slack/components/integrations/SlackBotForm.vue b/web-frontend/modules/integrations/slack/components/integrations/SlackBotForm.vue index 863f2e09d6..f5e182becf 100644 --- a/web-frontend/modules/integrations/slack/components/integrations/SlackBotForm.vue +++ b/web-frontend/modules/integrations/slack/components/integrations/SlackBotForm.vue @@ -1,15 +1,22 @@