jupyter-server / jupyter_ydoc

Jupyter document structures for collaborative editing using Yjs/pycrdt
https://jupyter-ydoc.readthedocs.io
BSD 3-Clause "New" or "Revised" License
27 stars 17 forks source link

Trust the default cell #161

Closed krassowski closed 1 year ago

krassowski commented 1 year ago

Empty cells should be trusted by default. This accompanies a larger work in JupyterLab (https://github.com/jupyterlab/jupyterlab/pull/14345) aimed at preserving trust behaviour of classing notebook (https://github.com/jupyterlab/jupyterlab/issues/14025).

Addresses one point in https://github.com/jupyterlab/jupyterlab/issues/14347.

An alternative (as discussed in the linked PR) is to always add trusted: true metadata to code cells which do not have any outputs (which could be implemented in sharedModel.insertCell) with the pros/cons:

Given that security implications I am proposing the more conservative approach of doing it manually each time for now, but I think we could as well automate this in the future (with a big disclaimer in extension upgrade guide).

welcome[bot] commented 1 year ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly. welcome You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

krassowski commented 1 year ago

In general it would be good to get your thoughts on https://github.com/jupyterlab/jupyterlab/issues/14347 but I have a suspicion that this is JupyterLab-side issue and "Trust Notebook" implementation will need to be adjusted to work well with RTC.

krassowski commented 1 year ago

Should we do the same here?

No, not in general as create_ycell can be called with an existing cells, e.g. here:

https://github.com/jupyter-server/jupyter_ydoc/blob/74f46930c24eede65f31cc7e1861b305be5f5f02/jupyter_ydoc/ynotebook.py#L263-L264

and that may contain an outputs which may need sanitising.

davidbrochart commented 1 year ago

Yes, not in general but maybe after checking that the cell has no output?

krassowski commented 1 year ago

I would lean towards "still no", because if it comes from an untrusted notebook (with a code cell with outputs that is not trusted) if we mark other code cells with no outputs we would be changing notebook's metadata in flight inducing an unnecessary noise and signature change later on.

But it's not a hard no.

davidbrochart commented 1 year ago

Sounds good, thanks for the explanation!

fcollonval commented 1 year ago

CI failure is not related

welcome[bot] commented 1 year ago

Congrats on your first merged pull request in this project! :tada: congrats Thank you for contributing, we are very proud of you! :heart:

fcollonval commented 1 year ago

@meeseeksdev please backport to 0.2.x

lumberbot-app[bot] commented 1 year ago

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 0.2.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:

    git cherry-pick -x -m1 3b3a4af50ea46abd886076b69b93ee0dab6b05c5
  2. You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #161: Trust the default cell'
  1. Push to a named branch:
git push YOURFORK 0.2.x:auto-backport-of-pr-161-on-0.2.x
  1. Create a PR against branch 0.2.x, I would have named this PR:

"Backport PR #161 on branch 0.2.x (Trust the default cell)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

krassowski commented 1 year ago

Thank you for the backport and release!