redirect after invalidate creates unwanted record in history

[konstantinov90]

Describe the bug

Hi!

There is a cryptic bug in svelte kit routing, which goes as follows

Say you have an app behind authorization. Whenever an unauthorized user tries to access the app, he is redirected to the login page. And likewise, whenever authorized user tries to access login page (for example from browser history, or with redirect param) he is forcefully redirected to the main page or another page according to redirect param. This logic should work both on client and server regardless

Such behaviour could be implemented in many different ways, but I find it natural to perform redirects via load functions and invalidation on the client, so that all the redirection rules are stored in one place (load functions)

Having an app with such routes structure, lets define scenarios, starting with welcome screen and describing expected browser history

• / (welcome screen, no auth)
• /signin (login page)
• /main (behind auth)

Authorized client
-> / (link to /main) -> /main --- history (1 /, 2 /main)
-> / (link to /signin?redirect=/main) -> /main --- history (1 /, 2 /main)
-> /signin?redirect=/main -> /main --- history (1 /main) (server side redirect with 302)
-> /main --- history (1 /main)

Unauthorized client
-> / (link to /signin?redirect=/main) -> /signin?redirect=/main --- history (1 /, 2 /signin)
-> / (link to /signin?redirect=/main) -> /signin?redirect=/main -> (client inputs password) -> /main --- history (1 /, 2 /main) STATE REPLACED BUG HERE
-> / (link to /main) -> /signin?redirect=/main --- history (1 /, 2 /signin)
-> / (link to /main) -> /signin?redirect=/main -> (client inputs password) -> /main --- history (1 /, 2 /main) STATE REPLACED BUG HERE

Thus a rule is described - authorized client should not have /signin page in his history, because this page is inaccessible at all when authorized but it redirects to /main page instead. If this rule is broken and /signin page is present inside browser history, should a client click "go back" button in his browser he becomes stuck in redirect loop - /main -> /signin -> /main

Returning finally to the bug at hand - in described circumstances such a redirect loop indeed does emerge, in the place which is marked with BUG HERE - route state is not replaced, when navigated via invalidate

This bug is kind of hard to describe in text, so please consider trying a repro

PS

Reproduction

I made a branch with a solution to this bug

#11895

reproduction scenario can be found in the basics test app (/redirect/app-with-auth)

please let me know, if I should provide any other form of reproduction

Logs

No response

System Info

System:
    OS: macOS 14.2.1
    CPU: (8) arm64 Apple M1
    Memory: 243.41 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.0 - /usr/local/bin/node
    Yarn: 1.22.17 - /opt/homebrew/bin/yarn
    npm: 8.4.1 - /opt/homebrew/bin/npm
    pnpm: 8.15.3 - /opt/homebrew/bin/pnpm
    bun: 1.0.0 - ~/.bun/bin/bun
  Browsers:
    Brave Browser: 122.1.63.161
    Chrome: 113.0.5672.92
    Safari: 17.2.1

Severity

serious, but I can work around it

Additional Information

No response

[teemingc]

I can't reproduce the issue based on the test from the PR. Please provide a minimal reproduction separately.

[HalfdanJ]

I just hit this same issue of redirects creating an infinite loop of redirects for a login page, specifically because we have SSR disabled, so the redirect doesnt cause a true 307 or 302 response, but is instead redirected clientside.

I think the issue here is that a true server side redirect would not be recorded in the history, so sveltekit should imitate the same behavior when doing client side redirects based on data from +page.server.ts

[konstantinov90]

so sveltekit should imitate the same behavior when doing client side redirects

That is exactly what I had in mind! Unfortunately I haven't had the opportunity to get back to this issue at the time, maybe you could help with the repro @eltigerchino

[konstantinov90]

Oops, I meant to tag @HalfdanJ

[teemingc]

-> / (link to /signin?redirect=/main) -> /signin?redirect=/main -> (client inputs password) -> /main --- history (1 /, 2 /main) STATE REPLACED BUG HERE

Shouldn't this have three history entries (one for each page)? And I think it does at the moment.

I'm not sure the test in the provided PR reproduces the issue correctly. In the PR, logging in, then navigating backwards redirects the user to the main page, which is correct, since the login page has logic to redirect to the main page if the user is already logged in. https://github.com/sveltejs/kit/pull/11895/files#diff-59777b7031f8495434e87f674619c4ea3ab3752fa8093e616a42dedd9a9dd705R3-R7

@konstantinov90 We need a proper minimal reproduction and with expected results compared to actual results to troubleshoot this.

[konstantinov90]

@eltigerchino in the PR it works correctly because I fixed it, you should run the test on the main branch, then you would see the redirect loop

[konstantinov90]

-> / (link to /signin?redirect=/main) -> /signin?redirect=/main -> (client inputs password) -> /main --- history (1 /, 2 /main) STATE REPLACED BUG HERE

Shouldn't this have three history entries (one for each page)? And I think it does at the moment.

No, it does not in my opinion, because the "go back button" makes you stuck between main and signin, which is not the case with the server redirects

[teemingc]

@eltigerchino in the PR it works correctly because I fixed it, you should run the test on the main branch, then you would see the redirect loop

I've run the test on the main branch and it has behaved as I have described earlier:

In the PR, logging in, then navigating backwards redirects the user to the main page, which is correct, since the login page has logic to redirect to the main page if the user is already logged in. #11895 (files)

With the fix in the PR, the login page is skipped and goes back to the pre-login page, as if there was no history entry for the login page (although we visited it and logged in).

[konstantinov90]

Ok, I will have to rework this PR

I suggest we close it for now, seeing that this PR has got minimal attention from community