cc'ing some navigation folks who might have ideas about what to do here

@estark: how problematic is this? In particular, should it be fixed before PlzNavigate launches? Nasko mentioned this was broken for a long time in regular navigation and recently fixed, hence why we're wondering.

This bug has existed forever, so it wouldn't be the end of the world if PlzNavigate launches without fixing it. But it could be kind of weird/frustrating for developers if it's fixed and then breaks again a couple releases later.

I can look into it some more next week. I suspect it might not be that hard to fix if I try to understand better how upgrade-insecure-requests works with PlzNavigate. If we can do the upgrade in the browser process instead of in Blink (maybe we already do in some cases?), then it should be relatively straightforward to fix this in the same way we fixed it in Blink.

Looked into this some more today. My thought in comment #5 about doing the upgrade in the browser process doesn't seem immediately doable, since NavigationThrottles can't modify the request AFAICT.

The only thing I can think of is to evaluate report-only policies in Blink, and enforced policies in the browser. This strikes me as pretty gross, but not necessarily more gross than what we do right now (which is upgrade in Blink, using a policy that is enforced in the browser).

Why does it need to be in a throttle? content/ is implementing the web platform, so any web platform feature can be implemented inside it, in the actual core code if needed. I don't see why upgrading insecure requests cannot be done inside RenderFrameHost/Navigator/NavigationRequest (ideally the last one).

Also, at this time, the policies we apply to navigations are parsed in the renderer process, so it isn't really any more gross. If we want to do better, we will have to parse the CSP header in the browser or network process in the future. I'm all for it, but we can make progress before waiting for that to happen.

Can arthursonzogni@'s work on enforcing some CSP properties be reused and furthered into supporting upgrade-insecure-requests? It can be done prior to invoking throttles, so it will keep to spec of executing before mixed content checks.

> Why does it need to be in a throttle?

It doesn't, I just meant "it can't be done in a throttle and I don't know where it should be done instead" :) If NavigationRequest can change the URL of the request, that seems ideal.

> Also, at this time, the policies we apply to navigations are parsed in the renderer process, so it isn't really any more gross

Here's an example of why it would be weird to do report-only + upgrade in Blink with enforced policies in the browser: report-only and enforced policies would get applied in opposite orders depending on whether the navigation is renderer- or browser-initiated. (Or at least, that's what it looked like when I was tracing through the code paths today.) For browser-initiated navigations, AncestorThrottle sees the request and would apply enforced policies before Blink, which would apply report-only policies; for renderer-initiated navigations, it looks like it's the other way around. That's not the end of the world and I don't think it would be visible to developers unless they were looking really hard for it, but it's weird and violates the Fetch spec. If we can have CSP applied in one place (even if it's parsed in another place), I think that would be cleaner.

Upgrade-Insecure-Request are checked on the renderer side at the same place with and without PlzNavigate.
A agree with #6. The simplest thing to do is to evaluate report-only policies in blink for the moment.
One thing to know is that report-only policies in blink shouldn't be checked when url_request.CheckForBrowserSideNavigation() is false. More info: http://crbug.com/692595

Moving Upgrade-Insecure-Request (for navigation) into the browser looks doable. I don't see any problems but it requires some works. A boolean can be added into the content::CSPDirective class. The concepts of inheritance of this property and the "upgrade insecure navigations set" would be unnecessary because we just have to search for what are the ancestors of the current frame in the FrameTreeNode.

FYI: I think that Upgrade-Insecure-Request still doesn't work when there is a redirect, regardless of whether PlzNavigate is enabled or not. For instance the request is upgraded a first time, gets redirected to an insecure scheme, then it should be upgraded again, but that is not the case.
There were some attempts to enforce it on the browser-side in order to handle redirects:
https://crrev.com/2053693002/

Re comment 9: yeah, you're correct that UIR still doesn't work across redirects :(

Nasko and I chatted about this some more the other day. I think a rough plan that seemed promising was doing UIR in the browser process by moving the CSP checks out of AncestorThrottle and into NavigationRequest (so that the CSP checks can modify the request URL if it needs to be upgraded). Then we'd split up the CSP checks into report-only and enforced, matching Blink, so that we check report-only CSP policies, then we check UIR and the request is upgraded if necessary, then we check enforced policies.

I think the alternative plan of doing report-only checks in Blink before upgrading the request could also be made to work, but it would take some care to make sure that reports aren't sent twice for some requests (once in Blink and once in the browser process). The browser process would need to check report-only policies only for requests that Blink doesn't check (like following redirects on navigation requests, I think).

arthursonzogni, since you're familiar with the browser-side CSP checks, would you be able to take this bug by any chance?

#10. I will work on what you and Nasko are suggesting.

I discussed with Nasko. We are close to M-60 and I have to work on higher priority bugs with PlzNavigate for the moment. @estark, by any chance, if you think that it is possible, feel free to start working on this. It would help me a lot.

Sure, I will try to take a look next week.

I'm removing the PlzNavigate-Blocking label. I realized that the non-PlzNavigate fix only landed in M60, so this bug shouldn't blcok PlzNavigate from shipping in M60. That is, I was concerned about fixing the bug in one release and then having it essentially regress in a subsequent release with PlzNavigate, but that won't be the case if PlzNavigate ships in M60.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/102feab6fd73e6b0510c572165d10e2b3831790c

commit 102feab6fd73e6b0510c572165d10e2b3831790c
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Mon May 22 14:20:27 2017

PlzNavigate: Add webkit layout test TestExpectations.

Several regressions happened with PlzNavigate.
This CL updates the
Webkit/LayoutTests/FlagExpectations/enable-browser-side-navigation file.

BUG= 723595 ,  723602 , 713388,  724989 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2893563002
Cr-Commit-Position: refs/heads/master@{#473574}

[modify] https://crrev.com/102feab6fd73e6b0510c572165d10e2b3831790c/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1cfb38a9685f1e8561435982acbe797912cdb9e1

commit 1cfb38a9685f1e8561435982acbe797912cdb9e1
Author: estark <estark@chromium.org>
Date: Tue Jun 06 20:26:46 2017

Move PlzNavigate frame-src CSP check to NavigationRequest

This is in the process of moving upgrade-insecure-requests to the browser
process for frame-src checks for PlzNavigate. By doing the frame-src CSP check
in NavigationRequest instead of a NavigationThrottle, we will be able to modify
the request URL if required by upgrade-insecure-requests. This CL just moves the
existing CSP check from AncestorThrottle to NavigationRequest but does not yet
implement upgrade-insecure-requests or reporting for upgrade-insecure-requests
(that will be in a follow-up CL).

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2909513002
Cr-Commit-Position: refs/heads/master@{#477388}

[modify] https://crrev.com/1cfb38a9685f1e8561435982acbe797912cdb9e1/content/browser/frame_host/ancestor_throttle.cc
[modify] https://crrev.com/1cfb38a9685f1e8561435982acbe797912cdb9e1/content/browser/frame_host/ancestor_throttle.h
[modify] https://crrev.com/1cfb38a9685f1e8561435982acbe797912cdb9e1/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/1cfb38a9685f1e8561435982acbe797912cdb9e1/content/browser/frame_host/navigation_request.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee

commit 4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee
Author: estark <estark@chromium.org>
Date: Sat Jun 10 00:46:07 2017

Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process for frame requests, and
properly splits up CSP checks per spec: (1) evaluate report-only CSPs,
(2) upgrade request if needed, (3) evaluate enforced CSPs.

There are other cases for which we might need to do something similar which
are not handled by this CL: namely form submissions and same-host main-frame
navigations.

Also note that I'm not attempting to apply upgrade-insecure-requests when
following redirects. UIR in general does not work when following redirects, and
that's a much larger issue outside the scope of this CL.
( https://crbug.com/615885 )

This is a follow-up to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2910573002
Cr-Commit-Position: refs/heads/master@{#478478}

[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/browser/frame_host/form_submission_throttle.cc
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/browser/frame_host/mixed_content_navigation_throttle.h
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/common/content_security_policy/content_security_policy.cc
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/common/content_security_policy/content_security_policy.h
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/common/content_security_policy/content_security_policy_unittest.cc
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/common/content_security_policy/csp_context.cc
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/common/content_security_policy/csp_context.h
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/common/content_security_policy/csp_context_unittest.cc
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/common/content_security_policy/csp_directive.cc
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/content/common/content_security_policy/csp_directive.h
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h
[modify] https://crrev.com/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee/third_party/WebKit/Source/core/loader/FrameLoader.cpp

For posterity, what's left to do on this bug is basically #17 for form submissions: in other words, we need to send CSP reports for the upgrades of form submissions in PlzNavigate. For frame navigations, we did this by moving the CSP check into NavigationRequest and doing things in the right order there (check report-only CSP, apply UIR, check enforced CSP). We basically need to do the analogous thing for forms, by moving the CSP logic from FormSubmissionThrottle into NavigationRequest and interleaving it with UIR properly. See also the TODO in https://cs.chromium.org/chromium/src/content/browser/frame_host/form_submission_throttle.cc?q=form_submission_throttle.cc&sq=package:chromium&l=63

I'm looking into the remainder as described in #18. It looks like it will remove the need for the FormSubmissionThrottle, but it seems like it would be good to keep the browser tests (from form_submission_throttle_browsertest.cc), since I'm not sure these paths get exercised otherwise.

For keeping the tests around, I think they'll need to be restructured because it looks like they're pretty tied to the throttle architecture. We might want to create a new navigation_request_browsertest.cc file and rewrite these tests more as integration tests that:
- navigate to the main page URL
- trigger a form submission navigation by injecting JS to submit a form on the page (see content::ExecuteScript)
- check that the navigation succeeded or failed as expected (which I think can be observed with a WebContentsObserver that implements the DidFinishNavigation method)

Yeah, that's roughly how I was thinking of going about rewriting the tests. Thanks for the pointer about ExecuteScript -- that seems much simpler than the current tests. I'll take a stab writing the new tests.

Do we also want to check for whether it reports (when the policy specifies)? I don't think there's an observer for the report IPC (from RenderFrameHostImpl::ReportContentSecurityPolicyViolation), and I'm not sure how to implement a new one.

Yeah, it would be good to have a test that the reporting works. Here's an idea that might work: have two EmbeddedTestServers, one which serves the main pages and one which is the reporting server. Use EmbeddedTestServer::RequestRequestHandler on the reporting server to register a request handler that quits a base::RunLoop when a report is received (see [1] for an example how to do that). Then use RegisterRequestHandler on the main page server to serve the pages for the tests, setting the CSP report-uri dynamically to the report server URL. Does that make sense?

[1] https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_expect_ct_reporter_browser_tests.cc?dr&l=47

I have a WIP CL for the form-action change at https://crrev.com/c/736180, including the new NavigationRequestBrowserTest suite. However, it's failing several form resubmission CSP layout tests and I'm a little stumped as to why (they mostly time out rather than producing unexpected results).

The current set that are failing are:
* http/tests/security/contentSecurityPolicy/1.1/form-action-resubmission-iframe-reload-from-child.html
* http/tests/security/contentSecurityPolicy/1.1/form-action-resubmission-iframe-reload-from-parent.html
* http/tests/security/contentSecurityPolicy/1.1/form-action-resubmission-main-page.html
* http/tests/security/contentSecurityPolicy/1.1/form-action-resubmission-window-reload-from-child.html
* http/tests/security/contentSecurityPolicy/1.1/form-action-resubmission-window-reload-from-parent.html
* http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked-with-redirect.html
* http/tests/security/contentSecurityPolicy/1.1/form-action-src-redirect-blocked.html

Andy may have thoughts, as he's in the process of killing those tests in favor of those in //third_party/WebKit/LayoutTests/external/wpt/content-security-policy/.

The reasons the tests time out is because they are waiting for a message that never comes. This message should come from a child iframe/child window/other window that triggers form submissions while having a CSP policy of `form-action: none`. This should cause `securitypolicyviolation` events which then are caught and sent back as messages to the parent.

I would start by investigating why the message is not sent anymore, perhaps the event is not fired properly.

Chris: I put some time on the calendar for later today to pair-debug, but in the meantime, in case you don't know, you can run the layout test server on its own and then open the test file in a local Chrome build to see what's going on -- I usually find it easiest to debug that way:
- `third_party/WebKit/Tools/Scripts/run-blink-httpd` <- runs the layout test server on 127.0.0.1:8000
- `out/Release/chrome http://127.0.0.1:8000/security/contentSecurityPolicy/1.1/form-action-resubmission-main-page.html` (or where ever your chrome build is)
Then you can do console logs, LOG(...)s to printf-debug, attach a debugger, etc. to see how far the test is getting and why it's timing out.

It appears that Blink is upgrading Form Actions before the navigation gets to NavigationRequest. This happens in Blink in the FormSubmission constructor. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FormSubmission.cpp?rcl=8f6e077159fe4d82fd71ee1db7922da9830aeee5&l=200

  if (document.GetInsecureRequestPolicy() & kUpgradeInsecureRequests &&
      action_url.ProtocolIs("http")) {
    UseCounter::Count(document,
                      WebFeature::kUpgradeInsecureRequestsUpgradedRequest);
    action_url.SetProtocol("https");
    if (action_url.Port() == 80)
      action_url.SetPort(443);
  }

This means that for form-action directives, we can't (browser side) properly handle reporting then upgrading (as in the "Finding insecure requests" example on MDN: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/upgrade-insecure-requests). This is currently causing one of my tests to hang indefinitely as it waits for a report (but the request has already been upgraded, so there is nothing to report).

In contrast, this isn't happening for subframe requests -- they are properly started in an un-upgraded state and the browser-side logic can handle the report->upgrade->enforce steps correctly.

I'm going to proceed with the CL I have in progress, but disable the tests around the interaction of upgrade-insecure-requests and form-action. If we want to remove the upgrade in Blink, then I can land another CL to re-enable the test.

Ahh, nice find. I think we should probably disable that Blink upgrade when PlzNavigate is enabled, which is what we do for frame navigation requests in Blink: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?type=cs&l=1652

Yet another complication or two from some debug-build spelunking: It looks like there are a few places still where Blink CSP handling for forms still kicks in.

For example there's a relatively short path of CSP blocking for HTMLFormElement.cpp::ScheduleForSubmission https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/forms/HTMLFormElement.cpp?l=446

  if (!GetDocument().GetContentSecurityPolicy()->AllowFormAction(
          submission->Action())) {
    return;
  }

This just directly acts on the document's CSP and blocks (after potentially sending a report).

But I'm not sure if anywhere in that execution path do we have access to the frame (and thus the settings flag for browser-side navigation), so I'm a little blocked on how to properly 

The CSP handling sprinkled throughout WebKit seems particularly complicated, but hopefully this is (one of) the last ones that need special-casing.

(Whoops -- hit submit a little too early and missed part of the second to last paragraph...)

I'm a little blocked on how to properly check for the browser-size navigation flag in either of HTMLFormElement and FormSubmission (for the blocking and upgrading logic, respectively).

To check the --enable-browser-side-navigation flag:
* Outside of blink: content::IsBrowserSideNavigationEnabled().
* Inside of blink: blink::Settings::GetBrowserSideNavigationEnabled().

So I think in HTMLFormElement::ScheduleFormSubmission(), you can do something close to:
GetDocument().GetFrame()->GetSettings()->GetBrowserSideNavigationEnabled().

A small update about the current state of the CL:

Disabling render-side CSP broke a number of cases where CSP is not properly handled browser side: (1) Javascript URL actions, and (2) forms that target other frames (blank or named windows).

For (1), I've added a check to keep doing the Blink CSP checking when the action is Javascript.

For (2), we may need to fix https://crbug.com/700964 to avoid leaving a gap in CSP enforcement.

Otherwise, we may be able to land the re-factoring in the browser side, while not disabling the blink-side CSP enforcement for now (until 700964 is fixed).