[Session] Do not start the session unless it is (or has been) written to #6388

TerjeBr · 2012-12-16T22:49:58Z

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #6036
License of the code: MIT

The session->get (and the like functions) should only start the session in the current request if the session had already been started in a previous request.

stof · 2012-12-16T22:54:12Z

src/Symfony/Component/HttpFoundation/Session/Attribute/EmptyAttributeBag.php

+     * @param SessionBagInterface   $realBag
+     */
+    public function __construct(EmptyStorageInterface $storage,
+                                SessionBagInterface $realBag)


Please keep the signature on one line

stloyd · 2013-01-07T08:21:12Z

@Drak @fabpot Any points on those changes?

ghost · 2013-01-07T09:54:41Z

I am sorry but I'm -1 on this PR. I've procrastinated a lot about replying.

The PR is extremely convoluted with bags having to know about sessions and each bag having to be proxied. Bags themselves, should not be starting the session. It's full of code smell. It introduces a BC break by adding methods to an interface, which I am also not in favor of. (the PR summary text incorrectly say BC break "no").

I've not had enough time to really devote to looking at other possibilities but this is definitely not it, imo. Sorry.

TerjeBr · 2013-01-08T20:50:13Z

Why do you take the principal stand that bags should not be starting the session?
It is the whole point of the code that if you do not have a session from a previous request, a session should not be started unless a bag writes something to the session. It means that the session start must be controlled in the bags, or this issue can not be solved.

I did not regard adding methods to an internal interface to be a BC break. But I guess you are right, it is technically a BC break, even though I think it will pass by unnoticed by most of the application programmers that is using symfony. So I have updated the description in the PR summary text.

TerjeBr · 2013-01-08T22:01:05Z

@Drak, if you do not like the session to be started in the bags, may be you like better option 3 that I wrote on the original bug issue:

'3'. Leave the bags as they are, and delay the start of the session until just before the response is about to be sendt. Then a new response event listner has to be created that checks if anything has been written to any session bag, and start the session if that is the case.

I do not know if that will be any cleaner code, because then the session stuff has to have its own response event listener, and then the session code will not be independent from the rest of symfony.

lsmith77 · 2013-02-01T09:16:46Z

BTW this is quite an important feature for high load websites

dlsniper · 2013-02-04T15:03:18Z

@Drak could you help out making this a better PR?

It's the second time I've seen tickets like #6917 that rise this problem. And I think we can all agree with what @lsmith said about being a problem for sites with high load.

Do you think you could help us out with some ideas on how to do this, the proper way?

/cc @fabpot

ghost · 2013-02-04T16:09:00Z

I may not be able to answer well until the weekend or just after.

disposable-ksa98 · 2013-05-11T19:13:45Z

Good job @TerjeBr ! I'm glad to see this fix is going to make it into 2.3. Like @lsmith77 said, this is very important for high traffic websites.

TerjeBr · 2013-05-12T20:51:32Z

Is it going to make it into 2.3? I have heard nothing from the main maintainers of Symfony about that. I am still waiting for feedback from @fabpot on either this issue or the parent issue #6036

stof · 2013-05-13T07:44:35Z

No it won't be in 2.3. New features cannot be added in it anymore as we already froze them.

dlsniper · 2013-05-13T12:36:21Z

@stof with all due respect this is something that appeared several times in the past and the PR is 5 months old. While it might be a new feature I could also consider this as a bug fix. Do you keep bug fixes out of 2.3 release? Imho this should have been added for 2.3 a long time ago...

ghost · 2013-05-13T12:39:13Z

@dlsniper better keep the emotion out of this. Fabien has already decided which PRs will be included in 2.3 and that decision is final so there is no point making noise about it.

disposable-ksa98 · 2013-05-13T14:38:34Z

@stof @TerjeBr @Drak @dlsniper
http://symfony.com/blog/symfony-2-3-schedule-update

fabpot · 2013-05-13T14:41:30Z

This is an issue but we need to find the best solution to fix it. Unfortunately, we are not all in agreement with the right thing to do.

ghost · 2013-05-13T14:42:37Z

FYI, Fabien referenced this particular PR as a reference to what ticket #6036 talks about. There is no promise of anything being merged at all. Just that he'd look at it. He has since decided, regardless of whether you think this is a bug or a feature, that the issue will not be addressed until post 2.3

disposable-ksa98 · 2013-05-13T14:51:25Z

Post 2.3 means 2.4? Or 2.3.X? Ok guys, either way thanks for looking at it, keep up the good work.

For anyone else reading: Meanwhile just put all your authenticated routes under a prefix like "/user", that's what I do and it works.

stof · 2013-05-14T01:47:21Z

@disposable-ksa98 As this PR contains a BC break, it has absolutely no chance to be included in a 2.3.x bugfix release

TerjeBr · 2013-05-16T21:53:50Z

It is only a BC break because a method has been added to the Symfony\Component\HttpFoundation\Session\Storage\SessionStorageInterface
I doubt if any application programmers using Symfony are using that interface in their own classes.

TerjeBr · 2013-05-16T21:57:28Z

We should continue the discussion on issue #6036 to find a resolution to this. I have made my comments there, but no answer seems to be coming.

disposable-ksa98 · 2013-07-15T14:39:57Z

I'm not sure this was the case when I first reported the issue and tested it in 2.2.?... But now, in 2.3.1, the toolbar is creating a session too. So remember to disable it in config_dev.php when debugging this issue.

ghost · 2013-07-15T14:43:58Z

@disposable-ksa98 afaik the toolbar requires a session because it displays the session bags and metadata. If this is not the intended behaviour you should open a separate issue for it.

ureimers · 2016-12-02T10:29:37Z

Since four years our "solution" to this problem has been to use the NullSessionHandler on our production sites as otherwise, with one million page views per day, the session folders explode within a few days.
Hopefully we never need to add community and login support to those sites...

TerjeBr · 2017-09-28T15:08:33Z

@fabpot @nicolas-grekas Do you want me to start work on this PR again? Have you decided if this will go into symfony soon?

nicolas-grekas · 2017-10-08T18:09:38Z

I might take over as it would be great to have this in 3.4.
But there is something I don't get from the expected requirements in the linked issue: when there is no existing session cookie, the session should not be started yes, but the Cache-Control or Vary: Cookie header still need to be sent to me (that's not the case when only remove() is called btw.)
So, why are so many ppl expecting that not starting the session should improve the HTTP cacheability of the pages where they "get()"?

znerol · 2017-10-08T19:40:08Z

when there is no existing session cookie, the session should not be started yes, but the Cache-Control or Vary: Cookie header still need to be sent to me

Correct.

(that's not the case when only remove() is called btw.)

I don't understand what you mean. Can you elaborate on that?

So, why are so many ppl expecting that not starting the session should improve the HTTP cacheability of the pages where they "get()"?

Starting a session when there is no session cookie on the request will result in a cookie being created via the response. Responses with the Set-Cookie header will never be saved to a cache (that is true for the browser cache as well as any intermediate/edge cache like e.g. varnish). Needless to say that subsequent requests will have that cookie and will obviously render any intermediate/edge cache ineffective.

Your skepticism regarding cacheability of responses with Vary: Cookie is sensible. In an ideal world there wouldn't be any cookies on anonymous requests and as a result the browser cache as well as any intermediate/edge cache could deliver cached responses out of the box. In reality most sites will have some tracking. In order to make an edge cache effective it is necessary to strip tracking cookies. However, it is important to understand that if the remaining cookies are identical for large groups of users, then an edge cache can be effective even though Vary: Cookie is on every response. But that is obviously only true for those requests without a session cookie, because that one we cannot simply strip for obvious reasons.

TerjeBr · 2017-10-08T23:04:32Z

Hi @nicolas-grekas Thank you for picking up this. If you want any help from me, just tell me.

Also, just wanted to mention ticket #12325 that has useful analysis/info about this.

nicolas-grekas · 2017-10-09T10:44:22Z

There is an issue: if something is written to the session after the headers have been sent to the client, then we'll introduce a bug, isn't it?
So, can we do this at all on 3.4? Shouldn't we instead trigger a deprecation in 3.4 when this happen, then only in 4.0 could we properly close the issue?

sroze · 2017-10-09T10:52:02Z

Good spot @nicolas-grekas. If we change Symfony to require an explicit usage of sessions instead of the existing "by default" usage, it needs to happen on 4.0 or a lot of applications would potentially be broken (in a not necessarily easy way to debug) while going to 3.4.

TerjeBr · 2017-10-09T11:18:58Z

It will only be a bug if the application programmer before used $session->get() to implicitly start the session before the headers where sent, and then later used a $session->write() after the headers are sent.

Here is my reasons that we do not necessarily need to consider this to be a backwards compatibility:

It has never been intended that application programmes could rely on $session->get() to start the session, it has always been a kind of unwanted side effect.
It should be obvious to most programmers that if you write to the session after the response have been sent, you must make sure to always start the session.
Sessions are not generally "exisiting by default" and symfony programmer should not expect it to be so, they only (currently) automatically exist if the application programmer does something with the session (read/write) before the headers are sent.

nicolas-grekas · 2017-10-09T18:16:06Z

Fortunately, HttpKernel saves the session before calling $response->send(), which means we have room for fixing the issue.
Now, I wonder if this is the correct approach. I currently think an approach that deals with that only in the session handler could handle more cases (like emptying the session, or writing-but-not-changing).
Could be better to take a hash on ready, then decide on save. Was this approach already tried?

TerjeBr · 2017-10-09T18:39:57Z

Hi @nicolas-grekas I am afraid you have lost me here.

Why is it important that the HttpKernel saves the session before it calls $response->send?
You wonder if this is the correct approach to what?

TerjeBr · 2017-10-09T18:45:37Z

You wrote "Could be better to take a hash on ready, then decide on save. Was this approach already tried?"

You have to explain more what you mean, I did not understand what you meant by that first sentence.

sroze · 2017-10-09T19:47:30Z

@TerjeBr let me try to translate Nico's thoughts.

HttpKernel saves the session before calling $response->send()

Symfony uses the headers bag on the Response object to send headers (and not at any point in time the PHP header function). These headers are sent when calling the $response->send() in your front controller (web/app.php or similar). This means that the request is fully handled when we send the headers. This means we can actually do whatever we want with the headers and therefore we can start the session only if some set has been called without the BC break previously mentioned.

Now, I wonder if this is the correct approach. I currently think an approach that deals with that only in the session handler could handle more cases (like emptying the session, or writing-but-not-changing).

I guess the idea is that instead of "detecting" set, clear and such session changes, wouldn't it be better to "flush" the session (my words for his "decide on flush") based on a hash of the session content - or similar -. Not sure I see the value here @nicolas-grekas as this issue is as far as I understood only for non-already initialised sessions; therefore it's only when we start putting something in it 🤔

nicolas-grekas · 2017-10-09T20:43:16Z

@sroze thanks :) 1st point perfectly translated :)

For the 2nd point, the idea is to fix several issues at once with a simpler approach: instead of requiring a proxy for bags to track "sets", we could track only the data itself:
let the session start as is now, but kill the cookie if the saved data ends up being empty (let's forget about "hashing" I mentioned, it is already handled by php7.0 lazy-write mode).
I don't know yet how to "kill the cookie", but doing so would provide the following result:

the Cache-Control/Vary header would still be sent, which is needed
when reading data from non-already-existing session, no data nor cookie would be written/sent
when removing all data from the session, any pre-existing session would be destroyed, and a reset cookie could be sent.

(I don't know the feasibility of this yet.)

TerjeBr · 2017-10-10T10:10:50Z

"the idea is to fix several issues at once with a simpler approach: instead of requiring a proxy for bags to track "sets", we could track only the data itself:
let the session start as is now, but kill the cookie if the saved data ends up being empty"

This may not be a good idea. It kind of forces you to either write code yourself to "kill" the session cookie, that will depend on the inner workings of the session handler you are using, or delegate it to the session handlers by adding some method to the session interface.

There may be many types of session classes, and users may have written their own session storage classes to replace the standard symfony ones. Sessions may be stored in memory (for testing), in files, in database, write to queues or what not. That is why I added the wasStarted method to the session interface: https://github.com/symfony/symfony/pull/6388/files#diff-fa20e46327d302a47a2663577c84634b This new method delegates to the session storage class how to detect that a session was started on an earlier request. It will most likely result in checking for the presence of a cookie, but storage handlers should have the freedom to implement it in any way they see fit for purpose. If you allow the session to start, we do not know what other side effects might happen for a spesific session storage, and it may be diffucult to undo it. Just removing the cookie may result in leaking resources. To keep the framwork as agnostic as possible to which session storage is in use, I think your best option is to keep the session from starting in the first place if it does not need to start. I think the idea that you described above will lead you into a whole lot of complications that will make it a complex soluton.

nicolas-grekas · 2017-10-12T16:06:50Z

Closing in favor of #24523.
Thank you @TerjeBr for your help clarifying the topic!
Hope to see another PR of yours soon :)

TerjeBr · 2017-10-12T18:05:52Z

You should reopen this, since #24523 does not solve the problem.

TerjeBr · 2017-10-14T18:18:59Z

Ok, #24523 does address this now. So this can be closed.

…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

stof reviewed Dec 16, 2012
View reviewed changes

TerjeBr mentioned this pull request Dec 16, 2012

Do not start session on get, if no prevoius session #6152

Closed

Do not start the session unless it is (or has been) written to

a6ce6c8

dlsniper mentioned this pull request Feb 1, 2013

Sessions are always started for anonymous users, unnessesarily. #6917

Closed

TerjeBr mentioned this pull request Apr 18, 2013

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

Closed

nicolas-grekas modified the milestones: 2.7, 3.x Dec 6, 2016

nicolas-grekas modified the milestones: 3.4, 4.1 Sep 28, 2017

carsonbot added the Status: Reviewed label Oct 4, 2017

nicolas-grekas modified the milestones: 4.1, 3.4 Oct 8, 2017

nicolas-grekas mentioned this pull request Oct 11, 2017

[HttpFoundation] Make sessions secure and lazy #24523

Merged

nicolas-grekas closed this Oct 12, 2017

fabpot reopened this Oct 12, 2017

TerjeBr closed this Oct 14, 2017

Uh oh!

[Session] Do not start the session unless it is (or has been) written to #6388

[Session] Do not start the session unless it is (or has been) written to #6388

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!