Bug 143653 – Implement CSP upgrade-insecure-requests directive

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[sideshowbarker]

See https://w3c.github.io/webappsec/specs/upgrade/

“Instead of blocking mixed content, this will automatically upgrade it, helping sites with lots of legacy content to more easily move to TLS without having to worry about mixed content warnings in their UI.” (description from Anne van Kesteren)

Blink has already landed support for this and will ship it in Chrome 43: https://www.chromestatus.com/features/6534575509471232
Mozilla has an assigned bug open for it with implementation work in progress: https://bugzilla.mozilla.org/show_bug.cgi?id=1139297
Spartan/IE has it "under consideration" https://status.modern.ie/upgradeinsecureresourcerequests
Opera has already shipped it in Opera 30

[sideshowbarker]

Note that this has landed now in gecko https://bugzilla.mozilla.org/show_bug.cgi?id=1139297 and will ship in Firefox 42. And Chrome already shipped it in Chrome 43.

[Radar WebKit Bug Importer]

<rdar://problem/25201674>

[Brent Fulgham]

Created attachment 279146 [details]
Patch (no new tests yet)

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:16:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 12 files


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

[Brent Fulgham]

An initial stab at doing this. It works well enough to show me that it's properly redirecting the content of the demo site <https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/>.

I have a few questions about the current patch:
1. Should we be applying the upgrade logic to outbound targets? I assumed that we would want to...
2. I think the specification wants us to 'advertise' support for upgrade-insecure-requests, so I still need to do that.
3. Should we be logging when we make this kind of URL upgrade?

[Brent Fulgham]

Created attachment 279149 [details]
Patch (no tests, fixed gtk/efl build)

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:16:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 12 files


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

[Daniel Bates]

Comment on attachment 279149 [details]
Patch (no tests, fixed gtk/efl build)

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

This looks like a reasonable first pass. We need to support upgrading WebSockets per <http://www.w3.org/TR/upgrade-insecure-requests/#websockets-integration> and we need tests.

> Source/WebCore/html/HTMLMediaElement.cpp:1348
> +    if (frame->document() && frame->document()->contentSecurityPolicy()->shouldUpgradeInsecureRequests(url)) {
> +        ContentSecurityPolicy::upgradeInsecureRequestURL(url);
> +        request.setURL(url);
> +    }

Every caller of ContentSecurityPolicy::shouldUpgradeInsecureRequests() calls ContentSecurityPolicy::upgradeInsecureRequestURL(). I suggest that we incorporate the functionality of these two functions into one function that takes a reference to a ResourceRequest object and adjusts its URL if necessary. Then we can write code such as above as:

if (frame->document())
    frame->document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request);

[Daniel Bates]

(In reply to comment #8)
...
> I suggest that we
> incorporate the functionality of these two functions into one function that
> takes a reference to a ResourceRequest object and adjusts its URL if
> necessary.

This design will not work out for WebSockets. One idea is to consider an overload that takes a reference to a URL. Alternatively, we could use your original design with two functions.

[Daniel Bates]

On another note, should we be upgrading XHR?

[Brent Fulgham]

Comment on attachment 279149 [details]
Patch (no tests, fixed gtk/efl build)

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

>> Source/WebCore/html/HTMLMediaElement.cpp:1348
>> +    }
> 
> Every caller of ContentSecurityPolicy::shouldUpgradeInsecureRequests() calls ContentSecurityPolicy::upgradeInsecureRequestURL(). I suggest that we incorporate the functionality of these two functions into one function that takes a reference to a ResourceRequest object and adjusts its URL if necessary. Then we can write code such as above as:
> 
> if (frame->document())
>     frame->document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request);

Wil do!

[Brent Fulgham]

(In reply to comment #9)
> (In reply to comment #8)
> ...
> > I suggest that we
> > incorporate the functionality of these two functions into one function that
> > takes a reference to a ResourceRequest object and adjusts its URL if
> > necessary.
> 
> This design will not work out for WebSockets. One idea is to consider an
> overload that takes a reference to a URL. Alternatively, we could use your
> original design with two functions.

Yes, that's the approach I took. I don't think it's a bad approach, since WebSocket is distinct from the standard http->https case.

[Brady Eidson]

(In reply to comment #5)
> An initial stab at doing this. It works well enough to show me that it's
> properly redirecting the content of the demo site
> <https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/>.
> 
> I have a few questions about the current patch:
> 1. Should we be applying the upgrade logic to outbound targets? I assumed
> that we would want to...

I don't know what an "outbound target" is. The entirety of WebCore doesn't have one instance of the word "outbound".

Could you clarify what you mean in WebCore terms?

> 2. I think the specification wants us to 'advertise' support for
> upgrade-insecure-requests, so I still need to do that.

Okay.

> 3. Should we be logging when we make this kind of URL upgrade?

I don't think so. My gut reaction is a moderately strong "no", but if other browsers do we could consider it.

[Brady Eidson]

(In reply to comment #10)
> On another note, should we be upgrading XHR?

Yes.

[Brent Fulgham]

(In reply to comment #13)
> (In reply to comment #5)
> > An initial stab at doing this. It works well enough to show me that it's
> > properly redirecting the content of the demo site
> > <https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/>.
> > 
> > I have a few questions about the current patch:
> > 1. Should we be applying the upgrade logic to outbound targets? I assumed
> > that we would want to...
> 
> I don't know what an "outbound target" is. The entirety of WebCore doesn't
> have one instance of the word "outbound".

I'm thinking of "PingLoader::sendPing" and "PingLoader::sendViolationReport", both of which do a POST.

[Brady Eidson]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #5)
> > > An initial stab at doing this. It works well enough to show me that it's
> > > properly redirecting the content of the demo site
> > > <https://googlechrome.github.io/samples/csp-upgrade-insecure-requests/>.
> > > 
> > > I have a few questions about the current patch:
> > > 1. Should we be applying the upgrade logic to outbound targets? I assumed
> > > that we would want to...
> > 
> > I don't know what an "outbound target" is. The entirety of WebCore doesn't
> > have one instance of the word "outbound".
> 
> I'm thinking of "PingLoader::sendPing" and
> "PingLoader::sendViolationReport", both of which do a POST.

Still unfamiliar with "outbound target". But, yes, it applies to them too.

[Brent Fulgham]

Created attachment 279192 [details]
Patch. Still no tests. Add advertising header and additional load cases.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:16:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 21 files


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

[Daniel Bates]

Comment on attachment 279192 [details]
Patch. Still no tests. Add advertising header and additional load cases.

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

We still need tests for this change.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:787
> +void ContentSecurityPolicy::upgradeInsecureHTTPURLIfNeeded(URL& url)
> +{
> +    if (!m_upgradeInsecureRequests || !url.protocolIs("http"))
> +        return;
>      
> +    url.setProtocol("https");
> +    if (url.port() == 80)
> +        url.setPort(443);
> +}
> +
> +void ContentSecurityPolicy::upgradeInsecureWebSocketURLIfNeeded(URL& url)
> +{
> +    if (!m_upgradeInsecureRequests || !url.protocolIs("ws"))
> +        return;
> +
> +    url.setProtocol("wss");
> +    if (url.port() == 80)
> +        url.setPort(443);
> +}

How did you come to the decision to define distinct member functions for upgrading a URL used by a Web Socket and upgrading a non-Web Socket ("HTTP") URL? I suggest that we incorporate the functionality of these two functions into a single function that knows how to upgrade either a ws: or http: URL. I would name this function upgradeInsecureRequestIfNeeded such that it is an overloaded function. (Even though this function would have the same name as the overloaded-variant that takes a ResourceRequest object I do not feel that the use of the word "request" in the URL-overloaded variant would be confusing or an abuse of the word "request" because the prefix of the function "upgradeInsecureRequest" can be interpreted as the name of the spec/concept this function implements as opposed to a concatenation of disjoint words.)

> Source/WebCore/page/csp/ContentSecurityPolicy.h:152
> +    void setUpgradeInsecureRequests(bool policy) { m_upgradeInsecureRequests = policy; }

This OK as-is. We tend to name the argument of a setter after the name of the setter (which itself is influenced by the name of the instance variable). This unwritten convention is also depicted in the examples in the WebKit Code Style Guidelines. Following this convention, "policy" would be renamed "upgradeInsecureRequests".

> Source/WebCore/page/csp/ContentSecurityPolicy.h:156
> +    void upgradeInsecureHTTPURLIfNeeded(URL&);
> +    void upgradeInsecureWebSocketURLIfNeeded(URL&);

Can we consolidate these into a single function that takes a reference to a URL?

[Brent Fulgham]

Comment on attachment 279192 [details]
Patch. Still no tests. Add advertising header and additional load cases.

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

>> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:787
>> +}
> 
> How did you come to the decision to define distinct member functions for upgrading a URL used by a Web Socket and upgrading a non-Web Socket ("HTTP") URL? I suggest that we incorporate the functionality of these two functions into a single function that knows how to upgrade either a ws: or http: URL. I would name this function upgradeInsecureRequestIfNeeded such that it is an overloaded function. (Even though this function would have the same name as the overloaded-variant that takes a ResourceRequest object I do not feel that the use of the word "request" in the URL-overloaded variant would be confusing or an abuse of the word "request" because the prefix of the function "upgradeInsecureRequest" can be interpreted as the name of the spec/concept this function implements as opposed to a concatenation of disjoint words.)

This sounds very reasonable. I will make that change.

>> Source/WebCore/page/csp/ContentSecurityPolicy.h:152
>> +    void setUpgradeInsecureRequests(bool policy) { m_upgradeInsecureRequests = policy; }
> 
> This OK as-is. We tend to name the argument of a setter after the name of the setter (which itself is influenced by the name of the instance variable). This unwritten convention is also depicted in the examples in the WebKit Code Style Guidelines. Following this convention, "policy" would be renamed "upgradeInsecureRequests".

Will fix.

>> Source/WebCore/page/csp/ContentSecurityPolicy.h:156
>> +    void upgradeInsecureWebSocketURLIfNeeded(URL&);
> 
> Can we consolidate these into a single function that takes a reference to a URL?

Yes, done.

[Brent Fulgham]

Created attachment 279324 [details]
Patch

[Daniel Bates]

Comment on attachment 279324 [details]
Patch

Please add tests for XHR and WebSocket. We should also add tests to ensure that upgrade insecure requests works correctly on pages with mixed content. Some mixed content tests are in LayoutTests/http/tests/security/mixedContent. Maybe we can take inspiration from the tests in LayoutTests/http/tests/security/mixedContent or copy them and incorporate logic to opt-into upgrading insecure requests. I suggest that we add tests where the insecure scheme (http, ws) is in mixed case (e.g. HttP) to ensure we do not regress upgrading such URLs.

[Build Bot]

Comment on attachment 279324 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-nested.html
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-subresource.html
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-auxiliary.html
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-top-level.html

[Build Bot]

Created attachment 279331 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 279324 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-nested.html
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-subresource.html
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-auxiliary.html
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-top-level.html

[Build Bot]

Created attachment 279332 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 279324 [details]
Patch

Attachment 279324 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1345265

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-nested.html
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-subresource.html
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-auxiliary.html
http/tests/security/contentSecurityPolicy/1.1/upgrade-insecure-requests/https-header-top-level.html

[Build Bot]

Created attachment 279333 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 279324 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279334 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Brent Fulgham]

Deficiencies in our testing infrastructure make it impossible to properly test secure WebSocket features. I just filed Bug 157884 to deal with this.

[Brent Fulgham]

(In reply to comment #22)
> Comment on attachment 279324 [details]
> Patch
> 
> Please add tests for XHR and WebSocket. We should also add tests to ensure
> that upgrade insecure requests works correctly on pages with mixed content.
> Some mixed content tests are in
> LayoutTests/http/tests/security/mixedContent. Maybe we can take inspiration
> from the tests in LayoutTests/http/tests/security/mixedContent or copy them
> and incorporate logic to opt-into upgrading insecure requests. I suggest
> that we add tests where the insecure scheme (http, ws) is in mixed case
> (e.g. HttP) to ensure we do not regress upgrading such URLs.

I was unable to comply with the request to add 'wss' tests due to a test
infrastructure deficiency (see Bug 157884).

I did add the mixed-case examples, mixedContent tests, and XHR tests.

[Brent Fulgham]

Created attachment 279362 [details]
Patch

[Build Bot]

Comment on attachment 279362 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-css-in-iframe.html

[Build Bot]

Created attachment 279370 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 279362 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-css-in-iframe.html

[Build Bot]

Created attachment 279371 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 279362 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-simple.html
http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple.html
http/tests/workers/text-encoding.html
http/tests/xmlhttprequest/workers/referer.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-overrides.html
http/tests/security/contentSecurityPolicy/worker-without-own-csp.html
http/tests/xmlhttprequest/workers/methods.html
http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html
fast/workers/worker-inherits-csp-blocks-xhr.html
http/tests/security/isolatedWorld/bypass-worker-csp-for-xhr-redirect-cross-origin.html
http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html
http/tests/security/contentSecurityPolicy/worker-connect-src-allowed.html
http/tests/xmlhttprequest/workers/xmlhttprequest-file-not-found.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-synconworker.html
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-css-in-iframe.html
http/tests/xmlhttprequest/workers/close.html
http/tests/security/isolatedWorld/bypass-main-world-csp-worker-blob-xhr.html
imported/w3c/web-platform-tests/XMLHttpRequest/open-url-worker-simple.htm
http/tests/security/contentSecurityPolicy/worker-multiple-csp-headers.html
http/tests/xmlhttprequest/workers/methods-async.html
imported/blink/storage/indexeddb/blob-basics-metadata.html
fast/files/workers/worker-apply-blob-url-to-xhr.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-twice.html
http/tests/security/isolatedWorld/bypass-worker-csp-for-xhr.html
imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-overridesexpires.html

[Build Bot]

Created attachment 279373 [details]
Archive of layout-test-results from ews114 for mac-yosemite

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

[Brent Fulgham]

Debug layout tests showed that the XMLHttpRequest code was wrong, causing a debug assertion when the context had no document. Instead, I should have been using the scriptExecutionContext, as I had correctly done for WebSockets.

[Brent Fulgham]

Created attachment 279400 [details]
Patch

[Brent Fulgham]

<rdar://problem/23032067>

[Brent Fulgham]

Created attachment 279468 [details]
Patch

[Brent Fulgham]

Comment on attachment 279468 [details]
Patch

Added WebSocket tests (currently disabled until Bug 157884 can land).

[Brady Eidson]

That's a lot of green! \o/

[Brady Eidson]

(In reply to comment #45)
> That's a lot of green! \o/

(I meant in EWS, not in the patch itself)

[Brent Fulgham]

(In reply to comment #46)
> (In reply to comment #45)
> > That's a lot of green! \o/
> 
> (I meant in EWS, not in the patch itself)

At last! Now if I can just get Dan to approve it ;)

[Daniel Bates]

Comment on attachment 279468 [details]
Patch

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

Thank you for adding a Web Socket test. There are correctness issues in this patch and I noticed some minor cosmetic issues.

With regards to the correctness of the patch, we are not maintaining upgrade insecure navigations sets or propagating the upgrade insecure request policy to nested browsing contexts per <https://w3c.github.io/webappsec-upgrade-insecure-requests/#nesting> such that sub-resource URLs are upgraded in the upgraded nested browsing context or at least we do not seem to have any tests to check this behavior. We do not conform to the reporting requirements for Upgrade Insecure Requests, <https://w3c.github.io/webappsec-upgrade-insecure-requests/#reporting-upgrades>, which prevents web authors from making use of CSP to track down insecure requests. We also enforce the upgrade-insecure-request when delivered in a report-only policy. But the upgrade-insecure-request directive should be ignored when delivered in a report-only policy.  From briefly looking through the code change I was unclear if hyperlinks that navigate a target browsing context are upgraded (e.g. <a href="http://www.apple.com" target="some-frame">). If we handle this then should add tests to check that on a page served with upgrade-insecure-requests click()ing on a cross-origin HTTP hyperlink that targets a frame and click()ing on a cross-origin HTTP hyperlink that does not target a frame (i.e. starts a top-level navigation) is upgraded and not-upgraded, respectively per <https://w3c.github.io/webappsec-upgrade-insecure-requests/#algorithms>. 

With regarding to implementing the reporting requirements, <https://w3c.github.io/webappsec-upgrade-insecure-requests/#reporting-upgrades>, we may choose to do this in a follow-up bug so long as we identify a way to implement such support.

> Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:441
> +    if (m_upgradeInsecureRequests) {
> +        m_policy.reportDuplicateDirective(name);
> +        return;
> +    }

We need to ignore directive upgrade-insecure-request when defined in a report-only policy per the last paragraph of section The upgrade-insecure-requests Content Security Policy directive of the Upgrade Insecure Requests spec. (Editor's Draft, 11 May 2016), <https://w3c.github.io/webappsec-upgrade-insecure-requests/#delivery>. Towards this we should use a similar approach as we do when ignoring the directive sandbox in ContentSecurityPolicyDirectiveList::applySandboxPolicy().

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/basic-upgrade-cors.https.html:1
> +<html>

Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/basic-upgrade-cors.https.html:11
> +<iframe src="https://127.0.0.1:8443/security/contentSecurityPolicy/upgrade-insecure-requests/resources/basic-upgrade-cors.https.html";

This markup in this line is invalid. Specifically the placement of ';' makes this line invalid.

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

Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

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

Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-css-in-iframe.html:12
> +<iframe src="https://127.0.0.1:8443/security/contentSecurityPolicy/upgrade-insecure-requests/resources/frame-with-insecure-css.html";

This markup in this line is invalid. Specifically the placement of ';' makes this line invalid.

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

Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-redirect-http-to-https-script-in-iframe.html:1
> +<html>

Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-redirect-http-to-https-script-in-iframe.html:12
> +<iframe src="https://127.0.0.1:8443/security/contentSecurityPolicy/upgrade-insecure-requests/resources/frame-with-redirect-http-to-https-script.html";

This markup in this line is invalid. Specifically the placement of ';' makes this line invalid.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-redirect-https-to-http-script-in-iframe.html:1
> +<html>

Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-redirect-https-to-http-script-in-iframe.html:13
> +<iframe src="https://127.0.0.1:8443/security/contentSecurityPolicy/upgrade-insecure-requests/resources/frame-with-redirect-https-to-http-script.html";
> +></iframe>

Ditto.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-simple-ws.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Minor: I know that you created this file by either copying LayoutTests/http/tests/websocket/tests/hybi/simple.html or copying LayoutTests/http/tests/websocket/tests/hybi/simple-wss.html included in the patch attachment #279418 [details] (bug #157884). I suggest that we use the HTML5 doctype, <!DOCTYPE html> as I do not see the need to run this test in quirks mode.

> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-simple-ws.html:48
> +var timeoutID = setTimeout(timeOutCallback, 3000);

Can we get rid of this timeout or at least use a reduce the timeout period? Waiting 3 seconds for the timeout seems excessive.

[Brent Fulgham]

Comment on attachment 279468 [details]
Patch

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

>> Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:441
>> +    }
> 
> We need to ignore directive upgrade-insecure-request when defined in a report-only policy per the last paragraph of section The upgrade-insecure-requests Content Security Policy directive of the Upgrade Insecure Requests spec. (Editor's Draft, 11 May 2016), <https://w3c.github.io/webappsec-upgrade-insecure-requests/#delivery>. Towards this we should use a similar approach as we do when ignoring the directive sandbox in ContentSecurityPolicyDirectiveList::applySandboxPolicy().

Ah! Makes sense. I duplicated http/tests/security/contentSecurityPolicy/report-only.php to http/tests/security/contentSecurityPolicy/report-only-upgrade-insecure.php.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/basic-upgrade-cors.https.html:1
>> +<html>
> 
> Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

Fixed!

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/basic-upgrade-cors.https.html:11
>> +<iframe src="https://127.0.0.1:8443/security/contentSecurityPolicy/upgrade-insecure-requests/resources/basic-upgrade-cors.https.html";
> 
> This markup in this line is invalid. Specifically the placement of ';' makes this line invalid.

Weird. I wonder why I did that? Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-audio-video-in-main-frame.html:1
>> +<html>
> 
> Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-css-in-iframe.html:1
>> +<html>
> 
> Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-css-in-iframe.html:12
>> +<iframe src="https://127.0.0.1:8443/security/contentSecurityPolicy/upgrade-insecure-requests/resources/frame-with-insecure-css.html";
> 
> This markup in this line is invalid. Specifically the placement of ';' makes this line invalid.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-insecure-xhr-in-main-frame.html:1
>> +<html>
> 
> Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-redirect-http-to-https-script-in-iframe.html:1
>> +<html>
> 
> Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-redirect-http-to-https-script-in-iframe.html:12
>> +<iframe src="https://127.0.0.1:8443/security/contentSecurityPolicy/upgrade-insecure-requests/resources/frame-with-redirect-http-to-https-script.html";
> 
> This markup in this line is invalid. Specifically the placement of ';' makes this line invalid.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-redirect-https-to-http-script-in-iframe.html:1
>> +<html>
> 
> Minor: I do not see the need to run this test in quirks mode. Please add <!DOCTYPE html> to the top of this file.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-redirect-https-to-http-script-in-iframe.html:13
>> +></iframe>
> 
> Ditto.

Fixed.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-simple-ws.html:1
>> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> Minor: I know that you created this file by either copying LayoutTests/http/tests/websocket/tests/hybi/simple.html or copying LayoutTests/http/tests/websocket/tests/hybi/simple-wss.html included in the patch attachment #279418 [details] (bug #157884). I suggest that we use the HTML5 doctype, <!DOCTYPE html> as I do not see the need to run this test in quirks mode.

Done.

>> LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-simple-ws.html:48
>> +var timeoutID = setTimeout(timeOutCallback, 3000);
> 
> Can we get rid of this timeout or at least use a reduce the timeout period? Waiting 3 seconds for the timeout seems excessive.

Sure -- I'll drop it. I'll revisit it when we activate this test with Bug 157884.

[Brent Fulgham]

Created attachment 279623 [details]
Patch

[Brent Fulgham]

(In reply to comment #48)
> Comment on attachment 279468 [details]
>
> We do not conform to the reporting requirements for Upgrade Insecure Requests,
> <https://w3c.github.io/webappsec-upgrade-insecure-requests/#reporting-
> upgrades>, which prevents web authors from making use of CSP to track down
> insecure requests.

I decided to tackle this part of the specification in a separate stage (Bug 158012), because this patch is already getting quite large and is becoming unwieldy.

I think this change now properly implements the specification (or at least the specification as I understand it), and is worth integrating so we can start getting some experience with this feature.

[Daniel Bates]

Comment on attachment 279623 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:5177
> +    } else if (openerDocument)
> +        contentSecurityPolicy()->populateInsecureNavigationRequestsToUpgradeFromOther(*openerDocument->contentSecurityPolicy());

Should the opener document inherit the navigation sets?

> Source/WebCore/loader/DocumentWriter.cpp:155
> +    // Per <http://www.w3.org/TR/upgrade-insecure-requests/>, we need to retain an ongoing set of upgraded
> +    // requests in new navigation contexts from
> +    HashSet<String> insecureNavigationRequestsToUpgrade;
> +    bool upgradeInsecureRequests = false;
> +    if (auto* existingDocument = m_frame->document()) {
> +        upgradeInsecureRequests = existingDocument->contentSecurityPolicy()->upgradeInsecureRequests();
> +        insecureNavigationRequestsToUpgrade = existingDocument->contentSecurityPolicy()->insecureNavigationRequestsToUpgrade();
> +    }
> +

It would be good to better understand the need for this code given that we inherited the policy in Document::Document().

> Source/WebCore/loader/FrameLoader.cpp:969
> +        if (m_opener)
> +            m_frame.document()->contentSecurityPolicy()->populateInsecureNavigationRequestsToUpgradeFromOther(*m_opener->document()->contentSecurityPolicy());
> +    }

Similarly, we should understand if this is needed given the inheritance that we do in Document::Document().

> Source/WebCore/page/DOMWindow.cpp:2138
> +    activeDocument->contentSecurityPolicy()->upgradeInsecureNavigationRequestIfNeeded(completedURL);

The spec seems unclear whether we should upgrade such a navigation ("Processing Algorithms" step 2(3)). What does Chrome and Firefox do? Maybe we should write to webappsec?

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:98
> +    if (auto* other = scriptExecutionContext.contentSecurityPolicy())
> +        populateInsecureNavigationRequestsToUpgradeFromOther(*other);

Is this necessary given the ordering of how we instantiate a ContentSecurityPolicy object and inherit from an existing policy?

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:107
> +    if (Document* document = frame ? frame->document() : nullptr)
> +        populateInsecureNavigationRequestsToUpgradeFromOther(*document->contentSecurityPolicy());

Ditto.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:789
> +
> +static String hostAndPortAsString(const String& host, unsigned short port)
> +{
> +    StringBuilder builder;
> +    builder.append(host);
> +    builder.append('_');
> +    builder.appendNumber(port);
> +    return builder.toString();
> +}

Can we use a HashSet of SecurityOrigins instead of building a string?

[Brent Fulgham]

Created attachment 280027 [details]
Patch (all comments integrated).

[Brent Fulgham]

Comment on attachment 280027 [details]
Patch (all comments integrated).

Hopefully final version of this patch with Dan and Andy's comments incorporated.

[Brent Fulgham]

Comment on attachment 279623 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:5177
>> +        contentSecurityPolicy()->populateInsecureNavigationRequestsToUpgradeFromOther(*openerDocument->contentSecurityPolicy());
> 
> Should the opener document inherit the navigation sets?

Yes -- this code is correct.

>> Source/WebCore/loader/DocumentWriter.cpp:155
>> +
> 
> It would be good to better understand the need for this code given that we inherited the policy in Document::Document().

Whatever content that was pulled over from the frame during construction is discarded in the "m_frame->loader().clear(document.ptr()..." call in the next line. We have to capture the 'upgrade-insecure-requests' state from the parent so we can properly pass it along to the new document.

>> Source/WebCore/loader/FrameLoader.cpp:969
>> +    }
> 
> Similarly, we should understand if this is needed given the inheritance that we do in Document::Document().

This isn't necessary, as all the steps here get taken during 'initSecurityContext', so I can get rid of this.

>> Source/WebCore/page/DOMWindow.cpp:2138
>> +    activeDocument->contentSecurityPolicy()->upgradeInsecureNavigationRequestIfNeeded(completedURL);
> 
> The spec seems unclear whether we should upgrade such a navigation ("Processing Algorithms" step 2(3)). What does Chrome and Firefox do? Maybe we should write to webappsec?

I performed this experiment with Chrome and Firefox, and confirmed that they do not upgrade the connection for these top level navigations. Consequently, this line is not needed.

>> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:98
>> +        populateInsecureNavigationRequestsToUpgradeFromOther(*other);
> 
> Is this necessary given the ordering of how we instantiate a ContentSecurityPolicy object and inherit from an existing policy?

No -- it is nearly always empty, and is always override by the containing document (where appropriate). I'll remove this.

>> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:107
>> +        populateInsecureNavigationRequestsToUpgradeFromOther(*document->contentSecurityPolicy());
> 
> Ditto.

Ditto.

>> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:789
>> +}
> 
> Can we use a HashSet of SecurityOrigins instead of building a string?

Unfortunately, no. I thought SecurityOrigins were cached in some global fashion, but I was wrong. We use them elsewhere because we get them from the document where they live as long as the document does. These come from temporary URLs that we can't hold onto.

[Brent Fulgham]

Comment on attachment 280027 [details]
Patch (all comments integrated).

Just in case this got obscured by my updates to the last code review, the current patch incorporates all existing comments and suggestions from Dan and Andy.

Hopefully this version will be ready to land!

[Build Bot]

Comment on attachment 280027 [details]
Patch (all comments integrated).

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

New failing tests:
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-simple-ws.html

[Build Bot]

Created attachment 280034 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 280027 [details]
Patch (all comments integrated).

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

New failing tests:
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-simple-ws.html

[Build Bot]

Created attachment 280035 [details]
Archive of layout-test-results from ews114 for mac-yosemite

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

[Build Bot]

Comment on attachment 280027 [details]
Patch (all comments integrated).

Attachment 280027 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1395520

New failing tests:
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-simple-ws.html

[Build Bot]

Created attachment 280036 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Brent Fulgham]

Created attachment 280164 [details]
Patch (WSS test fix)

[Build Bot]

Comment on attachment 280164 [details]
Patch (WSS test fix)

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

New failing tests:
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-simple-ws.html

[Build Bot]

Created attachment 280171 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Build Bot]

Comment on attachment 280164 [details]
Patch (WSS test fix)

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

New failing tests:
http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/upgrade-simple-ws.html

[Build Bot]

Created attachment 280172 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Andy Estes]

Comment on attachment 280164 [details]
Patch (WSS test fix)

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

> Source/WebCore/dom/Document.cpp:5190
> +    Document* enclosingDocument = parentDocument;
> +    if (!enclosingDocument && m_frame->tree().parent())
> +        enclosingDocument = m_frame->tree().parent()->document();
> +    
> +    Document* openerDocument = m_frame->loader().opener() ? m_frame->loader().opener()->document() : nullptr;

Not wild about how we re-discover the parent frame and opener frame here after having already done so immediately above. You could refactor the code above to track the openerFrame separately.

> Source/WebCore/dom/Document.cpp:5195
> +    // Per <http://www.w3.org/TR/upgrade-insecure-requests/>, we need to retain an ongoing set of upgraded
> +    // requests in new navigation contexts. Although this information is present when we construct the
> +    // Document object, it is discard in the subsequent 'clear' statements below. So, we must capture it
> +    // so we can restore it.

This comment is confusing, since there are no statements in this function with the term "clear".

> Source/WebCore/dom/Document.cpp:5200
> +    if (enclosingDocument) {
> +        contentSecurityPolicy()->setUpgradeInsecureRequests(enclosingDocument->contentSecurityPolicy()->upgradeInsecureRequests());
> +        contentSecurityPolicy()->populateInsecureNavigationRequestsToUpgradeFromOther(*enclosingDocument->contentSecurityPolicy());
> +    } else if (openerDocument)
> +        contentSecurityPolicy()->populateInsecureNavigationRequestsToUpgradeFromOther(*openerDocument->contentSecurityPolicy());

Won't setUpgradeInsecureRequests() be called a second time by DocumentWriter::begin() when the load commits? I'm unclear why we have to call it here, too.

> Source/WebCore/dom/Document.cpp:5204
>      if (!ownerFrame) {
>          didFailToInitializeSecurityOrigin();
>          return;

This early return can come before your new code. If ownerFrame is null then enclosingDocument and openerDocument will also be null.

> Source/WebCore/html/HTMLAnchorElement.cpp:558
> +    document().contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(kurl);

I'm not wild about having this call sprinkled at the various call sites of loader entry points. This makes it easy to forget to call this when adding some new thing that starts a navigation. Can this be done once inside FrameLoader?

> Source/WebCore/loader/DocumentWriter.cpp:156
> +    // Per <http://www.w3.org/TR/upgrade-insecure-requests/>, we need to retain an ongoing set of upgraded
> +    // requests in new navigation contexts. Although this information is present when we construct the
> +    // Document object, it is discard in the subsequent 'clear' statements below. So, we must capture it
> +    // so we can restore it.
> +    HashSet<String> insecureNavigationRequestsToUpgrade;
> +    bool upgradeInsecureRequests = false;
> +    if (auto* existingDocument = m_frame->document()) {
> +        upgradeInsecureRequests = existingDocument->contentSecurityPolicy()->upgradeInsecureRequests();
> +        insecureNavigationRequestsToUpgrade = existingDocument->contentSecurityPolicy()->insecureNavigationRequestsToUpgrade();
> +    }

It's not great that we copy the insecureNavigationRequestsToUpgrade HashSet (twice!) every time we commit a new document.

Since we know we're about to overwrite the existing CSP in FrameLoader::clear(), can we have a method to "take" it from the existing Document by WTFMoving it to the caller, then apply the insecure request bits to the cleared document via another move?

> Source/WebCore/loader/FrameLoader.cpp:2755
> +    if (m_frame.document())
> +        m_frame.document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(newRequest);

We ASSERT at the top of this function that m_frame.document() is non-null, so you don't need this null check.

> Source/WebCore/loader/PingLoader.cpp:112
> +    if (frame.document())
> +        frame.document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request);
> +

This function already dereferences frame.document() unconditionally, so you don't need this null check.

> Source/WebCore/loader/SubframeLoader.cpp:238
> +    if (document())
> +        document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(completedURL);
> +

Ditto.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:827
> +void ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(URL& url)
> +{
> +    if (!url.protocolIs("http") && !url.protocolIs("ws"))
> +        return;
> +    
> +    String hostAndPortString = hostAndPortAsString(url.host(), url.port());
> +    if (!m_upgradeInsecureRequests && !m_insecureNavigationRequestsToUpgrade.contains(hostAndPortString))
> +        return;
> +
> +    m_insecureNavigationRequestsToUpgrade.add(hostAndPortString);
> +    
> +    if (url.protocolIs("http"))
> +        url.setProtocol("https");
> +    else if (url.protocolIs("ws"))
> +        url.setProtocol("wss");
> +    else
> +        return;
> +    
> +    if (url.port() == 80)
> +        url.setPort(443);
> +}
> +
> +void ContentSecurityPolicy::upgradeInsecureNavigationRequestIfNeeded(URL& url)
> +{
> +    if (!url.protocolIs("http") && !url.protocolIs("ws"))
> +        return;
>      
> +    String hostAndPortString = hostAndPortAsString(url.host(), url.port());
> +    if (!m_insecureNavigationRequestsToUpgrade.contains(hostAndPortString))
> +        return;
> +    
> +    m_insecureNavigationRequestsToUpgrade.add(hostAndPortString);
> +    
> +    if (url.protocolIs("http"))
> +        url.setProtocol("https");
> +    else if (url.protocolIs("ws"))
> +        url.setProtocol("wss");
> +    else
> +        return;
> +    
> +    if (url.port() == 80)
> +        url.setPort(443);
> +}

So the only difference between these two functions is whether or not we early return if m_upgradeInsecureRequests is false? Can these be combined into a single function that takes a parameter indicating whether or not that extra check is made?

[Brent Fulgham]

Created attachment 280211 [details]
Patch

[Build Bot]

Comment on attachment 280211 [details]
Patch

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

New failing tests:
http/tests/websocket/tests/hybi/upgrade-simple-ws.html

[Build Bot]

Created attachment 280215 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Build Bot]

Comment on attachment 280211 [details]
Patch

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

New failing tests:
http/tests/websocket/tests/hybi/upgrade-simple-ws.html

[Build Bot]

Created attachment 280220 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

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

[Brent Fulgham]

Created attachment 280240 [details]
Patch

[Brent Fulgham]

I'm not sure why the WebSocket upgrade test is so flaky on the EWS machines. I don't see these errors locally. I added some additional logging in the hopes of seeing what's failing.

[Build Bot]

Comment on attachment 280240 [details]
Patch

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

New failing tests:
http/tests/websocket/tests/hybi/upgrade-simple-ws.html

[Build Bot]

Created attachment 280246 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 280240 [details]
Patch

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

New failing tests:
http/tests/websocket/tests/hybi/upgrade-simple-ws.html

[Build Bot]

Created attachment 280247 [details]
Archive of layout-test-results from ews112 for mac-yosemite

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

[Brent Fulgham]

Created attachment 280258 [details]
Patch

[Andy Estes]

Comment on attachment 280258 [details]
Patch

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

This looks great! r=me with one major comment and a number of minor ones.

I'm confused why form submission is not treated as a navigation request. If that's right then maybe a comment would be useful for clarity.

> Source/WebCore/dom/Document.cpp:5199
> +        contentSecurityPolicy()->populateInsecureNavigationRequestsToUpgradeFromOther(*openerDocument->contentSecurityPolicy());

A more clear name for populateInsecureNavigationRequestsToUpgradeFromOther() might be inheritInsecureNavigationRequestsToUpgradeFromOpener().

> Source/WebCore/loader/DocumentWriter.cpp:156
> +    HashSet<String> insecureNavigationRequestsToUpgrade;
> +    bool upgradeInsecureRequests = false;
> +    if (auto* existingDocument = m_frame->document()) {
> +        upgradeInsecureRequests = existingDocument->contentSecurityPolicy()->upgradeInsecureRequests();
> +        insecureNavigationRequestsToUpgrade = existingDocument->contentSecurityPolicy()->takeNavigationRequestsToUpgrade();
> +    }

This is fine as-is, but one improvement would be to take existingDocument's unique_ptr<ContentSecurityPolicy> wholesale instead of keeping track of two pieces of its state.

> Source/WebCore/loader/FormSubmission.cpp:193
> +    document.contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(actionURL, ContentSecurityPolicy::InsecureRequestType::Load);

I'm confused why this treats form submission as a subresource load rather than a navigation.

> Source/WebCore/loader/FrameLoader.cpp:2660
> +void FrameLoader::addHTTPUpgradeInsecureRequestsIfNeeded(ResourceRequest& request)

I think a better function name would be setUpgradeInsecureRequestsHTTPHeaderIfNeeded()

> Source/WebCore/loader/FrameLoader.cpp:2667
> +    if (request.url().protocolIs("https")) {
> +        // FIXME: Identify HSTS cases and avoid adding the header. <https://bugs.webkit.org/show_bug.cgi?id=157885>
> +        return;
> +    }
> +
> +    request.setHTTPHeaderField(HTTPHeaderName::UpgradeInsecureRequests, ASCIILiteral("1"));

This could be written a little more concisely:

    // FIXME: Identify HSTS cases and avoid adding the header. <https://bugs.webkit.org/show_bug.cgi?id=157885>
    if (!request.url().protocolIs("https"))
        request.setHTTPHeaderField(HTTPHeaderName::UpgradeInsecureRequests, ASCIILiteral("1"));

> Source/WebCore/loader/FrameLoader.cpp:2695
> +    if (m_frame.document())
> +        m_frame.document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(workingResourceRequest, ContentSecurityPolicy::InsecureRequestType::Load);

We can avoid calling Frame::document() twice like this:

    if (Document* document = m_frame.document())
        document->...

> Source/WebCore/loader/PingLoader.cpp:86
> +    if (frame.document())
> +        frame.document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request, ContentSecurityPolicy::InsecureRequestType::Load);

Ditto.

> Source/WebCore/loader/PingLoader.cpp:144
> +    if (frame.document())
> +        frame.document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request, ContentSecurityPolicy::InsecureRequestType::Load);

Ditto.

> Source/WebCore/loader/SubframeLoader.cpp:92
> +    ownerElement.document().contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(url, ContentSecurityPolicy::InsecureRequestType::Load);
> +

Can this be moved into loadOrRedirectSubframe()?

> Source/WebCore/loader/SubframeLoader.cpp:237
> +    document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(completedURL, ContentSecurityPolicy::InsecureRequestType::Load);
> +

Can this be moved into loadOrRedirectSubframe(), or does something between here and there depend on the URL having been upgraded?

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:380
> +bool ApplicationCacheHost::shouldLoadResourceFromApplicationCache(const ResourceRequest& originalRequest, ApplicationCacheResource*& resource)
>  {
>      ApplicationCache* cache = applicationCache();
>      if (!cache || !cache->isComplete())
>          return false;
>  
> +    ResourceRequest request(originalRequest);

This is fine as-is, but you could avoid the local variable and the renaming of the request parameter by changing shouldLoadResourceFromApplicationCache() to take request by value.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:183
> +            if (frame->document())
> +                frame->document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request.mutableResourceRequest(), ContentSecurityPolicy::InsecureRequestType::Load);

Isn't this already being done in PingLoader::loadImage()?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:553
> +    if (document())
> +        document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request.mutableResourceRequest(), ContentSecurityPolicy::InsecureRequestType::Load);

Comment about avoiding calling document() twice.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:778
> +void ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(URL& url, InsecureRequestType requestType)

I think this should be called upgradeInsecureURLIfNeeded().

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:807
> +void ContentSecurityPolicy::addInsecureNavigationRequestsToUpgrade(const HashSet<String>& insecureNavigationRequests)
> +{
> +    m_insecureNavigationRequestsToUpgrade.add(insecureNavigationRequests.begin(), insecureNavigationRequests.end());
> +}

Let's fold this function into populateInsecureNavigationRequestsToUpgradeFromOther(), since that's its only caller.

> Source/WebCore/page/csp/ContentSecurityPolicy.h:155
> +    enum InsecureRequestType { Load, Navigation };

This should be an enum class. Also, I think the terms "load" and "navigation" are a bit confusing, since they aren't mutually exclusive. What we want to know is whether or not this is a navigation request, so can we use the names IsNavigationRequest::Yes and IsNavigationRequest::No?

> Source/WebCore/page/csp/ContentSecurityPolicy.h:159
> +    const HashSet<String>& insecureNavigationRequestsToUpgrade() const { return m_insecureNavigationRequestsToUpgrade; }

This function doesn't seem to have any callers. Let's get rid of it.

[Brent Fulgham]

Although Andy approved the patch as-is, I went over things with Dan again in person and he suggested a few improvements:

1. Add a test for the Application Cache modification I made.
2. Important: Don’t add to the upgrade-insecure-request set if we did receive the "Upgrade-Insecure-Request" header from that domain. Currently, it is enough for us to successfully upgrade an <img> load from a domain to add it to our navigation set.
not identified by the CSP flag itself.
3. Add a new test to confirm we don’t add to the insecure navigation set by simply loading an <img> or similar.
4. Use HashSet<RefPtr<SecurityOrigin>> instead of my custom string concatenation thing.

I was concerned that constructing a SecurityOrigin for each pass through this logic would be undesirable, but upon further review this is the way we handle similar identification in other parts of WebKit, so this should not be a performance concern.

[Brent Fulgham]

Created attachment 280482 [details]
Patch

[Andy Estes]

Comment on attachment 280482 [details]
Patch

A number of items I brought up in my review are unaddressed in this patch. In particular, I'm still concerned about form submissions.

[Build Bot]

Comment on attachment 280482 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 280486 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

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

[Brent Fulgham]

Comment on attachment 280258 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:5199
>> +        contentSecurityPolicy()->populateInsecureNavigationRequestsToUpgradeFromOther(*openerDocument->contentSecurityPolicy());
> 
> A more clear name for populateInsecureNavigationRequestsToUpgradeFromOther() might be inheritInsecureNavigationRequestsToUpgradeFromOpener().

Sounds good!

>> Source/WebCore/loader/DocumentWriter.cpp:156
>> +    }
> 
> This is fine as-is, but one improvement would be to take existingDocument's unique_ptr<ContentSecurityPolicy> wholesale instead of keeping track of two pieces of its state.

Great idea! I'll switch to doing so.

>> Source/WebCore/loader/FormSubmission.cpp:193
>> +    document.contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(actionURL, ContentSecurityPolicy::InsecureRequestType::Load);
> 
> I'm confused why this treats form submission as a subresource load rather than a navigation.

The specification language <http://www.w3.org/TR/upgrade-insecure-requests/#upgrade-request> requires that FormSubmissions always be upgraded (even for cross-origin requests), just like loads of embedded content. Since the "InsecureRequestType::Load" did what I wanted, I just reused it.

Instead, I'll add another enum "InsecureRequestType::FormSubmission" so that it is clear that this behavior is specific to form submissions.

>> Source/WebCore/loader/FrameLoader.cpp:2660
>> +void FrameLoader::addHTTPUpgradeInsecureRequestsIfNeeded(ResourceRequest& request)
> 
> I think a better function name would be setUpgradeInsecureRequestsHTTPHeaderIfNeeded()

I based this name on the existing "addHTTPOriginIfNeeded", so for consistency I think its better to keep this name.

>> Source/WebCore/loader/FrameLoader.cpp:2667
>> +    request.setHTTPHeaderField(HTTPHeaderName::UpgradeInsecureRequests, ASCIILiteral("1"));
> 
> This could be written a little more concisely:
> 
>     // FIXME: Identify HSTS cases and avoid adding the header. <https://bugs.webkit.org/show_bug.cgi?id=157885>
>     if (!request.url().protocolIs("https"))
>         request.setHTTPHeaderField(HTTPHeaderName::UpgradeInsecureRequests, ASCIILiteral("1"));

I actually wrote it in this form because I intend to fill in the "FIXME" section with a call to new code to handle the HSTS case. I'm not sure much is gained by changing this at the moment.

>> Source/WebCore/loader/FrameLoader.cpp:2695
>> +        m_frame.document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(workingResourceRequest, ContentSecurityPolicy::InsecureRequestType::Load);
> 
> We can avoid calling Frame::document() twice like this:
> 
>     if (Document* document = m_frame.document())
>         document->...

Oh, right! Changing...

>> Source/WebCore/loader/PingLoader.cpp:86
>> +        frame.document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request, ContentSecurityPolicy::InsecureRequestType::Load);
> 
> Ditto.

Ditto.

>> Source/WebCore/loader/PingLoader.cpp:144
>> +        frame.document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request, ContentSecurityPolicy::InsecureRequestType::Load);
> 
> Ditto.

Ditto!

>> Source/WebCore/loader/SubframeLoader.cpp:92
>> +
> 
> Can this be moved into loadOrRedirectSubframe()?

Yes, at the cost of making a new URL copy. But that seems reasonable.

The only thing I don't like about this change is that 'requestObject' might hit the upgrade path twice (which is not harmful). That's largely irrelevant since we check for a secure origin as the first step of upgrading, so aside from the test of https/wss there won't be a cost for checking it twice.

>> Source/WebCore/loader/SubframeLoader.cpp:237
>> +
> 
> Can this be moved into loadOrRedirectSubframe(), or does something between here and there depend on the URL having been upgraded?

Yes. Done.

>> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:380
>> +    ResourceRequest request(originalRequest);
> 
> This is fine as-is, but you could avoid the local variable and the renaming of the request parameter by changing shouldLoadResourceFromApplicationCache() to take request by value.

That's true. However, there are some number of cases where 'applicationCache' is non-null, and is not complete, where we can avoid a copy. So I thought it would be slightly better to take this path.

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:183
>> +                frame->document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request.mutableResourceRequest(), ContentSecurityPolicy::InsecureRequestType::Load);
> 
> Isn't this already being done in PingLoader::loadImage()?

Yes ... but I believe the "canRequest" call will pass in some cases after upgrading that would not pass without the upgrade.

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:553
>> +        document()->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request.mutableResourceRequest(), ContentSecurityPolicy::InsecureRequestType::Load);
> 
> Comment about avoiding calling document() twice.

Will do!

>> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:778
>> +void ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(URL& url, InsecureRequestType requestType)
> 
> I think this should be called upgradeInsecureURLIfNeeded().

That's what I originally called it, but Dan asked me to make the two functions have the same names.

Should I put you in a room to battle it out? ;-)

>> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:807
>> +}
> 
> Let's fold this function into populateInsecureNavigationRequestsToUpgradeFromOther(), since that's its only caller.

Will do!

>> Source/WebCore/page/csp/ContentSecurityPolicy.h:155
>> +    enum InsecureRequestType { Load, Navigation };
> 
> This should be an enum class. Also, I think the terms "load" and "navigation" are a bit confusing, since they aren't mutually exclusive. What we want to know is whether or not this is a navigation request, so can we use the names IsNavigationRequest::Yes and IsNavigationRequest::No?

Changed to enum class.

I now have three states: Load, Navigation, and FrameSubmission. I think its useful for clarity to have distinct names, but I'm not sure if Load is the right term.

>> Source/WebCore/page/csp/ContentSecurityPolicy.h:159
>> +    const HashSet<String>& insecureNavigationRequestsToUpgrade() const { return m_insecureNavigationRequestsToUpgrade; }
> 
> This function doesn't seem to have any callers. Let's get rid of it.

Done!

[Build Bot]

Comment on attachment 280482 [details]
Patch

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

New failing tests:
media/track/track-text-track-cue-list.html
fast/loader/scroll-position-restored-on-back.html
plugins/crash-restoring-plugin-page-from-page-cache.html
media/restore-from-page-cache.html
imported/blink/fast/frames/navigation-in-pagehide.html
fast/history/page-cache-createObjectURL.html
fast/loader/input-element-page-cache-crash.html
fast/frames/frame-crash-with-page-cache.html
fast/forms/autocomplete-off-with-default-value-does-not-clear.html
imported/w3c/web-platform-tests/XMLHttpRequest/open-url-multi-window-5.htm
http/tests/xmlhttprequest/detaching-frame-2.html
fast/loader/stateobjects/popstate-fires-with-page-cache.html
loader/go-back-cached-main-resource.html
fast/images/animated-gif-restored-from-bfcache.html
plugins/netscape-plugin-page-cache-works.html

[Build Bot]

Created attachment 280489 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 280482 [details]
Patch

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

New failing tests:
media/track/track-text-track-cue-list.html
loader/go-back-cached-main-resource.html
plugins/crash-restoring-plugin-page-from-page-cache.html
media/restore-from-page-cache.html
http/tests/xmlhttprequest/detaching-frame-2.html
fast/history/page-cache-createObjectURL.html
fast/loader/input-element-page-cache-crash.html
fast/frames/frame-crash-with-page-cache.html
fast/forms/autocomplete-off-with-default-value-does-not-clear.html
imported/w3c/web-platform-tests/XMLHttpRequest/open-url-multi-window-5.htm
media/track/regions-webvtt/vtt-region-parser.html
fast/loader/stateobjects/popstate-fires-with-page-cache.html
fast/loader/scroll-position-restored-on-back.html
fast/images/animated-gif-restored-from-bfcache.html
plugins/netscape-plugin-page-cache-works.html
imported/blink/fast/frames/navigation-in-pagehide.html

[Build Bot]

Created attachment 280492 [details]
Archive of layout-test-results from ews113 for mac-yosemite

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

[Build Bot]

Comment on attachment 280482 [details]
Patch

Attachment 280482 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1431105

New failing tests:
imported/blink/fast/frames/navigation-in-pagehide.html
fast/loader/input-element-page-cache-crash.html
fast/forms/autocomplete-off-with-default-value-does-not-clear.html
imported/w3c/web-platform-tests/XMLHttpRequest/open-url-multi-window-5.htm
imported/blink/fast/multicol/span/trailing-margin-before-spanner.html
http/tests/xmlhttprequest/detaching-frame-2.html
fast/loader/scroll-position-restored-on-back.html
fast/images/animated-gif-restored-from-bfcache.html

[Build Bot]

Created attachment 280496 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Brent Fulgham]

The switch to moving CSP created crashes. I'll go back to the original version of that code.

[Brent Fulgham]

Created attachment 280502 [details]
Patch

[Brent Fulgham]

Comment on attachment 280502 [details]
Patch

Patch is updated to test Application Cache, and to incorporate Andy Estes' comments.

[WebKit Commit Bot]

Comment on attachment 280502 [details]
Patch

Clearing flags on attachment: 280502

Committed r201679: <http://trac.webkit.org/changeset/201679>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158464

[Alexey Proskuryakov]

LayoutTest http/tests/websocket/tests/hybi/upgrade-simple-ws.html is flaky. The test is a flaky failure on El Capitan and iOS simulator. It is failing and crashing on Windows.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fupgrade-simple-ws.html

Rolling out.

[Brent Fulgham]

(In reply to comment #100)
> LayoutTest http/tests/websocket/tests/hybi/upgrade-simple-ws.html is flaky.
> The test is a flaky failure on El Capitan and iOS simulator. It is failing
> and crashing on Windows.
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fupgrade
> -simple-ws.html
> 
> Rolling out.

An even and measured response to a new test that I added. Nicely done.

I have put the code back and will investigate the cause of my new test being flaky. Please do not touch this code again.

[Alexey Proskuryakov]

The logic here is that a flaky test is a problem for everyone.

[Brent Fulgham]

(In reply to comment #102)
> The logic here is that a flaky test is a problem for everyone.

I was imprecise in my last comment: I rolled my change back in, but did mark the web socket upgrade test to be skipped so that it would not keep the test bots red.

I also apologize for being grumpy in my last comment. Keeping the test bots green is very important to this project, and I should have kept an eye on that myself.

If anyone wants to stay up-to-date on the flaky test, please hop over to Bug 158480 where I'm investigating.

[Daniel Bates]

For completeness, the patch for this bug was landed again in <https://trac.webkit.org/changeset/201753>.