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

Prevent misleading status pages from being shown #1580

Open
Masterjun3 opened this issue Jun 25, 2023 · 3 comments
Open

Prevent misleading status pages from being shown #1580

Masterjun3 opened this issue Jun 25, 2023 · 3 comments

Comments

@Masterjun3
Copy link
Collaborator

Masterjun3 commented Jun 25, 2023

Quick example: Currently we use the default settings, so that if you try to log in but enter your password incorrectly 5 times, you're locked out for 5 minutes. You get redirected to this page:
https://tasvideos.org/Account/Lockout
If you then wait some time and click "Log in" on the top right, you will go to this page:
https://tasvideos.org/Account/Login?returnUrl=%2FAccount%2FLockout
Notice the returnUrl!

A successful login will redirect you to the Lockout page, even though you logged in.

Something similar happens if you try to access a page while logged out, you're redirected to the AccessDenied page. Then you log in and you're redirected to the AccessDenied page yet again, even if you now have the permission to the previous page. (The information about the previous page is even completely lost now.)

@Masterjun3
Copy link
Collaborator Author

Maybe the two problems can be solved in different ways:

  • For the problem with the wrong Lockout being shown, maybe these pages should redirect to the Homepage or something when a logged-in user visits them. All Account pages should be checked for similar situations.
  • For the AccessDenied page, maybe it'd be worth it to pass a return url to the AccessDenied page itself, and then if logged out, show the login form there (or show a link that redirects to the Login page with a return url to the original url attempted to be accessed). This however wouldn't solve the problem when clicking the Login button on the top, it would still redirect to AccessDenied...

@nattthebear
Copy link
Collaborator

These probably shouldn't be redirect -> 200 at all. If you fail to log in too many times in a row, we should return a 4xx from the login POST itself that says you're locked out.

Similarly, if you can't access a specific page, we should return a 4xx for the GET to that URL, with no redirection involved, that shows the access denied message.

This was how it worked in days of yore, and the correct status codes plus no extraneous redirects matches what browsers are expecting to see when it comes to things like network cache, bfcache, and reloads, so the browser will give the right behavior.

@adelikat
Copy link
Collaborator

a quick fix would be to null out the returnUrl before directing them to problematic pages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants