Ignore unknown options to subresource integrity

Source/core/frame/SubresourceIntegrity.cpp

Index: Source/core/frame/SubresourceIntegrity.cpp
diff --git a/Source/core/frame/SubresourceIntegrity.cpp b/Source/core/frame/SubresourceIntegrity.cpp
index 0832fccf0233a8642aa933f89679ab5b4ef65fde..c412bb632501df0ce08efe12398cf8b8d07510c5 100644
--- a/Source/core/frame/SubresourceIntegrity.cpp
+++ b/Source/core/frame/SubresourceIntegrity.cpp
@@ -35,9 +35,14 @@ static bool isIntegrityCharacter(UChar c)
     return isASCIIAlphanumeric(c) || c == '_' || c == '-' || c == '+' || c == '/' || c == '=';
 }
 
-static bool isTypeCharacter(UChar c)
+static bool isOptionNameCharacter(UChar c)
 {
-    return isASCIIAlphanumeric(c) || c == '+' || c == '.' || c == '-';
+    return isASCIIAlphanumeric(c) || c == '-';
+}
+
+static bool isOptionValueCharacter(UChar c)
+{
+    return isASCIIAlphanumeric(c) || c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' || c == '+' || c == '-'  || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~' || c == '/';

[Mike West, 2015/05/11 03:25:29]

This is a strangely arbitrary list of characters. I expected some note in the
spec about why you chose this set of characters and not some other set. For
instance, why no '()' or '[]'? I'm sure there's a reason y'all considered, but I
don't see it in the spec.

[jww, 2015/05/11 05:39:07]

Fair point. As per fmarier's suggestion in
https://github.com/w3c/webappsec/issues/343, I made a pull request to redefine
this as *VCHAR, so I'll update the implementation here accordingly.

 }
 
 static void logErrorToConsole(const String& message, Document& document)
@@ -64,7 +69,7 @@ static String digestToString(const DigestValue& digest)
     return base64URLEncode(reinterpret_cast<const char*>(digest.data()), digest.size(), Base64DoNotInsertLFs);
 }
 
-bool SubresourceIntegrity::CheckSubresourceIntegrity(const Element& element, const String& source, const KURL& resourceUrl, const String& resourceType, const Resource& resource)
+bool SubresourceIntegrity::CheckSubresourceIntegrity(const Element& element, const String& source, const KURL& resourceUrl, const Resource& resource)
 {
     if (!RuntimeEnabledFeatures::subresourceIntegrityEnabled())
         return true;
@@ -101,13 +106,8 @@ bool SubresourceIntegrity::CheckSubresourceIntegrity(const Element& element, con
             DigestValue convertedHashVector;
             convertedHashVector.append(reinterpret_cast<uint8_t*>(hashVector.data()), hashVector.size());
 
-            if (DigestsEqual(digest, convertedHashVector)) {
-                String& type = metadata.type;
-                if (!type.isEmpty() && !equalIgnoringCase(type, resourceType))
-                    UseCounter::count(document, UseCounter::SRIElementWithNonMatchingIntegrityType);
-                else
-                    return true;
-            }
+            if (DigestsEqual(digest, convertedHashVector))
+                return true;
         }
     }
 
@@ -117,7 +117,7 @@ bool SubresourceIntegrity::CheckSubresourceIntegrity(const Element& element, con
         // need to be very careful not to expose this in exceptions or
         // JavaScript, otherwise it risks exposing information about the
         // resource cross-origin.
-        logErrorToConsole("Failed to find a valid digest with matching content-type in the 'integrity' attribute for resource '" + resourceUrl.elidedString() + "' with computed SHA-256 integrity '" + digestToString(digest) + "'. The resource has been blocked.", document);
+        logErrorToConsole("Failed to find a valid digest in the 'integrity' attribute for resource '" + resourceUrl.elidedString() + "' with computed SHA-256 integrity '" + digestToString(digest) + "'. The resource has been blocked.", document);
     } else {
         logErrorToConsole("There was an error computing an integrity value for resource '" + resourceUrl.elidedString() + "'. The resource has been blocked.", document);
     }
@@ -203,45 +203,47 @@ bool SubresourceIntegrity::parseDigest(const UChar*& position, const UChar* end,
     return true;
 }
 
-
 // Before:
 //
-// [algorithm]-[hash]     OR      [algorithm]-[hash]?[options]
-//                   ^                              ^         ^
-//        position/end                       position       end
+// [algorithm]-[hash]     OR      [algorithm]-[hash]?[name]=[value]     OR      [algorithm]-[hash]?[name]=[value]?[rest of options]

[Mike West, 2015/05/11 03:25:29]

This is strange syntax. I'm not going to bikeshed it here, but it seems like `&`
would have been a fairly natural separator, given the `?` syntax. If you wanted
to make it less like a URL, `;` or `,` would be equally reasonable. Repeating
the `?` seems fairly unique, and a bit surprising. *shrug*

[jww, 2015/05/11 05:39:07]

Bikeshedding irrelevant in my latest pull request which removes the option
syntax (well, postponed at least).

+//                   ^                              ^              ^                              ^                                ^
+//        position/end                       position            end                       position                              end
 //
 // After (if successful: if the method returns false, we make no promises and the caller should exit early):
 //
-// [algorithm]-[hash]     OR      [algorithm]-[hash]?[options]
-//                   ^                                        ^
-//        position/end                             position/end
-bool SubresourceIntegrity::parseMimeType(const UChar*& position, const UChar* end, String& type)
+// [algorithm]-[hash]     OR      [algorithm]-[hash]?[name]=[value]     OR      [algorithm]-[hash]?[name]=[value]?[rest of options]
+//                   ^                                             ^                                            ^                  ^
+//        position/end                                  position/end                                     position                end
+bool SubresourceIntegrity::parseOption(const UChar*& position, const UChar* end, String& name, String& value)
 {
-    type = emptyString();
+    name = emptyString();
+    value = emptyString();
 
     if (position == end)
         return true;
 
-    if (!skipToken<UChar>(position, end, "?ct="))
+    if (!skipExactly<UChar>(position, end, '?'))
         return false;
 
     const UChar* begin = position;
-    skipWhile<UChar, isASCIIAlpha>(position, end);
-    if (position == end)
+    skipWhile<UChar, isOptionNameCharacter>(position, end);
+    if (position == begin || position == end)
         return false;
 
-    if (!skipExactly<UChar>(position, end, '/'))
-        return false;
+    name = String(begin, position - begin);

[Mike West, 2015/05/11 03:25:29]

This means `name` will be set to some value, even if the parse fails. Below, you
do the same with `value`. This is fine, but I would have expected them to remain
empty in the error case. Would you mind adding some documentation (here or in
the .h file (or both, go crazy :) )).

[jww, 2015/05/11 05:39:07]

Also irrelevant given the broader changes, but in fairness, the comment did
read:
"After (if successful: if the method returns false, we make no promises and the
caller should exit early)"

 
-    if (position == end)
+    if (!skipExactly<UChar>(position, end, '='))
         return false;
 
-    skipWhile<UChar, isTypeCharacter>(position, end);
-    if (position != end)
-        return false;
+    begin = position;
+    skipWhile<UChar, isOptionValueCharacter>(position, end);
 
-    type = String(begin, position - begin);
-    return true;
+    value = String(begin, position - begin);
+
+    if (position == end || *position == '?')

[Mike West, 2015/05/11 03:25:29]

Nit: Please ASSERT that `position` is in-range before dereferencing it (or does
`skipWhile` already ASSERT?).

[jww, 2015/05/11 05:39:07]

Also irrelevant now.

+        return true;
+
+    return false;
 }
 
 SubresourceIntegrity::IntegrityParseResult SubresourceIntegrity::parseIntegrityAttribute(const WTF::String& attribute, WTF::Vector<IntegrityMetadata>& metadataList, Document& document)
@@ -257,12 +259,11 @@ SubresourceIntegrity::IntegrityParseResult SubresourceIntegrity::parseIntegrityA
 
     // The integrity attribute takes the form:
     //    *WSP hash-with-options *( 1*WSP hash-with-options ) *WSP / *WSP
-    // To parse this, break on whitespace, parsing each algorithm/digest/mime
-    // type in order.
+    // To parse this, break on whitespace, parsing each algorithm/digest/option
+    // in order.
     while (position < end) {
         WTF::String digest;
         HashAlgorithm algorithm;
-        WTF::String type;
 
         skipWhile<UChar, isASCIISpace>(position, end);
         currentIntegrityEnd = position;
@@ -300,20 +301,28 @@ SubresourceIntegrity::IntegrityParseResult SubresourceIntegrity::parseIntegrityA
             continue;
         }
 
-        if (!parseMimeType(position, currentIntegrityEnd, type)) {
-            logErrorToConsole("Error parsing 'integrity' attribute ('" + attribute + "'). The content type could not be parsed.", document);
-            error = true;
-            skipUntil<UChar, isASCIISpace>(position, end);
-            UseCounter::count(document, UseCounter::SRIElementWithUnparsableIntegrityAttribute);
-            continue;
+        bool unparsableOptions = false;
+        while (*position == '?') {
+            String name, value;
+            if (!parseOption(position, currentIntegrityEnd, name, value)) {
+                logErrorToConsole("Error parsing 'integrity' attribute ('" + attribute + "'). The options could not be parsed.", document);
+                unparsableOptions = true;
+                error = true;
+                skipUntil<UChar, isASCIISpace>(position, end);
+                UseCounter::count(document, UseCounter::SRIElementWithUnparsableIntegrityAttribute);
+                break;
+            }
+
+            logErrorToConsole("Ignoring unrecogized 'integrity' attribute option '" + name + "' with value '" + value + "'.", document);

[Mike West, 2015/05/11 03:25:29]

This would end up logging a lot if/when options become a thing. Would you
consider building up a list and logging once? Or just logging once without
explicitly listing the options that are being ignored?

[jww, 2015/05/11 05:39:07]

My new change logs just once the entire unknown option expression.

         }
 
-        IntegrityMetadata integrityMetadata = {
-            digest,
-            algorithm,
-            type
-        };
-        metadataList.append(integrityMetadata);
+        if (!unparsableOptions) {

[Mike West, 2015/05/11 03:25:29]

Why do you need a separate boolean here? Would checking `!error` be enough?

[jww, 2015/05/11 05:39:07]

Alas, it wouldn't have been :-( Error tracks if any error occurred across *all*
integrities in the attribute. Thus, if you had two hash values (one sha256 and
the second sha512), and the first had a syntax error, error would be true. This
is used below to know if IntegrityParseNoValidResult should be returned or
IntegrityParseValidResult.

In any case, also irrelevant given the latest changes :-)

+            IntegrityMetadata integrityMetadata = {
+                digest,
+                algorithm
+            };
+            metadataList.append(integrityMetadata);
+        }
     }
 
     if (metadataList.size() == 0 && error)