[SECUR-245] fix(security): guard PDF image srcs and authenticate /convert-document/ (GHSA-55gq-rf47-9pqx)

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 = {
@@ -77,7 +83,24 @@ 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},
+            timeout=LIVE_REQUEST_TIMEOUT,
+        )
         if response.status_code == 200:
             return response.json()
     except requests.RequestException as e:

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

apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py

@@ -0,0 +1,137 @@
+# 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
+
+import requests
+from django.test import override_settings
+
+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"
+
+
+@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", "<p>hello</p>")
+
+    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=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():
+    """
+    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", "<p>hello</p>")
+
+    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", "<p>hello</p>")
+
+    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", "<p>hello</p>")
+
+    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", "<p>hello</p>")
+
+    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", "<p>x</p>")
+        assert mock_post.call_args.kwargs["json"]["variant"] == "rich"
+
+        sync_with_external_service("ISSUE", "<p>x</p>")
+        assert mock_post.call_args.kwargs["json"]["variant"] == "document"

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

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 (
+        <View key={ctx.getKey()} style={[pdfStyles.imagePlaceholder, alignmentStyle]}>
+          <Text style={pdfStyles.imagePlaceholderText}>[Image unavailable]</Text>
+        </View>
+      );
+    }
+
     return (
       <View key={ctx.getKey()} style={[{ width: "100%" }, alignmentStyle]}>
         <Image
@@ -308,7 +321,12 @@ export const nodeRenderers: NodeRendererRegistry = {
           ? { alignItems: "flex-end" as const }
           : { alignItems: "flex-start" as const };
 
-    if (!resolvedSrc.startsWith("http") && !resolvedSrc.startsWith("data:")) {
+    // 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).
+    if (!isSafeImageSrc(resolvedSrc)) {
       return (
         <View key={ctx.getKey()} style={[pdfStyles.imagePlaceholder, alignmentStyle]}>
           <Text style={pdfStyles.imagePlaceholderText}>[Image: {assetId.slice(0, 8)}...]</Text>