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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fmitchell’s picture

Patch attached.

quicksketch’s picture

Thanks, 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:

-      setcookie($cookie_name . '[' . $time . ']', $time, $time + $node->webform['submit_interval'] + 86400);
+      $params = session_get_cookie_params() + array('httponly' => FALSE);
+      setcookie($cookie_name . '[' . $time . ']', $time, $time + $node->webform['submit_interval'] + 86400, $params['path'], $params['domain'], $params['secure'], $params['httponly']);

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.

fmitchell’s picture

Drupal 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

quicksketch’s picture

Hm, 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.

quicksketch’s picture

Okay, anonymous users do not seem to be a problem as far as I can tell, but PHP 5.1 throws a nasty PHP warning:

Warning: setcookie() expects at most 6 parameters, 7 given in webform_client_form_submit() (line 2338 of /Users/nate/Sites/drupal6/sites/all/modules/webform/webform.module).

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.

quicksketch’s picture

Status: Active » Fixed
FileSize
1.69 KB
1.51 KB

Here'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!

quicksketch’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

added link to example in core