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

Jump to bottom

Detect expired session use #14502

Merged
merged 8 commits into from Jun 17, 2019
Merged

Detect expired session use #14502

merged 8 commits into from Jun 17, 2019

Conversation

diosmosis
Copy link
Member

No description provided.

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels May 29, 2019
@diosmosis diosmosis added this to the 3.10.0 milestone May 29, 2019
mattab
mattab reviewed May 31, 2019
View reviewed changes
config/global.ini.php Outdated
@@ -397,6 +397,9 @@
; Sets the session cookie path
login_cookie_path =

; the amount of time before a session that was created without the "remember me" option is considered expired
Copy link
Member

Choose a reason for hiding this comment

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

Is this from the time the session was created or rather from the last server side requets this user made?

Copy link
Member Author

Choose a reason for hiding this comment

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

the time the last server side request the user made, will update the docs

@mattab
Copy link
Member

mattab commented May 31, 2019

Feedback

  • we could change the session idle timeout to 1 hour or maybe even 30min?

according to https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Session_Management_Cheat_Sheet.md#session-expiration

Both the idle and absolute timeout values are highly dependent on how critical the web application and its data are. Common idle timeouts ranges are 2-5 minutes for high-value applications and 15-30 minutes for low risk applications.
When a session expires, the web application must take active actions to invalidate the session on both sides, client and server. The latter is the most relevant and mandatory from a security perspective.

@diosmosis
Copy link
Member Author

we could change the session idle timeout to 1 hour or maybe even 30min?

Sure, I'll change the default config value.

@diosmosis
Copy link
Member Author

Updated. Feel free to change the default config value.

@mattab
Copy link
Member

mattab commented Jun 5, 2019

@diosmosis Looking at https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Session_Management_Cheat_Sheet.md#session-expiration
I'm also wondering, do we already have Absolute Timeout implemented server side?

@diosmosis
Copy link
Member Author

I don't think we want both, otherwise some will complain that the session shouldn't just end while they're using it.

@tsteur
Copy link
Member

tsteur commented Jun 7, 2019

Maybe the absolute session timeout could be set to something like 3 months?

@tsteur
Copy link
Member

tsteur commented Jun 7, 2019

Would then happen max 4 times a year. Any shorter absolute value be annoying indeed.

@diosmosis
Copy link
Member Author

This PR implements an idle timeout, not an absolute one (the saved start time of the session updated on each request).

@mattab mattab requested a review from tsteur June 10, 2019 09:47
@mattab
Copy link
Member

mattab commented Jun 10, 2019

@tsteur could you please review this and merge?

tsteur
tsteur reviewed Jun 11, 2019
View reviewed changes
core/Date.php Show resolved Hide resolved
@tsteur
Copy link
Member

tsteur commented Jun 11, 2019

Not sure what happens, I set login_session_not_remembered_idle_timeout = 30 and I log in, then it appears like I was logged in successfully but am still anonymous and then get also these warnings:

core/Session/SessionAuth.php(209): Warning - A non-numeric value encountered - Matomo 3.10.0-b3 - Please report this message in the Matomo forums: https://forum.matomo.org (please do a search first as it might have been reported already)

There is no login failure, I get forwarded to dashboard but am not logged in.

@tsteur
Copy link
Member

tsteur commented Jun 14, 2019

Works for me 👍 should be good to merge.

@diosmosis diosmosis merged commit 824204a into 3.x-dev Jun 17, 2019
@diosmosis diosmosis deleted the session-expire branch June 17, 2019 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsteur tsteur tsteur left review comments

@mattab mattab mattab left review comments

Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
3.10.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@diosmosis @mattab @tsteur