-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
…y widgets, minor cleanup
|
||
For instance if your js bundle is loaded as `bundle.js` on your page and you wanted to set [jsdelivr](https://www.jsdelivr.com) as your default CDN url for custom widgets, you could do the following: | ||
```html | ||
<script data-jupyter-widgets-cdn="https://cdn.jsdelivr.net/npm" src="bundle.js"></script> |
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.
Interesting -- this makes it so a custom frontend can specify where custom widgets are allowed to be loaded from?
Correct. This allows you to set the CDN to use when loading widgets. The HTML widget manager does something similar (ref).
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).
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).
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.
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 the WidgetManager
is getting constructed. Since the WidgetManager
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:
- Merge this PR so we can get the basic groundwork in
- Ship an alpha release of the package
- Open an issue to address the security issue, particularly use the alpha package to create an MVP of what the attack would look like, also see if we can get some insight from the ipywidgets folks on this
- Followup on the issue from Add support for Output widgets #3 as needed
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.
Co-authored-by: Safia Abdalla <[email protected]>
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.
@vivek1729 Mind creating a new issue for the CDN url thing?
Added #50 to track the security issue and engage with ipywidgets folks. |
Addresses #4 to support 3rd party widgets loaded from a CDN. We follow Jupyter standards and base the custom widget loader implementation off the
HTML manager
example in the ipywidgets project.Additional goodies with this PR
widget-comms.ts
widget-manager
testsHere's an example of the
ipyleaflet
custom widget in action:cc: @captainsafia