Created attachment 285087 [details] Patch

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 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.

Created attachment 285114 [details] Patch

(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 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.

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

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.

(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 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.

(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.

Created attachment 285332 [details] Improving tests

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?

Created attachment 285405 [details] Patch for landing

(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.

Created attachment 285407 [details] Patch for landing

> > 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 on attachment 285407 [details] Patch for landing Clearing flags on attachment: 285407 Committed r204164: <http://trac.webkit.org/changeset/204164>

All reviewed patches have been landed. Closing bug.

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 ]

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

Reopening to attach test expectation change

Created attachment 402866 [details] Patch

Committed r263560: <https://trac.webkit.org/changeset/263560> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402866 [details].

<rdar://problem/64807825>