Consider allowing HTML in datatip markdown #99
Comments
Datatips are much like our original tooltips, but they have a way to become sticky, after which the user can drag them anywhere he wants and also scroll through them. This pretty much removes the need for a separate dock. It also makes our UI consistent with that of other IDE packages and allows us to benefit from UI and functionality improvements done in atom-ide-ui itself. Finally, this code is pretty much ready for use, except there are some minor nitpicks in atom-ide-ui that I'd like ironed out before pushing this in php-integrator: - facebookarchive/atom-ide-ui#98 - facebookarchive/atom-ide-ui#99 References #315
This replaces our own implementation with the one from atom-ide-ui. Next
to having a consistent UI with other IDE packages and benefitting from
improvements made over there, they also offer several advantages over
our own implementation:
- It is configured here to automatically trigger (!) when the
paranthesis is typed. To add to that, it automatically remains open
as long as the user continues typing and doesn't move outside the
function call. This means it will never pop up automatically unless
either typing a new function call or explicitly requesting it.
- You can easily close it by pressing escape (#300) or moving outside
of the function call, so it gets in the way much less often.
- It also displays, next to the active parameter's documentation, the
documentation of the function itself (mostly deals with #301).
- It displays above the function call, so it doesn't overlap with
autocompletion (#311)
- You can drag them out of the way.
- You can attach keyboard triggers to them.
Finally, this code is pretty much ready for use on our end, but there
are some minor issues I'd like to see ironed out in atom-ide-ui before
releasing this:
- The API is currently still experimental
- The styling is currently a bit barebones; the name of the active
parameter is not displayed.
- HTML in descriptions is being escaped (same issue as
facebookarchive/atom-ide-ui#99)
References #300
References #301
References #311
|
This is somewhat related to #45, as the ability to render HTML in the Markdown is needed for compatibility with Linter v2 providers. |
|
Any news or progress with this issue? |
|
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. |
|
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 ***@***.***> 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
<#99 (comment)>,
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: |
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
MarkedStringdata 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
datatipsuses themarkedlibrary, 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
The text was updated successfully, but these errors were encountered: