Fix for session_start() which doesn't check if the session has already started #5248

Closed
wants to merge 1 commit into
from

Projects

None yet
@sc0ttkclark

session_start() runs in the current logic for Native Sessions here without checking first if the session has already started previous to the code running.

@sc0ttkclark sc0ttkclark Update src/Symfony/Component/HttpFoundation/Session/Storage/NativeSes…
…sionStorage.php

Fix for session_start() which doesn't check if the session has already started
09059f1
@travisbot

This pull request passes (merged 09059f1 into 7bc6d6d).

@fabpot
Member
fabpot commented Aug 13, 2012

ping @drak

@ghost
ghost commented Aug 13, 2012

You cannot assume that a non empty ID means the session has not started. The session ID can be set before starting a session.

What problem are you trying to solve? I can give some better advice if I understand what issue you are experiencing.

@sc0ttkclark

The patch resolves an issue where start_session() is called even if the session has already been started. This causes a PHP notice and can be seen on certain hosting environments.

@ghost
ghost commented Aug 13, 2012

can you show an example? Sessions are not supposed to be started outside of Symfony.
There is only a way to detect if a session has really started starting from PHP 5.4 using http://uk3.php.net/manual/en/function.session-status.php but I deliberately left this out because it would mean different behaviour depending on version. Sessions are not supposed to be started outside of Symfony's Session class as already documented.

This for example, is illegal under Symfony and the resulting PHP notice is correct. Starting the session outside of Symfony can result of other knock on effects (like a session starting with a different save handler than you expect) - therefor, the PHP notice is very welcome to show that something wrong has happened.

session_start();
$session = new Symfony\Component\HttpFoundation\Session\Session();
$session->start();
@sc0ttkclark

If this is the expected behavior, that's ok. I have a project that sits ontop of WordPress, and a user has integrated their WordPress environment with their Symfony environment. WordPress loads first and then Symfony, which I'm guessing should then be flipped and this pull request can safely be closed as wontfix.

@mvrhov
Contributor
mvrhov commented Aug 14, 2012

Having sf embedded inside another app is not that farfetched, so IMHO this should be supported.

@fabpot
Member
fabpot commented Aug 14, 2012

I also agree that it should be supported.

@fabpot fabpot reopened this Aug 14, 2012
@travisbot

This pull request fails (merged 09059f1 into 7bc6d6d).

@ghost
ghost commented Aug 14, 2012

If you are embedding Symfony into Wordpress, the you have to let Wordpress use it's own session management.

@lsmith77
Contributor

Yeah, I agree we should support this, but I guess in that case we should have a more explicit mechanism to delegate session starting.

@fabpot
Member
fabpot commented Aug 14, 2012

@drak The point is that we need to allow this kind of integration (that's part of the interoperability we try to promote). So, even if the session was started by Wordpress, it should be usable within Symfony. We must allow the session to be shared between the two libraries.

@ghost
ghost commented Aug 14, 2012

@fabpot, are you willing to accept "fingers crossed" hacks in Symfony? It's going to mean people have to be deeply aware of the moving parts in order to avoid a collision - I would suggest if we look deeper here, there is probably another way of doing whatever the reporter is trying to do. For one, if using HttpFoundation, there should be no reason for the SF2 session code to be hit at all. If someone is running Symfony2 fullstack framework inside wordpress, it's seems possibly the wrong approach to solve the problem.

@sc0ttkclark can you provide some more details of what you are doing?

@fabpot
Member
fabpot commented Aug 14, 2012

@drak: Don't get me wrong. I'm not suggesting to merge this PR. I'm just saying that it should be able to share the session between different PHP projects into one project. Note that I've not thought about that more than that, so I don't have all the implications in mind. That being said, in symfony1, it was definitely possible to do that kind of stuff.

@lsmith77
Contributor

don't we support the necessary config options for people to swap out the session handling? IIRC this is what we ended up doing for the Magento integration Bundle:
https://github.com/liip/LiipMagentoBundle/blob/master/SessionStorage/MagentoSessionStorage.php

@mevers47
Contributor

With a simple experiment I noticed that $_SESSION is null before session_start is called. Can we rely on this?

@lsmith77
Contributor

well to me the issue is that if Symfony2 is embedded into app X .. it means app X will execute code before and after. as a result if app X didnt start a session before Symfony2 was called, there is no guarantee that it will not want to start one afterwards. so imho session starting needs to be handled by only one app and the other simply has to hook into the other one.

@ghost
ghost commented Aug 15, 2012

@mevers47 - The point is, this particular concept here, while solving a problem one way, introduces other more serious problems elsewhere. I do not believe this particular approach is the way to solve the problem being experienced by @sc0ttkclark, but we really need to see his use-case first to know how best to approach it.

@sc0ttkclark

Thanks for the discussion, I'm glad this ended up being something you were interested in supporting.

If Symfony is pulled into the page on-top of WordPress (through a plugin or theme), there's a good chance that a session has already started. In certain circumstances, using session_start() can conflict, either with PHP notices, or very rare occurrences could actually change the existing session ID used previously, or reset the session data. I swear I've seen this happen with at least one user, but I didn't document my findings as well as I should have.. so feel free to ignore that.

If there can be some sort of method to check for an existing session, that would improve the ill effects caused by loading Symfony's session handling on-top of other projects.

@dlsniper
Contributor

@sc0ttkclark maybe it's a dumb idea but if the session is started maybe you could do a count($_SESSION) != 0 ?

@ghost
ghost commented Aug 15, 2012

@sc0ttkclark - I am not saying I support it, just that I am interested in finding a solution for you that is in the best interest of the integrity of the code.

Can you please tell me (off line if you like drak at zikula dot org) how you are integrating Sf2 into wordpress - until I see the implementation detail I can't help with any certainty.

@sc0ttkclark

That's where I can't assist, it wasn't I setting up the integration, it was someone else. I should have followed up with them and asked them to chime in on this thread. I don't know them personally, they were using my plugin for WordPress and I was offering support to them to help figure out what was going on when I found the issue.

@Fonsini
Fonsini commented Sep 28, 2012

I'm having what i think it is a similar problem:

  • In Symfony 2.0 i successful configured External Authentication using simplesamlphp (http://simplesamlphp.org/):
    • Configured a simple custom User Provider and a custom Authentication Provider that calls simplesamlphp functions.
    • Everything worked fine in Symfony 2.0 and the user was being authenticated using simplesamlphp;
  • I migrated everything to Symfony 2,1 but the external authentication stopped working:
    • I concluded that simplesamlphp is calling at least session_start() and session_id() functions causing problems similar to what @sc0ttkclark described: "change the existing session ID used previously, or reset the session data."
    • When simplesamlphp is trying to load a previous state, it is already on a diferent session, not finding the state it is supposed to load.
    • This causes an error on simplesaml: SimpleSAML_Error_NoState: NOSTATE;
    • I guess that Symfony Session Managment is causing this and i don't think it is a problem on simplesamlphp since it's working with a bunch of other applications.

What can we do to solve this? Is this normal behavior from symfony? I think this can cause problems in other External Authentication Providers with their own session functions.

@drak if you are still interested in finding a solution for this problem i can give you any details you need just ask. I'm very interested in solving this because we are investing a lot on symfony in my company but every single of our aplications uses saml for authentication.

@bamarni
Contributor
bamarni commented Mar 1, 2013

I also need to start the session outside symfony and let it continue with it once the framework is booted, ignoring app level session config.

how about allowing this for PHP >= 5.4.0 for the moment? @drak's solution (session_status()) looks clean enough.

@bamarni
Contributor
bamarni commented Mar 6, 2013

As apparently before PHP 5.4 tehre is no reliable way to check wether the session is started or not, I suggest instead to have a NativeProxySessionStorage, using the already existing NativeProxy handler, its code would be pretty straightforward, it'd never set any session ini setting, it'd never start any session, wouldn't use any metadatabag, because I don't think it makes sense to have 2 different starting points in that situation.

Basically here we only want to say : "the session management is handled outside symfony, if symfony needs the session, it should only populate the $_SESSION superGlobal with its stuff".

A drawback I see about this is whenever symfony needs to populate the session while it hasn't been started by the wrapping app, imo this case should not be considered / allowed, the wrapping app must start the session for all requests.

@shacharsol

Can you guys merge this fix into release 2.2?

Jaza replied Jul 5, 2013

This is still affecting me on Symfony 2.3, I have a project where I need to manually start my own PHP session before Symfony's session handler kicks in. Would be great to get this merged in.

Jaza replied Jul 5, 2013

I just discovered the new PHP bridge session storage library in Symfony 2.3:
http://symfony.com/doc/current/cookbook/session/php_bridge.html

After switching the session storage system per those instructions, no longer having any session issues with my app.

So, no sure if any fix like this is actually still needed for Symfony 2.3... my use case is gone, not sure if others still have a need for it.

@bamarni
Contributor
bamarni commented Mar 8, 2013

I've just submitted a PR with my last suggestion (#7305), I'd be happy to get some feedback.

@bamarni
Contributor
bamarni commented Mar 20, 2013

I've also noticed something similar in the default form csrf provider, https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Csrf/CsrfProvider/DefaultCsrfProvider.php#L68 , which could probably be fixed for php 5.4 at least.

@fabpot
Member
fabpot commented Mar 20, 2013

Can someone work on a patch from PHP 5.4 by using the new session_status() function?

@ghost
ghost commented Mar 20, 2013

@fabpot - I've been working on something over the last weekend - will submit soon.

@bamarni
Contributor
bamarni commented Mar 21, 2013

@drak : did you see #7305? I'd love to get your feedback / remarks on this.

@fabpot
Member
fabpot commented Apr 21, 2013

it has been addressed by #7571

@fabpot fabpot closed this Apr 21, 2013
@dichen001 dichen001 referenced this pull request in dichen001/Paper-Reading Jun 28, 2016
Open

Summary of the 20 issues in Herbsleb's 2014 FSE paper. #6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment