Validator rollup (#29830)

* cl/324926702 Add value_regex support for css declarations. * cl/325128764 Fix css "bug" where declaration::FirstIdent doesn't return numerical identifiers. * cl/325141241 Fix AMP4Email CSS spec by ensuring only properties that should be allowed in emails are allowlisted. * cl/325253809 Improve `!important` CSS marker error message * cl/325256844 Revision bump for #29643 * cl/325359140 Validator: Add support for link imagesrcset and imagesizes * cl/325491434 Replace INVALID_DOCTYPE_HTML with a new message. * cl/325899515 Ban max-entries for AMP for Email * cl/326083373 Revert removal of requires_usage and deprecated_allow_duplicates for email * cl/326527018 Revision bump for #29766 * cl/326533269 Revision bump for #29748 Co-authored-by: Greg Grothaus <greggrothaus@google.com> Co-authored-by: honeybadgerdontcare <sedano@google.com> Co-authored-by: Justin Ridgewell <jridgewell@google.com>
ampproject · Aug 13, 2020 · 4d5e5d3 · 4d5e5d3
1 parent 09517c5
commit 4d5e5d3
Show file tree

Hide file tree

Showing 25 changed files with 1,320 additions and 501 deletions.
diff --git a/extensions/amp-accordion/validator-amp-accordion.protoascii b/extensions/amp-accordion/validator-amp-accordion.protoascii
@@ -34,6 +34,8 @@ tags: {  # amp-accordion
     name: "amp-accordion"
     # AMP4EMAIL doesn't allow version: "latest".
     version: "0.1"
+    requires_usage: EXEMPTED
+    deprecated_allow_duplicates: true
   }
   attr_lists: "common-extension-attrs"
 }

diff --git a/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii b/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii
@@ -105,7 +105,6 @@ tags: {  # <amp-autocomplete>
   attrs: { name: "highlight-user-entry" }
   attrs: { name: "inline" }
   attrs: { name: "items" }
-  attrs: { name: "max-entries" }
   attrs: { name: "max-items" }
   attrs: { name: "min-characters" }
   attrs: {

diff --git a/extensions/amp-fit-text/validator-amp-fit-text.protoascii b/extensions/amp-fit-text/validator-amp-fit-text.protoascii
@@ -35,6 +35,8 @@ tags: {  # amp-fit-text (AMP4EMAIL)
     name: "amp-fit-text"
     # AMP4EMAIL doesn't allow version: "latest".
     version: "0.1"
+    requires_usage: EXEMPTED
+    deprecated_allow_duplicates: true
   }
   attr_lists: "common-extension-attrs"
 }

diff --git a/extensions/amp-form/validator-amp-form.protoascii b/extensions/amp-form/validator-amp-form.protoascii
@@ -36,6 +36,8 @@ tags: {  # amp-form
     name: "amp-form"
     # AMP4EMAIL doesn't allow version: "latest".
     version: "0.1"
+    deprecated_allow_duplicates: true
+    requires_usage: EXEMPTED
   }
   attr_lists: "common-extension-attrs"
 }
diff --git a/validator/js/engine/parse-css.js b/validator/js/engine/parse-css.js
@@ -353,7 +353,8 @@ const Declaration = class extends Rule {
 
   /**
    * For a declaration, if the first non-whitespace token is an identifier,
-   * returns its string value. Otherwise, returns the empty string.
+   * (including a number token type), returns its string value. Otherwise,
+   * returns the empty string.
    * @return {string}
    */
   firstIdent() {
@@ -364,11 +365,18 @@ const Declaration = class extends Rule {
       return /** @type {!tokenize_css.StringValuedToken} */ (this.value[0])
           .value;
     }
+    if (this.value[0].tokenType === tokenize_css.TokenType.NUMBER) {
+      return /** @type {!tokenize_css.NumberToken} */ (this.value[0]).repr;
+    }
     if (this.value.length >= 2 &&
-        (this.value[0].tokenType === tokenize_css.TokenType.WHITESPACE) &&
-        this.value[1].tokenType === tokenize_css.TokenType.IDENT) {
-      return /** @type {!tokenize_css.StringValuedToken} */ (this.value[1])
-          .value;
+        (this.value[0].tokenType === tokenize_css.TokenType.WHITESPACE)) {
+      if (this.value[1].tokenType === tokenize_css.TokenType.IDENT) {
+        return /** @type {!tokenize_css.StringValuedToken} */ (this.value[1])
+            .value;
+      }
+      if (this.value[1].tokenType === tokenize_css.TokenType.NUMBER) {
+        return /** @type {!tokenize_css.NumberToken} */ (this.value[1]).repr;
+      }
     }
     return '';
   }

diff --git a/validator/js/engine/validator.js b/validator/js/engine/validator.js
@@ -2087,28 +2087,62 @@ class InvalidRuleVisitor extends parse_css.RuleVisitor {
 /** @private */
 class InvalidDeclVisitor extends parse_css.RuleVisitor {
   /**
-   * @param {!ParsedDocCssSpec} spec
+   * @param {!ParsedDocCssSpec} cssSpec
    * @param {!Context} context
+   * @param {string} tagDescriptiveName
    * @param {!generated.ValidationResult} result
    */
-  constructor(spec, context, result) {
+  constructor(cssSpec, context, tagDescriptiveName, result) {
     super();
     /** @type {!ParsedDocCssSpec} */
-    this.spec = spec;
+    this.cssSpec = cssSpec;
     /** @type {!Context} */
     this.context = context;
+    /** @type {string} */
+    this.tagDescriptiveName = tagDescriptiveName;
     /** @type {!generated.ValidationResult} */
     this.result = result;
   }
 
   /** @inheritDoc */
   visitDeclaration(declaration) {
-    if (!this.spec.cssDeclarationByName(declaration.name)) {
+    const cssDeclaration = this.cssSpec.cssDeclarationByName(declaration.name);
+    const firstIdent = declaration.firstIdent();
+    const lineCol = new LineCol(declaration.line, declaration.col);
+    if (!cssDeclaration) {
       this.context.addError(
           generated.ValidationError.Code.CSS_SYNTAX_INVALID_PROPERTY_NOLIST,
-          new LineCol(declaration.line, declaration.col),
-          ['style amp-custom', declaration.name], this.spec.spec().specUrl,
-          this.result);
+          lineCol, [this.tagDescriptiveName, declaration.name],
+          this.cssSpec.spec().specUrl, this.result);
+      // Don't emit additional errors for this declaration
+      return;
+    }
+    if (cssDeclaration.valueCasei.length > 0) {
+      let hasValidValue = false;
+      for (const value of cssDeclaration.valueCasei) {
+        if (firstIdent.toLowerCase() == value) {
+          hasValidValue = true;
+          break;
+        }
+      }
+      if (!hasValidValue) {
+        // Declaration value not allowed.
+        this.context.addError(
+            generated.ValidationError.Code.CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE,
+            lineCol, /* params */
+            [this.tagDescriptiveName, declaration.name, firstIdent],
+            this.cssSpec.spec().specUrl, this.result);
+      }
+    } else if (cssDeclaration.valueRegexCasei != null) {
+      const valueRegex = this.context.getRules().getFullMatchCaseiRegex(
+          /** @type {number} */ (cssDeclaration.valueRegexCasei));
+      if (!valueRegex.test(firstIdent)) {
+        this.context.addError(
+            generated.ValidationError.Code.CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE,
+            lineCol, /* params */
+            [this.tagDescriptiveName, declaration.name, firstIdent],
+            this.cssSpec.spec().specUrl, this.result);
+      }
     }
   }
 }
@@ -2477,11 +2511,10 @@ class CdataMatcher {
       parse_css.extractImportantDeclarations(stylesheet, important);
       for (const decl of important) {
         context.addError(
-            generated.ValidationError.Code.CDATA_VIOLATES_DENYLIST,
+            generated.ValidationError.Code.CSS_SYNTAX_DISALLOWED_IMPORTANT,
             new LineCol(decl.important_line, decl.important_col),
-            /* params */
-            [getTagDescriptiveName(this.tagSpec_), 'CSS !important'],
-            getTagSpecUrl(this.tagSpec_), validationResult);
+            /* params */[], context.getRules().getStylesSpecUrl(),
+            validationResult);
       }
     }
 
@@ -2511,8 +2544,9 @@ class CdataMatcher {
     // Validate the allowed CSS declarations (eg: `background-color`)
     if (maybeDocCssSpec !== null &&
         !maybeDocCssSpec.spec().allowAllDeclarationInStyleTag) {
-      const invalidDeclVisitor =
-          new InvalidDeclVisitor(maybeDocCssSpec, context, validationResult);
+      const invalidDeclVisitor = new InvalidDeclVisitor(
+          maybeDocCssSpec, context, getTagDescriptiveName(this.tagSpec_),
+          validationResult);
       stylesheet.accept(invalidDeclVisitor);
     }
 
@@ -3493,7 +3527,8 @@ class UrlErrorInAttrAdapter {
     context.addError(
         generated.ValidationError.Code.INVALID_URL_PROTOCOL,
         context.getLineCol(),
-        /* params */[this.attrName_, getTagDescriptiveName(tagSpec), protocol],
+        /* params */
+        [this.attrName_, getTagDescriptiveName(tagSpec), protocol],
         getTagSpecUrl(tagSpec), result);
   }
 
@@ -3547,7 +3582,8 @@ function validateAttrValueUrl(parsedAttrSpec, context, attr, tagSpec, result) {
       } else {
         context.addError(
             parseResult.errorCode, context.getLineCol(),
-            /* params */[attr.name, getTagDescriptiveName(tagSpec), attr.value],
+            /* params */
+            [attr.name, getTagDescriptiveName(tagSpec), attr.value],
             getTagSpecUrl(tagSpec), result);
       }
       return;
@@ -4655,12 +4691,14 @@ function validateAttrNotFoundInSpec(parsedTagSpec, context, attrName, result) {
     context.addError(
         generated.ValidationError.Code.TEMPLATE_IN_ATTR_NAME,
         context.getLineCol(),
-        /* params */[attrName, getTagDescriptiveName(parsedTagSpec.getSpec())],
+        /* params */
+        [attrName, getTagDescriptiveName(parsedTagSpec.getSpec())],
         context.getRules().getTemplateSpecUrl(), result);
   } else {
     context.addError(
         generated.ValidationError.Code.DISALLOWED_ATTR, context.getLineCol(),
-        /* params */[attrName, getTagDescriptiveName(parsedTagSpec.getSpec())],
+        /* params */
+        [attrName, getTagDescriptiveName(parsedTagSpec.getSpec())],
         getTagSpecUrl(parsedTagSpec), result);
   }
 }
@@ -4902,6 +4940,7 @@ function validateAttrCss(
     // in the allowed list for this DocCssSpec, and have allowed values if
     // relevant.
     for (const declaration of declarations) {
+      const firstIdent = declaration.firstIdent();
       // Allowed declarations vary by context. SVG has its own set of CSS
       // declarations not supported generally in HTML.
       const cssDeclaration = parsedAttrSpec.getSpec().valueDocSvgCss === true ?
@@ -4919,7 +4958,6 @@ function validateAttrCss(
         continue;
       } else if (cssDeclaration.valueCasei.length > 0) {
         let hasValidValue = false;
-        const firstIdent = declaration.firstIdent();
         for (const value of cssDeclaration.valueCasei) {
           if (firstIdent.toLowerCase() == value) {
             hasValidValue = true;
@@ -4935,16 +4973,23 @@ function validateAttrCss(
               [getTagDescriptiveName(tagSpec), declaration.name, firstIdent],
               context.getRules().getStylesSpecUrl(), result.validationResult);
         }
+      } else if (cssDeclaration.valueRegexCasei != null) {
+        const valueRegex = context.getRules().getFullMatchCaseiRegex(
+            /** @type {number} */ (cssDeclaration.valueRegexCasei));
+        if (!valueRegex.test(firstIdent)) {
+          context.addError(
+              generated.ValidationError.Code
+                  .CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE,
+              context.getLineCol(), /* params */
+              [getTagDescriptiveName(tagSpec), declaration.name, firstIdent],
+              context.getRules().getStylesSpecUrl(), result.validationResult);
+        }
       }
       if (!maybeSpec.spec().allowImportant) {
         if (declaration.important)
-          // TODO(gregable): Use a more specific error message for
-          // `!important` errors.
           context.addError(
-              generated.ValidationError.Code.INVALID_ATTR_VALUE,
-              context.getLineCol(),
-              /* params */
-              [attrName, getTagDescriptiveName(tagSpec), 'CSS !important'],
+              generated.ValidationError.Code.CSS_SYNTAX_DISALLOWED_IMPORTANT,
+              context.getLineCol(), /* params */[],
               context.getRules().getStylesSpecUrl(), result.validationResult);
       }
       /** @type {!Array<!tokenize_css.ErrorToken>} */
@@ -5030,6 +5075,7 @@ function validateAttrDeclaration(
   const cssDeclarationByName = parsedAttrSpec.getCssDeclarationByName();
 
   for (const declaration of declarations) {
+    const firstIdent = declaration.firstIdent();
     const declarationName =
         parse_css.stripVendorPrefix(declaration.name.toLowerCase());
     if (!(declarationName in cssDeclarationByName)) {
@@ -5043,7 +5089,6 @@ function validateAttrDeclaration(
       const cssDeclaration = cssDeclarationByName[declarationName];
       if (cssDeclaration.valueCasei.length > 0) {
         let has_valid_value = false;
-        const firstIdent = declaration.firstIdent();
         for (const value of cssDeclaration.valueCasei) {
           if (firstIdent.toLowerCase() === value) {
             has_valid_value = true;
@@ -5059,6 +5104,17 @@ function validateAttrDeclaration(
               /* params */[tagSpecName, declaration.name, firstIdent],
               context.getRules().getStylesSpecUrl(), validationResult);
         }
+      } else if (cssDeclaration.valueRegexCasei != null) {
+        const valueRegex = context.getRules().getFullMatchCaseiRegex(
+            /** @type {number} */ (cssDeclaration.valueRegexCasei));
+        if (!valueRegex.test(firstIdent)) {
+          context.addError(
+              generated.ValidationError.Code
+                  .CSS_SYNTAX_DISALLOWED_PROPERTY_VALUE,
+              context.getLineCol(), /* params */
+              /* params */[tagSpecName, declaration.name, firstIdent],
+              context.getRules().getStylesSpecUrl(), validationResult);
+        }
       }
     }
   }
@@ -5134,7 +5190,8 @@ function validateAttributes(
         continue;
       }
     } else if (attr.name === 'class') {
-      // For non-transformed AMP, `class` must not contain 'i-amphtml-' prefix.
+      // For non-transformed AMP, `class` must not contain 'i-amphtml-'
+      // prefix.
       validateClassAttr(attr, spec, context, result.validationResult);
     }
 
@@ -6773,6 +6830,46 @@ const ValidationHandler =
     }
   }
 
+  /**
+   * Currently, the Javascript HTML parser considers Doctype to be another HTML
+   * tag, which is not technically accurate. We have special handling for
+   * doctype in Javascript which applies to all AMP formats, as this is strict
+   * handling for all HTML in general. Specifically "attributes" are not
+   * allowed, even things like `data-foo`.
+   * @param {!parserInterface.ParsedHtmlTag} encounteredTag
+   * @param {!Context} context
+   * @param {!generated.ValidationResult} validationResult
+   */
+  validateDocType(encounteredTag, context, validationResult) {
+    // <!doctype html> - OK
+    if (encounteredTag.attrs().length === 1 &&
+        encounteredTag.attrs()[0].name === 'html')
+      return;
+    // <!doctype html lang=...> OK
+    // This is technically invalid. The 'correct' way to do this is to emit the
+    // lang attribute on the `<html>` tag. However, we observe a number of
+    // websites incorrectly emitting `lang` as part of doctype, so this specific
+    // attribute is allowed to avoid breaking existing pages.
+    if (encounteredTag.attrs().length === 2) {
+      if (encounteredTag.attrs()[0].name === 'html' &&
+          encounteredTag.attrs()[1].name === 'lang')
+        return;
+      if (encounteredTag.attrs()[0].name === 'lang' &&
+          encounteredTag.attrs()[1].name === 'html')
+        return;
+    }
+    if (encounteredTag.attrs().length !== 1 ||
+        encounteredTag.attrs()[0].name !== 'html') {
+      this.context_.addError(
+          generated.ValidationError.Code.INVALID_DOCTYPE_HTML,
+          this.context_.getLineCol(),
+          /* params */[],
+          /* specUrl */ 'https://amp.dev/documentation/' +
+              'guides-and-tutorials/start/create/basic_markup/',
+          this.validationResult_);
+    }
+  }
+
   /**
    * Callback for a start HTML tag.
    * @param {!parserInterface.ParsedHtmlTag} encounteredTag
@@ -6783,6 +6880,14 @@ const ValidationHandler =
       this.context_.getRules().validateHtmlTag(
           encounteredTag, this.context_, this.validationResult_);
     }
+    if (encounteredTag.upperName() === '!DOCTYPE') {
+      this.validateDocType(
+          encounteredTag, this.context_, this.validationResult_);
+      // Even though validateDocType emits all necessary errors about the tag,
+      // we continue to process it further (validateTag and such) so that we can
+      // record the tag was present and record it as the root pseudo element for
+      // the document.
+    }
     /** @type {?string} */
     const maybeDuplicateAttrName = encounteredTag.hasDuplicateAttrs();
     if (maybeDuplicateAttrName !== null) {

diff --git a/validator/testdata/amp4ads_feature_tests/doctype.out b/validator/testdata/amp4ads_feature_tests/doctype.out
@@ -1,4 +1,4 @@
-PASS
+FAIL
 |  <!--
 |    Copyright 2019 The AMP HTML Authors. All Rights Reserved.
 |
@@ -19,6 +19,8 @@ PASS
 |    Only html attribute is allowed in <!doctype> declaration.
 |  -->
 |  <!doctype html data-foo id=doctype>
+>> ^~~~~~~~~
+amp4ads_feature_tests/doctype.html:20:0 Invalid or missing doctype declaration. Should be '!doctype html'. (see https://amp.dev/documentation/guides-and-tutorials/start/create/basic_markup/)
 |  <html ⚡4ads>
 |  <head>
 |    <meta charset="utf-8">

diff --git a/validator/testdata/amp4ads_feature_tests/doctype.out.cpponly b/validator/testdata/amp4ads_feature_tests/doctype.out.cpponly
@@ -20,7 +20,7 @@ FAIL
 |  -->
 |  <!doctype html data-foo id=doctype>
 >>             ^~~~~~~~~
-amp4ads_feature_tests/doctype.html:20:12 Invalid or missing doctype declaration. Should be <!doctype html>. See https://amp.dev/documentation/guides-and-tutorials/start/create/basic_markup/
+amp4ads_feature_tests/doctype.html:20:12 Invalid or missing doctype declaration. Should be '!doctype html'. (see https://amp.dev/documentation/guides-and-tutorials/start/create/basic_markup/)
 |  <html ⚡4ads>
 |  <head>
 |    <meta charset="utf-8">

diff --git a/validator/testdata/amp4email_feature_tests/amp-autocomplete.html b/validator/testdata/amp4email_feature_tests/amp-autocomplete.html
@@ -74,7 +74,6 @@
       highlight-user-entry
       inline
       min-characters="0"
-      max-entries="3"
       max-items="3"
       submit-on-enter
       suggest-first