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

gh-3403: Remove encodeURI call for redirect load property #3404

Merged
merged 1 commit into from
Jan 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/early-clocks-sneeze.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

The redirect property returned from a module's load function must now be a properly encoded URI string value.
2 changes: 2 additions & 0 deletions documentation/docs/03-loading.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ If something goes wrong during `load`, return an `Error` object or a `string` de

If the page should redirect (because the page is deprecated, or the user needs to be logged in, or whatever else) return a `string` containing the location to which they should be redirected alongside a `3xx` status code.

The `redirect` string should be a [properly encoded](https://en.wikipedia.org/wiki/Percent-encoding) URI. Both absolute and relative URIs are acceptable.

#### maxage

To cause pages to be cached, return a `number` describing the page's max age in seconds. The resulting cache header will include `private` if user data was involved in rendering the page (either via `session`, or because a credentialed `fetch` was made in a `load` function), but otherwise will include `public` so that it can be cached by CDNs.
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/src/runtime/server/page/respond.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export async function respond(opts) {
{
status: loaded.loaded.status,
headers: {
location: encodeURI(loaded.loaded.redirect)
location: loaded.loaded.redirect
}
},
set_cookie_headers
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<a href="/encoded/苗条">苗条</a>
<a href="/encoded/土豆">土豆</a>
<a href="/encoded/反应">反应</a>
<a href="/encoded/redirect">Redirect</a>
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<script context="module">
/** @type {import('@sveltejs/kit').Load} */
export function load() {
return {
status: 307,
redirect: 'redirected?embedded=' + encodeURIComponent('/苗条?foo=bar&fizz=buzz')
};
}
</script>
17 changes: 17 additions & 0 deletions packages/kit/test/apps/basics/src/routes/encoded/redirected.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<script context="module">
/** @type {import('@sveltejs/kit').Load} */
export function load({ url }) {
return {
props: {
// nb: .get() on URLSearchParams does a decoding pass, so we should see the raw values.
embedded: url.searchParams.get('embedded')
}
};
}
</script>
<script>
/** @type {string} */
export let embedded;
</script>

<pre>{embedded}</pre>
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
export function load() {
return {
status: 307,
redirect: '苗条'
redirect: encodeURI('苗条')
};
}
</script>
16 changes: 16 additions & 0 deletions packages/kit/test/apps/basics/test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,22 @@ test.describe.parallel('Encoded paths', () => {
expect(decodeURI(await page.innerHTML('h3'))).toBe('/encoded/苗条');
});

test('redirects do not re-encode the redirect string', async ({ page, clicknav }) => {
await page.goto('/encoded');

await clicknav('[href="/encoded/redirect"]');

// check innerText instead of innerHTML because innerHTML would return the '&amp;' character reference instead of '&' character.
expect(await page.innerText('pre')).toBe('/苗条?foo=bar&fizz=buzz');
});

test('redirects do not re-encode the redirect string during ssr', async ({ page }) => {
await page.goto('/encoded/redirect');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have experience with playwright, but I made the assumption goto does a full page load that triggers SSR.

Copy link
Member

Choose a reason for hiding this comment

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

you assume correctly :)


// check innerText instead of innerHTML because innerHTML would return the '&amp;' character reference instead of '&' character.
expect(await page.innerText('pre')).toBe('/苗条?foo=bar&fizz=buzz');
});

test('sets charset on JSON Content-Type', async ({ request }) => {
const response = await request.get('/encoded/endpoint');
expect(response.headers()['content-type']).toBe('application/json; charset=utf-8');
Expand Down