Created:
6 years, 3 months ago by jww
Modified:
6 years, 2 months ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.
|
Total comments: 8
Total comments: 4
Created: 6 years, 2 months ago
Total messages: 13 (3 generated)
|
jww
|
6 years, 3 months ago
(2014-09-23 23:36:37 UTC)
#1
|
|
Mike West
|
|
6 years, 3 months ago
(2014-09-24 08:40:28 UTC)
#2
|
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
|
|
6 years, 2 months ago
(2014-09-30 01:38:50 UTC)
#3
|
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
|
|
6 years, 2 months ago
(2014-09-30 09:13:28 UTC)
#4
|
|
jww
|
|
6 years, 2 months ago
(2014-09-30 17:59:33 UTC)
#5
|
|
jww
|
|
6 years, 2 months ago
(2014-09-30 17:59:33 UTC)
#6
|
|
jww
|
|
6 years, 2 months ago
(2014-09-30 17:59:38 UTC)
#7
|
|
commit-bot: I haz the power
|
|
6 years, 2 months ago
(2014-09-30 18:00:11 UTC)
#8
|
|
commit-bot: I haz the power
|
|
6 years, 2 months ago
(2014-09-30 19:02:06 UTC)
#9
|
|
commit-bot: I haz the power
|
|
6 years, 2 months ago
(2014-09-30 19:02:07 UTC)
#10
|
|
jww
|
|
6 years, 2 months ago
(2014-09-30 19:16:04 UTC)
#11
|
|
commit-bot: I haz the power
|
|
6 years, 2 months ago
(2014-09-30 19:16:38 UTC)
#12
|
|
commit-bot: I haz the power
|
|
6 years, 2 months ago
(2014-09-30 23:05:55 UTC)
#13
|
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 183000
|
