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

decode url link targets #5383

Merged
merged 2 commits into from Sep 27, 2018
Merged

decode url link targets #5383

merged 2 commits into from Sep 27, 2018

Conversation

@minrk
Copy link
Contributor

@minrk minrk commented Sep 26, 2018

link targets can be escaped urls, but APIs expect unescaped "API path" strings

Example valid and correct markdown links that didn't resolve, but do now:

- [link](has%20space) (should resolve to "has space")
- [link](has%25percent) (should resolve to "has%percent")

Fixes #5153

I'd love to add a test, but I couldn't figure out how.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 26, 2018

Hmm, maybe this should be done here: https://github.com/jupyterlab/jupyterlab/blob/master/packages/rendermime/src/renderers.ts#L632

That would also be easier to test.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 26, 2018

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 27, 2018

Or here:

linkHandler.handleLink(anchor, path, hash);

I guess the big question: should the handlelink function be called with the proper api path, or should that be handlelink's responsibility to convert to an api path?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 27, 2018

But first all, thanks @minrk! We really appreciate you diving in to fix this!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 27, 2018

Certainly where @minrk put the logic is closest to where it's actually needed, and doesn't change the API any, so I think it's great to merge this PR and move the logic up the call stack if needed in a later PR too.

@minrk minrk force-pushed the markdown-url branch 2 times, most recently from 995e24b to 45cb44f Sep 27, 2018
- resolveUrl takes urls, returns urls
- decode urls to api path before passing to handleLink, contents api
- encode contents api paths before combining with urls
@minrk
Copy link
Contributor Author

@minrk minrk commented Sep 27, 2018

Thanks for the input! Following the links, I think the right place is in urlResolver, clarifying that the inputs are URLs and the outputs are URLs. This should fix escaping. There are a few places where URLs are passed directly to contents without unescaping and where contents api paths are added to URLs without escaping.

It should work now:

  • resolveUrl takes and returns urls
  • getDownloadUrl takes and returns urls
  • decode before calling contents api / handleLink
  • encode after getting path from contents before adding to url

And I added tests that are passing for me locally, which I'm pretty sure would fail on master.

@minrk
Copy link
Contributor Author

@minrk minrk commented Sep 27, 2018

I also tested manually by creating a double-encodable directory has%20 and creating the files:

  • notebook.ipynb
  • has spåce.png
  • double%20encode.png

Testing with this markdown in a notebook:

Images:

- `has spåce.png` *test space / unicode*
- `double%20encode.png` *test double-decode*

```
<img src="has%20sp%C3%A5ce.png">
<img src="double%2520encode.png">
```

<img src="has%20sp%C3%A5ce.png">
<img src="double%2520encode.png">


```
![test](has%20spåce.png)
![test](double%2520encode.png)

```

![test](has%20spåce.png)
![test](double%2520encode.png)

```
- [test space](has%20spåce.png)
- [test space](has%20sp%C3%A5ce.png)
- [test encode](double%2520encode.png)

```

- [test space](has%20spåce.png) (partially encoded)
- [test space](has%20sp%C3%A5ce.png) (fully encoded)
- [test encode](double%2520encode.png)

Being super clear and explicit when something's a URL and when it's a path is something we took a pretty long time to get right in nbclassic. One of the things that helped was to try to run every test with names and paths that needed escaping (base url contains space and unicode, always run in a subdir that needs escaping, and most if not all test filenames should be non-ascii or contain control characters)

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 27, 2018

Awesome, do you mind making a new issue to track adding those tests?

@blink1073 blink1073 added this to the 0.35 milestone Sep 27, 2018
@blink1073 blink1073 merged commit 17a165c into jupyterlab:master Sep 27, 2018
2 checks passed
@minrk minrk deleted the markdown-url branch Sep 27, 2018
@minrk
Copy link
Contributor Author

@minrk minrk commented Sep 27, 2018

@blink1073 opened #5389 for the testing

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 27, 2018

Thanks Min!

@blink1073 blink1073 mentioned this pull request Sep 28, 2018
31 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants