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

Feature rendermime api #2595

Merged
merged 14 commits into from Jul 6, 2017
Merged

Conversation

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Jul 6, 2017

This cleans up the rendermime API by making the factories simpler, and establishing the following tenets:

  • All renderers must be able to handle trusted and untrusted data. However, a renderer may choose to handle untrusted data by rendering a simple message such as "Execute the cell to display the data".
  • A renderer factory can be marked "safe". A "safe" factory does something more intelligent with untrusted data than simply rendering "run cell to show data".
  • Getting the preferred mimetype takes the option of "preferSafe". For that case, only "safe" factories are considered first. If one is not found, then the "unsafe" factories are considered. This is okay, because even "unsafe" factories should properly handle untrusted data.
@sccolbert sccolbert closed this Jul 6, 2017
@sccolbert sccolbert reopened this Jul 6, 2017
@ellisonbg ellisonbg added this to the 0.25.0 milestone Jul 6, 2017
Copy link
Member

@blink1073 blink1073 left a comment

Thanks!

@blink1073 blink1073 merged commit 9c2623b into jupyterlab:master Jul 6, 2017
2 checks passed
@blink1073 blink1073 mentioned this pull request Jul 7, 2017
@vidartf
Copy link
Member

@vidartf vidartf commented Jul 7, 2017

To me this change has made the API less useful: It is not possible to gain as much information about the renderers any more. The main question is: can a renderer perform a helpful sanitation of untrusted outputs? A message like "Execute the cell to display the data" is not helpful.

I also notice that the HTML renderer has been marked as safe. To me, marking a renderer as safe should imply that the output will be the same regardless of the trusted status. This also gives sanitized HTML a false boost in the preference order. Combined with not being able to query about helpful sanitation, this makes it really hard to perform introspection on the available renderers.

@sccolbert
Copy link
Contributor Author

@sccolbert sccolbert commented Jul 7, 2017

The main question is: can a renderer perform a helpful sanitation of untrusted outputs

That's exactly what the "safe" flag means.

To me, marking a renderer as safe should imply that the output will be the same regardless of the trusted status.

That's at odds with your first statement.

@vidartf
Copy link
Member

@vidartf vidartf commented Jul 7, 2017

To be more precise, the information that I need is:

  • Will this renderer do things differently if this output is trusted or not (previously wouldSanitize()). I initially thought safe would replace this, but if HTML is marked as safe, this is not the case.
  • Is this renderer's sanitized rendering better than the next renderer in the search order. This is not a question that is possible to answer definitely, but not being able to distinguish unhelpful sanitizers only makes it harder.

@sccolbert
Copy link
Contributor Author

@sccolbert sccolbert commented Jul 7, 2017

Will this renderer do things differently if this output is trusted or not (previously wouldSanitize()). I initially thought safe would replace this, but if HTML is marked as safe, this is not the case.

That was also previously the case:

return !options.model.trusted;

@sccolbert
Copy link
Contributor Author

@sccolbert sccolbert commented Jul 7, 2017

You are also free to define your own factories with different ranks and safe values.

@vidartf
Copy link
Member

@vidartf vidartf commented Jul 7, 2017

That was also previously the case:

I thought this only marked it safe if the output was trusted? Now it says that it is safe even if untrusted?

@sccolbert
Copy link
Contributor Author

@sccolbert sccolbert commented Jul 7, 2017

There were two problems with the previous API:

The canRender() method was redundant. The factory already declared it handles specific mimetypes, for it then to not handle when called is not a good API as it makes it too hard to reason about what the system is actually designed to do and who is responsible for what.

The wouldSanitize() method seems to have little difference with a safe flag. It's not clear that the extra flexibility offered by making it a method is worth the complexity tradeoff.

@sccolbert
Copy link
Contributor Author

@sccolbert sccolbert commented Jul 7, 2017

I thought this only marked it safe if the output was trusted

That wouldn't make much sense. All renderers are safe if the output is trusted.

The renderer is saying it would sanitize the source if the model is not trusted, which is the same as saying the renderer is "safe" because it will sanitize untrusted data, but still show something more useful than an "I can't render this untrusted data" message.

@vidartf
Copy link
Member

@vidartf vidartf commented Jul 7, 2017

I read things over again very slowly, and I now understand the role of the safe flag. I guess I was confused by the name (now everything is "safe", the flag simply indicates the presence of a helpful sanitizer).

The wouldSanitize() method seems to have little difference with a safe flag.

Yes, the original goal of that would be to allow a sanitizer to do a quick scan for flags indication sanitation was needed, without any of the expensive operations needed to perform a correct sanitation. However, it never actually got implemented.

@sccolbert
Copy link
Contributor Author

@sccolbert sccolbert commented Jul 7, 2017

now everything is "safe", the flag simply indicates the presence of a helpful sanitizer

Yes, exactly. All renderers must be able to handle untrusted data, because there is no guarantee that someone wont create one outside the context of a factory which says its okay to do so. So, a renderer which ignores the trusted flag and does something potentially unsafe with untrusted data, should be considered broken.

@sccolbert
Copy link
Contributor Author

@sccolbert sccolbert commented Jul 7, 2017

maybe we rename the safe property to something more explicit likecanIntelligentlyHandleUntrustedData

@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants