jira/apps/api/plane/app/serializers/webhook.py
Manish Gupta 30527927d7
[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>
2026-08-28 00:16:01 +05:30

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"]