Bug 161686 – ScriptElement should use FetchOptions::mode according its crossOrigin attribute

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

ScriptElement should use FetchOptions::mode according its crossOrigin attribute

[youenn fablet]

Created attachment 288134 [details]
Patch

[youenn fablet]

Created attachment 288138 [details]
Patch

[Alex Christensen]

Comment on attachment 288138 [details]
Patch

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

> LayoutTests/imported/blink/http/tests/security/script-crossorigin-loads-correctly-credentials-expected.txt:5
> -PASS
> +FAIL

This doesn't look good.  Should we change the test?  Has chromium changed this test?  Is this correct?

[youenn fablet]

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288138&action=review
> 
> > LayoutTests/imported/blink/http/tests/security/script-crossorigin-loads-correctly-credentials-expected.txt:5
> > -PASS
> > +FAIL
> 
> This doesn't look good.  Should we change the test?  Has chromium changed
> this test?  Is this correct?

I was not sure what to do with that test.
It is a blink test so it is better to not change it.

But at the same time, in blink it is expected to be served through an http server, not from the file system as here.

So maybe it is better to make a webkit test per se from it and marking PASS when script does not load.

I am very much surprised that this test is passing without the patch btw.
The origin is not right and credentials are not authorized.
Chrome and firefox catch the error, not safari :(

This patch copies this test to http/tests/security.
It updates the script as done in blink to add/not add Access-Control-Allow-Credentials.

[youenn fablet]

Created attachment 288399 [details]
Rebasing and removing blink test

[WebKit Commit Bot]

Attachment 288399 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/LoadableClassicScript.h:56:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 288138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288138&action=review
> 
> > LayoutTests/imported/blink/http/tests/security/script-crossorigin-loads-correctly-credentials-expected.txt:5
> > -PASS
> > +FAIL
> 
> This doesn't look good.  Should we change the test?  Has chromium changed
> this test?  Is this correct?

To improve clarity, I removed the blink test since it is expected to be run from a http server. To keep coverage of this test, I copied it in fast/dom and updated it to expect to fail loading.

[Build Bot]

Comment on attachment 288399 [details]
Rebasing and removing blink test

Attachment 288399 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2041496

New failing tests:
fast/dom/script-crossorigin-loads-fail-origin.html

[Build Bot]

Created attachment 288401 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 288403 [details]
Removing test as it is flaky

[youenn fablet]

(In reply to comment #10)
> Created attachment 288403 [details]
> Removing test as it is flaky

New test was flaky as it requires HTTP server to be running but loaded from file system. I am not sure if there is a way to test that kind of configuration.

[WebKit Commit Bot]

Attachment 288403 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/LoadableClassicScript.h:56:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

I seem to remember that we are intentionally doing something different with ScriptElement's cross origin checks because a lot of people on the internet use cross origin scripts wrong and expect them to behave like they do now.  I could be wrong, though.  I think someone with more experience than I needs to look at this.

[youenn fablet]

(In reply to comment #13)
> I seem to remember that we are intentionally doing something different with
> ScriptElement's cross origin checks because a lot of people on the internet
> use cross origin scripts wrong and expect them to behave like they do now. 
> I could be wrong, though.  I think someone with more experience than I needs
> to look at this.

Is it related to script loaded with data URLs?
If so, the behavior should be kept the same.
I added one test about that in this patch.

[youenn fablet]

Created attachment 288557 [details]
Readding local file test

[WebKit Commit Bot]

Attachment 288557 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/LoadableClassicScript.h:56:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to comment #15)
> Created attachment 288557 [details]
> Readding local file test

I added the flaky test in http/tests/local so that it is run while the http server is also running.

[Darin Adler]

Comment on attachment 288557 [details]
Readding local file test

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

> Source/WebCore/dom/LoadableClassicScript.cpp:72
> +    ASSERT(resource);

An assertion like this is usually a clue that the argument should be a reference rather than a pointer. We should investigate fixing that.

> Source/WebCore/loader/cache/CachedImage.cpp:-122
> -    setLoading(false);

I don’t understand this change. I looked at the change log and it didn’t explain it.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:550
>      // FIXME: We should progressively extend this to other reusable resources
> -    if (resource.type() != CachedResource::Type::ImageResource && resource.type() != CachedResource::Type::TextTrackResource)
> +    if (resource.type() != CachedResource::Type::ImageResource && resource.type() != CachedResource::Type::Script && resource.type() != CachedResource::Type::TextTrackResource)
>          return false;

Seems like this should be factored into a isReusableResource helper function, either here in this file or even possibly as a member function. Then the name of the function helps document what the list of types means.

> Source/WebCore/loader/cache/CachedScript.cpp:129
> +    const CachedScript& script = static_cast<const CachedScript&>(resource);

Instead of repeating the type twice, this should be:

    auto& script = static_cast<const CachedScript&>(resource);

[youenn fablet]

> > Source/WebCore/dom/LoadableClassicScript.cpp:72
> > +    ASSERT(resource);
> 
> An assertion like this is usually a clue that the argument should be a
> reference rather than a pointer. We should investigate fixing that.

Right.

> > Source/WebCore/loader/cache/CachedImage.cpp:-122
> > -    setLoading(false);
> 
> I don’t understand this change. I looked at the change log and it didn’t
> explain it.

It got lost when rebasing against bug 161792.
setLoading(false) is now done in CachedResource::loadFrom.


> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:550
> >      // FIXME: We should progressively extend this to other reusable resources
> > -    if (resource.type() != CachedResource::Type::ImageResource && resource.type() != CachedResource::Type::TextTrackResource)
> > +    if (resource.type() != CachedResource::Type::ImageResource && resource.type() != CachedResource::Type::Script && resource.type() != CachedResource::Type::TextTrackResource)
> >          return false;
> 
> Seems like this should be factored into a isReusableResource helper
> function, either here in this file or even possibly as a member function.
> Then the name of the function helps document what the list of types means.

There is another patch (bug 161609) that improves on that.
This check should be changed to an ASSERT really once all necessary resources support this.

> 
> > Source/WebCore/loader/cache/CachedScript.cpp:129
> > +    const CachedScript& script = static_cast<const CachedScript&>(resource);
> 
> Instead of repeating the type twice, this should be:
> 
>     auto& script = static_cast<const CachedScript&>(resource);

OK

[youenn fablet]

Created attachment 288676 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 288676 [details]
Patch for landing

Clearing flags on attachment: 288676

Committed r205854: <http://trac.webkit.org/changeset/205854>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(In reply to comment #4)
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=288138&action=review
> > 
> > > LayoutTests/imported/blink/http/tests/security/script-crossorigin-loads-correctly-credentials-expected.txt:5
> > > -PASS
> > > +FAIL
> > 
> > This doesn't look good.  Should we change the test?  Has chromium changed
> > this test?  Is this correct?
> 
> I was not sure what to do with that test.
> It is a blink test so it is better to not change it.
> 
> But at the same time, in blink it is expected to be served through an http
> server, not from the file system as here.
> 
> So maybe it is better to make a webkit test per se from it and marking PASS
> when script does not load.
> 
> I am very much surprised that this test is passing without the patch btw.
> The origin is not right and credentials are not authorized.
> Chrome and firefox catch the error, not safari :(

The reason of the test passing before the patch is due to the possibility for file:// documents to be granted universal access.
In that case, CORS checks should be overridden.
Because we were passing the origin as a string from DocumentThreadableLoader to CachedResource, we were losing the universal access. This became apparent when CORS checks moved to CachedResource.

The origin should be passed as a SecurityOrigin and not a string.
Proposed patch for bug 162151 is fixing this amongst other things.