Exakat 0.12.16 review

Exakat 0.12.16 review

Exakat 0.12.16 is the version before Zend Con week ! We’ll be attending and speaking at Zend Con next week, then to PHP Québec, by the end of October. Come and let’s have a chat about code reviews ! This week, we upgraded Zend Framework 3’s analysis with all the most recent component version. We also added several vicious analysis : is zero, expression too complex and unconditional breaks in loops. Thanks to Peter Breuls for pushing us with ‘expression too complex’ and to Nicolas Grekas for ‘Session Handler must implements SessionUpdateTimestampHandlerInterface’, a little-know PHP interface for Session handlers. Let ‘s review that together!

Unconditional break in loops

Breaks may happens within a loop to prevent it from wasting time on later cycles. But when the break is unconditional, this leads to a very weird kind of loop.

<?php

function foo($array) {
	foreach($array as $item) {
		return $item;
	}
}
?>

In the example above, the ‘break’ made actually with ‘return’, as return jumps out of the function AND the loop. So, break really means break, continue, return or goto.

Besides, the foo() function here is actually a long version for array_values($array)[0].

Conditional breaks are valid, and probably the expected way to write those loops.

<?php

function foo($array) {
	foreach($array as $item) {
		// Check $item
		if (bar($item)) {
			return $item;
		}
		continue; // Actually, continue is superfluous here.
	}
}
?>

After poking various codes, it appears that this analysis report issues in at least 10 % of code, so this is not exceptional coding.

Is Zero

‘Is zero’ spots additions that are including + $a and – $a. This looks obvious when reading simple code, but may get tricky when the addition gets longer.

<?php

// This is quite obvious
$a = 2 - 2;

// This is less obvious
$a = $b[3] - $c + $d->foo(1,2,3) + $c + $b[3];

// This is obvious too but may be a typo
$diff = $fx[9][4] - $fx[9][4];

?>

If the first example is obvious, then it may very well be forgotten in the code: it is some development artifact is lying in the code. This is quite harmless, though a bit ridiculous.

The second example is typical of complex expression that are growing with code maturity, and that should be reviewed.

The third example is most probably a typo. May be the real code was comparing [4] and [5] (or [3] and [4]), but a typo made it equal, leading to a constant 0. This is a real bug.

Finally, we could also build a ‘Is One’, which applies the same concept to Multiplication: whenever a large product has both multiplication and division by $a, may be simplified. However, large products are quite rare, so this may be overkill.

Expression too complex

While chatting with Peter Breuls during 010 PHP about the ‘is zero’ analysis, he pointed out that long addition are hard to read anyway. We already had an analysis in the work that could spot complex expression: here is it.

<?php
fwrite($fp, 'HEAD ' . @$url['path'] . @$url['query'] . ' HTTP/1.0' . "\r\n" . 'Host: ' . @$url['host'] . "\r\n\r\n");
?>

In the example above, the ‘complex expression’ is the 2nd argument, which has arrays, literals, noscreams and concatenations. In total, 15 elements for that expression, which is the actual lower limit. Noscream and literals counts as one, but arrays (or properties, method calls) count as two elements : one for variable, one for index.

On top of that, the expression may be upgraded with several checks, instead of using @ as the go-to data filtering. Remember that @ is actually slower than checking the value with isset() and defaulting it.

Complex expressions appear in arguments, conditions, and assignations. They require 15 elements or more. They definitely attract attention to important part of the code.

Session Handlers must implements SessionUpdateTimestampHandlerInterface

Thanks to Nicolas Grekas, from Blackfire, a little known interface will improve your session handler security: https://github.com/symfony/symfony/pull/24523. Since PHP 7.0, it is possible to provide methods to handle ‘lazy reading’ from userland handler.

<?php

class mySessionHandler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface{
	// Don't forget the  SessionHandlerInterface methods
	function validateId($sessionId){}
	function updateTimestamp($key, $value)  {}
}
?>

updateTimestamp is called for read session variables, so one may update the last-accessed timestamp. This way, the session may keep track of actual usage, even if nothing happens but data reading.

validateId($sessionId) is given a session ID, and reports if a matching session is found or not. This prevents session fixation: the attacker generate the ID, and for the victim to use it, creating a session on the server which he already knows. ValidateId() is your chance to update that.

Documentation is still scarce on the php.net about this interface, which has been available since PHP 7.0. So, take a look at what did Symfony, and update your security!

Happy PHP code reviews

Next week, we’ll be at Zend Con 2017, talking about ‘reviewing unknown PHP code’ and ‘static analysis saved my code tonight’. Exakat features a zend-framework specific report, including a review of all used component, and recommendations for version upgrade. What to see more ? Ping us @exakat, and let’s meet !

All the 320+ analyzers are presented in the docs, including the common ‘No Noscream‘, which suggest speeding up code by removing all @ usage. Try Exakat on exakat.io, download it it on https://github.com/exakat/exakat or upgrade it with ‘exakat.phar upgrade -u’.