8000 Checking whether there is data in the session, creates a session · Issue #6036 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Checking whether there is data in the session, creates a session #6036

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
disposable-ksa98 opened this issue Nov 17, 2012 · 64 comments
Closed

Comments

@disposable-ksa98
Copy link

UPDATE: This issue has 2 parts. The first one is explained in this post. The second one is explained in this comment.

I noticed the website I am developing was sending a cookie in the response, even for anonymous requests (ie: not logged in, and deleted all cookies).

After trying everything and failing to find the problem, I created a "hello world" with a fresh install, and there was no cookie.

But as soon as I added this, the cookie came back:

{% for flashMessage in app.session.flashbag.get('success') %}
    <div class="alert alert-success">
        <button type="button" class="close" data-dismiss="alert">×</button>
        {{ flashMessage }}
    </div>
{% endfor %}

So apparently app.session.flashbag.get() creates the session. I'm sure many other parts of my website are unintentionally creating a session, and that's why I couldn't find the culprit.

Is there a reason for this? Isn't it adding overhead that could have been avoided? I imagine this won't affect sites that are login-only, as they will need the session anyway for most of the visits. But sites that can be used anonymously might suffer a bit.

@ghost
Copy link
ghost commented Nov 17, 2012

The session once had to be specifically started, but it was changed to autostart when used by request.

@morgen2009
Copy link

Just add in your template something like {% if app.session.started %} ... {% endif %}

@disposable-ksa98
Copy link
Author

@morgen2009 Thanks, that helped. I still think get() should not start the session though. Why add a session for every visit to any page in the website? (I disabled lots of things in my real website but still can't find where I'm not checking if the session started, before using it) I haven't checked it yet, but if this also adds an empty session to the database (when using a db as storage for sessions) then it's definitely wrong IMO. This performance detail is similar to that of using a different domain for static assets, to avoid sending/receiving cookies when not necessary.

@Drak ping

@dlsniper
Copy link
Contributor

Hi there,

It seems that the changes in this PR #4541 have been reverted somehow and this is why the session is not correctly reported as being active or not by the WDT for example.
I'll investigate right now to see what happened.

Best regards.

@dlsniper
Copy link
Contributor

Seems @fabpot didn't agreed with the change of the functionality but in this case I'm not sure how to apply the change as the functionality is currently broken for 2.1 and newer and HttpFoundation/Request is marked with @api.
While the change is simple I'm not sure how to proceed about it as imo the hasSession() should be changed to reflect the functionality of 2.0.X branch and not introduce a BC break as well.

Any thoughts?

@dlsniper
Copy link
Contributor

@disposable-ksa98 If you do a get() operation from the session then it should be activated, what doesn't seem right to you? There's no other way to check if the variable is in session or not unless you start it. The fact that the WDT displays the session as always active it's a bug from a previous commit, see my comment above.

I've just tested this against master: fresh copy of master + session in memcached. When you enter the default page, WDT displays that you have a active session in your request but if you apply the change from #4541 in the Request.php file, then you'll receive the right values when checking if the session is indeed started or not.

@disposable-ksa98
Copy link
Author

@dlsniper I'm not using the WDT to check the session, I'm checking the http headers and the cookies in my browser. And I originally detected the problem in production, so...

To sum up:
If I'm not storing anything in the session, why is there a session cookie? It's unnecessary overhead, the fastest framework shouldn't do that.

Checking whether there is data in the session should not create a session. I don't know much about Sf2's internals, but if I were to design a session component, I would probably do something like this:

function get($key) {
    // If the request didn't give us a cookie, and we didn't set() it ourselves,
    // why do anything else? Just return null.
    if( ! $this->session) { return null; }

    // find and return the value for that key
}

Another option could be to throw an exception: "There is no session to get values from". That would be correct but more annoying to use, because every time you want to check for a key/value you will have to first ask hasSession() or something like that (I would prefer that though, rather than the current situation). Session should be started only if the request comes with a session cookie, or if we set() a key/value.

I'm sure many bundles and extensions are doing checks on the session, without knowing they are starting it, because we don't have the proposed exception, so they didn't know they had to first check whether the session existed.

@dlsniper
Copy link
Contributor

@disposable-ksa98 I don't think that the approach is good, either with throwing an exception or returning null.
If you write: $this->get('session')->get($key); then I really see no point in not starting the session automatically for you.

Also, using exceptions as flow control methods is wrong! Look only how the code would look like:

try {
    $this->get('session')->get($key);
} catch (SessionNotStartedException $e) {
    // Now what? 

    //Start the session and get the value again?
    $this->get('session')->start();
    $this->get('session')->get($key);

    // Or just ignore it?

    // Other option?
}

Imagine the above code in a for fetching things from the session, like flashes. And what happens if you don't have any of that code in the controller/service/model/whatever and you have it in the templates? {% app.session.start %}

Using $this->get('session')->hasSession(); would be a sufficient check for your application in order to know if it has a session or not and the either fetch the values from the session.

And thinking the functional way of making things, do you remember who all apps have session_start() on the very first line? Or if they don't have that, what a pita is to actually have all the session detection for each read/write you are going to do?

You could disagree with me, and I'm sorry to say it, but it seems that your application is actually poorly thought / written rather that a problem in the framework.

And I'm fairly sure that with bundles using the session then they might actually need it, that's what hasSession() function is for.

@disposable-ksa98
Copy link
Author

@dlsniper I got emotional when you said my application is poorly thought, so I'll start over and just ignore that part. All what I'm gonna say is that the only part in my own code that was doing this, was that flash snippet. Code that does not check for hasSession() is all over the place apparently, probably in things that aren't enabled by default.

Nowhere in the framework's documentation it says that getting the session will start the session. And calling hasSession() is not being enforced by the framework's code either (no exceptions thrown).

Don't tell m 8000 e what I'm saying is ridiculous, I don't know the internals, I can't give you a fix. I don't even know the exact moment when the session is started. Is it when it's created by the DIC? Is it on $session->get() ?

The code you posted could be changed to this:

if there is a session
    do something with the session
endif

If someone tries to do it your way but without the try/catch, will get an exception soon enough and learn that he must either check hasSession() or catch the exception. I didn't want to force people to use try/catch everywhere, only force them to read "you must check hasSession() first!". That way, if I install a third party bundle that uses the session component, I will know I won't be sending an unwanted cookie by mistake, without reading thousands of third party LoC.
Or, the more friendly approach: get() returns null. I don't see how the creation of the session object or calling get() must start the session no matter what. What's so wrong with this?:

$this->get('session')->get('bla');
/**
 * Does not start the session, and a cookie is not sent,
 * because this is a smart object that can make a simple
 * check before doing that.
 */

For a moment imagine you are trying to "sell" the framework and you are asked: "If I didn't store anything in the session, why are you sending a session cookie? Is that good for performance?".
What would you answer?

Another interesting scenario to think about: In some european websites I've been welcome with a modal box telling me something like "By the law 1234 we are forced to ask for your approval to receive cookies. Cookies are X, that are used for Y". What would they do if they used Symfony2? They would be in trouble.

@dlsniper
Copy link
Contributor

First of all, I apologize I was rude to you without having a reason to be so and even then I shouldn't have been so. Reading over and over again helped me realize what you wanted to say :)

Second, I've now finally been able to see that the session driver itself is broken, it tries to store the serialized object like this: _sf2_attributes|a:0:{}_sf2_flashes|a:0:{}_sf2_meta|a:3:{s:1:"u";i:1353365612;s:1:"c";i:1353365612;s:1:"l";s:1:"0";}.

Third, I'll come up with a fix for the driver ASAP but like I've said, the other request, to not autostart the session doesn't seem reasonable to me, if you want, open a PR for this or a RFC and see the feedback, I won't interfere with it.

Fourth, having the session started even if not used at all is a bad idea for performance indeed hence I'll try and fix it asap. As for the EU law, you can do whatever the rest of the websites do: We are going to use these cookies to make your experience better. And IIRC, the EU law doesn't say anything about local storage so it would be nice to see a new way to bypass the thingy by having the local storage store the 'former cookie' info then pass it via AJAX :D But I don't suggest you to break the law!

@dlsniper
Copy link
Contributor

Oh and one more detail, what version of PHP are you using? I'm on 5.4.6 / Ubuntu 12.10. Thanks!

@cs278
Copy link
Contributor
cs278 commented Nov 19, 2012

@dlsniper FYI the EU directive applies to all forms of client side storage

@dlsniper
Copy link
Contributor

It seems that I had:

                {% for flashMessage in app.session.flashbag.get('notice') %}
                    <div class="flash-message">
                        <em>Notice</em>: {{ flashMessage }}
                    </div>
                {% endfor %}

in my template, using AcmeDemo, it would indeed start the session. Once I've wrapped it with {% if app.session.started %} ... {% endif %} and removed the existing cookie it would start behaving correctly and not send the cookie.

So to sum it up. There's indeed a 'issue' when you don't store anything in the session but you use it to retrieve flashes/data that could be in it without checking if the session is started or not first.

Then there's a section in the manual that explains the sessions and it documents the behavior pretty well: http://symfony.com/doc/master/components/http_foundation/sessions.html saying
While it is recommended to explicitly start a session, a sessions will actually start on demand, that is, if any session request is made to read/write session data.

Also, any of the suggested methods for starting a session / returning null / throwing an exception won't guarantee you that a bundle won't do the same thing to start a session, think about it ;)

@disposable-ksa98
Copy link
Author

@dlsniper I'm using PHP 5.3.10-1ubuntu3.4 with Suhosin-Patch, and Ubuntu 12.04

To me, the way it works right now, is anti-intuitive and unwanted behavior. Just like when you pass an invalid argument, and the framework throws InvalidArgumentException, trying to get something from a non-existent session should either issue a warning/throw an exception, or just return null.

Yes I've already seen that checking for app.session.started prevents the cookie issue, I said it here. But we shouldn't rely on people remembering to do that. Also, code that comes with the framework is already making this mistake (let alone third party bundles or extensions)

Please compare these two and tell me if it's not doable and good.

// Note: I haven't read the source code, this is only to show the concept

// Current situation
class Session {
    public function __construct($request) {
        // Someone asked for the session, I'll just start it even if this might
        // add unnecessary overhead and unwanted or even illegal cookies
        $this->startSession();
    }
    public function get($key) {
        // return value
    }
    public function set($key, $value) {
        // set the value
    }
}

// Desired
class Session {
    public function __construct($request) {
        // Maybe the object has been created only to call get(), no need to rush.
        // So I'll only start the session if there already is one.
        if($this->hasSessionCookie($request)) { $this->startSession(); }
    }
    public function get($key) {
        if( ! $this->isSessionStarted()) { return null; }

        // return value
    }
    public function set($key, $value) {
        if( ! $this->isSessionStarted()) { $this->startSession(); }

        // set the value
    }
}

Same usage, better perfomance, no unexpected behavior, and no legal issues.

@morgen2009
Copy link

@disposable-ksa98, your sketch is wrong (look into source code at Symfony/Component/HttpFoundation/Session). IMHO, session.get must be pure function (same output for the same input). Different results depending on internal switches on/off is bad idea. The solution would be either the third parameter for get method (auto_start or not) or twig extension with filter function ({{ app.session | myget('flash') }}).

@TerjeBr
Copy link
TerjeBr commented Nov 20, 2012

Different results depending on if the session is started or not is a good idea, since it was the whole point of this bug report.
And it makes good sense to not start the session if you do a {{ app.session.flashbag.get('notice')}} and there is no session cookie from before.
I agree with the original poster. Please reconsider.

@dlsniper
Copy link
Contributor

Just make a PR and see if Fabien will merge it, the best way to solve it.

The long way to solve it:

  • different return types based on session states checked internally in the framework is a very BAD idea as it won't provide consistency in output and it will surprise the user. And there's a thing that says: don't surprise your users. I deal with a lot of bad code lately, and I mean bad code, and one of the major issues of it is that you call a function and based on some random arcane stuff it does some things or other things, would you really want that in a framework???
  • how would you store null then?
  • the code in the demo framework isn't flawed, it's just a demo to show you how you can do some things, a demo should be regarded as a demo and if you consider it a bug, by all means, open a PR/issue;
  • read the manual, like I've said/pointed out, it's written there for a reason. Understand the manual. If you find something that is not properly documented help the guys who wrote the manual by letting them know about the problem or having a PR opened with the missing information;
  • how can you say that by having the check there is a good thing? If I need to start the session in the code the I'll need to start it at some point and since we are using a modern framework which has a DI container and events and some other bells and whistles, how do you expect for a bundle author to write a bundle that uses the session to provide some functionality if he'll do the very same thing as you? And how do you plan to control that, starting point of the session in the code that is not yours? Or even if it's yours, a code that's using events for example, or the DI? I think you just view all the implications this change would have: imagine that if everyone would be doing session start calls and then the framework component that handles the session does more checks internally. You'll surely not have a performance increase but as at the same time you just successfully added a new layer of complexity and useless code by just having everything in 'can I do? this mode';
  • TWIG has some flow control structures, you should use them as I'm really not sure how you handle the case where no flash is in your session;
  • what isn't natural about having the session stared for you when YOU requested something from it explicitly in YOUR code? In non-oop, out of framework code, how would you write that code???? I don't get it, really, how would your code look like in your code? Like this?
echo $_SESSON['myVar'];

It would give a hell of a big PHP Notice and if you tell me that your sever doesn't run with error_reporting on or with notices turned off then it means that you are just promoting bad code and that's for sure what Symfony tries to force you not to do, maybe you've seen the 'annoying' notice error screen from the framework.

If your code would on the other hand look like:

// Detect if the session was started
$isSessionStarted = // MAGIC??

if ($isSessionStarted) {
    echo $_SESSION['myVar'];
}

Then I'd ask you, why did you just did that? Put a check if the session is started or not? Was it because you knew it would give you a warning if you tried to access it directly? If so, what prevents you from having the same thing in the current situation we are talking about? And if you do this, how would other bundle authors be able to provide you with features. Could it be... the same way you do it? Or just restrict non-user bundles accessing the session? (besides sounding like an Apple programmer would rather trap the users soul forever that to give him choices) Do you really really really want that option?!?!

  • as a last point. If you think that empty sessions should be closed/destroyed from the server then there's the following issue, besides the performance thing which might not be that big but... it would mean that before writing off the session data you should check if the session is empty or not and if not then what? Either remove the existing session cookie from the cookies that are about to be sent, BAAAD idea because you might just want to have session cleaned but NOT removed and there wouldn't be any way to prevent it unless you add more logic to the framework to handle this, or leave the cookie alone but then you'll end up with the cookie which you didn't wanted.

If you still can't see how many problems would have to be fixed by this change, also consider this. All, and I mean all, programmers want faster, lighter, smarter framework. Having one that meets all those three requirements is very hard. If you didn't noticed how many problems that the framework would have then to fix for you and not to mention the increase in unnecessary code you just added then there's your issue.

And RTFM :)

Finally, since my job is exhausting as it is, I won't bother you with more messages on this topic as I think I've did tried to explain the best way possible how a documented feature works and why and I might not be the right guy for this. If you have a different opinion on this topic, fine by me, respect mine since I'm trying to respect yours, even if I'm trying to help you understand why you are wrong, at least from my point of view. just /mention someone from GH like Drak or Fabien or Stof and maybe they will be able to help you out.

Have a great day.

@disposable-ksa98
Copy link
Author

@dlsniper I wasn't talking about demo code, I was talking about core and/or third party code. I don't know what I have enabled in my app that is generating the cookie (I will track it down when I have time), but I'm only using core code and well-known third party bundles (so it's definitely not my code).

RTFM is not the answer, the current behavior is bad and you should feel bad*. We use names for functions as a hint of what they do. If I only call get(), and it sets a cookie, it's wrong.

If you** decide to stick to your position, at least fix the core code and do what you said we are supposed to do, because apparently you or well known third party developers didn't RTFM. But I don't think this will solve anything, someone will forget to do the check and the cookie will be back.

_just kidding
*_I'm talking to everyone who develops this, not just @dlsniper

@dlsniper
Copy link
Contributor
8000

Download the Symfony Standard distribution, run check the layout.twig.html file for the flashes thing to be enveloped by the above check and you'll see that the core doesn't produce any cookie by its own.

Can you paste a list of third party bundles you are using from composer.json? I'll add them in a new/fresh copy of Symfony 2.1.3 and check it against it. And if it's a third party bundle, just open a issue to the bundle author when you find it.

@disposable-ksa98
Copy link
Author

@dlsniper As I said two times, I did the test and it worked fine, that's not the point. I'm saying core things that are not enabled by default are doing this. I'll be very busy until friday, please don't close this, I'll be back with a real example. But again, even if it's third party, I don't think it's their fault: get shouldn't set.

@disposable-ksa98
Copy link
Author
  • Install 2.1.3
  • Create a new bundle and an empty page for '/'
  • Add a new firewall to security.yml:
security:
    firewalls:
        main:
            pattern: ^/
            form_login: ~
            anonymous: ~
  • Optionally disable csrf, doesn't matter
  • '/' will create a session cookie

The reason is probably this:

Anonymous users are technically authenticated, meaning that the isAuthenticated()
method of an anonymous user object will return true.

The issue just became bigger.

To sum up: With Symfony2, you can't avoid sending cookies to anonymous users, if your pages also allow users that are logged in. You can also be sending cookies even if you don't have any firewalls, if you make the mistake of calling get() without first checking isSessionStarted(), because get will set the session (unexpected and undesired behavior).

@TerjeBr
Copy link
TerjeBr commented Nov 24, 2012

I guess the main problem is; that if there is a session, you have to start it to get the data in the session. That is why get must start a session.

@TerjeBr
Copy link
TerjeBr commented Nov 24, 2012

I think I have found the source of the big misunderstanding and disagreement in this issue. It is that we can mean different things when we say "the session has started" or "start the session". It is a big difference if we mean "the session was started in this request" or "the session was started in a previous request". By being more explicit about this we can avoid some misunderstanding.

If the session was not started in a previous request it makes sense not to start the session on a get. Even if you start the session, you will still get null as the value. So the get function will not return different values depending on if the request was started in this request or not.

What this issue is about then is that the session->get function should only start the session in the current request if it had already been started in a previous request.

@webda2l
Copy link
webda2l commented Nov 24, 2012

"If you use a form login, Symfony2 will create a cookie even if you set stateless to true."
http://symfony.com/doc/current/book/security.html#stateless-authentication

Remains to understand why.

@dlsniper
Copy link
Contributor

Maybe @schmittjoh could help us out on this one?

@disposable-ksa98
Copy link
Author

I think I have found the source of the big misunderstanding and disagreement in this issue. It is that we can mean different things when we say "the session has started" or "start the session". It is a big difference if we mean "the session was started in this request" or "the session was started in a previous request". By being more explicit about this we can avoid some misunderstanding.

If the session was not started in a previous request it makes sense not to start the session on a get. Even if you start the session, you will still get null as the value. So the get function will not return different values depending on if the request was started in this request or not.

What this issue is about then is that the session->get function should only start the session in the current request if it had already been started in a previous request.

@TerjeBr That's right.

@TerjeBr
Copy link
TerjeBr commented Nov 29, 2012

Please see if not PR #6152 does what you want.

@ghost
Copy link
ghost commented Nov 29, 2012

Second, I've now finally been able to see that the session driver itself is broken, it tries to store the serialized object like this: _sf2_attributes|a:0:{}_sf2_flashes|a:0:{}_sf2_meta|a:3:{s:1:"u";i:1353365612;s:1:"c";i:1353365612;s:1:"l";s:1:"0";}.

@dlsniper - what is broken here in your opinion? That looks exactly as it should be to me unless I am missing something obvious.

@dlsniper
Copy link
Contributor

@Drak I should have removed that comment. That part of the sessions is ok, the problem was that I was too tired when I've did the debugging. So mind that comment please.

Also as the other guys found out, the session component is ok, the security one was doing all the cookie session issues.

9E88
@Tobion
Copy link
Contributor
Tobion commented Oct 26, 2014

I think how sessions should behave is clear to me:

  • Reading from the session when there is no session cookie, i.e. there is no previous session: It SHOULD NOT start the session. There is no reason to create a session with a cookie when only reading data. It just means the data is not available as we already know since there is no session. This is also what @TerjeBr suggested. @Drak argument that people need to check themselves whether the session is active is also possible. But the example he gave {% if app.session.isstarted() %}{% for flashMessage in app.session.flashbag.get('notice') %} is wrong. If the session is closed/saved this won't even work and not show the messages. So actually one would need to check Request::hasPreviousSession each time one reads session data at the moment. But the session part does not even have a method for that. So when injecting the session alone (without request) you don't have access to this important method. So in reality, forcing users to check the session state everywhere where they read data is not viable.
  • Reading from the session when there is IS a session cookie . It should auto-start the session which reads the information stored in the session and makes them available. Otherwise reading data would not find the data unless it has been started manually which is also not developer friendly.
  • Reading from the session when the session has already been started in the current request but is currently closed/saved: In this case we don't necessarily need to re-start the session because the data is already available. This is a performance enhancement because it prevents to read the data again that we already have. The only disavantage is that it does not synchronize the data written in another request between your save() and the get(). But this is highly unlikely and rather strange case anway. And devs could just call start() themselves if they want that sync.
  • Writing to the session should always start the session so that the data is actually persisted. Otherwise there would be no point in writing to the session when it's not available in the next request. Zend Session raises an immutable exception when writing to a session that is already closed/saved. But I don't think that's a good idea because then you cannot add session data later in the lifecycle anymore.

@mpdude
Copy link
Contributor
mpdude commented Nov 17, 2014

+1 for what @Tobion said, actually that is what I thought things were like.

@TerjeBr
Copy link
TerjeBr commented Nov 17, 2014

I am still waiting for any feedback on if I should continue working on #6388

@ureimers
Copy link
Contributor

Thanks @Tobion for the summary. This issue got a little bit out of hand. As @lsmith77 commented on #6388 this is an important feature on high traffic sites:

So by having a restricted form-login area and a shared-directory between our load-balancers for the sessions, the first thing that happened to our site was that the session directory was flooded with sessions after some hours (even if the users didn't access the restricted area that frequently) and on top of that we had to realize that our varnish didn't cache anything because of those sessions being set in the cookie on every response.

This could have been avoided if we configured varnish correctly in the first place but we didn't expect Symfony to store sessions/set cookies in every response.

So by referring to one of Symfonys mantras: "Use sane defaults (including default behavior)", I'm absolutely in favor of Tobion's suggestion/view on how Sessions should work in Symfony.

@netmikey
Copy link
netmikey commented Jan 8, 2015

We're starting to think about varnish caching as our site is growing. During our analysis, we stumbled upon the cacheability issue that @ureimers explained. Also, we're storing sessions into mysql and all this implicit session creation generates an awful lot of unnecessary load.

+1 for @Tobion's excellent behavior description and @ureimers quote about sane defaults btw. ;)

Also, github got me a bit confused with the merging and reverting... did the pull request eventually make it into Symfony @fabpot ? If so, what Symfony version would we need to migrate to, to be able to tame session creation?

@TerjeBr
Copy link
TerjeBr commented Jan 8, 2015

It is not in any symfony version yet.

I am still waiting for any response from @fabpot if we should go ahead with #6388 or not.

@weaverryan
Copy link
Member

@Tobion I agree fully with your description as well - there's been some confusion between "is the session started" and "is there a session cookie - i.e. was the session started on a previous request". You explain that well. I think this is the right approach, so do you have time and knowledge of this area to look at #6388 to see if it's the right approach? We really either need to say "yes, this is the right approach let's finish this PR" or "no, it's not the right approach, let's work on a different PR".

Thanks :)

@weaverryan
Copy link
Member

@TerjeBr And thanks for not letting this go - I know it's been a huge, long confusing road :)

@TerjeBr
Copy link
TerjeBr commented Jan 24, 2015

Thank you @weaverryan for giving your support for this issue. I absolutely agree with what you said.

As I have said before, I am happy to continue work on PR #6388 but I hesitate to do so before any one with some authority in this symfony2 community are willing to give me some positive feedback and say this is the way to go.

@TerjeBr
Copy link
TerjeBr commented Jul 27, 2016

@fabpot Any input on how to solve this? Should I update my #6388 pull request?

@ureimers
Copy link
Contributor
ureimers commented Dec 2, 2016

@TerjeBr Thumbs up for not letting this slip for four years now!

@Koriit
Copy link
Koriit commented Dec 9, 2016

I fail to understand why this issue hasn't been resolved for 4 whole years now while it seems to me like an obvious bug!

There surely is a notice in Symfony documentation:

While it is recommended to explicitly start a session, a session will actually start on demand, that is, if any session request is made to read/write session data.

While my reaction was "Well, sure, that's obvious thing to do" it never occured to me that reading data from non existent session(no session cookie) would first create empty session and only then try to check if the data I'm trying to get even exists. In my opinion this is wrong as this impacts performance and this is not an intuitive thing to do. When I read that notice, I thought that if read operation notices that there is no session to load(no session cookie) then it would return with NoData as there actually is no data to read. And I only stumbled on this thread by sheer luck on Google.

I think the problem here is that to start a session is actually ambiguous as it can mean either creating completly new session or loading existing session from the storage.

And as above, thumbs up for @TerjeBr.

@iloveyii
Copy link

I fixed the problem by entering the following in config.yml

session:
        cookie_domain: otp.softhem.se

@TerjeBr
Copy link
TerjeBr commented Oct 14, 2017

This is fixed by #24523 and can be closed.

@xabbuh
Copy link
Member
xabbuh commented Oct 16, 2017

#24523 isn't merged yet

fabpot added a commit that referenced this issue Oct 16, 2017
…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
@fabpot fabpot closed this as completed Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

0