Bug 160445 – [Fetch API] Activate CSP checks

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

We should activate CSP checks for the fetch API

[youenn fablet]

Created attachment 285087 [details]
Patch

[Alex Christensen]

Comment on attachment 285087 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchLoader.cpp:84
> +    ResourceRequest fetchRequest = request.internalRequest();
> +    URL url = fetchRequest.url();

Use references.  This creates a copy of the ResourceRequest, then possibly upgrades it, then doesn't use the newly-secure request for anything but blocking.  If we should upgrade the insecure request, we should use the upgraded request.

[youenn fablet]

Comment on attachment 285087 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchLoader.cpp:95
>      m_loader = ThreadableLoader::create(context, *this, request.internalRequest(), options);

Woups, I forgot to move fetchRequest in ThreadableLoader::create, thanks!
I'll fix it.

[youenn fablet]

Created attachment 285114 [details]
Patch

[youenn fablet]

(In reply to comment #4)
> Created attachment 285114 [details]
> Patch

Fixed the issue.
The fact that the tests do not pick that one clearly shows that there are missing tests  here. I'll work on that tomorrow.

[Sam Weinig]

Comment on attachment 285114 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchLoader.cpp:86
> +    context.contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(fetchRequest.url(), ContentSecurityPolicy::InsecureRequestType::Load);

This makes me nervous.  For the most part, ResourceRequest's members should be set with their setters, so that the flags, like m_platformRequestUpdated, be set correctly.  Passing in a reference to the requests URL seems suspect.  Can we instead just pass in the ResourceRequest itself, since upgradeInsecureRequestIfNeeded is overloaded to take a request, and have it do the correct, get URL, modify, set URL dance?

> Source/WebCore/platform/network/ResourceRequestBase.cpp:111
> +URL& ResourceRequestBase::url()
> +{
> +    updateResourceRequest();
> +
> +    return m_url;
> +}

For the reasons above, this seems dangerous.

[youenn fablet]

Created attachment 285215 [details]
Adding CSP test and using the right upgradeInsecureRequestIfNeeded

[youenn fablet]

Thanks for the review.

(In reply to comment #6)
> Comment on attachment 285114 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285114&action=review
> 
> > Source/WebCore/Modules/fetch/FetchLoader.cpp:86
> > +    context.contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(fetchRequest.url(), ContentSecurityPolicy::InsecureRequestType::Load);
> 
> This makes me nervous.  For the most part, ResourceRequest's members should
> be set with their setters, so that the flags, like m_platformRequestUpdated,
> be set correctly.  Passing in a reference to the requests URL seems suspect.
> Can we instead just pass in the ResourceRequest itself, since
> upgradeInsecureRequestIfNeeded is overloaded to take a request, and have it
> do the correct, get URL, modify, set URL dance?

Got it.
Patch is using upgradeInsecureRequestIfNeeded(ResourceRequest&).

In the end, I would like to migrate those calls just before preflighting if possible.
That would mean in DocumentThreadabeLoader.

> Fixed the issue.
> The fact that the tests do not pick that one clearly shows that there are
> missing tests  here. I'll work on that tomorrow.

I added a fetch-specific test. It only works for WK1.
There might be bug in WK2 runtime flags handling: fetch is defined in the main window but not the pop-up window.

[Alex Christensen]

(In reply to comment #8)
> I added a fetch-specific test. It only works for WK1.
> There might be bug in WK2 runtime flags handling: fetch is defined in the
> main window but not the pop-up window.

Does it work in WK2 after landing https://bugs.webkit.org/show_bug.cgi?id=160452 ?

[Alex Christensen]

Comment on attachment 285215 [details]
Adding CSP test and using the right upgradeInsecureRequestIfNeeded

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

Also please add a test like insecure-fetch-in-main-frame-window.html but from workers.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-fetch-in-main-frame-window.html:7
> +    fetch("https://127.0.0.1:8443/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-fetch-in-main-frame-window.html").then(() => {

The point of this test is to make an http request and have it succeed because it is upgraded to a https request.  Here we try to make an https request, and nothing interesting happens.  Replace https with http.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-xhr-in-main-frame-window.html:14
> -    setTimeout(function() {
> +    xhr.onloadend = function() {

It might be nice to add a setTimeout(function() { console.log("timeout"); if (window.testRunner) testRunner.notifyDone(); }, 2000);
That way, if this ever breaks, it will be easier to investigate what is going on.
Also, we should add a console.log("unloadend") here to distinguish our behavior from Firefox's.

[youenn fablet]

(In reply to comment #9)
> (In reply to comment #8)
> > I added a fetch-specific test. It only works for WK1.
> > There might be bug in WK2 runtime flags handling: fetch is defined in the
> > main window but not the pop-up window.
> 
> Does it work in WK2 after landing
> https://bugs.webkit.org/show_bug.cgi?id=160452 ?

Yes

I'll update the tests.

[youenn fablet]

Created attachment 285332 [details]
Improving tests

[Daniel Bates]

Comment on attachment 285332 [details]
Improving tests

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

> Source/WebCore/ChangeLog:12
> +        (WebCore::FetchLoader::start): Adding CSP and url upgrade checks.

Nit: url => URL

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-fetch-in-main-frame-window.html:1
> +<html>

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

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-fetch-in-main-frame-window.html:26
> +    setTimeout(() => {
> +        if (testIsFinished)
> +            return;
> +        alert("Test timed out");
> +        if (window.testRunner)
> +            testRunner.notifyDone();
> +    }, 2000);

Can we avoid using a 2 second timeout? Even though it seems reasonable to expect the XHR to load before 2 seconds I do not see the need to explicitly enforce this timeout. It seems sufficient to let the test runner/run-webkit-tests determine when the test is considered to have timed out. If you feel it is beneficial to provide a JavaScript alert to tell a human being that the test timed out then we should make this setTimeout() conditional on running outside a test tool (i.e window.testRunner is not defined) and we should add some English text to explain the purpose of this test and how to interpret the results.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-xhr-in-main-frame-window.html:15
> +        alert("onloadend called");

The showing of this alert is characteristic with the successful run of this test. We should add the prefix "PASS: " to this message to convey that the alert indicates expected behavior.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-xhr-in-main-frame-window.html:34
> +    setTimeout(() => {
> +        if (testIsFinished)
> +            return;
> +        alert("Test timed out");
> +        if (window.testRunner)
> +            testRunner.notifyDone();
> +    }, 2000);

Similar to my remark about setTimeout() in test LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-fetch-in-main-frame-window.html, can we avoid using a 2 second timeout?

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-main-frame.html:13
> +<p>This test opens a HTTPS window that loads insecure data via fetch API.  We should upgrade

a => an
"via fetch" => "via the Fetch API"

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-main-frame.html:14
> +this request and thereby avoid a mixed content callback.</p>

callback?

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-main-frame.html:18
> +onload = function() {
> +    window.open("https://127.0.0.1:8443/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-fetch-in-main-frame-window.html");
> +}

I do not see the need to use an onload handler given that this script is at the end of <body>.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-worker.html:1
> +<html>

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

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-worker.html:18
> +], { type: "text/javascript" })

Missing ';' at the end of this line.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-worker.html:22
> +worker.onmessage =function(e) {

Nit: Missing space character after the '='.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-worker.html:35
> +setTimeout(() => {
> +    if (testIsFinished)
> +        return;
> +    alert("Test timed out");
> +    if (window.testRunner)
> +        testRunner.notifyDone();
> +}, 2000);

Similar to my remark about setTimeout() in test LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/resources/insecure-fetch-in-main-frame-window.html, can we avoid using a 2 second timeout?

[youenn fablet]

Created attachment 285405 [details]
Patch for landing

[youenn fablet]

(In reply to comment #14)
> Created attachment 285405 [details]
> Patch for landing

Thanks for the review.
Updated patch is taking care of all points.
I kept the setTimeout for normal environments for debug purposes.

[youenn fablet]

Created attachment 285407 [details]
Patch for landing

[youenn fablet]

> > LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-worker.html:35
> > +setTimeout(() => {
> > +    if (testIsFinished)
> > +        return;
> > +    alert("Test timed out");
> > +    if (window.testRunner)
> > +        testRunner.notifyDone();
> > +}, 2000);
> 
> Similar to my remark about setTimeout() in test
> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-
> requests/resources/insecure-fetch-in-main-frame-window.html, can we avoid
> using a 2 second timeout?

Due to bug 155132, this test is timing out in Mac WK2.

[WebKit Commit Bot]

Comment on attachment 285407 [details]
Patch for landing

Clearing flags on attachment: 285407

Committed r204164: <http://trac.webkit.org/changeset/204164>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

There are entries in TestExpectations referencing this bug, please clean them up:

$ find-expectations 160445
LayoutTests/platform/mac-wk2/TestExpectations:380:webkit.org/b/160445 http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-worker.html [ Failure Timeout ]
LayoutTests/platform/wk2/TestExpectations:45:webkit.org/b/160445 http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-main-frame.html [ Timeout ]

[Karl Rackler]

http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-main-frame.html and http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-fetch-in-worker.html is no longer failing - remove expectations

Current history is green: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Fupgrade-insecure-requests%2Fupgrade-insecure-fetch-in-main-frame.html&test=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Fupgrade-insecure-requests%2Fupgrade-insecure-fetch-in-worker.html

[Truitt Savell]

Reopening to attach test expectation change

[Karl Rackler]

Created attachment 402866 [details]
Patch

[EWS]

Committed r263560: <https://trac.webkit.org/changeset/263560>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402866 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64807825>