Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Detailed here why this is important: https://www.owasp.org/index.php/HTTPOnly
The core instances of setcookie() grab the parameters set in php.ini to allow httponly to be set. This is very important in high-profile, production sites. See session.inc, line 348, etc.
Although minor, the instances used in webform should also follow this practice.
http://php.net/manual/en/function.setcookie.php
Note: httponly was introduced in PHP 5.2, so not sure if that should be a requirement (or already is) for 7.x-3.x.
Comment | File | Size | Author |
---|---|---|---|
#6 | webform-setcookie_httponly-d6.patch | 1.51 KB | quicksketch |
#6 | webform-setcookie_httponly-d7.patch | 1.69 KB | quicksketch |
#1 | webform-setcookie_httponly-1469530-1.patch | 1.56 KB | fmitchell |
Comments
Comment #1
fmitchell CreditAttribution: fmitchell commentedPatch attached.
Comment #2
quicksketchThanks, looks like a good idea to me. As for *requiring* PHP 5.2 just for this one precautionary change, I think that may be overkill. Since you can easily pass extra parameters to functions in PHP and they just get ignored, I think a backwards-compatible approach could be used here, like this:
That way httponly still exists even in PHP 5.1 (to prevent notices), and if it's passed into the function it doesn't do any harm.
Comment #3
fmitchell CreditAttribution: fmitchell commentedDrupal 7 requires 5.2 anyways, so we should be good without this. Probably only need this if you backport to D6.
http://drupal.org/requirements
Comment #4
quicksketchHm, I just realized this may cause some issues if a session isn't yet started, which seems like it may happen when an anonymous user submits a form and the page cache is enabled. I'll have to check this doesn't cause anything unexpected.
Comment #5
quicksketchOkay, anonymous users do not seem to be a problem as far as I can tell, but PHP 5.1 throws a nasty PHP warning:
I had expected it to just eat the extra parameter like normal PHP functions do, but because this is is not a user-defined function, it seems to have a problem with it. I think this overall patch is still a good idea, but it probably won't be backported to Drupal 6 with the httponly parameter intact. Still the ideas of setting the rest of the properties is a good idea I think. Right now we're only setting the cookie path for the page the user is already on for example, so the cookie won't work if the form is displayed on multiple pages with different paths.
Comment #6
quicksketchHere's the backport for D6 which doesn't include the thing you actually wanted, but all the other changes. I made a minor modification to the D7 patch to use REQUEST_TIME instead of the call to time(). Which somehow didn't get updated in our D7 upgrade (see http://drupal.org/node/224333#time).
Committed to both branches. Thanks for the patch, appreciated!
Comment #7
quicksketchTo be clear, the D6 patch does not include $params['httponly'] since PHP 5.1 does not support it. The D7 patch includes $params['httponly'], since as you pointed out, D7 already requires PHP 5.2.
Comment #8.0
(not verified) CreditAttribution: commentedadded link to example in core