Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Prevent sending multiple Set-Cookie headers #1780

Closed
wants to merge 1 commit into from

6 participants

@aaronpk

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

@aaronpk aaronpk 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
@alexbilbie

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

@aaronpk

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

@dchill42

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

@aaronpk

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.

@dchill42

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.

@dchill42

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.

@aaronpk

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.

@dchill42

@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.

@narfbg narfbg referenced this pull request
Closed

cookie set multiple times #1345

@narfbg narfbg referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@narfbg
Owner

How does the referenced commit work?

@narfbg narfbg referenced this pull request from a commit
@narfbg narfbg 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
@tlianza tlianza commented on the diff
system/core/Output.php
@@ -420,6 +420,11 @@ public function _display($output = '')
}
}
+ if($CI->session) {
@tlianza
tlianza added a note

Seems like line 433 checks if the $CI object exits... so maybe this block should be after that one.

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

Superseded by #3073.

@narfbg narfbg closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 10, 2012
  1. @aaronpk

    Prevent sending multiple Set-Cookie headers. Only runs 'sess_save()' …

    aaronpk authored
    …one time right after all other headers are sent before outputting the page contents.
    
    Signed-off-by: Aaron Parecki <aaron@parecki.com>
This page is out of date. Refresh to see the latest.
View
5 system/core/Output.php
@@ -420,6 +420,11 @@ public function _display($output = '')
}
}
+ if($CI->session) {
@tlianza
tlianza added a note

Seems like line 433 checks if the $CI object exits... so maybe this block should be after that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // Save the session. If using session cookies, this ensures only one "Set-Cookie" header is sent
+ $CI->session->get_driver()->sess_save();
+ }
+
// --------------------------------------------------------------------
// Does the $CI object exist?
View
8 system/libraries/Session/Session.php
@@ -177,6 +177,11 @@ public function select_driver($driver)
}
}
+ public function get_driver()
+ {
+ return $this->current;
+ }
+
// ------------------------------------------------------------------------
/**
@@ -279,9 +284,6 @@ public function set_userdata($newdata = array(), $newval = '')
$this->userdata[$key] = $val;
}
}
-
- // Tell driver data changed
- $this->current->sess_save();
}
// ------------------------------------------------------------------------
View
6 system/libraries/Session/drivers/Session_cookie.php
@@ -512,9 +512,6 @@ protected function _sess_create()
// Add empty user_data field and save the data to the DB
$this->CI->db->set('user_data', '')->insert($this->sess_table_name, $this->userdata);
}
-
- // Write the cookie
- $this->_set_cookie();
}
// ------------------------------------------------------------------------
@@ -555,9 +552,6 @@ protected function _sess_update($force = FALSE)
'session_id' => $this->userdata['session_id']
), array('session_id' => $old_sessid));
}
-
- // Write the cookie
- $this->_set_cookie();
}
// ------------------------------------------------------------------------
Something went wrong with that request. Please try again.