Add `suppressErrorRendering` option to avoid inserting 'Syntax error' message to DOM directly #4359

rhysd · 2023-04-29T15:45:52Z

📑 Summary

This PR adds new boolean option suppressErrorRendering. When it is set to true, mermaid won't insert 'Syntax error' message to DOM directly so that the user can take full control on handling the error.

Resolves #4358
Resolves #3205

📏 Design Decisions

I think there are two options to realize this feature:

Adding new option
Adding new argument to mermaid.render

I chose the first one because (1) it should affect all APIs such as mermaid.run and (2) adding new argument to the function should be avoided since it makes the API more complicated.

📋 Tasks

Make sure you

📖 have read the contribution guidelines
💻 have added unit/e2e tests (if appropriate) (Note: I couldn't find an error test case for mermaid.render. Please let me know if I'm missing something)
📓 have added documentation (if appropriate)
🔖 targeted develop branch

… message in diagram

packages/mermaid/src/mermaidAPI.ts

…ering` is set

* develop: (255 commits) chore(deps): update all minor dependencies chore: Run codecov based on E2E test status change REAMDME.md coverage from coveralls into codecov Add codecov.yaml Upload E2E set normal mode for vitest coverage Fix path name Add codecov to unit tests Add codecov to E2E chore: Add coverage scripts chore: add excludes chore: update deps Merge coverages Add coverage paths Rebuild chore: update pnpm Add coverage for E2E tests rename plugin variable into info in infoDetector.ts remove cypress/platform/index.html update pnpm-lock.yaml ...

codecov-commenter · 2023-06-18T10:45:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.79%. Comparing base (be1270d) to head (ecfa149).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4359   +/-   ##
========================================
  Coverage    44.79%   44.79%           
========================================
  Files           25       25           
  Lines         5353     5353           
  Branches        27       27           
========================================
  Hits          2398     2398           
  Misses        2954     2954           
  Partials         1        1

Flag	Coverage Δ
unit	`44.79% <ø> (ø)`

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

* develop: (595 commits) chore: Fix unit tests chore(deps): update all patch dependencies chore: Update docs Update docs New Mermaid Live Editor for Confluence Cloud (mermaid-js#4814) Update link to Discourse theme component (mermaid-js#4811) Update flowchart.md (mermaid-js#4810) chore: remove unneeded `CommomDB` chore: Update docs "CSS" instead of "css" in flowchart.md (mermaid-js#4797) Update CONTRIBUTING.md Update CONTRIBUTING.md fix: typos (mermaid-js#4801) chore: Align with convention fix: Add support for `~test Array~string~` chore: Add JSDoc to apply in sequenceDB refactor: Tidy up direction handling chore: Fix flowchart arrow chore: Add test to verify activate chore: Update tests snapshot ...

netlify · 2023-09-06T07:30:04Z

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	`ecfa149`
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/65fe9014875ea60008b175e1
😎 Deploy Preview	https://deploy-preview-4359--mermaid-js.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

sidharthv96 · 2023-09-06T07:48:19Z

This is the current scenario

This is what the user will see with the option set.

sidharthv96 · 2023-09-06T07:50:28Z

@rhysd an empty SVG is still being inserted into the page.
Is that the desired behaviour?

rhysd · 2023-09-06T11:51:45Z

Is that the desired behaviour?

Oh, that's not the behaviour I expected.

sidharthv96 · 2023-09-14T17:51:09Z

Fixed

* develop: (37 commits) fix: Arrow markers in flowchart-elk Bump version chore: Fix type in 'getLineFunctionsWithOffset' Update cypress/platform/marker_unique_id.html refactor: Add getLineFunctionsWithOffset function refactor: Move EdgeData to types fix: PointStart marker refX Added cypress test chore(deps): update all patch dependencies refactor: Fix typings in utils.ts Give markers unique id's per graph chore: Add @internal to createCSSStyles chore: Bump version refactor: Remove unused variables fix: mermaid-js#4818 support `getClasses` in external diagrams. Remove unnecessary tests Remove optional chaining chore: Update docs refactor: Use `||` instead of `??` Update flowchart.md (mermaid-js#4798) ...

aloisklink

Everything looks good to me, except that mermaid.render() won't throw any errors if there is a diag.renderer.draw() exception (unless suppressErrorRendering: true is set).

I think this might be a breaking change for the https://github.com/mermaid-js/mermaid-cli project, but I'm 90% sure this is just a typo by @sidharthv96 in 16c12a4 :)

By the way, does it make sense to add 'suppressErrorRendering' to the default secure key list:

mermaid/packages/mermaid/src/schemas/config.schema.yaml

Lines 150 to 158 in 4201e47

    
             secure: 
        
               description: | 
        
                 This option controls which `currentConfig` keys are considered secure and 
        
                 can only be changed via call to `mermaidAPI.initialize`. 
        
                 Calls to `mermaidAPI.reinitialize` cannot make changes to the secure keys 
        
                 in the current `currentConfig`. 
        
                 This prevents malicious graph directives from overriding a site's default security. 
        
               default: ['secure', 'securityLevel', 'startOnLoad', 'maxTextSize']

To be honest, doing something like the following seems pretty safe to me:

```mermaid
---
config:
  suppressErrorRendering: true # should this be allowed by default??
---
flowchart
  This diagram has an error in it
```

But there is a small part of me that is worried that it might cause a denial-of-service attack if some third-party code is expecting Mermaid to return an SVG and it doesn't.

packages/mermaid/src/mermaidAPI.ts

Co-authored-by: Alois Klink <alois@aloisklink.com>

sidharthv96 · 2023-09-15T03:14:49Z

Added suppressErrorRendering to the secure flags.
I don't see a reason for the individual diagrams to be modify the config.

aloisklink

Added suppressErrorRendering to the secure flags.
I don't see a reason for the individual diagrams to be modify the config.

👍 One last comment, there seems to be a race-condition in the E2E Cypress tests you added, but I've got a fix, see my comments :)

Other than that, everything seems great to me! It might be good to wait for @knsv to review, just to make sure there's no potential security issues with this change, but I think now that we've added supressErrorRendering to the secure flags, it should be okay.

cypress/integration/other/configuration.spec.js

Co-authored-by: Alois Klink <alois@aloisklink.com>

nirname

LGTM

* develop: (154 commits) chore(deps): update all patch dependencies chore: release v10.6.1 fix(flow): fix invalid ellipseText regex review fixes Update XYChart's nav link in the docs template add comment for ts ignore move decodeEntities to utils review fixes chore(deps): update all minor dependencies chore: Point to correct changelog add spec fix: getMessageAPI so it considers entity codes chore(deps): update all patch dependencies Update integrations-community.md docs: upate the list of tools with native support of mermaid Fix typo in build-docs.yml Updated mermaid version Limiting the number of edges that are allowed in the flowchart Update README.md Update README.md ...

jgreywolf · 2023-12-05T18:05:53Z

Can someone follow up on status here?

…error_rendering I ran `pnpm run --filter mermaid docs:build` to fix merge conflicts in docs/config/setup/modules/mermaidAPI.md

aloisklink · 2023-12-06T09:13:02Z

Can someone follow up on status here?

I'd like @knsv (or somebody experienced with the secure config key) to review this first. I'm pretty sure it's okay now that we've added suppressErrorRendering as a default value for the secure config key, but I'd like someone with more security knowledge to double-check that for me!

* develop: (171 commits) Lint Remove echo RefTest Echo event Update cypress Fix applitools docs: fix lint docs: move community to Discord Swap condition blocks to avoid using negation Reposition const declaration to ideal place Change repetitive values into consts docs: fix swimm link Fix Update Browserslist Use pnpm/action-setup@v2 Fix lint Cleanup e2e.yml Ignore push events on merge queue Remove :: Remove :: Remove :: ...

knsv · 2024-02-29T11:27:22Z

This looks great! Lets merge it!

* develop: (453 commits) chore: Minor fixes chore: Build docs Use develop as base on develop branch. Update renovate json update link update announcement and blog pages Remove `--force` flag Tweak editor.bash update link chore: update browsers list Update integrations-community: add Drupal and module. Support Firefox Address review comments Change run symbol feat: Make the examples interactive in the documentation site. Add langium chore: update browsers list chore(deps): update all patch dependencies chore(deps): update all minor dependencies Update keywords and description ...

mermaid-bot · 2024-03-23T10:00:45Z

@rhysd, Thank you for the contribution!
You are now eligible for a year of Premium account on MermaidChart.
Sign up with your GitHub account to activate.

Update mermaid to [v11](https://github.com/mermaid-js/mermaid/releases/tag/v11.0.0) and enable the new [`suppressErrorRendering` option](mermaid-js/mermaid#4359) to ensure mermaid never renders error elements into the DOM (we have per-chart error rendering, so don't need it). Tested various chart types. BTW, I was unable to reproduce that error rendering from mermaid with `suppressErrorRendering: false` and I thought we had some CSS to hide the error element, but I could not find it, not even in git history.

Add suppressErrorRendering option to avoid inserting 'Syntax error'…

2c5d83f

… message in diagram

sidharthv96 reviewed Apr 29, 2023

View reviewed changes

packages/mermaid/src/mermaidAPI.ts Outdated Show resolved Hide resolved

rhysd and others added 2 commits April 30, 2023 22:29

Throw error when detecting diagram type failed and `suppressErrorRend…

b4618f9

…ering` is set

sidharthv96 added 2 commits September 6, 2023 12:58

chore: Add suppressErrorRendering to config

b09cdc0

sidharthv96 added 2 commits September 6, 2023 13:09

Add test

0480339

Add test

65dabea

docs: Rebuild

8b8b828

sidharthv96 mentioned this pull request Sep 14, 2023

make errorRenderer.draw be optional #3205

Closed

sidharthv96 added 2 commits September 14, 2023 23:32

fix: Remove blank SVG

16c12a4

sidharthv96 requested review from nirname, knsv, aloisklink and Yokozuna59 September 14, 2023 18:05

aloisklink reviewed Sep 14, 2023

View reviewed changes

packages/mermaid/src/mermaidAPI.ts Show resolved Hide resolved

sidharthv96 and others added 2 commits September 15, 2023 08:41

chore: Add suppressErrorRendering to secure flags.

5f41b65

Co-authored-by: Alois Klink <alois@aloisklink.com>

fix: Retain default behavior when rendering errors cases

bd1e7a7

Co-authored-by: Alois Klink <alois@aloisklink.com>

aloisklink reviewed Sep 15, 2023

View reviewed changes

cypress/integration/other/configuration.spec.js Outdated Show resolved Hide resolved

cypress/integration/other/configuration.spec.js Outdated Show resolved Hide resolved

sidharthv96 and others added 2 commits September 16, 2023 13:33

fix: Race condition when running suppressError test.

56d339b

Co-authored-by: Alois Klink <alois@aloisklink.com>

fix: Cypress test for Suppress Error

577f0ca

nirname reviewed Oct 2, 2023

View reviewed changes

Merge remote-tracking branch 'origin/develop' into bug/4358_suppress_…

dacd2cb

…error_rendering I ran `pnpm run --filter mermaid docs:build` to fix merge conflicts in docs/config/setup/modules/mermaidAPI.md

github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Dec 6, 2023

nirname self-requested a review January 14, 2024 22:26

sidharthv96 added 3 commits January 26, 2024 23:05

chore: Remove cy.viewport

4256177

Merge branch 'develop' into bug/4358_suppress_error_rendering

7d0faa3

knsv approved these changes Feb 29, 2024

View reviewed changes

sidharthv96 enabled auto-merge March 23, 2024 08:11

sidharthv96 force-pushed the bug/4358_suppress_error_rendering branch from 63a77b0 to ecfa149 Compare March 23, 2024 08:17

sidharthv96 disabled auto-merge March 23, 2024 10:00

sidharthv96 merged commit 78587e1 into mermaid-js:develop Mar 23, 2024
18 of 19 checks passed

silverwind mentioned this pull request Aug 24, 2024

Update mermaid to v11 go-gitea/gitea#31913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `suppressErrorRendering` option to avoid inserting 'Syntax error' message to DOM directly #4359

Add `suppressErrorRendering` option to avoid inserting 'Syntax error' message to DOM directly #4359

rhysd commented Apr 29, 2023 •

edited by sidharthv96

Loading

codecov-commenter commented Jun 18, 2023 •

edited by codecov bot

Loading

netlify bot commented Sep 6, 2023 •

edited

Loading

sidharthv96 commented Sep 6, 2023

sidharthv96 commented Sep 6, 2023

rhysd commented Sep 6, 2023

sidharthv96 commented Sep 14, 2023 •

edited

Loading

aloisklink left a comment

sidharthv96 commented Sep 15, 2023

aloisklink left a comment

nirname left a comment

jgreywolf commented Dec 5, 2023

aloisklink commented Dec 6, 2023

knsv commented Feb 29, 2024

mermaid-bot bot commented Mar 23, 2024

	secure:
	description: \|
	This option controls which `currentConfig` keys are considered secure and
	can only be changed via call to `mermaidAPI.initialize`.
	Calls to `mermaidAPI.reinitialize` cannot make changes to the secure keys
	in the current `currentConfig`.

	This prevents malicious graph directives from overriding a site's default security.
	default: ['secure', 'securityLevel', 'startOnLoad', 'maxTextSize']

Add suppressErrorRendering option to avoid inserting 'Syntax error' message to DOM directly #4359

Add suppressErrorRendering option to avoid inserting 'Syntax error' message to DOM directly #4359

Conversation

rhysd commented Apr 29, 2023 • edited by sidharthv96 Loading

📑 Summary

📏 Design Decisions

📋 Tasks

codecov-commenter commented Jun 18, 2023 • edited by codecov bot Loading

Codecov Report

netlify bot commented Sep 6, 2023 • edited Loading

✅ Deploy Preview for mermaid-js ready!

sidharthv96 commented Sep 6, 2023

sidharthv96 commented Sep 6, 2023

rhysd commented Sep 6, 2023

sidharthv96 commented Sep 14, 2023 • edited Loading

aloisklink left a comment

Choose a reason for hiding this comment

sidharthv96 commented Sep 15, 2023

aloisklink left a comment

Choose a reason for hiding this comment

nirname left a comment

Choose a reason for hiding this comment

jgreywolf commented Dec 5, 2023

aloisklink commented Dec 6, 2023

knsv commented Feb 29, 2024

mermaid-bot bot commented Mar 23, 2024

Add `suppressErrorRendering` option to avoid inserting 'Syntax error' message to DOM directly #4359

Add `suppressErrorRendering` option to avoid inserting 'Syntax error' message to DOM directly #4359

rhysd commented Apr 29, 2023 •

edited by sidharthv96

Loading

codecov-commenter commented Jun 18, 2023 •

edited by codecov bot

Loading

netlify bot commented Sep 6, 2023 •

edited

Loading

sidharthv96 commented Sep 14, 2023 •

edited

Loading