Upgrade Insecure Requests: bugfixes, tests, and support for OOPIF.

Upgrade Insecure Requests: bugfixes, tests, and support for OOPIF.

This CL fixes several bugs with Upgrade Insecure Requests. The main one
is that URL were compared against the 'upgrade insecure navigation set'
of the frame that is navigating instead of the frame that initiated the
navigation.

8 new tests are added to ensure that regressions won't happen.

Finally, it adds the support for OOPIF. The 'upgrade insecure
navigations set' is now replicated across the different processes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=670219

[arthursonzogni, 2016-12-07 13:32:01]

Patchset #1 (id:1) has been deleted

[arthursonzogni, 2016-12-07 13:32:10]

Patchset #1 (id:20001) has been deleted

[arthursonzogni, 2016-12-07 13:51:25]

arthursonzogni@chromium.org changed reviewers:
+ mkwst@chromium.org, nasko@chromium.org

[arthursonzogni, 2016-12-07 13:51:26]

Hi @mkwst and @nasko,

@mkwst: Contrary to what I told you, it isn't a PlzNavigate-only solution, it
works with/without PlzNavigate and with/without OOPIF. Please, I would like you
to take a look at how the URLs are upgraded and the tests. You already have seen
4 of them in a previous patch I didn't commit.

@nasko: Please, could you take a look? Especially how the 'upgrade insecure
navigations set' is replicated accross the remote frames. I am not sure you will
like how the HashSet is passed to the IPC. If so, please tell me what would be a
better solution.

Thanks!

https://codereview.chromium.org/2557063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2557063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1675:
modifyRequestForCSP(resourceRequest, frameLoadRequest.originDocument());
This line makes Upgrade Insecure Requests works with and without
browser-side-navigation. We only need this to make the 8 tests pass.

Then, the remaining lines in the patch make it works with OOPIF. With
cross-process iframe, the originDocument is not accessible (i.e nullptr). To
make it works, the 'upgrade insecure navigations set' is replicated accross
process and the url is upgraded just before we left the initiating frame (see
Frame::upgradeInsecureRequest).

[nasko, 2016-12-08 22:45:44]

The content bits look good overall with a couple of minor comments. However, I
don't know enough about how we represent the upgrade set and whether it needs to
be a set or just a boolean. I defer to Mike for those parts.

https://codereview.chromium.org/2557063002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2557063002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:171: const std::vector<unsigned>&
insecure_navigations_set);
Please be explicit. Unsigned int?

https://codereview.chromium.org/2557063002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2557063002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:746: // Update a proxy's replicated enforcement
of insecure navigations set.
nit: replicated set for enforcement of insecure navigations.

[Charlie Reis, 2016-12-09 00:59:50]

Description was changed from

==========
Upgrade Insecure Requests: bugfixes, tests, and support for OOPIF.

This CL fixes several bugs with Upgrade Insecure Requests. The main one
is that URL were compared against the 'upgrade insecure navigation set'
of the frame that is navigating instead of the frame that initiated the
navigation.

8 new tests are added to ensure that regressions won't happen.

Finally, it adds the support for OOPIF. The 'upgrade insecure
navigations set' is now replicated across the different processes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=670219
==========

to

==========
Upgrade Insecure Requests: bugfixes, tests, and support for OOPIF.

This CL fixes several bugs with Upgrade Insecure Requests. The main one
is that URL were compared against the 'upgrade insecure navigation set'
of the frame that is navigating instead of the frame that initiated the
navigation.

8 new tests are added to ensure that regressions won't happen.

Finally, it adds the support for OOPIF. The 'upgrade insecure
navigations set' is now replicated across the different processes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=670219
==========

[arthursonzogni, 2016-12-09 10:59:52]

Patchset #2 (id:60001) has been deleted

[arthursonzogni, 2016-12-09 11:06:50]

Thanks Nasko!

https://codereview.chromium.org/2557063002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2557063002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:171: const std::vector<unsigned>&
insecure_navigations_set);
On 2016/12/08 22:45:44, nasko (very slow til Dec 13th) wrote:
> Please be explicit. Unsigned int?

I agree, but at the same time I tried to stay coherent with what is done in the
current source code:
* using InsecureNavigationsSet = HashSet<unsigned, WTF::AlreadyHashed>;
* unsigned StringImpl::hash() const
* void addInsecureNavigationUpgrade(unsigned hashedHost)

I think "unsigned" is more used in blink:
grep "<unsigned int>"    -r third_party/WebKit/ | wc -l => 0
grep "<unsigned>"        -r third_party/WebKit/ | wc -l => 454
grep "unsigned int i ="  -r third_party/WebKit/ | wc -l => 2
grep "unsigned i ="      -r third_party/WebKit/ | wc -l => 789

There is no consensus in content/
grep "<unsigned int>"    -r content/ | wc -l => 24
grep "<unsigned>"        -r content/ | wc -l => 18
grep "unsigned int i ="  -r content/ | wc -l => 6
grep "unsigned i ="      -r content/ | wc -l => 32


I think the best thing to do is to apply your suggestion to my patch for
everything that is not in blink. Are you okay with this?

https://codereview.chromium.org/2557063002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2557063002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:746: // Update a proxy's replicated enforcement
of insecure navigations set.
On 2016/12/08 22:45:44, nasko (very slow til Dec 13th) wrote:
> nit: replicated set for enforcement of insecure navigations.

Done.

[nasko, 2016-12-09 15:39:54]

On 2016/12/09 11:06:50, arthursonzogni wrote:
> Thanks Nasko!
> 
>
https://codereview.chromium.org/2557063002/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/frame_tree_node.h (right):
> 
>
https://codereview.chromium.org/2557063002/diff/40001/content/browser/frame_h...
> content/browser/frame_host/frame_tree_node.h:171: const std::vector<unsigned>&
> insecure_navigations_set);
> On 2016/12/08 22:45:44, nasko (very slow til Dec 13th) wrote:
> > Please be explicit. Unsigned int?
> 
> I agree, but at the same time I tried to stay coherent with what is done in
the
> current source code:
> * using InsecureNavigationsSet = HashSet<unsigned, WTF::AlreadyHashed>;
> * unsigned StringImpl::hash() const
> * void addInsecureNavigationUpgrade(unsigned hashedHost)
> 
> I think "unsigned" is more used in blink:
> grep "<unsigned int>"    -r third_party/WebKit/ | wc -l => 0
> grep "<unsigned>"        -r third_party/WebKit/ | wc -l => 454
> grep "unsigned int i ="  -r third_party/WebKit/ | wc -l => 2
> grep "unsigned i ="      -r third_party/WebKit/ | wc -l => 789

This is correct. Blink's style is the one omitting the type.

> There is no consensus in content/
> grep "<unsigned int>"    -r content/ | wc -l => 24
> grep "<unsigned>"        -r content/ | wc -l => 18
> grep "unsigned int i ="  -r content/ | wc -l => 6
> grep "unsigned i ="      -r content/ | wc -l => 32

There is more. In content/ and chromium in general, we use uint* types:

content/ $ git grep uint[0-9] | wc -l
    2876

> I think the best thing to do is to apply your suggestion to my patch for
> everything that is not in blink. Are you okay with this?

Yes. That is perfectly fine and the right thing to do. Let's use uint32_t in
chromium code and unsigned in blink.
Also, keep in mind that we will be converting Blink to Chromium style, so in the
near future we won't have these discrepancies.

>
https://codereview.chromium.org/2557063002/diff/40001/content/common/frame_me...
> File content/common/frame_messages.h (right):
> 
>
https://codereview.chromium.org/2557063002/diff/40001/content/common/frame_me...
> content/common/frame_messages.h:746: // Update a proxy's replicated
enforcement
> of insecure navigations set.
> On 2016/12/08 22:45:44, nasko (very slow til Dec 13th) wrote:
> > nit: replicated set for enforcement of insecure navigations.
> 
> Done.

[nasko, 2016-12-12 23:11:30]

content/ LGTM with a couple of nits.

https://codereview.chromium.org/2557063002/diff/100001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2557063002/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:105: std::vector<uint32_t>() /*
insecure url to upgrade */,
nit: since this is a vector, shouldn't url be plural? "urls"

https://codereview.chromium.org/2557063002/diff/100001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2557063002/diff/100001/content/common/frame_m...
content/common/frame_messages.h:752: std::vector<uint32_t>)
nit: The parameter name should be added as a comment - /* param_name */

[arthursonzogni, 2016-12-13 10:30:23]

Patchset #2 (id:80001) has been deleted

[arthursonzogni, 2016-12-13 10:43:37]

Thanks Nasko!

Now, it remains @mkwst's review for the remaining files in /third_party/Webkit.
It's essentially checking the tests and that the two functions
* FrameLoader::upgradeInsecureRequest(originDocument)
* Frame::UpgradeInsecureRequest(url)
are corrects.

https://codereview.chromium.org/2557063002/diff/100001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2557063002/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:105: std::vector<uint32_t>() /*
insecure url to upgrade */,
On 2016/12/12 23:11:30, nasko (very slow til Dec 13th) wrote:
> nit: since this is a vector, shouldn't url be plural? "urls"

Done.

https://codereview.chromium.org/2557063002/diff/100001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2557063002/diff/100001/content/common/frame_m...
content/common/frame_messages.h:752: std::vector<uint32_t>)
On 2016/12/12 23:11:30, nasko (very slow til Dec 13th) wrote:
> nit: The parameter name should be added as a comment - /* param_name */

Done.