From c77dde905605ab6dfa886e1fda4f7ff994ed3afb Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 20 May 2026 09:35:28 +0100 Subject: [PATCH 1/8] Hide exception details from webhook test response --- api/webhooks/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/webhooks/views.py b/api/webhooks/views.py index 4b8946f82b45..26826b50fa8a 100644 --- a/api/webhooks/views.py +++ b/api/webhooks/views.py @@ -63,11 +63,11 @@ def test(self, request: Request) -> Response: }, status=status.HTTP_200_OK, ) - except requests.exceptions.RequestException as e: + except requests.exceptions.RequestException: return Response( { "detail": "Could not connect to webhook URL", - "body": str(e), + "body": "Please check the URL, and ensure it is valid and accessible from the server.", }, status=status.HTTP_400_BAD_REQUEST, ) From fa80188a02189edd881418fa5a2b5f22272747c7 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 20 May 2026 09:57:29 +0100 Subject: [PATCH 2/8] Update unit test --- api/tests/unit/webhooks/test_unit_webhooks.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/api/tests/unit/webhooks/test_unit_webhooks.py b/api/tests/unit/webhooks/test_unit_webhooks.py index 69f9e7e5e674..408f7b4c0c0b 100644 --- a/api/tests/unit/webhooks/test_unit_webhooks.py +++ b/api/tests/unit/webhooks/test_unit_webhooks.py @@ -504,7 +504,9 @@ def test_send_test_webhook__request_exception__returns_error_response( # Given webhook_url = "http://test.webhook.com" mock_post = mocker.patch("requests.post") - mock_post.side_effect = requests.exceptions.RequestException("Connection refused") + mock_post.side_effect = requests.exceptions.RequestException( + "Some internal exception details that should not be exposed!" + ) url = reverse("api-v1:webhooks:webhooks-test") @@ -523,7 +525,7 @@ def test_send_test_webhook__request_exception__returns_error_response( assert response.status_code == status.HTTP_400_BAD_REQUEST assert response.json() == { "detail": "Could not connect to webhook URL", - "body": "Connection refused", + "body": "Please check the URL, and ensure it is valid and accessible from the server.", } From 0a05e843a10468a001750f7a852f2b4f337c5b08 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 20 May 2026 12:52:50 +0100 Subject: [PATCH 3/8] Conceal all non-200 response bodies --- api/tests/unit/webhooks/test_unit_webhooks.py | 5 ++++- api/webhooks/views.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/api/tests/unit/webhooks/test_unit_webhooks.py b/api/tests/unit/webhooks/test_unit_webhooks.py index 408f7b4c0c0b..523bcd8e3f68 100644 --- a/api/tests/unit/webhooks/test_unit_webhooks.py +++ b/api/tests/unit/webhooks/test_unit_webhooks.py @@ -427,7 +427,10 @@ def test_send_test_webhook__various_status_codes__returns_correct_response( # Then assert response.status_code == expected_final_status mock_post.assert_called_once() - assert response.json() == expected_response_body + assert ( + response.json() + == "Please check the webhook endpoint to validate it returns a 200 OK." + ) @pytest.mark.parametrize( diff --git a/api/webhooks/views.py b/api/webhooks/views.py index 26826b50fa8a..cfb756537eb2 100644 --- a/api/webhooks/views.py +++ b/api/webhooks/views.py @@ -51,7 +51,7 @@ def test(self, request: Request) -> Response: return Response( { "detail": "Webhook returned invalid status", - "body": response.text, + "body": "Please check the webhook endpoint to validate it returns a 200 OK.", "status": response.status_code, }, status=status.HTTP_400_BAD_REQUEST, From b3bdf88282a1b7073542b12a9375a4ba597adc42 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 20 May 2026 16:24:59 +0100 Subject: [PATCH 4/8] Fix tests --- api/tests/unit/webhooks/test_unit_webhooks.py | 81 +++++++++++-------- 1 file changed, 46 insertions(+), 35 deletions(-) diff --git a/api/tests/unit/webhooks/test_unit_webhooks.py b/api/tests/unit/webhooks/test_unit_webhooks.py index 523bcd8e3f68..73038bc17d91 100644 --- a/api/tests/unit/webhooks/test_unit_webhooks.py +++ b/api/tests/unit/webhooks/test_unit_webhooks.py @@ -358,50 +358,58 @@ def test_call_integration_webhook__backoff_give_up__does_not_raise_error( assert result is None +def test_send_test_webhook__200_response_from_webhook__returns_correct_response( + mocker: MockerFixture, + admin_client: APIClient, + external_api_response_status: int, + expected_final_status: int, + external_api_error_text: str, + organisation: Organisation, +) -> None: + # Given + webhook_url = "http://test.webhook.com" + mock_post = mocker.patch("requests.post") + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.text = "success" + mock_post.return_value = mock_response + + url = reverse("api-v1:webhooks:webhooks-test") + + data = { + "webhook_url": webhook_url, + "secret": "some-secret", + "scope": {"type": "organisation", "id": organisation.id}, + } + + # When + response = admin_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == expected_final_status + mock_post.assert_called_once() + response_json = response.json() + assert response_json["status"] == 200 + assert response_json["detail"] == "Webhook test successful" + + @pytest.mark.parametrize( - "external_api_response_status, external_api_error_text, expected_final_status, expected_response_body", + "external_api_response_status, external_api_error_text, expected_final_status", [ - (200, "", 200, {"detail": "Webhook test successful", "status": 200}), - ( - 400, - "wrong-payload", - 400, - { - "detail": "Webhook returned invalid status", - "status": 400, - "body": "wrong-payload", - }, - ), - ( - 401, - "invalid-signature", - 400, - { - "detail": "Webhook returned invalid status", - "status": 401, - "body": "invalid-signature", - }, - ), - ( - 500, - "internal-server-error", - 400, - { - "detail": "Webhook returned invalid status", - "status": 500, - "body": "internal-server-error", - }, - ), + (400, "wrong-payload", 400), + (401, "invalid-signature", 400), + (500, "internal-server-error", 400), ], ) -def test_send_test_webhook__various_status_codes__returns_correct_response( +def test_send_test_webhook__various_error_status_codes__returns_correct_response( mocker: MockerFixture, admin_client: APIClient, external_api_response_status: int, expected_final_status: int, external_api_error_text: str, organisation: Organisation, - expected_response_body: dict[str, str | int], ) -> None: # Given webhook_url = "http://test.webhook.com" @@ -427,8 +435,11 @@ def test_send_test_webhook__various_status_codes__returns_correct_response( # Then assert response.status_code == expected_final_status mock_post.assert_called_once() + response_json = response.json() + assert response_json["status"] == external_api_response_status + assert response_json["detail"] == "Webhook returned invalid status" assert ( - response.json() + response_json["body"] == "Please check the webhook endpoint to validate it returns a 200 OK." ) From d31d3393f20db33eeba1717898c43154c8646109 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 20 May 2026 16:35:31 +0100 Subject: [PATCH 5/8] Restrict to valid public URLs --- api/environments/serializers.py | 3 + .../unit/webhooks/test_webhooks_fields.py | 103 ++++++++++++++++++ api/webhooks/fields.py | 48 ++++++++ api/webhooks/serializers.py | 6 +- 4 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 api/tests/unit/webhooks/test_webhooks_fields.py create mode 100644 api/webhooks/fields.py diff --git a/api/environments/serializers.py b/api/environments/serializers.py index 448c8d261e46..303a51e67ed6 100644 --- a/api/environments/serializers.py +++ b/api/environments/serializers.py @@ -14,6 +14,7 @@ from util.drf_writable_nested.serializers import ( DeleteBeforeUpdateWritableNestedModelSerializer, ) +from webhooks.fields import NoSSRFURLField class EnvironmentSerializerFull(serializers.ModelSerializer): # type: ignore[type-arg] @@ -185,6 +186,8 @@ 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/tests/unit/webhooks/test_webhooks_fields.py b/api/tests/unit/webhooks/test_webhooks_fields.py new file mode 100644 index 000000000000..ca6b01d711ff --- /dev/null +++ b/api/tests/unit/webhooks/test_webhooks_fields.py @@ -0,0 +1,103 @@ +import socket +from unittest import mock + +import pytest +from rest_framework.exceptions import ValidationError + +from webhooks.fields import NoSSRFURLField + + +@pytest.fixture() +def field() -> NoSSRFURLField: + return NoSSRFURLField() + + +@pytest.mark.parametrize( + "url,label", + [ + ("http://127.0.0.1/hook", "loopback IPv4"), + ("http://127.0.0.2/hook", "loopback IPv4 (non-first)"), + ("http://[::1]/hook", "loopback IPv6"), + ("http://10.0.0.1/hook", "RFC1918 10.0.0.0/8"), + ("http://10.255.255.255/hook", "RFC1918 10.0.0.0/8 boundary"), + ("http://172.16.0.1/hook", "RFC1918 172.16.0.0/12"), + ("http://172.31.255.255/hook", "RFC1918 172.16.0.0/12 boundary"), + ("http://192.168.0.1/hook", "RFC1918 192.168.0.0/16"), + ("http://192.168.255.255/hook", "RFC1918 192.168.0.0/16 boundary"), + ("http://169.254.1.1/hook", "link-local IPv4"), + ("http://[fe80::1]/hook", "link-local IPv6"), + ("http://224.0.0.1/hook", "multicast"), + ("http://240.0.0.1/hook", "reserved"), + ], +) +def test_no_ssrf_url_field__internal_ip__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 "internal_address" in str(exc_info.value.detail) + + +def test_no_ssrf_url_field__localhost_hostname__raises_validation_error( + field: NoSSRFURLField, +) -> None: + # Given — localhost resolves to 127.0.0.1 + # When / Then + with pytest.raises(ValidationError) as exc_info: + field.run_validation("http://localhost/hook") + + assert "internal_address" in str(exc_info.value.detail) + + +def test_no_ssrf_url_field__hostname_resolving_to_private_ip__raises_validation_error( + field: NoSSRFURLField, +) -> None: + # Given — a hostname that resolves to an RFC1918 address + with mock.patch( + "webhooks.fields.socket.gethostbyname", + return_value="192.168.1.100", + ): + # When / Then + with pytest.raises(ValidationError) as exc_info: + field.run_validation("http://internal.example.com/hook") + + assert "internal_address" in str(exc_info.value.detail) + + +@pytest.mark.parametrize( + "url,label", + [ + ("https://example.com/hook", "public hostname"), + ("https://hooks.example.org/path?foo=bar", "public hostname with path"), + ("http://8.8.8.8/hook", "public IPv4"), + ], +) +def test_no_ssrf_url_field__public_address__returns_value( + field: NoSSRFURLField, + url: str, + label: str, +) -> None: + # Given / When + result = field.run_validation(url) + + # Then + assert result == url + + +def test_no_ssrf_url_field__unresolvable_hostname__returns_value( + field: NoSSRFURLField, +) -> None: + # Given — the hostname cannot be resolved; URL format is still valid + with mock.patch( + "webhooks.fields.socket.gethostbyname", + side_effect=socket.gaierror, + ): + # When + result = field.run_validation("https://unresolvable.example.com/hook") + + # Then + assert result == "https://unresolvable.example.com/hook" diff --git a/api/webhooks/fields.py b/api/webhooks/fields.py new file mode 100644 index 000000000000..e48c5038799a --- /dev/null +++ b/api/webhooks/fields.py @@ -0,0 +1,48 @@ +import ipaddress +import socket +from urllib.parse import urlparse + +from rest_framework import serializers + + +class NoSSRFURLField(serializers.URLField): + """ + A URL field that 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. + """ + + 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 "" + + try: + ip = ipaddress.ip_address(hostname) + except ValueError: + # hostname is a name rather than a literal IP — resolve it. + try: + ip = ipaddress.ip_address(socket.gethostbyname(hostname)) + except socket.gaierror: + # Unresolvable hostname; leave it to the URL validator. + return + + if ( + ip.is_loopback + or ip.is_private + or ip.is_link_local + or ip.is_reserved + or ip.is_multicast + ): + self.fail("internal_address") diff --git a/api/webhooks/serializers.py b/api/webhooks/serializers.py index 6623d1e7ab81..ef8ee518a837 100644 --- a/api/webhooks/serializers.py +++ b/api/webhooks/serializers.py @@ -1,5 +1,7 @@ from rest_framework import serializers +from webhooks.fields import NoSSRFURLField + class WebhookSerializer(serializers.Serializer[None]): event_type = serializers.ChoiceField(choices=["FLAG_UPDATED", "AUDIT_LOG_CREATED"]) @@ -7,7 +9,7 @@ class WebhookSerializer(serializers.Serializer[None]): class WebhookURLSerializer(serializers.Serializer[None]): - url = serializers.URLField() + url = NoSSRFURLField() class ScopeSerializer(serializers.Serializer[None]): @@ -15,7 +17,7 @@ class ScopeSerializer(serializers.Serializer[None]): class TestWebhookSerializer(serializers.Serializer[None]): - webhook_url = serializers.URLField(required=True) + webhook_url = NoSSRFURLField(required=True) scope = ScopeSerializer(required=True) secret = serializers.CharField(required=False, allow_blank=True, allow_null=True) From 36a586830c4bfe3e96d0127c61ca267388bfa4be Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 20 May 2026 16:39:17 +0100 Subject: [PATCH 6/8] Fix test --- api/tests/unit/webhooks/test_unit_webhooks.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/api/tests/unit/webhooks/test_unit_webhooks.py b/api/tests/unit/webhooks/test_unit_webhooks.py index 73038bc17d91..9ef69bcd2561 100644 --- a/api/tests/unit/webhooks/test_unit_webhooks.py +++ b/api/tests/unit/webhooks/test_unit_webhooks.py @@ -361,9 +361,6 @@ def test_call_integration_webhook__backoff_give_up__does_not_raise_error( def test_send_test_webhook__200_response_from_webhook__returns_correct_response( mocker: MockerFixture, admin_client: APIClient, - external_api_response_status: int, - expected_final_status: int, - external_api_error_text: str, organisation: Organisation, ) -> None: # Given @@ -388,7 +385,7 @@ def test_send_test_webhook__200_response_from_webhook__returns_correct_response( ) # Then - assert response.status_code == expected_final_status + assert response.status_code == 200 mock_post.assert_called_once() response_json = response.json() assert response_json["status"] == 200 From f8cc8b7e1d09d11e8c6ab5784ae91e4d6c74c2c4 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 21 May 2026 14:28:11 +0100 Subject: [PATCH 7/8] Prevent webhook redirects --- api/webhooks/webhooks.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/api/webhooks/webhooks.py b/api/webhooks/webhooks.py index a9362f8c74a1..4b765541c075 100644 --- a/api/webhooks/webhooks.py +++ b/api/webhooks/webhooks.py @@ -143,7 +143,11 @@ def _call_webhook( try: res = requests.post( - str(webhook.url), data=json_data, headers=headers, timeout=10 + str(webhook.url), + data=json_data, + headers=headers, + timeout=10, + allow_redirects=False, ) res.raise_for_status() return res @@ -188,7 +192,11 @@ def call_webhook_with_failure_mail_after_retries( # type: ignore[no-untyped-def try: res = requests.post( - str(webhook.url), data=json_data, headers=headers, timeout=10 + str(webhook.url), + data=json_data, + headers=headers, + timeout=10, + allow_redirects=False, ) if not res.ok: logger.warning( From 626db5caf1e9d58ee018bd8997cb45f85e508f66 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 21 May 2026 15:29:11 +0100 Subject: [PATCH 8/8] Use getaddrinfo to catch private IPv6 addresses in SSRF validator Co-Authored-By: Claude Sonnet 4.6 --- .../unit/webhooks/test_webhooks_fields.py | 21 +++++++++++++--- api/webhooks/fields.py | 24 +++++++++++-------- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/api/tests/unit/webhooks/test_webhooks_fields.py b/api/tests/unit/webhooks/test_webhooks_fields.py index ca6b01d711ff..f2763a2c85b6 100644 --- a/api/tests/unit/webhooks/test_webhooks_fields.py +++ b/api/tests/unit/webhooks/test_webhooks_fields.py @@ -58,8 +58,8 @@ def test_no_ssrf_url_field__hostname_resolving_to_private_ip__raises_validation_ ) -> None: # Given — a hostname that resolves to an RFC1918 address with mock.patch( - "webhooks.fields.socket.gethostbyname", - return_value="192.168.1.100", + "webhooks.fields.socket.getaddrinfo", + return_value=[(socket.AF_INET, None, None, None, ("192.168.1.100", 0))], ): # When / Then with pytest.raises(ValidationError) as exc_info: @@ -68,6 +68,21 @@ def test_no_ssrf_url_field__hostname_resolving_to_private_ip__raises_validation_ assert "internal_address" in str(exc_info.value.detail) +def test_no_ssrf_url_field__hostname_resolving_to_private_ipv6__raises_validation_error( + field: NoSSRFURLField, +) -> None: + # Given — an AAAA-only hostname resolving to a private IPv6 address + with mock.patch( + "webhooks.fields.socket.getaddrinfo", + return_value=[(socket.AF_INET6, None, None, None, ("fc00::1", 0, 0, 0))], + ): + # When / Then + with pytest.raises(ValidationError) as exc_info: + field.run_validation("http://internal-v6.example.com/hook") + + assert "internal_address" in str(exc_info.value.detail) + + @pytest.mark.parametrize( "url,label", [ @@ -93,7 +108,7 @@ def test_no_ssrf_url_field__unresolvable_hostname__returns_value( ) -> None: # Given — the hostname cannot be resolved; URL format is still valid with mock.patch( - "webhooks.fields.socket.gethostbyname", + "webhooks.fields.socket.getaddrinfo", side_effect=socket.gaierror, ): # When diff --git a/api/webhooks/fields.py b/api/webhooks/fields.py index e48c5038799a..8729c1262b0a 100644 --- a/api/webhooks/fields.py +++ b/api/webhooks/fields.py @@ -29,20 +29,24 @@ def run_validators(self, value: str) -> None: hostname = urlparse(value).hostname or "" try: - ip = ipaddress.ip_address(hostname) + ips = [ipaddress.ip_address(hostname)] except ValueError: # hostname is a name rather than a literal IP — resolve it. try: - ip = ipaddress.ip_address(socket.gethostbyname(hostname)) + results = socket.getaddrinfo(hostname, None, socket.AF_UNSPEC) + ips = [ + ipaddress.ip_address(str(r[4][0]).split("%")[0]) for r in results + ] except socket.gaierror: # Unresolvable hostname; leave it to the URL validator. return - if ( - ip.is_loopback - or ip.is_private - or ip.is_link_local - or ip.is_reserved - or ip.is_multicast - ): - self.fail("internal_address") + for ip in ips: + if ( + ip.is_loopback + or ip.is_private + or ip.is_link_local + or ip.is_reserved + or ip.is_multicast + ): + self.fail("internal_address")