From e92fdf9a32efb4ac2b2eb530a70817b5a5ddc9b0 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Tue, 4 Aug 2026 17:02:01 +0530 Subject: [PATCH 1/2] [SECUR-245] fix(security): guard PDF image srcs and authenticate /convert-document/ (GHSA-55gq-rf47-9pqx) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Live service exposed two problems reported as GHSA-55gq-rf47-9pqx. 1. SSRF via PDF image rendering. The `image` node renderer passed `node.attrs.src` straight to `@react-pdf/image`, which fetch()es any URL with a host and fs.readFile()s a bare path. Because the Live container shares a Docker network with api, db, redis, rabbitmq and minio, page content could drive requests at internal-only services. Adds `apps/live/src/lib/url-security.ts` and routes both `` call sites through it. Unsafe srcs render a placeholder instead. Note the existing `imageComponent` check was not a usable model: its `startsWith("http")` test passes `http://api:8000/` and `http://plane-minio:9000/` — every payload that matters. The scheme is irrelevant; the destination is what has to be judged. That check is replaced too. Blocked: non-http(s)/data schemes, bare and relative filesystem paths, loopback, RFC1918, CGNAT 100.64/10, link-local incl. 169.254.169.254, multicast, reserved and test ranges, IPv6 ULA/link-local/site-local/ multicast/NAT64/6to4/Teredo/IPv4-mapped, obfuscated encodings (2130706433, 0x7f000001, 127.1), single-label hosts (the shape of a Compose service name), .local/.internal/.lan suffixes, embedded credentials, and control-character scheme smuggling. The IPv6 ranges are kept in step with the Python guard's _BLOCKED_NETWORKS in apps/api/plane/utils/ip_address.py; the first draft here was missing Teredo and fec0::/10, and two implementations of one policy drifting apart is how this class of bug keeps recurring. 2. Unauthenticated /convert-document/. `requireSecretKey` existed but was applied to no controller, leaving an expensive HTML -> Y.js conversion open to anyone who could reach the service (CWE-306). It is now applied. Its only caller — the API's copy_s3_object duplication task — sent `headers=None`, so it now sends the shared secret, and LIVE_SERVER_SECRET_KEY is wired into Django settings (it was previously only in .env.example). A missing key short-circuits with a logged misconfiguration rather than firing a request that can only 401. Scope notes: - /pdf-export/ is deliberately untouched. Contrary to the advisory it is not unauthenticated: it requires a Cookie and forwards it to the API to fetch the page, so the API enforces page permissions. Gating it on the shared secret would break the browser client that design implies. - The advisory's pre-auth chain does not connect. /convert-document/ performs no outbound fetch, and its output is never fed to /pdf-export/, which reads content from the API by pageId. The SSRF requires a valid session, so severity is PR:L rather than the reported pre-auth. - Residual DNS rebinding on the http(s) path is documented in the helper and NOT closed here: the renderer is synchronous and the fetch happens inside @react-pdf/image, so the resolved address cannot be pinned. Closing it means pre-fetching raw image nodes into data: URIs the way imageComponent already pre-fetches assets. Follow-up to SECUR-245. Tests: 78 new Live tests covering every advisory payload plus range boundaries, and 6 API tests for the header contract and the missing-key/no-live-url paths. Co-authored-by: Plane AI --- apps/api/plane/bgtasks/copy_s3_object.py | 14 +- apps/api/plane/settings/common.py | 4 + .../unit/bg_tasks/test_copy_s3_object_auth.py | 105 +++++++++ .../src/controllers/document.controller.ts | 11 +- apps/live/src/lib/pdf/node-renderers.tsx | 20 +- apps/live/src/lib/url-security.ts | 180 ++++++++++++++++ apps/live/tests/lib/url-security.test.ts | 201 ++++++++++++++++++ 7 files changed, 532 insertions(+), 3 deletions(-) create mode 100644 apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py create mode 100644 apps/live/src/lib/url-security.ts create mode 100644 apps/live/tests/lib/url-security.test.ts diff --git a/apps/api/plane/bgtasks/copy_s3_object.py b/apps/api/plane/bgtasks/copy_s3_object.py index 742966a6fbb..d2ffbe28560 100644 --- a/apps/api/plane/bgtasks/copy_s3_object.py +++ b/apps/api/plane/bgtasks/copy_s3_object.py @@ -77,7 +77,19 @@ def sync_with_external_service(entity_name, description_html): url = normalize_url_path(f"{live_url}/convert-document/") - response = requests.post(url, json=data, headers=None) + # The Live service authenticates this endpoint with a shared secret + # (GHSA-55gq-rf47-9pqx). Without the header the request is rejected as 401. + secret_key = settings.LIVE_SERVER_SECRET_KEY + if not secret_key: + log_exception( + Exception( + "LIVE_SERVER_SECRET_KEY is not configured; skipping document conversion " + "for duplication. Set it to the same value as the Live service." + ) + ) + return {} + + response = requests.post(url, json=data, headers={"live-server-secret-key": secret_key}) if response.status_code == 200: return response.json() except requests.RequestException as e: diff --git a/apps/api/plane/settings/common.py b/apps/api/plane/settings/common.py index 25a212e7639..a85efc79eda 100644 --- a/apps/api/plane/settings/common.py +++ b/apps/api/plane/settings/common.py @@ -418,6 +418,10 @@ LIVE_URL = urljoin(LIVE_BASE_URL, LIVE_BASE_PATH) if LIVE_BASE_URL else None +# Shared secret for server-to-server calls into the Live service. Must match the +# Live container's LIVE_SERVER_SECRET_KEY. +LIVE_SERVER_SECRET_KEY = os.environ.get("LIVE_SERVER_SECRET_KEY") + # WEB URL WEB_URL = os.environ.get("WEB_URL") 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 new file mode 100644 index 00000000000..ab449a3ac1c --- /dev/null +++ b/apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py @@ -0,0 +1,105 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +""" +Authentication of the API -> Live `/convert-document/` call (GHSA-55gq-rf47-9pqx). + +The Live service previously served `/convert-document/` to anyone who could reach +it (`requireSecretKey` was defined but never applied to a controller). Now that the +endpoint is gated on the `live-server-secret-key` header, this background task is +the one caller that has to present it — so the header must actually be sent, and a +missing key must fail loudly rather than firing a request that 401s. + +These are pure unit tests: no database, no network. +""" + +from unittest.mock import MagicMock, patch + +from django.test import override_settings + +from plane.bgtasks.copy_s3_object import sync_with_external_service + +LIVE_URL = "http://live:3000/live/" +SECRET = "unit-test-live-secret" + + +@override_settings(LIVE_URL=LIVE_URL, LIVE_SERVER_SECRET_KEY=SECRET) +def test_sends_secret_key_header(): + """The shared secret must travel on the request, or Live returns 401.""" + response = MagicMock(status_code=200) + response.json.return_value = {"description_json": {}, "description_binary": "AA=="} + + with patch("plane.bgtasks.copy_s3_object.requests.post", return_value=response) as mock_post: + result = sync_with_external_service("PAGE", "

hello

") + + assert result == {"description_json": {}, "description_binary": "AA=="} + mock_post.assert_called_once() + + headers = mock_post.call_args.kwargs["headers"] + assert headers == {"live-server-secret-key": SECRET} + + +@override_settings(LIVE_URL=LIVE_URL, LIVE_SERVER_SECRET_KEY=None) +def test_missing_secret_key_skips_request(): + """ + With no key configured the call could only ever 401, so it is not attempted. + Returning {} leaves `description_binary` untouched upstream (the caller guards + on `if external_data:`), which degrades duplication rather than corrupting it. + """ + with ( + patch("plane.bgtasks.copy_s3_object.requests.post") as mock_post, + patch("plane.bgtasks.copy_s3_object.log_exception") as mock_log, + ): + result = sync_with_external_service("PAGE", "

hello

") + + assert result == {} + mock_post.assert_not_called() + # The misconfiguration must be surfaced, not swallowed silently. + mock_log.assert_called_once() + + +@override_settings(LIVE_URL=LIVE_URL, LIVE_SERVER_SECRET_KEY="") +def test_empty_secret_key_treated_as_missing(): + """An empty string is a misconfiguration, not a valid credential.""" + with ( + patch("plane.bgtasks.copy_s3_object.requests.post") as mock_post, + patch("plane.bgtasks.copy_s3_object.log_exception"), + ): + result = sync_with_external_service("PAGE", "

hello

") + + assert result == {} + mock_post.assert_not_called() + + +@override_settings(LIVE_URL=None, LIVE_SERVER_SECRET_KEY=SECRET) +def test_no_live_url_short_circuits(): + """Deployments without a Live service must not attempt the call at all.""" + with patch("plane.bgtasks.copy_s3_object.requests.post") as mock_post: + result = sync_with_external_service("PAGE", "

hello

") + + assert result == {} + mock_post.assert_not_called() + + +@override_settings(LIVE_URL=LIVE_URL, LIVE_SERVER_SECRET_KEY=SECRET) +def test_non_200_returns_empty_dict(): + """A rejected call (e.g. a stale key on one side) must not raise.""" + with patch("plane.bgtasks.copy_s3_object.requests.post", return_value=MagicMock(status_code=401)): + result = sync_with_external_service("PAGE", "

hello

") + + assert result == {} + + +@override_settings(LIVE_URL=LIVE_URL, LIVE_SERVER_SECRET_KEY=SECRET) +def test_variant_depends_on_entity_name(): + """Guard the existing contract while changing the auth around it.""" + 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", "

x

") + assert mock_post.call_args.kwargs["json"]["variant"] == "rich" + + sync_with_external_service("ISSUE", "

x

") + assert mock_post.call_args.kwargs["json"]["variant"] == "document" diff --git a/apps/live/src/controllers/document.controller.ts b/apps/live/src/controllers/document.controller.ts index 3a0282f90d4..ed607cefdc9 100644 --- a/apps/live/src/controllers/document.controller.ts +++ b/apps/live/src/controllers/document.controller.ts @@ -7,10 +7,11 @@ import type { Request, Response } from "express"; import { z } from "zod"; // helpers -import { Controller, Post } from "@plane/decorators"; +import { Controller, Middleware, Post } from "@plane/decorators"; import { convertHTMLDocumentToAllFormats } from "@plane/editor"; // logger import { logger } from "@plane/logger"; +import { requireSecretKey } from "@/lib/auth-middleware"; import type { TConvertDocumentRequestBody } from "@/types"; // Define the schema with more robust validation @@ -25,7 +26,15 @@ const convertDocumentSchema = z.object({ @Controller("/convert-document") export class DocumentController { + /** + * Server-to-server only: the sole caller is the API's `copy_s3_object` background + * task (page / work-item duplication). It was previously reachable unauthenticated + * by anyone who could hit the Live service, which made an expensive HTML -> Y.js + * conversion available as free compute to the internet (GHSA-55gq-rf47-9pqx, + * CWE-306). Callers must now present `live-server-secret-key`. + */ @Post("/") + @Middleware(requireSecretKey) async convertDocument(req: Request, res: Response) { try { // Validate request body diff --git a/apps/live/src/lib/pdf/node-renderers.tsx b/apps/live/src/lib/pdf/node-renderers.tsx index 003d21f552a..c08daeb22c1 100644 --- a/apps/live/src/lib/pdf/node-renderers.tsx +++ b/apps/live/src/lib/pdf/node-renderers.tsx @@ -8,6 +8,7 @@ import { Image, Link, Text, View } from "@react-pdf/renderer"; import type { Style } from "@react-pdf/types"; import type { ReactElement } from "react"; import { CORE_EXTENSIONS } from "@plane/editor"; +import { isSafeImageSrc } from "@/lib/url-security"; import { BACKGROUND_COLORS, EDITOR_BACKGROUND_COLORS, resolveColorForPdf, TEXT_COLORS } from "./colors"; import { CheckIcon, ClipboardIcon, DocumentIcon, GlobeIcon, LightbulbIcon, LinkIcon } from "./icons"; import { applyMarks } from "./mark-renderers"; @@ -272,6 +273,18 @@ export const nodeRenderers: NodeRendererRegistry = { ? { alignItems: "flex-end" as const } : { alignItems: "flex-start" as const }; + // SSRF guard (GHSA-55gq-rf47-9pqx). `src` comes from page content, and + // @react-pdf/image will fetch() any URL with a host — including internal + // Docker service names — or fs.readFile() a bare path. Anything we are not + // willing to fetch renders as a placeholder instead. + if (!isSafeImageSrc(src)) { + return ( + + [Image unavailable] + + ); + } + return ( [Image: {assetId.slice(0, 8)}...] diff --git a/apps/live/src/lib/url-security.ts b/apps/live/src/lib/url-security.ts new file mode 100644 index 00000000000..7f3f58ee0e2 --- /dev/null +++ b/apps/live/src/lib/url-security.ts @@ -0,0 +1,180 @@ +/** + * Copyright (c) 2023-present Plane Software, Inc. and contributors + * SPDX-License-Identifier: AGPL-3.0-only + * See the LICENSE file for details. + */ + +import net from "node:net"; + +/** + * SSRF guards for URLs that the Live service may cause to be fetched. + * + * Context (GHSA-55gq-rf47-9pqx): the PDF exporter renders TipTap `image` nodes by + * handing `node.attrs.src` straight to `@react-pdf/image`, which calls `fetch()` on + * anything with a host. Because the Live container shares a Docker network with the + * API, database, Redis, RabbitMQ and MinIO, an unvalidated `src` turns PDF export + * into a request forgery primitive against internal-only services. + * + * A prefix check such as `src.startsWith("http")` does NOT close this: the payloads + * that matter — `http://api:8000/`, `http://plane-minio:9000/` — all start with + * "http". The scheme is irrelevant; the *destination* is what has to be judged. + */ + +/** Schemes we are willing to hand to the PDF image pipeline. */ +const ALLOWED_SCHEMES = new Set(["http:", "https:", "data:"]); + +/** + * Hostname suffixes that only ever resolve inside a private network. + * Compared against the lowercased hostname, with a leading dot to avoid + * matching a public registrable domain that merely ends in these letters. + */ +const BLOCKED_HOST_SUFFIXES = [".local", ".localhost", ".internal", ".home.arpa", ".lan"]; + +/** Bare hostnames that need no DNS to be dangerous. */ +const BLOCKED_HOST_EXACT = new Set(["localhost", "metadata", "metadata.google.internal"]); + +/** + * Returns true when an IPv4 literal falls in a range that must never be fetched. + * Ranges follow IANA special-purpose registries rather than a hand-rolled + * "private IP" list, so CGNAT and benchmarking space are covered too. + */ +const isBlockedIPv4 = (ip: string): boolean => { + const parts = ip.split(".").map((p) => Number(p)); + if (parts.length !== 4 || parts.some((p) => !Number.isInteger(p) || p < 0 || p > 255)) { + // Not a canonical dotted quad — callers treat unparseable hosts as unsafe. + return true; + } + const [a, b] = 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 + if (a === 127) return true; // 127.0.0.0/8 loopback + if (a === 100 && b >= 64 && b <= 127) return true; // 100.64.0.0/10 CGNAT + 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 + + return false; +}; + +/** Returns true when an IPv6 literal must never be fetched. */ +const isBlockedIPv6 = (ip: string): boolean => { + const addr = ip.toLowerCase().replace(/^\[|\]$/g, ""); + + // IPv4-mapped (::ffff:127.0.0.1) and IPv4-compatible forms: judge the embedded v4. + const mapped = addr.match(/^(?:::ffff:|::)((?:\d{1,3}\.){3}\d{1,3})$/); + if (mapped?.[1]) return isBlockedIPv4(mapped[1]); + + if (addr === "::" || addr === "::1") return true; // unspecified / loopback + if (addr.startsWith("fe8") || addr.startsWith("fe9") || addr.startsWith("fea") || addr.startsWith("feb")) return true; // fe80::/10 link-local + // fec0::/10 deprecated site-local. Kept in step with the Python guard's + // _BLOCKED_NETWORKS (apps/api/plane/utils/ip_address.py) — the two lists must not + // drift, or one service will block a range the other happily fetches. + if (addr.startsWith("fec") || addr.startsWith("fed") || addr.startsWith("fee") || addr.startsWith("fef")) return true; + if (addr.startsWith("fc") || addr.startsWith("fd")) return true; // fc00::/7 unique local + if (addr.startsWith("ff")) return true; // ff00::/8 multicast + if (addr.startsWith("64:ff9b:")) return true; // 64:ff9b::/96 + 64:ff9b:1::/48 NAT64 + if (addr.startsWith("2002:")) return true; // 6to4 — can wrap a private v4 + if (/^2001:(0{1,4})?:/.test(addr)) return true; // 2001::/32 Teredo + if (addr.startsWith("::ffff:")) return true; // any other IPv4-mapped form + + return false; +}; + +/** + * Returns true when `host` is an IP literal pointing somewhere we refuse to fetch, + * or a numeric/obfuscated host form that is not a canonical address at all. + * + * Obfuscated encodings (`0x7f000001`, `2130706433`, `127.1`) are rejected outright: + * some HTTP clients expand them to loopback, none of them are legitimate image + * hosts, and normalising every variant is a losing game. + */ +export const isBlockedHostLiteral = (host: string): boolean => { + const bare = host.replace(/^\[|\]$/g, ""); + + const ipVersion = net.isIP(bare); + if (ipVersion === 4) return isBlockedIPv4(bare); + if (ipVersion === 6) return isBlockedIPv6(bare); + + // Hex (0x…), octal-ish, decimal, or short-form dotted numbers — never a real host. + if (/^0x[0-9a-f]+$/i.test(bare)) return true; + if (/^[0-9]+$/.test(bare)) return true; + if (/^[0-9.]+$/.test(bare)) return true; + + return false; +}; + +/** + * Returns true when a hostname is safe enough to hand to the image fetcher. + * + * Single-label hostnames are refused because that is exactly the shape of a Docker + * Compose service name — `api`, `web`, `plane-db`, `plane-redis`, `plane-minio` — + * which is the primary escalation path in this advisory. Public image hosts always + * carry a dot. + */ +const isAllowedHostname = (hostname: string): boolean => { + const host = hostname.toLowerCase(); + + if (!host) return false; + if (BLOCKED_HOST_EXACT.has(host)) return false; + if (BLOCKED_HOST_SUFFIXES.some((suffix) => host.endsWith(suffix))) return false; + if (isBlockedHostLiteral(host)) return false; + // No dot => single-label => container/service name on the internal network. + if (!host.includes(".")) return false; + // A trailing dot ("api.") sidesteps the check above without adding a real label. + if (host.endsWith(".")) return false; + + return true; +}; + +/** + * Decides whether a TipTap image `src` may be passed to the PDF image pipeline. + * + * `data:` URIs are allowed because the asset pipeline deliberately pre-fetches + * images server-side and inlines them as `data:image/jpeg;base64,…`; those never + * touch the network again at render time. + * + * NOTE ON DNS REBINDING: for an `http(s)` host that clears these checks we cannot + * pin the resolved address here — the renderer is synchronous and the actual + * `fetch()` happens inside `@react-pdf/image`, out of our reach. A hostname under + * attacker control that resolves to a blocked address therefore remains a residual + * TOCTOU. Closing it properly means pre-fetching raw image nodes the way + * `imageComponent` already pre-fetches assets, then rendering only `data:` URIs. + * Tracked as follow-up to SECUR-245 — do not mistake this helper for a complete + * SSRF defence on the http(s) path. + */ +export const isSafeImageSrc = (src: string): boolean => { + if (!src) return false; + + const trimmed = src.trim(); + if (!trimmed) return false; + + // Reject control characters and whitespace, which URL parsers strip and + // which have historically been used to smuggle a scheme past naive checks. + // oxlint-disable-next-line no-control-regex -- intentional: these are exactly what we reject + if (/[\u0000-\u0020\u007F]/.test(trimmed)) return false; + + let parsed: URL; + try { + parsed = new URL(trimmed); + } catch { + // Relative paths and bare filesystem paths land here. `@react-pdf/image` + // would hand those to fs.readFile(), so they are refused. + return false; + } + + if (!ALLOWED_SCHEMES.has(parsed.protocol)) return false; + + // data: carries its payload inline; there is no host to judge. + if (parsed.protocol === "data:") return trimmed.toLowerCase().startsWith("data:image/"); + + // Credentials in an image URL are never legitimate and can confuse host parsing. + if (parsed.username || parsed.password) return false; + + return isAllowedHostname(parsed.hostname); +}; diff --git a/apps/live/tests/lib/url-security.test.ts b/apps/live/tests/lib/url-security.test.ts new file mode 100644 index 00000000000..c41e238ebd3 --- /dev/null +++ b/apps/live/tests/lib/url-security.test.ts @@ -0,0 +1,201 @@ +/** + * Copyright (c) 2023-present Plane Software, Inc. and contributors + * SPDX-License-Identifier: AGPL-3.0-only + * See the LICENSE file for details. + */ + +import { describe, expect, it } from "vitest"; +import { isBlockedHostLiteral, isSafeImageSrc } from "@/lib/url-security"; + +describe("isSafeImageSrc — GHSA-55gq-rf47-9pqx", () => { + describe("advisory payloads: internal Docker service names", () => { + // The escalation path named in the advisory. Every one of these starts with + // "http", which is why the imageComponent-style startsWith("http") guard + // does not close this vulnerability. + it.each([ + "http://api:8000/api/workspaces/", + "http://plane-minio:9000/uploads/", + "http://plane-db:5432/", + "http://plane-redis:6379/", + "http://plane-mq:5672/", + "http://web:3000/", + "http://admin:3000/", + "http://space:3000/", + "http://live:3000/", + ])("rejects %s", (src) => { + expect(isSafeImageSrc(src)).toBe(false); + }); + + it("rejects a single-label host even over https", () => { + expect(isSafeImageSrc("https://api/")).toBe(false); + }); + + it("rejects a trailing-dot host that would otherwise look multi-label", () => { + expect(isSafeImageSrc("http://api./")).toBe(false); + }); + }); + + describe("cloud metadata and loopback", () => { + it.each([ + "http://169.254.169.254/latest/meta-data/", + "http://metadata.google.internal/computeMetadata/v1/", + "http://metadata/computeMetadata/v1/", + "http://localhost:8000/", + "http://127.0.0.1:8000/", + "http://127.1.2.3/", + "http://[::1]/", + "http://[::ffff:127.0.0.1]/", + ])("rejects %s", (src) => { + expect(isSafeImageSrc(src)).toBe(false); + }); + }); + + describe("private, CGNAT, link-local and reserved ranges", () => { + it.each([ + "http://10.0.0.5/", + "http://172.16.0.1/", + "http://172.31.255.254/", + "http://192.168.1.1/", + "http://100.64.0.1/", // CGNAT — missed by naive "private IP" lists + "http://100.127.255.255/", + "http://0.0.0.0/", + "http://224.0.0.1/", // multicast + "http://255.255.255.255/", + "http://198.18.0.1/", // benchmarking + "http://[fd00::1]/", // IPv6 unique local + "http://[fe80::1]/", // IPv6 link-local + "http://[ff02::1]/", // IPv6 multicast + "http://[fec0::1]/", // IPv6 deprecated site-local + "http://[2002:7f00:1::]/", // 6to4 wrapping 127.0.0.1 + "http://[2001::1]/", // Teredo + "http://[64:ff9b::7f00:1]/", // NAT64 wrapping 127.0.0.1 + "http://[64:ff9b:1::1]/", // NAT64 local-use prefix + ])("rejects %s", (src) => { + expect(isSafeImageSrc(src)).toBe(false); + }); + + it("allows a public IP just outside a blocked range", () => { + // 100.63.x is public; the CGNAT block starts at 100.64. + expect(isSafeImageSrc("http://100.63.0.1/")).toBe(true); + // 172.32.x is public; the private block ends at 172.31. + expect(isSafeImageSrc("http://172.32.0.1/")).toBe(true); + }); + }); + + describe("obfuscated address encodings", () => { + // Not canonical addresses, but several HTTP clients expand them to loopback. + it.each([ + "http://2130706433/", // decimal 127.0.0.1 + "http://0x7f000001/", // hex 127.0.0.1 + "http://127.1/", // short-form 127.0.0.1 + "http://0/", // shorthand for 0.0.0.0 + ])("rejects %s", (src) => { + expect(isSafeImageSrc(src)).toBe(false); + }); + }); + + describe("scheme handling", () => { + it.each([ + "file:///etc/passwd", + "ftp://example.com/x.png", + "gopher://example.com/", + "javascript:alert(1)", + "vbscript:msgbox(1)", + "blob:https://example.com/abc", + ])("rejects %s", (src) => { + expect(isSafeImageSrc(src)).toBe(false); + }); + + it("rejects bare filesystem paths that would reach fs.readFile", () => { + // The advisory's secondary local-file-read finding. + expect(isSafeImageSrc("/etc/passwd")).toBe(false); + expect(isSafeImageSrc("./relative.png")).toBe(false); + expect(isSafeImageSrc("../../etc/hosts")).toBe(false); + }); + + it("allows image data URIs (the asset pipeline's own output)", () => { + expect(isSafeImageSrc("data:image/jpeg;base64,/9j/4AAQSkZJRg==")).toBe(true); + expect(isSafeImageSrc("data:image/png;base64,iVBORw0KGgo=")).toBe(true); + }); + + it("rejects non-image data URIs", () => { + expect(isSafeImageSrc("data:text/html,")).toBe(false); + expect(isSafeImageSrc("data:application/javascript,alert(1)")).toBe(false); + }); + }); + + describe("whitespace and control-character smuggling", () => { + // The same class of bypass as GHSA-v2vv-7wq3-8w2j: URL parsers strip these, + // so a check performed before stripping can be walked straight past. + it.each([ + "\thttp://api:8000/", + "\nhttp://api:8000/", + "\rhttp://api:8000/", + " http://127.0.0.1/", + "http://api\t:8000/", + "\u0000http://api:8000/", + "\u00A0http://api:8000/", // non-breaking space + "\uFEFFhttp://api:8000/", // BOM + ])("rejects %j", (src) => { + expect(isSafeImageSrc(src)).toBe(false); + }); + }); + + describe("embedded credentials", () => { + it("rejects URLs carrying credentials", () => { + expect(isSafeImageSrc("http://user:pass@images.example.com/a.png")).toBe(false); + // Credentials can also be used to make the real host hard to read. + expect(isSafeImageSrc("http://images.example.com@127.0.0.1/a.png")).toBe(false); + }); + }); + + describe("internal-only hostname suffixes", () => { + it.each([ + "http://printer.local/x.png", + "http://app.localhost/x.png", + "http://svc.internal/x.png", + "http://box.lan/x.png", + "http://thing.home.arpa/x.png", + ])("rejects %s", (src) => { + expect(isSafeImageSrc(src)).toBe(false); + }); + }); + + describe("legitimate images still render", () => { + it.each([ + "https://images.example.com/photo.png", + "http://cdn.example.org/a/b/c.jpg", + "https://user-images.githubusercontent.com/1/2.png", + "https://example.co.uk/img.webp", + "https://sub.domain.example.com:8443/img.png", + ])("allows %s", (src) => { + expect(isSafeImageSrc(src)).toBe(true); + }); + }); + + describe("empty and malformed input", () => { + it.each(["", " ", "not a url", "http://", "://example.com"])("rejects %j", (src) => { + expect(isSafeImageSrc(src)).toBe(false); + }); + }); +}); + +describe("isBlockedHostLiteral", () => { + it("classifies canonical IPv4 literals", () => { + expect(isBlockedHostLiteral("127.0.0.1")).toBe(true); + expect(isBlockedHostLiteral("10.1.2.3")).toBe(true); + expect(isBlockedHostLiteral("8.8.8.8")).toBe(false); + expect(isBlockedHostLiteral("1.1.1.1")).toBe(false); + }); + + it("classifies IPv6 literals with and without brackets", () => { + expect(isBlockedHostLiteral("::1")).toBe(true); + expect(isBlockedHostLiteral("[::1]")).toBe(true); + expect(isBlockedHostLiteral("2606:4700:4700::1111")).toBe(false); + }); + + it("treats real hostnames as non-literals", () => { + expect(isBlockedHostLiteral("example.com")).toBe(false); + expect(isBlockedHostLiteral("api")).toBe(false); + }); +}); From 7dd98832f53b6e4a88bf47f0c26cc74b35b0a09c Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Tue, 4 Aug 2026 17:27:02 +0530 Subject: [PATCH 2/2] [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 d2ffbe28560..73f5a48dd1d 100644 --- a/apps/api/plane/bgtasks/copy_s3_object.py +++ b/apps/api/plane/bgtasks/copy_s3_object.py @@ -18,6 +18,12 @@ 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 ab449a3ac1c..0ed5d1cc892 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 @@ 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 7f3f58ee0e2..973ef102ab2 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 c41e238ebd3..35da757d2ea 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", () => {