Fix notebook trust in RTC #12756
Fix notebook trust in RTC #12756
Conversation
|
Thanks for making a pull request to jupyterlab! |
|
Trigger CI. |
| const codeCell = this.sharedModel as models.YCodeCell; | ||
| const metadata = codeCell.getMetadata(); | ||
| metadata.trusted = true; | ||
| codeCell.setMetadata(metadata); |
There was a problem hiding this comment.
Does it mean that if other users execute one cell, this cell on my notebook is trusted automatically?
There was a problem hiding this comment.
Yes, I would say that if they can execute a cell it means they are trusted users. What do you think?
There was a problem hiding this comment.
The feedback I get from my PR (#11494 (comment)) is that in some contexts, trusting automatically, especially for cells containing JS code, can make users exposed to intended attacks
There was a problem hiding this comment.
I agree that user roles can be part of the picture here, for instance giving a user the right to execute cells implicitly means that this user is trusted.
But since we don't have roles yet, maybe your work in #11494 could be extended to all outputs, not only widgets?
There was a problem hiding this comment.
You're right, since users have the right to execute cells, they are trusted for their activites on the server-side. My PR prevents malicious activities on the frontend side, which mostly relates to the HTML/JS rendering. So I don't think we need to extend this to the text/plain output.
There was a problem hiding this comment.
But outputs can execute JavaScript too, if they are of type text/html. This is what the trust mechanism protects from. So I think your PR can be very relevant in this case too.
There was a problem hiding this comment.
For now, I only check the application/* type, but indeed I also need to take a look at the text/html type.
|
We should discuss this at a JupyterLab call. There is something wrong to use the cell metadata to store the trust status (actually there is a protection to clean that key in nbformat when reading or writing a notebook). Although I agree on the statement, if the user has execution rights, everybody should trust him. But using the metadata implies modifying the data will change the trust for everybody; this may not be wanted if the expected roles are read, read/write, read/write/execute. |
In this PR I only apply the current trust mechanism, but it's true that it works by passing a trust flag in the cells' metadata transiently between the front-end and the back-end (this information is not saved in the notebook format). |
|
I think this makes sense. The alternative is for cell-level trust to not be shared and for each individual users to need to trust (run) each cell. That seems redundant and inconsistent with the level of mutual trust that collaborators need to have in each other to do real-time collaboration where everyone can run code. |
|
@davidbrochart can you rebase? Others, any more comments on this? |
Actually my changes don't apply any more to the master branch, for instance |
|
I think this should be merged in 4.0 first. |
References
None.
Code changes
Propagate cell trust to/from shared model.
User-facing changes
The notebook trust mechanism now works in RTC.
Backwards-incompatible changes
None.