Please talk with the network folks before making too many changes to HSTS. It occurs to me that they might not be thrilled with moving it out of //net and into //content given the way that cronet is used by other embedders. :(

+rsleevi@

On its face, "implemented as a navigation throttle" sounds like the wrong approach, but I may just be misunderstanding. Do you have a design doc?

And as Mike mentioned, moving HSTS out of //net into //content is probably the wrong layer. But I'm trying not to prejudge on a bug; happy to set up a VC and talk through, even happier to review a doc that summarizes the issues so we can discuss.

"As a navigation throttle" is more of a suggestion as it seemed like the more straightforward way to deal with the issue I described above.

This comes from the need to move some security checks to the browser because of PlzNavigate, what included part of mixed content checks, what affects how HSTS checks happen.

For now we don't have anything beyond the awareness that we need to fix HSTS in the context of PlzNavigate and browser side mixed content checks. If there are other potentially better approaches we'd be very happy to know about them.

@Comment 3: The issue is I can't find any good summary of what the issues are and how PlzNavigate "breaks" HSTS. I was hoping you could point me to some sort of existing summary of the issue, and perhaps how you proposed to address it, and we could iterate from there. Absent that, setting up a VC to have a chat and then update the bug with that sort of summary was another option :)

I can only re-tell the problem as I understood it from mkwst@ when we were discussing the effects of the CL linked in the OP. Maybe he can chime in with more details?

With the partial moving of mixed content checks to the browser -- only for frame-level resource loads -- there will be situations where no mixed content check happens in Blink. What mkwst@ explained is that the HSTS implementation in Blink relies on the fact that mixed content checks happen *before* HSTS ones and this change might cause that not to happen if the mixed content checks are performed by the browser process (when they are simply not performed inside Blink).

As the browser side mixed content checker was implemented as a navigation throttle, it seemed like a potential approach for solving this problem. We'd have both throttles running in the browser, HSTS added right after the mixed content one, so that the checking ordering would be reestablished.

For now there's not test failure I refer to for getting a clear breaking example but IIUC they should come with the deployment of web-platform-test.

Finally, I agree we should probably schedule a VC but it must to involve someone else from the PlzNavigate team as I will not be the one working on solving this issue. I'll see about who will that be and we can go from there.

HSTS happens deep in the net stack and is surfaced as a redirect, so I suspect that we'll just naturally get the right behavior. It's been a long time since I've been involved in PlzNavigate, but I'd expect a browser-side mixed content check and net-side HSTS check to go something like:

- Navigation code gets asked to navigate to A (a pre-HSTS URL)
- Navigation code checks mixed content on A. Navigation code tells //net to fetch A (or rejects).
- //net converts A to A' due to HSTS and simulates a redirect. It informs navigation code.
- Navigation code sees a redirect and checks mixed content on A' (presumably fine). Navigation code tells //net to continue.
- //net fetches A'. Maybe A' redirects to B (a pre-HSTS URL). It informs navigation code.
- Navigation code sees a redirect and checks mixed content on B. Navigation code tells //net to continue (or rejects).
- //net fetches B.

etc.

That's the behavior we want, right?

Oh, I suppose for full generality, replace the last step over there with:

- //net converts B to B' due to HSTS and simulates a redirect. It informs navigation code.
- Navigation code sees a redirect and checks mixed content on B' (presumably fine). Navigation code tells //net to continue.
- //net fetches B'.

And, of course, each of those "converts X to X'" steps might not happen if X does not need to be rewritten, in which case we won't surface a funny redirect and just fetch the URL directly.

Described like this it really seems like what would happen today with that patch. If the browser side HSTS checks happen at the net level then maybe there's nothing to worry about?

It seems the best would be to run the HSTS tests from web-platform-test with PlzNavigate enabled and see if we actually have an issue.

mkwst@: can you confirm if there will be HSTS tests in web-platform-test? That was my assumption from previous discussions.

davidben@: Given that HSTS redirects happen in URLRequestHttpJob::Factory, I'm pretty sure that the original request is not going to go through interceptors or throttles, since those only happen after we have a URLRequestHttpJob that sends a request. The throttles would only see a redirect request. I might be misunderstanding the ordering in which things happen, though. I'd be happy to be wrong.

> can you confirm if there will be HSTS tests in web-platform-test? That was my assumption from previous discussions.

I doubt we have tests for this today. I'll add one. It will look very much like the following:

```
  <iframe src="http://youtube.com/"></iframe>
```

Throttles and friends should the original URL just fine. WillStartRequest happens before //net does anything at all.

From //net's perspective, PlzNavigate doesn't really change anything. Someone makes a URLRequest, we notify them when stuff happens, and maybe they abort the request at some point. It's everything beyond //net that changes in PlzNavigate, but it's still performing the same role as far as //net is concerned. Just rather than popping to the renderer process for a decision, we stay in the browser process.

If HSTS has to happen after Mixed Content, then I do think that things as they are now will "just work". davidben@ is correct that we will check the navigation in WillSendRequest first, before even going to the network stack, so we will block it there.

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

commit a6345fb8100733f36a40b8578c38ec3980919780
Author: mkwst <mkwst@chromium.org>
Date: Wed Feb 22 08:01:19 2017

Ensure that mixed content checks preceed HSTS checks.

BUG= 692157 
R=carlosk@chromium.org

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

[add] https://crrev.com/a6345fb8100733f36a40b8578c38ec3980919780/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-iframe-with-hsts.https-expected.txt
[add] https://crrev.com/a6345fb8100733f36a40b8578c38ec3980919780/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-iframe-with-hsts.https.html
[add] https://crrev.com/a6345fb8100733f36a40b8578c38ec3980919780/third_party/WebKit/LayoutTests/http/tests/security/resources/hsts.php

Yay for adding tests rather than speculating on complex behaviors! :-)

While reviewing the test added in #12 I ran it with PlzNavigate enabled. The test fails but not because of mixed content or HSTS: the only problem is that the "line 18: " log snippet didn't show up. So it seems that behavior-wise we are good.

Investigating the renderer code, the more likely spot where this line number is obtained is in the call to Document::addConsoleMessage [1] where it does some "location finding magic" using information from ScriptableDocumentParser.

I don't think this will ever be possible from the browser side log call and I'm unsure on how to fix this. Suggestions?


[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?type=cs&q=%22Document::addConsoleMessage%22&l=5757

@carlosk: That's  issue 695072 . I'm working on it.

Great! Adding it as a dependency.

With issue  issue 695072  being fixed can we consider this one fixed too? Or are there other aspects of HSTS versus PlzNavigate that still need to be addressed before it launches?

I wasn't sure that the tests were landed, but looking at c#12 it seems they are. Closing this.

Apologies, applied the wrong component in bulk.