[Service Worker Registration] removed protocolIsInHTTPFamily and replaced with SchemeRegistry check

Service Workers require a secure origin, such as HTTPS.  chrome-extension:// pages are not HTTP/HTTPS, but are secure so this change becomes a necessary step to allow extensions to register a Service Worker.

SchemeRegistry files were changed to add registerURLSchemeAsAllowingServiceWorkers and shouldTreatURLSchemeAsAllowingServiceWorkers allowing chrome-extension schemes to register Service Workers.  The registerURLSchemeAs... will take place in chrome_content_renderer_client.cc

WebSecurityPolicy files were changed to follow the pattern of the other registerURLSchemeAs functions in SchemeRegistry.

The ServiceWorkerContainer no longer uses protocolIsIntHTTPFamily and instead uses the new shouldTreatURLSchemeAs... function which for now checks schemes against HTTP, HTTPS, and chrome-extension.

BUG=501569
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198079

[annekao, 2015-06-17 18:58:51]

annekao@google.com changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2015-06-17 22:32:41]

We typically have a BUG=99999 line in the CL description, which allows us to
track our progress.  I've created a bug for this project at crbug.com/501569 (so
you should just add BUG=501569 as the last line in the description).

Also, it looks like you have a typo - protocolIsInt, instead of protocolIsIn. :)

[annekao, 2015-06-18 01:17:34]

On 2015/06/17 22:32:41, Devlin wrote:
> We typically have a BUG=99999 line in the CL description, which allows us to
> track our progress.  I've created a bug for this project at crbug.com/501569
(so
> you should just add BUG=501569 as the last line in the description).
> 
> Also, it looks like you have a typo - protocolIsInt, instead of protocolIsIn.
:)

Updated!

[Devlin, 2015-06-18 15:27:55]

On 2015/06/18 01:17:34, annekao wrote:
> On 2015/06/17 22:32:41, Devlin wrote:
> > We typically have a BUG=99999 line in the CL description, which allows us to
> > track our progress.  I've created a bug for this project at crbug.com/501569
> (so
> > you should just add BUG=501569 as the last line in the description).
> > 
> > Also, it looks like you have a typo - protocolIsInt, instead of
protocolIsIn.
> :)
> 
> Updated!

Looking at this from an outsider's perspective, it might be good to provide a
bit more background in the CL description. Maybe touch on:
- Why is this necessary (chrome-extension:// pages aren't HTTP/HTTPS, so
registration fails)
- Why is this okay (chrome-extension://, and other whitelisted secure origins,
should always be secure like HTTPS, so there's no increased risk)

Otherwise, LG. :)

[Devlin, 2015-06-19 00:00:10]

Don't forget to ping when you update a CL - otherwise we never know. :)  Tiny
nit: to say that this change allows extensions to register a ServiceWorker is
kind of misleading - it's a necessary step in doing so.

LGTM

[annekao, 2015-06-19 19:16:47]

annekao@google.com changed reviewers:
+ jsbell@chromium.org

[annekao, 2015-06-19 19:19:10]

On 2015/06/19 00:00:10, Devlin wrote:
> Don't forget to ping when you update a CL - otherwise we never know. :)  Tiny
> nit: to say that this change allows extensions to register a ServiceWorker is
> kind of misleading - it's a necessary step in doing so.
> 
> LGTM

Updated the description to add clarity! Thanks Devlin :)

[jsbell, 2015-06-19 21:44:50]

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

[jsbell, 2015-06-19 21:48:35]

Deferring to falken@ who may have insights on this, since I haven't been
tracking the spec. The definition of `isSecure()` seems like it may allow other
protocols such as blob: and filesystem: that we did not want to permit at the
time, but perhaps this has been rethought?

I'm watching closely due to my related CL
https://codereview.chromium.org/1177983007/

[Devlin, 2015-06-19 21:56:57]

On 2015/06/19 21:48:35, jsbell wrote:
> Deferring to falken@ who may have insights on this, since I haven't been
> tracking the spec. The definition of `isSecure()` seems like it may allow
other
> protocols such as blob: and filesystem: that we did not want to permit at the
> time, but perhaps this has been rethought?
> 
> I'm watching closely due to my related CL
> https://codereview.chromium.org/1177983007/

Good point, Josh.  It's worth noting that the content layer also does checks for
whether or not the url is allowed, I think, and it would probably be easier to
wire in a chrome-extension scheme there (only because it's one fewer layers). 
But, of course, if need be we can also introduce a new concept of "service
worker allowed schemes" similar to our custom secure origin schemes here.

[falken, 2015-06-22 06:12:46]

On 2015/06/19 21:56:57, Devlin wrote:
> On 2015/06/19 21:48:35, jsbell wrote:
> > Deferring to falken@ who may have insights on this, since I haven't been
> > tracking the spec. The definition of `isSecure()` seems like it may allow
> other
> > protocols such as blob: and filesystem: that we did not want to permit at
the
> > time, but perhaps this has been rethought?
> > 
> > I'm watching closely due to my related CL
> > https://codereview.chromium.org/1177983007/
> 
> Good point, Josh.  It's worth noting that the content layer also does checks
for
> whether or not the url is allowed, I think, and it would probably be easier to
> wire in a chrome-extension scheme there (only because it's one fewer layers). 
> But, of course, if need be we can also introduce a new concept of "service
> worker allowed schemes" similar to our custom secure origin schemes here.

Yes, we added the HTTP check since our system doesn't support blob, file, etc,
which caused crashes:
https://chromium.googlesource.com/chromium/blink/+/7cb98e793aa61f1747b5c7b7e5...

I'd be more comfortable with a check like isHTTPProtocol || isExtension.

I also think no one really knows what would happen today if you do register a SW
from an extension. Do you plan to add a test and/or first make this change
behind a flag?

[Devlin, 2015-06-22 15:22:48]

On 2015/06/22 06:12:46, falken wrote:
> I'd be more comfortable with a check like isHTTPProtocol || isExtension.

"isExtension" creates a layering violation (blink shouldn't know about
extensions), and since we're making this change, we should probably have
something abstract enough to support other custom urls in the future (I wouldn't
be surprised if we wanted webui, like chrome://settings, to be able to use
service workers).  But we can certainly do something like
"shouldAllowServiceWorkers", and have another set in the SchemeRegistry, similar
to what we do for custom secure origins
(SchemeRegistry::shouldTreatURLSchemeAsSecure).  SGTY?

> I also think no one really knows what would happen today if you do register a
SW
> from an extension. Do you plan to add a test and/or first make this change
> behind a flag?

Right now, you can't register service workers for extensions at all - there are
a few different errors, the first of which is that the url isn't supported. 
Since it doesn't currently work, I think it's safe to hold off on putting the
change behind a flag, at least for this part.

[not at google - send to devlin, 2015-06-22 18:11:48]

Drive-by: I sympathise with falken's concern that this is treading in new
territory and who-really-knows-what-will-happen. It would be nice to see a test
which does validate the behavior that "extensions can't register a SW" and
gradually modify that as we iterate to make sure nothing crazy does happen.

[falken, 2015-06-23 03:28:31]

On 2015/06/22 15:22:48, Devlin wrote:
> On 2015/06/22 06:12:46, falken wrote:
> > I'd be more comfortable with a check like isHTTPProtocol || isExtension.
> 
> "isExtension" creates a layering violation (blink shouldn't know about
> extensions), and since we're making this change, we should probably have
> something abstract enough to support other custom urls in the future (I
wouldn't
> be surprised if we wanted webui, like chrome://settings, to be able to use
> service workers).  But we can certainly do something like
> "shouldAllowServiceWorkers", and have another set in the SchemeRegistry,
similar
> to what we do for custom secure origins
> (SchemeRegistry::shouldTreatURLSchemeAsSecure).  SGTY?

I'm not so familiar with SchemeRegistry but if we can do a
shouldAllowSeviceWorkers in a way that's HTTP/HTTPS + extensions and is not a
layering violation it SGTM :)

BTW, I think the patch as written would break
fast/serviceworker/access-container-on-local-file.html which was added in the
commit I cited.

> 
> > I also think no one really knows what would happen today if you do register
a
> SW
> > from an extension. Do you plan to add a test and/or first make this change
> > behind a flag?
> 
> Right now, you can't register service workers for extensions at all - there
are
> a few different errors, the first of which is that the url isn't supported. 
> Since it doesn't currently work, I think it's safe to hold off on putting the
> change behind a flag, at least for this part.

Understood. I still do think a test would be nice.

[annekao, 2015-06-23 17:27:51]

Patchset #2 (id:20001) has been deleted

[annekao, 2015-06-23 17:42:49]

annekao@google.com changed reviewers:
+ brettw@chromium.org

[annekao, 2015-06-23 17:47:04]

Added "shouldAllowServiceWorkers" to SchemeRegistry and used that instead of
"isSecure" in ServiceWorkerContainer.cpp to avoid any crashes that may be caused
by blobs, files, etc.

Schemes added are 'http', 'https', and 'chrome-extension'.

brettw, please take a look at the SchemeRegistry changes.

[Devlin, 2015-06-23 17:52:32]

https://codereview.chromium.org/1191793003/diff/40001/Source/platform/weborig...
File Source/platform/weborigin/SchemeRegistry.cpp (right):

https://codereview.chromium.org/1191793003/diff/40001/Source/platform/weborig...
Source/platform/weborigin/SchemeRegistry.cpp:171:
serviceWorkerSchemes.add("chrome-extension");
This shouldn't be added here (it's a layering violation).  Instead, we should
expose a shouldAllowServiceWorkersOnScheme() method, similar to
shouldTreatURLSchemeAsSecure(), and call into it from the chrome/content layers.

This will mean that we need two patches to accomplish this - this one, and a
followup in the chrome repo.

[annekao, 2015-06-23 18:10:46]

Patchset #3 (id:60001) has been deleted

[annekao, 2015-06-23 20:37:50]

annekao@google.com changed reviewers:
+ dmazzoni@chromium.org

[annekao, 2015-06-23 20:37:51]

Fixed the SchemeRegistry for ServiceWorkers.  Took out the
serviceWorkerSchemes.add("chrome-extension") line from Patch#2 and added a
registerURLSchemeAsAllowServiceWorkers to be used through WebSecurityPolicy to
register 'chrome-extension' in chrome_content_renderer_client.cc (similarly to
the other register functions in the SchemeRegistry)

The changes to chrome_content_renderer_client.cc will on a separate patch.

dmazzoni@, please take a look at the changes in WebSecurityPolicy

Note: I wasn't sure of the best way to implement the SchemeRegistry Service
Worker check in ServiceWorkerContainer, so any suggestions on that are welcome.

[Devlin, 2015-06-23 20:53:47]

Cool!  Just a couple nits, but otherwise lgtm.

https://codereview.chromium.org/1191793003/diff/80001/Source/platform/weborig...
File Source/platform/weborigin/SchemeRegistry.h (right):

https://codereview.chromium.org/1191793003/diff/80001/Source/platform/weborig...
Source/platform/weborigin/SchemeRegistry.h:90: // Schemes that can register a
service worker
nit: Add a period to the comment.

https://codereview.chromium.org/1191793003/diff/80001/Source/platform/weborig...
Source/platform/weborigin/SchemeRegistry.h:91: static void
registerURLSchemeAsAllowServiceWorkers(const String& scheme);
registerURLSchemeAsAllowServiceWorkers sounds a little odd ;) Though it also
matches the rest of the registerURLSchemeAs* format.  Maybe just
s/Allow/Allowing to make this closer normal speech, and match other methods here
(like registerUrlSchemeAsRestringMixedContent)

[brettw, 2015-06-23 21:26:26]

I lack context on this. What feedback are you hoping to get from me?

[falken, 2015-06-24 04:38:30]

Sorry to go back and forth on this, I don't intend to block this CL, just have
some more comments.

We'll have to repeat this exercise in content/. Is content/ able to access
SchemeRegistry? Another approach (which Devlin hinted at before but I didn't
carefully read) is to move the check to content/ and relax the blink check. To
do that without causing renderer crashes, you could:
1) Change content::ServiceWorkerDispatcherHost to not do BadMessageReceived()
when the URL is disallowed, and instead return a polite error (it currently does
BadMessageReceived() because it expects blink to filter out disallowed URLs)
2) Change blink to no longer check for http family and leave it to content do
those checks
3) Change content::ServiceWorkerDispatchHost to allow HTTP + HTTPS +
chrome-extension:// (I'm not sure whether this counts as a layering violation or
you need to expose something implemented in chrome/ like the
ContentBrowserClient checks)

Would that approach work better for you? Apologies if this was your plan from
the beginning, I just saw (2) without (1) it would have caused crashes if landed
in that order.

If you want to stick with this patch, then I'd:
- Update the CL description. Also Blink changes traditionally require a test or
mention "no behavior change" in the description.
- You'd need a web/ OWNER (dmazzoni is an OWNER of a subset of web/, the AXEnum
files)

[falken, 2015-06-24 04:38:31]

Sorry to go back and forth on this, I don't intend to block this CL, just have
some more comments.

We'll have to repeat this exercise in content/. Is content/ able to access
SchemeRegistry? Another approach (which Devlin hinted at before but I didn't
carefully read) is to move the check to content/ and relax the blink check. To
do that without causing renderer crashes, you could:
1) Change content::ServiceWorkerDispatcherHost to not do BadMessageReceived()
when the URL is disallowed, and instead return a polite error (it currently does
BadMessageReceived() because it expects blink to filter out disallowed URLs)
2) Change blink to no longer check for http family and leave it to content do
those checks
3) Change content::ServiceWorkerDispatchHost to allow HTTP + HTTPS +
chrome-extension:// (I'm not sure whether this counts as a layering violation or
you need to expose something implemented in chrome/ like the
ContentBrowserClient checks)

Would that approach work better for you? Apologies if this was your plan from
the beginning, I just saw (2) without (1) it would have caused crashes if landed
in that order.

If you want to stick with this patch, then I'd:
- Update the CL description. Also Blink changes traditionally require a test or
mention "no behavior change" in the description.
- You'd need a web/ OWNER (dmazzoni is an OWNER of a subset of web/, the AXEnum
files)

[Devlin, 2015-06-24 15:46:31]

On 2015/06/24 04:38:31, falken wrote: 
> We'll have to repeat this exercise in content/. Is content/ able to access
> SchemeRegistry?

content/ (and chrome/) can access the SchemeRegistry through the exposed methods
registerURLSchemeAs*.  A good example of this is the
registerURLSchemeAsSecure(), which is used to make chrome-extension://,
chrome-search://, etc secure.  So doing it like this (making a
registerURLSchemeAsAllowingServiceWorkers) is consistent and easy enough (the
chrome/ side patch is a one-liner in
chrome/renderer/chrome_content_renderer_client).

> Another approach (which Devlin hinted at before but I didn't
> carefully read) is to move the check to content/ and relax the blink check. To
> do that without causing renderer crashes, you could:
> 1) Change content::ServiceWorkerDispatcherHost to not do BadMessageReceived()
> when the URL is disallowed, and instead return a polite error (it currently
does
> BadMessageReceived() because it expects blink to filter out disallowed URLs)
> 2) Change blink to no longer check for http family and leave it to content do
> those checks
> 3) Change content::ServiceWorkerDispatchHost to allow HTTP + HTTPS +
> chrome-extension:// (I'm not sure whether this counts as a layering violation
or
> you need to expose something implemented in chrome/ like the
> ContentBrowserClient checks)
> 
> Would that approach work better for you? Apologies if this was your plan from
> the beginning, I just saw (2) without (1) it would have caused crashes if
landed
> in that order.

That is mostly what I was suggesting before, and I think would work.  That said,
it is a little more of a behavior change, so this way might be less invasive. 
This way also seems like it is a little more consistent with how we currently
special-case URL schemes, and lets blink continue to filter out bad URLs (which
is kind of nice, if only to avoid the IPC messages).

> If you want to stick with this patch, then I'd:
> - Update the CL description. Also Blink changes traditionally require a test
or
> mention "no behavior change" in the description.
> - You'd need a web/ OWNER (dmazzoni is an OWNER of a subset of web/, the
AXEnum
> files)

I think I'd have a slight preference overall for sticking with this patch - but
you know better, so up to you. :)  (If we do go with this patch, I think it's
safe to say "no behavior change", since this won't actually add the
chrome-extension scheme - by default, blink should continue filtering out
everything except http[s] urls.  We'll add a test in the chrome/ layer when we
add the extension scheme there.)

[falken, 2015-06-24 15:49:12]

ok LGTM

[annekao, 2015-06-24 17:28:22]

annekao@google.com changed reviewers:
+ pdr@chromium.org
- dmazzoni@chromium.org

[annekao, 2015-06-24 17:34:45]

Just updated the patch with some changes to the function names.  Thanks Devlin
and falkin :)

brettw@, I've made some changes to the SchemeRegistry files, adding a register
and shouldTreat function for schemes that allow service workers.  This is
important in order for chrome-extensions to register them.  Before, the check in
ServiceWorkerContainer only allowed HTTP/HTTPS urls.  Now, I've replaced it with
the shouldTreat function in SchemeRegistry.

pdr@, please take a look at the changes in the WebSecurityPolicy files.  I've
added a registerURLSchemeAsAllowingServiceWorkers function to follow the pattern
that similar functions had.  This will end up being used in the chrome content
renderer client.

[brettw, 2015-06-24 17:47:31]

I don't actually know anything about the service worker security model. It
doesn't seem obviously wrong, but I'm reluctant to say say anything given that
you guys probably know more than me. If you can't find anybody qualified, I
might be able to help you find somebody, although I would probably just ask
Dimitri who's not in today.

[not at google - send to devlin, 2015-06-24 17:53:15]

On 2015/06/24 17:47:31, brettw wrote:
> I don't actually know anything about the service worker security model. It
> doesn't seem obviously wrong, but I'm reluctant to say say anything given that
> you guys probably know more than me. If you can't find anybody qualified, I
> might be able to help you find somebody, although I would probably just ask
> Dimitri who's not in today.

falken@ is from the SW team - does your lg cover this question?

[annekao, 2015-06-24 19:47:20]

annekao@google.com changed reviewers:
+ dcheng@chromium.org
- pdr@chromium.org

[annekao, 2015-06-24 19:48:41]

Hi dcheng@, please take a look at the changes in the WebSecurityPolicy files. 
I've
added a registerURLSchemeAsAllowingServiceWorkers function to follow the pattern
that similar functions had.  This will end up being used in the chrome content
renderer client to add chrome-extension as a scheme for service worker
registration.

[dcheng, 2015-06-24 20:20:15]

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
File Source/platform/weborigin/SchemeRegistry.cpp (right):

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
Source/platform/weborigin/SchemeRegistry.cpp:169:
serviceWorkerSchemes.add("http");
Why is http in this list?

[palmer, 2015-06-24 20:26:01]

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

[palmer, 2015-06-24 20:26:03]

As the commit message says, SWs require secure origins. HTTP is not. :)

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
File Source/platform/weborigin/SchemeRegistry.cpp (right):

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
Source/platform/weborigin/SchemeRegistry.cpp:169:
serviceWorkerSchemes.add("http");
> Why is http in this list?

It must not be.

[annekao, 2015-06-24 20:30:36]

On 2015/06/24 20:20:15, dcheng wrote:
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> File Source/platform/weborigin/SchemeRegistry.cpp (right):
> 
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> Source/platform/weborigin/SchemeRegistry.cpp:169:
> serviceWorkerSchemes.add("http");
> Why is http in this list?

Being that it originally checked for HTTP or HTTPS with protocolIsInHTTPFamily I
tried to keep it the same.  However, I agree it makes more sense to remove HTTP
from the list.

[Devlin, 2015-06-24 20:38:44]

On 2015/06/24 20:30:36, annekao wrote:
> On 2015/06/24 20:20:15, dcheng wrote:
> >
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > File Source/platform/weborigin/SchemeRegistry.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > Source/platform/weborigin/SchemeRegistry.cpp:169:
> > serviceWorkerSchemes.add("http");
> > Why is http in this list?


As Anne points out, this was sticking with original behavior as much as
possible, but it should probably be changed.  (The more thorough checks are done
in the content/ layer.)

Anne, this might mean you need to add/change some of the blink tests, since it
is now a behavior change.

[kinuko, 2015-06-25 05:25:19]

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

[kinuko, 2015-06-25 05:26:35]

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

[kinuko, 2015-06-25 05:26:36]

Um, I feel I come a bit late. I'm not fully sure the current design is right
yet.  For SchemeRegistry changes I recommend we ask @mkwst to take a look at.
(Mike, would you?)

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
File Source/platform/weborigin/SchemeRegistry.h (right):

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
Source/platform/weborigin/SchemeRegistry.h:92: static bool
shouldTreatURLSchemeAsAllowingServiceWorkers(const String& scheme);
Having an API specific list added here slightly feels like a layering violation.
 Are these really only for ServiceWorker?  Or do we want to consider
chrome-extension as potentially trustworthy scheme for powerful new features?

https://www.chromium.org/Home/chromium-security/prefer-secure-origins-for-pow...

[falken, 2015-06-25 05:45:01]

On 2015/06/24 20:38:44, Devlin wrote:
> On 2015/06/24 20:30:36, annekao wrote:
> > On 2015/06/24 20:20:15, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > > File Source/platform/weborigin/SchemeRegistry.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > > Source/platform/weborigin/SchemeRegistry.cpp:169:
> > > serviceWorkerSchemes.add("http");
> > > Why is http in this list?
> 
> 
> As Anne points out, this was sticking with original behavior as much as
> possible, but it should probably be changed.  (The more thorough checks are
done
> in the content/ layer.)
> 
> Anne, this might mean you need to add/change some of the blink tests, since it
> is now a behavior change.

Hey, I don't think this a behavior change. The ToT code does the secure origin
check in executionContext->isPrivilegedContext. That's the
"https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged"
check. Later on, we do the HTTP/HTTPS family check, which further filters out
blob/file/chrome-extension/etc. This patch is just adding chrome-extension to
the HTTP/HTTPS check.

HTTP is allowed for http://localhost. http://example.com would have been already
disallowed by isPrivilegedContext.

[Devlin, 2015-06-25 15:26:16]

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
File Source/platform/weborigin/SchemeRegistry.h (right):

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
Source/platform/weborigin/SchemeRegistry.h:92: static bool
shouldTreatURLSchemeAsAllowingServiceWorkers(const String& scheme);
On 2015/06/25 05:26:36, kinuko wrote:
> Having an API specific list added here slightly feels like a layering
violation.
>  Are these really only for ServiceWorker?  Or do we want to consider
> chrome-extension as potentially trustworthy scheme for powerful new features?
> 
>
https://www.chromium.org/Home/chromium-security/prefer-secure-origins-for-pow...

The original version of this patch leaned more towards that approach - checking
only that the origin was secure (SecurityOrigin::isSecure()), which
chrome-extension is already included in - but falken@ mentioned that that would
include other secure origins that we didn't want to allow service workers on
(e.g., blob, file, etc).

[Devlin, 2015-06-25 15:46:49]

All, I think we need to be careful, because we might be at the point of "too
many cooks" here.  There are a lot of different approaches, many of which will
work (especially since, IIUC, content/ performs additional checks on the host,
and this is merely a first filter - which is the original code didn't even
filter out http).  I think someone has to own this review, since asking n
different people often leads to n different approaches. :)

Mike, would you mind looking over this and lending us your opinion?  After that,
I'd like to try and get this through sooner rather than later. :)

[not at google - send to devlin, 2015-06-25 15:47:56]

On 2015/06/23 21:26:26, brettw wrote:
> I lack context on this. What feedback are you hoping to get from me?

I believe you're on this review as one of the very few OWNERs of SchemeRegistry:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Mike West, 2015-06-25 15:49:37]

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
File Source/platform/weborigin/SchemeRegistry.h (right):

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
Source/platform/weborigin/SchemeRegistry.h:92: static bool
shouldTreatURLSchemeAsAllowingServiceWorkers(const String& scheme);
On 2015/06/25 at 15:26:15, Devlin wrote:
> On 2015/06/25 05:26:36, kinuko wrote:
> > Having an API specific list added here slightly feels like a layering
violation.
> >  Are these really only for ServiceWorker?  Or do we want to consider
> > chrome-extension as potentially trustworthy scheme for powerful new
features?
> > 
> >
https://www.chromium.org/Home/chromium-security/prefer-secure-origins-for-pow...
> 
> The original version of this patch leaned more towards that approach -
checking only that the origin was secure (SecurityOrigin::isSecure()), which
chrome-extension is already included in - but falken@ mentioned that that would
include other secure origins that we didn't want to allow service workers on
(e.g., blob, file, etc).

Does this map to whether or not a scheme is CORS-enabled? It seems like it does.

[Devlin, 2015-06-25 16:19:32]

Talked with Mike a bit offline, and got a few suggestions:
- Use isSecure() +  CORS-enabled to determine if we can register a SW
- Use a new set of schemes for service worker-allowed schemes (Patch Set 4)

I think I lean towards the latter, since the former will include the data
scheme, which isn't allowed as a service worker scheme - and it seems that it
could more easily lead to some funny edge cases if we ever add new cors-enabled
schemes that we don't want SWs on.

Re the http in the scheme registry, I think we need that for http://localhost,
which *is* supposed to work for service workers.  Since the content/ layer
performs additional checks, and since the ToT code allows HTTP origins (at least
for the parts we're changing in this patch), I think we should leave it in the
SW-allowed schemes, though we can add a comment as to why, and note that it's
not all HTTP checks.

Re testing, most of this behavior should be tested already, since it doesn't
actually change any behavior.  We can add one more test in
ServiceWorkerContainerTest.cpp that checks that adding another scheme to the
SchemeRegistry allows service worker registration, or we could do this in a more
end-to-end manner from the chrome side.  It seems like having the former can't
hurt, assuming no obstacles.

Mike, SGTY?

[brettw, 2015-06-25 19:28:10]

On Thu, Jun 25, 2015 at 8:47 AM, <kalman@chromium.org> wrote:

> On 2015/06/23 21:26:26, brettw wrote:
>
>> I lack context on this. What feedback are you hoping to get from me?
>>
>
> I believe you're on this review as one of the very few OWNERs of
> SchemeRegistry:
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


Ah. I'm there because I'm the "URL guy" but I'm definitely not the
"security origin guy" I'll remove myself from the review as I don't think
I'm qualified for this decision.

Thanks,
Brett

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[brettw, 2015-06-25 19:28:23]

brettw@chromium.org changed reviewers:
- brettw@chromium.org

[michaeln, 2015-06-25 23:03:16]

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

[michaeln, 2015-06-25 23:03:19]

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
File Source/platform/weborigin/SchemeRegistry.cpp (right):

https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
Source/platform/weborigin/SchemeRegistry.cpp:169:
serviceWorkerSchemes.add("http");
On 2015/06/24 20:26:02, palmer wrote:
> > Why is http in this list?
> 
> It must not be.

Isn't http://localhost/ secure and fine-n-good to use serviceworkers?

[dcheng, 2015-06-25 23:10:26]

On 2015/06/25 at 23:03:19, michaeln wrote:
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> File Source/platform/weborigin/SchemeRegistry.cpp (right):
> 
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> Source/platform/weborigin/SchemeRegistry.cpp:169:
serviceWorkerSchemes.add("http");
> On 2015/06/24 20:26:02, palmer wrote:
> > > Why is http in this list?
> > 
> > It must not be.
> 
> Isn't http://localhost/ secure and fine-n-good to use serviceworkers?

I understand why this is in the scheme list now, but still find it confusing.
Access checks appear to be somewhat scattered in many different layers, making
it hard (for me) to figure out what is and what is not allowed. Obviously,
someone who's familiar with this code might not have the same issues, but code
should be written for the reader.

[Devlin, 2015-06-25 23:27:56]

On 2015/06/25 23:10:26, dcheng wrote:
> On 2015/06/25 at 23:03:19, michaeln wrote:
> >
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > File Source/platform/weborigin/SchemeRegistry.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > Source/platform/weborigin/SchemeRegistry.cpp:169:
> serviceWorkerSchemes.add("http");
> > On 2015/06/24 20:26:02, palmer wrote:
> > > > Why is http in this list?
> > > 
> > > It must not be.
> > 
> > Isn't http://localhost/ secure and fine-n-good to use serviceworkers?
> 
> I understand why this is in the scheme list now, but still find it confusing.
> Access checks appear to be somewhat scattered in many different layers, making
> it hard (for me) to figure out what is and what is not allowed. Obviously,
> someone who's familiar with this code might not have the same issues, but code
> should be written for the reader.

I agree; it is rather confusing to have checks in so many places.  But I'd also
be inclined to say that this CL shouldn't be gated on a refactor to coalesce
them all together.  (And if/when that happens, it should probably be done by
someone more familiar with the code than Anne or myself. :))  IMO, the added
part does not reduce readability or add to confusion (since a method called
treatSchemeAsAllowingServiceWorkers seems reasonably self-explanatory).

[falken, 2015-06-26 01:27:05]

On 2015/06/25 23:27:56, Devlin wrote:
> On 2015/06/25 23:10:26, dcheng wrote:
> > On 2015/06/25 at 23:03:19, michaeln wrote:
> > >
> >
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > > File Source/platform/weborigin/SchemeRegistry.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > > Source/platform/weborigin/SchemeRegistry.cpp:169:
> > serviceWorkerSchemes.add("http");
> > > On 2015/06/24 20:26:02, palmer wrote:
> > > > > Why is http in this list?
> > > > 
> > > > It must not be.
> > > 
> > > Isn't http://localhost/ secure and fine-n-good to use serviceworkers?
> > 
> > I understand why this is in the scheme list now, but still find it
confusing.
> > Access checks appear to be somewhat scattered in many different layers,
making
> > it hard (for me) to figure out what is and what is not allowed. Obviously,
> > someone who's familiar with this code might not have the same issues, but
code
> > should be written for the reader.
> 
> I agree; it is rather confusing to have checks in so many places.  But I'd
also
> be inclined to say that this CL shouldn't be gated on a refactor to coalesce
> them all together.  (And if/when that happens, it should probably be done by
> someone more familiar with the code than Anne or myself. :))  IMO, the added
> part does not reduce readability or add to confusion (since a method called
> treatSchemeAsAllowingServiceWorkers seems reasonably self-explanatory).

The ToT code would have benefited from comments like:

In Blink:
// Restrict to secure origins:
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged
if (!executionContext->isPrivilegedContext())
  return;
// Restrict to HTTP(S) as blob/file/etc is not supported by SW. HTTP is allowed
for http://localhost
if (!isHttpFamily())
  return;

In //content:
// Blink is expected to filter out disallowed URLs, so kill the renderer.
if (!CanRegisterServiceWorker())
  BadMessageReceived();

[dcheng, 2015-06-26 06:51:35]

On 2015/06/26 at 01:27:05, falken wrote:
> On 2015/06/25 23:27:56, Devlin wrote:
> > On 2015/06/25 23:10:26, dcheng wrote:
> > > On 2015/06/25 at 23:03:19, michaeln wrote:
> > > >
> > >
> >
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > > > File Source/platform/weborigin/SchemeRegistry.cpp (right):
> > > > 
> > > >
> > >
> >
https://codereview.chromium.org/1191793003/diff/100001/Source/platform/webori...
> > > > Source/platform/weborigin/SchemeRegistry.cpp:169:
> > > serviceWorkerSchemes.add("http");
> > > > On 2015/06/24 20:26:02, palmer wrote:
> > > > > > Why is http in this list?
> > > > > 
> > > > > It must not be.
> > > > 
> > > > Isn't http://localhost/ secure and fine-n-good to use serviceworkers?
> > > 
> > > I understand why this is in the scheme list now, but still find it
confusing.
> > > Access checks appear to be somewhat scattered in many different layers,
making
> > > it hard (for me) to figure out what is and what is not allowed. Obviously,
> > > someone who's familiar with this code might not have the same issues, but
code
> > > should be written for the reader.
> > 
> > I agree; it is rather confusing to have checks in so many places.  But I'd
also
> > be inclined to say that this CL shouldn't be gated on a refactor to coalesce
> > them all together.  (And if/when that happens, it should probably be done by
> > someone more familiar with the code than Anne or myself. :))  IMO, the added
> > part does not reduce readability or add to confusion (since a method called
> > treatSchemeAsAllowingServiceWorkers seems reasonably self-explanatory).
> 
> The ToT code would have benefited from comments like:
> 
> In Blink:
> // Restrict to secure origins:
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged
> if (!executionContext->isPrivilegedContext())
>   return;
> // Restrict to HTTP(S) as blob/file/etc is not supported by SW. HTTP is
allowed for http://localhost
> if (!isHttpFamily())
>   return;
> 
> In //content:
> // Blink is expected to filter out disallowed URLs, so kill the renderer.
> if (!CanRegisterServiceWorker())
>   BadMessageReceived();

OK, thanks for explaining why "http" in this list is required, and why we have
the additional protocol check. It would be nice to refactor some of the checks
(the first check in registerServiceWorker and the ones in getRegistrations) to
be moved into a common helper to make this code a bit more readable/easier to
document.

mkwst@, mind doing the final review for public/web and Source/web? You're much
more familiar with the intricacies of this area than I am.

[Mike West, 2015-06-29 08:01:32]

Assuming that we actually want to expose service workers to extensions, this
seems like a totally reasonable way of doing that. LGTM to build out this
mechanism for piping the scheme addition down into Blink.

It might be reasonable to gate the scheme's addition on the Chrome side on
chrome://flags/#extension-apis while you hammer out the rest of the plan.

[annekao, 2015-06-29 17:07:15]

On 2015/06/29 08:01:32, Mike West wrote:
> Assuming that we actually want to expose service workers to extensions, this
> seems like a totally reasonable way of doing that. LGTM to build out this
> mechanism for piping the scheme addition down into Blink.
> 
> It might be reasonable to gate the scheme's addition on the Chrome side on
> chrome://flags/#extension-apis while you hammer out the rest of the plan.

Alright! I'm going to go ahead and commit this latest patch.  If time allows, we
can go back through and clean up some of the blink checks. For now, this is
closest to the current behavior.

[dcheng, 2015-06-29 17:11:54]

On 2015/06/29 at 17:07:15, annekao wrote:
> On 2015/06/29 08:01:32, Mike West wrote:
> > Assuming that we actually want to expose service workers to extensions, this
> > seems like a totally reasonable way of doing that. LGTM to build out this
> > mechanism for piping the scheme addition down into Blink.
> > 
> > It might be reasonable to gate the scheme's addition on the Chrome side on
> > chrome://flags/#extension-apis while you hammer out the rest of the plan.
> 
> Alright! I'm going to go ahead and commit this latest patch.  If time allows,
we can go back through and clean up some of the blink checks. For now, this is
closest to the current behavior.

Please add some comments before landing this (a comment that mentions that we
check the context is privileged before checking the scheme, the scheme checking
is to filter out things like blob URLs, etc, and therefore http is required
because http://localhost is considered secure).

[annekao, 2015-06-29 19:45:28]

On 2015/06/29 17:11:54, dcheng wrote:
> On 2015/06/29 at 17:07:15, annekao wrote:
> > On 2015/06/29 08:01:32, Mike West wrote:
> > > Assuming that we actually want to expose service workers to extensions,
this
> > > seems like a totally reasonable way of doing that. LGTM to build out this
> > > mechanism for piping the scheme addition down into Blink.
> > > 
> > > It might be reasonable to gate the scheme's addition on the Chrome side on
> > > chrome://flags/#extension-apis while you hammer out the rest of the plan.
> > 
> > Alright! I'm going to go ahead and commit this latest patch.  If time
allows,
> we can go back through and clean up some of the blink checks. For now, this is
> closest to the current behavior.
> 
> Please add some comments before landing this (a comment that mentions that we
> check the context is privileged before checking the scheme, the scheme
checking
> is to filter out things like blob URLs, etc, and therefore http is required
> because http://localhost is considered secure).

OK, I added comments in ServiceWorkerContainer and the SchemeRegistry.cpp file.

[Devlin, 2015-06-29 19:54:44]

https://codereview.chromium.org/1191793003/diff/120001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/1191793003/diff/120001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:217: if
(!executionContext->isPrivilegedContext(errorMessage)) {
so let's say (courtesy of falken@)
// Restrict to secure origins:
https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged

https://codereview.chromium.org/1191793003/diff/120001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:223: // There is a
check that a context is privileged before checking the scheme.
nit: It's better to comment exactly where the relevant code is, so that when
someone updates the code, they are more likely to update the comment, as well. 
So we can remove this comment in favor of the one you already have in scheme
registry and the one that should go above.

[annekao, 2015-06-29 22:35:38]

Patchset #6 (id:140001) has been deleted

[annekao, 2015-06-30 16:33:45]

The CQ bit was checked by annekao@google.com

[annekao, 2015-06-30 16:33:45]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
rdevlin.cronin@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1191793003/#ps160001
(title: "Comments")

[commit-bot: I haz the power, 2015-06-30 16:34:07]

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

[commit-bot: I haz the power, 2015-06-30 17:55:59]

Committed patchset #6 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198079