* [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>
102 lines
4.2 KiB
Python
102 lines
4.2 KiB
Python
# Copyright (c) 2023-present Plane Software, Inc. and contributors
|
|
# SPDX-License-Identifier: AGPL-3.0-only
|
|
# See the LICENSE file for details.
|
|
|
|
# Python imports
|
|
import logging
|
|
from urllib.parse import urlparse
|
|
|
|
# Third party imports
|
|
from rest_framework import serializers
|
|
|
|
# Django imports
|
|
from django.conf import settings
|
|
|
|
# Module imports
|
|
from .base import DynamicBaseSerializer
|
|
from plane.db.models import Webhook, WebhookLog
|
|
from plane.db.models.webhook import validate_domain, validate_schema
|
|
from plane.utils.ip_address import validate_url
|
|
|
|
logger = logging.getLogger(__name__)
|
|
|
|
|
|
class WebhookSerializer(DynamicBaseSerializer):
|
|
url = serializers.URLField(validators=[validate_schema, validate_domain])
|
|
|
|
def _validate_webhook_url(self, url):
|
|
"""Validate a webhook URL against SSRF and disallowed domain rules."""
|
|
try:
|
|
validate_url(
|
|
url,
|
|
allowed_ips=settings.WEBHOOK_ALLOWED_IPS,
|
|
allowed_hosts=settings.WEBHOOK_ALLOWED_HOSTS,
|
|
)
|
|
except ValueError as e:
|
|
logger.warning("Webhook URL validation failed for %s: %s", url, e)
|
|
raise serializers.ValidationError({"url": "Invalid or disallowed webhook URL."})
|
|
|
|
hostname = (urlparse(url).hostname or "").rstrip(".").lower()
|
|
|
|
# Hosts explicitly trusted via WEBHOOK_ALLOWED_HOSTS bypass the
|
|
# disallowed-domain check — they're already trusted for SSRF, so
|
|
# the loop-back guard would only get in the way of legitimate
|
|
# sibling services that share a parent domain with Plane.
|
|
if hostname in settings.WEBHOOK_ALLOWED_HOSTS:
|
|
return
|
|
|
|
request = self.context.get("request")
|
|
disallowed_domains = list(settings.WEBHOOK_DISALLOWED_DOMAINS)
|
|
if request:
|
|
request_host = request.get_host().split(":")[0].rstrip(".").lower()
|
|
disallowed_domains.append(request_host)
|
|
|
|
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)
|
|
return Webhook.objects.create(**validated_data)
|
|
|
|
def update(self, instance, validated_data):
|
|
url = validated_data.get("url", None)
|
|
if url:
|
|
self._validate_webhook_url(url)
|
|
return super().update(instance, validated_data)
|
|
|
|
class Meta:
|
|
model = Webhook
|
|
fields = "__all__"
|
|
read_only_fields = ["workspace", "secret_key", "deleted_at"]
|
|
|
|
|
|
class WebhookLogSerializer(DynamicBaseSerializer):
|
|
class Meta:
|
|
model = WebhookLog
|
|
fields = "__all__"
|
|
read_only_fields = ["workspace", "webhook"]
|