mirror of
https://github.com/makeplane/plane.git
synced 2026-07-10 04:25:24 +02:00
[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>
This commit is contained in:
@@ -54,6 +54,17 @@ 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. The per-request fields= allowlist that
|
||||
# the webhook views pass to exclude it is silently dropped by
|
||||
# DynamicBaseSerializer.__init__, so filter it here (GHSA-83rj).
|
||||
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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
|
||||
@@ -0,0 +1,99 @@
|
||||
# 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 GHSA-83rj-4282-x39v.
|
||||
|
||||
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 the network-dependent SSRF URL validation for this unit.
|
||||
monkeypatch.setattr(WebhookSerializer, "_validate_webhook_url", lambda self, url: 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
|
||||
revealed = WebhookSerializer(webhook, context={"show_secret_key": True}).data
|
||||
assert revealed["secret_key"] == webhook.secret_key
|
||||
Reference in New Issue
Block a user