feat: make webhook SSRF protection configurable for self-hosted deployments#8732
feat: make webhook SSRF protection configurable for self-hosted deployments#8732Quentin-M wants to merge 1 commit intomakeplane:previewfrom
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new environment flag, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR makes the webhook SSRF protection in Plane configurable via an environment variable (ENABLE_WEBHOOK_SSRF_PROTECTION), defaulting to enabled ("1"). The motivation is to allow self-hosted deployments (Kubernetes, docker-compose on private networks) to point webhooks at internal services — a currently blocked use-case — without requiring image patches. Plane Cloud deployments are unaffected since the default keeps protection on.
Changes:
- Adds the
ENABLE_WEBHOOK_SSRF_PROTECTIONenv var (default"1") that gates the private/loopback/reserved IP check inWebhookSerializer.create()and.update() - Gates the
validate_domainmodel validator (which blockslocalhost/127.0.0.1) behind the same env var - Reads the flag as a module-level constant in the serializer, and inline via
os.environ.getin the model validator
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
apps/api/plane/app/serializers/webhook.py |
Adds module-level ENABLE_WEBHOOK_SSRF_PROTECTION boolean and wraps the SSRF IP-check block in both create() and update() behind it |
apps/api/plane/db/models/webhook.py |
Wraps the validate_domain localhost/127.0.0.1 check behind an inline os.environ.get(...) call for the same env var |
| domain = parsed_url.netloc | ||
| if domain in ["localhost", "127.0.0.1"]: | ||
| raise ValidationError("Local URLs are not allowed.") | ||
| if os.environ.get("ENABLE_WEBHOOK_SSRF_PROTECTION", "1") != "0": |
There was a problem hiding this comment.
The ENABLE_WEBHOOK_SSRF_PROTECTION flag is evaluated at module import time in the serializer (line 19 caches the boolean), but is read fresh from os.environ on every call inside the validate_domain function in the model. This inconsistency means that if the env var were ever changed after startup (unlikely but possible in some environments), the two checks would diverge. More importantly, this makes the behavior harder to reason about and test, because the serializer's flag is fixed at import time while the model validator reads it dynamically.
The recommended fix is to use the same approach in both places. Since a module-level constant (evaluated once at startup) is the more efficient and predictable pattern, validate_domain in webhook.py should also use a module-level constant instead of calling os.environ.get(...) inline. Alternatively, both places could call os.environ.get(...) on every invocation, but that's less common for this type of configuration flag.
| from plane.db.models import Webhook, WebhookLog | ||
| from plane.db.models.webhook import validate_domain, validate_schema | ||
|
|
||
| ENABLE_WEBHOOK_SSRF_PROTECTION = os.environ.get("ENABLE_WEBHOOK_SSRF_PROTECTION", "1") != "0" |
There was a problem hiding this comment.
The new ENABLE_WEBHOOK_SSRF_PROTECTION environment variable is not documented in .env.example, whereas all other env vars (like DEBUG, USE_MINIO, ENABLE_READ_REPLICA, API_KEY_RATE_LIMIT, etc.) are listed there. Without a corresponding entry, self-hosted operators who consult .env.example as the reference for available configuration options will not discover this new security toggle.
An entry with a comment explaining its purpose and the default value (1 = protection enabled, 0 = disabled) should be added to .env.example.
| if ENABLE_WEBHOOK_SSRF_PROTECTION: | ||
| for addr in ip_addresses: | ||
| ip = ipaddress.ip_address(addr[4][0]) | ||
| if ip.is_private or ip.is_loopback or ip.is_reserved or ip.is_link_local: | ||
| raise serializers.ValidationError({"url": "URL resolves to a blocked IP address."}) |
There was a problem hiding this comment.
The new conditional SSRF protection behavior in WebhookSerializer.create() and .update() is not covered by any tests. The codebase has a dedicated unit test directory for serializers (apps/api/plane/tests/unit/serializers/) with tests for other serializers (e.g., test_workspace.py, test_label.py). Given this existing testing convention, tests should be added for:
- Default behavior: webhook creation with a private/loopback IP is blocked when
ENABLE_WEBHOOK_SSRF_PROTECTIONis not set (or set to"1"). - Opt-out behavior: webhook creation with a private/loopback IP succeeds when
ENABLE_WEBHOOK_SSRF_PROTECTION=0.
Similarly, the conditional behavior in validate_domain (in webhook.py) should be tested.
Quentin-M
force-pushed
the
fix/webhook-ssrf-self-hosted
branch
from
b45ff1c to
dfe481e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/plane/app/serializers/webhook.py (2)
19-19: Add regression coverage for both flag states.This toggle changes security behavior in the model validator plus both serializer mutation paths, but the diff doesn't add tests for unset vs
"0"or theupdate()flow. A small matrix here would make regressions much harder to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/app/serializers/webhook.py` at line 19, Add regression tests covering both states of ENABLE_WEBHOOK_SSRF_PROTECTION (unset/true and set to "0"/false) and exercise both serializer mutation paths including the update() flow and the model validator that changes behavior based on this flag; specifically, write tests that toggle os.environ for ENABLE_WEBHOOK_SSRF_PROTECTION, instantiate the relevant serializer(s) used in apps/api/plane/app/serializers/webhook.py, call create/save and update() mutation paths, and assert the expected security-related validations/side-effects for each flag state so regressions are detected.
42-46: Extract the blocked-IP check into one helper.The same guarded
ipaddressloop now lives in bothcreate()andupdate(). Pulling it into a private helper will keep the flag behavior and blocked-range list from drifting on the next edit.Also applies to: 78-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/app/serializers/webhook.py` around lines 42 - 46, Extract the repeated IP-blocking logic used under ENABLE_WEBHOOK_SSRF_PROTECTION into a private helper (e.g., _raise_if_blocked_ip or _validate_webhook_ip) that accepts ip_addresses and raises serializers.ValidationError({"url": "URL resolves to a blocked IP address."}) when any ip in ip_addresses meets is_private/is_loopback/is_reserved/is_link_local; then replace the duplicated loop in create() and update() with calls to that helper so both paths share the exact same flag check and blocked-range behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/plane/db/models/webhook.py`:
- Around line 29-33: The SSRF host check in the webhook URL validator uses
parsed_url.netloc which can include ports/credentials and vary in case; change
it to use parsed_url.hostname (e.g., replace references to parsed_url.netloc
with parsed_url.hostname) so the host portion is normalized and stripped of
ports/credentials, and ensure you handle None (missing hostname) before
comparing to literals like "localhost" or "127.0.0.1" when enforcing
ENABLE_WEBHOOK_SSRF_PROTECTION in the validation logic.
---
Nitpick comments:
In `@apps/api/plane/app/serializers/webhook.py`:
- Line 19: Add regression tests covering both states of
ENABLE_WEBHOOK_SSRF_PROTECTION (unset/true and set to "0"/false) and exercise
both serializer mutation paths including the update() flow and the model
validator that changes behavior based on this flag; specifically, write tests
that toggle os.environ for ENABLE_WEBHOOK_SSRF_PROTECTION, instantiate the
relevant serializer(s) used in apps/api/plane/app/serializers/webhook.py, call
create/save and update() mutation paths, and assert the expected
security-related validations/side-effects for each flag state so regressions are
detected.
- Around line 42-46: Extract the repeated IP-blocking logic used under
ENABLE_WEBHOOK_SSRF_PROTECTION into a private helper (e.g., _raise_if_blocked_ip
or _validate_webhook_ip) that accepts ip_addresses and raises
serializers.ValidationError({"url": "URL resolves to a blocked IP address."})
when any ip in ip_addresses meets
is_private/is_loopback/is_reserved/is_link_local; then replace the duplicated
loop in create() and update() with calls to that helper so both paths share the
exact same flag check and blocked-range behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83913d8c-a350-4d7c-8782-142aab0201b9
📒 Files selected for processing (2)
apps/api/plane/app/serializers/webhook.pyapps/api/plane/db/models/webhook.py
| parsed_url = urlparse(value) | ||
| domain = parsed_url.netloc | ||
| if domain in ["localhost", "127.0.0.1"]: | ||
| raise ValidationError("Local URLs are not allowed.") | ||
| if os.environ.get("ENABLE_WEBHOOK_SSRF_PROTECTION", "1") != "0": | ||
| if domain in ["localhost", "127.0.0.1"]: | ||
| raise ValidationError("Local URLs are not allowed.") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python's urllib.parse.urlparse, what is the difference between ParseResult.netloc and ParseResult.hostname? Does hostname strip ports/credentials and normalize the host casing?
💡 Result:
-
ParseResult.netlocis the raw “network location” / authority string exactly as parsed from the URL (no further sub-splitting), e.g. it can contain credentials and port:username:password@Host.EXAMPLE:8443(and IPv6 brackets, etc.). Also, URL parsing does not expand%escapes in components. [1] -
ParseResult.hostnameis a derived convenience property that extracts just the host part fromnetloc:- strips userinfo (
user:pass@) and port (:8443) - strips IPv6
[...]brackets when present - returns the host in lower case (with a special-case: for scoped IPv6 like
fe80::1%Zone, only the part before%is lowercased; the zone is preserved) [1][2]
- strips userinfo (
So yes: hostname strips ports/credentials and normalizes host casing to lowercase (with the IPv6 zone exception above). [1][2]
Sources: Python urllib.parse docs [1], CPython urllib.parse implementation [2].
Use parsed_url.hostname instead of parsed_url.netloc to prevent SSRF bypass.
The current validator compares the raw netloc string against literals like "localhost", which fails to match:
- URLs with ports:
localhost:8000hasnetloc = "localhost:8000" - URLs with credentials:
user@localhosthasnetloc = "user@localhost" - Uppercase hostnames:
LOCALHOSTwill not equal the literal"localhost"
The hostname property extracts only the host portion, strips ports and credentials, and normalizes to lowercase, ensuring the check is effective.
Suggested fix
def validate_domain(value):
parsed_url = urlparse(value)
- domain = parsed_url.netloc
+ domain = parsed_url.hostname
if os.environ.get("ENABLE_WEBHOOK_SSRF_PROTECTION", "1") != "0":
- if domain in ["localhost", "127.0.0.1"]:
+ if domain in {"localhost", "127.0.0.1"}:
raise ValidationError("Local URLs are not allowed.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parsed_url = urlparse(value) | |
| domain = parsed_url.netloc | |
| if domain in ["localhost", "127.0.0.1"]: | |
| raise ValidationError("Local URLs are not allowed.") | |
| if os.environ.get("ENABLE_WEBHOOK_SSRF_PROTECTION", "1") != "0": | |
| if domain in ["localhost", "127.0.0.1"]: | |
| raise ValidationError("Local URLs are not allowed.") | |
| parsed_url = urlparse(value) | |
| domain = parsed_url.hostname | |
| if os.environ.get("ENABLE_WEBHOOK_SSRF_PROTECTION", "1") != "0": | |
| if domain in {"localhost", "127.0.0.1"}: | |
| raise ValidationError("Local URLs are not allowed.") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/db/models/webhook.py` around lines 29 - 33, The SSRF host
check in the webhook URL validator uses parsed_url.netloc which can include
ports/credentials and vary in case; change it to use parsed_url.hostname (e.g.,
replace references to parsed_url.netloc with parsed_url.hostname) so the host
portion is normalized and stripped of ports/credentials, and ensure you handle
None (missing hostname) before comparing to literals like "localhost" or
"127.0.0.1" when enforcing ENABLE_WEBHOOK_SSRF_PROTECTION in the validation
logic.
Summary
Self-hosted Plane deployments (Kubernetes, docker-compose on private networks) need webhooks pointing at internal services — for example,
http://my-service.default.svc.cluster.local:9090/webhook. The current unconditional SSRF check blocks all private/loopback/reserved/link-local IPs, making webhooks unusable for any self-hosted integration without patching the image.Changes
ENABLE_WEBHOOK_SSRF_PROTECTIONenvironment variable (default:"1", enabled)"0", the IP-range check inWebhookSerializer.create()and.update()is skippedvalidate_domainmodel validator that blocks literallocalhost/127.0.0.1Motivation
Every self-hosted Plane user who needs webhook integrations with internal services has to patch the Docker image or exec into the container to modify
webhook.py. The SSRF protection makes sense for Plane Cloud (multi-tenant, untrusted URLs) but is overly restrictive for self-hosted (single-tenant, trusted network).Test plan
ENABLE_WEBHOOK_SSRF_PROTECTION=0, verify webhook creation with private IP succeedsvalidate_domainstill blockslocalhostwhen protection is enabledSummary by CodeRabbit