Bug 160445 - [Fetch API] Activate CSP checks
Summary: [Fetch API] Activate CSP checks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 151937
  Show dependency treegraph
 
Reported: 2016-08-02 02:28 PDT by youenn fablet
Modified: 2016-08-23 04:13 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.14 KB, patch)
2016-08-02 02:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (7.21 KB, patch)
2016-08-02 09:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Adding CSP test and using the right upgradeInsecureRequestIfNeeded (11.09 KB, patch)
2016-08-03 02:38 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Improving tests (16.35 KB, patch)
2016-08-04 09:24 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (16.37 KB, patch)
2016-08-04 23:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (16.39 KB, patch)
2016-08-05 00:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-08-02 02:28:49 PDT
We should activate CSP checks for the fetch API
Comment 1 youenn fablet 2016-08-02 02:31:13 PDT
Created attachment 285087 [details]
Patch
Comment 2 Alex Christensen 2016-08-02 08:53:18 PDT
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.
Comment 3 youenn fablet 2016-08-02 08:59:42 PDT
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.
Comment 4 youenn fablet 2016-08-02 09:10:39 PDT
Created attachment 285114 [details]
Patch
Comment 5 youenn fablet 2016-08-02 09:11:46 PDT
(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.
Comment 6 Sam Weinig 2016-08-02 09:43:19 PDT
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.
Comment 7 youenn fablet 2016-08-03 02:38:23 PDT
Created attachment 285215 [details]
Adding CSP test and using the right upgradeInsecureRequestIfNeeded
Comment 8 youenn fablet 2016-08-03 02:42:16 PDT
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.
Comment 9 Alex Christensen 2016-08-03 09:34:33 PDT
(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 ?
Comment 10 Alex Christensen 2016-08-03 09:54:09 PDT
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.
Comment 11 youenn fablet 2016-08-03 10:03:03 PDT
(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.
Comment 12 youenn fablet 2016-08-04 09:24:09 PDT
Created attachment 285332 [details]
Improving tests
Comment 13 Daniel Bates 2016-08-04 10:18:54 PDT
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?
Comment 14 youenn fablet 2016-08-04 23:31:57 PDT
Created attachment 285405 [details]
Patch for landing
Comment 15 youenn fablet 2016-08-04 23:33:03 PDT
(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.
Comment 16 youenn fablet 2016-08-05 00:09:24 PDT
Created attachment 285407 [details]
Patch for landing
Comment 17 youenn fablet 2016-08-05 00:12:21 PDT
> > 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.
Comment 18 WebKit Commit Bot 2016-08-05 00:40:24 PDT
Comment on attachment 285407 [details]
Patch for landing

Clearing flags on attachment: 285407

Committed r204164: <http://trac.webkit.org/changeset/204164>
Comment 19 WebKit Commit Bot 2016-08-05 00:40:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Alexey Proskuryakov 2016-08-22 15:21:16 PDT
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 ]