Page MenuHomePhabricator

Cover WebResponse::setCookie() with tests to prevent eg: Notice: Undefined variable: date in includes/WebResponse.php on line <i>144
Open, NormalPublic

Description

Due to https://gerrit.wikimedia.org/r/#/c/265928/2/includes/WebResponse.php,cm whenever the WebResponseSetCookie runs, the session yields:

Notice: Undefined variable: date in includes/WebResponse.php on line <i>144

4d6d06253b28ee1fac28301ef596d78c1ba7859b

Some block executed when the hook returns true reference the wrong variable $date instead of $data. That is easy to fix but I am wondering why:

  • none of the tests exercise the block of code triggered on WebResponseSetCookie
  • login via the API does trigger WebResponseSetCookie since it is not set afaik

Event Timeline

hashar created this task.Jan 25 2016, 10:22 AM
hashar updated the task description. (Show Details)
hashar raised the priority of this task from to Needs Triage.
hashar added subscribers: hashar, Tgr, Anomie, bd808.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptJan 25 2016, 10:22 AM

Change 266245 had a related patch set uploaded (by BryanDavis):
Fix typo in cookie key

https://gerrit.wikimedia.org/r/266245

Change 266245 merged by jenkins-bot:
Fix typo in cookie key

https://gerrit.wikimedia.org/r/266245

bd808 added a comment.Jan 25 2016, 4:18 PM

Typo fix is merged, but we need to do some thinking about how to actually test this code path under phpunit.

I have tried writing a test.

We use setcookie() which sets headers() and at the end of a test that is output by PHPUnit. That prevents the next test from invoking it or it will trigger the infamous 'headers already set' error.

Side note
The workaround is to use PHPUnit process isolation. That reset the internal PHP flag about headers that got emitted. PHPUnit does it by with a serialize() of the class to run which is written in a temp file and then sourced. When serializing an object having protected methods, null bytes are inserted and PHP 5.3 can't read a file having null bit (it outputs garbages to the buffer).

The easiest would be to let us inject the setcookie function to use in WebResponse. We vary based on $options['raw'] ? setrawcookie : setcookie and invoke that. If we added an option setcookieMethod, the test can then inject its own little method to keep track of headers being set with invoking headers().

The PHP bug

Given a file serializeme.php

<?php

class ClassWithProtected {
	protected $foobar;

}
print "<?php\nvar_dump(unserialize('" . serialize(new ClassWithProtected ) . "'));\n";

PHP 5.3

$ php -version
PHP 5.3.29 (cli) (built: Sep 21 2015 18:49:50) 
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2014 Zend Technologies

$ php serializeme.php |php
???????????????????????????????*f???????

PHP 5.5 from Mac OS X:

$ /usr/bin/php --version
PHP 5.5.30 (cli) (built: Oct 23 2015 17:21:45) 
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2015 Zend Technologies

$ php serializeme.php|/usr/bin/php
object(__PHP_Incomplete_Class)#1 (2) {
  ["__PHP_Incomplete_Class_Name"]=>
  string(18) "ClassWithProtected"
  ["foobar":protected]=>
  NULL
}

Teaching WebResponse to be injected a setCookie method is imho the best approach.

Change 266420 had a related patch set uploaded (by Hashar):
test for WebResponse::setCookie

https://gerrit.wikimedia.org/r/266420

https://gerrit.wikimedia.org/r/266420 has a basic test which would have caught the typo by exercising the MediaWiki core code (just have to clear the hook). See my cover message on Gerrit and implementation rational in the test file :-}

hashar renamed this task from Notice: Undefined variable: date in includes/WebResponse.php on line <i>144 to Cover WebResponse::setCookie() with tests to prevent eg: Notice: Undefined variable: date in includes/WebResponse.php on line <i>144.Jan 25 2016, 10:41 PM
hashar triaged this task as Normal priority.
hashar set Security to None.

Change 266424 had a related patch set uploaded (by BryanDavis):
Fix typo in cookie key

https://gerrit.wikimedia.org/r/266424

Change 266424 merged by jenkins-bot:
Fix typo in cookie key

https://gerrit.wikimedia.org/r/266424

Change 266420 abandoned by Hashar:
test for WebResponse::setCookie

https://gerrit.wikimedia.org/r/266420