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

themeVariable validation not allowing valid CSS attributes with dashes #6256

Open
MikeSQ opened this issue Feb 6, 2025 · 3 comments · May be fixed by #6261
Open

themeVariable validation not allowing valid CSS attributes with dashes #6256

MikeSQ opened this issue Feb 6, 2025 · 3 comments · May be fixed by #6261
Labels
Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect

Comments

@MikeSQ
Copy link

MikeSQ commented Feb 6, 2025

Description

When I try to use either the css function light-dark() or css variables like var(--bs-blue) to help make the diagrams I'm creating react well with light mode/dark mode changes in the hosting application, the validation is removing them.

Steps to reproduce

  1. Create new diagram, specifying CSS variables or the light-dark() function such as this:
---
title: Sequence Diagram
config:
  themeVariables:
    activationBorderColor: var(--bs-green)
    activationBkgColor: light-dark(lavender, saddlebrown)
---
sequenceDiagram
    autonumber
    participant Alice
    participant Bob
    Alice ->>+ Bob: Hello Bob
    Bob ->>- Alice: Hello Alice
Loading
  1. Observe: The generated styling does not define the requested variables at all

Screenshots

Image

Code Sample


Setup

  • Mermaid version: 11.4.1
  • Browser and Version: Edge 134.0.3109.0 (Official build) dev (64-bit)

Suggested Solutions

I think the issue is with the validation code found in sanitizeDirective.ts. It loops through with a regex and blanks out anything not matching the regex as so:

for (const k of Object.keys(args.themeVariables)) {
      const val = args.themeVariables[k];
      if (val?.match && !val.match(/^[\d "#%(),.;A-Za-z]+$/)) {
        args.themeVariables[k] = '';
      }
    }

I think it should be sufficient to add the dash character as a valid match, but I'm not sure if it was omitted for a specific reason. I think changing it to something like this would work for allowing the CSS variables and the light-dark() function:

for (const k of Object.keys(args.themeVariables)) {
      const val = args.themeVariables[k];
      if (val?.match && !val.match(/^[\d \-"#%(),.;A-Za-z]+$/)) {
        args.themeVariables[k] = '';
      }
    }

Here is a quick test I ran to verify expected behavior

var origRegex = /^[\d "#%(),.;A-Za-z]+$/
var proposedRegex = /^[\d \-"#%(),.;A-Za-z]+$/
var tester = { testVars: "--var(--bs-green)", testFunc: "light-dark(lavender, saddlebrown)" }

var testFunc = function(objecToTest, regexToUse) {
    for (const k of Object.keys(objecToTest)) {
      const val =objecToTest[k];
      if (val?.match && !val.match(regexToUse)) {
        objecToTest[k] = '';
      }
    }
    console.log(`After applying ${regexToUse}:`)
    console.dir(objecToTest)
}

testFunc({ ...tester}, origRegex)
testFunc({ ...tester}, proposedRegex)

Additional Context

No response

@MikeSQ MikeSQ added Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect labels Feb 6, 2025
@nour0205
Copy link

nour0205 commented Feb 7, 2025

Hi @MikeSQ,

Thanks for reporting this issue! I’ve reviewed the validation logic in sanitizeDirective.ts and completely understand why the current regex is stripping out valid CSS properties like var(--bs-blue) and light-dark().

Issue Analysis
The existing regex (/^[\d "#%(),.;A-Za-z]+$/) excludes dashes (-), which are essential for CSS variables and modern CSS functions.
This causes valid properties like var(--bs-blue) and light-dark(white, black) to be removed, breaking expected behaviour.

Proposed Fix & Considerations

  • Allowing - in the regex should resolve the issue without introducing security risks.
  • We should verify that no unintended attributes slip through and that Mermaid’s sanitization remains secure.
  • Expanding sanitizeCss() to intelligently validate CSS properties (rather than blanking them out) could be a future enhancement.

I’d be happy to test this fix locally and submit a PR if this approach aligns with the maintainers' expectations.
Let me know your thoughts especially if there was a reason-was initially excluded!

@MikeSQ MikeSQ linked a pull request Feb 7, 2025 that will close this issue
2 tasks
@MikeSQ
Copy link
Author

MikeSQ commented Feb 7, 2025

Hi @nour0205 thank you for taking a look and interest in this! I actually tried to fix it yesterday with my own commit but ran into some trouble with the pre commit hook. I was able to resolve it this morning and put in the above PR. If you would like to take the time to check it out to see if that looks alright then I would appreciate it!

@nour0205
Copy link

nour0205 commented Feb 7, 2025

Hi @MikeSQ,

I appreciate your effort in addressing the issue with your own commit. I’ll review your PR in detail and get back to you with my feedback as soon as possible.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants