Do not escape query included in url launch parameter

[manics]

I'm not sure if this test is correct, but I think it illustrates the bug in
jupyterhub/nbgitpuller#329
https://discourse.jupyter.org/t/nb-on-binder-with-parameters-now-resulting-in-404/21860

 FAIL  js/packages/binderhub-client/lib/index.test.js
  ● Get full redirect URL and deal with query and encoded query (with pathType=url)

    expect(received).toBe(expected) // Object.is equality

    Expected: "https://hub.test-binder.org/user/something/endpoint?a=1/2&b=3%3F%2F&token=token"
    Received: "https://hub.test-binder.org/user/something/endpoint%3Fa=1/2&b=3%3F%2F?token=token"

      179 |       )
      180 |       .toString(),
    > 181 |   ).toBe(
          |     ^
      182 |     "https://hub.test-binder.org/user/something/endpoint?a=1/2&b=3%3F%2F&token=token",
      183 |   );
      184 | });

      at Object.toBe (js/packages/binderhub-client/lib/index.test.js:181:5)

[yuvipanda]

@manics I opened a PR to this PR fixing this: manics#8. Well, almost fixing it - you'll see it fails currently because the unencoded / in path does get encoded when we use URL. I'm not sure unencoded / are allowed in queryparams, but clearly our prior implementation did allow them.

So our two options are:

1. Determine that encoding any / in query params is ok
2. Use another method that strictly fully preserves path

I think (1) is ok, and we can sort of validate this by looking at the two reported URLs that had problems, and perhaps also look at our analytics event streams for paths with unencoded / in them. What do you think?

[manics]

First of all was my test definitely correct? I wasn't completely sure whether the path (including query) argument of getFullRedirectURL was meant to be encoded or not. If / is meant to be encoded in a query then I think let's assume it is for now since browsers often auto encode/decode some things even something doesn't quite follow the spec.

The main problem is the leading ? being encoded which it looks like you've fixed!

Edit: Feel free to rebase and force push if you want, otherwise I'll do it later

[manics]

Rebased

[consideRatio]

@manics could you update the title of this PR as well to reflect its more than just test related now?

[manics]

Done!

[yuvipanda]

I added an additional test specifically using nbgitpuller, and showing that URL encoding seems to be done now just the right amount, rather than double or none.

[consideRatio]

Ah!

I think there is a breaking change here.

-url.pathname = url.pathname + "/doc/tree/" + encodeURI(path);
+url = new URL("doc/tree/" + encodeURI(path), url);

These are different I think, to verify that:

console.log("old new behavior presented below")

let nonRoot1 = new URL("https://developer.mozilla.org/test")
nonRoot1.pathname = nonRoot1.pathname.replace(/\/$/, "");
nonRoot1.pathname = nonRoot1.pathname + "/doc/tree/"
console.log(nonRoot1.href)

let root1 = new URL("https://developer.mozilla.org")
root1.pathname = root1.pathname.replace(/\/$/, "");
root1.pathname = root1.pathname + "/doc/tree/"
console.log(root1.href)

console.log("new behavior comes below")

let nonRoot2 = new URL("https://developer.mozilla.org/test")
let newNonRoot2 = new URL("doc/tree", nonRoot2)
console.log(newNonRoot2.href)

let root2 = new URL("https://developer.mozilla.org")
let newRoot2 = new URL("doc/tree", root2)
console.log(newRoot2.href)

[manics]

The trailing / in the pathname is required (/test/ not /test), it affects how the parts are joined.

[yuvipanda]

Yeah, preventing this behavior is why we enforce path ends with / even if it is not passed in. So baseUrl will never not have trailing slash. As @manics says, it affects how parts are joined.

[yuvipanda]

@consideRatio I added another test case to demonstrate why I think this is fine.

[consideRatio]

Hmmm but what if baseUrl is a jupyterhub with a prefix?

Note that now, but not before, the nonRoot examples doesnt have the /test prefix in the path?

[consideRatio]

If you both think there is no loss of path issue, this is good even if i dont understand the details.

[manics]

Hmmm but what if baseUrl is a jupyterhub with a prefix?

The code appends a trailing / if there isn't one, so this should behave correctly

let nonRoot2 = new URL("https://developer.mozilla.org/test")
if (!nonRoot2.pathname.endsWith("/")) {
    nonRoot2.pathname += "/";
}
let newNonRoot2 = new URL("doc/tree", nonRoot2)
console.log(newNonRoot2.href)

https://developer.mozilla.org/test/doc/tree

[consideRatio]

Ah, so the href can be the same for a URL object, but end up resulting in a different URL when used to construct another. I think i got it and this LGTM

[consideRatio]

This looks good to me from what I can tell, and with the added tests, we should be in better shape than we already are I think.

I'm not sure about the expected behavior and such =/

[consideRatio]

See https://github.com/jupyterhub/binderhub/pull/1774/files#r1358088687, I think this isn't correct yet because there is a behavior change unrelated to escaping.

[yuvipanda]

ok, I tested this locally. You can first see me try an nbgitpuller link on the main branch, and it fails. Then I switch to this branch, and it succeeds (once if hard refresh the page to get new JS).

Screen.Recording.2023-10-14.at.12.57.24.AM.mov

Thank you for working on this @manics and thanks for the review, @consideRatio. I am hoping that my cleanup + automated testing efforts will allow us to build more of these things without breaking.