Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(114)

Issue 618113005: Kill renderers that dink with Service Workers from non-secure origins. (Closed)

Created:
6 years, 2 months ago by dominicc (has gone to gerrit)
Modified:
6 years, 2 months ago
Reviewers:
falken, michaeln
CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Kill renderers that dink with Service Workers from non-secure origins. The API use is checked on the Blink side, so these checks are to thwart corrupted renderers. Specifically, renderers that try to register, unregister or get registrations for non-secure origins must be nuked from orbit. It's the only way to be sure. BUG=394213 TEST=content_unittests ServiceWorkerDispatcherHostTest.* Committed: https://crrev.com/67fac000875442072230a0e1033879c19a038000 Cr-Commit-Position: refs/heads/master@{#297851}

Patch Set 1 #

Total comments: 1

Patch Set 2 : Remove copy-pasted provider_hosts. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -22 lines) Patch
M content/browser/service_worker/service_worker_dispatcher_host.cc View 2 chunks +19 lines, -7 lines 1 comment Download
M content/browser/service_worker/service_worker_dispatcher_host_unittest.cc View 1 6 chunks +71 lines, -15 lines 0 comments Download

Messages

Total messages: 16 (5 generated)
dominicc (has gone to gerrit)
PTAL Bot redness appears unrelated--speex GYP target problem?
6 years, 2 months ago (2014-10-02 05:53:03 UTC) #2
falken
lgtm https://codereview.chromium.org/618113005/diff/1/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/618113005/diff/1/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc#newcode132 content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:132: base::WeakPtr<ServiceWorkerProviderHost> provider_host = host->AsWeakPtr(); Not from your patch, ...
6 years, 2 months ago (2014-10-02 06:24:00 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/618113005/20001
6 years, 2 months ago (2014-10-02 07:38:50 UTC) #5
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_rel/builds/20938) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_dbg/builds/4313)
6 years, 2 months ago (2014-10-02 07:46:09 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/618113005/20001
6 years, 2 months ago (2014-10-02 15:46:13 UTC) #9
commit-bot: I haz the power
Committed patchset #2 (id:20001) as deb1fc6a0ffef9daacea8f3c73cbb8802bed3284
6 years, 2 months ago (2014-10-02 16:48:44 UTC) #10
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/67fac000875442072230a0e1033879c19a038000 Cr-Commit-Position: refs/heads/master@{#297851}
6 years, 2 months ago (2014-10-02 16:49:17 UTC) #11
michaeln
https://codereview.chromium.org/618113005/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/618113005/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode49 content/browser/service_worker/service_worker_dispatcher_host.cc:49: return url.SchemeIsSecure() || net::IsLocalhost(url.host()); This allows filesystem: urls and ...
6 years, 2 months ago (2014-10-02 23:00:25 UTC) #13
dominicc (has gone to gerrit)
On 2014/10/02 23:00:25, michaeln wrote: > https://codereview.chromium.org/618113005/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > https://codereview.chromium.org/618113005/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode49 > ...
6 years, 2 months ago (2014-10-02 23:48:31 UTC) #14
michaeln1
Its not pessimism. The file, filesystem, and wss schemes won't work, that's primarily why I ...
6 years, 2 months ago (2014-10-03 22:48:23 UTC) #15
dominicc (has gone to gerrit)
6 years, 2 months ago (2014-10-05 23:50:51 UTC) #16
I'm more persuaded that filesystem URLs should not work because filesystem
does not look likely to be implemented by another vendor. What you're
saying about inner URLs is too vague for me to critically evaluate.

file:// not returning 2xx and not working is persuasive.

So that leaves wss. Is it possible to load a main resource from wss?

On Fri, Oct 3, 2014 at 3:48 PM, <michaeln@google.com> wrote:

> Its not pessimism. The file, filesystem, and wss schemes won't work, that's
> primarily why I think they should be excluded.
> * interception hooks won't catch those protocols
> * file urls don't produce the http status codes the spec is written in
> terms of
> and we've coded to
> * filesystem urls are somewhat funkified with 'innerurls', they won't come
> for
> free
> * fyi, looks like the code you committed already excludes file urls
>
> Is there a use case for supporting serviceworkers on filesystem urls? Is
> anybody
> asking for that? This is my the secondmost reason to exclude them, I think
> there
> is not.
>
> We can deliberately add support for other schemes when the need arises. I
> wonder
> if it would make sense to support chrome-extension urls at some point?
>
>
> https://codereview.chromium.org/618113005/
>

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

Powered by Google App Engine
This is Rietveld 408576698