Skip to content

Respect ini session.use_strict_mode #18247

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.

Sign up for GitHub

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
Merged
merged 33 commits into from
Oct 31, 2020
Merged

Respect ini session.use_strict_mode #18247

merged 33 commits into from
Oct 31, 2020

Conversation

@rhertogh
Copy link
Contributor

@rhertogh rhertogh commented Aug 20, 2020 •
edited
Loading

Q A
Is bugfix? ❌
New feature? ✔️
Breaks BC? ✔️/❌ Only if Session::openSession() and/or Session::writeSession() are overwritten.
Tests pass? ✔️
Fixed issues #18061

Implements support for session.use_strict_mode by the Yii2 Session handler.
When a session is started with a non existing session id a new id will be generated. This imitates the behavior for the session_set_save_handler validate_sid param (which requires PHP >= 7.0).
The session will only be stored under the newly generated id.

To Do's to fully implement:

  • Test behavior of native storage: Yii2 implementation doesn't seem to interfere with native storage behavior.
  • Implement checks on all custom storage classes
  • Add tests
  • Add steps to upgrade.md

@samdark samdark added this to the 2.0.38 milestone Aug 20, 2020
@samdark
Copy link
Member

samdark commented Aug 20, 2020

👍

Test behavior of native storage
Implement checks on all custom storage classes
Add tests
Add steps to upgrade.md

Good plan.

@samdark samdark added the status:under development Someone is working on a pull request. label Aug 20, 2020
@rhertogh rhertogh changed the title Proof of Concept: Respect ini session.use_strict_mode Respect ini session.use_strict_mode Sep 5, 2020
@rhertogh
Copy link
Contributor Author

rhertogh commented Sep 5, 2020

@samdark If this PR is approved I'll also implement the useStrictMode for Redis and MongoDB. Those were the ones I could come up with, are there other extensions extending Session?

@rhertogh rhertogh marked this pull request as ready for review September 5, 2020 17:04
@bizley bizley added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Sep 6, 2020
@rhertogh
Copy link
Contributor Author

rhertogh commented Sep 6, 2020

The failing of the tests seem to have something to do with https://www.leaseweb.com/labs/2012/08/ps_files_cleanup_dir-permission-denied/
I've tried to implement a workaround (cd8c0e3) but with no success. Locally the tests run fine (Centos 7 PHP 7.3) any ideas how to fix this on the Github build environment?

@rhertogh
Another attempt to solve the "session_start(): ps_files_cleanup_dir: …
002c7e8 
…opendir(/var/lib/php5) failed: Permission denied (13)" error when staring a session when testing (also see https://www.leaseweb.com/labs/2012/08/ps_files_cleanup_dir-permission-denied/)
@rhertogh
Set sessin directory permissions in order to solve the "session_start…
8de2a59 
…(): ps_files_cleanup_dir: opendir(/var/lib/php5) failed: Permission denied (13)" error when staring a session when testing (also see https://www.leaseweb.com/labs/2012/08/ps_files_cleanup_dir-permission-denied/)
@rhertogh
Only set session dir permissions if a sessision.save_path is set. Add…
697e2d8 
…ed debug info
@rhertogh
Set session directory to github runner temp in order to solve the "se…
9093103 
…ssion_start(): ps_files_cleanup_dir: opendir(/var/lib/php5) failed: Permission denied (13)" error when staring a session when testing (also see https://www.leaseweb.com/labs/2012/08/ps_files_cleanup_dir-permission-denied/)
@rhertogh
Copy link
Contributor Author

rhertogh commented Sep 6, 2020 •
edited
Loading

Found the problem. In the Github action, phpunit runs under the 'runner' user (locally it's under the 'root' user).
Setting the session.save_path ini directive to the 'runner' temp directory solves the problem.

@samdark samdark requested a review from a team September 6, 2020 21:59
@rhertogh
Copy link
Contributor Author

rhertogh commented Sep 7, 2020

@samdark

Also it's not clear why PHP 5.4 fails tests. It may be not related but I'm not sure...

That's indeed something I also haven't figured out yet. It looks like the session table isn't correctly created during the setUp(). But why this would fail on 5.4 is still a mystery.

This was referenced Sep 7, 2020
Merged
Merged
@rhertogh
Copy link
Contributor Author

rhertogh commented Sep 7, 2020

@samdark

Implementations for redis and mongo are needed.

Redis (yiisoft/yii2-redis#212) and MongoDB (yiisoft/yii2-mongodb#319) pull requests are created. Their build fails at the moment because they require this unreleased Yii2 version.
My suggestion would be to merge this PR in master and tag it as 2.0.38-alpha. Another option would be to merge this PR to a dedicated branch in the Yii2 repository. In both cases those pull requests could temporarily depend on it.

@samdark samdark modified the milestones: 2.0.38, 2.0.39 Sep 11, 2020
@rhertogh
Implemented polyfill for Session::$useStrictMode for PHP < 5.5.2
574ac5c 
* Implemented polyfill for Session::$useStrictMode for PHP < 5.5.2
* Fixed tests for polyfill for Session::$useStrictMode for PHP < 5.5.2
* Added note that enabling `useStrictMode` on PHP < 5.5.2 is only supported with custom storage classes.
@rhertogh
Copy link
Contributor Author

rhertogh commented Sep 12, 2020

@samdark I've found the reason why the PHP 5.4 build failed, session.use-strict-mode is only available since PHP 5.5.2.
As a workaround I've implemented a polyfill to support the custom storage classes.

@rhertogh
Copy link
Contributor Author

rhertogh commented Sep 15, 2020

@samdark #18061 (comment)

code seems to be good, I need to think more about how to release it with least issues possible.

Perhaps I was to strict with my answer in the "Breaks BC?" question in the opening post. At the moment I don't see any way it would break existing installations. The only thing is that this option won't work if Session::openSession() and/or Session::writeSession() are overwritten.
So I'm not sure if it really counts as a BC change?

@samdark
Copy link
Member

samdark commented Oct 30, 2020 •
edited
Loading

So I'm not sure if it really counts as a BC change?

Likely it's fine. I've moved MongoDB to GitHub actions and is now ready to continue with the PR.

@rhertogh
Copy link
Contributor Author

rhertogh commented Oct 30, 2020

I've moved MongoDB to GitHub actions and is not ready to continue with the PR.

If you mean that the build fails? That's because it depends on this PR to be released first. Currently it depends on a Yii version that does not yet include the required changes in framework/web/Session.php

@samdark
Copy link
Member

samdark commented Oct 30, 2020

I've meant "now". That was an interesting typo :)

@samdark samdark merged commit ce088e0 into yiisoft:master Oct 31, 2020
@rhertogh rhertogh mentioned this pull request Nov 1, 2020
Merged
@rhertogh
Copy link
Contributor Author

rhertogh commented Nov 1, 2020

@samdark When validating the PR I encountered an error in the UPGRADE.md since there has been a new release since the original PR. I've created PR #18362 to fix it.

@samdark samdark mentioned this pull request Nov 10, 2020
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samdark samdark samdark approved these changes

@bizley bizley bizley approved these changes

Assignees

No one assigned

Labels

status:code review The pull request needs review.

Projects

None yet

Milestone

2.0.39

Development

Successfully merging this pull request may close these issues.

3 participants

@rhertogh @samdark @bizley