search

yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.23k stars 6.91k forks source link

Session not switching ID on login due to session instantiation order #18063

Open vswarte opened 4 years ago

vswarte commented 4 years ago

What steps will reproduce the problem?

Simply logging in. Our application will not generate a new session ID.

What is the expected result?

Ideally, the switchIdentity implementation would ensure that the session is opened for the request before attempting to change the session ID.

What do you get instead?

regenerateID is being called before the open method is called to open the session. This only happens in non-development environments as our debug tooling seems to start the session before control gets handed to the controller.

Additional info

https://github.com/yiisoft/yii2/blob/master/framework/web/User.php#L651-L656 When regenerateID is called, the session is still closed for the request causing it to not generate a new session ID. Do note that a session was created by previous requests, it is simply not set for this specific request yet. When it calls remove 2 lines below the session is then implicitly opened.

I'm unsure whether or not this is intended behavior, but the docblocks for the base implementation of regenerateID state the following.

     * This method has no effect when session is not [[getIsActive()|active]].
     * Make sure to call [[open()]] before calling it.

( https://github.com/yiisoft/yii2/blob/master/framework/web/Session.php#L288 ) So the omission of a corresponding open call in switchIdentity seems like an oversight to me.

Q A
Yii version 2.0.34
PHP version 7.3.16
Operating system Replicated on both Windows and (mystery) Linux prod environment
vswarte commented 4 years ago

Anything I can do to help this along? Were you planning on just adding the extra call open and some unit tests to assert it actually opens the session? If so I can def give it a shot!

samdark commented 4 years ago

It would definitely be very helpful to start with some unit tests. Ideally without a fix first to ensure they fail.

vswarte commented 4 years ago

https://github.com/vswarte/yii2/commit/1fcb77d574d82041afec4321754f83f7b36e692c

This test illustrates the issue, $before and $after are identical.

vswarte commented 4 years ago

I think there are two ways to go at this: 1) We can ensure the session is always started when calling regenerateId 2) A more local-to-the-issue fix would involve adding an open call to User's switchIdentity method.

Option 1 seems to me as the most logical as other calls eg. remove do implicitly open the session. Option 2 would require me to write a more involving test :-)

samdark commented 4 years ago
  1. That would change current documented behavior. That is backwards compatibility break so it's not desired to do it.
  2. I'd go this way.
vswarte commented 4 years ago

Been a busy weeks' end and weekend, will get another PR illustrating the issue from the authentication side-of-things down somewhere this week.

samdark commented 4 years ago

@vswarte I've moved it to next release. Hope you'll find some time to dig more into it.