fix(client): replace innerHTML and dangerouslySetInnerHTML with safe alternatives#41836
Open
subrata71 wants to merge 1 commit into
Open
fix(client): replace innerHTML and dangerouslySetInnerHTML with safe alternatives#41836subrata71 wants to merge 1 commit into
subrata71 wants to merge 1 commit into
Conversation
Contributor
WalkthroughThis PR replaces unsafe HTML injection patterns: component HTML rendering now uses Interweave or plain JSX (no dangerouslySetInnerHTML), tooltip docs render as plain text, and generated STYLE content uses textContent instead of innerHTML. XSS-focused tests are added across components and widget CSS generation. ChangesSecurity hardening - Remove dangerous HTML injection patterns
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/client/src/pages/Editor/GeneratePage/components/CrudInfoModal.tsx (1)
109-111: 💤 Low valueHeads-up:
Interweavedefaults to wrapping output in a<span>.
InfoContentHeadingTextis itself a styled<span>, so the rendered markup becomes<span><span>…</span></span>. Harmless, but if you’d prefer flat output, passtagName="fragment"/noWrap(depending on Interweave version) or render directly into a<div>.🤖 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 `@app/client/src/pages/Editor/GeneratePage/components/CrudInfoModal.tsx` around lines 109 - 111, The Interweave output is wrapped in a <span>, causing nested spans because InfoContentHeadingText is a styled <span>; update the rendering in CrudInfoModal to avoid the extra wrapper by passing Interweave props to render a fragment/no wrap (e.g., tagName="fragment" or noWrap depending on your Interweave version) or replace InfoContentHeadingText with a block element (e.g., a styled <div>) and render <Interweave content={successMessage} /> directly; locate the usage in the component where InfoContentHeadingText wraps Interweave and apply one of these changes to produce flat output.app/client/src/widgets/__tests__/generateAppsmithCssVariables.test.js (2)
69-106: ⚡ Quick winConsider adding behavior-level XSS assertions alongside the
innerHTMLspy.The spy approach is a fair workaround for jsdom’s
<style>parsing limitation, but it only proves "implementation does not callinnerHTML" — it would silently pass if a future refactor switched to e.g.insertAdjacentHTMLorRange.createContextualFragment. A cheap complement is to also assert that the malicious payload is preserved as rawtextContentand no stray<script>/<img>ended up in the document.♻️ Suggested additional assertions
const styleSetterCalls = spy.mock.contexts.filter( (ctx) => ctx?.tagName === "STYLE", ); expect(styleSetterCalls).toHaveLength(0); + const style = document.getElementById(`appsmith-${provider}-css-tokens`); + expect(style.textContent).toContain("</style><script>alert(1)</script>"); + expect(document.querySelectorAll("script")).toHaveLength(0);🤖 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 `@app/client/src/widgets/__tests__/generateAppsmithCssVariables.test.js` around lines 69 - 106, Add behavior-level XSS assertions to the existing tests that spy on Element.prototype.innerHTML: after calling apply = fn(provider); apply({...}) assert that the malicious payload remains as raw textContent on the affected <style> node and that no <script> or <img> elements were actually appended to the document (e.g. search document.querySelectorAll("script")/("img") or inspect the created style element's textContent for the payload). Keep these checks alongside the existing styleSetterCalls assertions so the tests fail if a refactor uses insertAdjacentHTML/Range.createContextualFragment or otherwise injects nodes instead of preserving textContent.
14-21: 💤 Low valueMinor: scoped DOM reset is safer than wiping
document.head.
document.head.innerHTML = ""between every test nukes anything jsdom or other setup scripts may have added to<head>(e.g., RTL’s injected styles, jest-dom global setup). Removing only the generated<style id="appsmith-*-css-tokens">keeps the test hermetic without touching unrelated head children.♻️ Suggested scoped cleanup
beforeEach(() => { - document.head.innerHTML = ""; - document.body.innerHTML = ""; + document + .querySelectorAll('[id^="appsmith-"][id$="-css-tokens"]') + .forEach((el) => el.remove()); });🤖 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 `@app/client/src/widgets/__tests__/generateAppsmithCssVariables.test.js` around lines 14 - 21, Replace the global head wipe in the test setup with a scoped removal of the generated style token elements: instead of clearing document.head.innerHTML in the beforeEach, locate and remove any <style> elements whose id matches the generated pattern (e.g., ids starting with "appsmith-" and ending with "-css-tokens") so you only remove the test-injected CSS tokens while preserving other jsdom/jest/RTL head injections; keep the afterEach jest.restoreAllMocks() as-is.app/client/src/utils/autocomplete/__tests__/ternDocTooltip.test.tsx (1)
36-45: 💤 Low valueStrengthen the XSS assertion with a positive check on escaped text.
Asserting only the absence of
<img>passes even if the component renders nothing at all. A complementary positive assertion that the raw payload appears as escaped text content makes the regression test more durable against future refactors.♻️ Suggested tightening
const imgElements = container.querySelectorAll("img"); expect(imgElements).toHaveLength(0); + expect(container.querySelector("pre")).toHaveTextContent(xssPayload);🤖 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 `@app/client/src/utils/autocomplete/__tests__/ternDocTooltip.test.tsx` around lines 36 - 45, The test only asserts no <img> nodes exist but should also positively assert the XSS payload is rendered as escaped text; update the test for TernDocToolTip (in ternDocTooltip.test.tsx) that uses makeCompletion(xssPayload) to check container.textContent or a getByText query contains the literal string '<img src=x onerror="alert(1)">' (or otherwise assert innerHTML does not include an unescaped "<img" while textContent includes the payload) so the component is verified to render the payload as escaped text rather than silently render nothing.
🤖 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.
Nitpick comments:
In `@app/client/src/pages/Editor/GeneratePage/components/CrudInfoModal.tsx`:
- Around line 109-111: The Interweave output is wrapped in a <span>, causing
nested spans because InfoContentHeadingText is a styled <span>; update the
rendering in CrudInfoModal to avoid the extra wrapper by passing Interweave
props to render a fragment/no wrap (e.g., tagName="fragment" or noWrap depending
on your Interweave version) or replace InfoContentHeadingText with a block
element (e.g., a styled <div>) and render <Interweave content={successMessage}
/> directly; locate the usage in the component where InfoContentHeadingText
wraps Interweave and apply one of these changes to produce flat output.
In `@app/client/src/utils/autocomplete/__tests__/ternDocTooltip.test.tsx`:
- Around line 36-45: The test only asserts no <img> nodes exist but should also
positively assert the XSS payload is rendered as escaped text; update the test
for TernDocToolTip (in ternDocTooltip.test.tsx) that uses
makeCompletion(xssPayload) to check container.textContent or a getByText query
contains the literal string '<img src=x onerror="alert(1)">' (or otherwise
assert innerHTML does not include an unescaped "<img" while textContent includes
the payload) so the component is verified to render the payload as escaped text
rather than silently render nothing.
In `@app/client/src/widgets/__tests__/generateAppsmithCssVariables.test.js`:
- Around line 69-106: Add behavior-level XSS assertions to the existing tests
that spy on Element.prototype.innerHTML: after calling apply = fn(provider);
apply({...}) assert that the malicious payload remains as raw textContent on the
affected <style> node and that no <script> or <img> elements were actually
appended to the document (e.g. search
document.querySelectorAll("script")/("img") or inspect the created style
element's textContent for the payload). Keep these checks alongside the existing
styleSetterCalls assertions so the tests fail if a refactor uses
insertAdjacentHTML/Range.createContextualFragment or otherwise injects nodes
instead of preserving textContent.
- Around line 14-21: Replace the global head wipe in the test setup with a
scoped removal of the generated style token elements: instead of clearing
document.head.innerHTML in the beforeEach, locate and remove any <style>
elements whose id matches the generated pattern (e.g., ids starting with
"appsmith-" and ending with "-css-tokens") so you only remove the test-injected
CSS tokens while preserving other jsdom/jest/RTL head injections; keep the
afterEach jest.restoreAllMocks() as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c03a4ddb-8c6e-40a9-b3f3-5fdaf0895d46
📒 Files selected for processing (9)
app/client/src/components/BottomBar/ManualUpgrades.tsxapp/client/src/components/BottomBar/__tests__/ManualUpgrades.test.tsxapp/client/src/pages/Editor/GeneratePage/components/CrudInfoModal.tsxapp/client/src/pages/Editor/GeneratePage/components/__tests__/CrudInfoModal.test.tsxapp/client/src/utils/autocomplete/__tests__/ternDocTooltip.test.tsxapp/client/src/utils/autocomplete/ternDocTooltip.tsxapp/client/src/widgets/CustomWidget/component/customWidgetscript.jsapp/client/src/widgets/__tests__/generateAppsmithCssVariables.test.jsapp/client/src/widgets/wds/WDSCustomWidget/component/customWidgetscript.js
…alternatives Replace unsafe DOM manipulation patterns to prevent XSS: - Custom widget scripts: replace innerHTML with textContent on <style> elements to prevent </style> breakout attacks (GHSA-frqc-9x9h-fv9v) - ternDocTooltip: replace dangerouslySetInnerHTML with React text children since doc content is always plain text (GHSA-vqqg-rp36-9c62) - CrudInfoModal/ManualUpgrades: replace dangerouslySetInnerHTML with Interweave component for safe HTML rendering (GHSA-vqqg-rp36-9c62)
subrata71
force-pushed
the
fix/xss-innerhtml-dangerously-set-ghsa-frqc-vqqg
branch
from
25a37cb to
2b09a73
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Replace unsafe DOM manipulation patterns across 5 client-side files to close two security advisories:
GHSA-frqc-9x9h-fv9v — CSS token injection via
innerHTMLon<style>elements in custom widget scripts. Replaced withtextContent, which sets raw text without HTML parsing and prevents</style>breakout attacks.GHSA-vqqg-rp36-9c62 —
dangerouslySetInnerHTMLusage in 3 UI components. Fixed per content type:ternDocTooltip.tsx: Replaced with React text children{doc}(content is always plain text from Tern.js!docmetadata)CrudInfoModal.tsx: Replaced withInterweavecomponent (content contains intentional<b>tags from server)ManualUpgrades.tsx: Replaced withInterweavecomponent (content contains intentional<br>,<code>,<strong>tags)No new dependencies added —
Interweaveis already used inHTMLCellfor table widget HTML rendering.Files changed (production)
app/client/src/widgets/wds/WDSCustomWidget/component/customWidgetscript.js— 3 innerHTML → textContentapp/client/src/widgets/CustomWidget/component/customWidgetscript.js— 1 innerHTML → textContentapp/client/src/utils/autocomplete/ternDocTooltip.tsx— dangerouslySetInnerHTML → React text childrenapp/client/src/pages/Editor/GeneratePage/components/CrudInfoModal.tsx— dangerouslySetInnerHTML → Interweaveapp/client/src/components/BottomBar/ManualUpgrades.tsx— dangerouslySetInnerHTML → InterweaveFiles added (tests)
generateAppsmithCssVariables.test.js— tests both WDS and legacy custom widget CSS variable generation including XSS preventionternDocTooltip.test.tsx— tests autocomplete tooltip rendering and XSS preventionCrudInfoModal.test.tsx— tests InfoContent rendering and XSS preventionManualUpgrades.test.tsx— tests UpdatesModal rendering and XSS preventionTDD verification
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Communication
Should the DevRel and Marketing teams inform users about this change?
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/26435948997
Commit: 2b09a73
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 26 May 2026 10:16:47 UTC
Summary by CodeRabbit
Bug Fixes
Tests