From 5a0286b76fcbd87d1a2a4bd76c282b3cb9c552a7 Mon Sep 17 00:00:00 2001 From: Manish Gupta Date: Fri, 7 Aug 2026 15:48:31 +0530 Subject: [PATCH] style(security): drop advisory identifiers and shorten comments Advisory IDs map a published advisory to the exact fix location and the repo is public. Keep the invariant and the non-obvious failure mode; drop the narration, restated code and stale line references. Co-authored-by: Plane AI --- apps/api/plane/bgtasks/copy_s3_object.py | 9 ++- .../unit/bg_tasks/test_copy_s3_object_auth.py | 12 ++-- .../src/controllers/document.controller.ts | 7 +-- apps/live/src/lib/pdf/node-renderers.tsx | 15 ++--- apps/live/src/lib/url-security.ts | 59 +++++++------------ apps/live/tests/lib/url-security.test.ts | 15 +++-- 6 files changed, 45 insertions(+), 72 deletions(-) diff --git a/apps/api/plane/bgtasks/copy_s3_object.py b/apps/api/plane/bgtasks/copy_s3_object.py index 73f5a48dd1..f604f0e0f5 100644 --- a/apps/api/plane/bgtasks/copy_s3_object.py +++ b/apps/api/plane/bgtasks/copy_s3_object.py @@ -19,9 +19,8 @@ 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. +# timeout, so without one a Live service that stalls after accepting the connection +# pins a Celery worker forever. The read budget is wide: conversion is genuinely slow. LIVE_REQUEST_TIMEOUT = (5, 30) @@ -83,8 +82,8 @@ def sync_with_external_service(entity_name, description_html): url = normalize_url_path(f"{live_url}/convert-document/") - # The Live service authenticates this endpoint with a shared secret - # (GHSA-55gq-rf47-9pqx). Without the header the request is rejected as 401. + # The Live service authenticates this endpoint with a shared secret. + # Without the header the request is rejected as 401. secret_key = settings.LIVE_SERVER_SECRET_KEY if not secret_key: log_exception( 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 0ed5d1cc89..2a6f6ff0fc 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 @@ -3,15 +3,11 @@ # See the LICENSE file for details. """ -Authentication of the API -> Live `/convert-document/` call (GHSA-55gq-rf47-9pqx). +Authentication of the API -> Live `/convert-document/` call. -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. +Live now gates the endpoint on the `live-server-secret-key` header, and this task is +its only caller: the header must actually be sent, and a missing key must fail loudly +rather than firing a request that can only 401. Pure unit tests — no DB, no network. """ from unittest.mock import MagicMock, patch diff --git a/apps/live/src/controllers/document.controller.ts b/apps/live/src/controllers/document.controller.ts index ed607cefdc..25238f8857 100644 --- a/apps/live/src/controllers/document.controller.ts +++ b/apps/live/src/controllers/document.controller.ts @@ -28,10 +28,9 @@ const convertDocumentSchema = z.object({ 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`. + * task. It was previously reachable unauthenticated by anyone who could hit the + * Live service, making an expensive HTML -> Y.js conversion free compute for the + * internet. Callers must now present `live-server-secret-key`. */ @Post("/") @Middleware(requireSecretKey) diff --git a/apps/live/src/lib/pdf/node-renderers.tsx b/apps/live/src/lib/pdf/node-renderers.tsx index c08daeb22c..7c2f25909b 100644 --- a/apps/live/src/lib/pdf/node-renderers.tsx +++ b/apps/live/src/lib/pdf/node-renderers.tsx @@ -273,10 +273,9 @@ 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. + // SSRF guard: `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 won't fetch renders as a placeholder. if (!isSafeImageSrc(src)) { return ( @@ -321,11 +320,9 @@ export const nodeRenderers: NodeRendererRegistry = { ? { alignItems: "flex-end" as const } : { alignItems: "flex-start" as const }; - // Normally `resolvedSrc` is the `data:image/jpeg;base64,…` URI produced by the - // service's own pre-fetch, so nothing is fetched at render time. If asset - // resolution failed it is still the raw asset id, which is not a fetchable URL. - // Use the same guard as the `image` renderer rather than a startsWith("http") - // check, which would happily pass http://api:8000/ (GHSA-55gq-rf47-9pqx). + // `resolvedSrc` is normally the pre-fetched `data:image/…` URI, or the raw asset + // id if resolution failed. Same guard as the `image` renderer: a + // startsWith("http") check would happily pass http://api:8000/. if (!isSafeImageSrc(resolvedSrc)) { return ( diff --git a/apps/live/src/lib/url-security.ts b/apps/live/src/lib/url-security.ts index 973ef102ab..ed82c5877c 100644 --- a/apps/live/src/lib/url-security.ts +++ b/apps/live/src/lib/url-security.ts @@ -7,17 +7,12 @@ import net from "node:net"; /** - * SSRF guards for URLs that the Live service may cause to be fetched. + * SSRF guards for URLs 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. + * The PDF exporter hands TipTap `node.attrs.src` to `@react-pdf/image`, which + * fetch()es anything with a host — and Live shares a Docker network with the API, + * database, Redis, RabbitMQ and MinIO. A `startsWith("http")` check is no defence: + * `http://api:8000/` starts with "http" too. The destination is what must be judged. */ /** Schemes we are willing to hand to the PDF image pipeline. */ @@ -56,10 +51,9 @@ const isBlockedIPv4 = (ip: string): boolean => { if (a === 198 && (b === 18 || b === 19)) return true; // 198.18.0.0/15 benchmarking 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. + // The remaining special-purpose blocks are /24s inside otherwise-public /16s, so + // they must be matched on the third octet — testing only the second would blackhole + // real public space (192.0.3.0/24, 198.51.x, 203.0.x) and break legitimate images. 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 @@ -93,12 +87,10 @@ const isBlockedIPv6 = (ip: string): boolean => { }; /** - * 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. + * Returns true when `host` is an IP literal 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, and normalising every variant is a losing game. */ export const isBlockedHostLiteral = (host: string): boolean => { const bare = host.replace(/^\[|\]$/g, ""); @@ -118,10 +110,9 @@ export const isBlockedHostLiteral = (host: string): boolean => { /** * 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. + * Single-label hostnames are refused: that is exactly the shape of a Docker Compose + * service name (`api`, `plane-db`, `plane-minio`), the primary escalation path here. + * Public image hosts always carry a dot. */ const isAllowedHostname = (hostname: string): boolean => { const host = hostname.toLowerCase(); @@ -139,20 +130,12 @@ const isAllowedHostname = (hostname: string): boolean => { }; /** - * 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. + * Decides whether a TipTap image `src` may reach the PDF image pipeline. `data:` is + * allowed because the asset pipeline pre-fetches images server-side and inlines them, + * so nothing is fetched at render time. Not a complete http(s) defence: the fetch + * happens inside `@react-pdf/image`, so a host that passes here but resolves to a + * blocked address is a residual DNS-rebinding TOCTOU (SECUR-245 follow-up). + * TODO(SECUR-245): close it by pre-fetching raw image nodes, as imageComponent does. */ export const isSafeImageSrc = (src: string): boolean => { if (!src) return false; diff --git a/apps/live/tests/lib/url-security.test.ts b/apps/live/tests/lib/url-security.test.ts index 35da757d2e..c38bd10622 100644 --- a/apps/live/tests/lib/url-security.test.ts +++ b/apps/live/tests/lib/url-security.test.ts @@ -7,11 +7,10 @@ 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. +describe("isSafeImageSrc", () => { + describe("internal Docker service names", () => { + // The escalation path that matters: every one of these starts with "http", which + // is why an imageComponent-style startsWith("http") guard does not close it. it.each([ "http://api:8000/api/workspaces/", "http://plane-minio:9000/uploads/", @@ -126,7 +125,7 @@ describe("isSafeImageSrc — GHSA-55gq-rf47-9pqx", () => { }); it("rejects bare filesystem paths that would reach fs.readFile", () => { - // The advisory's secondary local-file-read finding. + // Secondary local-file-read path: @react-pdf/image would fs.readFile these. expect(isSafeImageSrc("/etc/passwd")).toBe(false); expect(isSafeImageSrc("./relative.png")).toBe(false); expect(isSafeImageSrc("../../etc/hosts")).toBe(false); @@ -144,8 +143,8 @@ describe("isSafeImageSrc — GHSA-55gq-rf47-9pqx", () => { }); 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. + // URL parsers strip these, so a check performed before stripping can be walked + // straight past — a well-known class of URL-validation bypass. it.each([ "\thttp://api:8000/", "\nhttp://api:8000/",