[webkit-reviews] review granted: [Bug 162819] Improve error message for Access-Control-Allow-Origin violation due to misconfigured server : [Attachment 310532] Patch and layout test

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 162819: Improve error message for Access-Control-Allow-Origin violation due
to misconfigured server
https://bugs.webkit.org/show_bug.cgi?id=162819

Attachment 310532: Patch and layout test

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 310532
  --> https://bugs.webkit.org/attachment.cgi?id=310532
Patch and layout test

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

r=me

Looks like we will have to mark the WPT tests as failures?

    imported/w3c/web-platform-tests/fetch/api/cors/cors-multiple-origins.html
   
imported/w3c/web-platform-tests/fetch/api/cors/cors-multiple-origins-worker.htm
l

> Source/WebCore/loader/CrossOriginAccessControl.cpp:161
>	       errorDescription = "Cannot use wildcard in
Access-Control-Allow-Origin when credentials flag is true.";
> +	   else if (accessControlOriginString.find(',') != notFound)
> +	       errorDescription = "Access-Control-Allow-Origin cannot contain
more than one origin.";

You can ASCIILiteral(...) these strings.

>
LayoutTests/http/tests/xmlhttprequest/resources/origin-exact-matching-iframe.ht
ml:48
> +//shouldFail(pageOrigin + "\0"); // Doesn't fail in chromium-linux. See
http://wkbug.com/88688 and http://wkbug.com/88139

Do we pass this? Seems like this was only an issue in chromium-linux, we should
be able to enable this test.

>
LayoutTests/http/tests/xmlhttprequest/resources/origin-exact-matching-iframe.ht
ml:49
> +shouldFail((pageOrigin).toUpperCase());

Nit: You can drop the parenthesis around pageOrigin now that it is not an
expression.

>
LayoutTests/http/tests/xmlhttprequest/resources/origin-exact-matching-iframe.ht
ml:71
> +shouldFail(pageOrigin + ", *");

How about adding cases starting with or ending with a comma:

    shouldFail(",");
    shouldFail(","+pageOrigin);
    shouldFail(pageOrigin+",");