Mirrors/plane
1
0
Fork 0
mirror of https://github.com/makeplane/plane.git synced 2026-09-01 19:48:42 +02:00
Code Issues Packages Projects Releases Wiki Activity

style(security): drop advisory identifiers and shorten comments

Browse Source 
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 <noreply@plane.so>
This commit is contained in:
Manish Gupta
2026-08-07 15:48:31 +05:30
parent 57b5fb17ad
commit 5a0286b76f
6 changed files with 45 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
apps/api/plane/bgtasks/copy_s3_object.py
View File

@@ -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(

12
apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py
View File

@@ -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

7
apps/live/src/controllers/document.controller.ts
View File

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

15
apps/live/src/lib/pdf/node-renderers.tsx
View File

@@ -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 (
<View key={ctx.getKey()} style={[pdfStyles.imagePlaceholder, alignmentStyle]}>
@@ -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 (
<View key={ctx.getKey()} style={[pdfStyles.imagePlaceholder, alignmentStyle]}>

59
apps/live/src/lib/url-security.ts
View File

@@ -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;

15
apps/live/tests/lib/url-security.test.ts
View File

@@ -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/",