[SECUR-245] fix(security): guard PDF image srcs and authenticate /convert-document/ (GHSA-55gq-rf47-9pqx) - #9540
[SECUR-245] fix(security): guard PDF image srcs and authenticate /convert-document/ (GHSA-55gq-rf47-9pqx)#9540mguptahub wants to merge 2 commits into
Conversation
…vert-document/ (GHSA-55gq-rf47-9pqx)
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 `<Image>` 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 <noreply@plane.so>
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds ChangesLive service authentication and SSRF protection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant API as sync_with_external_service
participant Live as document conversion endpoint
participant Renderer as PDF image renderer
API->>Live: POST conversion request with secret header
Live->>Live: validate secret with requireSecretKey
Live->>Renderer: render document images
Renderer->>Renderer: validate image source with isSafeImageSrc
alt unsafe image source
Renderer-->>Live: render [Image unavailable] placeholder
else safe image source
Renderer-->>Live: render image
end
Live-->>API: return conversion response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/live/src/lib/url-security.ts (1)
120-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePublic IPv6 literal hosts are rejected by the dotless-host rule.
parsed.hostnamefor an IPv6 URL is bracketed, for example[2606:4700:4700::1111]. It contains no dot, so Line 128 returnsfalseeven afterisBlockedHostLiteralclassifies the address as allowed. The test atapps/live/tests/lib/url-security.test.tsLines 191-195 asserts the opposite classification at the literal level, so the two layers disagree.The direction is safe. If the rejection is intentional, state it in the comment and lock it with an
isSafeImageSrctest. If it is not intentional, skip the dotless rule for bracketed literals.♻️ Optional: apply the dotless rule only to DNS names
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; + // Bracketed IPv6 literals carry no dot; they were already judged above. + if (host.startsWith("[") && host.endsWith("]")) return true; // No dot => single-label => container/service name on the internal network. if (!host.includes(".")) return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/live/src/lib/url-security.ts` around lines 120 - 133, Update isAllowedHostname so bracketed IPv6 literals classified as allowed by isBlockedHostLiteral are not rejected by the dotless-host check; apply that rule only to DNS names. Preserve blocking for disallowed literals, single-label hostnames, and trailing-dot hostnames, and align the isSafeImageSrc tests with the resulting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/bgtasks/copy_s3_object.py`:
- Line 92: Update the Live request in the copy S3 object flow by passing a
configured bounded connect/read timeout tuple to requests.post. Use the existing
timeout configuration pattern if available, and update
test_copy_s3_object_auth.py to assert that requests.post receives the timeout
argument.
---
Nitpick comments:
In `@apps/live/src/lib/url-security.ts`:
- Around line 120-133: Update isAllowedHostname so bracketed IPv6 literals
classified as allowed by isBlockedHostLiteral are not rejected by the
dotless-host check; apply that rule only to DNS names. Preserve blocking for
disallowed literals, single-label hostnames, and trailing-dot hostnames, and
align the isSafeImageSrc tests with the resulting behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 384e96ff-e215-482b-bc78-681791a871d9
📒 Files selected for processing (7)
apps/api/plane/bgtasks/copy_s3_object.pyapps/api/plane/settings/common.pyapps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.pyapps/live/src/controllers/document.controller.tsapps/live/src/lib/pdf/node-renderers.tsxapps/live/src/lib/url-security.tsapps/live/tests/lib/url-security.test.ts
There was a problem hiding this comment.
Pull request overview
This PR addresses security advisory GHSA-55gq-rf47-9pqx by (1) hardening PDF export image src handling in the Live service to reduce SSRF/local-file-read risk, and (2) enforcing server-to-server authentication on Live’s /convert-document/ endpoint while updating the API background task that calls it.
Changes:
- Introduces
isSafeImageSrc/isBlockedHostLiteralSSRF guard logic and routes PDF image rendering through it with placeholders for unsafe sources. - Applies
requireSecretKeymiddleware to/convert-document/and updates the API duplication task to sendlive-server-secret-key, including new API setting wiring. - Adds extensive Live unit tests for URL filtering and API unit tests for the new header contract / misconfiguration behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/live/src/lib/url-security.ts | Adds URL/IP/scheme validation helpers used to gate PDF image fetching. |
| apps/live/tests/lib/url-security.test.ts | Adds unit tests covering advisory payloads and edge cases for the new URL guard. |
| apps/live/src/lib/pdf/node-renderers.tsx | Uses isSafeImageSrc to block unsafe image sources and render placeholders. |
| apps/live/src/controllers/document.controller.ts | Protects /convert-document/ with requireSecretKey middleware. |
| apps/api/plane/settings/common.py | Adds LIVE_SERVER_SECRET_KEY to Django settings from env. |
| apps/api/plane/bgtasks/copy_s3_object.py | Sends the shared secret header (or short-circuits loudly when missing) for /convert-document/. |
| apps/api/plane/tests/unit/bg_tasks/test_copy_s3_object_auth.py | Adds unit tests asserting the new auth/header behavior and failure modes. |
…Live request timeout 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 <noreply@plane.so>
|
Both review findings addressed in @copilot — over-blocking reserved ranges ✅ fixed, and it was 3 cases not 1Correct, and the pattern held in two more places I'd written the same way. The blocks in question are
Now matched on the third octet. Failure mode was silent — legitimate images would just stop rendering with no error. Verified against the Python guard rather than by inspection. Ran Worth naming: @copilot — missing test coverage for those ranges ✅ addedAdded assertions in both directions, which is what matters here: the four reserved @coderabbitai — unbounded
|
|
Tip For best results, initiate chat on the files or code changes.
The timeout fix prevents stalled Live requests from indefinitely occupying Celery workers. The The boundary tests for both blocked and adjacent allowed ranges correctly prevent the 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/live/src/lib/url-security.ts:166
isSafeImageSrctrims the input before checking for whitespace/control characters, so leading/trailing whitespace (including NBSP/BOM) is silently removed and a URL like" https://images.example.com/x.png"would be accepted. This contradicts the intent of rejecting scheme-smuggling whitespace/control chars; consider failing closed when trimming changes the string and also rejecting NBSP/BOM inside the URL.
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.
What
Fixes the two issues reported as GHSA-55gq-rf47-9pqx in
apps/live, plus the one API-side change needed to keep the existing caller working.Work item: SECUR-245
1. SSRF via PDF image rendering
The
imagenode renderer handednode.attrs.srcstraight to@react-pdf/image, whichfetch()es any URL with a host andfs.readFile()s a bare path. The Live container shares a Docker network withapi,plane-db,plane-redis,plane-mqandplane-minio, so page content could drive requests at internal-only services.Adds
apps/live/src/lib/url-security.tsand routes both<Image>call sites through it; anything unsafe renders a placeholder.Rejected: non-
http(s)/dataschemes · bare and relative filesystem paths · loopback · RFC1918 · CGNAT100.64/10· link-local incl.169.254.169.254· multicast/reserved/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/.lansuffixes · embedded credentials · control-character scheme smuggling.The IPv6 ranges are deliberately kept in step with the Python guard's
_BLOCKED_NETWORKSinapps/api/plane/utils/ip_address.py. The first draft here was missing Teredo2001::/32andfec0::/10— two implementations of one policy drifting apart is how this class of bug keeps coming back.2. Unauthenticated
/convert-document/requireSecretKeyexisted 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 is the API's
copy_s3_objectduplication task, which sentheaders=None. So this PR also:LIVE_SERVER_SECRET_KEYinto Django settings — it was previously only in.env.example.A missing key short-circuits with a logged misconfiguration instead of firing a request that can only 401.
Two corrections to the advisory
/pdf-export/is not unauthenticated and is deliberately untouched.parseRequestrejects requests with noCookieand forwards that cookie 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 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 bypageId. Triggering the SSRF requires a valid session, so this isPR:L, not the reported pre-auth. Suggest reconciling severity before publishing — GitHub metadata says CVSS 8.6 while the advisory body says 7.5.Known residual — please read before approving
DNS rebinding on the
http(s)path is NOT closed here. An attacker-controlled hostname that resolves to a blocked address still gets fetched: the renderer is synchronous and thefetch()happens inside@react-pdf/image, so the resolved address cannot be pinned. This is documented in the helper's doc comment.The proper fix is to pre-fetch raw
imagenodes intodata:URIs exactly asimageComponentalready pre-fetches assets (pdf-export.service.ts:229-230), then accept onlydata:at render time. Tracked as a follow-up to SECUR-245. Flagging it so this is not read as "SSRF fully fixed".Deployment note
Gating
/convert-document/means duplication degrades if the API lacks the key:if external_data:prevents corruption, but the duplicate keeps a staledescription_binary, so its images break.Verified
LIVE_SERVER_SECRET_KEYreachesapi/worker/beat-workerviax-app-envindeployments/cli/community/docker-compose.yml, and that aio auto-generates it on first boot. Not verified: the Helm charts (separate repo) — worth confirming before release so self-hosted K8s users don't hit this.Testing
apps/live/tests/lib/url-security.test.ts) covering every payload named in the advisory, plus range boundaries (e.g.100.63.xallowed /100.64.xblocked) so the CIDR edges are pinned.test_copy_s3_object_auth.py) for the header contract and the missing-key / no-live-url paths. The pre-existing duplication test mockssync_with_external_serviceentirely, so this path had no coverage.tsc --noEmit: 24 errors before and after — zero introduced, all pre-existing unbuilt@plane/logger/@plane/decorators. oxlint 0 errors, oxfmt and ruff clean.Out of scope, worth follow-ups
requireSecretKeycompares with!==(auth-middleware.ts:38) — not constant-time. Pre-existing, but it now actually guards something, socrypto.timingSafeEqualis warranted. Its docstring also claims it acceptsx-admin-secret-key"(preferred)" while the code only readslive-server-secret-key.<Link src={href}>(node-renderers.tsx:65) is unguarded. Not SSRF (no fetch), but it embeds arbitrary URIs includingjavascript:into PDF link annotations.Co-authored-by: Plane AI noreply@plane.so
Summary by CodeRabbit
Security
Bug Fixes
Tests