From 57b5fb17addb0ead768937ac0fdac7b53c95048c Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Tue, 4 Aug 2026 17:27:02 +0530 Subject: [PATCH] [SECUR-245] fix(security): match reserved /24s exactly and bound the Live request timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/api/plane/bgtasks/copy_s3_object.py | 13 ++++++- .../unit/bg_tasks/test_copy_s3_object_auth.py | 34 ++++++++++++++++++- apps/live/src/lib/url-security.ts | 14 +++++--- apps/live/tests/lib/url-security.test.ts | 19 +++++++++++ 4 files changed, 74 insertions(+), 6 deletions(-) diff --git a/apps/api/plane/bgtasks/copy_s3_object.py b/apps/api/plane/bgtasks/copy_s3_object.py index d2ffbe2856..73f5a48dd1 100644 --- a/apps/api/plane/bgtasks/copy_s3_object.py +++ b/apps/api/plane/bgtasks/copy_s3_object.py @@ -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: diff --git a/apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py b/apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py index ab449a3ac1..0ed5d1cc89 100644 --- a/apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py +++ b/apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py @@ -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", "

hello

") + + 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", "

hello

") + + assert result == {} + + @override_settings(LIVE_URL=LIVE_URL, LIVE_SERVER_SECRET_KEY=None) def test_missing_secret_key_skips_request(): """ diff --git a/apps/live/src/lib/url-security.ts b/apps/live/src/lib/url-security.ts index 7f3f58ee0e..973ef102ab 100644 --- a/apps/live/src/lib/url-security.ts +++ b/apps/live/src/lib/url-security.ts @@ -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; }; diff --git a/apps/live/tests/lib/url-security.test.ts b/apps/live/tests/lib/url-security.test.ts index c41e238ebd..35da757d2e 100644 --- a/apps/live/tests/lib/url-security.test.ts +++ b/apps/live/tests/lib/url-security.test.ts @@ -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", () => {