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.

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

Fix CSRF on logout #1040

Merged
merged 6 commits into from Jul 14, 2022
Merged

Fix CSRF on logout #1040

merged 6 commits into from Jul 14, 2022

Conversation

Glandos
Copy link
Member

@Glandos Glandos commented Jul 12, 2022

Superseed #1028

@Glandos
Add logout as POST action
498ceec 
for now, there is nothing when /exit is called manually
@Glandos
adapt tests
5e1a47b
@Glandos
based on test failure, change the init of confirm_logout_form.
b49ead7 
It should be moved in its own preprocessor OR rename the current one
@Glandos Glandos mentioned this pull request Jul 12, 2022
Closed
@Glandos Glandos requested review from almet and zorun July 12, 2022 11:12
@zorun
Copy link
Collaborator

zorun commented Jul 12, 2022

Thanks, the new approach looks good!

Just to clarify, in the end you didn't implement a confirmation dialog? That's totally fine for me, I think logging out needs no confirmation. But you should maybe remove "confirm" in the name of the form (ConfirmLogoutForm) to make that clear.

almet
almet approved these changes Jul 12, 2022
View reviewed changes
Copy link
Member

@almet almet left a comment

Choose a reason for hiding this comment

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

That looks good to me! Thanks for doing this maintenance work.

ihatemoney/tests/budget_test.py Outdated Show resolved Hide resolved
ihatemoney/web.py Show resolved Hide resolved
@Glandos
Copy link
Member Author

Glandos commented Jul 12, 2022

Thanks, the new approach looks good!

Just to clarify, in the end you didn't implement a confirmation dialog? That's totally fine for me, I think logging out needs no confirmation. But you should maybe remove "confirm" in the name of the form (ConfirmLogoutForm) to make that clear.

You're right: no confirmation, since this is far easier to code, and the UX is smoother. And you're right, I let the old name, it's meaningless now, so I renamed it.

zorun
zorun approved these changes Jul 12, 2022
View reviewed changes
Copy link
Collaborator

@zorun zorun left a comment

Choose a reason for hiding this comment

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

Everything good for me, thanks!

@Glandos Glandos merged commit 31fef4f into spiral-project:master Jul 14, 2022
@Glandos Glandos deleted the exit_with_post branch July 14, 2022 13:45
TomRoussel pushed a commit to TomRoussel/ihatemoney that referenced this pull request Mar 2, 2024
@Glandos
Fix CSRF on logout (spiral-project#1040)
f02f8a8 
fix for https://huntr.dev/bounties/a3045614-1125-4901-bb7a-9d51be4beeed/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@almet almet almet approved these changes

@zorun zorun zorun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Glandos @zorun @almet