[webkit-reviews] review denied: [Bug 184560] From-Origin: Support for 'same' and 'same-site' response header, nested frame origin check : [Attachment 338186] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Apr 17 21:03:35 PDT 2018

Previous message (by thread): [webkit-reviews] review granted: [Bug 184515] Add support for converting a local window to a remote window : [Attachment 338144] Patch
Next message (by thread): [webkit-reviews] review canceled: [Bug 184428] Remove access to keychain from the WebContent process : [Attachment 337544] Patch
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Daniel Bates <dbates at webkit.org> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 184560: From-Origin: Support for 'same' and 'same-site' response header,
nested frame origin check
https://bugs.webkit.org/show_bug.cgi?id=184560

Attachment 338186: Patch

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




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

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

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:64
> +    Vector<String> frameAncestorOrigins;

Is there a reason we are not using SecurityOrigin?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:359
> +	   if (frameOrigin != "null" && frameOrigin !=
response.url().protocolHostAndPort())

How did you come to the decision to treat unique origins (i.e. frameOrigin ==
"null") as same-origin?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:379
> +    default:

Please remove this default case and move its contents outside of the switch
block. The benefit of omitting a default case statement is that it allows the
compiler to check that the switch block covers all enumeration values for us.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:403
> +	       ResourceError error = { String(), 0, url, "Cancelled load
because it violates the resource's From-Origin response header." };

Would it make sense to specify ResourceError::Type::AccessControl as the error
type? We should use ASCIILiteral() to construct the string for this error
message as it avoids copying the characters. For you consideration I suggest
using constructor syntax instead of assignment syntax to initialize this local
and use uniform initializer syntax throughout the entire line for consistency:

ResourceError error { String { }, 0, url, ASCIILiteral { "Cancelled load
because it violates the resource's From-Origin response header." } };

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:283
> +	   Vector<String> frameAncestorOrigins;

Is there a reason we are not using SecurityOrigin?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:285
> +	   while (currentFrame) {

We are underutilizing the scope of currentFrame as we never use it outside of
the while loop. I suggest that we implement this loop using a for loop:

for (Frame* frame = resourceLoader.frame(); frame; frame =
frame->tree().parent()) {
    ...
}

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:286
> +	       if (currentFrame->document())

Can you please provide an example when a frame would not have a document when
this function is invoked?

> LayoutTests/http/tests/from-origin/document-from-origin-same-accepted.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/document-from-origin-same-accepted.html:19
> +	   setTimeout("iframeLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

> LayoutTests/http/tests/from-origin/document-from-origin-same-blocked.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/document-from-origin-same-blocked.html:19
> +	   setTimeout("iframeLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

>
LayoutTests/http/tests/from-origin/document-from-origin-same-site-accepted.html
:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/document-from-origin-same-site-accepted.html
:19
> +	   setTimeout("iframeLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

>
LayoutTests/http/tests/from-origin/document-from-origin-same-site-blocked.html:
1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/document-from-origin-same-site-blocked.html:
19
> +	   setTimeout("iframeLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

>
LayoutTests/http/tests/from-origin/document-nested-from-origin-same-accepted.ht
ml:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/document-nested-from-origin-same-blocked.htm
l:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/fetch-from-origin-same-accepted.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/fetch-from-origin-same-blocked-expected.txt:
6
> +PASS Fetch blocked. TypeError: Cancelled load because it violates the
resource's From-Origin response header.

It is disingenuous to classify this security error as a TypeError.

> LayoutTests/http/tests/from-origin/fetch-from-origin-same-blocked.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/fetch-from-origin-same-site-accepted.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/fetch-from-origin-same-site-blocked-expected
.txt:6
> +PASS Fetch blocked. TypeError: Cancelled load because it violates the
resource's From-Origin response header.

It is disingenuous to classify this security error as a TypeError.

> LayoutTests/http/tests/from-origin/fetch-from-origin-same-site-blocked.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/image-from-origin-same-accepted.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/image-from-origin-same-blocked.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/image-from-origin-same-site-accepted.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/image-from-origin-same-site-blocked.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-accepte
d.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-accepte
d.html:19
> +	   setTimeout("iframeLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

>
LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-blocked
.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-blocked
.html:19
> +	   setTimeout("iframeLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

>
LayoutTests/http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-
accepted.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-
blocked.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/script-from-origin-same-accepted.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/script-from-origin-same-accepted.html:18
> +	   setTimeout("scriptLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

> LayoutTests/http/tests/from-origin/script-from-origin-same-blocked.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/script-from-origin-same-blocked.html:18
> +	   setTimeout("scriptLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

>
LayoutTests/http/tests/from-origin/script-from-origin-same-site-accepted.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/script-from-origin-same-site-accepted.html:1
8
> +	   setTimeout("scriptLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

>
LayoutTests/http/tests/from-origin/script-from-origin-same-site-blocked.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

>
LayoutTests/http/tests/from-origin/script-from-origin-same-site-blocked.html:18
> +	   setTimeout("scriptLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

>
LayoutTests/http/tests/from-origin/top-frame-document-from-origin-same-accepted
.php:4
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/resources/nestedIPAddressIframe.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/resources/nestedIPAddressIframe.html:15
> +    setTimeout("iframeLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

> LayoutTests/http/tests/from-origin/resources/nestedLocalhostIframe.html:1
> +<html>

I do not see the need to use quirks mode for this page. Please add <!DOCTYPE
html>.

> LayoutTests/http/tests/from-origin/resources/nestedLocalhostIframe.html:15
> +    setTimeout("iframeLoadError()", 500);

This is not a good approach as it will impose a lower bound on the how long
this test takes to run in the best case and introduces test flakiness in the
worst case. Can we think of another way to write this test without using a
timeout?

Previous message (by thread): [webkit-reviews] review granted: [Bug 184515] Add support for converting a local window to a remote window : [Attachment 338144] Patch
Next message (by thread): [webkit-reviews] review canceled: [Bug 184428] Remove access to keychain from the WebContent process : [Attachment 337544] Patch
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

More information about the webkit-reviews mailing list