ChrisW


The PHP implementation for Windows Live ID Web Authentication seems to contain a couple of bugs. The first bug is in lib/windowslivelogin.php. The parameter $flags is not defined in the function setFlags.

The function setFlags is defined on lines 87-93:

Code Snippet
private function setFlags()
{
$this->_usePersistentCookie = false;
if (preg_match('/^\d+$/', $flags)) {
$this->_usePersistentCookie = (($flags % 2) == 1);
}
}

The function uses the variable $flags, but this variable is not defined in the context. The call to setFlags on line 24 learns us that the function should have 1 parameter.

Code Snippet
self::setFlags($flags);

To fix this problem, the signature of the function on line 87 should be changed to:

Code Snippet
private function setFlags($flags)

The second bug is in index.php. On line 48 a check is performed to see if there is a name attribute is the HTTP request parameters, however this raises a warning if the name attribute is not defined in the parameters of the HTTP request.

Code Snippet
if (!$username && $_REQUEST['name']) {

A better way to check if the a variable is set, is by using the function isset. To fix this problem, line 48 should be changed to:

Code Snippet
if (!$username && isset($_REQUEST['name'])) {




Re: [BUG] Web Authentication - PHP

Navindra


Hi Chris,

The first is indeed a bug... your fix is right. In this release it will probably not have too much of an impact because the use persistent cookie flag is always going to be false currently... this is probably why it wasn't caught. We should fix this in the next release.

Thanks for pointing out the second issue with the warning. We could also use @$_REQUEST['name'] here instead of $_REQUEST['name'] if I'm not mistaken to be consistent with the rest of the code -- this should suppress the warning.

Thanks a lot for your feedback on these issues! We will file bugs and make sure they get addressed.

-N.






Re: [BUG] Web Authentication - PHP

Alex Media

In my opinion, suppressing an error (using the @) is worse than double-checking (using isset()), because you don't know if it is future-proof. I personally always check using

Code Snippet

if(isset($_GET['variable']) && $_GET['variable'] != "")

{

// code here

}

This way, I'll only execute code if the variable has something in it. I dislike the !$_GET['variable']-notation, it isn't clear enough in my opinion.






Re: [BUG] Web Authentication - PHP

Navindra

Hi Alex, your example is significantly more verbose than simply using @$... but perhaps it is the nature of PHP. Many of the other languages support the shorter idiom. Why do you say @$ is not future-proof

Thanks very much for your input either way... we can consider the style and best practice enhancements for the next release.





Re: [BUG] Web Authentication - PHP

Alex Media

It's just personal preference, I'd rather avoid errors than suppressing them, somehow I feel that's a bad thing to do.

@$ is indeed a really short version of writing something, but what happens when PHP is configured to ignore the @ and display errors (PHP is open source, so somebody might configure it to display all errors), that's why I prefer my solution, future-proof might have been the wrong word though.