fix(LiveControls): restrict postMessage handler to trusted origins - #1183
fix(LiveControls): restrict postMessage handler to trusted origins#11830xcucumbersalad wants to merge 2 commits into
Conversation
The onMessage handler executed eval() on script payloads from any sender. Gate it behind an origin allowlist (deco.cx, *.deco.cx, same-origin) to prevent cross-origin XSS via the editor::inject message type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthrough
ChangesMessage Origin Validation & Runtime Injection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🤖 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 `@components/LiveControls.tsx`:
- Around line 123-130: The handler onMessage should validate runtime shape of
event.data before switching: ensure data is a non-null object and has a string
"type" property (and any other expected props) and bail out if not, to avoid
runtime exceptions and accidental branching; update the onMessage function to
perform this runtime check (e.g., typeof data === "object" && data !== null &&
typeof (data as any).type === "string") before the switch(data.type) so only
validated payloads reach the sensitive branches.
- Around line 113-121: Update the trusted-origin allowlist used by
TRUSTED_ORIGINS / isTrustedOrigin to include the missing admin hosts so valid
editor messages aren't dropped: add "https://admin-cx.deco.page",
"https://deco.chat", "https://admin.decocms.com", "https://decocms.com",
"https://studio.decocms.com" to TRUSTED_ORIGINS and ensure any runtime-admin
list (ADMIN_DOMAINS) is consulted (e.g., merge ADMIN_DOMAINS into
TRUSTED_ORIGINS or check it inside isTrustedOrigin) while preserving the
existing pattern checks for subdomains (*.deco.cx) and the
WINDOW.location.origin comparison.
🪄 Autofix (Beta)
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
Run ID: 23fbd359-3fb2-4541-9ebf-da8c8be9eb71
📒 Files selected for processing (1)
components/LiveControls.tsx
- Add admin hosts (admin-cx.deco.page, deco.chat, admin.decocms.com, decocms.com, studio.decocms.com) so legitimate editor messages aren't dropped by the postMessage origin gate. - Inject runtime adminDomains (which includes ADMIN_DOMAINS env entries) via a __DECO_TRUSTED_ORIGINS JSON script tag so self-hosted admins configured at deploy time are also trusted. - Validate event.data is a non-null object with a string `type` before switching, preventing runtime exceptions on malformed payloads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/LiveControls.tsx (1)
124-132: ⚡ Quick winConsider validating runtime origin strings to HTTPS scheme.
runtimeTrustedOriginsonly checkstypeof o === "string". AnadminDomainsentry like"http://attacker.com"or""would pass this filter and be added toTRUSTED_ORIGINS, potentially trusting non-HTTPS origins. Adding a scheme check (o.startsWith("https://")) here adds defense-in-depth without any runtime cost.♻️ Proposed change
- return Array.isArray(parsed) ? parsed.filter((o) => typeof o === "string") : []; + return Array.isArray(parsed) ? parsed.filter((o) => typeof o === "string" && o.startsWith("https://")) : [];🤖 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 `@components/LiveControls.tsx` around lines 124 - 132, The runtimeTrustedOrigins initializer currently only checks typeof o === "string", so update the filter in the runtimeTrustedOrigins IIFE to only accept non-empty strings that use the HTTPS scheme (e.g., o && typeof o === "string" && o.startsWith("https://")), rejecting empty strings and non-HTTPS origins; this provides defense-in-depth before those values are merged into TRUSTED_ORIGINS.
🤖 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 `@components/LiveControls.tsx`:
- Around line 223-229: The embedded JSON in LiveControls.tsx currently uses
dangerouslySetInnerHTML with JSON.stringify(adminDomains) (the script tag
id="__DECO_TRUSTED_ORIGINS"), which doesn't escape '<' characters; change the
value to a sanitized string by calling
JSON.stringify(adminDomains).replace(/</g, '\\u003c') (or equivalent) before
assigning to __html so any '<' is escaped and the script is safe against
accidental </script> sequences while preserving the existing adminDomains data
and use of dangerouslySetInnerHTML.
---
Nitpick comments:
In `@components/LiveControls.tsx`:
- Around line 124-132: The runtimeTrustedOrigins initializer currently only
checks typeof o === "string", so update the filter in the runtimeTrustedOrigins
IIFE to only accept non-empty strings that use the HTTPS scheme (e.g., o &&
typeof o === "string" && o.startsWith("https://")), rejecting empty strings and
non-HTTPS origins; this provides defense-in-depth before those values are merged
into TRUSTED_ORIGINS.
🪄 Autofix (Beta)
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
Run ID: 36e04d9f-dffa-4e6e-9665-84e03695e117
📒 Files selected for processing (1)
components/LiveControls.tsx
| <script | ||
| type="application/json" | ||
| id="__DECO_TRUSTED_ORIGINS" | ||
| dangerouslySetInnerHTML={{ | ||
| __html: JSON.stringify(adminDomains), | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect adminDomains declaration and its source in utils/admin.ts
fd -i 'admin.ts' --type f | xargs grep -n 'adminDomains\|ADMIN_DOMAINS\|env\|Deno.env\|process.env' -A3 -B1Repository: deco-cx/deco
Length of output: 868
🏁 Script executed:
fd -type f -name 'admin.ts' | xargs head -60 | cat -nRepository: deco-cx/deco
Length of output: 227
🏁 Script executed:
fd -t f -name 'admin.ts' | xargs cat | head -60Repository: deco-cx/deco
Length of output: 287
🏁 Script executed:
fd -t f -name 'admin.ts' -x cat {} | head -60Repository: deco-cx/deco
Length of output: 287
🏁 Script executed:
git ls-files | grep -i 'admin.ts' | xargs catRepository: deco-cx/deco
Length of output: 1829
JSON.stringify(adminDomains) in a <script> tag should escape < characters for defense in depth.
While adminDomains is populated from the ADMIN_DOMAINS environment variable and validated as valid URL origins (which cannot naturally contain </script> sequences), embedding JSON directly in <script> tags without escaping < is an anti-pattern that should be avoided defensively. The HTML parser treats </ as a tag terminator regardless of the script type, so any future code changes or data sources that bypass the current URL validation could introduce XSS.
The fix is a one-liner and should be applied to prevent this pattern from being copied elsewhere:
🛡️ Proposed fix
- __html: JSON.stringify(adminDomains),
+ __html: JSON.stringify(adminDomains).replace(/</g, "\\u003c"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <script | |
| type="application/json" | |
| id="__DECO_TRUSTED_ORIGINS" | |
| dangerouslySetInnerHTML={{ | |
| __html: JSON.stringify(adminDomains), | |
| }} | |
| /> | |
| <script | |
| type="application/json" | |
| id="__DECO_TRUSTED_ORIGINS" | |
| dangerouslySetInnerHTML={{ | |
| __html: JSON.stringify(adminDomains).replace(/</g, "\\u003c"), | |
| }} | |
| /> |
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 225-225: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🤖 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 `@components/LiveControls.tsx` around lines 223 - 229, The embedded JSON in
LiveControls.tsx currently uses dangerouslySetInnerHTML with
JSON.stringify(adminDomains) (the script tag id="__DECO_TRUSTED_ORIGINS"), which
doesn't escape '<' characters; change the value to a sanitized string by calling
JSON.stringify(adminDomains).replace(/</g, '\\u003c') (or equivalent) before
assigning to __html so any '<' is escaped and the script is safe against
accidental </script> sequences while preserving the existing adminDomains data
and use of dangerouslySetInnerHTML.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="components/LiveControls.tsx">
<violation number="1" location="components/LiveControls.tsx:227">
P2: Escape `<` characters in the JSON output to prevent potential script tag breakout. The HTML parser treats `</script>` as a closing tag regardless of `type="application/json"`, so if `adminDomains` ever contains a string with `</script>`, it would allow injection. Apply `.replace(/</g, "\\u003c")` after `JSON.stringify()` for defense in depth.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| type="application/json" | ||
| id="__DECO_TRUSTED_ORIGINS" | ||
| dangerouslySetInnerHTML={{ | ||
| __html: JSON.stringify(adminDomains), |
There was a problem hiding this comment.
P2: Escape < characters in the JSON output to prevent potential script tag breakout. The HTML parser treats </script> as a closing tag regardless of type="application/json", so if adminDomains ever contains a string with </script>, it would allow injection. Apply .replace(/</g, "\\u003c") after JSON.stringify() for defense in depth.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/LiveControls.tsx, line 227:
<comment>Escape `<` characters in the JSON output to prevent potential script tag breakout. The HTML parser treats `</script>` as a closing tag regardless of `type="application/json"`, so if `adminDomains` ever contains a string with `</script>`, it would allow injection. Apply `.replace(/</g, "\\u003c")` after `JSON.stringify()` for defense in depth.</comment>
<file context>
@@ -197,6 +220,13 @@ function LiveControls({ site, page, flags }: Props) {
+ type="application/json"
+ id="__DECO_TRUSTED_ORIGINS"
+ dangerouslySetInnerHTML={{
+ __html: JSON.stringify(adminDomains),
+ }}
+ />
</file context>
| __html: JSON.stringify(adminDomains), | |
| __html: JSON.stringify(adminDomains).replace(/</g, "\\u003c"), |
1 participant
The onMessage handler executed eval() on script payloads from any sender. Gate it behind an origin allowlist (deco.cx, *.deco.cx, same-origin) to prevent cross-origin XSS via the editor::inject message type.
Summary by cubic
Lock down LiveControls postMessage handling to trusted origins and validate messages to block cross-origin XSS from editor::inject. Expands the allowlist and supports runtime admin domains so legitimate editor messages keep working.
isTrustedOriginwith same-origin,*.deco.cx,deco.cx,admin.deco.cx,play.deco.cx, plusadmin-cx.deco.page,deco.chat,admin.decocms.com,decocms.com,studio.decocms.com.adminDomainsvia a__DECO_TRUSTED_ORIGINSJSON script tag.event.datais a non-null object with a stringtype; ignore untrusted or malformed messages.Written for commit 3517f04. Summary will update on new commits.
Summary by CodeRabbit