Mirrors/plane
1
0
Fork 0
mirror of https://github.com/makeplane/plane.git synced 2026-09-01 19:48:42 +02:00
Code Issues Packages Projects Releases Wiki Activity

[SECUR-245] fix(security): match reserved /24s exactly and bound the Live request timeout

Browse Source 
Addresses review on #9540.

Copilot: the IPv4 blocklist tested only the second octet for blocks that are
actually /24s inside public /16s, so it blackholed real public space. Reviewing
the whole class rather than the one reported case found three instances, not one:

  192.0.0.0/16   -> 192.0.0.0/24 + 192.0.2.0/24   (was blocking 192.0.3.x etc.)
  198.51.0.0/16  -> 198.51.100.0/24               (was blocking 198.51.99.x etc.)
  203.0.0.0/16   -> 203.0.113.0/24                (was blocking 203.0.112.x etc.)

Now matched on the third octet. Verified against the Python guard's is_blocked_ip
(apps/api/plane/utils/ip_address.py) for all 15 boundary cases — TS and Python
verdicts now agree exactly, which is the property the original comment claimed
but did not hold. This was the same implementation-drift failure the helper's own
comment warns about, one commit later.

Tests added in both directions so a future edit cannot silently over-block:
the four reserved /24s must be rejected, and the six adjacent public /24s must
still be allowed. The second assertion is what caught this.

CodeRabbit: requests has no default timeout, so the Live call could pin a Celery
worker indefinitely on a server that accepts the connection then stalls. Adds
LIVE_REQUEST_TIMEOUT = (5, 30) and asserts it is passed. Also asserts a ReadTimeout
degrades duplication rather than failing the task — requests.Timeout subclasses
RequestException, so the existing handler already covers it.

Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
Manish Gupta
2026-08-04 17:27:02 +05:30
parent 184596c5db
commit 57b5fb17ad
4 changed files with 74 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
apps/api/plane/bgtasks/copy_s3_object.py
View File

@@ -18,6 +18,12 @@ from plane.settings.storage import S3Storage
from celery import shared_task
from plane.utils.url import normalize_url_path
# (connect, read) timeout for the Live service call. `requests` has no default
# timeout, so omitting this lets a duplication task occupy a Celery worker
# indefinitely if Live accepts the connection and then stops responding. The read
# budget is generous because converting a large document is genuinely slow.
LIVE_REQUEST_TIMEOUT = (5, 30)
def get_entity_id_field(entity_type, entity_id):
entity_mapping = {
@@ -89,7 +95,12 @@ def sync_with_external_service(entity_name, description_html):
)
return {}
response = requests.post(url, json=data, headers={"live-server-secret-key": secret_key})
response = requests.post(
url,
json=data,
headers={"live-server-secret-key": secret_key},
timeout=LIVE_REQUEST_TIMEOUT,
)
if response.status_code == 200:
return response.json()
except requests.RequestException as e:

34
apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py
View File

@@ -16,9 +16,10 @@ These are pure unit tests: no database, no network.
from unittest.mock import MagicMock, patch
import requests
from django.test import override_settings
from plane.bgtasks.copy_s3_object import sync_with_external_service
from plane.bgtasks.copy_s3_object import LIVE_REQUEST_TIMEOUT, sync_with_external_service
LIVE_URL = "http://live:3000/live/"
SECRET = "unit-test-live-secret"
@@ -40,6 +41,37 @@ def test_sends_secret_key_header():
assert headers == {"live-server-secret-key": SECRET}
@override_settings(LIVE_URL=LIVE_URL, LIVE_SERVER_SECRET_KEY=SECRET)
def test_sends_bounded_timeout():
"""
`requests` has no default timeout. Without one, a Live service that accepts the
connection and then stalls would pin a Celery worker indefinitely.
"""
response = MagicMock(status_code=200)
response.json.return_value = {}
with patch("plane.bgtasks.copy_s3_object.requests.post", return_value=response) as mock_post:
sync_with_external_service("PAGE", "<p>hello</p>")
timeout = mock_post.call_args.kwargs["timeout"]
assert timeout == LIVE_REQUEST_TIMEOUT
connect, read = timeout
assert 0 < connect <= 10
assert 0 < read <= 60
@override_settings(LIVE_URL=LIVE_URL, LIVE_SERVER_SECRET_KEY=SECRET)
def test_timeout_is_swallowed_not_raised():
"""A stalled Live service must degrade duplication, not fail the whole task."""
with patch(
"plane.bgtasks.copy_s3_object.requests.post",
side_effect=requests.exceptions.ReadTimeout("timed out"),
):
result = sync_with_external_service("PAGE", "<p>hello</p>")
assert result == {}
@override_settings(LIVE_URL=LIVE_URL, LIVE_SERVER_SECRET_KEY=None)
def test_missing_secret_key_skips_request():
"""

14
apps/live/src/lib/url-security.ts
View File

@@ -44,7 +44,7 @@ const isBlockedIPv4 = (ip: string): boolean => {
// Not a canonical dotted quad — callers treat unparseable hosts as unsafe.
return true;
}
const [a, b] = parts as [number, number, number, number];
const [a, b, c] = parts as [number, number, number, number];
if (a === 0) return true; // 0.0.0.0/8 "this host on this network"
if (a === 10) return true; // 10.0.0.0/8 private
@@ -53,12 +53,18 @@ const isBlockedIPv4 = (ip: string): boolean => {
if (a === 169 && b === 254) return true; // 169.254.0.0/16 link-local (incl. 169.254.169.254 metadata)
if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 private
if (a === 192 && b === 168) return true; // 192.168.0.0/16 private
if (a === 192 && b === 0) return true; // 192.0.0.0/24 + 192.0.2.0/24 (IETF protocol / TEST-NET-1)
if (a === 198 && (b === 18 || b === 19)) return true; // 198.18.0.0/15 benchmarking
if (a === 198 && b === 51) return true; // 198.51.100.0/24 TEST-NET-2
if (a === 203 && b === 0) return true; // 203.0.113.0/24 TEST-NET-3
if (a >= 224) return true; // 224.0.0.0/4 multicast, 240.0.0.0/4 reserved, 255.255.255.255
// The remaining special-purpose blocks are /24s sitting inside otherwise-public
// /16s, so they must be matched on the third octet. Testing only the second octet
// would blackhole real public space (192.0.3.0/24, 198.51.x, 203.0.x) and quietly
// stop legitimate images from rendering.
if (a === 192 && b === 0 && c === 0) return true; // 192.0.0.0/24 IETF protocol assignments
if (a === 192 && b === 0 && c === 2) return true; // 192.0.2.0/24 TEST-NET-1
if (a === 198 && b === 51 && c === 100) return true; // 198.51.100.0/24 TEST-NET-2
if (a === 203 && b === 0 && c === 113) return true; // 203.0.113.0/24 TEST-NET-3
return false;
};

19
apps/live/tests/lib/url-security.test.ts
View File

@@ -80,6 +80,25 @@ describe("isSafeImageSrc — GHSA-55gq-rf47-9pqx", () => {
// 172.32.x is public; the private block ends at 172.31.
expect(isSafeImageSrc("http://172.32.0.1/")).toBe(true);
});
// These /24s sit inside otherwise-public /16s. Blocking the whole /16 would
// silently stop legitimate images from rendering, so the boundaries are pinned
// in both directions.
it("blocks the reserved /24s exactly", () => {
expect(isSafeImageSrc("http://192.0.0.1/")).toBe(false); // 192.0.0.0/24 IETF protocol assignments
expect(isSafeImageSrc("http://192.0.2.1/")).toBe(false); // 192.0.2.0/24 TEST-NET-1
expect(isSafeImageSrc("http://198.51.100.1/")).toBe(false); // 198.51.100.0/24 TEST-NET-2
expect(isSafeImageSrc("http://203.0.113.1/")).toBe(false); // 203.0.113.0/24 TEST-NET-3
});
it("still allows the public space surrounding those /24s", () => {
expect(isSafeImageSrc("http://192.0.1.1/")).toBe(true);
expect(isSafeImageSrc("http://192.0.3.1/")).toBe(true);
expect(isSafeImageSrc("http://198.51.99.1/")).toBe(true);
expect(isSafeImageSrc("http://198.51.101.1/")).toBe(true);
expect(isSafeImageSrc("http://203.0.112.1/")).toBe(true);
expect(isSafeImageSrc("http://203.0.114.1/")).toBe(true);
});
});
describe("obfuscated address encodings", () => {