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
Make sure automatic login feature does not result in multiple set-cookie headers. #13397
Conversation
@diosmosis things around session and authentication are starting to get a bit complicated... I presume there is no simpler solution to this? |
Simplest solution would be to do nothing, AFAIK browsers handle multiple set-cookie headers correctly. This is perhaps more of an issue for some kind of automation. Otherwise since PHP will always send multiple set-cookie headers in this case, only solution is to avoid regenerating the session ID before the session is started for the first time. |
Just to understand... what triggers the 2nd |
The 2nd set-cookie ONLY happens when there's no session (ie, no session cookie), and we call EDIT: when there's already a session (ie, there's a session ID in the cookie), session_start() doesn't need to issue a set-cookie call. So it's fine in that case. |
I've just had a look how symfony does it. Can maybe I'm tempted to simply send two cookies as browsers handle it fine. Unless there was actually a bug or problem. |
I think that would replace the cookie check? ie instead of checking for the cookie, just calling I guess we could do this: In SessionInitializer, do not regenerate ID if there is no session cookie. That would prevent two set-cookies (I think). Not sure it's a security issue, but since this all happens in one request, perhaps it's not a big deal. |
I haven't dealt with sessions in a while... from what I remember we should always call |
In this case there's no session ID in the first request, so the session created would be the first ID. If an attacker can get this, then I guess it should be regenerated, but if session fixation is only possible w/ a pre-existing unauthenticated session, then it should be fine in this case? |
I guess it really depends on the implementation but would need to think into it again. Basically yes if you create a session without regenerating an ID, there is possibly a security problem as the attacker would eg do From what I understand so far is, that we start the session because we don't know if we will use @matzesa was there a specific problem by having two set cookie headers? |
Yes & no. The logic on the login page is split over multiple requests. The first request shows the page and starts a session. Then the next request logs in, which regenerates the session ID. The problem is that logme() combines those steps, so it will start the session and then right after regenerate the ID, within the same request. Which results in two headers. |
We use an proxy Tool in our CMS to easily login user in Matomo. This stopped working with Matomo 3.6.0. I investigated the problem again and found, that not the doubled "Set-Cookie"-Header was the problem. It seems that Matomo missed some request header(s). I now copy all headers from the client request and it is working again. Sorry for the inconvenience. |
Cheers @matzesa |
Cause of duplicate headers is due to following workflow:
Fixed by introducing new hidden event
Session.shouldStartSession
. The Login plugin does not start a session for logme(). Then in SessionInitializer, instead of calling Session::regenerateId() (which will have no effect if the session is not started), we call Session::start() if the session hasn't been started yet, and Session::regenerateId() only if there is already an existing session.Fixes #13393