-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Comments
The session once had to be specifically started, but it was changed to autostart when used by request. |
Just add in your template something like {% if app.session.started %} ... {% endif %} |
@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 |
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. Best regards. |
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 Any thoughts? |
@disposable-ksa98 If you do a 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. |
@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: 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:
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 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. |
@disposable-ksa98 I don't think that the approach is good, either with throwing an exception or returning null. 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? Using And thinking the functional way of making things, do you remember who all apps have 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 |
@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 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.
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?". 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. |
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: 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! |
Oh and one more detail, what version of PHP are you using? I'm on 5.4.6 / Ubuntu 12.10. Thanks! |
@dlsniper FYI the EU directive applies to all forms of client side storage |
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 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 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 ;) |
@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.
Same usage, better perfomance, no unexpected behavior, and no legal issues. |
@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') }}). |
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. |
Just make a PR and see if Fabien will merge it, the best way to solve it. The long way to solve it:
echo $_SESSON['myVar']; It would give a hell of a big 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?!?!
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. |
@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 |
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. |
@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. |
The reason is probably this:
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). |
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. |
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. |
"If you use a form login, Symfony2 will create a cookie even if you set stateless to true." Remains to understand why. |
Maybe @schmittjoh could help us out on this one? |
@TerjeBr That's right. |
Please see if not PR #6152 does what you want. |
@dlsniper - what is broken here in your opinion? That looks exactly as it should be to me unless I am missing something obvious. |
@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. |
I think how sessions should behave is clear to me:
|
+1 for what @Tobion said, actually that is what I thought things were like. |
I am still waiting for any feedback on if I should continue working on #6388 |
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. |
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? |
@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 :) |
@TerjeBr And thanks for not letting this go - I know it's been a huge, long confusing road :) |
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 Thumbs up for not letting this slip for four years now! |
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 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. |
I fixed the problem by entering the following in config.yml
|
This is fixed by #24523 and can be closed. |
#24523 isn't merged yet |
…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
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:
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.
The text was updated successfully, but these errors were encountered: