[fix] don't redirect to external URLs

[benmccann]

fixes #2515

[changeset-bot]

🦋 Changeset detected

Latest commit: 9adab9e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

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

[mrkishi]

I believe it'd make sense to put the full url into the location header instead. Even if unusual, those filtered paths are valid.

[Conduitry]

I'd be concerned about also using the origin/host settings for that. I haven't double checked to make sure, but I believe that would make my life more difficult with an app that lives behind a proxy alongside other services for its API endpoints.

[Conduitry]

Hm. I may have been wrong about that for my particular case, and would need to dig in more to be sure, but my gut instinct is still that always using absolute URLs is more trouble than it's worth, if there's something a bit unusual with how the app is deployed.

[Conduitry]

Apologies for the comment spam. I'm now re-reading @mrkishi's comment in light of what this PR is supposed to actually be fixing, and I'm a bit confused.

This does feel like an incorrect fix to me, but I'm not sure what a correct fix would be. When we're redirecting to the normalized version of a particular URL, we should probably make sure we're always specifying a URL that's host-relative. normalize_path is always going to be getting passed just the path part, and so that's what normalized is going to contain as well. In what circumstances is this generating a redirect that the browser is interpreting as an absolute URL redirect, and how can we stop precisely those situations, without having a hardcoded https?:// check?

[mrkishi]

I share your concerns when absolute URLs are involved, @Conduitry, but wouldn't other things break if we didn't have the correct origin/host settings?

As to why these paths cause issues: we normalize the pathname and send it back as a root-relative URL in the location header. That's fine for most pathnames, but if they start with a double-slash you end up with a scheme-relative URL instead: //path resolves to scheme://path instead of scheme://host//path.

[Conduitry]

Okay, yet more comment spam:

I think the (https?:)? part of the check is unnecessary. This is always going to be checking something that comes from normalize_path, which is always going to be something that begins with /.

I also think that preventing the redirect from happening here is unnecessarily prohibitive. I don't think we want to refuse to handle paths that happen to begin with two slashes (as weird as that might be). We should instead make sure we return a Location header that the browser won't misinterpret as a protocol-relative URL. The only way to do that might simply be to prepend the original origin, which brings us back to @mrkishi's suggestion. I don't know whether it would be better to avoid including the origin whenever possible, or to consistently include the origin, forcing everyone with weird setups to potentially worry about that for the sake of this weird case.

[Conduitry]

After hashing it out some, I'm in favor of @mrkishi 's suggestion #4414 (comment) above. We shouldn't refuse to properly handle these weird URLs with double leading slashes, and users should already have a trustworthy origin value on the server for other reasons. If they need to do something sneaky, that's what the externalFetch hook is for.

[Conduitry]

I've gone back and forth a few times on whether it would be better to always use the origin or only use it when the path begins with //. I think this limited change will, overall, be better.

It's less likely to break already-working apps. Apps that legitimately use paths beginning with // are probably pretty uncommon. And, if the origin setting is incorrect, getting redirected there when normalizing a // path seems pretty unlikely to cause confusing behavior for a real user - they would likely just be sent to a hostname that doesn't resolve.

[saostad]

This is the workaround I put in my project:

"@sveltejs/adapter-auto": "^2.0.0",
"@sveltejs/kit": "^1.15.2",
"svelte": "^3.58.0",
"svelte-check": "^3.2.0",
"svelte-preprocess": "^5.0.3",
"vite": "^4.2.1"

function validateUrl(url: string) {
		if (url.startsWith('http://') || url.startsWith('https://')) {
			return url;
		} else {
			return `http://${url}`;
		}
	}