fix: replace IS_SELF_MANAGED with WEBHOOK_ALLOWED_IPS allowlist#8884
fix: replace IS_SELF_MANAGED with WEBHOOK_ALLOWED_IPS allowlist#8884sriramveeraghanta merged 2 commits intopreviewfrom
Conversation
… allowlist Instead of blanket-allowing all private IPs on self-managed deployments, webhook URL validation now blocks all private/internal IPs by default and only permits specific networks listed in the WEBHOOK_ALLOWED_IPS env variable (comma-separated IPs/CIDRs).
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughCentralizes webhook URL security by adding a shared Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebhookSerializer
participant validate_url
participant DNS as DNS_Resolver
participant DB as Webhook_DB
participant Task as Webhook_Task
participant HTTP as External_Endpoint
Client->>WebhookSerializer: create/update webhook (url)
WebhookSerializer->>validate_url: _validate_webhook_url(url, allowed_ips)
validate_url->>DNS_Resolver: resolve hostname
DNS_Resolver-->>validate_url: IP(s)
validate_url-->>WebhookSerializer: ok / raise ValidationError
WebhookSerializer->>Webhook_DB: save Webhook
Webhook_DB-->>Task: enqueue/send event
Task->>validate_url: validate_url(url, allowed_ips) (re-check before send)
validate_url->>DNS_Resolver: resolve hostname
validate_url-->>Task: ok / raise ValueError (abort)
Task->>HTTP: requests.post(url, payload)
HTTP-->>Task: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR tightens SSRF protections for webhook endpoints by blocking private/internal IP resolution by default and introducing an explicit WEBHOOK_ALLOWED_IPS allowlist for self-managed deployments. It centralizes URL-to-IP validation in plane.utils.ip_address.validate_url and updates the webhook serializer to reuse it, with unit tests covering the allowlist behavior.
Changes:
- Add shared
validate_urlutility to block private/internal IP targets unless explicitly allowlisted. - Introduce
WEBHOOK_ALLOWED_IPSsetting parsed from an env var intoip_networkentries. - Refactor webhook serializer SSRF checks to use the shared validator and add allowlist unit tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/api/plane/utils/ip_address.py | Adds shared validate_url SSRF/allowlist validator used by webhook URL validation. |
| apps/api/plane/settings/common.py | Parses WEBHOOK_ALLOWED_IPS from env into a network allowlist for SSRF exceptions. |
| apps/api/plane/app/serializers/webhook.py | Deduplicates SSRF checks by calling validate_url and normalizes domain comparisons. |
| apps/api/plane/tests/unit/bg_tasks/test_work_item_link_task.py | Adds unit tests verifying allowlist behavior for private/loopback addresses. |
| def _validate_webhook_url(self, url): | ||
| """Validate a webhook URL against SSRF and disallowed domain rules.""" | ||
| try: | ||
| ip_addresses = socket.getaddrinfo(hostname, None) | ||
| except socket.gaierror: | ||
| raise serializers.ValidationError({"url": "Hostname could not be resolved."}) | ||
|
|
||
| if not ip_addresses: | ||
| raise serializers.ValidationError({"url": "No IP addresses found for the hostname."}) | ||
| validate_url(url, allowed_ips=settings.WEBHOOK_ALLOWED_IPS) | ||
| except ValueError as e: | ||
| raise serializers.ValidationError({"url": str(e)}) | ||
|
|
There was a problem hiding this comment.
The webhook URL is validated only at create/update time, but webhook_send_task later posts to webhook.url without re-validating the resolved IP and (by default) will follow redirects. This leaves a DNS-rebinding/redirect path where a URL that was safe when saved can resolve/redirect to a private/internal IP at send time. Consider validating the target immediately before each send (using the same validate_url(..., allowed_ips=settings.WEBHOOK_ALLOWED_IPS) logic) and disabling redirects or validating each redirect hop.
|
|
||
|
|
There was a problem hiding this comment.
The new allowlist tests don’t cover the mixed IPv4/IPv6 allowlist case (e.g., allowed_ips contains an IPv6 CIDR while the hostname resolves to an IPv4 address). Today this can surface as a TypeError during membership checks. Add a unit test that passes mixed-version networks and asserts validation still works (and blocks/permits correctly) without crashing.
| def test_allowlist_permits_matching_ipv4_with_mixed_version_networks(self): | |
| allowed = [ | |
| ipaddress.ip_network("2001:db8::/32"), | |
| ipaddress.ip_network("192.168.1.0/24"), | |
| ] | |
| with patch("plane.utils.ip_address.socket.getaddrinfo") as mock_dns: | |
| mock_dns.return_value = [(None, None, None, None, ("192.168.1.50", 0))] | |
| validate_url("http://example.com", allowed_ips=allowed) # Should not raise | |
| def test_allowlist_blocks_non_matching_ipv4_with_mixed_version_networks(self): | |
| allowed = [ | |
| ipaddress.ip_network("2001:db8::/32"), | |
| ipaddress.ip_network("192.168.1.0/24"), | |
| ] | |
| with patch("plane.utils.ip_address.socket.getaddrinfo") as mock_dns: | |
| mock_dns.return_value = [(None, None, None, None, ("10.0.0.1", 0))] | |
| with pytest.raises(ValueError, match="private/internal"): | |
| validate_url("http://example.com", allowed_ips=allowed) |
![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
🤖 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/app/serializers/webhook.py`:
- Around line 28-29: The except block currently re-raises
serializers.ValidationError with str(e) which can leak internals; instead log
the original exception server-side (e.g., using logger.exception or an error
reporting client) and raise a fixed, client-safe message like
serializers.ValidationError({"url": "Invalid URL"}) or similar user-facing text;
update the except ValueError as e handler (the block that currently raises
serializers.ValidationError({"url": str(e)})) to perform logging of e and return
the sanitized ValidationError to the client.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ab8cf65-b83d-4364-919f-f60c6bad2fed
📒 Files selected for processing (4)
apps/api/plane/app/serializers/webhook.pyapps/api/plane/settings/common.pyapps/api/plane/tests/unit/bg_tasks/test_work_item_link_task.pyapps/api/plane/utils/ip_address.py
- Sanitize error messages to avoid leaking internal details to clients - Guard against TypeError with mixed IPv4/IPv6 allowlist networks - Re-validate webhook URL at send time to prevent DNS-rebinding - Add unit tests for mixed-version IP network allowlists
…plane#8884) * fix: replace IS_SELF_MANAGED toggle with explicit WEBHOOK_ALLOWED_IPS allowlist Instead of blanket-allowing all private IPs on self-managed deployments, webhook URL validation now blocks all private/internal IPs by default and only permits specific networks listed in the WEBHOOK_ALLOWED_IPS env variable (comma-separated IPs/CIDRs). * fix: address PR review comments for webhook SSRF protection - Sanitize error messages to avoid leaking internal details to clients - Guard against TypeError with mixed IPv4/IPv6 allowlist networks - Re-validate webhook URL at send time to prevent DNS-rebinding - Add unit tests for mixed-version IP network allowlists
Summary
IS_SELF_MANAGEDflag-based private IP toggle with an explicitWEBHOOK_ALLOWED_IPSenv variable (comma-separated IPs/CIDRs)10.0.0.0/8,192.168.1.0/24) instead of blanket-allowing all private IPsvalidate_urlfunction inip_address.pyand deduplicates inline SSRF checks in the webhook serializerTest plan
WEBHOOK_ALLOWED_IPS=10.0.0.0/8to allow internal webhook targetsSummary by CodeRabbit
New Features
Improvements
Tests