fix: target blank removed from anchor tag

[devbyharshit]

📑 Summary

target=_blank was getting sanitized

Resolves #4716

📏 Design Decisions

I've added a Dompurify hook that will add target="_blank" and rel="noopener noreferrer" back to the elements which will have target property in them. While debugging I found that Dompurify when sanitizing the input string removes target property for security reasons but if we really need that I found this could be an option.

[netlify]

✅ Deploy Preview for mermaid-js ready!

Name	Link
🔨 Latest commit	3394541
🔍 Latest deploy log	https://app.netlify.com/sites/mermaid-js/deploys/653a30f88bf275000896e232
😎 Deploy Preview	https://deploy-preview-4933--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.

[codecov]

Codecov Report

Merging #4933 (3394541) into develop (5f117fc) will decrease coverage by 0.79%.
The diff coverage is 76.19%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4933      +/-   ##
===========================================
- Coverage    80.17%   79.39%   -0.79%     
===========================================
  Files          164      164              
  Lines        13820    13840      +20     
  Branches       693      698       +5     
===========================================
- Hits         11080    10988      -92     
- Misses        2591     2701     +110     
- Partials       149      151       +2     

Flag	Coverage Δ
e2e	85.25% <83.33%> (-0.98%)	⬇️
unit	42.96% <52.38%> (+0.02%)	⬆️

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

Files	Coverage Δ
packages/mermaid/src/diagrams/common/common.ts	66.43% <76.19%> (-1.60%)	⬇️

... and 8 files with indirect coverage changes

[devbyharshit]

@sidharthv96 I don't understand the checks that are failing and how to resolve those?
Any help!?

[nirname]

I have not investigated it further, but GA says

TypeError: Le.addHook is not a function
    at file:///home/runner/work/mermaid/mermaid/packages/mermaid/src/vitepress/.vitepress/.temp/app.js:8476:4
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

perhaps something else is needed to make it happen, we cannot merge it while the error it the build is being reproduced

[devbyharshit]

It could be that dompurify apis are not accessible when we're using the sanitzation on server end maybe, it's my speculation.

[nirname]

Yes, is it a maybe. Same guess. In that case we have to find a workaround

[nirname]

I am not well aware of DOMPurify functions, but DOMPurify.sanitize works

[devbyharshit]

@nirname So I was digging in on this and I found out that people are facing issues while using dompurify with SSR and the alternative is to use either jsDom or isomorphic-dompurify you may refer to the following thread

stackoverflow

but it still gave me addhook is not defined

[nirname]

@REVERB283 Yes, I saw this issue too. And I am at the same point as you are now. Try investigating it further, please.

Let's also summon @sidharthv96, perhaps he will give us the right direction. To summarize:

GA actions say addHook is not a function. We rely on the assumption that this should be available just as sanitize. But it may happen that addHook won't be available in SSR (if there is server side rendering). I have tried building docs locally and could not reproduce this error.

[devbyharshit]

You can try using

cd packages/mermaid && npm run docs:build:vitepress       

[devbyharshit]

@nirname I managed to resolve the remaining issues, please review it once again.

[nirname]

@REVERB283 yes, I see. Changes are OK. I do not fully understand why this works. Could you explain it for me? I'd like to clarify it for myself.

[nirname]

@REVERB283 could you add some tests to ensure that no future requests will brake this feature?

[devbyharshit]

@REVERB283 yes, I see. Changes are OK. I do not fully understand why this works. Could you explain it for me? I'd like to clarify it for myself.

I too don't have the proper clarity myself but it seems like when I call addHook() in the global scope it doesn't seem to be accessible; could be possible that it gets called before DOMPurify gets imported/defined (which should be highly unlikely). However when I call addHook() inside removeScript() it seems to work without errors.

I've come to this conclusion after updating the code and calling the guthub workflow "build-docs" multiple times on my local system using "act".

[devbyharshit]

@REVERB283 could you add some tests to ensure that no future requests will brake this feature?

I'm very new to unit testing but I can surely try. Can you help me out with the files where I need to write the test cases? Also can you point out some already implemented examples that I can take reference from?

Much appreciated.

[nirname]

@REVERB283 yes, sure. There are 2 types of tests - unit tests, which we mainly use for testing parsers, because they are internal. And e2e (end-to-end) also known as integration tests. What they do is starting client and performing actions on behalf of it (like clicking links in headless chrome, sending requests and fetching responses). In our case they check what the rendered image is, so these are used to test renderers.

All the integration tests are inside cypress folder, like this pie rendering test.

Tests for parsers are located in a specific diagram folder inside mermaid package, like this pie parser test.

Our current current e2e testing tool is Cypress. To reduce amount of checks and ensure that graph layout is stable we often compare 2 images. In our specific case we need to ensure that _target attribute inside SVG is present or absent. Have a look at this example, which in fact should have been implemented with image comparison (my bad). Anyway, this is just an example, good or bad, which you can take as a base. Very likely image comparison won't work for this current fix.

Here is another one for flowchart, which is basically identical. May be a little bit more descriptive since imgSnapshotTest and cy.get('svg').should((svg) => {}) expectations are used there along each other.

[devbyharshit]

@nirname I've added few test cases, can you review?

[nirname]

That'll do! May be we should wait for one more approval, but basically it is ready. Great job.

[devbyharshit]

@sidharthv96 any update?

[huynhicode]

@REVERB283 - Just chiming in here to let you know that @sidharthv96 is currently on vacation.

[devbyharshit]

@REVERB283 - Just chiming in here to let you know that @sidharthv96 is currently on vacation.

okay no worries!

[mermaid-bot]

@REVERB283, 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.