[WEB-8103] fix: stop leaking webhook HMAC secret_key on reads (#9382)

* [WEB-8103] fix: stop leaking webhook HMAC secret_key on reads (GHSA-83rj)

WebhookEndpoint list/retrieve/patch pass a fields= allowlist that excludes
secret_key, but DynamicBaseSerializer.__init__ discards the caller
allowlist (fields = self.expand). With WebhookSerializer using
fields="__all__" and secret_key only in read_only_fields (read-only is
still serialized), the HMAC signing secret leaked on every webhook read
(GHSA-83rj-4282-x39v; admin-only).

Rather than secret_key = CharField(write_only=True) — which would let a
client inject their own secret on create/patch and break the intended
one-time reveal — hide it by default and reveal only where intended:

- WebhookSerializer.to_representation drops secret_key unless the
  show_secret_key context flag is set (secure by default). secret_key
  stays server-generated (default=generate_token) and non-writable.
- POST create and WebhookSecretRegenerateEndpoint pass show_secret_key so
  the secret is still returned once for the caller to configure their
  receiver; list/retrieve/patch no longer emit it.

Add contract regression tests (fail-before verified). Follow-up: the
DynamicBaseSerializer.__init__ allowlist bug affects other serializers —
tracked separately.

Co-authored-by: Plane AI <noreply@plane.so>

* [WEB-8103] test: address review — patch network boundary + pin to_representation

Per review (@sriramveeraghanta):
- Patch the network boundary (validate_url) instead of the whole private
  _validate_webhook_url method, so the domain/schema checks still run and
  the test survives a rename of the private method.
- Add an assertion that an explicit fields=("secret_key",) request still
  hides the key, pinning to_representation as the enforcement point so a
  future DynamicBaseSerializer._filter_fields fix can't silently re-open
  the leak.

Co-authored-by: Plane AI <noreply@plane.so>

* [WEB-8103] docs: spell out both levels of the dead fields= allowlist

The previous comment named only DynamicBaseSerializer.__init__ discarding the
caller's fields=, which is half the root cause. _filter_fields never removes
anything either: it builds `allowed` purely to attach expansion serializers for
names not already on the serializer, then returns self.fields unfiltered
(serializers/base.py:45-119).

So the fields= kwargs in views/webhook/base.py are no-ops on two independent
levels. Documented so a future `fields = fields or self.expand` fix isn't
assumed to re-activate the allowlists for confidentiality — _filter_fields has
to be made restrictive first. The show_secret_key context flag remains the sole
enforcement point.

Addresses @sriramveeraghanta's review on #9382. 6/6 contract tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(security): drop advisory identifiers from code comments

Explanations kept unchanged; only the IDs are removed.

Co-authored-by: Plane AI <noreply@plane.so>

---------

Co-authored-by: Plane AI <noreply@plane.so>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

apps/api/plane/app/serializers/webhook.py

@@ -54,6 +54,30 @@ class WebhookSerializer(DynamicBaseSerializer):
         if any(hostname == domain or hostname.endswith("." + domain) for domain in disallowed_domains):
             raise serializers.ValidationError({"url": "URL domain or its subdomain is not allowed."})
 
+    def to_representation(self, instance):
+        data = super().to_representation(instance)
+        # secret_key is the HMAC signing secret. It is only revealed on creation
+        # and secret regeneration (opt-in via the show_secret_key context flag),
+        # never on list/retrieve/update.
+        #
+        # This context flag — not the fields= allowlists in views/webhook/base.py —
+        # is the sole enforcement point. Those fields= kwargs are dead
+        # on two independent levels, so do not assume fixing either one re-activates
+        # them:
+        #   1. DynamicBaseSerializer.__init__ pops the caller's fields= and then
+        #      overwrites it with self.expand (serializers/base.py:16-18), so the
+        #      requested allowlist never reaches _filter_fields at all.
+        #   2. _filter_fields never *removes* anything even when it does receive a
+        #      list — it builds `allowed` purely to add expansion serializers for
+        #      names not already present, then returns self.fields unfiltered
+        #      (serializers/base.py:45-119).
+        # A future one-line `fields = fields or self.expand` fix addresses only (1);
+        # (2) still has to be made restrictive before any fields= allowlist can be
+        # relied on for confidentiality.
+        if not self.context.get("show_secret_key"):
+            data.pop("secret_key", None)
+        return data
+
     def create(self, validated_data):
         url = validated_data.get("url", None)
         self._validate_webhook_url(url)

apps/api/plane/app/views/webhook/base.py

@@ -22,7 +22,10 @@ class WebhookEndpoint(BaseAPIView):
     def post(self, request, slug):
         workspace = Workspace.objects.get(slug=slug)
         try:
-            serializer = WebhookSerializer(data=request.data, context={"request": request})
+            serializer = WebhookSerializer(
+                data=request.data,
+                context={"request": request, "show_secret_key": True},
+            )
             if serializer.is_valid():
                 serializer.save(workspace_id=workspace.id)
                 return Response(serializer.data, status=status.HTTP_201_CREATED)
@@ -114,7 +117,7 @@ class WebhookSecretRegenerateEndpoint(BaseAPIView):
         webhook = Webhook.objects.get(workspace__slug=slug, pk=pk)
         webhook.secret_key = generate_token()
         webhook.save()
-        serializer = WebhookSerializer(webhook)
+        serializer = WebhookSerializer(webhook, context={"show_secret_key": True})
         return Response(serializer.data, status=status.HTTP_200_OK)
 
 

apps/api/plane/tests/contract/app/test_webhook_secret_key_scope_app.py

@@ -0,0 +1,106 @@
+# Copyright (c) 2023-present Plane Software, Inc. and contributors
+# SPDX-License-Identifier: AGPL-3.0-only
+# See the LICENSE file for details.
+
+"""
+Regression tests for WEB-8103.
+
+WebhookEndpoint list/retrieve/patch passed a fields= allowlist that excluded
+secret_key, but DynamicBaseSerializer.__init__ discards the allowlist, so the
+HMAC secret_key leaked on every webhook read. WebhookSerializer now hides
+secret_key by default and only reveals it on create / secret regeneration.
+"""
+
+import pytest
+from rest_framework import status
+
+from plane.app.serializers import WebhookSerializer
+from plane.db.models import Webhook
+
+
+def _webhooks_url(slug, pk=None, regenerate=False):
+    base = f"/api/workspaces/{slug}/webhooks/"
+    if pk and regenerate:
+        return f"{base}{pk}/regenerate/"
+    if pk:
+        return f"{base}{pk}/"
+    return base
+
+
+def _make_webhook(workspace, url="https://example.com/hook"):
+    return Webhook.objects.create(workspace=workspace, url=url)
+
+
+@pytest.mark.contract
+class TestWebhookSecretKeyScope:
+    @pytest.mark.django_db
+    def test_list_does_not_leak_secret_key(self, session_client, workspace):
+        _make_webhook(workspace)
+
+        response = session_client.get(_webhooks_url(workspace.slug))
+
+        assert response.status_code == status.HTTP_200_OK
+        payload = response.json()
+        assert payload, "expected at least one webhook"
+        assert all("secret_key" not in item for item in payload)
+
+    @pytest.mark.django_db
+    def test_retrieve_does_not_leak_secret_key(self, session_client, workspace):
+        webhook = _make_webhook(workspace)
+
+        response = session_client.get(_webhooks_url(workspace.slug, pk=webhook.id))
+
+        assert response.status_code == status.HTTP_200_OK
+        assert "secret_key" not in response.json()
+
+    @pytest.mark.django_db
+    def test_patch_does_not_leak_secret_key(self, session_client, workspace):
+        webhook = _make_webhook(workspace)
+
+        # Patch a non-url field so URL SSRF validation is not exercised.
+        response = session_client.patch(
+            _webhooks_url(workspace.slug, pk=webhook.id), {"is_active": False}, format="json"
+        )
+
+        assert response.status_code == status.HTTP_200_OK
+        assert "secret_key" not in response.json()
+        assert response.json()["is_active"] is False
+
+    @pytest.mark.django_db
+    def test_regenerate_reveals_secret_key(self, session_client, workspace):
+        webhook = _make_webhook(workspace)
+        old_secret = webhook.secret_key
+
+        response = session_client.post(_webhooks_url(workspace.slug, pk=webhook.id, regenerate=True))
+
+        assert response.status_code == status.HTTP_200_OK
+        body = response.json()
+        assert "secret_key" in body
+        assert body["secret_key"] and body["secret_key"] != old_secret
+
+    @pytest.mark.django_db
+    def test_create_reveals_secret_key(self, session_client, workspace, monkeypatch):
+        # Bypass only the network boundary (DNS/IP SSRF resolution); the domain and
+        # schema checks in _validate_webhook_url still run for example.com, and the
+        # test survives a rename of the private method.
+        monkeypatch.setattr("plane.app.serializers.webhook.validate_url", lambda *a, **k: None)
+
+        response = session_client.post(
+            _webhooks_url(workspace.slug), {"url": "https://example.com/hook"}, format="json"
+        )
+
+        assert response.status_code == status.HTTP_201_CREATED
+        assert response.json().get("secret_key")
+
+    @pytest.mark.django_db
+    def test_serializer_hides_secret_by_default_and_reveals_with_flag(self, workspace):
+        webhook = _make_webhook(workspace)
+
+        assert "secret_key" not in WebhookSerializer(webhook).data
+        # An explicit fields= request must not re-open the leak either: enforcement
+        # lives in to_representation, not the (currently no-op) DynamicBaseSerializer
+        # field allowlist. Pins the enforcement point so a future _filter_fields fix
+        # can't silently re-expose the key.
+        assert "secret_key" not in WebhookSerializer(webhook, fields=("secret_key",)).data
+        revealed = WebhookSerializer(webhook, context={"show_secret_key": True}).data
+        assert revealed["secret_key"] == webhook.secret_key