-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix: replace IS_SELF_MANAGED with WEBHOOK_ALLOWED_IPS allowlist #8884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,9 +2,12 @@ | |||||||||||||||||||||||||||||||||||||||||
| # SPDX-License-Identifier: AGPL-3.0-only | ||||||||||||||||||||||||||||||||||||||||||
| # See the LICENSE file for details. | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| import ipaddress | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| import pytest | ||||||||||||||||||||||||||||||||||||||||||
| from unittest.mock import patch, MagicMock | ||||||||||||||||||||||||||||||||||||||||||
| from plane.bgtasks.work_item_link_task import safe_get, validate_url_ip | ||||||||||||||||||||||||||||||||||||||||||
| from plane.utils.ip_address import validate_url | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| def _make_response(status_code=200, headers=None, is_redirect=False, content=b""): | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -43,6 +46,49 @@ def test_allows_public_ip(self): | |||||||||||||||||||||||||||||||||||||||||
| validate_url_ip("https://example.com") # Should not raise | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| @pytest.mark.unit | ||||||||||||||||||||||||||||||||||||||||||
| class TestValidateUrlAllowlist: | ||||||||||||||||||||||||||||||||||||||||||
| """Test validate_url allowlist permits specific private IPs.""" | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| def test_allowlist_permits_private_ip(self): | ||||||||||||||||||||||||||||||||||||||||||
| allowed = [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_does_not_permit_other_private_ip(self): | ||||||||||||||||||||||||||||||||||||||||||
| allowed = [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) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| def test_allowlist_permits_loopback_when_explicitly_allowed(self): | ||||||||||||||||||||||||||||||||||||||||||
| allowed = [ipaddress.ip_network("127.0.0.0/8")] | ||||||||||||||||||||||||||||||||||||||||||
| with patch("plane.utils.ip_address.socket.getaddrinfo") as mock_dns: | ||||||||||||||||||||||||||||||||||||||||||
| mock_dns.return_value = [(None, None, None, None, ("127.0.0.1", 0))] | ||||||||||||||||||||||||||||||||||||||||||
| validate_url("http://example.com", allowed_ips=allowed) # Should not raise | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+71
to
+91
|
||||||||||||||||||||||||||||||||||||||||||
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The webhook URL is validated only at create/update time, but
webhook_send_tasklater posts towebhook.urlwithout 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 samevalidate_url(..., allowed_ips=settings.WEBHOOK_ALLOWED_IPS)logic) and disabling redirects or validating each redirect hop.