[webkit-reviews] review granted: [Bug 148363] Implement Subresource Integrity (SRI) : [Attachment 308676] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 148363: Implement Subresource Integrity (SRI)
https://bugs.webkit.org/show_bug.cgi?id=148363

Attachment 308676: Patch

https://bugs.webkit.org/attachment.cgi?id=308676&action=review




--- Comment #40 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 308676
  --> https://bugs.webkit.org/attachment.cgi?id=308676
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308676&action=review

> Source/WebCore/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=148363

Please add the radar bug URL under this line.

> Source/WebCore/ChangeLog:22
> +	   ThreadableLoaderOptions::isolatedCopy to work correctly (it was
missing isolated
> +	   copy derivedCachedDataTypesToRetrieve).

Can we add a test for this change?

> Source/WebCore/ChangeLog:50
> +	   When requesting a stylesheet, cache the integrity metadata so it can
> +	   be used when the load completes (accessing the attribute then would
be
> +	   incorrect, as it a script might have changed its value since the
request).

This sentence does not read well. In particular, the "as it a script" part does
not read well.

> Source/WebCore/html/HTMLLinkElement.cpp:384
> +	   notifyLoadedSheetAndAllCriticalSubresources(true);

This is OK as-is. We should have notifyLoadedSheetAndAllCriticalSubresources()
take a enum instead of a boolean to indicate that an error occurred to make the
code read well at the call site.

> Source/WebCore/loader/SubresourceIntegrity.cpp:55
> +	   auto crypographicDigest = parseCryptographicDigest(position, end);

crypographicDigest => cryptographicDigest

(Notice the presence of the letter t in "cryptographic")

> Source/WebCore/loader/SubresourceIntegrity.cpp:107
> +    // FIXME: The spec says this should check XXX.

XXX?

> Source/WebCore/loader/SubresourceIntegrity.cpp:115
> +    return (a > b) ? a : b;

The parentheses are not necessary.

> Source/WebCore/loader/SubresourceIntegrity.cpp:122
> +    ResourceCryptographicDigest::Algorithm strongest;

The Win-EWS bots is not happy that this is uninitialized when it compiles the
assignment on line 134. VC++ cannot verify that this local would have been
initialized by line 128, which is guaranteed because the vector is initially
empty.

>
LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/subresource-i
ntegrity.sub.html:12
> +    // WEBKIT CHANGES

We should upstream these changes, find a better place for this test, come up
with a naming convention for modified WPTs that have not been upstreamed, or
add a remark to
LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/tools/w3c-imp
ort.log so as to prevent a person from inadvertently overwriting this test with
the upstream test of the same name when resyncing WPT.

>
LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/subresource-i
ntegrity.sub.html:17
> +    // - Changed script tests to match the style tests and operate on a
queue
> +    //   to address issue where console messages where coming in at
inconsistent
> +    //   times, making the results flaky.

This problem, out-of-order console messages, is not specific to just this test.
It affects our ability to run other web platform test. We do not need to solve
this issue now, but we should figure out how to solve it or come up with a plan
on how we are going to deal wit this issue moving forward. Individually
modifying WPTs and maintaining these variants does not seem practical in the
long term. We could fix all upstream asynchronous tests to use a queue to
ensure ordering. Another option is to leave the tests as-is and add the tests
to TestExpectations with the modifier DumpJSConsoleLogInStdErr to dump console
messages to standard error instead of emitting to standard output as part of
the test result. Obviously, the disadvantage of the latter approach is that
run-webkit-tests does not compare standard error output (at least at the time
of writing) and people are unlikely to compare such output manually so we may
miss output that could indicate a regression.

>
LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/tools/w3c-imp
ort.log:2
> +Do NOT modify these tests directly in WebKit.

We did this in this patch :)