Skip to content
This repository

Prevent sending multiple Set-Cookie headers #1780

Open
wants to merge 1 commit into from

5 participants

Aaron Parecki Alex Bilbie Darren Hill GDmac Andrey Andreev
Aaron Parecki

By default when using the cookie session handler (encrypted or unencrypted), CI sends the entire "Set-Cookie" header each time a new value is written to the session. This results in multiple headers being sent to the client.

This is a problem because if too many values are written to the session, the HTTP headers can grow quite large, and some web servers will reject the response. (see http://wiki.nginx.org/HttpProxyModule#proxy_buffer_size)

The solution is to only run 'sess_save()' one time right after all other headers are sent before outputting the page contents.

Signed-off-by: Aaron Parecki aaron@parecki.com

Aaron Parecki Prevent sending multiple Set-Cookie headers. Only runs 'sess_save()' …
…one time right after all other headers are sent before outputting the page contents.

Signed-off-by: Aaron Parecki <aaron@parecki.com>
7d4aeca
Alex Bilbie
Collaborator

What if you're not outputting data via the output class?

Aaron Parecki

I was not aware you could avoid using the output class. Suggestions if this is the case?

Darren Hill

Sess_save does more than just write the cookie. You can't just cut that call out of Session.php.

Aaron Parecki

Perhaps someone more familiar with the inner workings of CodeIgniter can submit a better patch. This problem exists. Whether my solution is the best is not important, I just want to see this fixed.

Darren Hill

I think my best suggestion is to add a defer option to the cookie driver. The app could then enable deferment and call the set_cookie manually when the time is right.

Trying to fully automate this process is fraught with peril. There is just no solid and consistent way to know when an app is going to start sending output. There is also the scenario in which something like show_error() gets called, and Output->display() never happens. Should the cookie still be sent then? Only the application developer can navigate the timing of setting headers reliably. I think a manual solution (which should be optional) is the only way to make this work for everybody.

Darren Hill

It looks like #1710 is a related request in trying to reduce redundant Set-Cookie headers. I'm not sure that one actually achieves anything, though. It appears to just detect changes in the actual session data, which are implied by the call to set_userdata(). In the case of an application making numerous different changes to session data during a request, there would still be a cookie header for each change.

Aaron Parecki

Yea, I believe #1710 doesn't completely solve the problem. I did notice the first time you initialize sessions, CI sends the Set-Cookie header, and then if you don't make any changes it will send an identical header. That's probably what #1710 fixed.

Darren Hill

@aaronpk Good eye - if you're correct, that fix addresses a different problem (miniscule as it may be).

As for the problem at hand here - I'm happy to implement a solution if we can come to a consensus about what is an appropriate fix. Maybe we can get some feedback from @alexbilbie if he's not buried in other tasks.

GDmac

This mainly seems to be a problem when using cookies only (less so when using DB or php native sessions). I agree with @dchill42 that setting cookies from the app is a responsibility for the developer, however in this instance it is something the session library does internally.

  1. IMO, the first shot at this issue is when using session with sess_use_database. It might only need to write the cookie after: create, destroy and update (update is also called by regenerate). One caveat is that _set_cookie() also updates the expire time of the cookie.

  2. For the second part (sending cookie userdata only once) there is a short answer that could go into the docs: When using session cookies only, don't use set_userdata() or any session userdata method more than needed. Every time you change session userdata, the cookie will be send.

2a. Implementing a way to send userdata late (register_shutdown) might be possible, but would need serious regression tests. For instance: When the new cookie data is send, but the connection is still open (e.g. php-script not finished), does the browser use any new data and more important, a possible new session_id it might have already received. Does the browser use this new session_id in subsequent (ajax) requests to the server, with the first request still running? If so, then sending userdata late (register_shutdown) might be somewhat problematic.

2b. If you would want to give such a 'defer' implementation a shot, i would prefer NOT making it a global implementation for the entire session class if 2a is problematic. e.g. it should then probably be able to set 'defer' on a per variable basis or use a separate method.

Andrey Andreev narfbg referenced this pull request from a commit December 01, 2012
Commit has since been removed from the repository and is no longer available.
Andrey Andreev
Collaborator

How does the referenced commit work?

Andrey Andreev narfbg referenced this pull request from a commit November 29, 2012
Andrey Andreev Replace cookie helper set_cookie() with an improved version
as a common function.

Also deprecated CI_Input::set_cookie() which is now an alias
for this new function.

The new function will now replace cookies with the same name
that were already set (either by set_cookie() or the native
setcookie() and header() functions) in the PHP's headers
queue.
This fixes issue #1345 and supersedes PR #1780,
which were aimed at fixing the Session library's behavior
where it sent multiple cookies with the sess_cookie_name
when the session cookie value had changed.

It will now also always send the relatively new Max-Age
cookie attribute (see http://tools.ietf.org/rfc/rfc6265.txt)
and Expire will always be sent as a GMT timestamp, in an
attempt to fix reported issues with Google Chrome (see
issues #1726 and #1908).

Cookies with the Secure attribute that are intended to only
be send by the browser via encrypted connections will no
longer be send if the website is not accessed via HTTPS.

Also, the optional parameters' default values are changed to
NULL instead of actually usable values, so that config_item()
calls are only used if we're sure that the user/developer
didn't set those intentionally.

All usage of the native setcookie() function in CI has been
replaces with set_cookie().
128d719
James shijialee referenced this pull request in benedmunds/CodeIgniter-Ion-Auth April 25, 2013
Open

unset_userdata calls send same name cookie multiple times in header #447

bioshock bioshock referenced this pull request in bcosca/fatfree April 11, 2014
Closed

Session bug: multiple set-cookie headers #533

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

Showing 1 unique commit by 1 author.

Sep 09, 2012
Aaron Parecki Prevent sending multiple Set-Cookie headers. Only runs 'sess_save()' …
…one time right after all other headers are sent before outputting the page contents.

Signed-off-by: Aaron Parecki <aaron@parecki.com>
7d4aeca
This page is out of date. Refresh to see the latest.
5  system/core/Output.php
@@ -420,6 +420,11 @@ public function _display($output = '')
420 420
 			}
421 421
 		}
422 422
 
  423
+		if($CI->session) {
  424
+			// Save the session. If using session cookies, this ensures only one "Set-Cookie" header is sent
  425
+			$CI->session->get_driver()->sess_save();
  426
+		}
  427
+
423 428
 		// --------------------------------------------------------------------
424 429
 
425 430
 		// Does the $CI object exist?
8  system/libraries/Session/Session.php
@@ -177,6 +177,11 @@ public function select_driver($driver)
177 177
 		}
178 178
 	}
179 179
 
  180
+	public function get_driver()
  181
+	{
  182
+		return $this->current;
  183
+	}
  184
+
180 185
 	// ------------------------------------------------------------------------
181 186
 
182 187
 	/**
@@ -279,9 +284,6 @@ public function set_userdata($newdata = array(), $newval = '')
279 284
 				$this->userdata[$key] = $val;
280 285
 			}
281 286
 		}
282  
-
283  
-		// Tell driver data changed
284  
-		$this->current->sess_save();
285 287
 	}
286 288
 
287 289
 	// ------------------------------------------------------------------------
6  system/libraries/Session/drivers/Session_cookie.php
@@ -512,9 +512,6 @@ protected function _sess_create()
@@ -555,9 +552,6 @@ protected function _sess_update($force = FALSE)
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.