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

Setting to disable redirect file browser launch #4999

Merged
merged 3 commits into from
Oct 31, 2019
Merged

Setting to disable redirect file browser launch #4999

merged 3 commits into from
Oct 31, 2019

Conversation

betteridiot
Copy link
Contributor

When it was originally added, the PR for launching a browser via a redirect file (#4260) created some issues for some environments-namely WSL, Chromebook, and Android (#4346 (comment), jupyter/help#496). The reason for the break in these environments is due to the file structure/path differences between the runtime and the browser.

This commit adds a setting to the jupyter_notebook_config.py that allows users to disable the use of redirect file for launching a browser in favor of the original, yet visible, URL + token approach. This setting: c.NotebookApp.use_redirect_file will be set to True by default.

When it was originally added, the PR for launching a browser via a redirect file (#4260) created some issues for some environments-namely WSL, Chromebook, and Android (#4346 (comment), jupyter/help#496). The reason for the break in these environments is due to the file structure/path differences between the runtime and the browser. 

This commit adds a setting to the `jupyter_notebook_config.py` that allows users to disable the use of redirect file for launching a browser in favor of the original, yet visible, URL + token approach. This setting: `c.NotebookApp.use_redirect_file` will be set to True by default.
@betteridiot
Copy link
Contributor Author

@takluyver, this PR solves #4346 as well

Copy link
Member

@takluyver takluyver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I don't much like the idea of letting users disable a security option, but I haven't come up with any better way around this. I've made some notes inline about the implementation, however.

notebook/notebookapp.py Outdated Show resolved Hide resolved
notebook/notebookapp.py Outdated Show resolved Hide resolved
notebook/notebookapp.py Outdated Show resolved Hide resolved
notebook/notebookapp.py Outdated Show resolved Hide resolved
notebook/notebookapp.py Outdated Show resolved Hide resolved
notebook/notebookapp.py Outdated Show resolved Hide resolved
Makes the necessary changes proposed by @takluyver in #4999
Comment on lines 652 to 663
For versions of notebook > 5.7.2, a security feature measure was added that
prevented the authentication token used to launch the browser from being visible.
This feature makes it difficult for other users on a multi-user system from
running code in your Jupyter session as you.

However, some environments (like Windows Subsystem for Linux (WSL) and Chromebooks),
launching a browser using a redirect file can lead the browser failing to load.
This is because of the difference in file structures/paths between the runtime and
the browser.

Disabling this setting will by setting to False will disable this behavior,
allowing the browser to launch by using a URL and visible token (as before).
Disabling this setting to False will disable this behavior, allowing the browser
to launch by using a URL and visible token (as before).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed language regarding the security risk if the setting is disabled

notebook/notebookapp.py Outdated Show resolved Hide resolved
@betteridiot
Copy link
Contributor Author

I believe these changes address all of your review notes @takluyver

Copy link
Member

@takluyver takluyver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I noticed one more small thing, otherwise I think this looks good.

notebook/notebookapp.py Outdated Show resolved Hide resolved
As per the comments in #4999 (comment), the setting was removed.
@takluyver takluyver merged commit a90957d into jupyter:master Oct 31, 2019
@takluyver
Copy link
Member

Thanks :-)

@betteridiot
Copy link
Contributor Author

No. Thank you!

Any input as to the release schedule to conda-forge on this?

@takluyver
Copy link
Member

@lresende has done the last couple of releases - I'll let him answer about if/when he's planning another one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants