Skip to content

Fix XSS in Brand: sanitize dangerouslySetInnerHTML with DOMPurify#34712

Open
tomohiro86 wants to merge 2 commits intostorybookjs:nextfrom
tomohiro86:fix/xss-brand-dangerouslysetinnerhtml
Open

Fix XSS in Brand: sanitize dangerouslySetInnerHTML with DOMPurify#34712
tomohiro86 wants to merge 2 commits intostorybookjs:nextfrom
tomohiro86:fix/xss-brand-dangerouslysetinnerhtml

Conversation

@tomohiro86
Copy link
Copy Markdown

@tomohiro86 tomohiro86 commented May 5, 2026

Summary

Fixes #34690

When theme.brand.image is null, Brand.tsx renders theme.brand.title via dangerouslySetInnerHTML to allow custom HTML markup. A crafted theme (e.g. via a compromised addon) could use this to execute arbitrary scripts in the Storybook Manager.

  • Add dompurify as a dependency
  • Sanitize title with DOMPurify.sanitize() before passing to dangerouslySetInnerHTML — strips <script> tags and event handlers while preserving safe formatting HTML
  • Add Brand.test.tsx covering XSS payloads and safe HTML pass-through

Test plan

  • yarn vitest run code/core/src/manager/components/sidebar/__tests__/Brand.test.tsx passes
  • Manager UI renders custom HTML brand title correctly (safe tags preserved)
  • Script tags and event handlers in brand title are stripped

Summary by CodeRabbit

  • Security

    • Improved security by sanitizing HTML in brand titles to prevent script injection and strip malicious attributes, while preserving safe inline formatting.
  • Tests

    • Added test coverage for brand title sanitization, covering script removal, event-handler stripping, safe HTML preservation, and linked-title scenarios.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 5, 2026

📝 Walkthrough

Walkthrough

Adds DOMPurify and uses it to sanitize the Brand component's title HTML when no image is present; package.json updated to include dompurify typings; tests added to verify script/event stripping and preservation of safe HTML, including when the title is wrapped in a link.

Changes

XSS Sanitization for Brand Component

Layer / File(s) Summary
Dependency Setup
code/core/package.json
Added dompurify (^3.4.2) and @types/dompurify (^3.2.0) to dependencies.
Core Implementation
code/core/src/manager/components/sidebar/Brand.tsx
Imported DOMPurify and assigns sanitizedTitle = DOMPurify.sanitize(title); uses sanitizedTitle with dangerouslySetInnerHTML in both the no-URL and link-wrapped rendering paths when theme.brand.image === null.
Tests / Verification
code/core/src/manager/components/sidebar/__tests__/Brand.test.tsx
Added Vitest/RTL tests and renderBrand helper; four tests confirm XSS sanitization: script tag removal, inline event handler removal, safe HTML preserved, and same behavior when a url is provided.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@code/core/src/manager/components/sidebar/Brand.tsx`:
- Around line 55-59: The external brand link rendered by LogoLink uses
target={targetValue} and can open user-configured URLs in a new tab; update the
LogoLink props to include rel="noopener noreferrer" whenever targetValue is
"_blank" (or add rel unconditionally) to prevent opener access and noreferrer
leakage. Locate the LogoLink element in Brand.tsx (the element using href={url},
target={targetValue}, dangerouslySetInnerHTML={{ __html: sanitizedTitle }}) and
add the rel attribute accordingly so external links opened with _blank are
hardened.
🪄 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: ebbbb806-6051-4ef9-99e5-2fca337cebf7

📥 Commits

Reviewing files that changed from the base of the PR and between fdcea98 and be267ec.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (3)
  • code/core/package.json
  • code/core/src/manager/components/sidebar/Brand.tsx
  • code/core/src/manager/components/sidebar/__tests__/Brand.test.tsx

Comment thread code/core/src/manager/components/sidebar/Brand.tsx
Prevents opener access and referrer leakage for external brand links.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/Brand.tsx (1)

49-49: 💤 Low value

Consider narrowing the DOMPurify allowlist for brand titles.

The default DOMPurify config is secure, but it allows a broad set of elements (e.g., <a>, <img>, <table>). Since brand titles are expected to contain only inline formatting, an explicit ALLOWED_TAGS list scopes the permitted surface more tightly and makes the security contract self-documenting.

♻️ Proposed stricter allowlist
-    const sanitizedTitle = DOMPurify.sanitize(title);
+    const sanitizedTitle = DOMPurify.sanitize(title, {
+      ALLOWED_TAGS: ['b', 'strong', 'i', 'em', 'u', 'span', 'br', 'small'],
+      ALLOWED_ATTR: ['class', 'style'],
+    });
🤖 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 `@code/core/src/manager/components/sidebar/Brand.tsx` at line 49, The brand
title currently uses DOMPurify.sanitize(title) with the default permissive tag
set; tighten this by calling DOMPurify.sanitize with an explicit ALLOWED_TAGS
array (e.g., only inline formatting like "b", "i", "em", "strong", "u", "span",
"br", "code") and minimal ALLOWED_ATTR if needed, then assign the result to
sanitizedTitle; update the sanitize call in Brand.tsx (reference sanitizedTitle
and DOMPurify.sanitize) so only the intended inline tags are permitted.
🤖 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 `@code/core/src/manager/components/sidebar/Brand.tsx`:
- Line 49: The brand title currently uses DOMPurify.sanitize(title) with the
default permissive tag set; tighten this by calling DOMPurify.sanitize with an
explicit ALLOWED_TAGS array (e.g., only inline formatting like "b", "i", "em",
"strong", "u", "span", "br", "code") and minimal ALLOWED_ATTR if needed, then
assign the result to sanitizedTitle; update the sanitize call in Brand.tsx
(reference sanitizedTitle and DOMPurify.sanitize) so only the intended inline
tags are permitted.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b471356e-00c6-4de8-827f-80d20dac9615

📥 Commits

Reviewing files that changed from the base of the PR and between be267ec and ff7793a.

📒 Files selected for processing (1)
  • code/core/src/manager/components/sidebar/Brand.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: dangerouslySetInnerHTML with unsanitized theme.brand.title allows XSS via theme configuration

1 participant