Relax same-origin policy for ServiceWorker openWindow() in Chromium.

Relax same-origin policy for ServiceWorker openWindow() in Chromium.

Per blink-dev discussion:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/vCd9TysLAso/UcNQMme-9LsJ

This is relaxing the browser-side checks in order to no longer kill a
renderer process if it tries to open a cross-origin window from a service
worker. It will allow the Blink restriction to be relaxed:
https://codereview.chromium.org/985043002

The browser process now checks whether the renderer process is allowed
access to the given URL, blocking access to special URLs like chrome://.

BUG=457187
Committed: https://crrev.com/1af793910c3a05bf5ab0d790e1bcf282d03592d9
Cr-Commit-Position: refs/heads/master@{#319648}

[mlamouri (slow - plz ping), 2015-03-06 15:44:55]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org, michaeln@chromium.org, mkwst@chromium.org,
tsepez@chromium.org

[mlamouri (slow - plz ping), 2015-03-06 15:44:56]

tsepez@, jochen@ and mkwst@, could one of you have a look with regard to the
security aspects of that change?

michaeln@, PTAL as an OWNER?

[Mike West, 2015-03-06 15:53:22]

Other than all the red bots, this looks pretty reasonable to me, given the
discussion in
https://groups.google.com/a/chromium.org/d/msg/blink-dev/vCd9TysLAso/UcNQMme-....
Please add a link to that thread to the CL discussion.

Please add some tests as well. :)

[mlamouri (slow - plz ping), 2015-03-06 15:57:44]

On 2015/03/06 at 15:53:22, mkwst wrote:
> Other than all the red bots, this looks pretty reasonable to me, given the
discussion in
https://groups.google.com/a/chromium.org/d/msg/blink-dev/vCd9TysLAso/UcNQMme-....
Please add a link to that thread to the CL discussion.
> 
> Please add some tests as well. :)

Discussion linked. The tests are in the Blink CL as LayoutTests.

[Mike West, 2015-03-06 16:00:08]

Got it. LGTM, but you'll need an OWNER.

[falken, 2015-03-06 16:03:08]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2015-03-06 16:03:08]

drive-by comment

https://codereview.chromium.org/980383004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1172: // URL. It is
possible to receive requests to open such URLs because the
nit: "receive requests to open disallowed URLs"?

https://codereview.chromium.org/980383004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1177: sanitized_url =
GURL(url::kAboutBlankURL);
Maybe I'm missing something... why open about:blank instead of rejecting
openWindow() with an error for such URLs?

[mlamouri (slow - plz ping), 2015-03-06 16:17:20]

https://codereview.chromium.org/980383004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1177: sanitized_url =
GURL(url::kAboutBlankURL);
On 2015/03/06 at 16:03:08, falken wrote:
> Maybe I'm missing something... why open about:blank instead of rejecting
openWindow() with an error for such URLs?

Hmm, actually, I should have left a comment about that. I have a few thoughts.

Ideally, I would like to kill the process but I can't because it wasn't very
hard to find cases (view-source://) where the request might go trough.

Also, I could use RenderProcessHost::FilterURL() which is doing a couple more
checks than what I do (and sets to about:blank too).

I have no strong opinion between rejecting and allowing the call with
about:blank. I ended up allowing the call to be closer to the behaviour of
window.open() which fails for chrome:// URLs or file://. openWindow() will only
reject for situations where window.open() would throw and will open about:blank
when window.open() would.

WDYT?

https://codereview.chromium.org/980383004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1177: sanitized_url =
GURL(url::kAboutBlankURL);
On 2015/03/06 at 16:03:08, falken wrote:
> Maybe I'm missing something... why open about:blank instead of rejecting
openWindow() with an error for such URLs?

Hmm, actually, I should have left a comment about that. I have a few thoughts.

Ideally, I would like to kill the process but I can't because it wasn't very
hard to find cases (view-source://) where the request might go trough.

Also, I could use RenderProcessHost::FilterURL() which is doing a couple more
checks than what I do (and sets to about:blank too).

I have no strong opinion between rejecting and allowing the call with
about:blank. I ended up allowing the call to be closer to the behaviour of
window.open() which fails for chrome:// URLs or file://. openWindow() will only
reject for situations where window.open() would throw and will open about:blank
when window.open() would.

WDYT?

https://codereview.chromium.org/980383004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1177: sanitized_url =
GURL(url::kAboutBlankURL);
On 2015/03/06 at 16:03:08, falken wrote:
> Maybe I'm missing something... why open about:blank instead of rejecting
openWindow() with an error for such URLs?

Hmm, actually, I should have left a comment about that. I have a few thoughts.

Ideally, I would like to kill the process but I can't because it wasn't very
hard to find cases (view-source://) where the request might go trough.

Also, I could use RenderProcessHost::FilterURL() which is doing a couple more
checks than what I do (and sets to about:blank too).

I have no strong opinion between rejecting and allowing the call with
about:blank. I ended up allowing the call to be closer to the behaviour of
window.open() which fails for chrome:// URLs or file://. openWindow() will only
reject for situations where window.open() would throw and will open about:blank
when window.open() would.

WDYT?

[mlamouri (slow - plz ping), 2015-03-06 16:17:57]

I swear, I'm not a parrot :)

[falken, 2015-03-06 16:43:32]

https://codereview.chromium.org/980383004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1177: sanitized_url =
GURL(url::kAboutBlankURL);
On 2015/03/06 16:17:19, Mounir Lamouri wrote:
> On 2015/03/06 at 16:03:08, falken wrote:
> > Maybe I'm missing something... why open about:blank instead of rejecting
> openWindow() with an error for such URLs?
> 
> Hmm, actually, I should have left a comment about that. I have a few thoughts.
> 
> Ideally, I would like to kill the process but I can't because it wasn't very
> hard to find cases (view-source://) where the request might go trough.
> 
> Also, I could use RenderProcessHost::FilterURL() which is doing a couple more
> checks than what I do (and sets to about:blank too).
> 
> I have no strong opinion between rejecting and allowing the call with
> about:blank. I ended up allowing the call to be closer to the behaviour of
> window.open() which fails for chrome:// URLs or file://. openWindow() will
only
> reject for situations where window.open() would throw and will open
about:blank
> when window.open() would.
> 
> WDYT?

Ah that's interesting, I didn't know window.open('view-source://blahblah');
opens about:blank. I was originally thinking it'd be weird for the developer to
not get an indication that the URL wasn't really opened, but don't feel
strongly. Following window.open sounds OK. I agree a comment to that effect
would be nice.

[mlamouri (slow - plz ping), 2015-03-06 17:55:08]

Double-checked: using FilterURL would be under optimized given that it requires
a RenderProcessHost which would require to do some thread hoping. It's probably
not worth it.

[mlamouri (slow - plz ping), 2015-03-06 21:25:54]

Note: falken@ and michaeln@ I will need one of your l-g-t-m in order to land
this.

[michaeln, 2015-03-06 23:26:03]

https://codereview.chromium.org/980383004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1179: sanitized_url =
GURL(url::kAboutBlankURL);
Are you sure about opening about:blank in this case? What about not doing
anything here?

(hmmm, reading the response to matts earlier question along these lines)

So long as this is subject to popup blocker protections similar to those applied
to window.open, mimic'ing its behavior seems reasonable. Otherwise it may be
more prudent to silently fail?

I'd be curious to know what folks closer to popup blocking think?

As an end user, my personal pref would probably be for this to result in no
windown opening since the window will show me nothing of value.

[mlamouri (slow - plz ping), 2015-03-06 23:40:41]

https://codereview.chromium.org/980383004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1179: sanitized_url =
GURL(url::kAboutBlankURL);
On 2015/03/06 at 23:26:03, michaeln wrote:
> Are you sure about opening about:blank in this case? What about not doing
anything here?
> 
> (hmmm, reading the response to matts earlier question along these lines)
> 
> So long as this is subject to popup blocker protections similar to those
applied to window.open, mimic'ing its behavior seems reasonable. Otherwise it
may be more prudent to silently fail?
> 
> I'd be curious to know what folks closer to popup blocking think?
> 
> As an end user, my personal pref would probably be for this to result in no
windown opening since the window will show me nothing of value.

Just to make sure it's clear: the CanRequestURL() is not expected to return
false in normal cases. If a SW tries to open a regular URL, it should just work.
Most unusual URLs should be blocked on the Blink side. If I didn't find
view-source:// was not blocked by Blink, I would have killed the renderer when
CanRequestURL() would return false.

[michaeln, 2015-03-07 00:02:51]

> Just to make sure it's clear: the CanRequestURL() is not expected to return
> false in normal cases. If a SW tries to open a regular URL, it should just
work.
> Most unusual URLs should be blocked on the Blink side. If I didn't find
> view-source:// was not blocked by Blink, I would have killed the renderer when
> CanRequestURL() would return false.

Ok, now you have me thinking more strongly that "do nothing" would be the better
option. You say this is very unexpected. If you get here, its for one of two
reasons
- something got past the blink-side tests so here we are
- the renderer is compromised and talking smack

if the blink side had caught it, no window would be opened
if we could reliable tell the renderer was borked, we'd kill it (but we can't
reliably tell)

i vote to "do nothing" in this case

[mlamouri (slow - plz ping), 2015-03-07 00:13:47]

On 2015/03/07 at 00:02:51, michaeln wrote:
> > Just to make sure it's clear: the CanRequestURL() is not expected to return
> > false in normal cases. If a SW tries to open a regular URL, it should just
work.
> > Most unusual URLs should be blocked on the Blink side. If I didn't find
> > view-source:// was not blocked by Blink, I would have killed the renderer
when
> > CanRequestURL() would return false.
> 
> Ok, now you have me thinking more strongly that "do nothing" would be the
better option. You say this is very unexpected. If you get here, its for one of
two reasons
> - something got past the blink-side tests so here we are
> - the renderer is compromised and talking smack
> 
> if the blink side had caught it, no window would be opened
> if we could reliable tell the renderer was borked, we'd kill it (but we can't
reliably tell)
> 
> i vote to "do nothing" in this case

It makes sense. We would still need to run the callback though. Maybe we should
reject it then?

[falken, 2015-03-07 04:58:33]

On 2015/03/07 00:13:47, Mounir Lamouri wrote:
> On 2015/03/07 at 00:02:51, michaeln wrote:
> > > Just to make sure it's clear: the CanRequestURL() is not expected to
return
> > > false in normal cases. If a SW tries to open a regular URL, it should just
> work.
> > > Most unusual URLs should be blocked on the Blink side. If I didn't find
> > > view-source:// was not blocked by Blink, I would have killed the renderer
> when
> > > CanRequestURL() would return false.
> > 
> > Ok, now you have me thinking more strongly that "do nothing" would be the
> better option. You say this is very unexpected. If you get here, its for one
of
> two reasons
> > - something got past the blink-side tests so here we are
> > - the renderer is compromised and talking smack
> > 
> > if the blink side had caught it, no window would be opened
> > if we could reliable tell the renderer was borked, we'd kill it (but we
can't
> reliably tell)
> > 
> > i vote to "do nothing" in this case
> 
> It makes sense. We would still need to run the callback though. Maybe we
should
> reject it then?

I guess you could leave the callback alone, it would just mean the openWindow()
promise never gets resolved, right? Seems nicer to the web author to reject the
promise though, and more consistent with the Blink-side checks.

[mlamouri (slow - plz ping), 2015-03-07 19:20:20]

On 2015/03/07 at 04:58:33, falken wrote:
> On 2015/03/07 00:13:47, Mounir Lamouri wrote:
> > On 2015/03/07 at 00:02:51, michaeln wrote:
> > > > Just to make sure it's clear: the CanRequestURL() is not expected to
return
> > > > false in normal cases. If a SW tries to open a regular URL, it should
just
> > work.
> > > > Most unusual URLs should be blocked on the Blink side. If I didn't find
> > > > view-source:// was not blocked by Blink, I would have killed the
renderer
> > when
> > > > CanRequestURL() would return false.
> > > 
> > > Ok, now you have me thinking more strongly that "do nothing" would be the
> > better option. You say this is very unexpected. If you get here, its for one
of
> > two reasons
> > > - something got past the blink-side tests so here we are
> > > - the renderer is compromised and talking smack
> > > 
> > > if the blink side had caught it, no window would be opened
> > > if we could reliable tell the renderer was borked, we'd kill it (but we
can't
> > reliably tell)
> > > 
> > > i vote to "do nothing" in this case
> > 
> > It makes sense. We would still need to run the callback though. Maybe we
should
> > reject it then?
> 
> I guess you could leave the callback alone, it would just mean the
openWindow() promise never gets resolved, right? Seems nicer to the web author
to reject the promise though, and more consistent with the Blink-side checks.

I think not fulfilling the Promise is a no-go. I've update the CL to reject it.
Hopefully, Blink checks will include those edge cases so we will be able to
sammu CanRequestURL() can't return false.

PTAL

[falken, 2015-03-08 12:23:49]

Rejecting sgtm, some comments though.

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1167: // them
accordingly to prevent CanRequestURL() call below to fail on them.
This is kinda inconsistent now... why not just let about: URLs that aren't
about:blank get rejected when they fail CanRequestURL, in the same way
view-source gets rejected?

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1175: // that call will
reject the promise instead of killing the renderer.
nits: "such URLs" reads like "URLs that the process can access", which is the
opposite of what you mean. Overall this comment can be tightened up, something
like: "Reject requests for URLs that the process is not allowed to access. It's
possible to receive such requests since the renderer-side checks are slightly
different. For example, the view-source scheme will not be filtered out by
Blink."

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1206: request_id,
"Something went wrong while trying to open the window"));
nit: Add a period to be consistent with the error message above (and other SW
exception messages try to do this too)

https://codereview.chromium.org/980383004/diff/60001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/980383004/diff/60001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.cc:503:
blink::WebString::fromLatin1(message)));
nit: WebString::fromUTF8() is much more widely used, I'd only use fromLatin1 if
you're converting a hard-coded string or it's really perf sensitive code and
you're absolutely sure you can never have a UTF8 string. That's actually not the
case here: url.spec() can be UTF8.

[mlamouri (slow - plz ping), 2015-03-08 14:34:12]

PTAL

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1167: // them
accordingly to prevent CanRequestURL() call below to fail on them.
On 2015/03/08 at 12:23:49, falken wrote:
> This is kinda inconsistent now... why not just let about: URLs that aren't
about:blank get rejected when they fail CanRequestURL, in the same way
view-source gets rejected?

I've updated the comment to have it match RPHImpl::FilterURL(). I tried to stay
consistent with the checks there which consider about: URLs different. My
understanding is that as much as view-source:// not being filtered out might be
an oversight, about:* URL might be on purpose.

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1167: // them
accordingly to prevent CanRequestURL() call below to fail on them.
On 2015/03/08 at 12:23:49, falken wrote:
> This is kinda inconsistent now... why not just let about: URLs that aren't
about:blank get rejected when they fail CanRequestURL, in the same way
view-source gets rejected?

I've updated the comment to have it match RPHImpl::FilterURL(). I tried to stay
consistent with the checks there which consider about: URLs different. My
understanding is that as much as view-source:// not being filtered out might be
an oversight, about:* URL might be on purpose.

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1175: // that call will
reject the promise instead of killing the renderer.
On 2015/03/08 at 12:23:49, falken wrote:
> nits: "such URLs" reads like "URLs that the process can access", which is the
opposite of what you mean. Overall this comment can be tightened up, something
like: "Reject requests for URLs that the process is not allowed to access. It's
possible to receive such requests since the renderer-side checks are slightly
different. For example, the view-source scheme will not be filtered out by
Blink."

Done.

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1206: request_id,
"Something went wrong while trying to open the window"));
On 2015/03/08 at 12:23:49, falken wrote:
> nit: Add a period to be consistent with the error message above (and other SW
exception messages try to do this too)

Done.

https://codereview.chromium.org/980383004/diff/60001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/980383004/diff/60001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.cc:503:
blink::WebString::fromLatin1(message)));
On 2015/03/08 at 12:23:49, falken wrote:
> nit: WebString::fromUTF8() is much more widely used, I'd only use fromLatin1
if you're converting a hard-coded string or it's really perf sensitive code and
you're absolutely sure you can never have a UTF8 string. That's actually not the
case here: url.spec() can be UTF8.

Done.

[falken, 2015-03-08 15:01:51]

lgtm modulo comments

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1167: // them
accordingly to prevent CanRequestURL() call below to fail on them.
On 2015/03/08 14:34:12, Mounir Lamouri wrote:
> On 2015/03/08 at 12:23:49, falken wrote:
> > This is kinda inconsistent now... why not just let about: URLs that aren't
> about:blank get rejected when they fail CanRequestURL, in the same way
> view-source gets rejected?
> 
> I've updated the comment to have it match RPHImpl::FilterURL(). I tried to
stay
> consistent with the checks there which consider about: URLs different. My
> understanding is that as much as view-source:// not being filtered out might
be
> an oversight, about:* URL might be on purpose.

Ah I see, makes sense.

https://codereview.chromium.org/980383004/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1164: return;
We should kill the renderer if !url.is_valid(). It shouldn't be sending us those
and you'd hit an assert in url.spec().

https://codereview.chromium.org/980383004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1171: //Reject requests
for URLs that the process is not allowed to access. It's
nit: space after //

[mlamouri (slow - plz ping), 2015-03-08 17:53:56]

https://codereview.chromium.org/980383004/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/980383004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1164: return;
On 2015/03/08 at 15:01:50, falken wrote:
> We should kill the renderer if !url.is_valid(). It shouldn't be sending us
those and you'd hit an assert in url.spec().

Very good point. That actually crossed my mind then I forgot ;)

https://codereview.chromium.org/980383004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1171: //Reject requests
for URLs that the process is not allowed to access. It's
On 2015/03/08 at 15:01:50, falken wrote:
> nit: space after //

Done.

[falken, 2015-03-09 03:31:31]

still lgtm, i think Mike West or another security expert should check it again
as it's changed since the last review.

[mlamouri (slow - plz ping), 2015-03-09 10:16:47]

On 2015/03/09 at 03:31:31, falken wrote:
> still lgtm, i think Mike West or another security expert should check it again
as it's changed since the last review.

Yes. I was actually waiting for at least jochen@ to give feedback given that he
seemed to have opinions on the matter.

[jochen (gone - plz use gerrit), 2015-03-09 16:14:52]

lgtm

[mlamouri (slow - plz ping), 2015-03-09 16:17:00]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-03-09 16:17:00]

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/980383004/#ps120001
(title: "review comments")

[commit-bot: I haz the power, 2015-03-09 16:17:13]

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

[commit-bot: I haz the power, 2015-03-09 16:21:02]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-09 16:21:42]

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