Chore: Use DOMPurify to sanitize strings rather than js-xss

[KristianGrafana]

What is this feature?

This PR removes the use of the js-xss library in the sanitize function and utilize DOMPurify for sanitation of content, such as user-supplied HTML.

Furthermore, Markdown rendering will be sanitized with js-xss. Also, more characters are being escaped in the escapeHtml function.

Why do we need this feature?

DOMPurify has better performance and features, such as support for Trusted Types. DOMPurify is also more widely used. DOMPurify tries to handle mXSS, prototype pollution, SVG-sanitation and DOM-clobbering among other types of XSS attacks.

[ryantxu]

It seems like we used to allow the style and class

Yes, we will need that to be more lenient than just div -- i'm not sure what a reasonable limit list would be though

[KristianGrafana]

Yes, we will need that to be more lenient than just div -- i'm not sure what a reasonable limit list would be though
That is OK as long as we allow this behavior in Text Panels.

@ryantxu Take a look at 84c6302 -- the class and style attribute are now allowed on all elements.

This is fine to use in Text Panel's, but we should not use sanitizeTextPanelContent() or sanitize() on strict content (i.e where we only expect an <a>-tag), then we should only allow href.

[ryantxu]

I'll add some tests next week where we should aim to replicate existing behavior. In particular I think:

• sanitizeTextPanelContent -- needs to be pretty open, cleaning what it can and escaping the rest
• sanitize should be more strict -- I think this call should target your expected behavior and should be used everywhere we don't expect people to be mixing valid and invalid code together (eg the text panel!)

[KristianGrafana]

@ryantxu Some changes are now pushed. sanitizeTextPanelContent is not changed, only the sanitize function. The only modified test file is "markdown.test.ts" because renderMarkdown uses the sanitize function.

[ryantxu]

@KristianGrafana -- this looks good to me, but I think @ashharrison90 and @mckn should ✅ before merging since this will likely cause some mysterious change/side effect in their domain and/or stream of issues we see.

Can you

1. Update/add comments to sanitize and sanitizeTextPanelContent to more clearly say what they now do
2. Update the description of this PR to more clearly state what the impact of this change is? IIUC, the most notable change is that things that call sanitize(txt) will now have invalid things removed rather than escaped.
3. (With that in mind) should we change renderMarkdown(...) so it uses sanitizeTextPanelContent()? That better matches our current behavior and what happens in github etc <script>alert('hello')</script> :)

[leventebalogh]

Nice one! 👏

@KristianGrafana @ryantxu I have pushed an extra test case to check if the markdown rendering does not remove any code blocks while sanitising the text, I hope you don't mind. (I guess it is something that we would like to support?)

[KristianGrafana]

@ryantxu The latest commit adds comments in sanitize.ts, sanitizes markdown using js-xss (keeping sanitized content, old behavior) and modifies the markdown test file to reflect that.

This means that now only sanitize will sanitize using DOMPurify, sanitizeTextPanelContent will only sanitize Markdown (everywhere) and HTML rendered in the Text panel plugin.

@leventebalogh Thanks! I made some small changes to that test file since now markdown keeps sanitized content.

[ashharrison90]

had a quick test, looks good to me 🙌

you'll need to update the e2e test now you've added a couple more chars to the escapeHtml function

[mckn]

I have done some testing with the dynamic text panel which uses this functionality and seems to work as expected in this branch.

Great work with this!

[github-actions]

Backend code coverage report for PR #62787
No changes

[github-actions]

Frontend code coverage report for PR #62787

Plugin	Main	PR	Difference
elasticsearch	80.11%	80.11%	0%

[gabor]

hi @KristianGrafana , the changes in the folder public/app/plugins/datasource/elasticsearch seem unrelated to the PR.. may i ask why are they included?

[KristianGrafana]

@gabor Good catch, thanks! I unintendedly added files in a commit when fixed a merge conflict. Everything is OK now and fixed in ba85f5c

[torkelo]

@ryantxu why escape forward slash here? I don't see any other example of HTML escape doing that, and the browser does seem to do it either in (tested with this):

function escapeHtml(html){
var text = document.createTextNode(html);
var p = document.createElement('p');
p.appendChild(text);
return p.innerHTML;
}

[KristianGrafana]

@torkelo (answering since this was my PR) -- This is part of security enhancement, forward slash could be considered unsafe as it could be used like this <img/src/onerror=alert()> which breaks out of context. Other things such as paths, comments and schemas (file://) also use forward slash which can be used when exploiting vulnerabilities.

Is encoding forward slash causing issues?

[torkelo]

@KristianGrafana no, just curious why it was added, could not find it in any other escapeHtml implementation (incl browser impl)

[KristianGrafana]

@torkelo Understandable. It's a good question and it simply boils down to the context. In most implementations, / is not escaped as it has little to no impact of providing XSS protection. In odd corner cases escaping / might be helpful, but honestly, it might be a good idea to try to follow standards here.

[darkleaf]

I need to use pre tags in my html. Why did you forbid pre tags?

    return DOMPurify.sanitize(unsanitizedString, {
      USE_PROFILES: { html: true },
      FORBID_TAGS: ['form', 'input', 'pre'],
    });

27e2b03#diff-7c3fe444c88d3cc0c600cf945a13ce92e3932e054bee7e4cb8d53537fa9403d4R36

[KristianGrafana]

@darkleaf - that was to follow the policy that grafana/oncall uses. I can see that pre tags were recently allowed, so we can follow that. Fixed in #68512
Thanks for reaching out!