Skip to content

Commit 3b764c0

Browse files
westonruterwestonruter
authored
Merge pull request #4290 from ampproject/fix/4113-malformed-css-props
Add patch from PHP-CSS-Parser to prevent malformed CSS properties leaking AMP validation error past the sanitizer
2 parents 16c3cc9 + fad6040 commit 3b764c0

File tree

5 files changed

+375
-2
lines changed

5 files changed

+375
-2
lines changed

‎Gruntfile.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ module.exports = function( grunt ) {
2727
// ⚠️ Warning: These paths are passed straight to rm command in the shell, without any escaping.
2828
const productionVendorExcludedFilePatterns = [
2929
'composer.*',
30+
'patches',
3031
'vendor/*/*/.editorconfig',
3132
'vendor/*/*/.gitignore',
3233
'vendor/*/*/composer.*',
@@ -150,6 +151,7 @@ module.exports = function( grunt ) {
150151
paths.push( 'assets/js/*.js' ); // @todo Also include *.map files?
151152
paths.push( 'assets/js/*.asset.php' );
152153
paths.push( 'assets/css/*.css' );
154+
paths.push( 'patches/*.patch' );
153155

154156
grunt.config.set( 'copy', {
155157
build: {

‎composer.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@
4141
"extra": {
4242
"patches": {
4343
"sabberworm/php-css-parser": {
44-
"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"
44+
"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",
45+
"Validate name-start code points for identifier <https://github.com/sabberworm/PHP-CSS-Parser/pull/185>": "patches/php-css-parser-pull-185.patch"
4546
}
4647
}
4748
},

‎includes/sanitizers/class-amp-style-sanitizer.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1441,7 +1441,7 @@ private function get_parsed_stylesheet( $stylesheet, $options = [] ) {
14411441
$parsed = null;
14421442
$cache_key = null;
14431443
$cached = true;
1444-
$cache_group = 'amp-parsed-stylesheet-v24'; // This should be bumped whenever the PHP-CSS-Parser is updated or parsed format is updated.
1444+
$cache_group = 'amp-parsed-stylesheet-v25'; // This should be bumped whenever the PHP-CSS-Parser is updated or parsed format is updated.
14451445

14461446
$cache_impacting_options = array_merge(
14471447
wp_array_slice_assoc(

‎patches/php-css-parser-pull-185.patch

Lines changed: 364 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,364 @@
1+
From d42b64793f2edaffeb663c63e9de79069cdc0831 Mon Sep 17 00:00:00 2001
2+
From: Pierre Gordon <[email protected]>
3+
Date: Wed, 22 Jan 2020 01:00:18 -0500
4+
Subject: [PATCH 1/5] Validate name-start code points for identifier
5+
6+
---
7+
lib/Sabberworm/CSS/Parsing/ParserState.php | 40 +++++++++++++++++-----
8+
lib/Sabberworm/CSS/Value/Color.php | 2 +-
9+
tests/Sabberworm/CSS/ParserTest.php | 13 +++++--
10+
tests/files/-invalid-identifier.css | 6 ++++
11+
4 files changed, 48 insertions(+), 13 deletions(-)
12+
create mode 100644 tests/files/-invalid-identifier.css
13+
14+
diff --git a/lib/Sabberworm/CSS/Parsing/ParserState.php b/lib/Sabberworm/CSS/Parsing/ParserState.php
15+
index ad79820..1914f22 100644
16+
--- a/lib/Sabberworm/CSS/Parsing/ParserState.php
17+
+++ b/lib/Sabberworm/CSS/Parsing/ParserState.php
18+
@@ -48,8 +48,30 @@ public function getSettings() {
19+
return $this->oParserSettings;
20+
}
21+
22+
- public function parseIdentifier($bIgnoreCase = true) {
23+
- $sResult = $this->parseCharacter(true);
24+
+ public function parseIdentifier($bIgnoreCase = true, $bNameStartCodePoint = true) {
25+
+ $sResult = null;
26+
+ $bCanParseCharacter = true;
27+
+
28+
+ if ( $bNameStartCodePoint ) {
29+
+ // Check if 3 code points would start an identifier. See <https://drafts.csswg.org/css-syntax-3/#would-start-an-identifier>.
30+
+ $sNameStartCodePoint = '[a-zA-Z_]|[\x80-\xFF}]';
31+
+ $sEscapeCode = '\\[^\r\n\f]';
32+
+
33+
+ if (
34+
+ ! (
35+
+ preg_match("/-([-${sNameStartCodePoint}]|${sEscapeCode})/isSu", $this->peek(3)) ||
36+
+ preg_match("/${sNameStartCodePoint}/isSu", $this->peek()) ||
37+
+ preg_match("/${sEscapeCode}/isS", $this->peek(2))
38+
+ )
39+
+ ) {
40+
+ $bCanParseCharacter = false;
41+
+ }
42+
+ }
43+
+
44+
+ if ( $bCanParseCharacter ) {
45+
+ $sResult = $this->parseCharacter(true);
46+
+ }
47+
+
48+
if ($sResult === null) {
49+
throw new UnexpectedTokenException($sResult, $this->peek(5), 'identifier', $this->iLineNo);
50+
}
51+
@@ -97,13 +119,13 @@ public function parseCharacter($bIsForIdentifier) {
52+
}
53+
if ($bIsForIdentifier) {
54+
$peek = ord($this->peek());
55+
- // Ranges: a-z A-Z 0-9 - _
56+
+ // Matches a name code point. See <https://drafts.csswg.org/css-syntax-3/#name-code-point>.
57+
if (($peek >= 97 && $peek <= 122) ||
58+
($peek >= 65 && $peek <= 90) ||
59+
($peek >= 48 && $peek <= 57) ||
60+
($peek === 45) ||
61+
($peek === 95) ||
62+
- ($peek > 0xa1)) {
63+
+ ($peek > 0x81)) {
64+
return $this->consume(1);
65+
}
66+
} else {
67+
@@ -261,22 +283,22 @@ public function strlen($sString) {
68+
return mb_strlen($sString, $this->sCharset);
69+
} else {
70+
return strlen($sString);
71+
- }
72+
- }
73+
+ }
74+
+ }
75+
76+
private function substr($iStart, $iLength) {
77+
if ($iLength < 0) {
78+
$iLength = $this->iLength - $iStart + $iLength;
79+
- }
80+
+ }
81+
if ($iStart + $iLength > $this->iLength) {
82+
$iLength = $this->iLength - $iStart;
83+
- }
84+
+ }
85+
$sResult = '';
86+
while ($iLength > 0) {
87+
$sResult .= $this->aText[$iStart];
88+
$iStart++;
89+
$iLength--;
90+
- }
91+
+ }
92+
return $sResult;
93+
}
94+
95+
diff --git a/lib/Sabberworm/CSS/Value/Color.php b/lib/Sabberworm/CSS/Value/Color.php
96+
index c6ed9b1..f02777f 100644
97+
--- a/lib/Sabberworm/CSS/Value/Color.php
98+
+++ b/lib/Sabberworm/CSS/Value/Color.php
99+
@@ -14,7 +14,7 @@ public static function parse(ParserState $oParserState) {
100+
$aColor = array();
101+
if ($oParserState->comes('#')) {
102+
$oParserState->consume('#');
103+
- $sValue = $oParserState->parseIdentifier(false);
104+
+ $sValue = $oParserState->parseIdentifier(false, false);
105+
if ($oParserState->strlen($sValue) === 3) {
106+
$sValue = $sValue[0] . $sValue[0] . $sValue[1] . $sValue[1] . $sValue[2] . $sValue[2];
107+
} else if ($oParserState->strlen($sValue) === 4) {
108+
diff --git a/tests/Sabberworm/CSS/ParserTest.php b/tests/Sabberworm/CSS/ParserTest.php
109+
index ea34f2e..16cae89 100644
110+
--- a/tests/Sabberworm/CSS/ParserTest.php
111+
+++ b/tests/Sabberworm/CSS/ParserTest.php
112+
@@ -214,7 +214,7 @@ function testManipulation() {
113+
$this->assertSame('#header {margin: 10px 2em 1cm 2%;color: red !important;frequency: 30Hz;}
114+
body {color: green;}', $oDoc->render());
115+
}
116+
-
117+
+
118+
function testRuleGetters() {
119+
$oDoc = $this->parsedStructureForFile('values');
120+
$aBlocks = $oDoc->getAllDeclarationBlocks();
121+
@@ -319,7 +319,7 @@ function testNamespaces() {
122+
|test {gaga: 2;}';
123+
$this->assertSame($sExpected, $oDoc->render());
124+
}
125+
-
126+
+
127+
function testInnerColors() {
128+
$oDoc = $this->parsedStructureForFile('inner-color');
129+
$sExpected = 'test {background: -webkit-gradient(linear,0 0,0 bottom,from(#006cad),to(hsl(202,100%,49%)));}';
130+
@@ -359,7 +359,7 @@ function testListValueRemoval() {
131+
$this->assertSame('@media screen {html {some: -test(val2);}}
132+
#unrelated {other: yes;}', $oDoc->render());
133+
}
134+
-
135+
+
136+
/**
137+
* @expectedException Sabberworm\CSS\Parsing\OutputException
138+
*/
139+
@@ -766,4 +766,11 @@ function testLonelyImport() {
140+
$sExpected = "@import url(\"example.css\") only screen and (max-width: 600px);";
141+
$this->assertSame($sExpected, $oDoc->render());
142+
}
143+
+
144+
+ /**
145+
+ * @expectedException \Sabberworm\CSS\Parsing\UnexpectedTokenException
146+
+ */
147+
+ function testInvalidIdentifier() {
148+
+ $this->parsedStructureForFile('-invalid-identifier', Settings::create()->withLenientParsing(false));
149+
+ }
150+
}
151+
diff --git a/tests/files/-invalid-identifier.css b/tests/files/-invalid-identifier.css
152+
new file mode 100644
153+
index 0000000..da00caf
154+
--- /dev/null
155+
+++ b/tests/files/-invalid-identifier.css
156+
@@ -0,0 +1,6 @@
157+
+body {
158+
+ transition: all .3s ease-in-out;
159+
+ -webkit-transition: all .3s ease-in-out;
160+
+ -moz-transition: all .3s ease-in-out;
161+
+ -0-transition: all .3s ease-in-out;
162+
+}
163+
164+
From e031394fe3fc4448ed7e625e0c2b4ab334ad4ba2 Mon Sep 17 00:00:00 2001
165+
From: Pierre Gordon <[email protected]>
166+
Date: Sun, 26 Jan 2020 00:56:31 -0500
167+
Subject: [PATCH 2/5] Make validation of identifier more strict
168+
169+
---
170+
lib/Sabberworm/CSS/Parsing/ParserState.php | 8 +++----
171+
tests/Sabberworm/CSS/ParserTest.php | 26 +++++++++++++++++++---
172+
tests/files/-invalid-identifier.css | 6 -----
173+
3 files changed, 27 insertions(+), 13 deletions(-)
174+
delete mode 100644 tests/files/-invalid-identifier.css
175+
176+
diff --git a/lib/Sabberworm/CSS/Parsing/ParserState.php b/lib/Sabberworm/CSS/Parsing/ParserState.php
177+
index 1914f22..2271d03 100644
178+
--- a/lib/Sabberworm/CSS/Parsing/ParserState.php
179+
+++ b/lib/Sabberworm/CSS/Parsing/ParserState.php
180+
@@ -54,14 +54,14 @@ public function parseIdentifier($bIgnoreCase = true, $bNameStartCodePoint = true
181+
182+
if ( $bNameStartCodePoint ) {
183+
// Check if 3 code points would start an identifier. See <https://drafts.csswg.org/css-syntax-3/#would-start-an-identifier>.
184+
- $sNameStartCodePoint = '[a-zA-Z_]|[\x80-\xFF}]';
185+
+ $sNameStartCodePoint = '[a-zA-Z_]|[\x80-\xFF]';
186+
$sEscapeCode = '\\[^\r\n\f]';
187+
188+
if (
189+
! (
190+
- preg_match("/-([-${sNameStartCodePoint}]|${sEscapeCode})/isSu", $this->peek(3)) ||
191+
- preg_match("/${sNameStartCodePoint}/isSu", $this->peek()) ||
192+
- preg_match("/${sEscapeCode}/isS", $this->peek(2))
193+
+ preg_match("/^-([-${sNameStartCodePoint}]|${sEscapeCode})/isSu", $this->peek(3)) ||
194+
+ preg_match("/^${sNameStartCodePoint}/isSu", $this->peek()) ||
195+
+ preg_match("/^${sEscapeCode}/isS", $this->peek(2))
196+
)
197+
) {
198+
$bCanParseCharacter = false;
199+
diff --git a/tests/Sabberworm/CSS/ParserTest.php b/tests/Sabberworm/CSS/ParserTest.php
200+
index 16cae89..921209e 100644
201+
--- a/tests/Sabberworm/CSS/ParserTest.php
202+
+++ b/tests/Sabberworm/CSS/ParserTest.php
203+
@@ -767,10 +767,30 @@ function testLonelyImport() {
204+
$this->assertSame($sExpected, $oDoc->render());
205+
}
206+
207+
+ function getInvalidIdentifiers() {
208+
+ return array(
209+
+ array(
210+
+ 'body { -0-transition: all .3s ease-in-out; }',
211+
+ 'Identifier expected. Got “-0-tr” [line no: 1]'
212+
+ ),
213+
+ array(
214+
+ 'body { 4-o-transition: all .3s ease-in-out; }',
215+
+ 'Identifier expected. Got “4-o-t” [line no: 1]'
216+
+ )
217+
+ );
218+
+ }
219+
+
220+
/**
221+
- * @expectedException \Sabberworm\CSS\Parsing\UnexpectedTokenException
222+
+ * @dataProvider getInvalidIdentifiers
223+
*/
224+
- function testInvalidIdentifier() {
225+
- $this->parsedStructureForFile('-invalid-identifier', Settings::create()->withLenientParsing(false));
226+
+ function testInvalidIdentifier($css, $errorMessage) {
227+
+ try {
228+
+ $settings = Settings::create()->withLenientParsing(false);
229+
+ $parser = new Parser($css, $settings);
230+
+ $parser->parse();
231+
+ $this->fail( 'UnexpectedTokenException not thrown' );
232+
+ } catch ( UnexpectedTokenException $e ) {
233+
+ $this->assertEquals( $errorMessage, $e->getMessage() );
234+
+ }
235+
}
236+
}
237+
diff --git a/tests/files/-invalid-identifier.css b/tests/files/-invalid-identifier.css
238+
deleted file mode 100644
239+
index da00caf..0000000
240+
--- a/tests/files/-invalid-identifier.css
241+
+++ /dev/null
242+
@@ -1,6 +0,0 @@
243+
-body {
244+
- transition: all .3s ease-in-out;
245+
- -webkit-transition: all .3s ease-in-out;
246+
- -moz-transition: all .3s ease-in-out;
247+
- -0-transition: all .3s ease-in-out;
248+
-}
249+
250+
From 8fbd0fe82aa08ad2650def1b44f2f77154211e30 Mon Sep 17 00:00:00 2001
251+
From: Pierre Gordon <[email protected]>
252+
Date: Sun, 26 Jan 2020 01:08:21 -0500
253+
Subject: [PATCH 3/5] Refactor `testInvalidIdentifier` test
254+
255+
---
256+
tests/Sabberworm/CSS/ParserTest.php | 27 ++++++++++-----------------
257+
1 file changed, 10 insertions(+), 17 deletions(-)
258+
259+
diff --git a/tests/Sabberworm/CSS/ParserTest.php b/tests/Sabberworm/CSS/ParserTest.php
260+
index 921209e..68284ce 100644
261+
--- a/tests/Sabberworm/CSS/ParserTest.php
262+
+++ b/tests/Sabberworm/CSS/ParserTest.php
263+
@@ -769,28 +769,21 @@ function testLonelyImport() {
264+
265+
function getInvalidIdentifiers() {
266+
return array(
267+
- array(
268+
- 'body { -0-transition: all .3s ease-in-out; }',
269+
- 'Identifier expected. Got “-0-tr” [line no: 1]'
270+
- ),
271+
- array(
272+
- 'body { 4-o-transition: all .3s ease-in-out; }',
273+
- 'Identifier expected. Got “4-o-t” [line no: 1]'
274+
- )
275+
+ array('body { -0-transition: all .3s ease-in-out; }' ),
276+
+ array('body { 4-o-transition: all .3s ease-in-out; }' ),
277+
);
278+
}
279+
280+
/**
281+
* @dataProvider getInvalidIdentifiers
282+
+ *
283+
+ * @param string $css CSS text.
284+
*/
285+
- function testInvalidIdentifier($css, $errorMessage) {
286+
- try {
287+
- $settings = Settings::create()->withLenientParsing(false);
288+
- $parser = new Parser($css, $settings);
289+
- $parser->parse();
290+
- $this->fail( 'UnexpectedTokenException not thrown' );
291+
- } catch ( UnexpectedTokenException $e ) {
292+
- $this->assertEquals( $errorMessage, $e->getMessage() );
293+
- }
294+
+ function testInvalidIdentifier($css) {
295+
+ $this->setExpectedException( 'Sabberworm\CSS\Parsing\UnexpectedTokenException' );
296+
+
297+
+ $oSettings = Settings::create()->withLenientParsing(false);
298+
+ $oParser = new Parser($css, $oSettings);
299+
+ $oParser->parse();
300+
}
301+
}
302+
303+
From 586c684a990458d70af55b47f584b619ad5c3a41 Mon Sep 17 00:00:00 2001
304+
From: Pierre Gordon <[email protected]>
305+
Date: Fri, 31 Jan 2020 15:55:54 -0500
306+
Subject: [PATCH 4/5] Recover from invalid identifier if in lenient mode
307+
308+
---
309+
lib/Sabberworm/CSS/Parsing/ParserState.php | 2 +-
310+
tests/Sabberworm/CSS/ParserTest.php | 4 ++--
311+
2 files changed, 3 insertions(+), 3 deletions(-)
312+
313+
diff --git a/lib/Sabberworm/CSS/Parsing/ParserState.php b/lib/Sabberworm/CSS/Parsing/ParserState.php
314+
index 2271d03..7ab8e01 100644
315+
--- a/lib/Sabberworm/CSS/Parsing/ParserState.php
316+
+++ b/lib/Sabberworm/CSS/Parsing/ParserState.php
317+
@@ -72,7 +72,7 @@ public function parseIdentifier($bIgnoreCase = true, $bNameStartCodePoint = true
318+
$sResult = $this->parseCharacter(true);
319+
}
320+
321+
- if ($sResult === null) {
322+
+ if (!$this->oParserSettings->bLenientParsing && $sResult === null) {
323+
throw new UnexpectedTokenException($sResult, $this->peek(5), 'identifier', $this->iLineNo);
324+
}
325+
$sCharacter = null;
326+
diff --git a/tests/Sabberworm/CSS/ParserTest.php b/tests/Sabberworm/CSS/ParserTest.php
327+
index 68284ce..ff8c5c9 100644
328+
--- a/tests/Sabberworm/CSS/ParserTest.php
329+
+++ b/tests/Sabberworm/CSS/ParserTest.php
330+
@@ -769,8 +769,8 @@ function testLonelyImport() {
331+
332+
function getInvalidIdentifiers() {
333+
return array(
334+
- array('body { -0-transition: all .3s ease-in-out; }' ),
335+
- array('body { 4-o-transition: all .3s ease-in-out; }' ),
336+
+ array('body { -0-transition: all .3s ease-in-out; }'),
337+
+ array('body { 4-o-transition: all .3s ease-in-out; }'),
338+
);
339+
}
340+
341+
342+
From 113df5d55e94e21c6402021dfa959924941d4c29 Mon Sep 17 00:00:00 2001
343+
From: Pierre Gordon <[email protected]>
344+
Date: Fri, 14 Feb 2020 04:20:16 -0500
345+
Subject: [PATCH 5/5] Remove check of lenient parsing
346+
347+
The thrown exception will be caught when in lenient mode
348+
---
349+
lib/Sabberworm/CSS/Parsing/ParserState.php | 2 +-
350+
1 file changed, 1 insertion(+), 1 deletion(-)
351+
352+
diff --git a/lib/Sabberworm/CSS/Parsing/ParserState.php b/lib/Sabberworm/CSS/Parsing/ParserState.php
353+
index 7ab8e01..2271d03 100644
354+
--- a/lib/Sabberworm/CSS/Parsing/ParserState.php
355+
+++ b/lib/Sabberworm/CSS/Parsing/ParserState.php
356+
@@ -72,7 +72,7 @@ public function parseIdentifier($bIgnoreCase = true, $bNameStartCodePoint = true
357+
$sResult = $this->parseCharacter(true);
358+
}
359+
360+
- if (!$this->oParserSettings->bLenientParsing && $sResult === null) {
361+
+ if ($sResult === null) {
362+
throw new UnexpectedTokenException($sResult, $this->peek(5), 'identifier', $this->iLineNo);
363+
}
364+
$sCharacter = null;

0 commit comments

Comments
 (0)