Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow rich content in Mermaid diagrams #5771

Closed

Conversation

vermiculus
Copy link

As discussed at 1, being a static site generator affords MkDocs a higher trust in the source content. If content creators would like to embed HTML/scripts in their diagram, we can allow that.

As discussed at [1], being a static site generator affords MkDocs a
higher trust in the source content. If content creators would like to
embed HTML/scripts in their diagram, we can allow that.

[1]: squidfunk#3812
@squidfunk
Copy link
Owner

Thanks for the PR. I'm not sure it's a good idea to enable this for everyone. Please provide some more explanations about the implications and why you think that it's okay to do this. Also, the link is broken.

@vermiculus
Copy link
Author

vermiculus commented Jul 28, 2023

The link isn't broken; GitHub's default markdown representation of the commit message is broken. Refer to the commit itself for the link for now, if you would.

I'll update the commit message with these details once it's settled, but I've got a need to have an ordered list with links in a flowchart like so:

```mermaid
flowchart TD
  id(["Here's a flowchart"])
  id1["Review the following:<br><ol>
    <li>Resource 1</li>
    <li>Resource 2</li>
    <li><a href='https://example.com'>Resource 3</a></li>
    </ol>"]
  id --> id1
  id1 --> id2["Did these address your question?"]
  id2 -- Yes --> id3(["You're all set!"])
```

Without the security level changes from the linked discussion, the <> are escaped and the marked-up content appears literally.

@squidfunk
Copy link
Owner

Thanks. However, we're still missing the following information as asked in my last comment:

Please provide some more explanations about the implications and why you think that it's okay to do this.

@vermiculus
Copy link
Author

vermiculus commented Jul 28, 2023

From mermaid's docs (emphasis mine):

If you are taking responsibility for the diagram source security, you can set the securityLevel to a value of your choosing.

If I understand correctly, the risks incurred involve using a built site as a vector for javascript-based attacks against the client (unless we go with antiscript over loose). I'm not personally aware of any possible malicious behavior with pure HTML, but I'd defer to any experience you might have here.

This could realistically come up with untrusted developers in very large teams in a central repository all contributing to a site building untrusted code with mike, for instance. But given the existence of things like extra_javascript, I don't think we're any worse for wear.


Since it doesn't appear that any new attack vectors are opened (at least with my current understanding of how mkdocs-material is used in the wild, which you are certainly a better judge of), the benefits of being able to have rich content in diagrams would seem to outweigh the (lack of) risk.

@vermiculus
Copy link
Author

Is there any more info you need from me here or is this just waiting for time to review? Just want to make sure you're not waiting for me 🙂

@squidfunk
Copy link
Owner

We're waiting for feedback for other users, and we're currently very busy restructuring our docs and getting 9.2 into a stable state. Please give us some more time.

@vermiculus
Copy link
Author

Absolutely understand; not looking to add pressure :-) I know what's like being a maintainer for a popular project.

I'll check back in two weeks' time.

@vermiculus
Copy link
Author

Checking back as promised :-)

@squidfunk
Copy link
Owner

9.2 is still not released (still in beta) and we're busy with the refactoring to allow for better inter-op of the blog plugin with other plugins. We can talk about this once 9.2 is released and the dust settled (i.e. the first wave of issues has been resolved).

@vermiculus
Copy link
Author

Sounds good. I'll check back after 9.2 is released + whatever 'dust settling time' you think is likely – defaulting to another two weeks.

Re any 'dust settling time' estimate you can provide: not looking for anything even resembling a commitment/promise/etc. – I just don't want to clutter your notifications unnecessarily / prematurely 😃

Thanks for your work in maintaining this toolchain.

@squidfunk
Copy link
Owner

You don't need to check back regularly. We tackle this PR once we find the time, you'll get notified when we do 😉

@squidfunk
Copy link
Owner

@sisp @nejch I'd like your opinion on this PR ☺️

@sisp
Copy link
Contributor

sisp commented Sep 2, 2023

I think Mermaid's securityLevel: "strict" disables opportunities for XSS attacks which are mainly relevant for websites on which anybody can publish Mermaid diagrams, e.g. GitHub. But I believe it would be safe to use securityLevel: "loose" for statically built websites with Mermaid diagrams defined in its sources because only the site's author controls them. That said, it might be dangerous if a static website fetched Mermaid diagram sources via JavaScript from a third-party source. Although this use case might be quite rare, I'd prefer keeping securityLevel: "strict" by default and rather make it configurable for those who consciously want to change it while being aware of the implications.

@nejch
Copy link
Contributor

nejch commented Sep 3, 2023

I would agree with @sisp here, it might make sense to add it as a config option. It would also follow mermaid's recommendation:

It is the site owner's responsibility to discriminate between trustworthy and untrustworthy user-bases and we encourage the use of discretion.

There are lots of different ways MkDocs pages are deployed sometimes, including various integrations and multirepo plugins pulling in other sources, plugins and configuration, so IMO should be up to the user to decide this.

@squidfunk
Copy link
Owner

Thanks to both of you for your input. Were currently not considering making it configurable via mkdocs.yml, because we're not interested in adding more and more options for everything. If somebody can come up with an approach that let's the user configure it with additional Javascript, we can consider it.

As a workaround, the mkdocs-mermaid2-plugin might allow this already.

@squidfunk squidfunk closed this Sep 4, 2023
@vermiculus
Copy link
Author

As a workaround, the mkdocs-mermaid2-plugin might allow this already.

Thanks for the reference! Yes, that plugin does support what I need: https://mkdocs-mermaid2.readthedocs.io/en/latest/tips/#setting-the-security-level-to-loose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants