Stop CSP from matching independent scheme/port upgrades (content layer)

Stop CSP from matching independent scheme/port upgrades (content layer)

Made the changes necessary in content to mimic the behaviour of
https://codereview.chromium.org/2708873002
Copied from there:
"Content-Security-Policy allows an url to match a source-expression even
if the scheme or the port doesn't matches, but in this case it must be an
upgrade to a more secure scheme(http->https) and more secure port(80->443).
The problem is that it happens independently, so it is allowed to have an
upgrade of the port without the scheme (http over 443) or an upgrade of the
scheme without the port (https over 80).

This is a change to force the upgrade to be both over port and scheme."

BUG=692499, 692442
Review-Url: https://codereview.chromium.org/2792013002
Cr-Commit-Position: refs/heads/master@{#463570}
Committed: https://chromium.googlesource.com/chromium/src/+/76108c2bf3ad1a4bac901ad99230f58faf87bc4b

[andypaicu, 2017-04-03 14:38:09]

Description was changed from

==========
Stop CSP from matching independent scheme/port upgrades (content layer)

Made the changes necessary in content to mimic the behaviour of
https://codereview.chromium.org/2708873002
Copied from there:
"Content-Security-Policy allows an url to match a source-expression even
if the scheme or the port doesn't matches, but in this case it must be an
upgrade to a more secure scheme(http->https) and more secure port(80->443).
The problem is that it happens independently, so it is allowed to have an
upgrade of the port without the scheme (http over 443) or an upgrade of the
scheme without the port (https over 80).

This is a change to force the upgrade to be both over port and scheme."

BUG=692499
==========

to

==========
Stop CSP from matching independent scheme/port upgrades (content layer)

Made the changes necessary in content to mimic the behaviour of
https://codereview.chromium.org/2708873002
Copied from there:
"Content-Security-Policy allows an url to match a source-expression even
if the scheme or the port doesn't matches, but in this case it must be an
upgrade to a more secure scheme(http->https) and more secure port(80->443).
The problem is that it happens independently, so it is allowed to have an
upgrade of the port without the scheme (http over 443) or an upgrade of the
scheme without the port (https over 80).

This is a change to force the upgrade to be both over port and scheme."

BUG=692499,692442
==========

[andypaicu, 2017-04-03 14:38:19]

The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 14:38:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[andypaicu, 2017-04-03 14:41:11]

andypaicu@chromium.org changed reviewers:
+ arthursonzogni@chromium.org

[andypaicu, 2017-04-03 14:41:12]

Hi Arthur,

As discussed over e-mail: here is a code review for two of the bugs that you
mentioned.

[commit-bot: I haz the power, 2017-04-03 15:41:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 15:41:06]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-04-04 09:00:23]

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

[arthursonzogni, 2017-04-04 09:00:24]

On 2017/04/03 14:41:12, andypaicu wrote:
> Hi Arthur,
> 
> As discussed over e-mail: here is a code review for two of the bugs that you
> mentioned.

Thank you Andy! That's great!
I did not realize that making the port and protocol upgrade not independently
would complicate the code at this point.
I understand now that it is not easy.

I would like to keep the code very simple and simpler if possible. Maybe there
is a different way to see the problem.

I was thinking of this:
1) Remove the upgrade mechanism from the code.
2) Test the URL.
3) If it doesn't match and the url can be downgraded, test the downgraded URL.

It would replace the complicated logic: requireUpgrade, canUpgrade,
MatchingWildcard, MatchingUpgrade, MatchingExact, etc... It will simplify a
little bit the current version of AllowPort and AllowScheme as well.
The cons is that there will be a little bit more computation.

What do you think?
If you want, I will make an example. I have some other work to do before, so do
not worry if I don't post it today.

+CC mkwst

[andypaicu, 2017-04-04 09:26:43]

On 2017/04/04 at 09:00:24, arthursonzogni wrote:
> On 2017/04/03 14:41:12, andypaicu wrote:
> > Hi Arthur,
> > 
> > As discussed over e-mail: here is a code review for two of the bugs that you
> > mentioned.
> 
> Thank you Andy! That's great!
> I did not realize that making the port and protocol upgrade not independently
would complicate the code at this point.
> I understand now that it is not easy.
> 
> I would like to keep the code very simple and simpler if possible. Maybe there
is a different way to see the problem.
> 
> I was thinking of this:
> 1) Remove the upgrade mechanism from the code.
> 2) Test the URL.
> 3) If it doesn't match and the url can be downgraded, test the downgraded URL.
> 
> It would replace the complicated logic: requireUpgrade, canUpgrade,
MatchingWildcard, MatchingUpgrade, MatchingExact, etc... It will simplify a
little bit the current version of AllowPort and AllowScheme as well.
> The cons is that there will be a little bit more computation.
> 
> What do you think?
> If you want, I will make an example. I have some other work to do before, so
do not worry if I don't post it today.
> 
> +CC mkwst

Hi Arthur.

In principal I am all for making the code simpler and more readable. I have two
concerns though in this particular situation:

1) There might not be one single valid downgraded URL version for an URL, for
instance "https://a.com" could probably downgrade to both "http://a.com:80" and
"http://a.com" are we making the assumption that any downgraded URL will be good
enough to decide? Is it possible that based on how we implement the downgrade we
will get different matching results (for instance if we have directive that
matches one but no the other of these downgraded URLs)? It might be the case
that it actually does not matter and a simple translation of scheme:https->http,
port:443->80 or port:unspecified->unspecified is good enough.

2) Seeing that this code is a close mirror to the blink csp code, I would like
to keep them in sync as much as possible to make it easy to merge some day in
the future. This means that we would have to change the blink code as well which
might make the extra computation be a bit more impactful, perhaps not much, I
can't really tell if it will make a difference even there.

Let me know what your thoughts are.

[arthursonzogni, 2017-04-05 12:14:55]

Hi Andy,

Here is the example I was talking about:
https://codereview.chromium.org/2797183002
Andy & Mike, please tell me what do you think?

There is some test expectations that are different, but in the good direction.
I initially thought that it would require more computation, but it is not really
the case here.

On 2017/04/04 09:26:43, andypaicu wrote:
> 1) There might not be one single valid downgraded URL version for an URL, for
> instance "https://a.com" could probably downgrade to both "http://a.com:80"
and
> "http://a.com" are we making the assumption that any downgraded URL will be
good
> enough to decide? Is it possible that based on how we implement the downgrade
we
> will get different matching results (for instance if we have directive that
> matches one but no the other of these downgraded URLs)? It might be the case
> that it actually does not matter and a simple translation of
scheme:https->http,
> port:443->80 or port:unspecified->unspecified is good enough.

Thanks for the warning. I don't think there is such a problem in the example
since
it uses GURL::EffectiveIntPort.

On 2017/04/04 09:26:43, andypaicu wrote:
> 2) Seeing that this code is a close mirror to the blink csp code, I would like
> to keep them in sync as much as possible to make it easy to merge some day in
> the future. This means that we would have to change the blink code as well
which
> might make the extra computation be a bit more impactful, perhaps not much, I
> can't really tell if it will make a difference even there.
> 
> Let me know what your thoughts are.

Yes, if this change happens, it must be mirrored on the renderer side as well.

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
File content/common/content_security_policy/csp_source_unittest.cc (right):

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:50:
EXPECT_TRUE(Allow(source, GURL("https://a.com:80"), &context));
The example I made have the opposite test expectation here.
(https, 80) doesn't match the source-expression and it is not upgrade from the
insecure (http, 80). So it isn't allowed.

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:186:
EXPECT_TRUE(Allow(source, GURL("https://a.com:8080"), &context));
What about this test expectation?

Source's scheme is empty, so the scheme should be the same as the scheme of
'self' (e.g http).
(https, 8080) is not an upgrade from (http, 80), so it mustn't be allowed.

The example I made have the opposite test expectation.

[andypaicu, 2017-04-06 09:05:51]

Hi Arthur,

This is a good example and it only needs a tiny improvement

Consider this test (which is equivalent to one you raised in an inline comment
I think):

CSPSource source("http", "a.com", false, url::PORT_UNSPECIFIED, true, "");
EXPECT_TRUE(Allow(source, GURL("https://a.com:5224"), &context, true));

This test fails currently but it should pass because the source port is
wildcarded
and (https,*) surely is an upgrade of (http,*).
I think it should be easy to make sure it works with wildcard ports
and other than that I'm happy with it

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
File content/common/content_security_policy/csp_source_unittest.cc (right):

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:50:
EXPECT_TRUE(Allow(source, GURL("https://a.com:80"), &context));
On 2017/04/05 at 12:14:55, arthursonzogni wrote:
> The example I made have the opposite test expectation here.
> (https, 80) doesn't match the source-expression and it is not upgrade from the
insecure (http, 80). So it isn't allowed.

Yeah you're right it should not pass here, regardless of implementation this
should be changed to EXPECT_FALSE

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:50:
EXPECT_TRUE(Allow(source, GURL("https://a.com:80"), &context));
On 2017/04/05 at 12:14:55, arthursonzogni wrote:
> The example I made have the opposite test expectation here.
> (https, 80) doesn't match the source-expression and it is not upgrade from the
insecure (http, 80). So it isn't allowed.

Yeah this should be EXPECT_FALSE regardless of the implementation.

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:186:
EXPECT_TRUE(Allow(source, GURL("https://a.com:8080"), &context));
On 2017/04/05 at 12:14:55, arthursonzogni wrote:
> What about this test expectation?
> 
> Source's scheme is empty, so the scheme should be the same as the scheme of
'self' (e.g http).
> (https, 8080) is not an upgrade from (http, 80), so it mustn't be allowed.
> 
> The example I made have the opposite test expectation.

This is a wildcard port though so it should pass.

[Mike West, 2017-04-06 10:28:40]

This change LGTM once arthursonzogni@ is happy.

I'd like to see a layout test that exercises this code, however. I believe it
only kicks in for `frame-ancestors` and `frame-src` (is that correct, Arthur?),
so putting a test together with one of those directives that would match the
upgrade would be great.

[arthursonzogni, 2017-04-06 15:05:00]

On 2017/04/06 09:05:51, andypaicu wrote:
> Hi Arthur,
> 
> This is a good example and it only needs a tiny improvement
> 
> Consider this test (which is equivalent to one you raised in an inline comment
> I think):
> 
> CSPSource source("http", "a.com", false, url::PORT_UNSPECIFIED, true, "");
> EXPECT_TRUE(Allow(source, GURL("https://a.com:5224"), &context, true));
> 
> This test fails currently but it should pass because the source port is
> wildcarded
> and (https,*) surely is an upgrade of (http,*).
> I think it should be easy to make sure it works with wildcard ports
> and other than that I'm happy with it

I think that it should not pass because:
* (https, 5224) is not an upgrade from (http, <whatever you want>)
* (https, 5224) doesn't match "http://a.com:*

@mkwst: Is it okay to break the specification by not allowing independent
upgrade of the scheme and the port? If yes, what do you think is the correct new
test expectation for this case?

On 2017/04/06 10:28:40, Mike West (OOO until 4th) wrote:
> This change LGTM once arthursonzogni@ is happy.
> 
> I'd like to see a layout test that exercises this code, however. I believe it
> only kicks in for `frame-ancestors` and `frame-src` (is that correct,
Arthur?),
> so putting a test together with one of those directives that would match the
> upgrade would be great.
It only kicks for 'form-action' and 'frame-src'/'child-src' and only with
--browser-side-navigation for the moment.

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
File content/common/content_security_policy/csp_source_unittest.cc (right):

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:186:
EXPECT_TRUE(Allow(source, GURL("https://a.com:8080"), &context));
On 2017/04/06 09:05:51, andypaicu wrote:
> On 2017/04/05 at 12:14:55, arthursonzogni wrote:
> > What about this test expectation?
> > 
> > Source's scheme is empty, so the scheme should be the same as the scheme of
> 'self' (e.g http).
> > (https, 8080) is not an upgrade from (http, 80), so it mustn't be allowed.
> > 
> > The example I made have the opposite test expectation.
> 
> This is a wildcard port though so it should pass.

I think it should no pass for the reasons I have explained above, even if there
is a wildcard port. (https, 8080) is not an upgrade from (http, 80)  and (https,
8080) doesn't matches "a.com:*" because 'self' scheme is http.

[andypaicu, 2017-04-06 15:43:59]

On 2017/04/06 at 15:05:00, arthursonzogni wrote:
> On 2017/04/06 09:05:51, andypaicu wrote:
> > Hi Arthur,
> > 
> > This is a good example and it only needs a tiny improvement
> > 
> > Consider this test (which is equivalent to one you raised in an inline
comment
> > I think):
> > 
> > CSPSource source("http", "a.com", false, url::PORT_UNSPECIFIED, true, "");
> > EXPECT_TRUE(Allow(source, GURL("https://a.com:5224"), &context, true));
> > 
> > This test fails currently but it should pass because the source port is
> > wildcarded
> > and (https,*) surely is an upgrade of (http,*).
> > I think it should be easy to make sure it works with wildcard ports
> > and other than that I'm happy with it
> 
> I think that it should not pass because:
> * (https, 5224) is not an upgrade from (http, <whatever you want>)
> * (https, 5224) doesn't match "http://a.com:*
> 
> @mkwst: Is it okay to break the specification by not allowing independent
upgrade of the scheme and the port? If yes, what do you think is the correct new
test expectation for this case?
> 
> On 2017/04/06 10:28:40, Mike West (OOO until 4th) wrote:
> > This change LGTM once arthursonzogni@ is happy.
> > 
> > I'd like to see a layout test that exercises this code, however. I believe
it
> > only kicks in for `frame-ancestors` and `frame-src` (is that correct,
Arthur?),
> > so putting a test together with one of those directives that would match the
> > upgrade would be great.
> It only kicks for 'form-action' and 'frame-src'/'child-src' and only with
--browser-side-navigation for the moment.
> 
>
https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
> File content/common/content_security_policy/csp_source_unittest.cc (right):
> 
>
https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
> content/common/content_security_policy/csp_source_unittest.cc:186:
EXPECT_TRUE(Allow(source, GURL("https://a.com:8080"), &context));
> On 2017/04/06 09:05:51, andypaicu wrote:
> > On 2017/04/05 at 12:14:55, arthursonzogni wrote:
> > > What about this test expectation?
> > > 
> > > Source's scheme is empty, so the scheme should be the same as the scheme
of
> > 'self' (e.g http).
> > > (https, 8080) is not an upgrade from (http, 80), so it mustn't be allowed.
> > > 
> > > The example I made have the opposite test expectation.
> > 
> > This is a wildcard port though so it should pass.
> 
> I think it should no pass for the reasons I have explained above, even if
there is a wildcard port. (https, 8080) is not an upgrade from (http, 80)  and
(https, 8080) doesn't matches "a.com:*" because 'self' scheme is http.

I am tiny bit confused, perhaps @mkwst can shed some light. I thought the idea
of the upgrade was to allow insecure sources to also match the secure version of
the source (i.e. if a csp header is "http://a.com:80" we will also match
"https://a.com:443"). Surely by this logic if the header is "http://a.com:*"
then we would consider the secure version to be "https://a.com:*" and then we
would also allow
https requests on any port (in addition to allowing http requests on any port).

[arthursonzogni, 2017-04-06 15:57:52]

On 2017/04/06 15:43:59, andypaicu wrote:
> On 2017/04/06 at 15:05:00, arthursonzogni wrote:
> > On 2017/04/06 09:05:51, andypaicu wrote:
> > > Hi Arthur,
> > > 
> > > This is a good example and it only needs a tiny improvement
> > > 
> > > Consider this test (which is equivalent to one you raised in an inline
> comment
> > > I think):
> > > 
> > > CSPSource source("http", "a.com", false, url::PORT_UNSPECIFIED, true, "");
> > > EXPECT_TRUE(Allow(source, GURL("https://a.com:5224"), &context, true));
> > > 
> > > This test fails currently but it should pass because the source port is
> > > wildcarded
> > > and (https,*) surely is an upgrade of (http,*).
> > > I think it should be easy to make sure it works with wildcard ports
> > > and other than that I'm happy with it
> > 
> > I think that it should not pass because:
> > * (https, 5224) is not an upgrade from (http, <whatever you want>)
> > * (https, 5224) doesn't match "http://a.com:*
> > 
> > @mkwst: Is it okay to break the specification by not allowing independent
> upgrade of the scheme and the port? If yes, what do you think is the correct
new
> test expectation for this case?
> > 
> > On 2017/04/06 10:28:40, Mike West (OOO until 4th) wrote:
> > > This change LGTM once arthursonzogni@ is happy.
> > > 
> > > I'd like to see a layout test that exercises this code, however. I believe
> it
> > > only kicks in for `frame-ancestors` and `frame-src` (is that correct,
> Arthur?),
> > > so putting a test together with one of those directives that would match
the
> > > upgrade would be great.
> > It only kicks for 'form-action' and 'frame-src'/'child-src' and only with
> --browser-side-navigation for the moment.
> > 
> >
>
https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
> > File content/common/content_security_policy/csp_source_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
> > content/common/content_security_policy/csp_source_unittest.cc:186:
> EXPECT_TRUE(Allow(source, GURL("https://a.com:8080"), &context));
> > On 2017/04/06 09:05:51, andypaicu wrote:
> > > On 2017/04/05 at 12:14:55, arthursonzogni wrote:
> > > > What about this test expectation?
> > > > 
> > > > Source's scheme is empty, so the scheme should be the same as the scheme
> of
> > > 'self' (e.g http).
> > > > (https, 8080) is not an upgrade from (http, 80), so it mustn't be
allowed.
> > > > 
> > > > The example I made have the opposite test expectation.
> > > 
> > > This is a wildcard port though so it should pass.
> > 
> > I think it should no pass for the reasons I have explained above, even if
> there is a wildcard port. (https, 8080) is not an upgrade from (http, 80)  and
> (https, 8080) doesn't matches "a.com:*" because 'self' scheme is http.
> 
> I am tiny bit confused, perhaps @mkwst can shed some light. I thought the idea
> of the upgrade was to allow insecure sources to also match the secure version
of
> the source (i.e. if a csp header is "http://a.com:80" we will also match
> "https://a.com:443"). Surely by this logic if the header is "http://a.com:*"
> then we would consider the secure version to be "https://a.com:*" and then we
> would also allow
> https requests on any port (in addition to allowing http requests on any
port).

Yes, I understand what you mean.

You are trying to match the current url with an upgraded version of the
source-expression.
I am trying to match the source-expression with an url that upgrades to the
current one.

There is no url that matches the source-expression that can be upgraded to the
current url, but the source-expression can be upgraded to something that matches
the url.

If we make this change (not compatible with the current specification) then we
must decide how it behaves with the port wildcard.

[Mike West, 2017-04-07 07:20:28]

I talked with Andy briefly about this this morning:

The central motivation for allowing upgrades is to prevent things like
https://zyan.scripts.mit.edu/sniffly/ from using CSP and HSTS to discover what
sites you've been to in the past. It would be unfortunate if we reintroduced
that while tightening our behavior here. I'd suggest that `*` should mean `*` in
this case. We should allow `http://example.com:*` to match
`https://example.com:443` and `https://example.com:80` and
`https://example.com:10997832`. This also is in-line with HSTS and
Upgrade-Insecure-Requests, which will remap `80` to `443`, but will leave
non-standard ports alone when upgrading the scheme.

[arthursonzogni, 2017-04-07 09:20:27]

On 2017/04/07 07:20:28, Mike West (OOO until 4th) wrote:
> I talked with Andy briefly about this this morning:
> 
> The central motivation for allowing upgrades is to prevent things like
> https://zyan.scripts.mit.edu/sniffly/ from using CSP and HSTS to discover what
> sites you've been to in the past. It would be unfortunate if we reintroduced
> that while tightening our behavior here. I'd suggest that `*` should mean `*`
in
> this case. We should allow `http://example.com:*` to match
> `https://example.com:443` and `https://example.com:80` and
> `https://example.com:10997832`. This also is in-line with HSTS and
> Upgrade-Insecure-Requests, which will remap `80` to `443`, but will leave
> non-standard ports alone when upgrading the scheme.

Thanks Mike. I think it makes sense.

It's a pity. In that case, I suppose my suggestion no longer holds good
(downgrading the url instead of upgrading the source-expression).

Let's continue on what Andy proposes.
I am now globally okay with this change. Here are a few nit and suggestions:

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
File content/common/content_security_policy/csp_source.cc (right):

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:43:
url.SchemeIs(url::kHttpsScheme)) ||
Nit: git cl format: it should fit on one line.

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:47:
url.SchemeIs(url::kWssScheme))) {
Nit: git cl format: it should fit on one line.

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:76: const CSPSource&
source, const GURL& url) {
Nit: git cl format gives me:
CSPSource::PortMatchingResult SourceAllowPort(const CSPSource& source,
                                              const GURL& url) {

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:138: result ==
CSPSource::PortMatchingResult::MatchingWildcard;
Nit: git cl format gives me:
bool inline canUpgrade(const CSPSource::PortMatchingResult result) {
  return result == CSPSource::PortMatchingResult::MatchingUpgrade ||
         result == CSPSource::PortMatchingResult::MatchingWildcard;
}

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:181:
SchemeMatchingResult::NotMatching;
Nit: git cl format gives me:
return SourceAllowScheme(source, url, context) !=
       SchemeMatchingResult::NotMatching;

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:189: }
This looks clearer to me:

if (requiresUpgrade(schemeResult) && !canUpgrade(portResult)) return false;
if (requiresUpgrade(portResult) && !canUpgrade(schemeResult)) return false;

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
File content/common/content_security_policy/csp_source.h (right):

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.h:40: 
Can you please put these enums inside the implementation? It is not used in the
header and users of this struct doesn't need to know about our implementation.

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
File content/common/content_security_policy/csp_source_unittest.cc (right):

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:49: // This passes
because the source is "scheme only" so the upgrade is allowed
Nit: a dot is missing at the end of this comment.

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:204: // Should not
allow scheme upgrades unless both port and scheme are upgraded
Nit: A dot is missing at the end of this comment.

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:332: 
Nit: I think you can probably remove this empty line.

[andypaicu, 2017-04-07 11:33:24]

The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 11:33:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[andypaicu, 2017-04-07 11:34:24]

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
File content/common/content_security_policy/csp_source.cc (right):

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:43:
url.SchemeIs(url::kHttpsScheme)) ||
On 2017/04/07 at 09:20:27, arthursonzogni wrote:
> Nit: git cl format: it should fit on one line.

Done. I hated the fact that the middle one does not fit but long live "git cl
format".

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:47:
url.SchemeIs(url::kWssScheme))) {
On 2017/04/07 at 09:20:27, arthursonzogni wrote:
> Nit: git cl format: it should fit on one line.

Done. I hated the fact that the middle one does not fit but long live "git cl
format".

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:76: const CSPSource&
source, const GURL& url) {
On 2017/04/07 at 09:20:27, arthursonzogni wrote:
> Nit: git cl format gives me:
> CSPSource::PortMatchingResult SourceAllowPort(const CSPSource& source,
>                                               const GURL& url) {

Done

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:138: result ==
CSPSource::PortMatchingResult::MatchingWildcard;
On 2017/04/07 at 09:20:27, arthursonzogni wrote:
> Nit: git cl format gives me:
> bool inline canUpgrade(const CSPSource::PortMatchingResult result) {
>   return result == CSPSource::PortMatchingResult::MatchingUpgrade ||
>          result == CSPSource::PortMatchingResult::MatchingWildcard;
> }

Done

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.cc:181:
SchemeMatchingResult::NotMatching;
On 2017/04/07 at 09:20:27, arthursonzogni wrote:
> Nit: git cl format gives me:
> return SourceAllowScheme(source, url, context) !=
>        SchemeMatchingResult::NotMatching;

Done

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
File content/common/content_security_policy/csp_source.h (right):

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source.h:40: 
On 2017/04/07 at 09:20:27, arthursonzogni wrote:
> Can you please put these enums inside the implementation? It is not used in
the header and users of this struct doesn't need to know about our
implementation.

Done

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
File content/common/content_security_policy/csp_source_unittest.cc (right):

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:49: // This passes
because the source is "scheme only" so the upgrade is allowed
On 2017/04/07 at 09:20:27, arthursonzogni wrote:
> Nit: a dot is missing at the end of this comment.

Done

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:204: // Should not
allow scheme upgrades unless both port and scheme are upgraded
On 2017/04/07 at 09:20:27, arthursonzogni wrote:
> Nit: A dot is missing at the end of this comment.

Done

https://codereview.chromium.org/2792013002/diff/1/content/common/content_secu...
content/common/content_security_policy/csp_source_unittest.cc:332: 
On 2017/04/07 at 09:20:27, arthursonzogni wrote:
> Nit: I think you can probably remove this empty line.

Done

[commit-bot: I haz the power, 2017-04-07 12:28:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 12:28:46]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-04-10 09:00:33]

Thanks LGTM!

[andypaicu, 2017-04-10 09:07:39]

The CQ bit was checked by andypaicu@chromium.org

[andypaicu, 2017-04-10 09:07:39]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2792013002/#ps20001
(title: "CR changes")

[commit-bot: I haz the power, 2017-04-10 09:07:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 09:13:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 09:13:56]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[arthursonzogni, 2017-04-10 09:22:25]

On 2017/04/10 09:13:56, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

It looks like they have updated "git cl format" again and you need to use two
lines inside the conditions.

[arthursonzogni, 2017-04-10 09:24:00]

On 2017/04/10 09:22:25, arthursonzogni wrote:
> On 2017/04/10 09:13:56, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> It looks like they have updated "git cl format" again and you need to use two
> lines inside the conditions.

Please note that you will also need an OWNER here.

[andypaicu, 2017-04-10 10:02:36]

andypaicu@chromium.org changed reviewers:
+ jochen@chromium.org

[andypaicu, 2017-04-10 10:02:36]

andypaicu@chromium.org changed required reviewers:
+ jochen@chromium.org

[andypaicu, 2017-04-10 10:06:23]

On 2017/04/10 at 09:24:00, arthursonzogni wrote:
> On 2017/04/10 09:22:25, arthursonzogni wrote:
> > On 2017/04/10 09:13:56, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> > >
> >
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > 
> > It looks like they have updated "git cl format" again and you need to use
two
> > lines inside the conditions.
> 
> Please note that you will also need an OWNER here.

Hi @jochen, I need an OWNER's permission, could you have a loot at this?

@arthursonzogni
Hmm, I'm not getting any diffs with git cl format on my end. Which condition are
you saying needs to be on two lines?
I can see the warning in the presubmit build but I don't see any extra details.

[arthursonzogni, 2017-04-10 11:52:38]

On 2017/04/10 10:06:23, andypaicu wrote:
> On 2017/04/10 at 09:24:00, arthursonzogni wrote:
> > On 2017/04/10 09:22:25, arthursonzogni wrote:
> > > On 2017/04/10 09:13:56, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> > > >
> > >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > > 
> > > It looks like they have updated "git cl format" again and you need to use
> two
> > > lines inside the conditions.
> > 
> > Please note that you will also need an OWNER here.
> 
> Hi @jochen, I need an OWNER's permission, could you have a loot at this?
> 
> @arthursonzogni
> Hmm, I'm not getting any diffs with git cl format on my end. Which condition
are
> you saying needs to be on two lines?
> I can see the warning in the presubmit build but I don't see any extra
details.

Maybe you can try to do a git pull inside the depot_tools directory.
Here is the diff I get with git cl format.

diff --git a/content/common/content_security_policy/csp_context.cc
b/content/common/content_security_policy/csp_context.cc
index 8270b0fac5df..ab249dd3f258 100644
--- a/content/common/content_security_policy/csp_context.cc
+++ b/content/common/content_security_policy/csp_context.cc
@@ -61 +61,3 @@ bool CSPContext::ProtocolIsSelf(const GURL& url) {
-const std::string& CSPContext::GetSelfScheme() { return self_scheme_; }
+const std::string& CSPContext::GetSelfScheme() {
+  return self_scheme_;
+}

diff --git a/content/common/content_security_policy/csp_source.cc
b/content/common/content_security_policy/csp_source.cc
index bb5dcdf9229d..7ebd7e755061 100644
--- a/content/common/content_security_policy/csp_source.cc
+++ b/content/common/content_security_policy/csp_source.cc
@@ -37 +37,2 @@ enum class SchemeMatchingResult { NotMatching, MatchingUpgrade,
MatchingExact };
-SchemeMatchingResult SourceAllowScheme(const CSPSource& source, const GURL&
url,
+SchemeMatchingResult SourceAllowScheme(const CSPSource& source,
+                                       const GURL& url,
@@ -48 +49,2 @@ SchemeMatchingResult SourceAllowScheme(const CSPSource& source,
const GURL& url,
-  if (url.SchemeIs(source_scheme)) return SchemeMatchingResult::MatchingExact;
+  if (url.SchemeIs(source_scheme))
+    return SchemeMatchingResult::MatchingExact;
@@ -84 +86,2 @@ PortMatchingResult SourceAllowPort(const CSPSource& source,
const GURL& url) {
-  if (source.is_port_wildcard) return PortMatchingResult::MatchingWildcard;
+  if (source.is_port_wildcard)
+    return PortMatchingResult::MatchingWildcard;
@@ -190,2 +193,4 @@ bool CSPSource::Allow(const CSPSource& source,
-  if (requiresUpgrade(schemeResult) && !canUpgrade(portResult)) return false;
-  if (requiresUpgrade(portResult) && !canUpgrade(schemeResult)) return false;
+  if (requiresUpgrade(schemeResult) && !canUpgrade(portResult))
+    return false;
+  if (requiresUpgrade(portResult) && !canUpgrade(schemeResult))
+    return false;

[jochen (gone - plz use gerrit), 2017-04-10 12:51:26]

Mike, can you add yourself as owner for that dir?

lgtm..

[andypaicu, 2017-04-10 13:37:21]

The CQ bit was checked by andypaicu@chromium.org

[andypaicu, 2017-04-10 13:37:21]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mkwst@chromium.org, arthursonzogni@chromium.org
Link to the patchset: https://codereview.chromium.org/2792013002/#ps40001
(title: "Format changes")

[commit-bot: I haz the power, 2017-04-10 13:37:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 14:31:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 14:31:43]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[andypaicu, 2017-04-11 07:57:56]

The CQ bit was checked by andypaicu@chromium.org

[commit-bot: I haz the power, 2017-04-11 07:58:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 08:04:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491897476435490,
"parent_rev": "28d7e2fdd0bb11a7d0061b60ee1664728ce869d5", "commit_rev":
"76108c2bf3ad1a4bac901ad99230f58faf87bc4b"}

[commit-bot: I haz the power, 2017-04-11 08:04:50]

Description was changed from

==========
Stop CSP from matching independent scheme/port upgrades (content layer)

Made the changes necessary in content to mimic the behaviour of
https://codereview.chromium.org/2708873002
Copied from there:
"Content-Security-Policy allows an url to match a source-expression even
if the scheme or the port doesn't matches, but in this case it must be an
upgrade to a more secure scheme(http->https) and more secure port(80->443).
The problem is that it happens independently, so it is allowed to have an
upgrade of the port without the scheme (http over 443) or an upgrade of the
scheme without the port (https over 80).

This is a change to force the upgrade to be both over port and scheme."

BUG=692499,692442
==========

to

==========
Stop CSP from matching independent scheme/port upgrades (content layer)

Made the changes necessary in content to mimic the behaviour of
https://codereview.chromium.org/2708873002
Copied from there:
"Content-Security-Policy allows an url to match a source-expression even
if the scheme or the port doesn't matches, but in this case it must be an
upgrade to a more secure scheme(http->https) and more secure port(80->443).
The problem is that it happens independently, so it is allowed to have an
upgrade of the port without the scheme (http over 443) or an upgrade of the
scheme without the port (https over 80).

This is a change to force the upgrade to be both over port and scheme."

BUG=692499,692442

Review-Url: https://codereview.chromium.org/2792013002
Cr-Commit-Position: refs/heads/master@{#463570}
Committed:
https://chromium.googlesource.com/chromium/src/+/76108c2bf3ad1a4bac901ad99230...
==========

[commit-bot: I haz the power, 2017-04-11 08:04:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/76108c2bf3ad1a4bac901ad99230...