Make extensions use a correct same-origin check.

Make extensions use a correct same-origin check.

GURL::GetOrigin does not do the right thing for all types of URLs.

BUG=573317
Committed: https://crrev.com/5c437bcc7a51edbef45242c5173cf7871fde2866
Cr-Commit-Position: refs/heads/master@{#373381}

[palmer, 2016-02-02 00:53:55]

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

[palmer, 2016-02-02 00:54:19]

palmer@chromium.org changed reviewers:
+ meacer@chromium.org

[palmer, 2016-02-02 00:54:20]

meacer: Any thoughts on whether I'm on the right track?

[commit-bot: I haz the power, 2016-02-02 00:55:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1658913002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1658913002/1

[meacer, 2016-02-02 01:35:08]

I'm sure there is a good reason for this, but I'm wondering why there isn't an
overload for == operator instead of IsSameOriginWith (git archaeology didn't
reveal anything about this).
Otherwise looking good.

https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:135:
url::Origin(extension->url()))))) {
While you are at it, do you mind applying De Morgan's law here? This seems a bit
easier to reason about:

if (url.SchemeIs(url::kAboutScheme) &&
    !extension->permissions_data()->HasHostPermission(url) &&
    !url::Origin(url).IsSameOriginWith(url::Origin(extension->url()) {
  return false;
}

[commit-bot: I haz the power, 2016-02-02 01:41:55]

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

[commit-bot: I haz the power, 2016-02-02 01:41:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[palmer, 2016-02-02 23:12:38]

https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:135:
url::Origin(extension->url()))))) {
On 2016/02/02 01:35:08, Mustafa Emre Acer wrote:
> While you are at it, do you mind applying De Morgan's law here? This seems a
bit
> easier to reason about:
> 
> if (url.SchemeIs(url::kAboutScheme) &&
>     !extension->permissions_data()->HasHostPermission(url) &&
>     !url::Origin(url).IsSameOriginWith(url::Origin(extension->url()) {
>   return false;
> }

Good idea. But, it should be

    !url.SchemeIs(url::kAboutScheme)

, right?

[palmer, 2016-02-02 23:13:49]

> I'm sure there is a good reason for this, but I'm wondering why there isn't an
> overload for == operator instead of IsSameOriginWith (git archaeology didn't
> reveal anything about this).

Chromium style strongly prefers not to overload operators. You'll notice the
only operator overloads are the ones that are strictly necessary (for e.g.
printing and sorting). I'd prefer operator==, but I'm sure a style lawyer would
strike it down. Instead, I added a convenience function to clean things up.

[meacer, 2016-02-02 23:27:00]

On 2016/02/02 23:13:49, palmer wrote:
> > I'm sure there is a good reason for this, but I'm wondering why there isn't
an
> > overload for == operator instead of IsSameOriginWith (git archaeology didn't
> > reveal anything about this).
> 
> Chromium style strongly prefers not to overload operators. You'll notice the
> only operator overloads are the ones that are strictly necessary (for e.g.
> printing and sorting). I'd prefer operator==, but I'm sure a style lawyer
would
> strike it down. Instead, I added a convenience function to clean things up.

The reason I asked is that GURL has operator== overloaded, but I suppose that's
taken from google3 or some legacy code.

[meacer, 2016-02-02 23:27:22]

https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:135:
url::Origin(extension->url()))))) {
On 2016/02/02 23:12:38, palmer wrote:
> On 2016/02/02 01:35:08, Mustafa Emre Acer wrote:
> > While you are at it, do you mind applying De Morgan's law here? This seems a
> bit
> > easier to reason about:
> > 
> > if (url.SchemeIs(url::kAboutScheme) &&
> >     !extension->permissions_data()->HasHostPermission(url) &&
> >     !url::Origin(url).IsSameOriginWith(url::Origin(extension->url()) {
> >   return false;
> > }
> 
> Good idea. But, it should be
> 
>     !url.SchemeIs(url::kAboutScheme)
> 
> , right?

Sure :)

[palmer, 2016-02-03 00:17:11]

palmer@chromium.org changed reviewers:
+ brettw@chromium.org

[palmer, 2016-02-03 00:17:12]

brettw: I added a simple convenience function to url::Origin which makes the
call sites much nicer. PTAL.

meacer: See what you think of extensions/common/url_pattern_set.cc.

[palmer, 2016-02-03 00:18:42]

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

[commit-bot: I haz the power, 2016-02-03 00:23:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1658913002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1658913002/40001

[commit-bot: I haz the power, 2016-02-03 02:24:29]

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

[commit-bot: I haz the power, 2016-02-03 02:24:30]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-02-03 19:12:38]

lgtm

https://codereview.chromium.org/1658913002/diff/40001/extensions/common/url_p...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/1658913002/diff/40001/extensions/common/url_p...
extensions/common/url_pattern_set.cc:158: const url::Origin real_origin =
url::Origin(origin);
This would be better as:
  const url::Origin(origin);

https://codereview.chromium.org/1658913002/diff/40001/extensions/renderer/fil...
File extensions/renderer/file_system_natives.cc (right):

https://codereview.chromium.org/1658913002/diff/40001/extensions/renderer/fil...
extensions/renderer/file_system_natives.cc:46: const GURL origin =
GURL(url::Origin(context_url).Serialize());
This would be better as:
  const GURL origin(url::Origin(context_url).Serialize());

[meacer, 2016-02-03 20:34:40]

Lgtm (not an owner for any of these files)

[palmer, 2016-02-03 20:47:58]

https://codereview.chromium.org/1658913002/diff/40001/extensions/common/url_p...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/1658913002/diff/40001/extensions/common/url_p...
extensions/common/url_pattern_set.cc:158: const url::Origin real_origin =
url::Origin(origin);
On 2016/02/03 19:12:38, brettw wrote:
> This would be better as:
>   const url::Origin(origin);

Done.

https://codereview.chromium.org/1658913002/diff/40001/extensions/renderer/fil...
File extensions/renderer/file_system_natives.cc (right):

https://codereview.chromium.org/1658913002/diff/40001/extensions/renderer/fil...
extensions/renderer/file_system_natives.cc:46: const GURL origin =
GURL(url::Origin(context_url).Serialize());
On 2016/02/03 19:12:38, brettw wrote:
> This would be better as:
>   const GURL origin(url::Origin(context_url).Serialize());

Done.

[palmer, 2016-02-03 20:50:13]

palmer@chromium.org changed reviewers:
+ benwells@chromium.org, rdevlin.cronin@chromium.org

[palmer, 2016-02-03 20:50:13]

benwells and/or rdevlin.cronin: Could you please take an OWNERS look at
extensions/? Thanks!

[Devlin, 2016-02-03 21:01:25]

extensions lgtm

https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_p...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_p...
extensions/common/url_pattern_set.cc:159:
DCHECK(real_origin.IsSameOriginWith(url::Origin(origin.GetOrigin())));
Can we not use the static IsSameOriginWith() here?

[palmer, 2016-02-03 21:03:08]

https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_p...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_p...
extensions/common/url_pattern_set.cc:159:
DCHECK(real_origin.IsSameOriginWith(url::Origin(origin.GetOrigin())));
On 2016/02/03 21:01:25, Devlin wrote:
> Can we not use the static IsSameOriginWith() here?

No, I actually want to maintain the check on GURL::GetOrigin.

[Devlin, 2016-02-03 21:10:11]

https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_p...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_p...
extensions/common/url_pattern_set.cc:159:
DCHECK(real_origin.IsSameOriginWith(url::Origin(origin.GetOrigin())));
On 2016/02/03 21:03:08, palmer wrote:
> On 2016/02/03 21:01:25, Devlin wrote:
> > Can we not use the static IsSameOriginWith() here?
> 
> No, I actually want to maintain the check on GURL::GetOrigin.

I'm sure that's right.  Can you tell me why? :) (For my own edification)

[palmer, 2016-02-03 21:14:35]

> > No, I actually want to maintain the check on GURL::GetOrigin.
> 
> I'm sure that's right.  Can you tell me why? :) (For my own edification)

The caller is expecting a stringly-typed interface to hold, and GURL::GetOrigin
comes from that tradition. So, now that we have a stronger type — the ability to
distinguish GURL and Origin, and Origin having correct(er) behavior for
edge-case origin types — it's good to maintain that callers' expectations about
their strings either still hold, or DCHECK.

[palmer, 2016-02-03 21:47:20]

The CQ bit was checked by palmer@chromium.org

[palmer, 2016-02-03 21:47:21]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/1658913002/#ps60001
(title: "Respond to comments.")

[commit-bot: I haz the power, 2016-02-03 21:48:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1658913002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1658913002/60001

[commit-bot: I haz the power, 2016-02-03 23:21:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-03 23:22:41]

Description was changed from

==========
Make extensions use a correct same-origin check.

GURL::GetOrigin does not do the right thing for all types of URLs.

BUG=573317
==========

to

==========
Make extensions use a correct same-origin check.

GURL::GetOrigin does not do the right thing for all types of URLs.

BUG=573317

Committed: https://crrev.com/5c437bcc7a51edbef45242c5173cf7871fde2866
Cr-Commit-Position: refs/heads/master@{#373381}
==========

[commit-bot: I haz the power, 2016-02-03 23:22:42]

Patchset 4 (id:??) landed as
https://crrev.com/5c437bcc7a51edbef45242c5173cf7871fde2866
Cr-Commit-Position: refs/heads/master@{#373381}