Introduce WebInsecureRequestPolicy.

Introduce WebInsecureRequestPolicy.

This is the first step towards moving some of 'Upgrade-Insecure-Requests'
enforcement up to the browser process. It introduces WebInsecureRequestPolicy
which aims to represent both 'Upgrade-Insecure-Requests' and
'Block-All-Mixed-Content' as a single bitfield, as they're really two sides
of the same coin.

This patch uses the new type inside of //core/frame/csp. The next patch
will extend usage to 'SecurityContext' (which has a number of dependencies
including 'DocumentInit', 'FrameLoader' and etc, which makes it appealing
to split out). That will be followed by a patch which replicates the new
data to remote frames.

With that infrastructure in place, upgrading requests during redirects should
be (somewhat) straightforward.

[Step 1]: This patch.
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: https://codereview.chromium.org/2046733003

BUG=617947
R=yoav@yoav.ws

Committed: https://crrev.com/78faa7546aa65192af48bc2703791b06a7612eba
Cr-Commit-Position: refs/heads/master@{#398497}

[Mike West, 2016-06-07 09:47:55]

mkwst@chromium.org changed reviewers:
+ yoav@yoav.ws

[Mike West, 2016-06-07 09:47:59]

Yoav, do you have a few minutes to take a look at this patch?

[Yoav Weiss, 2016-06-07 10:30:23]

LGTM % nits

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:188: if
(m_insecureRequestPolicy & kUpgradeInsecureRequests) {
Can we move the bitwise and/or logic to WebInsecureRequestPolicy.h?

Here they feel kinda magical, so I'd much rather see that logic encapsulated in
a single place (ideally as a class. Alternatively, as static functions that
operate on an input WebInsecureRequestPolicy)

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:195:
document->enforceStrictMixedContentChecking();
Is there a particular reason this bit moved from above the "upgrade" check to be
below it?

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:263:
WebInsecureRequestPolicy getInsecureRequestPolicy() const { return
m_insecureRequestPolicy; }
Can you add a comment saying that getInsecureRequestsPolicy is required for
testing only?

[Mike West, 2016-06-07 10:39:49]

Thanks! A few responses inline. WDYT?

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:188: if
(m_insecureRequestPolicy & kUpgradeInsecureRequests) {
On 2016/06/07 at 10:30:23, Yoav Weiss wrote:
> Can we move the bitwise and/or logic to WebInsecureRequestPolicy.h?
> 
> Here they feel kinda magical, so I'd much rather see that logic encapsulated
in a single place (ideally as a class. Alternatively, as static functions that
operate on an input WebInsecureRequestPolicy)

Hrm. What's magical about it? It's a bitfield, which seems like a pretty common
sort of thing. It's not clear to me that something like
`shouldUpgradeInsecureRequests(m_insecureRequestPolicy)` is going to be easier
to understand than checking the relevant bit.

I'd like not to wrap it in a class, as this data will eventually need to be
replicated through `//content/common/frame_replication_state.h` (replacing
`FrameReplicationState::should_enforce_strict_mixed_content_checking`). Simple
types are simple to replicate.

Also, note that the eventual goal is to turn `WebInsecureRequestPolicy` into an
enum. I can't do that at the moment because of exciting bugs that make `upgrade
| block` behave differently than `upgrade`. This patch is first in the line of
patches that will get rid of that behavioral difference by moving much of the
processing up to the browser process.

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:195:
document->enforceStrictMixedContentChecking();
On 2016/06/07 at 10:30:23, Yoav Weiss wrote:
> Is there a particular reason this bit moved from above the "upgrade" check to
be below it?

I just wanted to keep all the request policy bits together. The next patch folds
both of these into a single call, so it simplified the diff. *shrug* Happy to
move it back if you think it's clearer.

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:263:
WebInsecureRequestPolicy getInsecureRequestPolicy() const { return
m_insecureRequestPolicy; }
On 2016/06/07 at 10:30:23, Yoav Weiss wrote:
> Can you add a comment saying that getInsecureRequestsPolicy is required for
testing only?

I think it will eventually be web-exposed once I get around to
designing/implementing a CSP API. But yeah. Sure. :)

[Yoav Weiss, 2016-06-07 10:51:47]

On 2016/06/07 10:39:49, Mike West (OOO until 30th) wrote:
> Thanks! A few responses inline. WDYT?
> 
>
https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
(right):
> 
>
https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:188: if
> (m_insecureRequestPolicy & kUpgradeInsecureRequests) {
> On 2016/06/07 at 10:30:23, Yoav Weiss wrote:
> > Can we move the bitwise and/or logic to WebInsecureRequestPolicy.h?
> > 
> > Here they feel kinda magical, so I'd much rather see that logic encapsulated
> in a single place (ideally as a class. Alternatively, as static functions that
> operate on an input WebInsecureRequestPolicy)
> 
> Hrm. What's magical about it? It's a bitfield, which seems like a pretty
common
> sort of thing. It's not clear to me that something like
> `shouldUpgradeInsecureRequests(m_insecureRequestPolicy)` is going to be easier
> to understand than checking the relevant bit.

The magical part for me is the "Where is this kUpgradeInsecureRequests global
coming from and what is its value?" part. 
Sure, I can grep my way out of it, but (at least to me), a "shouldUpgrade..."
function would have been clearer.

> 
> I'd like not to wrap it in a class, as this data will eventually need to be
> replicated through `//content/common/frame_replication_state.h` (replacing
> `FrameReplicationState::should_enforce_strict_mixed_content_checking`). Simple
> types are simple to replicate.
> 
> Also, note that the eventual goal is to turn `WebInsecureRequestPolicy` into
an
> enum. I can't do that at the moment because of exciting bugs that make
`upgrade
> | block` behave differently than `upgrade`. This patch is first in the line of
> patches that will get rid of that behavioral difference by moving much of the
> processing up to the browser process.

I agree that for an enum, a getter/setter function makes little sense. So if
this is a short term interim step, we can leave it. 

> 
>
https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:195:
> document->enforceStrictMixedContentChecking();
> On 2016/06/07 at 10:30:23, Yoav Weiss wrote:
> > Is there a particular reason this bit moved from above the "upgrade" check
to
> be below it?
> 
> I just wanted to keep all the request policy bits together. The next patch
folds
> both of these into a single call, so it simplified the diff. *shrug* Happy to
> move it back if you think it's clearer.

I don't really mind, just made me wonder if this makes any functional difference
that should be tested. If not, happy to see it stay put.

> 
>
https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):
> 
>
https://codereview.chromium.org/2046523005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:263:
> WebInsecureRequestPolicy getInsecureRequestPolicy() const { return
> m_insecureRequestPolicy; }
> On 2016/06/07 at 10:30:23, Yoav Weiss wrote:
> > Can you add a comment saying that getInsecureRequestsPolicy is required for
> testing only?
> 
> I think it will eventually be web-exposed once I get around to
> designing/implementing a CSP API. But yeah. Sure. :)

[Mike West, 2016-06-07 13:46:39]

Description was changed from

==========
Introduce WebInsecureRequestPolicy.

This is the first step towards moving some of 'Upgrade-Insecure-Requests'
enforcement up to the browser process. It introduces WebInsecureRequestPolicy
which aims to represent both 'Upgrade-Insecure-Requests' and
'Block-All-Mixed-Content' as a single bitfield, as they're really two sides
of the same coin.

This patch uses the new type inside of //core/frame/csp. The next patch
will extend usage to 'SecurityContext' (which has a number of dependencies
including 'DocumentInit', 'FrameLoader' and etc, which makes it appealing
to split out). That will be followed by a patch which replicates the new
data to remote frames.

With that infrastructure in place, upgrading requests during redirects should
be (somewhat) straightforward.

BUG=617584
==========

to

==========
Introduce WebInsecureRequestPolicy.

This is the first step towards moving some of 'Upgrade-Insecure-Requests'
enforcement up to the browser process. It introduces WebInsecureRequestPolicy
which aims to represent both 'Upgrade-Insecure-Requests' and
'Block-All-Mixed-Content' as a single bitfield, as they're really two sides
of the same coin.

This patch uses the new type inside of //core/frame/csp. The next patch
will extend usage to 'SecurityContext' (which has a number of dependencies
including 'DocumentInit', 'FrameLoader' and etc, which makes it appealing
to split out). That will be followed by a patch which replicates the new
data to remote frames.

With that infrastructure in place, upgrading requests during redirects should
be (somewhat) straightforward.

BUG=617947
R=yoav@yoav.ws
==========

[Mike West, 2016-06-07 13:54:05]

Description was changed from

==========
Introduce WebInsecureRequestPolicy.

This is the first step towards moving some of 'Upgrade-Insecure-Requests'
enforcement up to the browser process. It introduces WebInsecureRequestPolicy
which aims to represent both 'Upgrade-Insecure-Requests' and
'Block-All-Mixed-Content' as a single bitfield, as they're really two sides
of the same coin.

This patch uses the new type inside of //core/frame/csp. The next patch
will extend usage to 'SecurityContext' (which has a number of dependencies
including 'DocumentInit', 'FrameLoader' and etc, which makes it appealing
to split out). That will be followed by a patch which replicates the new
data to remote frames.

With that infrastructure in place, upgrading requests during redirects should
be (somewhat) straightforward.

BUG=617947
R=yoav@yoav.ws
==========

to

==========
Introduce WebInsecureRequestPolicy.

This is the first step towards moving some of 'Upgrade-Insecure-Requests'
enforcement up to the browser process. It introduces WebInsecureRequestPolicy
which aims to represent both 'Upgrade-Insecure-Requests' and
'Block-All-Mixed-Content' as a single bitfield, as they're really two sides
of the same coin.

This patch uses the new type inside of //core/frame/csp. The next patch
will extend usage to 'SecurityContext' (which has a number of dependencies
including 'DocumentInit', 'FrameLoader' and etc, which makes it appealing
to split out). That will be followed by a patch which replicates the new
data to remote frames.

With that infrastructure in place, upgrading requests during redirects should
be (somewhat) straightforward.

[Step 1]: This patch.
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: https://codereview.chromium.org/2046733003

BUG=617947
R=yoav@yoav.ws
==========

[Mike West, 2016-06-08 06:59:49]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-06-08 06:59:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046523005/1

[commit-bot: I haz the power, 2016-06-08 07:07:36]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-08 07:09:14]

Description was changed from

==========
Introduce WebInsecureRequestPolicy.

This is the first step towards moving some of 'Upgrade-Insecure-Requests'
enforcement up to the browser process. It introduces WebInsecureRequestPolicy
which aims to represent both 'Upgrade-Insecure-Requests' and
'Block-All-Mixed-Content' as a single bitfield, as they're really two sides
of the same coin.

This patch uses the new type inside of //core/frame/csp. The next patch
will extend usage to 'SecurityContext' (which has a number of dependencies
including 'DocumentInit', 'FrameLoader' and etc, which makes it appealing
to split out). That will be followed by a patch which replicates the new
data to remote frames.

With that infrastructure in place, upgrading requests during redirects should
be (somewhat) straightforward.

[Step 1]: This patch.
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: https://codereview.chromium.org/2046733003

BUG=617947
R=yoav@yoav.ws
==========

to

==========
Introduce WebInsecureRequestPolicy.

This is the first step towards moving some of 'Upgrade-Insecure-Requests'
enforcement up to the browser process. It introduces WebInsecureRequestPolicy
which aims to represent both 'Upgrade-Insecure-Requests' and
'Block-All-Mixed-Content' as a single bitfield, as they're really two sides
of the same coin.

This patch uses the new type inside of //core/frame/csp. The next patch
will extend usage to 'SecurityContext' (which has a number of dependencies
including 'DocumentInit', 'FrameLoader' and etc, which makes it appealing
to split out). That will be followed by a patch which replicates the new
data to remote frames.

With that infrastructure in place, upgrading requests during redirects should
be (somewhat) straightforward.

[Step 1]: This patch.
[Step 2]: https://codereview.chromium.org/2040133003
[Step 3]: https://codereview.chromium.org/2046733003

BUG=617947
R=yoav@yoav.ws

Committed: https://crrev.com/78faa7546aa65192af48bc2703791b06a7612eba
Cr-Commit-Position: refs/heads/master@{#398497}
==========

[commit-bot: I haz the power, 2016-06-08 07:09:16]

Patchset 1 (id:??) landed as
https://crrev.com/78faa7546aa65192af48bc2703791b06a7612eba
Cr-Commit-Position: refs/heads/master@{#398497}