Skip to content

Add patch from PHP-CSS-Parser to prevent malformed CSS properties leaking AMP validation error past the sanitizer #4290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 16, 2020
Merged

Add patch from PHP-CSS-Parser to prevent malformed CSS properties leaking AMP validation error past the sanitizer #4290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34dcbbe
Add patch from external PR that resolves the issue
pierlon Feb 14, 2020
8c8dfdd
Bump CSS cache group
pierlon Feb 14, 2020
1973ac3
Update patch
pierlon Feb 14, 2020
729a04a
Add tests
pierlon Feb 14, 2020
fad6040
Copy patches folder when building
pierlon Feb 16, 2020
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
2 changes: 2 additions & 0 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ module.exports = function( grunt ) {
// ⚠️ Warning: These paths are passed straight to rm command in the shell, without any escaping.
const productionVendorExcludedFilePatterns = [
'composer.*',
'patches',
'vendor/*/*/.editorconfig',
'vendor/*/*/.gitignore',
'vendor/*/*/composer.*',
Expand Down Expand Up @@ -150,6 +151,7 @@ module.exports = function( grunt ) {
paths.push( 'assets/js/*.js' ); // @todo Also include *.map files?
paths.push( 'assets/js/*.asset.php' );
paths.push( 'assets/css/*.css' );
paths.push( 'patches/*.patch' );

grunt.config.set( 'copy', {
build: {
Expand Down
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@
"extra": {
"patches": {
"sabberworm/php-css-parser": {
"PHP-CSS-Parser: Fix parsing CSS selectors which contain commas <https://github.com/sabberworm/PHP-CSS-Parser/pull/138>": "https://github.com/sabberworm/PHP-CSS-Parser/commit/fa139f65c5b098ae652c970b25e6eb03fc495eb4.diff"
"Fix parsing CSS selectors which contain commas <https://github.com/sabberworm/PHP-CSS-Parser/pull/138>": "https://github.com/sabberworm/PHP-CSS-Parser/commit/fa139f65c5b098ae652c970b25e6eb03fc495eb4.diff",
"Validate name-start code points for identifier <https://github.com/sabberworm/PHP-CSS-Parser/pull/185>": "patches/php-css-parser-pull-185.patch"
}
}
},
Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,7 +1441,7 @@ private function get_parsed_stylesheet( $stylesheet, $options = [] ) {
$parsed = null;
$cache_key = null;
$cached = true;
$cache_group = 'amp-parsed-stylesheet-v24'; // This should be bumped whenever the PHP-CSS-Parser is updated or parsed format is updated.
$cache_group = 'amp-parsed-stylesheet-v25'; // This should be bumped whenever the PHP-CSS-Parser is updated or parsed format is updated.

$cache_impacting_options = array_merge(
wp_array_slice_assoc(
Expand Down
364 changes: 364 additions & 0 deletions patches/php-css-parser-pull-185.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,364 @@
From d42b64793f2edaffeb663c63e9de79069cdc0831 Mon Sep 17 00:00:00 2001
From: Pierre Gordon <[email protected]>
Date: Wed, 22 Jan 2020 01:00:18 -0500
Subject: [PATCH 1/5] Validate name-start code points for identifier

---
lib/Sabberworm/CSS/Parsing/ParserState.php | 40 +++++++++++++++++-----
lib/Sabberworm/CSS/Value/Color.php | 2 +-
tests/Sabberworm/CSS/ParserTest.php | 13 +++++--
tests/files/-invalid-identifier.css | 6 ++++
4 files changed, 48 insertions(+), 13 deletions(-)
create mode 100644 tests/files/-invalid-identifier.css

diff --git a/lib/Sabberworm/CSS/Parsing/ParserState.php b/lib/Sabberworm/CSS/Parsing/ParserState.php
index ad79820..1914f22 100644
--- a/lib/Sabberworm/CSS/Parsing/ParserState.php
+++ b/lib/Sabberworm/CSS/Parsing/ParserState.php
@@ -48,8 +48,30 @@ public function getSettings() {
return $this->oParserSettings;
}

- public function parseIdentifier($bIgnoreCase = true) {
- $sResult = $this->parseCharacter(true);
+ public function parseIdentifier($bIgnoreCase = true, $bNameStartCodePoint = true) {
+ $sResult = null;
+ $bCanParseCharacter = true;
+
+ if ( $bNameStartCodePoint ) {
+ // Check if 3 code points would start an identifier. See <https://drafts.csswg.org/css-syntax-3/#would-start-an-identifier>.
+ $sNameStartCodePoint = '[a-zA-Z_]|[\x80-\xFF}]';
+ $sEscapeCode = '\\[^\r\n\f]';
+
+ if (
+ ! (
+ preg_match("/-([-${sNameStartCodePoint}]|${sEscapeCode})/isSu", $this->peek(3)) ||
+ preg_match("/${sNameStartCodePoint}/isSu", $this->peek()) ||
+ preg_match("/${sEscapeCode}/isS", $this->peek(2))
+ )
+ ) {
+ $bCanParseCharacter = false;
+ }
+ }
+
+ if ( $bCanParseCharacter ) {
+ $sResult = $this->parseCharacter(true);
+ }
+
if ($sResult === null) {
throw new UnexpectedTokenException($sResult, $this->peek(5), 'identifier', $this->iLineNo);
}
@@ -97,13 +119,13 @@ public function parseCharacter($bIsForIdentifier) {
}
if ($bIsForIdentifier) {
$peek = ord($this->peek());
- // Ranges: a-z A-Z 0-9 - _
+ // Matches a name code point. See <https://drafts.csswg.org/css-syntax-3/#name-code-point>.
if (($peek >= 97 && $peek <= 122) ||
($peek >= 65 && $peek <= 90) ||
($peek >= 48 && $peek <= 57) ||
($peek === 45) ||
($peek === 95) ||
- ($peek > 0xa1)) {
+ ($peek > 0x81)) {
return $this->consume(1);
}
} else {
@@ -261,22 +283,22 @@ public function strlen($sString) {
return mb_strlen($sString, $this->sCharset);
} else {
return strlen($sString);
- }
- }
+ }
+ }

private function substr($iStart, $iLength) {
if ($iLength < 0) {
$iLength = $this->iLength - $iStart + $iLength;
- }
+ }
if ($iStart + $iLength > $this->iLength) {
$iLength = $this->iLength - $iStart;
- }
+ }
$sResult = '';
while ($iLength > 0) {
$sResult .= $this->aText[$iStart];
$iStart++;
$iLength--;
- }
+ }
return $sResult;
}

diff --git a/lib/Sabberworm/CSS/Value/Color.php b/lib/Sabberworm/CSS/Value/Color.php
index c6ed9b1..f02777f 100644
--- a/lib/Sabberworm/CSS/Value/Color.php
+++ b/lib/Sabberworm/CSS/Value/Color.php
@@ -14,7 +14,7 @@ public static function parse(ParserState $oParserState) {
$aColor = array();
if ($oParserState->comes('#')) {
$oParserState->consume('#');
- $sValue = $oParserState->parseIdentifier(false);
+ $sValue = $oParserState->parseIdentifier(false, false);
if ($oParserState->strlen($sValue) === 3) {
$sValue = $sValue[0] . $sValue[0] . $sValue[1] . $sValue[1] . $sValue[2] . $sValue[2];
} else if ($oParserState->strlen($sValue) === 4) {
diff --git a/tests/Sabberworm/CSS/ParserTest.php b/tests/Sabberworm/CSS/ParserTest.php
index ea34f2e..16cae89 100644
--- a/tests/Sabberworm/CSS/ParserTest.php
+++ b/tests/Sabberworm/CSS/ParserTest.php
@@ -214,7 +214,7 @@ function testManipulation() {
$this->assertSame('#header {margin: 10px 2em 1cm 2%;color: red !important;frequency: 30Hz;}
body {color: green;}', $oDoc->render());
}
-
+
function testRuleGetters() {
$oDoc = $this->parsedStructureForFile('values');
$aBlocks = $oDoc->getAllDeclarationBlocks();
@@ -319,7 +319,7 @@ function testNamespaces() {
|test {gaga: 2;}';
$this->assertSame($sExpected, $oDoc->render());
}
-
+
function testInnerColors() {
$oDoc = $this->parsedStructureForFile('inner-color');
$sExpected = 'test {background: -webkit-gradient(linear,0 0,0 bottom,from(#006cad),to(hsl(202,100%,49%)));}';
@@ -359,7 +359,7 @@ function testListValueRemoval() {
$this->assertSame('@media screen {html {some: -test(val2);}}
#unrelated {other: yes;}', $oDoc->render());
}
-
+
/**
* @expectedException Sabberworm\CSS\Parsing\OutputException
*/
@@ -766,4 +766,11 @@ function testLonelyImport() {
$sExpected = "@import url(\"example.css\") only screen and (max-width: 600px);";
$this->assertSame($sExpected, $oDoc->render());
}
+
+ /**
+ * @expectedException \Sabberworm\CSS\Parsing\UnexpectedTokenException
+ */
+ function testInvalidIdentifier() {
+ $this->parsedStructureForFile('-invalid-identifier', Settings::create()->withLenientParsing(false));
+ }
}
diff --git a/tests/files/-invalid-identifier.css b/tests/files/-invalid-identifier.css
new file mode 100644
index 0000000..da00caf
--- /dev/null
+++ b/tests/files/-invalid-identifier.css
@@ -0,0 +1,6 @@
+body {
+ transition: all .3s ease-in-out;
+ -webkit-transition: all .3s ease-in-out;
+ -moz-transition: all .3s ease-in-out;
+ -0-transition: all .3s ease-in-out;
+}

From e031394fe3fc4448ed7e625e0c2b4ab334ad4ba2 Mon Sep 17 00:00:00 2001
From: Pierre Gordon <[email protected]>
Date: Sun, 26 Jan 2020 00:56:31 -0500
Subject: [PATCH 2/5] Make validation of identifier more strict

---
lib/Sabberworm/CSS/Parsing/ParserState.php | 8 +++----
tests/Sabberworm/CSS/ParserTest.php | 26 +++++++++++++++++++---
tests/files/-invalid-identifier.css | 6 -----
3 files changed, 27 insertions(+), 13 deletions(-)
delete mode 100644 tests/files/-invalid-identifier.css

diff --git a/lib/Sabberworm/CSS/Parsing/ParserState.php b/lib/Sabberworm/CSS/Parsing/ParserState.php
index 1914f22..2271d03 100644
--- a/lib/Sabberworm/CSS/Parsing/ParserState.php
+++ b/lib/Sabberworm/CSS/Parsing/ParserState.php
@@ -54,14 +54,14 @@ public function parseIdentifier($bIgnoreCase = true, $bNameStartCodePoint = true

if ( $bNameStartCodePoint ) {
// Check if 3 code points would start an identifier. See <https://drafts.csswg.org/css-syntax-3/#would-start-an-identifier>.
- $sNameStartCodePoint = '[a-zA-Z_]|[\x80-\xFF}]';
+ $sNameStartCodePoint = '[a-zA-Z_]|[\x80-\xFF]';
$sEscapeCode = '\\[^\r\n\f]';

if (
! (
- preg_match("/-([-${sNameStartCodePoint}]|${sEscapeCode})/isSu", $this->peek(3)) ||
- preg_match("/${sNameStartCodePoint}/isSu", $this->peek()) ||
- preg_match("/${sEscapeCode}/isS", $this->peek(2))
+ preg_match("/^-([-${sNameStartCodePoint}]|${sEscapeCode})/isSu", $this->peek(3)) ||
+ preg_match("/^${sNameStartCodePoint}/isSu", $this->peek()) ||
+ preg_match("/^${sEscapeCode}/isS", $this->peek(2))
)
) {
$bCanParseCharacter = false;
diff --git a/tests/Sabberworm/CSS/ParserTest.php b/tests/Sabberworm/CSS/ParserTest.php
index 16cae89..921209e 100644
--- a/tests/Sabberworm/CSS/ParserTest.php
+++ b/tests/Sabberworm/CSS/ParserTest.php
@@ -767,10 +767,30 @@ function testLonelyImport() {
$this->assertSame($sExpected, $oDoc->render());
}

+ function getInvalidIdentifiers() {
+ return array(
+ array(
+ 'body { -0-transition: all .3s ease-in-out; }',
+ 'Identifier expected. Got “-0-tr” [line no: 1]'
+ ),
+ array(
+ 'body { 4-o-transition: all .3s ease-in-out; }',
+ 'Identifier expected. Got “4-o-t” [line no: 1]'
+ )
+ );
+ }
+
/**
- * @expectedException \Sabberworm\CSS\Parsing\UnexpectedTokenException
+ * @dataProvider getInvalidIdentifiers
*/
- function testInvalidIdentifier() {
- $this->parsedStructureForFile('-invalid-identifier', Settings::create()->withLenientParsing(false));
+ function testInvalidIdentifier($css, $errorMessage) {
+ try {
+ $settings = Settings::create()->withLenientParsing(false);
+ $parser = new Parser($css, $settings);
+ $parser->parse();
+ $this->fail( 'UnexpectedTokenException not thrown' );
+ } catch ( UnexpectedTokenException $e ) {
+ $this->assertEquals( $errorMessage, $e->getMessage() );
+ }
}
}
diff --git a/tests/files/-invalid-identifier.css b/tests/files/-invalid-identifier.css
deleted file mode 100644
index da00caf..0000000
--- a/tests/files/-invalid-identifier.css
+++ /dev/null
@@ -1,6 +0,0 @@
-body {
- transition: all .3s ease-in-out;
- -webkit-transition: all .3s ease-in-out;
- -moz-transition: all .3s ease-in-out;
- -0-transition: all .3s ease-in-out;
-}

From 8fbd0fe82aa08ad2650def1b44f2f77154211e30 Mon Sep 17 00:00:00 2001
From: Pierre Gordon <[email protected]>
Date: Sun, 26 Jan 2020 01:08:21 -0500
Subject: [PATCH 3/5] Refactor `testInvalidIdentifier` test

---
tests/Sabberworm/CSS/ParserTest.php | 27 ++++++++++-----------------
1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/tests/Sabberworm/CSS/ParserTest.php b/tests/Sabberworm/CSS/ParserTest.php
index 921209e..68284ce 100644
--- a/tests/Sabberworm/CSS/ParserTest.php
+++ b/tests/Sabberworm/CSS/ParserTest.php
@@ -769,28 +769,21 @@ function testLonelyImport() {

function getInvalidIdentifiers() {
return array(
- array(
- 'body { -0-transition: all .3s ease-in-out; }',
- 'Identifier expected. Got “-0-tr” [line no: 1]'
- ),
- array(
- 'body { 4-o-transition: all .3s ease-in-out; }',
- 'Identifier expected. Got “4-o-t” [line no: 1]'
- )
+ array('body { -0-transition: all .3s ease-in-out; }' ),
+ array('body { 4-o-transition: all .3s ease-in-out; }' ),
);
}

/**
* @dataProvider getInvalidIdentifiers
+ *
+ * @param string $css CSS text.
*/
- function testInvalidIdentifier($css, $errorMessage) {
- try {
- $settings = Settings::create()->withLenientParsing(false);
- $parser = new Parser($css, $settings);
- $parser->parse();
- $this->fail( 'UnexpectedTokenException not thrown' );
- } catch ( UnexpectedTokenException $e ) {
- $this->assertEquals( $errorMessage, $e->getMessage() );
- }
+ function testInvalidIdentifier($css) {
+ $this->setExpectedException( 'Sabberworm\CSS\Parsing\UnexpectedTokenException' );
+
+ $oSettings = Settings::create()->withLenientParsing(false);
+ $oParser = new Parser($css, $oSettings);
+ $oParser->parse();
}
}

From 586c684a990458d70af55b47f584b619ad5c3a41 Mon Sep 17 00:00:00 2001
From: Pierre Gordon <[email protected]>
Date: Fri, 31 Jan 2020 15:55:54 -0500
Subject: [PATCH 4/5] Recover from invalid identifier if in lenient mode

---
lib/Sabberworm/CSS/Parsing/ParserState.php | 2 +-
tests/Sabberworm/CSS/ParserTest.php | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/Sabberworm/CSS/Parsing/ParserState.php b/lib/Sabberworm/CSS/Parsing/ParserState.php
index 2271d03..7ab8e01 100644
--- a/lib/Sabberworm/CSS/Parsing/ParserState.php
+++ b/lib/Sabberworm/CSS/Parsing/ParserState.php
@@ -72,7 +72,7 @@ public function parseIdentifier($bIgnoreCase = true, $bNameStartCodePoint = true
$sResult = $this->parseCharacter(true);
}

- if ($sResult === null) {
+ if (!$this->oParserSettings->bLenientParsing && $sResult === null) {
throw new UnexpectedTokenException($sResult, $this->peek(5), 'identifier', $this->iLineNo);
}
$sCharacter = null;
diff --git a/tests/Sabberworm/CSS/ParserTest.php b/tests/Sabberworm/CSS/ParserTest.php
index 68284ce..ff8c5c9 100644
--- a/tests/Sabberworm/CSS/ParserTest.php
+++ b/tests/Sabberworm/CSS/ParserTest.php
@@ -769,8 +769,8 @@ function testLonelyImport() {

function getInvalidIdentifiers() {
return array(
- array('body { -0-transition: all .3s ease-in-out; }' ),
- array('body { 4-o-transition: all .3s ease-in-out; }' ),
+ array('body { -0-transition: all .3s ease-in-out; }'),
+ array('body { 4-o-transition: all .3s ease-in-out; }'),
);
}


From 113df5d55e94e21c6402021dfa959924941d4c29 Mon Sep 17 00:00:00 2001
From: Pierre Gordon <[email protected]>
Date: Fri, 14 Feb 2020 04:20:16 -0500
Subject: [PATCH 5/5] Remove check of lenient parsing

The thrown exception will be caught when in lenient mode
---
lib/Sabberworm/CSS/Parsing/ParserState.php | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Sabberworm/CSS/Parsing/ParserState.php b/lib/Sabberworm/CSS/Parsing/ParserState.php
index 7ab8e01..2271d03 100644
--- a/lib/Sabberworm/CSS/Parsing/ParserState.php
+++ b/lib/Sabberworm/CSS/Parsing/ParserState.php
@@ -72,7 +72,7 @@ public function parseIdentifier($bIgnoreCase = true, $bNameStartCodePoint = true
$sResult = $this->parseCharacter(true);
}

- if (!$this->oParserSettings->bLenientParsing && $sResult === null) {
+ if ($sResult === null) {
throw new UnexpectedTokenException($sResult, $this->peek(5), 'identifier', $this->iLineNo);
}
$sCharacter = null;
Loading