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 unescaped '/' in URI #50

Closed
wants to merge 2 commits into from
Closed

Conversation

behinger
Copy link

hashes with "/" are wrongfully cut into parts.

This fix clues the substrings back together. It has two requirements though that we need confirmation by @fonsp though:

  1. If the "payload" exists, it is always in parts[end]
  2. the notebookhash always ends with "=" AND no other sub-string (split by '/') of the URI ends with "="

Addresses #46 #42 #45, but instead of changing server configuration as #46, this fixes it within PlutoSliderServer - #46 indicates an issue with "+" as well - but we could not replicate it.

fixed together with @llips

@Pat-Laub
Copy link

Thanks! Re: #45, I tried adding this patch on top of the static-export-1 branch and that 404 problem still appears.

It did make me realise that the URI's were being escaped twice though, which was the problem over there.
That is, for notebook hash 7uehaeBoJorFiesT4uKI7Rl3bFJcsQwdILATmeaacfs=

encodeURIComponent("7uehaeBoJorFiesT4uKI7Rl3bFJcsQwdILATmeaacfs=")
"7uehaeBoJorFiesT4uKI7Rl3bFJcsQwdILATmeaacfs%3D"

and

encodeURIComponent(encodeURIComponent("7uehaeBoJorFiesT4uKI7Rl3bFJcsQwdILATmeaacfs="))
"7uehaeBoJorFiesT4uKI7Rl3bFJcsQwdILATmeaacfs%253D"

which corresponds to Deno needing
http://localhost:8000/bondconnections/7uehaeBoJorFiesT4uKI7Rl3bFJcsQwdILATmeaacfs%3D

whereas the Python server needed
http://localhost:8000/bondconnections/7uehaeBoJorFiesT4uKI7Rl3bFJcsQwdILATmeaacfs%253D

How to proceed, though, I'm not sure.

Is there simply a way to create hashes which doesn't use special characters that need escaping in a URL?
E.g. is it possible to just use the filename of the notebook instead of a hash of the contents?

@fonsp
Copy link
Member

fonsp commented Feb 5, 2022

Thanks for looking into this! I'll get back to you very soon, January was a bit too busy for me 😅

@fonsp
Copy link
Member

fonsp commented Mar 7, 2022

Hey @behinger ! Thanks for starting this topic! Let me say that it is awesome that you are using PlutoSliderServer, that you were able to fork and fix this on your own, and that you opened a PR to share your fix with the community!!

I think the solution in this PR does not catch all cases, and we still need to think about the problems with URI escaping the other character: + (and = for padding). I started working on the underlying problem in

I worked on this today, see #57

@behinger
Copy link
Author

behinger commented Mar 7, 2022

Thanks for the warm words! You can close this PR then.

@llips noticed a similar problem when using precompute slider server; we fixed it by providing our own Pluto.jl frontend code: llips/Pluto.jl@e1c8a52 (the option to do so was not very visible in any documentation, but I understand it is a not-ready-yet-PR & a lot of work. kudos!)
I guess this is the right time to mention this, if you need a more detailed report we can do that too.

Happy to share the outcomes of our work, we (mostly Luis) used the precompute PR successfully; needed some further tweaking in some details. Hope to be able to share it soon

@fonsp
Copy link
Member

fonsp commented Mar 7, 2022

Hey @behinger !

I would love any feedback you can give about using the precompute PR! My main question is: what is it like to develop notebooks that will be precomputed? Do you often write notebooks with too many possibilities? How do you know about this, and how do you solve it?

Would you recommend others to use it? Any tips you would give to people who want to start using it?

Thanks again! -fons

@fonsp
Copy link
Member

fonsp commented Apr 5, 2022

Fixed by #57

@fonsp fonsp closed this Apr 5, 2022
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