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

FIX: Escape colon in JupyterHub link to repo #556

Merged
merged 5 commits into from
May 2, 2022

Conversation

matthew-brett
Copy link
Contributor

I've run into a problem using The Littlest JupyterHub with current
sphinx-book-theme. At the moment the generated URLs for JupyterHub are
of form:

<hub-url>/hub/user-redirect/git-pull?repo=https://<repo>&urlpath=tree/<nb_fname>

These work fine on a modern Kubernetes JupyterHub, I guess because of
a recent version of JupyterHub responding to the query.

But, on current The Littlest JupyterHub, still on JupyterHub 1.5.0, this
causes Nbgitpuller to stall if the user is not already logged in, with
log messages of form:

Disallowing redirect outside JupyterHub:
'/hub/user-redirect/git-pull?repo=https://github.com/<repo>&urlpath=tree/<nb_fname>'.

Once logged in, retrying the same link works correctly.

This can be fixed by escaping the : in the Nbgitpuller URL with a
%3A, in this PR.

@welcome
Copy link

welcome bot commented Apr 27, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This seems good to me - can you confirm that the link still works on a "modern" JupyterHub as well? Also it looks like you need to update your branch to the latest master

I've run into a problem using The Littlest JupyterHub with current
`sphinx-book-theme`. At the moment the generated URLs for JupyterHub are
of form:

```
<hub-url>/hub/user-redirect/git-pull?repo=https://<repo>&urlpath=tree/<nb_fname>
```

These work fine on a modern Kubernetes JupyterHub, I guess because of
a recent version of JupyterHub responding to the query.

But, on current The Littlest JupyterHub, still on JupyterHub 1.5.0, this
causes Nbgitpuller to stall if the user is not already logged in, with
log messages of form:

```
Disallowing redirect outside JupyterHub:
'/hub/user-redirect/git-pull?repo=https://github.com/<repo>&urlpath=tree/<nb_fname>'.
```

Once logged in, retrying the same link works correctly.

This can be fixed by escaping the `:` in the Nbgitpuller URL with a
`%3A`, in this PR.
@matthew-brett
Copy link
Contributor Author

I've rebased.

I tested a modern Kubernetes setup by replacing the : in the generated URL with %3A - it worked as expected.

I see that a few tests will need fixing if this is the right way to go.

Replace : with %3A in test files, to match escaping behavior.

Note: the file `test_topbar_launchbtns.html` does not appear to have
tests that check the URL.
@choldgraf
Copy link
Member

It's not realistic for us to add tests for JupyterHubs specifically, so I think it's fine to use this PR as confirmation that "it works" and just get the tests into a working state (my guess is the regression tests will need fixing - to update those we can just replace the relevant launch links w/ the escaped version)

@matthew-brett
Copy link
Contributor Author

Chris - you probably saw - but I updated the files for the regression tests. Tests now passing.

@@ -118,9 +118,10 @@ def add_launch_buttons(
)

if jupyterhub_url:
repo_esc = repo_url.replace(":", "%3A")
Copy link

@minrk minrk Apr 28, 2022

Choose a reason for hiding this comment

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

A more general replacement is

from urllib.parse import quote
repo_esc = quote(repo_url)

which will catch all characters that need encoding, not just :.

This will happen automatically if you use url-building functions to build the query, rather than by hand:

from urllib.parse import urlencode
url_params = urlencode(dict(repo=repo_url, urlpath=f"{ui_pre}/{repo}/{path_rel_repo}", branch=branch), safe="/")
url = f"{jupyterhub_url}/hub/user-redirect/git-pull?{url_params}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha - I was missing the safe="/" kwarg - that makes all the difference to readability.

Copy link

Choose a reason for hiding this comment

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

it does! I don't know why safe="/" is the default for urllib.parse.quote, but not urllib.parse.urlencode, but it should be fine for our purposes here.

Copy link
Member

@choldgraf choldgraf May 2, 2022

Choose a reason for hiding this comment

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

@matthew-brett wanna give this a shot so that our implementation is a bit less special-casey and more robust to other character wackiness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have done ...

matthew-brett and others added 2 commits May 2, 2022 10:41
Generalize URL encoding using urlencode function.
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

😍 I love it, thanks for the follow-up, and to @minrk for helpful guidance!

@choldgraf choldgraf changed the title Escape colon in JupyterHub link to repo FIX: Escape colon in JupyterHub link to repo May 2, 2022
@choldgraf choldgraf merged commit 5721a63 into executablebooks:master May 2, 2022
@welcome
Copy link

welcome bot commented May 2, 2022

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

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

Successfully merging this pull request may close these issues.

3 participants