From 60b72edfba8a0fbe887f86dc26eb572e02b9ec19 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Wed, 13 Mar 2013 11:26:49 +1300 Subject: [PATCH 1/3] FIX Parsing heredoc, nowdoc & comments in ConfigStaticManifest --- core/manifest/ConfigStaticManifest.php | 5 ++- .../manifest/ConfigStaticManifestTest.php | 33 +++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php index 486871e10..339a88e3a 100644 --- a/core/manifest/ConfigStaticManifest.php +++ b/core/manifest/ConfigStaticManifest.php @@ -268,6 +268,9 @@ class SS_ConfigStaticManifest_Parser { else if($type == ';' || $type == ',' || $type == '=') { break; } + else if($type == T_COMMENT) { + // NOP + } else { user_error('Unexpected token when building static manifest: '.print_r($token, true), E_USER_ERROR); } @@ -317,7 +320,7 @@ class SS_ConfigStaticManifest_Parser { $this->statics[$class][$variable] = array( 'access' => $access, - 'value' => eval('return '.$value.';') + 'value' => eval('return '.trim($value).";\n") ); if($token == ',') $this->parseStatic($access, $class); diff --git a/tests/core/manifest/ConfigStaticManifestTest.php b/tests/core/manifest/ConfigStaticManifestTest.php index ceaa13da1..fdd5aab83 100644 --- a/tests/core/manifest/ConfigStaticManifestTest.php +++ b/tests/core/manifest/ConfigStaticManifestTest.php @@ -20,9 +20,20 @@ class ConfigStaticManifestTest extends SapphireTest { static $sfloat = 2.5; static $sstring = 'string'; static $sarray = array(1, 2, array(3, 4), 5); + static $sheredoc = <<parseSelf()->getStatics(); + $this->assertEquals(self::$commented_int, $statics[__CLASS__]['commented_int']['value']); + $this->assertEquals(self::$commented_string, $statics[__CLASS__]['commented_string']['value']); + } + public function testIgnoresMethodStatics() { $statics = $this->parseSelf()->getStatics(); $this->assertNull(@$statics[__CLASS__]['method_static']); From 53595dc930ea0505ca822a571c3f81218bb860b3 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Wed, 13 Mar 2013 11:59:49 +1300 Subject: [PATCH 2/3] FIX Parsing docblock comments in ConfigStaticManifest --- core/manifest/ConfigStaticManifest.php | 2 +- tests/core/manifest/ConfigStaticManifestTest.php | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php index 339a88e3a..074bdbd0a 100644 --- a/core/manifest/ConfigStaticManifest.php +++ b/core/manifest/ConfigStaticManifest.php @@ -268,7 +268,7 @@ class SS_ConfigStaticManifest_Parser { else if($type == ';' || $type == ',' || $type == '=') { break; } - else if($type == T_COMMENT) { + else if($type == T_COMMENT || $type == T_DOC_COMMENT) { // NOP } else { diff --git a/tests/core/manifest/ConfigStaticManifestTest.php b/tests/core/manifest/ConfigStaticManifestTest.php index fdd5aab83..5655599f6 100644 --- a/tests/core/manifest/ConfigStaticManifestTest.php +++ b/tests/core/manifest/ConfigStaticManifestTest.php @@ -57,6 +57,14 @@ DOC; static /* Has comment inline */ $commented_int = 1, /* And here */ $commented_string = 'string'; + static + /** + * Has docblock inline + */ + $docblocked_int = 1, + /** And here */ + $docblocked_string = 'string'; + // Should ignore static methpds static function static_method() {} @@ -136,8 +144,12 @@ DOC; public function testIgnoreComments() { $statics = $this->parseSelf()->getStatics(); + $this->assertEquals(self::$commented_int, $statics[__CLASS__]['commented_int']['value']); $this->assertEquals(self::$commented_string, $statics[__CLASS__]['commented_string']['value']); + + $this->assertEquals(self::$docblocked_int, $statics[__CLASS__]['docblocked_int']['value']); + $this->assertEquals(self::$docblocked_string, $statics[__CLASS__]['docblocked_string']['value']); } public function testIgnoresMethodStatics() { From d8a1df4312fb717f95cedbfa8babcb403f0b51ee Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Wed, 13 Mar 2013 12:42:48 +1300 Subject: [PATCH 3/3] Further secure eval call in ConfigStaticManifest It shouldnt be possible to get ConfigStaticManifest to parse a user uploaded file, and if you could it shouldnt be possible to form PHP that token_get_all could parse which would end up executing any code. However just in case it is, this changes the eval to assign to a static, so the eval will give a syntax error if an attacker manages to make $value look like `ls` or some other expression --- core/manifest/ConfigStaticManifest.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php index 074bdbd0a..3689f59c8 100644 --- a/core/manifest/ConfigStaticManifest.php +++ b/core/manifest/ConfigStaticManifest.php @@ -318,9 +318,17 @@ class SS_ConfigStaticManifest_Parser { $this->statics[$class] = array(); } + $value = trim($value); + if ($value) { + $value = eval('static $temp = '.$value.";\n".'return $temp'.";\n"); + } + else { + $value = null; + } + $this->statics[$class][$variable] = array( 'access' => $access, - 'value' => eval('return '.trim($value).";\n") + 'value' => $value ); if($token == ',') $this->parseStatic($access, $class);