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", () => {