Document and use consistent encoding expectations for redirect output property

[moatra]

Describe the problem

The current documentation for the redirect output property of the loading function just says it should be:

a string containing the location to which they should be redirected

From what I can tell, this string is being treated with different expectations around encoding status depending on the current runtime location:

• The server will pass the redirect string into encodeURI(redirect)
• The client passes the redirect string into new URL(redirect, current_url)

If the expectation is that the developer should return an already properly encoded URI string, then the server path results in double encoding.

If the expectation is that the developer should be returning an unencoded string, the client path is relying on the URL constructor to tolerate unencoded URI strings. For the most part this works because the javascript engine bends over backwards, but can result in unintended results.

Example:

• Say I have a /login page that accepts a previous query parameter[1] that should contain the path of where to send the user after login.
• Pages behind a login wall that need to redirect unauthenticated users to the /login page with an appropriate previous query parameter.
• A user visits /app?foo=bar&fizz=buzz while unauthenticated, and the login wall wants to send them to the login page

If the login-wall treats the redirect string as pre-encoded:

return {
  status: 302,
  redirect: '/login?previous=' + encodeURIComponenent(request.url.pathname + request.url.search)
}

Server side rendering will use a Location header of

encodeURI('/login?previous=' + encodeURIComponent('/app?foo=bar&fizz=buzz'))
> "/login?previous=%252Fapp%253Ffoo%253Dbar%2526fizz%253Dbuzz"  // double encoded query parameter

And the client will use

new URL('/login?previous=' + encodeURIComponent('/app?foo=bar&fizz=buzz'), 'http://example.com')
> URL { href: "http://example.com/login?previous=%2Fapp%3Ffoo%3Dbar%26fizz%3Dbuzz", origin: "http://example.com", protocol: "http:", username: "", password: "", host: "example.com", hostname: "example.com", port: "", pathname: "/login", search: "?previous=%2Fapp%3Ffoo%3Dbar%26fizz%3Dbuzz" } // single encoded query parameters.

If the login-wall treats the redirect string as unencoded:

return {
  status: 302,
  redirect: '/login?previous=' + request.url.pathname + request.url.search
}

Server side rendering will use a Location header of

encodeURI('/login?previous=/app?foo=bar&fizz=buzz')
> "/login?previous=/app?foo=bar&fizz=buzz"  // completely unencoded query parameter

And the client will use

new URL('/login?previous=/app?foo=bar&fizz=buzz', 'http://example.com')
> URL { href: "http://example.com/login?previous=/app?foo=bar&fizz=buzz", origin: "http://example.com", protocol: "http:", username: "", password: "", host: "example.com", hostname: "example.com", port: "", pathname: "/login", search: "?previous=/app?foo=bar&fizz=buzz" }} // completely unencoded query parameters.

// Especially problematic because we effectively lose the `fizz` query param on the `previous` url:
$_.searchParams.get("previous")
> "/app?foo=bar"

[1] Yes, I'm familiar with the problems of open redirects. This is a simplified example.

Describe the proposed solution

The redirect property should be treated as already encoded, and the documentation should reflect that.

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

[moatra]

Possible duplicate of #2490

[moatra]

The encodeURI on the location header was added to kit/src/runtime/server/page/respond.js in PR #1273

Historically relevant issues:

• 1228

• 1238

• 3126 (removed encodeURI from /kit/src/runtime/server/index.js respond())

Nominally related issues:

• 3339