Skip to content
This repository has been archived by the owner on Jul 20, 2018. It is now read-only.

Add checks for batcache variant #282

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions tests/checks/test-BatcacheVariantCheck.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php

require_once( 'CodeCheckTestBase.php' );

class BatcacheVariantTest extends CodeCheckTestBase {

public function testBatcacheVariant() {
$expected_errors = array(
array(
'slug' => 'batcache-variant-error',
'level' => BaseScanner::LEVEL_BLOCKER,
'description' => 'Illegal word in variant determiner.',
'file' => 'BatcacheVariantTest.inc',
'lines' => 7,
),
array(
'slug' => 'batcache-variant-error',
'level' => BaseScanner::LEVEL_BLOCKER,
'description' => 'Variant determiner should refer to at least one $_ variable.',
'file' => 'BatcacheVariantTest.inc',
'lines' => 7,
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to check for the error than duplicating the entire error here? What if we change the message in the check itself?

);
$actual_errors = $this->checkFile( 'BatcacheVariantTest.inc' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive tests should also prove that the code works as expected when there are no errors.

$this->assertEqualErrors( $expected_errors, $actual_errors );
}

}
12 changes: 12 additions & 0 deletions tests/data/BatcacheVariantTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

die(); //Don't actually run the following code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can safely omit the die()...this only gets read in as text, not included/required as PHP so it won't be executed.


function is_feedburner() {
if ( function_exists( 'vary_cache_on_function' ) ) {
vary_cache_on_function( 'require("something_dangerous.php");' );
}
return (bool) preg_match("/feedburner|feedvalidator/i", $_SERVER["HTTP_USER_AGENT"]);
}


46 changes: 46 additions & 0 deletions vip-scanner/checks/BatcacheVariantCheck.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?php
/**
* Checks for invalid batcache variant function vary_cache_on_function() that:
*
* - includes dangerous illegal functions
* - does not reference superglobal variable
*
*/

class BatcacheVariantCheck extends CodeCheck {

function __construct() {
parent::__construct( array(
'PhpParser\Node\Expr\FuncCall' => function( $node ) {
$name = $node->name->toString();
if ( 'vary_cache_on_function' == $name ) {

$value = $node->args[0]->value;

if ( $value instanceof PhpParser\Node\Scalar\String ) {

$function_string = $value->value;

if ( preg_match('/include|require|echo|print|dump|export|open|sock|unlink|`|eval/i', $function_string) ) {
$this->add_error(
'batcache-variant-error',
'Illegal word in variant determiner.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we list the invalid words here so folks know what the issue was?

BaseScanner::LEVEL_BLOCKER
);
}

if ( !preg_match('/\$_/', $function_string) ) {
$this->add_error(
'batcache-variant-error',
'Variant determiner should refer to at least one $_ variable.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should mention superglobals

BaseScanner::LEVEL_BLOCKER
);
}
}
}
}
) );
}


}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newlines are inconsistent in this file - two at the end, sometimes an extra line after class / if, other times not - can we get that fixed to match the WP standards / rest of the project?

2 changes: 2 additions & 0 deletions vip-scanner/config-vip-scanner.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

VIP_Scanner::get_instance()->register_review( 'WP.com Theme Review', array(
'AtPackageCheck',
'BatCacheVariantCheck',
'BodyClassCheck',
'CDNCheck',
'CheckedSelectedDisabledCheck',
Expand Down Expand Up @@ -54,6 +55,7 @@
) );

VIP_Scanner::get_instance()->register_review( 'VIP Theme Review', array(
'BatCacheVariantCheck',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a typo in these class names - the second C is capitalized, but not in the class names (shouldn't be capitalized anywhere).

'CheckedSelectedDisabledCheck',
'DeprecatedConstantsCheck',
'DeprecatedFunctionsCheck',
Expand Down