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

Switch to server-side sessions #1783

Merged
merged 7 commits into from Mar 10, 2021
Merged

Switch to server-side sessions #1783

merged 7 commits into from Mar 10, 2021

Conversation

nextgens
Copy link
Contributor

@nextgens nextgens commented Mar 9, 2021

What type of PR?

bug-fix

What does this PR do?

It simplifies session management.

  • it ensures that sessions will eventually expire (*)
  • it implements some mitigation against session-fixation attacks
  • it switches from client-side to server-side sessions (in Redis)

It doesn't prevent us from (re)-implementing a "remember_me" type of feature if that's considered useful by some.

@nextgens nextgens added the type/security Related to security label Mar 9, 2021
@mergify
Copy link
Contributor

mergify bot commented Mar 9, 2021

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors bot added a commit that referenced this pull request Mar 9, 2021
@nextgens nextgens requested review from lub and Diman0 March 9, 2021 13:48
@bors
Copy link
Contributor

bors bot commented Mar 9, 2021

try

Build failed:

lub
lub previously requested changes Mar 9, 2021
Copy link
Member

@lub lub left a comment

Choose a reason for hiding this comment

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

It's missing a newsfragment (to later generate the changelog), otherwise seems fine.

@lub
Copy link
Member

lub commented Mar 9, 2021

Should we backport this?

@mergify mergify bot dismissed lub’s stale review March 9, 2021 19:14

Pull request has been modified.

@nextgens
Copy link
Contributor Author

nextgens commented Mar 9, 2021

It's missing a newsfragment (to later generate the changelog), otherwise seems fine.

I'm not sure how to phrase it... that's why I've left it out. Feel free to improve upon it :)
We may want to mention the change to session lifetime... but I'm not sure how the towncrier output will look like

@nextgens
Copy link
Contributor Author

nextgens commented Mar 9, 2021

Should we backport this?

I'd say yes... but we may want to wait until it gets some additional field testing.

@nextgens nextgens added priority/p1 (Critical) bug with workaround / Should have type/backport Automatic backport this PR to the current stable release type/bug Bug. Not working as intended labels Mar 10, 2021
Copy link
Member

@Diman0 Diman0 left a comment

Choose a reason for hiding this comment

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

It looks good to me. It is nice we can prevent replay attacks with this. This will make mailu more secure.
If this functionality does not work properly, we must not forget to revert the backport.
Fortunately test.mailu.io will be automatically updated later today. So we can already test via test.mailu.io.
However we should also test mailu 1.8.

@mergify
Copy link
Contributor

mergify bot commented Mar 10, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 10, 2021

Build succeeded:

@bors bors bot merged commit 25e8910 into Mailu:master Mar 10, 2021
@Diman0 Diman0 mentioned this pull request Aug 7, 2021
2 tasks
@nextgens nextgens deleted the session-ss branch October 31, 2021 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/p1 (Critical) bug with workaround / Should have type/backport Automatic backport this PR to the current stable release type/bug Bug. Not working as intended type/security Related to security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants