From 097bf5778965ef020ea96fbc4730ac33f12d9ebc Mon Sep 17 00:00:00 2001
From: Greg Grothaus <ggrothau@gmail.com>
Date: Wed, 9 Dec 2015 14:02:08 -0800
Subject: [PATCH] Disallow data: URLs.

Also some cleanups on the validator error messages.
---
 .../feature_tests/javascript_xss.html         |  1 +
 .../testdata/feature_tests/javascript_xss.out |  1 +
 validator/validator.js                        | 20 ++++++++++++++-----
 validator/validator.protoascii                |  4 ++--
 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/validator/testdata/feature_tests/javascript_xss.html b/validator/testdata/feature_tests/javascript_xss.html
index d96e3fbb1ed8..bc16cb6b6956 100644
--- a/validator/testdata/feature_tests/javascript_xss.html
+++ b/validator/testdata/feature_tests/javascript_xss.html
@@ -27,6 +27,7 @@
 <a href="javascript:alert('boo')">Click here to see an important message.</a>
 <a href="JavaScript:alert('boo')">It's CAse inSenSitive.</a>
 <a href="vbscript:bar">vbscript is also dangerous</a>
+<a href="data:baz">data urls have some risks</a>
 
 </body>
 </html>
diff --git a/validator/testdata/feature_tests/javascript_xss.out b/validator/testdata/feature_tests/javascript_xss.out
index fb2fcbd140cb..f3cbe7c46450 100644
--- a/validator/testdata/feature_tests/javascript_xss.out
+++ b/validator/testdata/feature_tests/javascript_xss.out
@@ -2,3 +2,4 @@ FAIL
 feature_tests/javascript_xss.html:27:0 INVALID_ATTR_VALUE href=javascript:alert('boo')
 feature_tests/javascript_xss.html:28:0 INVALID_ATTR_VALUE href=JavaScript:alert('boo')
 feature_tests/javascript_xss.html:29:0 INVALID_ATTR_VALUE href=vbscript:bar
+feature_tests/javascript_xss.html:30:0 INVALID_ATTR_VALUE href=data:baz
diff --git a/validator/validator.js b/validator/validator.js
index b6ef1b716646..94daf179a133 100644
--- a/validator/validator.js
+++ b/validator/validator.js
@@ -1165,9 +1165,7 @@ ParsedTagSpec.prototype.validateAttributes = function(
                            parsedSpec.getSpec().deprecation,
                        parsedSpec.getSpec().deprecationUrl,
                        resultForAttempt);
-      // Since the Javascript is always in development mode, deprecation
-      // is an error. So we return.
-      return;
+      // Deprecation is only a warning, so we don't return.
     }
     if (parsedSpec.getSpec().devModeEnabled !== null) {
       context.addError(amp.validator.ValidationError.Code.DEV_MODE_ENABLED,
@@ -1175,9 +1173,15 @@ ParsedTagSpec.prototype.validateAttributes = function(
                            parsedSpec.getSpec().devModeEnabled,
                        parsedSpec.getSpec().devModeEnabledUrl,
                        resultForAttempt);
-      // Since the Javascript is always in development mode, developer mode
-      // enabled is not an error, so no need to return.
+      // Enabling the developer attribute is now always an error, so we
+      // return.
+      return;
     }
+    // The value, value_regex, and value_properties fields are
+    // treated like a oneof, but we're not using oneof because it's
+    // a feature that was added after protobuf 2.5.0 (which our
+    // open-source version uses).
+    // begin oneof {
     if (parsedSpec.getSpec().value !== null) {
       if (encounteredAttrValue != parsedSpec.getSpec().value) {
         context.addError(amp.validator.ValidationError.Code.INVALID_ATTR_VALUE,
@@ -1204,6 +1208,7 @@ ParsedTagSpec.prototype.validateAttributes = function(
         return;
       }
     }
+    // } end oneof
     if (parsedSpec.getSpec().blacklistedValueRegex !== null) {
       const blacklistedValueRegex =
           new RegExp(parsedSpec.getSpec().blacklistedValueRegex, 'gi');
@@ -1217,6 +1222,9 @@ ParsedTagSpec.prototype.validateAttributes = function(
     if (parsedSpec.getSpec().mandatory) {
       mandatoryAttrsSeen.push(parsedSpec.getId());
     }
+    // The "at most 1" part of mandatory_oneof: mandatory_oneof
+    // wants exactly one of the alternatives, so here
+    // we check whether we already saw another alternative
     if (parsedSpec.getSpec().mandatoryOneof &&
         mandatoryOneofsSeen.contains(parsedSpec.getSpec().mandatoryOneof)) {
       context.addError(
@@ -1227,6 +1235,8 @@ ParsedTagSpec.prototype.validateAttributes = function(
     }
     mandatoryOneofsSeen.add(parsedSpec.getSpec().mandatoryOneof);
   }
+  // The "at least 1" part of mandatory_oneof: If none of the
+  // alternatives were present, we report that an attribute is missing.
   for (const mandatoryOneof of this.mandatoryOneofs_) {
     if (!mandatoryOneofsSeen.contains(mandatoryOneof)) {
       context.addError(
diff --git a/validator/validator.protoascii b/validator/validator.protoascii
index a22e7b2b8022..3bbba94043fd 100644
--- a/validator/validator.protoascii
+++ b/validator/validator.protoascii
@@ -25,7 +25,7 @@ min_validator_revision_required: 63
 # newer versions of the spec file. This is currently a Google internal
 # mechanism, validator.js does not use this facility. However, any
 # change to this file requires updating this revision id.
-spec_file_revision: 92
+spec_file_revision: 93
 
 # Rules for AMP HTML
 # (https://github.com/google/amphtml/blob/master/spec/amp-html-format.md).
@@ -593,7 +593,7 @@ tags: {
   name: "a"
   attrs: {
     name: "href"
-    blacklisted_value_regex: "(javascript|vbscript):.*"
+    blacklisted_value_regex: "(javascript|vbscript|data):.*"
   }
   attrs: {
     name: "target"