Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HttpFoundation] Make sessions secure and lazy#24523
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
e240114 tocd65297Comparewow, there's literally nothing on google about |
cd65297 to090a3a7Compare| if (self::LOCK_TRANSACTIONAL === $this->lockMode && 'sqlite' !== $this->driver) { | ||
| if (\PHP_VERSION_ID < 70000 && self::LOCK_TRANSACTIONAL === $this->lockMode && 'sqlite' !== $this->driver) { | ||
| // Before PHP 7.0, session fixation was possible so locking could be needed even when creating a session. | ||
| // Starting with 7.0, secure random ids are generated so not concurrency is possible, thus this code path can be removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Tobion do you agree with this statement?
6dcecf3 tof900f1bCompare| <argument>%session.save_path%</argument> | ||
| <service id="session.handler.native_file" class="Symfony\Component\HttpFoundation\Session\Storage\Handler\SessionHandlerProxy"> | ||
| <argument type="service"> | ||
| <service class="Symfony\Component\HttpFoundation\Session\Storage\Handler\NativeFileSessionHandler"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
for some obscure reason, the nativeSessionHandler doesn't implementSessionUpdateTimestampHandlerInterface
we have to proxy it to add strict mode and lazy writes...
aa5e67b toaa03463Comparenicolas-grekas commentedOct 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
PR is green and ready. This is a significant improvement over the current session handlers, thanks to this new PHP 7.0 interface. This adds two main classes:
The rest are related tweaks. |
aa03463 to7a898e9CompareTerjeBr commentedOct 12, 2017
Have you tested this with a full symfony stack? When a typical symfony SessionBag is empty it is not the empty string, it contains some metadata. Because of this, this PR will not fix#6388 |
src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| 1 => array('file', '/dev/null', 'w'), | ||
| 2 => array('file', '/dev/null', 'w'), | ||
| ); | ||
| if (!self::$server = @proc_open('exec php -S localhost:8053', $spec, $pipes, __DIR__.'/Fixtures')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
exec won't work on windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Correct, on purpose: signaling thephp -S process to kill it is complex, so this is skipped on Windows as I don't want to import the Process component just for this.
7a898e9 tob75cc85Compare| @@ -4,6 +4,7 @@ CHANGELOG | |||
| 3.4.0 | |||
| ----- | |||
| * Session `use_strict_mode` is now enabled by default and the corresponding option has been deprecated, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
extra comma at the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like there is a missing counterpart of the CHANGELOG inUPGRADE-4.0.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice work@nicolas-grekas. Improving the session system was long overdue and I like how you solved it.
| } | ||
| $container->setAlias('session.handler', $handlerId)->setPrivate(true); | ||
| $options['lazy_write'] = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why is lazy write only enabled in this case but not for native session handlers? Seems arbitrary as we don't know more about a custom session handler than we do about the native one. So lazy_write should be safe to be enabled all the time.
| @@ -401,11 +408,13 @@ public function setSaveHandler($saveHandler = null) | |||
| if (!$saveHandler instanceof AbstractProxy && $saveHandler instanceof \SessionHandlerInterface) { | |||
| $saveHandler = new SessionHandlerProxy($saveHandler); | |||
| } elseif (!$saveHandler instanceof AbstractProxy) { | |||
| $saveHandler = new SessionHandlerProxy(new \SessionHandler()); | |||
| $saveHandler = new SessionHandlerProxy(newStrictSessionHandler(new\SessionHandler())); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It feels strange that we need to make the native session handlers strict. Thefiles session handler is already strict I assume. So this is only needed because if you use\SessionHandler, you lose that strictness? \SessionHandler does not implement\SessionUpdateTimestampHandlerInterface! So if we not not set a save handler at all it would even be better as it would then use the validate id implementation of the native session handler? But currently that is not possible do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Correct, I don't think we can do better here, id validation works with the wrapper so not a big issue. There is only updateTimestamp which ships no optimization but only writes. Not sure we can do better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If php doesn't change this behavior, we can think about deprecating NativeFilesSessionHandler in SF 4.1. So by default it just sets thesession.save_handler ini without actually overwriting \SessionHandler.
| public function read($sessionId) | ||
| public function updateTimestamp($sessionId, $data) | ||
| { | ||
| $expiry = $this->createDateTime(time() + (int) ini_get('session.gc_maxlifetime')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is duplicated code from functiondoWrite (line 144). Would it not be better to put this common code in a new protected method? May be something likecreateExpiryTime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not worth it for one line IMHO
ef7cdc2 to347939cComparecomments addressed |
Thank you@nicolas-grekas. |
…s-grekas)This PR was merged into the 3.4 branch.Discussion----------[HttpFoundation] Make sessions secure and lazy| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | not yet| Fixed tickets |#6388,#6036,#12375,#12325| License | MIT| Doc PR | -The `SessionUpdateTimestampHandlerInterface` (new to PHP 7.0) is mostly undocumented, and just not implemented anywhere. Yet, it's required to implement session fixation preventions and lazy write in userland session handlers (there ishttps://wiki.php.net/rfc/session-read_only-lazy_write which describes the behavior.)By implementing it, we would make Symfony session handling much better and stronger. Meanwhile, doing some cookie headers management, this also gives the opportunity to fix the "don't start if session is only read issue".So, here we are for the general idea. Now needs more (and green) tests, and review of course.Commits-------347939c [HttpFoundation] Make sessions secure and lazy
@nicolas-grekas what I proposed in#12325 is to not start the session at all until data is set when there is no session cookie in the request. I don't think that it implemented yet. This would avoid generating a session id, starting the session handler etc when you just read session data to then realize nothing is there. |
@Tobion from the HTTP pov, the observed behavior is exactly the same. In fact, starting the session has the benefit of sending the appropriate Cache-Control header. If HTTP cacheability is improved by the current change, it means we may not have to care anymore about whether the session is really started or not. |
If I just want to check if the user is logged in to then redirect or whatever, I don't need to start the session if there is no session cookie. And a cache control header might not be what I want. |
This PR was merged into the 4.0-dev branch.Discussion----------[Session] remove lazy_write polyfill for php < 7.0| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? |no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Remove the session.lazy_write fallback implementation for php < 7 introduced in#24523 as we don't need it in sf 4Commits-------1f84b1f [Session] remove lazy_write polyfill for php < 7.0
* Make compatible withsymfony/symfony#24523.* Open session in all readers/writers to fix error during session_regenerate_id.* Return boolean from close(), which Symfony expects.
Uh oh!
There was an error while loading.Please reload this page.
The
SessionUpdateTimestampHandlerInterface(new to PHP 7.0) is mostly undocumented, and just not implemented anywhere. Yet, it's required to implement session fixation preventions and lazy write in userland session handlers (there ishttps://wiki.php.net/rfc/session-read_only-lazy_write which describes the behavior.)By implementing it, we would make Symfony session handling much better and stronger. Meanwhile, doing some cookie headers management, this also gives the opportunity to fix the "don't start if session is only read issue".
So, here we are for the general idea. Now needs more (and green) tests, and review of course.