fix: solve issues with basic auth username:password in URLs

[LorenzoLeonardini]

When using basic auth and embedding credentials in the URL (user:password@host) exceptions are raised in Chrome when calling history.replaceState with location.href as the URL. This is better documented in #10522

The issue is also reported in the Chromium bug tracker https://bugs.chromium.org/p/chromium/issues/detail?id=675884 but considering the lack of updates it seems like fixing this issue in the browser is not a priority.

I propose to fix this issue with the workaround suggested on the Chromium thread, by replacing location.href with location.pathname + location.search + location.hash.

[changeset-bot]

🦋 Changeset detected

Latest commit: c21b675

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

[LorenzoLeonardini]

Just converted this to a draft as some more manual testing revealed I just moved the problem from one exception to another. Now exceptions get thrown when prefetching pages and possibly in other places.
I will keep working on this, as this issue is quite important to me, but if no one has anything against it, I will keep the issue open in case someone else wants to share their input on this. It's my first contribution to SvelteKit, so someone might have clearer ideas than me

[LorenzoLeonardini]

My first commit solved the issues with the Chromium bug, but now reveals deeper problems: fetch, by design, does not support credentials inside URLs. As the client code tries to prefetch pages, exceptions are raised.

This means that to support basic auth with credentials embedded in the URL, it's necessary to add some logic that wraps around fetch and checks whether the URL contains some credentials, to then remove them. As far as I've seen, the best place to do this is as a wrapper around native_fetch, something like

export const native_fetch = (input, init) => {
    if (input instanceof URL || typeof input === 'string') {
        input = new URL(input);
        if (input.username || input.password) {
            input.username = ''
            input.password = ''
        }
    }
    return window.fetch(input, init)
}

This brings up a couple of discussion points:

• In my opinion, there's no reason why SvelteKit shouldn't support basic auth and URL credentials, but I understand someone might have different ideas on this
• The name native_fetch hints to being just an alias of window.fetch, making it behave differently is a poor naming choice. However, making this change here seems the best option in order to avoid code duplication every time native_fetch is used

[LorenzoLeonardini]

After some more testing, I have found an alternative fix.

If we create an anchor element and set the href attribute to /, when we read it the browser gives us the full URL, including credentials. So, we can replace my previous commit ce2420d with

// create initial history entry, so we can return here
try {
    history.replaceState(
        { ...history.state, [INDEX_KEY]: current_history_index },
        '',
        location.href
    );
} catch (error) {
    const a = document.createElement('a');
    a.href = '/';
    const url = new URL(a.href);
    if (!(url.username || url.password)) {
        throw error;
    }
    location.href = location.href;
}

In case of an exception caused by the presence of credentials in the current URL, this reloads the page, using location.href, which does not include credentials, solving every problem. The browser will take care of the authorization header automatically.

This solution feels less elegant, but it seems to me the most reliable and the less disruptive

[LorenzoLeonardini]

Even better, we can directly use document.URL that, unlike location.href, does include credentials

// create initial history entry, so we can return here
try {
    history.replaceState(
        { ...history.state, [INDEX_KEY]: current_history_index },
        '',
        location.href
    );
} catch (error) {
    const url = new URL(document.URL);
    if (!(url.username || url.password)) {
        throw error;
    }
    location.href = location.href;
}

[Rich-Harris]

Thanks for diagnosing this — I had no idea this Chromium bug existed.

It turns out that we can solve the first part very simply, just by omitting the third argument to replaceState. We should probably be doing that in the first place, since it's less code.

That does leave the fetch issue unsolved. We shouldn't use try-catch to detect the Chromium bug, since fetch apparently resolves against document.URL in non-Chrome browsers too — if we want to make it work as expected then I see two possible options:

1. always reload the page when username/password is in the URL. (it might be sufficient to compare document.URL against location.href?) this does feel a tiny bit heavy-handed, and could possibly have unintended consequences? I can't quite decide if I think this is in SvelteKit's remit or not, I can see both arguments
2. ensure that SvelteKit-initiated fetch requests resolve against location.href rather than document.URL, but otherwise leave things unchanged. it would then be up to the developer to ensure that a) any other fetch requests are also correctly resolved or b) they reload the page on startup if necessary

Option 1 is definitely easier and more comprehensive, if we're okay with forcibly reloading the page

[LorenzoLeonardini]

Option one is in practice equivalent to my proposed solution, simplified even more. I don't know why I decided to test for exceptions instead of directly comparing document.URL and location.href, that makes definitely more sense. Given the fact that the browser "caches" the credentials and sends them in each subsequent request to the host, I can't seem to think of any unintended consequences to doing this, although I do think it's a weird behavior to have the browser refresh unexpectedly (but it's just on the first load, so maybe it's ok?).

On one hand, the second option seems like the most proper and official way to solve the issue, on the other hand, it adds complexity and may cause a lot of confusion when fetch requests initiated in the load function work and fetch requests initiated inside event handlers (as an example) don't.

[Rich-Harris]

looks like you'll need to pnpm install to get the latest version of Prettier before running pnpm format (annoying, I know)

[Rich-Harris]

we should remove the try-catch, and add the URL check (document.URL !== location.href should work, I think) at the top of start

Suggested change

	try {
	original_replace_state.call(
	history,
	{
	...history.state,
	[HISTORY_INDEX]: current_history_index,
	[NAVIGATION_INDEX]: current_navigation_index
	},
	''
	);
	} catch (error) {
	// detect if the issue has been created by basic auth credentials in the current URL
	// https://github.com/sveltejs/kit/issues/10522
	// if so, refresh the page without credentials
	const url = new URL(document.URL);
	if (!(url.username || url.password)) {
	throw error;
	}
	location.href = `${location.href}`;
	}
	original_replace_state.call(
	history,
	{
	...history.state,
	[HISTORY_INDEX]: current_history_index,
	[NAVIGATION_INDEX]: current_navigation_index
	},
	''
	);

[LorenzoLeonardini]

Should we keep the (url.username || url.password) check, just in case we are missing another reason why location.href and document.URL? I really can't think of any other scenario, but I have to admit my knowledge of the spec is limited. It would result in something like

if (document.URL !== location.href) {
    const url = new URL(document.URL);
    if (url.username || url.password) {
        location.href = `${location.href}`;
    }
}

[Rich-Harris]

My inclination in these cases is to do the simpler thing that we're fairly sure is correct, rather than the more defensive thing. If it turns out there are other cases where document.URL and location.href differ, we'll find out about them soon enough, but if the defensive code is unnecessary we'll never find out — we'll just ship extra code to every SvelteKit app user for no good reason

[LorenzoLeonardini]

Makes sense