facebookarchive
Consider allowing HTML in datatip markdown
Description
I noticed that markdown in datatips sanitizes out HTML. I can understand this, but would like to request the ability to disable this, perhaps in the datatip or MarkedString data itself.
The reason for this is, at least in PHP, docblock comments can intermingle markdown and HTML and one codebase may decide to use markdown whilst one of its dependencies may resort solely to HTML.
I see that datatips uses the marked library, which I am currently also using. It should already allow this by simply not requesting sanitization of the input value. (In the end, the embedded HTML is just more HTML to be displayed in the datatip after the markdown has been converted.)
Expected Behavior
HTML would not be sanitized or there would be an option to disable it for datatip markdown strings.
Actual Behavior
HTML is always sanitized in datatip markdown strings.
Versions
- Atom: 1.21.0
- Client OS: Linux
- atom-ide-ui: 0.5.1
This is somewhat related to #45, as the ability to render HTML in the Markdown is needed for compatibility with Linter v2 providers.
Even though this is probably still useful, I'm considering trying to convert HTML to markdown for my use case - as really HTML in PHP docblocks is very rare nowadays. Not ideal, but it might work (for others as well).
I'll look into this - I enabled sanitization for fear of JS injection but that may not be a concern. (I imagine there is a flag to enable HTML but with script sanitization.)
Ah, I'm not sure how to do this. marked doesn't come with any builtin mechanism for basic HTML (related issues without answers: https://github.com/chjj/marked/issues/527, https://github.com/chjj/marked/issues/1037). The security consequences here aren't so severe but I'm still uncomfortable with the idea of allowing arbitrary JS in datatips... if there's a safe way of allowing basic HTML I'd be open to it.
Well, perhaps it is possible to let marked not touch the HTML and the running some kind of additional processing on the output to strip script tags? I think stripping script tags wouldn't be sufficient, as there are also other HTML attributes that can contain JavaScript.
Is there perhaps a way to activate a CSP for these inline scripts or evals or sandbox them?
Another idea would be to do the same I'm doing in my server now: convert HTML to markdown, but that comes with caveats of its own.
Yet another idea would be to either simply not allow plain HTML, it possible wouldn't be compatible with the linter API, but I guess security is more important than maintaining backwards compatibility, but that leaves other servers that do need this in the dark; I'm not sure if there are any servers that need this that aren't in a position to convert it themselves, though.
Finally, it could be hidden behind a switch to allow it via user settings, but it feels a bit like giving the user a shotgun to shoot himself in the foot: "Tick this checkbox to disable security and open everything up for unwanted scripts!".
There are plenty of HTML sanitizers out there we could use although the reality is that any language server on your box doesn't need to use HTML injection to do bad things - they run as a regular app with all the permissions and file access that comes with.
https://github.com/cure53/DOMPurify is the option we've used elsewhere on Atom with success.
On Sat, Jan 27, 2018 at 3:38 AM Gert-dev [email protected] wrote:
Well, perhaps it is possible to let marked not touch the HTML and the running some kind of additional processing on the output to strip script tags? I think stripping script tags wouldn't be sufficient, as there are also other HTML attributes that can contain JavaScript.
Is there perhaps a way to activate a CSP for these inline scripts or evals or sandbox them?
Another idea would be to do the same I'm doing in my server now: convert HTML to markdown, but that comes with caveats of its own.
Yet another idea would be to either simply not allow plain HTML, it possible wouldn't be compatible with the linter API, but I guess security is more important than maintaining backwards compatibility, but that leaves other servers that do need this in the dark; I'm not sure if there are any servers that need this that aren't in a position to convert it themselves, though.
Finally, it could be hidden behind a switch to allow it via user settings, but it feels a bit like giving the user a shotgun to shoot himself in the foot: "Tick this checkbox to disable security and open everything up for unwanted scripts!".
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/facebook-atom/atom-ide-ui/issues/99#issuecomment-360979098, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHQpy4CH0RlZE3c2JF4_pnXfqkUHcdQks5tOwqwgaJpZM4P-NEt .
That is very true.
I think there is one additional hole that opens up by not sanitizing script tags that propagate through the language server: the server might not care for it, but in my case, if I let HTML from PHP docblocks containing script tags propagate to Atom, theoretically any PHP dependency one uses in his or her project could use a script tag to try and exploit an Atom user as it would be injected into datatips or signature help.
FWIW, VS Code escapes any HTML tags inside of the markdown text returned from a language server:
https://github.com/Microsoft/vscode/blob/c67ef57cda90b5f28499646f7cc94e8dcc5b0586/src/vs/base/common/htmlContent.ts#L11
