Skip to content

Fix login loop if login CSRF fails and user is not logged in #35419

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 18, 2023
Merged

Fix login loop if login CSRF fails and user is not logged in #35419

merged 1 commit into from
Jan 18, 2023

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Nov 25, 2022 •
edited
Loading

Resolves: #33919

Summary

If CSRF fails but the user is logged in that they probably logged in in another tab. This is fine. We can just redirect.
If CSRF fails and the user is also not logged in then something is fishy. E.g. because Nextcloud contantly regenrates the session and the CSRF token and the user is stuck in an endless login loop.

TODO

  • Apply changes
  • Fix tests
  • Verify this patch in production

Checklist

@ChristophWurst ChristophWurst self-assigned this Nov 25, 2022
@ChristophWurst ChristophWurst added the 2. developing Work in progress label Nov 25, 2022
@ChristophWurst ChristophWurst mentioned this pull request Nov 25, 2022
Closed
9 tasks
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 25, 2022
View reviewed changes
@phonon112358
Copy link

I can confirm that this PR solves the issue #33919 ... It redirects only once , then I was able to login again . Hence, it stops the infinite loop.

@szaimen
Copy link
Contributor

szaimen commented Nov 25, 2022

/backport to stable25

@szaimen szaimen added this to the Nextcloud 26 milestone Nov 25, 2022
szaimen
szaimen approved these changes Nov 25, 2022
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and community confirmed that it fixes the issue

@szaimen
Copy link
Contributor

szaimen commented Nov 25, 2022

However @ChristophWurst nodb tests are failing: https://drone.nextcloud.com/nextcloud/server/26527/9/4

@phonon112358
Copy link

I only tested it with php8.1 .... the failed tests seems to be with php7.4 if I see it correctly.

@ChristophWurst
Copy link
Member Author

Tests\Core\Controller\LoginControllerTest has failing tests indeed. I should also add a case or two that covers this new logic.

@szaimen
Copy link
Contributor

szaimen commented Nov 28, 2022 •
edited
Loading

@ChristophWurst I know a way to reproduce the issue:

  1. Create a new instance using:
docker run -it \
--name nextcloud-easy-test \
-p 127.0.0.1:8443:443 \
-e SERVER_BRANCH=stable25 \
--volume="nextcloud_easy_test_npm_cache_volume:/var/www/.npm" \
ghcr.io/szaimen/nextcloud-easy-test:latest
  1. Log in with the user (and leave the tab open)
  2. delete the instance with CTRL+C and docker rm nextcloud-easy-test
  3. Create a new instance using the exact same command of step 1
  4. Try to log in again in the same tab -> login loop

However with this patch I can confirm it makes it work (you get redirected one time and can log in afterwards again) :)

szaimen
szaimen approved these changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested and works

@markusTraber
Copy link

Tested it as well. Had to log in twice, but no infinite loop. Also had only one tab open.

@kesselb kesselb mentioned this pull request Dec 15, 2022
Closed
9 tasks
@mathisdt
Copy link

Today this problem also struck me like described in #33919 but manually applying this patch solved the issue. So I'd vote for merging. 👍

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 16, 2023
@ChristophWurst ChristophWurst marked this pull request as ready for review January 16, 2023 09:38
@ChristophWurst
Copy link
Member Author

Tests adjusted

@szaimen
Copy link
Contributor

szaimen commented Jan 16, 2023

Thanks @ChristophWurst ! :)

Could you maybe also fix https://github.com/nextcloud/server/actions/runs/3929063051/jobs/6717407444? Otherwise I could take care of rebasing :)

@szaimen szaimen mentioned this pull request Jan 18, 2023
Closed
@ChristophWurst
Fix login loop if login CSRF fails and user is not logged in
f22101d 
If CSRF fails but the user is logged in that they probably logged in in
another tab. This is fine. We can just redirect.
If CSRF fails and the user is also not logged in then something is
fishy. E.g. because Nextcloud contantly regenrates the session and the
CSRF token and the user is stuck in an endless login loop.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the fix/login-csrf-not-logged-in-clear-cookies branch from 2f3ff02 to f22101d Compare January 18, 2023 08:39
@szaimen szaimen requested review from a team and PhrozenByte January 18, 2023 08:42
@szaimen szaimen requested review from ArtificialOwl, blizzz and icewind1991 and removed request for a team January 18, 2023 08:42
@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 18, 2023
@ChristophWurst ChristophWurst merged commit 9e08e49 into master Jan 18, 2023
@ChristophWurst ChristophWurst deleted the fix/login-csrf-not-logged-in-clear-cookies branch January 18, 2023 10:55
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 18, 2023
Merged
@blizzz
Copy link
Member

blizzz commented Feb 8, 2023

/backport to stable24

@blizzz
Copy link
Member

blizzz commented Feb 8, 2023

/backport to stable23

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

This was referenced Feb 16, 2023
Merged
Merged
@Yetangitu Yetangitu mentioned this pull request Apr 6, 2023
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artonge artonge artonge approved these changes

@szaimen szaimen szaimen approved these changes

@PhrozenByte PhrozenByte Awaiting requested review from PhrozenByte

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner automatically assigned from nextcloud/server-backend

@blizzz blizzz Awaiting requested review from blizzz blizzz is a code owner automatically assigned from nextcloud/server-backend

+1 more reviewer

@kiruto kiruto kiruto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ChristophWurst ChristophWurst

Labels

4. to release Ready to be released and/or waiting for tests to finish bug feature: authentication

Projects

None yet

Milestone

Nextcloud 26

Development

Successfully merging this pull request may close these issues.

[Bug]: Tried to log in "username" but could not verify token

10 participants

@ChristophWurst @phonon112358 @szaimen @markusTraber @mathisdt @blizzz @kiruto @artonge @skjnldsv