Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

redirect after invalidate creates unwanted record in history #11896

Open
konstantinov90 opened this issue Feb 24, 2024 · 0 comments · May be fixed by #11895
Open

redirect after invalidate creates unwanted record in history #11896

konstantinov90 opened this issue Feb 24, 2024 · 0 comments · May be fixed by #11895

Comments

@konstantinov90
Copy link

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

@benmccann benmccann linked a pull request Feb 25, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant