Basic console error messages for subresource integrity.

Basic console error messages for subresource integrity.

Subresource integrity is behind an experimental flag in blink (see
https://codereview.chromium.org/566083003). This CL adds in some useful
console error messages when things go wrong, such as non-matching
integrity values or an unparsable integrity attribute.

R=mkwst@chromium.org

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

[Mike West, 2014-09-24 08:40:28]

Thanks! A few comments inline.

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:117: logErrorToConsole("The
'integrity' attribute '" + attribute + "' does not parse as a valid integrity.",
document);
How about "The 'integrity' attribute's value '" + attribute + "' is not valid
integrity metadata."?

It would be nice to get a little more detail by hooking into the parser, as we
do in CSP, but this is a perfectly reasonable start.

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:136: logErrorToConsole("The computed
" + algorithmToString(algorithm) + " integrity '" + digestToString(digest) + "'
does not match the 'integrity' attribute '" + integrity + "'.", document);
This message exposes the digest of the resource. As long as the message _only_
goes to the console, this is fine. If we start throwing exceptions, or reporting
this as a violation event or something in the future, we'll need to be careful
to avoid exposing that information to JavaScript.

Would you mind adding a comment to that effect?

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:139: logErrorToConsole("There was an
error computing an 'integrity' value for a resource.", document);
Perhaps you could pass a KURL into this method to improve messages like this
one. (e.g. url.elidedString() instead of "a resource"). This is especially
important for the previous error; if there are three integrity'd resources on a
page, there's no easy way for the developer to figure out which one failed.

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:150: // respective entries in the
kAlgorithmMap array in checkDigest().
// as well as the array in algorithmToString().

[jww, 2014-09-30 01:38:50]

Hey Mike. Sorry it took so long, but I think I addressed your comments.

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:117: logErrorToConsole("The
'integrity' attribute '" + attribute + "' does not parse as a valid integrity.",
document);
On 2014/09/24 08:40:27, Mike West wrote:
> How about "The 'integrity' attribute's value '" + attribute + "' is not valid
> integrity metadata."?
> 
> It would be nice to get a little more detail by hooking into the parser, as we
> do in CSP, but this is a perfectly reasonable start.
Yeah, first part done, and I figure we can do more detailed later.

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:136: logErrorToConsole("The computed
" + algorithmToString(algorithm) + " integrity '" + digestToString(digest) + "'
does not match the 'integrity' attribute '" + integrity + "'.", document);
On 2014/09/24 08:40:28, Mike West wrote:
> This message exposes the digest of the resource. As long as the message _only_
> goes to the console, this is fine. If we start throwing exceptions, or
reporting
> this as a violation event or something in the future, we'll need to be careful
> to avoid exposing that information to JavaScript.
> 
> Would you mind adding a comment to that effect?

Done.

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:139: logErrorToConsole("There was an
error computing an 'integrity' value for a resource.", document);
On 2014/09/24 08:40:27, Mike West wrote:
> Perhaps you could pass a KURL into this method to improve messages like this
> one. (e.g. url.elidedString() instead of "a resource"). This is especially
> important for the previous error; if there are three integrity'd resources on
a
> page, there's no easy way for the developer to figure out which one failed.

Done.

https://codereview.chromium.org/596043003/diff/20001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:150: // respective entries in the
kAlgorithmMap array in checkDigest().
On 2014/09/24 08:40:28, Mike West wrote:
> // as well as the array in algorithmToString().

Done.

[Mike West, 2014-09-30 09:13:28]

LGTM % typos. Thanks!

https://codereview.chromium.org/596043003/diff/60001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/596043003/diff/60001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:117: logErrorToConsole("The
'integrity' attribute's value  '" + attribute + "' is not valid integrity
metadat.", document);
s/metadat/metadata/.

https://codereview.chromium.org/596043003/diff/60001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:138: // need to be very carefully not
to expose this in exceptions or
s/carefully/careful/

[jww, 2014-09-30 17:59:33]

https://codereview.chromium.org/596043003/diff/60001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/596043003/diff/60001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:117: logErrorToConsole("The
'integrity' attribute's value  '" + attribute + "' is not valid integrity
metadat.", document);
On 2014/09/30 09:13:28, Mike West wrote:
> s/metadat/metadata/.

Done.

https://codereview.chromium.org/596043003/diff/60001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:138: // need to be very carefully not
to expose this in exceptions or
On 2014/09/30 09:13:28, Mike West wrote:
> s/carefully/careful/

Done.

[jww, 2014-09-30 17:59:33]

https://codereview.chromium.org/596043003/diff/60001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/596043003/diff/60001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:117: logErrorToConsole("The
'integrity' attribute's value  '" + attribute + "' is not valid integrity
metadat.", document);
On 2014/09/30 09:13:28, Mike West wrote:
> s/metadat/metadata/.

Done.

https://codereview.chromium.org/596043003/diff/60001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:138: // need to be very carefully not
to expose this in exceptions or
On 2014/09/30 09:13:28, Mike West wrote:
> s/carefully/careful/

Done.

[jww, 2014-09-30 17:59:38]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:00:11]

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

[commit-bot: I haz the power, 2014-09-30 19:02:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:02:07]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[jww, 2014-09-30 19:16:04]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:16:38]

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

[commit-bot: I haz the power, 2014-09-30 23:05:55]

Committed patchset #6 (id:100001) as 183000