[HttpFoundation] Make sessions secure and lazy #24523
Conversation
e240114 to
cd65297
Compare
|
wow, there's literally nothing on google about |
cd65297 to
090a3a7
Compare
| 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.
@Tobion do you agree with this statement?
6dcecf3 to
f900f1b
Compare
| <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.
for some obscure reason, the native SessionHandler doesn't implement SessionUpdateTimestampHandlerInterface
we have to proxy it to add strict mode and lazy writes...
aa5e67b to
aa03463
Compare
|
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 to
7a898e9
Compare
|
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
Outdated
Show resolved
Hide resolved
| 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.
Correct, on purpose: signaling the php -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 to
b75cc85
Compare
| @@ -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.
Looks like there is a missing counterpart of the CHANGELOG in UPGRADE-4.0.md
There was a problem hiding this comment.
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.
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(new StrictSessionHandler(new \SessionHandler())); | |||
There was a problem hiding this comment.
It feels strange that we need to make the native session handlers strict. The files 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.
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.
If php doesn't change this behavior, we can think about deprecating NativeFilesSessionHandler in SF 4.1. So by default it just sets the session.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.
This is duplicated code from function doWrite (line 144). Would it not be better to put this common code in a new protected method? May be something like createExpiryTime.
There was a problem hiding this comment.
not worth it for one line IMHO
ef7cdc2 to
347939c
Compare
|
comments 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 is https://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 4 Commits ------- 1f84b1f [Session] remove lazy_write polyfill for php < 7.0
* Make compatible with symfony/symfony#24523. * Open session in all readers/writers to fix error during session_regenerate_id. * Return boolean from close(), which Symfony expects.
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 is https://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.