fix: prerendering encodes url multiple times

[ivanhofer]

Currently the prerendering of SvelteKit will take the url from a redirect and pass it to the encodeURI function. This causes url parts e.g. query strings that are already encoded to be encoded a second time which may lead to incorrect urls.

This PR fixes that issue. Redirect urls only get encoded a single time.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

linting will fail because of src/core/adapt/test/fixtures/prerender/build/redirect-url-encoding/index.html. I was not able to exclude the file from prettier, without removing the functionality for reading the ignore patterns from the .gitignore file. What should we de here?

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

⚠️ No Changeset found

Latest commit: d77e6a0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[netlify]

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: d77e6a0

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61e3f29f54ea5200086837a5

[Rich-Harris]

Thank you — comments inline. Please remember to add a changeset :)

[Rich-Harris]

This should probably just be location?

Suggested change

	writeFileSync(file, `<meta http-equiv="refresh" content="0;url=${url.toString()}">`);
	writeFileSync(file, `<meta http-equiv="refresh" content="0;url=${location}">`);

[ivanhofer]

I'm not sure why it was originally set to encodeURI. I didn't want to break it so I first convert it to an URL object.
Is it possible that a unencoded url reaches this part? I think yes, so we can't use location here.

[Rich-Harris]

Spelunking through the git history led me here: #256

I think we need to escape the string, so that this doesn't cause havoc...

location: "><script>alert('pwned')</script>

...but it seems like encodeURI was probably the wrong choice.

[ivanhofer]

location: "><script>alert('pwned')</script>

This will no longer be a problem because it is an invalid URL. The code will throw an error

[Rich-Harris]

This is a valid (unencoded) URL:

https://example.com/"><script>alert('pwned')</script>

[ivanhofer]

and with this implementation, this would output the url as:
"https://example.com/%22%3E%3Cscript%3Ealert('pwned')%3C/script%3E"

[Rich-Harris]

linting will fail because of src/core/adapt/test/fixtures/prerender/build/redirect-url-encoding/index.html. I was not able to exclude the file from prettier, without removing the functionality for reading the ignore patterns from the .gitignore file. What should we de here?

this is a deeply stupid consequence of the lack of an ignore option in .prettierrc files, but there's a hack that works — add requirePragma: true for specific files

kit/.prettierrc

Lines 13 to 24 in 9de8094

	{
	"files": ["packages/kit/src/packaging/test/fixtures/**/expected/**/*"],
	"options": {
	"requirePragma": true
	}
	},
	{
	"files": ["packages/kit/src/core/adapt/prerender/fixtures/**/*"],
	"options": {
	"requirePragma": true
	}
	}

[ivanhofer]

I will address the comments in the next days!

[Rich-Harris]

Thank you. I think we can do this more simply by just escaping the attribute — I've opened #3456, will close this