diff --git a/api/core/fields.py b/api/core/fields.py index ef07f7caae31..90051714cfd3 100644 --- a/api/core/fields.py +++ b/api/core/fields.py @@ -1,15 +1,40 @@ import base64 import hashlib import json -from typing import Any +from typing import Any, TypeVar import structlog from cryptography.fernet import Fernet, InvalidToken from django.conf import settings from django.db import models +from core.validators import validate_http_url_scheme, validate_no_internal_address + logger = structlog.get_logger("core") +_ST = TypeVar("_ST") +_GT = TypeVar("_GT") + + +class NoSSRFURLField(models.URLField[_ST, _GT]): + """ + A URL field restricted to http(s) URLs that do not resolve to internal or + private network addresses. + + DRF copies these validators onto the `ModelSerializer` field it builds, so + any serialiser over a model using this field validates the URL on input. + Use `webhooks.fields.NoSSRFURLField` for serialisers not backed by a model. + + Kept generic so django-stubs can still derive nullability from `null=`; + subclassing `models.URLField` unparameterised resolves every usage to `Any`. + """ + + default_validators = [ + *models.URLField.default_validators, + validate_http_url_scheme, + validate_no_internal_address, + ] + def _get_fernet() -> Fernet: secret: str = settings.WAREHOUSE_CREDENTIALS_SECRET diff --git a/api/core/validators.py b/api/core/validators.py new file mode 100644 index 000000000000..8a4d973e021a --- /dev/null +++ b/api/core/validators.py @@ -0,0 +1,31 @@ +from urllib.parse import urlparse + +from django.core.exceptions import ValidationError + +from core.network import is_internal_address + +ALLOWED_URL_SCHEMES = ("http", "https") + + +def validate_http_url_scheme(value: str) -> None: + """ + Restrict a URL to http(s). Django's `URLValidator` also allows ftp(s). + + Deliberately not a `URLValidator` subclass: DRF drops any `URLValidator` + it copies from a model field onto a `ModelSerializer` field, replacing it + with its own scheme-permissive one. + """ + if urlparse(value).scheme not in ALLOWED_URL_SCHEMES: + raise ValidationError("Enter a valid http(s) URL.", code="invalid") + + +def validate_no_internal_address(value: str) -> None: + """ + Reject URLs that target, or resolve to, an internal or private network + address, preventing Server-Side Request Forgery (SSRF). + """ + if is_internal_address(urlparse(value).hostname or ""): + raise ValidationError( + "URLs must not target internal or private network addresses.", + code="internal_address", + ) diff --git a/api/environments/migrations/0039_use_no_ssrf_url_field.py b/api/environments/migrations/0039_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..74234e3b4420 --- /dev/null +++ b/api/environments/migrations/0039_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("environments", "0038_add_first_evaluated_fields"), + ] + + operations = [ + migrations.AlterField( + model_name="webhook", + name="url", + field=core.fields.NoSSRFURLField(), + ), + ] diff --git a/api/environments/serializers.py b/api/environments/serializers.py index 303a51e67ed6..448c8d261e46 100644 --- a/api/environments/serializers.py +++ b/api/environments/serializers.py @@ -14,7 +14,6 @@ from util.drf_writable_nested.serializers import ( DeleteBeforeUpdateWritableNestedModelSerializer, ) -from webhooks.fields import NoSSRFURLField class EnvironmentSerializerFull(serializers.ModelSerializer): # type: ignore[type-arg] @@ -186,8 +185,6 @@ def create(self, validated_data): # type: ignore[no-untyped-def] class WebhookSerializer(serializers.ModelSerializer): # type: ignore[type-arg] - url = NoSSRFURLField() - class Meta: model = Webhook fields = ("id", "url", "enabled", "created_at", "updated_at", "secret") diff --git a/api/features/feature_external_resources/models.py b/api/features/feature_external_resources/models.py index 32dff039a183..f80adbf97fc6 100644 --- a/api/features/feature_external_resources/models.py +++ b/api/features/feature_external_resources/models.py @@ -53,6 +53,9 @@ class ResourceType(models.TextChoices): class FeatureExternalResource(LifecycleModelMixin, models.Model): # type: ignore[misc] + # Deliberately not a `NoSSRFURLField`: this URL is never fetched, only parsed + # for its path segments, with the request itself going to `GITHUB_API_URL`. + # Restricting it would break self-hosted GitHub instances on internal hosts. url = models.URLField() type = models.CharField(max_length=20, choices=ResourceType.choices) diff --git a/api/integrations/amplitude/migrations/0007_use_no_ssrf_url_field.py b/api/integrations/amplitude/migrations/0007_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..86f5ea8760a6 --- /dev/null +++ b/api/integrations/amplitude/migrations/0007_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("amplitude", "0006_add_default_base_url"), + ] + + operations = [ + migrations.AlterField( + model_name="amplitudeconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(default="https://api2.amplitude.com"), + ), + ] diff --git a/api/integrations/amplitude/models.py b/api/integrations/amplitude/models.py index d9745c156b2c..54dcd249468e 100644 --- a/api/integrations/amplitude/models.py +++ b/api/integrations/amplitude/models.py @@ -1,12 +1,13 @@ from django.db import models +from core.fields import NoSSRFURLField from environments.models import Environment from integrations.amplitude.constants import DEFAULT_AMPLITUDE_API_URL from integrations.common.models import EnvironmentIntegrationModel class AmplitudeConfiguration(EnvironmentIntegrationModel): - base_url = models.URLField(default=DEFAULT_AMPLITUDE_API_URL) + base_url = NoSSRFURLField(default=DEFAULT_AMPLITUDE_API_URL) environment = models.OneToOneField( Environment, related_name="amplitude_config", on_delete=models.CASCADE ) diff --git a/api/integrations/common/models.py b/api/integrations/common/models.py index 87c323ea59b4..7c35ae78e55a 100644 --- a/api/integrations/common/models.py +++ b/api/integrations/common/models.py @@ -8,6 +8,7 @@ hook, ) +from core.fields import NoSSRFURLField from core.models import SoftDeleteExportableModel from environments.models import Environment @@ -15,7 +16,7 @@ class IntegrationsModel(SoftDeleteExportableModel): - base_url = models.URLField(blank=False, null=True) + base_url = NoSSRFURLField(blank=False, null=True) api_key = models.CharField(max_length=100, blank=False, null=False) class Meta: diff --git a/api/integrations/datadog/migrations/0005_use_no_ssrf_url_field.py b/api/integrations/datadog/migrations/0005_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..512f4533a450 --- /dev/null +++ b/api/integrations/datadog/migrations/0005_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("datadog", "0004_add_use_custom_source"), + ] + + operations = [ + migrations.AlterField( + model_name="datadogconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(), + ), + ] diff --git a/api/integrations/datadog/models.py b/api/integrations/datadog/models.py index 396255b891e6..d524bbb846e9 100644 --- a/api/integrations/datadog/models.py +++ b/api/integrations/datadog/models.py @@ -2,6 +2,7 @@ from django.db import models +from core.fields import NoSSRFURLField from integrations.common.models import IntegrationsModel from projects.models import Project @@ -12,6 +13,6 @@ class DataDogConfiguration(IntegrationsModel): project = models.OneToOneField( Project, on_delete=models.CASCADE, related_name="data_dog_config" ) - base_url = models.URLField(blank=False, null=False) + base_url = NoSSRFURLField(blank=False, null=False) use_custom_source = models.BooleanField(default=False) diff --git a/api/integrations/dynatrace/migrations/0004_use_no_ssrf_url_field.py b/api/integrations/dynatrace/migrations/0004_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..c624bfa154fe --- /dev/null +++ b/api/integrations/dynatrace/migrations/0004_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("dynatrace", "0003_dynatraceconfiguration_deleted_at"), + ] + + operations = [ + migrations.AlterField( + model_name="dynatraceconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(null=True), + ), + ] diff --git a/api/integrations/gitlab/migrations/0004_use_no_ssrf_url_field.py b/api/integrations/gitlab/migrations/0004_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..7b42d86cca70 --- /dev/null +++ b/api/integrations/gitlab/migrations/0004_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("gitlab", "0003_gitlabconfiguration_labeling_enabled"), + ] + + operations = [ + migrations.AlterField( + model_name="gitlabconfiguration", + name="gitlab_instance_url", + field=core.fields.NoSSRFURLField(), + ), + ] diff --git a/api/integrations/gitlab/models.py b/api/integrations/gitlab/models.py index 1d4708f7f209..5d4a224f119a 100644 --- a/api/integrations/gitlab/models.py +++ b/api/integrations/gitlab/models.py @@ -1,5 +1,6 @@ from django.db import models +from core.fields import NoSSRFURLField from core.models import SoftDeleteExportableModel @@ -9,7 +10,7 @@ class GitLabConfiguration(SoftDeleteExportableModel): on_delete=models.CASCADE, related_name="gitlab_config", ) - gitlab_instance_url = models.URLField(max_length=200) + gitlab_instance_url = NoSSRFURLField(max_length=200) access_token = models.CharField(max_length=300) labeling_enabled = models.BooleanField(default=False) diff --git a/api/integrations/grafana/migrations/0003_use_no_ssrf_url_field.py b/api/integrations/grafana/migrations/0003_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..b33ded4b7918 --- /dev/null +++ b/api/integrations/grafana/migrations/0003_use_no_ssrf_url_field.py @@ -0,0 +1,24 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("grafana", "0002_add_grafana_organisation_configuration"), + ] + + operations = [ + migrations.AlterField( + model_name="grafanaorganisationconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(null=True), + ), + migrations.AlterField( + model_name="grafanaprojectconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(null=True), + ), + ] diff --git a/api/integrations/heap/migrations/0004_use_no_ssrf_url_field.py b/api/integrations/heap/migrations/0004_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..31c0cba90ca2 --- /dev/null +++ b/api/integrations/heap/migrations/0004_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("heap", "0003_heapconfiguration_deleted_at"), + ] + + operations = [ + migrations.AlterField( + model_name="heapconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(null=True), + ), + ] diff --git a/api/integrations/mixpanel/migrations/0004_use_no_ssrf_url_field.py b/api/integrations/mixpanel/migrations/0004_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..4a7198773d82 --- /dev/null +++ b/api/integrations/mixpanel/migrations/0004_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("mixpanel", "0003_mixpanelconfiguration_deleted_at"), + ] + + operations = [ + migrations.AlterField( + model_name="mixpanelconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(null=True), + ), + ] diff --git a/api/integrations/new_relic/migrations/0005_use_no_ssrf_url_field.py b/api/integrations/new_relic/migrations/0005_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..13274acec6e7 --- /dev/null +++ b/api/integrations/new_relic/migrations/0005_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("new_relic", "0004_newrelicconfiguration_deleted_at"), + ] + + operations = [ + migrations.AlterField( + model_name="newrelicconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(null=True), + ), + ] diff --git a/api/integrations/rudderstack/migrations/0004_use_no_ssrf_url_field.py b/api/integrations/rudderstack/migrations/0004_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..81c4729807e0 --- /dev/null +++ b/api/integrations/rudderstack/migrations/0004_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("rudderstack", "0003_rudderstackconfiguration_deleted_at"), + ] + + operations = [ + migrations.AlterField( + model_name="rudderstackconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(null=True), + ), + ] diff --git a/api/integrations/segment/migrations/0007_use_no_ssrf_url_field.py b/api/integrations/segment/migrations/0007_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..0504e6954212 --- /dev/null +++ b/api/integrations/segment/migrations/0007_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("segment", "0006_set_base_url_to_default_again"), + ] + + operations = [ + migrations.AlterField( + model_name="segmentconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(null=True), + ), + ] diff --git a/api/integrations/sentry/migrations/0002_use_no_ssrf_url_field.py b/api/integrations/sentry/migrations/0002_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..7e19193a4a2c --- /dev/null +++ b/api/integrations/sentry/migrations/0002_use_no_ssrf_url_field.py @@ -0,0 +1,24 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("sentry", "0001_sentry_change_tracking"), + ] + + operations = [ + migrations.AlterField( + model_name="sentrychangetrackingconfiguration", + name="base_url", + field=core.fields.NoSSRFURLField(null=True), + ), + migrations.AlterField( + model_name="sentrychangetrackingconfiguration", + name="webhook_url", + field=core.fields.NoSSRFURLField(), + ), + ] diff --git a/api/integrations/sentry/models.py b/api/integrations/sentry/models.py index 16be448312c8..e1fa6af77eab 100644 --- a/api/integrations/sentry/models.py +++ b/api/integrations/sentry/models.py @@ -1,6 +1,7 @@ from django.core import validators from django.db import models +from core.fields import NoSSRFURLField from integrations.common.models import EnvironmentIntegrationModel @@ -17,7 +18,7 @@ class SentryChangeTrackingConfiguration(EnvironmentIntegrationModel): related_name="sentry_change_tracking_configuration", ) - webhook_url = models.URLField( + webhook_url = NoSSRFURLField( max_length=200, ) diff --git a/api/integrations/webhook/migrations/0005_use_no_ssrf_url_field.py b/api/integrations/webhook/migrations/0005_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..068f6ac5721b --- /dev/null +++ b/api/integrations/webhook/migrations/0005_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("webhook", "0004_alter_webhookconfiguration_url"), + ] + + operations = [ + migrations.AlterField( + model_name="webhookconfiguration", + name="url", + field=core.fields.NoSSRFURLField(), + ), + ] diff --git a/api/organisations/migrations/0059_use_no_ssrf_url_field.py b/api/organisations/migrations/0059_use_no_ssrf_url_field.py new file mode 100644 index 000000000000..8468fdd846cf --- /dev/null +++ b/api/organisations/migrations/0059_use_no_ssrf_url_field.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.16 on 2026-08-07 11:54 + +import core.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("organisations", "0058_update_audit_and_history_limits_in_sub_cache"), + ] + + operations = [ + migrations.AlterField( + model_name="organisationwebhook", + name="url", + field=core.fields.NoSSRFURLField(), + ), + ] diff --git a/api/projects/code_references/models.py b/api/projects/code_references/models.py index 21062fc2c277..51d7e09c142f 100644 --- a/api/projects/code_references/models.py +++ b/api/projects/code_references/models.py @@ -17,6 +17,8 @@ class VCSRepository(models.Model): ) # Provider-agnostic URL to the web UI of the repository, e.g. https://github.flagsmith.com/backend/ + # Deliberately not a `NoSSRFURLField`: only ever rendered as a link, never + # fetched server-side, and self-hosted instances live on internal hosts. url = models.URLField() vcs_provider = models.CharField( diff --git a/api/tests/unit/core/test_fields.py b/api/tests/unit/core/test_fields.py index d891fa667a60..e67cc9a81254 100644 --- a/api/tests/unit/core/test_fields.py +++ b/api/tests/unit/core/test_fields.py @@ -1,8 +1,69 @@ import pytest +from django.core.exceptions import ValidationError from pytest_django.fixtures import SettingsWrapper from pytest_structlog import StructuredLogCapture -from core.fields import EncryptedJSONField +from core.fields import EncryptedJSONField, NoSSRFURLField +from integrations.gitlab.serializers import GitLabConfigurationSerializer + + +@pytest.mark.parametrize( + "url,expected_code", + [ + ("http://127.0.0.1/", "internal_address"), + ("ftp://example.com/", "invalid"), + ], +) +def test_no_ssrf_url_field__unsafe_url__raises_validation_error( # noqa: FT004 + url: str, + expected_code: str, +) -> None: + # Given + field: NoSSRFURLField[str, str] = NoSSRFURLField() + + # When / Then + with pytest.raises(ValidationError) as exc_info: + field.run_validators(url) + + assert exc_info.value.error_list[0].code == expected_code + + +def test_no_ssrf_url_field__model_serializer__validates_internal_address() -> None: + # Given — `GitLabConfiguration.gitlab_instance_url` is a `NoSSRFURLField`, + # so DRF copies its validators onto the field it builds for the serialiser + # without the serialiser having to declare one. + serializer = GitLabConfigurationSerializer( + data={ + "gitlab_instance_url": "http://127.0.0.1/", + "access_token": "glpat-xxxxxxxxxxxxxxxxxxxx", + } + ) + + # When + is_valid = serializer.is_valid() + + # Then + assert is_valid is False + assert "internal_address" in str(serializer.errors["gitlab_instance_url"]) + + +def test_no_ssrf_url_field__model_serializer__rejects_non_http_scheme() -> None: + # Given — DRF discards the `URLValidator` it copies from the model field and + # substitutes its own, which permits ftp(s). The scheme check therefore lives + # in a plain function validator, which DRF leaves alone. Restoring it to a + # `URLValidator` on the field would let this through. + serializer = GitLabConfigurationSerializer( + data={ + "gitlab_instance_url": "ftp://example.com/", + "access_token": "glpat-xxxxxxxxxxxxxxxxxxxx", + } + ) + + # When + is_valid = serializer.is_valid() + + # Then + assert is_valid is False def test_get_prep_value__json_value__returns_ciphertext_that_roundtrips() -> None: diff --git a/api/tests/unit/integrations/datadog/test_unit_datadog_views.py b/api/tests/unit/integrations/datadog/test_unit_datadog_views.py index 87a532da23e0..94c1d5120227 100644 --- a/api/tests/unit/integrations/datadog/test_unit_datadog_views.py +++ b/api/tests/unit/integrations/datadog/test_unit_datadog_views.py @@ -177,3 +177,27 @@ def test_datadog_project_view__no_permissions__return_expected( # Then assert response.status_code == status.HTTP_403_FORBIDDEN assert not DataDogConfiguration.objects.filter(project=project).exists() + + +def test_datadog_config__private_ip_base_url__returns_bad_request( + admin_client: APIClient, + project: Project, +) -> None: + # Given + data = { + "base_url": "http://169.254.169.254/", + "api_key": "abc-123", + "use_custom_source": True, + } + url = reverse("api-v1:projects:integrations-datadog-list", args=[project.id]) + + # When + response = admin_client.post( + url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not DataDogConfiguration.objects.filter(project=project).exists() diff --git a/api/tests/unit/integrations/dynatrace/test_unit_dynatrace_views.py b/api/tests/unit/integrations/dynatrace/test_unit_dynatrace_views.py index e752bfeb3e01..519adf0e4eb9 100644 --- a/api/tests/unit/integrations/dynatrace/test_unit_dynatrace_views.py +++ b/api/tests/unit/integrations/dynatrace/test_unit_dynatrace_views.py @@ -171,3 +171,30 @@ def test_dynatrace_environment_view__no_permissions__return_expected( # Then assert response.status_code == status.HTTP_403_FORBIDDEN assert not DynatraceConfiguration.objects.filter(environment=environment).exists() + + +def test_create_dynatrace_config__private_ip_base_url__returns_bad_request( + admin_client: APIClient, + environment: Environment, +) -> None: + # Given + data = { + "base_url": "http://127.0.0.1/", + "api_key": "abc-123", + "entity_selector": "type(APPLICATION),entityName(docs)", + } + url = reverse( + "api-v1:environments:integrations-dynatrace-list", + args=[environment.api_key], + ) + + # When + response = admin_client.post( + url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not DynatraceConfiguration.objects.filter(environment=environment).exists() diff --git a/api/tests/unit/integrations/gitlab/test_configuration.py b/api/tests/unit/integrations/gitlab/test_configuration.py index c66b77794eca..f0cf31b93d77 100644 --- a/api/tests/unit/integrations/gitlab/test_configuration.py +++ b/api/tests/unit/integrations/gitlab/test_configuration.py @@ -315,3 +315,22 @@ def test_delete_configuration__non_admin__returns_403( # Then assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_create_configuration__private_ip_instance_url__returns_400( + admin_client_new: APIClient, + project: Project, +) -> None: + # Given / When + response = admin_client_new.post( + f"/api/v1/projects/{project.id}/integrations/gitlab/", + data={ + "gitlab_instance_url": "http://127.0.0.1/", + "access_token": "glpat-xxxxxxxxxxxxxxxxxxxx", + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not GitLabConfiguration.objects.filter(project=project).exists() diff --git a/api/tests/unit/integrations/new_relic/test_unit_new_relic_views.py b/api/tests/unit/integrations/new_relic/test_unit_new_relic_views.py index ed9a962a73e9..266fc95a04cc 100644 --- a/api/tests/unit/integrations/new_relic/test_unit_new_relic_views.py +++ b/api/tests/unit/integrations/new_relic/test_unit_new_relic_views.py @@ -173,3 +173,27 @@ def test_new_relic_config__project_with_deleted_config__creates_new_configuratio assert response_json["api_key"] == api_key assert response_json["base_url"] == base_url assert response_json["app_id"] == app_id + + +def test_new_relic_config__private_ip_base_url__returns_bad_request( + admin_client: APIClient, + project: Project, +) -> None: + # Given + data = { + "base_url": "http://127.0.0.1/", + "api_key": "key-123", + "app_id": "app-123", + } + url = reverse("api-v1:projects:integrations-new-relic-list", args=[project.id]) + + # When + response = admin_client.post( + url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not NewRelicConfiguration.objects.filter(project=project).exists() diff --git a/api/tests/unit/integrations/sentry/test_unit_sentry_views.py b/api/tests/unit/integrations/sentry/test_unit_sentry_views.py index 520591190633..15cf7670b985 100644 --- a/api/tests/unit/integrations/sentry/test_unit_sentry_views.py +++ b/api/tests/unit/integrations/sentry/test_unit_sentry_views.py @@ -106,3 +106,24 @@ def test_sentry_change_tracking_setup__invalid_payload__rejects_with_400( # Then assert response.status_code == 400 assert response.json() == errors + + +def test_sentry_change_tracking_setup__private_ip_webhook_url__rejects_with_400( + admin_client: APIClient, + environment: Environment, +) -> None: + # Given + url = f"/api/v1/environments/{environment.api_key}/integrations/sentry/" + payload = { + "webhook_url": "http://127.0.0.1/webhook", + "secret": "hush hush!", + } + + # When + response = admin_client.post(url, payload, format="json") + + # Then + assert response.status_code == 400 + assert not SentryChangeTrackingConfiguration.objects.filter( + environment=environment + ).exists() diff --git a/api/tests/unit/integrations/webhook/test_unit_webhook_views.py b/api/tests/unit/integrations/webhook/test_unit_webhook_views.py index 312f54ef26b3..64bda29e22da 100644 --- a/api/tests/unit/integrations/webhook/test_unit_webhook_views.py +++ b/api/tests/unit/integrations/webhook/test_unit_webhook_views.py @@ -116,3 +116,25 @@ def test_delete_webhook_config__existing_config__returns_204( # type: ignore[no # Then assert res.status_code == status.HTTP_204_NO_CONTENT assert not WebhookConfiguration.objects.filter(environment=environment).exists() + + +def test_create_webhook_config__private_ip_url__returns_400( # type: ignore[no-untyped-def] + admin_client, organisation, environment +): + # Given + url = reverse( + "api-v1:environments:integrations-webhook-list", + args=[environment.api_key], + ) + data = {"url": "http://127.0.0.1/webhooks", "secret": "random_secret"} + + # When + response = admin_client.post( + url, + data=json.dumps(data), + content_type="application/json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not WebhookConfiguration.objects.filter(environment=environment).exists() diff --git a/api/tests/unit/organisations/test_unit_organisations_serializers.py b/api/tests/unit/organisations/test_unit_organisations_serializers.py index d9033659f800..1486fc532db5 100644 --- a/api/tests/unit/organisations/test_unit_organisations_serializers.py +++ b/api/tests/unit/organisations/test_unit_organisations_serializers.py @@ -1,8 +1,15 @@ +import socket +from unittest import mock + +import pytest from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture from organisations.models import Organisation -from organisations.serializers import UpdateSubscriptionSerializer +from organisations.serializers import ( + OrganisationWebhookSerializer, + UpdateSubscriptionSerializer, +) def test_update_subscription_serializer__create__updates_subscription( @@ -39,3 +46,66 @@ def test_update_subscription_serializer__create__updates_subscription( organisation.subscription.refresh_from_db() assert organisation.subscription.subscription_id == "new-sub-id" assert organisation.subscription.plan == "startup-v2" + + +def test_organisation_webhook_serializer__private_ip__is_invalid() -> None: + # Given + serializer = OrganisationWebhookSerializer(data={"url": "http://127.0.0.1/hook"}) + + # When + is_valid = serializer.is_valid() + + # Then + assert is_valid is False + assert "internal_address" in str(serializer.errors["url"]) + + +def test_organisation_webhook_serializer__hostname_resolving_to_private_ip__is_invalid() -> ( # noqa: E501 + None +): + # Given — a hostname that resolves to an RFC1918 address + serializer = OrganisationWebhookSerializer( + data={"url": "http://internal.example.com/hook"} + ) + + # When + with mock.patch( + "core.network.socket.getaddrinfo", + return_value=[(socket.AF_INET, None, None, None, ("10.0.0.5", 0))], + ): + is_valid = serializer.is_valid() + + # Then + assert is_valid is False + assert "internal_address" in str(serializer.errors["url"]) + + +def test_organisation_webhook_serializer__non_http_scheme__is_invalid() -> None: + # Given + serializer = OrganisationWebhookSerializer(data={"url": "ftp://example.com/hook"}) + + # When + is_valid = serializer.is_valid() + + # Then + assert is_valid is False + assert "url" in serializer.errors + + +@pytest.mark.parametrize( + "url", + ["https://example.com/hook", "http://8.8.8.8/hook"], +) +def test_organisation_webhook_serializer__public_url__is_valid(url: str) -> None: + # Given + serializer = OrganisationWebhookSerializer(data={"url": url}) + + # When + with mock.patch( + "core.network.socket.getaddrinfo", + return_value=[(socket.AF_INET, None, None, None, ("8.8.8.8", 0))], + ): + is_valid = serializer.is_valid() + + # Then + assert is_valid is True diff --git a/api/tests/unit/webhooks/test_webhooks_fields.py b/api/tests/unit/webhooks/test_webhooks_fields.py index 5706a8337279..dd1d54a6fe4d 100644 --- a/api/tests/unit/webhooks/test_webhooks_fields.py +++ b/api/tests/unit/webhooks/test_webhooks_fields.py @@ -97,7 +97,11 @@ def test_no_ssrf_url_field__public_address__returns_value( label: str, ) -> None: # Given / When - result = field.run_validation(url) + with mock.patch( + "core.network.socket.getaddrinfo", + return_value=[(socket.AF_INET, None, None, None, ("8.8.8.8", 0))], + ): + result = field.run_validation(url) # Then assert result == url @@ -116,3 +120,24 @@ def test_no_ssrf_url_field__unresolvable_hostname__returns_value( # Then assert result == "https://unresolvable.example.com/hook" + + +@pytest.mark.parametrize( + "url,label", + [ + ("ftp://example.com/hook", "ftp"), + ("ftps://example.com/hook", "ftps"), + ("file:///etc/passwd", "file"), + ("gopher://example.com/hook", "gopher"), + ], +) +def test_no_ssrf_url_field__non_http_scheme__raises_validation_error( # noqa: FT004 + field: NoSSRFURLField, + url: str, + label: str, +) -> None: + # Given / When / Then + with pytest.raises(ValidationError) as exc_info: + field.run_validation(url) + + assert "invalid" in str(exc_info.value.detail) diff --git a/api/webhooks/fields.py b/api/webhooks/fields.py index 13ffe8fa8b9b..82f3afd5368f 100644 --- a/api/webhooks/fields.py +++ b/api/webhooks/fields.py @@ -1,31 +1,20 @@ -from urllib.parse import urlparse +from typing import Any from rest_framework import serializers -from core.network import is_internal_address +from core.validators import validate_http_url_scheme, validate_no_internal_address class NoSSRFURLField(serializers.URLField): """ - A URL field that rejects URLs resolving to internal network addresses, - preventing Server-Side Request Forgery (SSRF) attacks. + A URL field that only allows http(s) URLs and rejects URLs resolving to + internal network addresses, preventing Server-Side Request Forgery + (SSRF) attacks. - Blocks loopback (127.0.0.0/8, ::1), RFC 1918 private ranges - (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), link-local - (169.254.0.0/16, fe80::/10), and other reserved/multicast ranges. - Hostnames are resolved to their IP address before checking. + For serialisers not backed by a model. Model-backed serialisers get the + same validation from `core.fields.NoSSRFURLField`. """ - default_error_messages = { - **serializers.URLField.default_error_messages, - "internal_address": ( - "Webhook URLs must not target internal or private network addresses." - ), - } - - def run_validators(self, value: str) -> None: - super().run_validators(value) - - hostname = urlparse(value).hostname or "" - if is_internal_address(hostname): - self.fail("internal_address") + def __init__(self, **kwargs: Any) -> None: + super().__init__(**kwargs) + self.validators += [validate_http_url_scheme, validate_no_internal_address] diff --git a/api/webhooks/models.py b/api/webhooks/models.py index 0ee206d37c8d..760643509140 100644 --- a/api/webhooks/models.py +++ b/api/webhooks/models.py @@ -1,10 +1,11 @@ from django.db import models +from core.fields import NoSSRFURLField from core.models import AbstractBaseExportableModel, SoftDeleteExportableModel class AbstractBaseWebhookModel(models.Model): - url = models.CharField(max_length=200) + url = NoSSRFURLField(max_length=200) secret = models.CharField(max_length=255, blank=True) class Meta: