fix: encode non-ASCII characters before redirecting#16065
Conversation
🦋 Changeset detectedLatest commit: 74c6947 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…the try/catch, so a lone-surrogate location throws an unhelpful `URIError` instead of the intended descriptive error.
This commit fixes the issue reported at packages/kit/src/exports/internal/index.js:31
## Bug
In `Redirect`'s constructor, the location encoding step ran *before* the `try` block:
```js
location = location.replace(/[^\u0020-\u007E\t\n\r]+/g, (match) => encodeURIComponent(match));
try {
new Headers({ location });
} catch {
throw new Error(`Invalid redirect location ...`);
}
```
`encodeURIComponent` throws `URIError: URI malformed` when given a string containing a lone/unpaired surrogate. This was verified:
```
$ node -e "'/\uD800'.replace(/[^\u0020-\u007E\t\n\r]+/g, m=>encodeURIComponent(m))"
URIError: URI malformed
```
**Trigger:** `redirect(303, '/\uD800')` (any redirect location containing an unpaired surrogate).
Because the `.replace()` call happens *outside* the `try`, the raw `URIError` escapes to the caller. Previously, such input flowed into `new Headers({ location })`, which threw a `TypeError` that the `catch` converted into the descriptive `Invalid redirect location ...: this string contains characters that cannot be used in HTTP headers` error. So this is a regression in error quality for malformed/unpaired-surrogate inputs.
## Fix
Moved the `.replace()`/`encodeURIComponent` call *inside* the existing `try` block. Now any `URIError` from malformed surrogates is caught and converted into the intended descriptive error, restoring the previous contract.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: teemingc <chewteeming01@gmail.com>
|
Doesn't import { redirect } from '@sveltejs/kit';
export const load = () => {
redirect(303, 'https://github.com/hyunbinseo/'); // works
redirect(303, 'https://내도메인.한국/'); // fails
redirect(303, new URL('https://내도메인.한국/')); // works
};Wouldn't it be safer to use |
|
Yeah, good idea. Let's try that |
|
I'm trying to do something like: // encode non-ASCII characters
if (typeof location === 'string') {
const event = getRequestEvent();
// location could be a pathname so we need to add a base URL
location = new URL(location, event.request.url);
}But having |
| constructor(status, location) { | ||
| try { | ||
| // Encode non-ASCII characters so the location is valid in HTTP headers | ||
| location = location.replace(/[^\u0020-\u007E\t\n\r]+/g, (match) => encodeURIComponent(match)); |
There was a problem hiding this comment.
Is there a good reason not to just use encodeURI on the whole location?
There was a problem hiding this comment.
const str = 'https://내도메인.한국/';
new URL(str).href; // "https://xn--220b31d95hq8o.xn--3e0b707e/"
encodeURI(str); // "https://%EB%82%B4%EB%8F%84%EB%A9%94%EC%9D%B8.%ED%95%9C%EA%B5%AD/"The resulting string doesn't use puny code, but I can access it in desktop Firefox and Chrome.
There was a problem hiding this comment.
Yeah I don't think we have to use punycode, it should work fine... it also should work okay for locations that are just pathnames and not full URLs as well.
There was a problem hiding this comment.
Notably new URL on the output string of encodeURI parses it down to the punycode version
There was a problem hiding this comment.
Is there a good reason not to just use
encodeURIon the wholelocation?
There's the double encoding of % but we can probably workaround it with a similar solution to our decoding utility.
/**
* Decode pathname excluding %25 to prevent further double decoding of params
* @param {string} pathname
*/
export function decode_pathname(pathname) {
return pathname.split('%25').map(decodeURI).join('%25');
}There was a problem hiding this comment.
Actually I did some work on this today and I think your current solution is close to right -- will push up a couple of commits with tests tomorrow
| constructor(status, location) { | ||
| try { | ||
| // Encode non-ASCII characters so that the location is valid in HTTP headers | ||
| if (!location.match(/[\r\n]/)) { |
There was a problem hiding this comment.
@elliott-with-the-longest-name-on-github is this necessary? Otherwise, I can't seem to get make redirect throw an "invalid character" error because they always get URL encoded
There was a problem hiding this comment.
So I'm pretty sure we can do away with this error. We originally put it in place because we wanted "no characters that are invalid in headers", but if we encode them, they're valid in headers (and don't cause the security issue we had before, where newlines could inject set-cookie headers)
| if (!location.match(/[\r\n]/)) { | ||
| location = location.startsWith('/') | ||
| ? encode_pathname(location) | ||
| : new URL(location).toString(); |
|
Looks like we purposely removed this in the past #3404 wonder if that's a case against this. The docs from that PR were also lost in time and should maybe be added back https://github.com/sveltejs/kit/pull/3404/changes#diff-820e98511cb87c8891f398a00ff2f70793de22e53396d0a3ac6bc2cd6cc3e56eR146 |
|
Re: above, the reason for this was double encoding, so if we can avoid that somehow... 🤔 |
closes #16054
This PR calls
encodeURIComponentfor non-ASCII characters before redirecting. Not sure if this is the best solution as I used Opus for this. Tests seem useful though.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits