Ignore unknown options to subresource integrity

Ignore unknown options to subresource integrity

The spec states that unknown options must be ignored by the user agent
so that new options may be added in the future. This adds support for
that along with appropriate tests.

BUG=355467
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195212

[jww, 2015-05-07 22:52:21]

jww@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2015-05-07 23:11:01]

On 2015/05/07 at 22:52:21, jww wrote:
> 

I don't really understand why we're keeping all this code around if we're not
going to be shipping the mime types bit. It is somewhat complicated, and
throwing it away would solve the options problem nicely, wouldn't it?

I think I'd prefer that we either ship it as part of the MVP, or drop it from
our implementation until we get closer to wanting to ship v2.

[jww, 2015-05-08 17:31:29]

On 2015/05/07 23:11:01, Mike West (traveling. slow.) wrote:
> On 2015/05/07 at 22:52:21, jww wrote:
> > 
> 
> I don't really understand why we're keeping all this code around if we're not
> going to be shipping the mime types bit. It is somewhat complicated, and
> throwing it away would solve the options problem nicely, wouldn't it?
> 
> I think I'd prefer that we either ship it as part of the MVP, or drop it from
> our implementation until we get closer to wanting to ship v2.

Yeah, fair enough. I'm convinced. I'll upload a CL w/out mime types. We still
need the options changes, though, since the spec requires that we acknowledge
but ignore them.

[jww, 2015-05-08 19:36:42]

Mike, this removes the MIME types completely. Can you take a look? Thanks.

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

https://codereview.chromium.org/1126343003/diff/1/Source/core/frame/Subresour...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/1126343003/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:246: // [algorithm]-[hash]     OR    
 [algorithm]-[hash]?[name]=[value]     OR     
[algorithm]-[hash]?[name]=[value]?[rest of options]
This is such a weird syntax.

https://codereview.chromium.org/1126343003/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:275: if (position == end || *position
== '?')
Would you mind asserting that `position < end` before dereferencing it?

https://codereview.chromium.org/1126343003/diff/1/Source/core/frame/Subresour...
File Source/core/frame/SubresourceIntegrityTest.cpp (right):

https://codereview.chromium.org/1126343003/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrityTest.cpp:27: static const char
kSha256IntegrityWithUnknownOptions[] =
"sha256-GAF48QOoxRvu0gZAmQivUdJPyBacqznBAXwnkfpmQX4=?foo=bar?baz=foz";
Nit: Add a test for a single unknown option as well, please. :)

https://codereview.chromium.org/1126343003/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrityTest.cpp:135:
EXPECT_FALSE(SubresourceIntegrity::parseOption(position, end, key, value));
Please also EXPECT that `key` and `value` are the empty string.

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:45: return isASCIIAlphanumeric(c) ||
c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c ==
'*' || c == '+' || c == '-'  || c == '.' || c == '^' || c == '_' || c == '`' ||
c == '|' || c == '~' || c == '/';
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.

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:208: // [algorithm]-[hash]     OR    
 [algorithm]-[hash]?[name]=[value]     OR     
[algorithm]-[hash]?[name]=[value]?[rest of options]
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*

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:233: name = String(begin, position -
begin);
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 :) )).

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:243: if (position == end || *position
== '?')
Nit: Please ASSERT that `position` is in-range before dereferencing it (or does
`skipWhile` already ASSERT?).

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:316: logErrorToConsole("Ignoring
unrecogized 'integrity' attribute option '" + name + "' with value '" + value +
"'.", document);
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?

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:319: if (!unparsableOptions) {
Why do you need a separate boolean here? Would checking `!error` be enough?

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
File Source/core/frame/SubresourceIntegrityTest.cpp (right):

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrityTest.cpp:27: static const char
kSha256IntegrityWithUnknownOptions[] =
"sha256-GAF48QOoxRvu0gZAmQivUdJPyBacqznBAXwnkfpmQX4=?foo=bar?baz=foz";
Could you add another test with a single option?

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:589:
DeprecatedSRIElementWithNonMatchingIntegrityType = 660,
Nit: Just remove it. :)

[jww, 2015-05-11 05:39:08]

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:45: return isASCIIAlphanumeric(c) ||
c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c ==
'*' || c == '+' || c == '-'  || c == '.' || c == '^' || c == '_' || c == '`' ||
c == '|' || c == '~' || c == '/';
On 2015/05/11 03:25:29, Mike West (traveling. slow.) wrote:
> 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.

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.

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:208: // [algorithm]-[hash]     OR    
 [algorithm]-[hash]?[name]=[value]     OR     
[algorithm]-[hash]?[name]=[value]?[rest of options]
On 2015/05/11 03:25:29, Mike West (traveling. slow.) wrote:
> 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*

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

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:233: name = String(begin, position -
begin);
On 2015/05/11 03:25:29, Mike West (traveling. slow.) wrote:
> 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 :) )).

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)"

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:243: if (position == end || *position
== '?')
On 2015/05/11 03:25:29, Mike West (traveling. slow.) wrote:
> Nit: Please ASSERT that `position` is in-range before dereferencing it (or
does
> `skipWhile` already ASSERT?).

Also irrelevant now.

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:316: logErrorToConsole("Ignoring
unrecogized 'integrity' attribute option '" + name + "' with value '" + value +
"'.", document);
On 2015/05/11 03:25:29, Mike West (traveling. slow.) wrote:
> 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?

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

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrity.cpp:319: if (!unparsableOptions) {
On 2015/05/11 03:25:29, Mike West (traveling. slow.) wrote:
> Why do you need a separate boolean here? Would checking `!error` be enough?

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 :-)

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
File Source/core/frame/SubresourceIntegrityTest.cpp (right):

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/Subre...
Source/core/frame/SubresourceIntegrityTest.cpp:27: static const char
kSha256IntegrityWithUnknownOptions[] =
"sha256-GAF48QOoxRvu0gZAmQivUdJPyBacqznBAXwnkfpmQX4=?foo=bar?baz=foz";
On 2015/05/11 03:25:29, Mike West (traveling. slow.) wrote:
> Could you add another test with a single option?

Done.

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1126343003/diff/40001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:589:
DeprecatedSRIElementWithNonMatchingIntegrityType = 660,
On 2015/05/11 03:25:29, Mike West (traveling. slow.) wrote:
> Nit: Just remove it. :)

Hrm, I guess that works? I thought we weren't supposed to remove old use
counters, but I see now that there are quite a few gaps. Done.

[Mike West, 2015-05-11 07:31:32]

LGTM. Thanks!

[jww, 2015-05-11 23:06:39]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2015-05-11 23:06:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126343003/60001

[commit-bot: I haz the power, 2015-05-12 00:24:49]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195212