cl/394490119 Fix a bug where the CSS parser was not correctly account…

…ing for the possibility of calc() in the media expression. (ampproject#35937)
BroadBandyDev · Sep 9, 2021 · db9180f · db9180f
1 parent 1edb48d
commit db9180f
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 13 deletions.
diff --git a/validator/js/engine/parse-css.js b/validator/js/engine/parse-css.js
@@ -1297,6 +1297,35 @@ class MediaQueryVisitor extends RuleVisitor {
     return false;
   }
 
+  /**
+   * token_stream->Current() must be a FUNCTION_TOKEN. Consumes all Tokens up
+   * to and including the matching closing paren for that FUNCTION_TOKEN.
+   * If false, recursion exceeded maximum depth.
+   * @param {!TokenStream} tokenStream
+   * @param {number} depth
+   * @returns {boolean}
+   */
+  consumeAFunction_(tokenStream, depth) {
+    if (depth > kMaximumCssRecursion) return false;
+    if (tokenStream.current().tokenType !=
+        tokenize_css.TokenType.FUNCTION_TOKEN)
+      return false;
+    tokenStream.consume();  // FUNCTION_TOKEN
+    while (tokenStream.current().tokenType !=
+           tokenize_css.TokenType.EOF_TOKEN) {
+      const type = tokenStream.current().tokenType;
+      if (type == tokenize_css.TokenType.FUNCTION_TOKEN) {
+        if (!this.consumeAFunction_(tokenStream, depth + 1)) return false;
+      } else if (type == tokenize_css.TokenType.CLOSE_PAREN) {
+        tokenStream.consume();
+        return true;
+      } else {
+        tokenStream.consume();
+      }
+    }
+    return false;  // EOF before function CLOSE_PAREN
+  }
+
   /**
    * Parse a media expression
    * @param {!TokenStream} tokenStream
@@ -1321,19 +1350,18 @@ class MediaQueryVisitor extends RuleVisitor {
       // The CSS3 grammar at this point just tells us to expect some
       // expr. Which tokens are accepted here are defined by the media
       // feature found above. We don't implement media features here, so
-      // we just loop over tokens until we find a CLOSE_PAREN or EOF.
-      // While expr in general may have arbitrary sets of open/close parens,
-      // it seems that https://www.w3.org/TR/css3-mediaqueries/#media1
-      // suggests that media features cannot:
-      //
-      // "Media features only accept single values: one keyword, one number,
-      // or a number with a unit identifier. (The only exceptions are the
-      // ‘aspect-ratio’ and ‘device-aspect-ratio’ media features.)
-      while (tokenStream.current().tokenType !==
-                 tokenize_css.TokenType.EOF_TOKEN &&
-             tokenStream.current().tokenType !==
-                 tokenize_css.TokenType.CLOSE_PAREN) {
-        tokenStream.consume();
+      // we just loop over tokens until we find a CLOSE_PAREN or EOF while
+      // handling nested functions like calc().
+      while (tokenStream.current().tokenType !=
+             tokenize_css.TokenType.EOF_TOKEN) {
+        const type = tokenStream.current().tokenType;
+        if (type == tokenize_css.TokenType.CLOSE_PAREN) {
+          break;
+        } else if (type == tokenize_css.TokenType.FUNCTION_TOKEN) {
+          if (!this.consumeAFunction_(tokenStream, 0)) return false;
+        } else {
+          tokenStream.consume();
+        }
       }
     }
     if (tokenStream.current().tokenType !==

diff --git a/validator/js/engine/parse-css_test.js b/validator/js/engine/parse-css_test.js
@@ -1206,6 +1206,7 @@ describe('parseMediaQueries', () => {
       'only screen and (color)',
       'NOT screen AND (color)',
       'screen \t \n , \t \n braille',
+      '(min-width: calc(840px - 48px))',
     ];
 
     for (const testcase of cases) {

diff --git a/validator/testdata/feature_tests/media_expression_calc.html b/validator/testdata/feature_tests/media_expression_calc.html
@@ -0,0 +1,23 @@
+<!--
+  Test Description:
+  This tests that CSS media expression parsing correctly handles calc(),
+  specifically this bug:
+  https://github.com/ampproject/amphtml/issues/35793
+-->
+<!doctype html>
+<html ⚡>
+<head>
+  <meta charset="utf-8">
+  <link rel="canonical" href="./regular-html-version.html">
+  <meta name="viewport" content="width=device-width">
+  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+  <style amp-custom>
+    @media (min-width: calc(840px - 48px)) {}
+    @media (min-width: calc(840px - calc(24px + 24px))) {}
+  </style>
+</head>
+<body>
+Hello, world.
+</body>
+</html>
diff --git a/validator/testdata/feature_tests/media_expression_calc.out b/validator/testdata/feature_tests/media_expression_calc.out
@@ -0,0 +1,24 @@
+PASS
+|  <!--
+|    Test Description:
+|    This tests that CSS media expression parsing correctly handles calc(),
+|    specifically this bug:
+|    https://github.com/ampproject/amphtml/issues/35793
+|  -->
+|  <!doctype html>
+|  <html ⚡>
+|  <head>
+|    <meta charset="utf-8">
+|    <link rel="canonical" href="./regular-html-version.html">
+|    <meta name="viewport" content="width=device-width">
+|    <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+|    <script async src="https://cdn.ampproject.org/v0.js"></script>
+|    <style amp-custom>
+|      @media (min-width: calc(840px - 48px)) {}
+|      @media (min-width: calc(840px - calc(24px + 24px))) {}
+|    </style>
+|  </head>
+|  <body>
+|  Hello, world.
+|  </body>
+|  </html>