Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support custom widgets #47
Support custom widgets #47
Changes from 9 commits
947756f
a6b63c2
2ea82ee
aadbe7c
af29994
658b54f
8dc6a5e
4e8eba6
f2ce589
8e89ce8
e693ef4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgbkrk Thoughts on this model for passing custom properties to the JavaScript assets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting -- this makes it so a custom frontend can specify where custom widgets are allowed to be loaded from?
Probably worth noting that any output can add a tag to say to load other assets, so that's another vector for attack (though it means you need some HTML output already emitted in the document to use it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. This allows you to set the CDN to use when loading widgets. The HTML widget manager does something similar (ref).
Good point. The logic currently loads the the attribute from any script tag. I believe the way the for-loop is setup will cause it to favor the most recently inserted script tag which might be a problem.
@vivek1729 Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point @rgbkrk. @captainsafia rightly pointed out that I followed the pattern that the HTML widget manager to override the default CDN URL. The logic of reading and looping over
script
tags is also based off the HTML Widget manager (ref).Trying to understand how favoring the most recent added script tag might be a problem. Ideally, we'd hope that there's only 1 script tag that specifies the
data-jupyter-widgets-cdn
tag. Looping over script tags potentially opens up the possibility of script tags being added dynamically which can lead to the cdn URL being over-written. However, we should also note that the CDN url is set when theWidgetManager
is getting constructed. Since theWidgetManager
is a singleton, this would only happen once and it makes sense because we don't really want to have the base CDN URL change very often. It's more like a one-time setup configuration.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
WidgetManager
gets instantiated once when the first widget is rendered onto the page. It's possible for a nefarious to render a HTML output (or inject HTML into the page in some other way) that renders a script tag with a malicious CDN onto the page then render a second output containing the ipywidget.@jasongrout Do you have any thoughts on this since I believe you implemented the original code in the HTML manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captainsafia, thanks for the additional details. How do you suggest we address this concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I'm thinking that we:
If that sounds good to you we can move forward with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captainsafia , this sounds like a good plan to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll proceed with the PR when you get a chance to do a final review and approve.