Skip to content

fix(common): replace .toString() with String() on DOMPurify.sanitize() calls#7439

Draft
ashishjain0512 wants to merge 1 commit intodevelopfrom
bug/6113_fix-tostring-eslint-error
Draft

fix(common): replace .toString() with String() on DOMPurify.sanitize() calls#7439
ashishjain0512 wants to merge 1 commit intodevelopfrom
bug/6113_fix-tostring-eslint-error

Conversation

@ashishjain0512
Copy link
Collaborator

Summary

Resolves #6113

  • Replace .toString() with String() wrapper on DOMPurify.sanitize() calls in common.ts to avoid @typescript-eslint/no-base-to-string lint errors
  • DOMPurify.sanitize() can return DocumentFragment (when config includes RETURN_DOM_FRAGMENT), which lacks a meaningful .toString()String() is the safe alternative

Test plan

  • Existing unit tests pass (22/22 in common.spec.ts)
  • ESLint passes on common.ts
  • Pre-commit hooks pass

🤖 Generated with Claude Code

…) calls

Resolves #6113

DOMPurify.sanitize() can return DocumentFragment when config includes
RETURN_DOM_FRAGMENT, which lacks a meaningful .toString(). Using String()
wrapper avoids the @typescript-eslint/no-base-to-string lint rule.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@netlify
Copy link

netlify bot commented Mar 3, 2026

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit c62c074
🔍 Latest deploy log https://app.netlify.com/projects/mermaid-js/deploys/69a6adc6bd48550008490a2d
😎 Deploy Preview https://deploy-preview-7439--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@changeset-bot
Copy link

changeset-bot bot commented Mar 3, 2026

🦋 Changeset detected

Latest commit: c62c074

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mermaid Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 3, 2026

Open in StackBlitz

@mermaid-js/examples

npm i https://pkg.pr.new/@mermaid-js/examples@7439

mermaid

npm i https://pkg.pr.new/mermaid@7439

@mermaid-js/layout-elk

npm i https://pkg.pr.new/@mermaid-js/layout-elk@7439

@mermaid-js/layout-tidy-tree

npm i https://pkg.pr.new/@mermaid-js/layout-tidy-tree@7439

@mermaid-js/mermaid-zenuml

npm i https://pkg.pr.new/@mermaid-js/mermaid-zenuml@7439

@mermaid-js/parser

npm i https://pkg.pr.new/@mermaid-js/parser@7439

@mermaid-js/tiny

npm i https://pkg.pr.new/@mermaid-js/tiny@7439

commit: fd301b2

@codecov
Copy link

codecov bot commented Mar 3, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 3.54%. Comparing base (84e7a3a) to head (c62c074).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
packages/mermaid/src/diagrams/common/common.ts 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #7439   +/-   ##
=======================================
  Coverage     3.53%   3.54%           
=======================================
  Files          491     490    -1     
  Lines        48972   48965    -7     
  Branches       766     766           
=======================================
+ Hits          1733    1734    +1     
+ Misses       47239   47231    -8     
Flag Coverage Δ
unit 3.54% <80.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/mermaid/src/diagrams/common/common.ts 32.18% <80.00%> (+0.29%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@argos-ci
Copy link

argos-ci bot commented Mar 3, 2026

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ✅ No changes detected - Mar 3, 2026, 10:27 AM

Copy link
Collaborator

@knsv knsv left a comment

Choose a reason for hiding this comment

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

[sisyphus-bot]

Thanks for picking up issue #6113, @ashishjain0512 — this lint error has been tripping up contributors for a while, so it's great to see it tackled. The approach is sound and the changeset is properly scoped.


What's working well

🎉 Correct fix for the lint rule. String() is exactly the right idiom here. @typescript-eslint/no-base-to-string fires because TypeScript knows DOMPurify.sanitize() can return string | DocumentFragment | Node depending on the config shape, and .toString() on those non-string types would silently produce "[object DocumentFragment]". Using String() satisfies the rule because it's an explicit global conversion rather than a method lookup on the object.

🎉 Minimal and focused. Two call sites changed, changeset included, PR description clearly explains the why. This is a well-scoped fix.


Things to consider before marking ready

💡 [suggestion] — Consistency with removeScript() (common.ts:62)

export const removeScript = (txt: string): string => {
  setupDompurifyHooksIfNotSetup();
  const sanitizedText = DOMPurify.sanitize(txt);
  return sanitizedText;
};

removeScript also calls DOMPurify.sanitize() and returns the result directly. The no-arg overload of DOMPurify does narrow the return type to string, so the lint rule probably does not fire here — but worth a quick check: run pnpm lint with the fix in place and confirm no residual warnings on this function. If it's clean, no action needed.

🟢 [nit] — Guard against misuse of RETURN_DOM_FRAGMENT in public config

If a user ever passes { dompurifyConfig: { RETURN_DOM_FRAGMENT: true } } in their Mermaid config, String(fragment) will silently produce "[object DocumentFragment]" rather than sanitized HTML — same behavior as the old .toString(), so no regression introduced here. But it's a footgun in the public API surface. A small JSDoc note on sanitizeText (or on the dompurifyConfig config schema key) warning that RETURN_DOM and RETURN_DOM_FRAGMENT must not be set would help future contributors. This is out of scope for this PR — just flagging it as a follow-up idea.


Security

No XSS or injection issues introduced. Sanitization happens in DOMPurify.sanitize() before the string conversion, and String() vs .toString() makes no difference to the sanitization result. Reviewed sanitizeText, sanitizeMore, and the DOMPurify hook setup — all intact.


Good work — this is on the right track. Once you've confirmed no residual lint warnings, this should be straightforward to land. Let's get it across the finish line!

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug / Error Something isn't working or is incorrect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix eslint error @typescript-eslint/no-base-to-string causing test failures

2 participants