Prevent XSS in Latex and SVG Elements

[roman-mibex-2]

• The svg handler added the notebook content directly to the .innerHTML, allowing it to run JavaScript by using HTML tags instead of HTML. Fixed by running the sanitizer over the SVG.
• The latex handler added its content directly to the .innerHTML. This allowed to inject any JavaScript/HTML instead of Latex. Fixed by using .innerText, to render the content.

[roman-mibex-2]

To discuss:

• [ ] The Latex fix will 'break' the 3rd party addon / preprocess like this library: https://github.com/bradhowes/notebookjs-katex/tree/master. Because that library works by replacing the Latex in the *.ipynb JSON with rendered HTML. Note sure how to handle that. In the follow up commit I've extended the Katex support, so that 3rd party library isn't needed anymore.

Also, the Latex text is rendered differently. Example Before: After fix:

[jsvine]

Thanks for this, @roman-mibex-2. Some notes below:

The Latex fix will 'break' the 3rd party addon / preprocess like this library: https://github.com/bradhowes/notebookjs-katex/tree/master. Because that library works by replacing the Latex in the *.ipynb JSON with rendered HTML.

Is there a method we could provide 3rd-party developers that would allow them to override this issue. Or are the current options sufficient, and only require the 3rd-party developers to make minor adjustments?

Note sure how to handle that. In the follow up commit I've extended the Katex support, so that 3rd party library isn't needed anymore.

For my own notes: This refers to https://github.com/jsvine/notebookjs/pull/48/files

Also, the Latex text is rendered differently.

Do you think there's a way to get the "after" version to render as it previously did? Or is this unavoidable if we're trying to prevent XSS?

[roman-mibex-2]

Is there a method we could provide 3rd-party developers that would allow them to override this issue. Or are the current > options sufficient, and only require the 3rd-party developers to make minor adjustments?

We used the mentioned 3rd party app so far. I couldn't make it work while also preventing any XSS. The main reason is that this 3rd party app relied on the XSS 'friendliness'. It was a pre-processor for the ipynb-JSON and replaced String literals with HTML tags to then have it rendered by notebookjs. That is a very fragile way to do it and I failed to make it safe. Note, the mentioned library also 'competed' with the AutoKatex approach directly supported by notebookjs. By using it we had two mechanisms detecting Latex. Therefore, with 'https://github.com/jsvine/notebookjs/pull/48/files' we get rid of the 3rd party library and have a consistent Latex rendering code path.

Do you think there's a way to get the "after" version to render as it previously did? Or is this unavoidable if we're trying to prevent XSS?

There are multiple options here:

• The current option: Display Latex as proper 'Text', optionally enhanced by AutoKatex. It formats the Latex as pure text with interpreting newlines "\n" as new lines in the actual text. Will not interpret any HTML tags at all. I would argue that this is more 'correct', as Latex shouldn't be interpreted as HTML. However, the question is, if newlines should be interpreted or now.
• The conservative option/more backward compatible, use the nb.sanatizer() on the Latex field: It formats the Latex as 'HTML', but prevents XSS. This renders as before and is more backward compatible on how the library behaved. It may even keep the 3rd party plugin working. I would say this is more 'incorrect', as Latex still is interpreted as HTML, but at least it prevents XSS.

[roman-mibex-2]

Update. I've checked how MathJax etc do render Text in Latex. New lines are ignored. So, I've updated the pull request to not render new lines. This will now render the same as the previous versions, but won't interpret and HTML tags.

[roman-mibex-2]

Ping. In case I won't react next week, I am on vacation.

[jsvine]

Thank you; ping received. I've been a bit busy, but still intend to review this and respond. Thanks for your patience.

[jsvine]

Thank you again for this, @roman-mibex-2, and my apologies for the delay in following up. I'm going to merge this, but tweak the latex solution a bit — default will be nb.sanitize(...) but user will be able to configure that option.

[jsvine]

Actually, now that I see your proposed fix in #48, let's go with that as the Latex solution. (Still merging the SVG fix in this PR.)