fix: sanitize HTML before dangerouslySetInnerHTML#41545
Open
yashwant938 wants to merge 3 commits intoappsmithorg:releasefrom
Open
fix: sanitize HTML before dangerouslySetInnerHTML#41545yashwant938 wants to merge 3 commits intoappsmithorg:releasefrom
yashwant938 wants to merge 3 commits intoappsmithorg:releasefrom
Conversation
Harden client-side rendering paths that use dangerouslySetInnerHTML by sanitizing HTML with a small allowlist sanitizer.\n\n- Add utils/sanitizeHtml helper (removes scripts, event handlers, unsafe URLs; enforces rel=noopener noreferrer for target=_blank).\n- Apply sanitization to release notes, upgrade carousel details, tern tooltip docs, manual upgrades, and CRUD info modal.\n- Add unit tests for sanitizer behavior.\n\nThis reduces DOM-XSS risk if upstream HTML or interpolated content is compromised.
Contributor
WalkthroughAdds a dependency-free client-side HTML sanitizer and integrates it into several UI components to sanitize dynamic HTML (descriptions, tooltips, messages) before rendering, plus unit tests and a minor constant presentation tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as UI Component
participant Memo as useMemo
participant Sanitizer as sanitizeHtml
participant DOM as Browser DOM
Component->>Memo: provide dynamicHtml
Memo->>Sanitizer: call sanitizeHtml(dynamicHtml)
Sanitizer-->>Memo: return safeHtml
Memo-->>Component: memoized safeHtml
Component->>DOM: render via dangerouslySetInnerHTML(safeHtml)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/client/src/utils/autocomplete/ternDocTooltip.tsx (1)
25-29:⚠️ Potential issue | 🔴 CriticalRules of Hooks violation:
useMemocalled after early return.
useMemoon line 29 is below the early return on line 25. React requires hooks to be called unconditionally and in the same order every render. This will cause runtime warnings and potentially broken state.Move the
useMemoabove the early return.Proposed fix
if (!doc || !displayText) return null; const examples = displayText in ternDocsInfo ? ternDocsInfo[displayText].exampleArgs : null; - const safeDoc = useMemo(() => sanitizeHtml(doc), [doc]);Move
useMemobefore the guard:+ const safeDoc = useMemo(() => sanitizeHtml(doc ?? ""), [doc]); + if (!doc || !displayText) return null; const examples = displayText in ternDocsInfo ? ternDocsInfo[displayText].exampleArgs : null;
🤖 Fix all issues with AI agents
In `@app/client/src/components/BottomBar/ManualUpgrades.tsx`:
- Around line 160-171: The CLEAN_URL_UPDATE description uses inline style on a
code tag which is being stripped by sanitizeHtml, so either allow the style
attribute for code tags in sanitizeHtml or move those inline styles into the
existing StyledList CSS; specifically update sanitizeHtml (function
sanitizeHtml) to permit the style attribute on the code element OR remove the
inline style from the CLEAN_URL_UPDATE template in messages
(CLEAN_URL_UPDATE.description) and add equivalent styling in the StyledList
component's CSS rules that target code elements so the visual appearance is
preserved when createMessage(...) output is sanitized.
In `@app/client/src/utils/sanitizeHtml.test.ts`:
- Around line 4-10: sanitizeHtml test fails because sanitizeElement currently
unwraps <script> children instead of removing them, so their text (e.g.,
"alert(1)") remains; update sanitizeElement to fully remove SCRIPT elements and
their content rather than returning their child nodes. Specifically, inside
sanitizeElement (and any branch that handles tagName or node.nodeName), detect
node.tagName === "SCRIPT" (or node.nodeName === "SCRIPT") and return null (or
remove the node) so its children are not processed/concatenated into the parent;
ensure sanitizeHtml uses that behavior so
sanitizeHtml("<p>ok<script>...</script></p>") yields "<p>ok</p>" and the test
assertion expecting no "alert(1)" passes.
- Around line 1-33: Add comprehensive unit tests for the sanitizeHtml function
to cover the missing security and edge cases: write tests that assert removal of
<style>, <iframe>, <object>, <embed>, and nested disallowed elements (e.g.,
<div><script>...</script></div>), ensure SVG vectors with event attributes
(e.g., <svg onload=...>) are sanitized, verify data: URI schemes are stripped or
rejected for <img src> and <a href>, confirm HTML comments are removed, add
tests for empty/null/non-string inputs (e.g., undefined, null, empty string) to
ensure graceful handling, assert that style attributes are removed while allowed
tags/attributes remain intact (positive cases), and include assertions that
rel="noopener noreferrer" is added for target="_blank"; use the existing
sanitizeHtml test file and the same describe/it structure targeting the
sanitizeHtml function.
In `@app/client/src/utils/sanitizeHtml.ts`:
- Around line 1-185: The sanitizer (sanitizeHtml, sanitizeNode, sanitizeElement)
is brittle and misses mXSS/namespace/template edge cases; replace the custom
sanitizer with a proven library: install and integrate DOMPurify, use
DOMPurify.sanitize(dirtyHtml, {ALLOWED_TAGS: Array.from(DEFAULT_ALLOWED_TAGS),
ALLOWED_ATTR: /* map from DEFAULT_ALLOWED_ATTRS_BY_TAG */}) for client-side HTML
rendering, and remove or deprecate the hand-rolled logic
(sanitizeNode/sanitizeElement/escapeHtml/isSafeUrl/ensureSafeRel) so that
DOMPurify handles SVG/MathML/ template/ noscript contexts and URL/rel
sanitization consistently.
- Around line 89-102: The sanitizeElement function currently unwraps children of
any disallowed tag which re-parents script/style/etc. text into the DOM; change
the logic so that for a dedicated set of full-remove tags (e.g., "script",
"style", "iframe", "object", "embed", "noscript") you remove the element and all
its children outright instead of moving children into the parent, while keeping
the existing "unwrap children" behavior for other disallowed tags; update
sanitizeElement to check tag membership in this FULL_REMOVE_TAGS set and call
parent.removeChild(el) immediately for those tags (ensuring parent exists) to
prevent visible or retained malicious content.
🧹 Nitpick comments (2)
app/client/src/utils/sanitizeHtml.ts (2)
59-66:escapeHtmldoesn't escape backticks.Minor, but if the escaped output ever lands inside a JS template literal context, unescaped backticks could be an issue. Standard practice is to also escape
`→`. Low risk given current usage but worth noting.
105-105: Allocating a new emptySeton every attribute iteration for unrecognized tags.Minor perf nit — hoist a shared empty set to avoid per-element allocation.
Proposed fix
+const EMPTY_SET = new Set<string>(); + function sanitizeElement(el: Element) { ... - const allowedAttrs = DEFAULT_ALLOWED_ATTRS_BY_TAG[tag] ?? new Set<string>(); + const allowedAttrs = DEFAULT_ALLOWED_ATTRS_BY_TAG[tag] ?? EMPTY_SET;
Remove inline style attributes from CLEAN_URL_UPDATE message templates since HTML is now sanitized before rendering.\n\n- Render <code> without style attributes in message constants.\n- Move the intended wrapping/line-height to StyledList CSS for consistent styling.
- Drop script/style/iframe/object/embed/noscript entirely (do not unwrap children).\n- Disallow data: URIs in <a href> while still allowing safe data:image in <img src>.\n- Add broad test coverage for common XSS vectors and edge cases (comments, SVG/Math, style stripping, non-string inputs).
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.
Harden client-side rendering paths that use dangerouslySetInnerHTML by sanitizing HTML with a small allowlist sanitizer.\n\n- Add utils/sanitizeHtml helper (removes scripts, event handlers, unsafe URLs; enforces rel=noopener noreferrer for target=_blank).\n- Apply sanitization to release notes, upgrade carousel details, tern tooltip docs, manual upgrades, and CRUD info modal.\n- Add unit tests for sanitizer behavior.\n\nThis reduces DOM-XSS risk if upstream HTML or interpolated content is compromised.
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests