Conversation
WalkthroughMigrates HTML sanitization in devportal and publisher webapps from a disallowed-tags Changes
Sequence DiagramsequenceDiagram
actor Client
participant Carousel
participant HTMLRender
participant Settings
participant DOMPurify
participant Parser
Client->>Carousel: render slide
Carousel->>HTMLRender: provide slide.content
HTMLRender->>Settings: read app.sanitizeHtmlDocs.enabled
alt sanitization enabled and sanitize=true
HTMLRender->>DOMPurify: sanitize(html, {ADD_TAGS, ADD_ATTR})
DOMPurify-->>HTMLRender: sanitized HTML
HTMLRender->>Parser: parse(sanitized HTML)
else sanitization disabled or sanitize=false
HTMLRender->>Parser: parse(raw HTML)
end
Parser-->>HTMLRender: parsed nodes
HTMLRender-->>Carousel: rendered content
Carousel-->>Client: display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx (1)
59-59: Preferincludes()for array membership check.Using
includes()is more idiomatic and readable thanfind()when checking for membership.♻️ Suggested change
- if (node.type === 'tag' && tagsNotAllowed.find(t => t === node.name)) { + if (node.type === 'tag' && tagsNotAllowed.includes(node.name)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx` at line 59, Replace the array membership check that uses tagsNotAllowed.find(...) with the more idiomatic tagsNotAllowed.includes(...) where the condition node.type === 'tag' && tagsNotAllowed.find(t => t === node.name) appears; locate this conditional (in the HTMLRender.jsx render/walk logic) and change it to use includes on node.name, ensuring tagsNotAllowed is an array of strings so includes works correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@portals/devportal/src/main/webapp/site/public/theme/settings.json`:
- Around line 27-31: The JSON file contains invalid JavaScript-style comments
inside the sanitizeHtmlDocs object (specifically on the additionalAllowedTags
and additionalAllowedAttributes lines); remove those inline comments so the
values remain valid JSON arrays (keep "sanitizeHtmlDocs",
"additionalAllowedTags", and "additionalAllowedAttributes" intact) or move the
examples into external documentation/README instead, ensuring the settings.json
contains only pure JSON.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx`:
- Around line 58-65: The replace callback customReplace currently returns the
raw dom node for unchanged elements which can confuse html-react-parser; update
customReplace (and keep tagsNotAllowed logic) so that when node.type !== 'tag'
or node.name is not in tagsNotAllowed it returns undefined (i.e., no return
value) rather than returning node, while still returning the React element for
disallowed tags (e.g., <div>{node.name} tags are not allowed</div>).
In `@portals/publisher/src/main/webapp/site/public/conf/settings.json`:
- Around line 36-40: The settings.json contains invalid inline comments inside
the "sanitizeHtmlDocs" object (specifically on the "additionalAllowedTags" and
"additionalAllowedAttributes" entries) which breaks JSON parsing; remove the
trailing comment tokens and text so the keys remain valid JSON arrays (or move
the example text into a separate README or a top-level "examples" string field)
and ensure "sanitizeHtmlDocs", "additionalAllowedTags", and
"additionalAllowedAttributes" are left as plain JSON values without comments.
In
`@portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx`:
- Around line 58-65: The customReplace callback (customReplace) is returning the
original node object which is incorrect for html-react-parser's replace: to
leave a node unchanged the function must return undefined (or nothing); update
customReplace so when node.type !== 'tag' or the tag is allowed it returns
undefined (not the node), and only return a React element (e.g., a message div)
for disallowed tags using tagsNotAllowed and node.name to detect them.
---
Nitpick comments:
In
`@portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx`:
- Line 59: Replace the array membership check that uses tagsNotAllowed.find(...)
with the more idiomatic tagsNotAllowed.includes(...) where the condition
node.type === 'tag' && tagsNotAllowed.find(t => t === node.name) appears; locate
this conditional (in the HTMLRender.jsx render/walk logic) and change it to use
includes on node.name, ensuring tagsNotAllowed is an array of strings so
includes works correctly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 180af823-1d07-4c04-8b02-ce6bdd8d450b
⛔ Files ignored due to path filters (2)
portals/devportal/src/main/webapp/package-lock.jsonis excluded by!**/package-lock.jsonportals/publisher/src/main/webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
portals/devportal/src/main/webapp/package.jsonportals/devportal/src/main/webapp/site/public/theme/settings.jsonportals/devportal/src/main/webapp/source/src/app/components/Base/index.jsxportals/devportal/src/main/webapp/source/src/app/components/LandingPage/Carousel.jsxportals/devportal/src/main/webapp/source/src/app/components/LandingPage/Contact.jsxportals/devportal/src/main/webapp/source/src/app/components/LandingPage/ParallaxScroll.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsxportals/publisher/src/main/webapp/package.jsonportals/publisher/src/main/webapp/site/public/conf/settings.jsonportals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx
portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx
Outdated
Show resolved
Hide resolved
portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
portals/publisher/src/main/webapp/site/public/conf/settings.json (1)
38-39:⚠️ Potential issue | 🔴 CriticalRemove inline comments from JSON to restore valid parsing.
settings.jsonis invalid because of the// Example: ...tokens on Line 38 and Line 39, which will break config parsing/loading.🐛 Proposed fix
"sanitizeHtmlDocs": { "enabled": true, - "additionalAllowedTags": [], // Example: ["iframe", "svg"] - "additionalAllowedAttributes": [] // Example: ["onclick"] + "additionalAllowedTags": [], + "additionalAllowedAttributes": [] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/publisher/src/main/webapp/site/public/conf/settings.json` around lines 38 - 39, The JSON file contains inline JavaScript-style comments after the keys "additionalAllowedTags" and "additionalAllowedAttributes" which invalidates parsing; remove the trailing comment tokens (the "// Example: [...]" text) so the entries are pure JSON arrays (e.g., "additionalAllowedTags": [] and "additionalAllowedAttributes": []), or move those examples to external docs or a separate non-JSON metadata key if examples must be retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@portals/devportal/src/main/webapp/source/src/app/components/Base/index.jsx`:
- Around line 757-761: The footer rendering currently passes sanitize={false} to
HTMLRender even when dangerMode is false, which makes the "safe" path as
permissive as the dangerous one; update the conditional in the Base component so
that when dangerMode is false you either omit the sanitize prop or explicitly
pass sanitize={true} to HTMLRender (keep the dangerMode branch using
dangerouslySetInnerHTML only), or remove the dangerMode toggle entirely if you
intend both branches to be equally permissive; reference the footerHTML and
dangerMode variables and the HTMLRender component prop sanitize in your change.
---
Duplicate comments:
In `@portals/publisher/src/main/webapp/site/public/conf/settings.json`:
- Around line 38-39: The JSON file contains inline JavaScript-style comments
after the keys "additionalAllowedTags" and "additionalAllowedAttributes" which
invalidates parsing; remove the trailing comment tokens (the "// Example: [...]"
text) so the entries are pure JSON arrays (e.g., "additionalAllowedTags": [] and
"additionalAllowedAttributes": []), or move those examples to external docs or a
separate non-JSON metadata key if examples must be retained.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d36335bb-b859-4ffc-9e9c-415c80121e65
⛔ Files ignored due to path filters (2)
portals/devportal/src/main/webapp/package-lock.jsonis excluded by!**/package-lock.jsonportals/publisher/src/main/webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
portals/devportal/src/main/webapp/package.jsonportals/devportal/src/main/webapp/site/public/theme/settings.jsonportals/devportal/src/main/webapp/source/src/app/components/Base/index.jsxportals/devportal/src/main/webapp/source/src/app/components/LandingPage/Carousel.jsxportals/devportal/src/main/webapp/source/src/app/components/LandingPage/Contact.jsxportals/devportal/src/main/webapp/source/src/app/components/LandingPage/ParallaxScroll.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsxportals/publisher/src/main/webapp/package.jsonportals/publisher/src/main/webapp/site/public/conf/settings.jsonportals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx
✅ Files skipped from review due to trivial changes (2)
- portals/publisher/src/main/webapp/package.json
- portals/devportal/src/main/webapp/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- portals/devportal/src/main/webapp/source/src/app/components/LandingPage/Contact.jsx
- portals/devportal/src/main/webapp/source/src/app/components/LandingPage/Carousel.jsx
- portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx
- portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx
portals/devportal/src/main/webapp/source/src/app/components/Base/index.jsx
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx (1)
27-35: Keep the unsafe path out of the shared renderer.Because this sits under
Shared/,sanitize={false}makes the safety boundary very easy to bypass from future call sites. If there are genuinely trusted HTML sources, I’d keepHTMLRenderalways sanitized and expose a separateUnsafeHTMLRender(or similarly explicit API) for those narrow cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx` around lines 27 - 35, Change HTMLRender so it always sanitizes and remove the opt-out prop: stop reading or honoring the sanitize prop in the HTMLRender function and ensure it always runs the sanitization path before rendering; then add a clearly named separate component (e.g., UnsafeHTMLRender or RawHTMLRender) for callers that truly need to bypass sanitization so the shared component under Shared/ cannot be accidentally used unsafely. Update exports/usage so existing call sites that relied on sanitize={false} either switch to the new UnsafeHTMLRender or are updated to accept the always-sanitized HTMLRender.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx`:
- Around line 37-52: Change the sanitization gate so missing theme keys default
to enabled: instead of checking sanitizeConfig?.enabled === true, treat
undefined as true. For example, derive a defaulted flag from
Settings.app?.sanitizeHtmlDocs (e.g. const enabled = sanitizeConfig?.enabled ??
true) and then set isSanitizationEnabled using that flag and the incoming
sanitize prop (e.g. isSanitizationEnabled = enabled && sanitize === true).
Update the logic around sanitizeConfig, isSanitizationEnabled, and the DOMPurify
branch (the code that computes parsedHtml and calls DOMPurify.sanitize)
accordingly.
In
`@portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx`:
- Around line 50-53: The branch that calls parse(html) in the HTMLRender
component can throw when html is null/undefined; update the else branch (where
parsedHtml = parse(html)) to defensively handle empty input—either coerce html
to an empty string before calling parse (e.g., use html || ''), or skip parsing
and set parsedHtml to an empty string/null and return early from the component;
refer to the parsedHtml assignment and parse(html) call in HTMLRender.jsx to
locate and change the code.
---
Nitpick comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx`:
- Around line 27-35: Change HTMLRender so it always sanitizes and remove the
opt-out prop: stop reading or honoring the sanitize prop in the HTMLRender
function and ensure it always runs the sanitization path before rendering; then
add a clearly named separate component (e.g., UnsafeHTMLRender or RawHTMLRender)
for callers that truly need to bypass sanitization so the shared component under
Shared/ cannot be accidentally used unsafely. Update exports/usage so existing
call sites that relied on sanitize={false} either switch to the new
UnsafeHTMLRender or are updated to accept the always-sanitized HTMLRender.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2b622d3-549e-44d1-9f2d-298458fc60e0
📒 Files selected for processing (5)
portals/devportal/src/main/webapp/site/public/theme/settings.jsonportals/devportal/src/main/webapp/source/src/app/components/LandingPage/Carousel.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsxportals/publisher/src/main/webapp/site/public/conf/settings.jsonportals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx
✅ Files skipped from review due to trivial changes (1)
- portals/devportal/src/main/webapp/source/src/app/components/LandingPage/Carousel.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- portals/devportal/src/main/webapp/site/public/theme/settings.json
- portals/publisher/src/main/webapp/site/public/conf/settings.json
portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx
Outdated
Show resolved
Hide resolved
portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx (2)
36-53: Good security-first approach with DOMPurify.The refactoring to use DOMPurify with an allowlist approach is a solid security improvement over the previous tag-blocking mechanism. The default-enabled logic (
enabled !== false) correctly ensures sanitization is active even when the config key is missing.Minor consistency suggestion: the sanitization-enabled path (line 48) doesn't guard against falsy
html, while the disabled path (line 52) does. Both paths work correctly (DOMPurify handles null/undefined gracefully), but aligning them would improve clarity.✨ Optional: Align falsy handling for consistency
if (isSanitizationEnabled) { // Use DOMPurify for sanitization when enabled const sanitizationOptions = { ADD_TAGS: sanitizeConfig.additionalAllowedTags || [], ADD_ATTR: sanitizeConfig.additionalAllowedAttributes || [], }; - const sanitizedHtml = DOMPurify.sanitize(html, sanitizationOptions); - parsedHtml = parse(sanitizedHtml); + parsedHtml = html ? parse(DOMPurify.sanitize(html, sanitizationOptions)) : null; } else { // Render without sanitization parsedHtml = html ? parse(html) : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx` around lines 36 - 53, The sanitization-enabled branch doesn't guard against falsy html like the disabled branch does; update the logic around DOMPurify.sanitize and parse so both branches handle null/undefined html consistently: when isSanitizationEnabled is true, check html before calling DOMPurify.sanitize and parse (use the same html ? parse(...) : null pattern used in the else branch) referencing Settings, sanitizeConfig, isSanitizationEnabled, DOMPurify.sanitize, parsedHtml, and parse to locate and modify the code.
1-3: Consider removing stale eslint-disable comments.The
no-unused-varsandconsistent-returndisables at the top of the file may no longer be necessary after this refactoring. The new code has a single consistent return path and no apparent unused variables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx` around lines 1 - 3, Remove the now-stale ESLint disables at the top of HTMLRender.jsx: delete the "eslint-disable no-unused-vars" and "eslint-disable consistent-return" pragmas (keep "eslint-disable react/prop-types" only if still required). Confirm the file-level disables are removed from the header comment block and run lint to ensure no violations remain; if any legitimate rule violations appear, fix the code (or add narrowly scoped disables adjacent to the offending lines) rather than re-enabling broad file-level rules.portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx (1)
44-47: Prefer DOMPurify's HTML-only profile here.Unless this component is expected to render SVG/MathML, this option set is broader than it needs to be. DOMPurify's docs say the default profile permits HTML, SVG, and MathML, and show
USE_PROFILES: { html: true }for HTML-only use cases. (github.com)♻️ Proposed change
const sanitizationOptions = { + USE_PROFILES: { html: true }, ADD_TAGS: sanitizeConfig.additionalAllowedTags || [], ADD_ATTR: sanitizeConfig.additionalAllowedAttributes || [], };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx` around lines 44 - 47, The sanitization options currently built in the HTMLRender component (sanitizationOptions using ADD_TAGS and ADD_ATTR from sanitizeConfig) use the default DOMPurify profile which allows HTML, SVG, and MathML; update sanitizationOptions to include DOMPurify's HTML-only profile by adding USE_PROFILES: { html: true } alongside the existing ADD_TAGS/ADD_ATTR values so the sanitizer restricts to HTML only (refer to the sanitizationOptions variable and sanitizeConfig.additionalAllowedTags/additionalAllowedAttributes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx`:
- Around line 38-52: The unsanitized fallback (parsedHtml = html ? parse(html) :
null) must be removed so untrusted HTML is always sanitized; modify the
HTMLRender component to always run DOMPurify.sanitize before calling parse (use
DOMPurify.sanitize(html, sanitizationOptions) and assign parsedHtml =
parse(sanitizedHtml)), remove or deprecate the sanitize prop and ensure
sanitizeConfig.enabled cannot disable sanitization (or default it to true), and
eliminate any code path that directly calls parse(html) without sanitization
(references: HTMLRender component, sanitize prop, sanitizeConfig, parsedHtml,
parse, and DOMPurify.sanitize).
---
Nitpick comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx`:
- Around line 36-53: The sanitization-enabled branch doesn't guard against falsy
html like the disabled branch does; update the logic around DOMPurify.sanitize
and parse so both branches handle null/undefined html consistently: when
isSanitizationEnabled is true, check html before calling DOMPurify.sanitize and
parse (use the same html ? parse(...) : null pattern used in the else branch)
referencing Settings, sanitizeConfig, isSanitizationEnabled, DOMPurify.sanitize,
parsedHtml, and parse to locate and modify the code.
- Around line 1-3: Remove the now-stale ESLint disables at the top of
HTMLRender.jsx: delete the "eslint-disable no-unused-vars" and "eslint-disable
consistent-return" pragmas (keep "eslint-disable react/prop-types" only if still
required). Confirm the file-level disables are removed from the header comment
block and run lint to ensure no violations remain; if any legitimate rule
violations appear, fix the code (or add narrowly scoped disables adjacent to the
offending lines) rather than re-enabling broad file-level rules.
In
`@portals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx`:
- Around line 44-47: The sanitization options currently built in the HTMLRender
component (sanitizationOptions using ADD_TAGS and ADD_ATTR from sanitizeConfig)
use the default DOMPurify profile which allows HTML, SVG, and MathML; update
sanitizationOptions to include DOMPurify's HTML-only profile by adding
USE_PROFILES: { html: true } alongside the existing ADD_TAGS/ADD_ATTR values so
the sanitizer restricts to HTML only (refer to the sanitizationOptions variable
and sanitizeConfig.additionalAllowedTags/additionalAllowedAttributes).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f345df40-80cd-4139-b2a4-559c9d719949
📒 Files selected for processing (2)
portals/devportal/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsxportals/publisher/src/main/webapp/source/src/app/components/Shared/HTMLRender.jsx



Purpose
Improve the component
Approach
Upgraded dompurify version
Improved the component to be customizable
Updated the config
Samples
N/A
Summary by CodeRabbit
Chores
Improvements
Bug Fixes / UX