Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 2071433003: Reland: service worker: Don't control a subframe of an insecure context (Closed)

Created:
2 years, 11 months ago by falken
Modified:
2 years, 11 months ago
CC:
chromium-reviews, kenjibaheux+watch_chromium.org, tzik, nasko+codewatch_chromium.org, eae+blinkwatch, kinuko+watch, rwlbuis, extensions-reviews_chromium.org, jam, blink-reviews-dom_chromium.org, dglazkov+blink, darin-cc_chromium.org, blink-reviews, chromium-apps-reviews_chromium.org, blink-reviews-api_chromium.org, blink-worker-reviews_chromium.org, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, jsbell+serviceworker_chromium.org, sof, nhiroki, michaeln, mlamouri+watch-blink_chromium.org, serviceworker-reviews, kinuko+serviceworker, mkwst+moarreviews-renderer_chromium.org, horo+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland: service worker: Don't control a subframe of an insecure context We must check isSecureContext when creating the network provider to adhere to https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-privileged. We already did this for getRegistration(), register(), unregister() but must also do this when deciding whether to control an in-scope document. BUG=607543 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation TBR=reviewers from the original review Original review: https://codereview.chromium.org/2009453002 Committed: https://crrev.com/ad1850962644e19cdb040d60eb236e0ebc23c243 Cr-Commit-Position: refs/heads/master@{#400093}

Patch Set 1 : original patch #

Patch Set 2 : revised #

Unified diffs Side-by-side diffs Delta from patch set Stats (+558 lines, -148 lines) Patch
M chrome/browser/chrome_content_browser_client.h View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/extensions/service_worker_apitest.cc View 3 chunks +20 lines, -3 lines 0 comments Download
M content/browser/service_worker/service_worker_browsertest.cc View 1 chunk +6 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_context_core.cc View 1 chunk +7 lines, -7 lines 0 comments Download
M content/browser/service_worker/service_worker_context_request_handler_unittest.cc View 1 chunk +6 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_context_unittest.cc View 1 chunk +12 lines, -4 lines 0 comments Download
M content/browser/service_worker/service_worker_controllee_request_handler.cc View 2 chunks +12 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc View 8 chunks +13 lines, -12 lines 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host.h View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host.cc View 3 chunks +12 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host_unittest.cc View 5 chunks +13 lines, -7 lines 0 comments Download
M content/browser/service_worker/service_worker_handle_unittest.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/service_worker/service_worker_job_unittest.cc View 1 chunk +6 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_provider_host.h View 1 6 chunks +28 lines, -2 lines 0 comments Download
M content/browser/service_worker/service_worker_provider_host.cc View 1 8 chunks +25 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_provider_host_unittest.cc View 1 4 chunks +70 lines, -14 lines 0 comments Download
M content/browser/service_worker/service_worker_registration.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_request_handler_unittest.cc View 1 chunk +9 lines, -9 lines 0 comments Download
M content/browser/service_worker/service_worker_storage_unittest.cc View 3 chunks +3 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_url_request_job_unittest.cc View 13 chunks +17 lines, -20 lines 0 comments Download
M content/browser/service_worker/service_worker_version_unittest.cc View 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc View 1 chunk +5 lines, -3 lines 0 comments Download
M content/child/service_worker/service_worker_network_provider.h View 2 chunks +11 lines, -4 lines 0 comments Download
M content/child/service_worker/service_worker_network_provider.cc View 5 chunks +25 lines, -11 lines 0 comments Download
M content/common/service_worker/service_worker_messages.h View 1 chunk +13 lines, -2 lines 0 comments Download
M content/public/browser/content_browser_client.h View 1 chunk +6 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M content/renderer/service_worker/service_worker_context_client.cc View 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/shared_worker/embedded_shared_worker_stub.cc View 1 chunk +2 lines, -1 line 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/serviceworker/insecure-parent-frame.html View 1 1 chunk +85 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/insecure-inscope.html View 1 1 chunk +12 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/insecure-parent.html View 1 1 chunk +16 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 4 chunks +10 lines, -21 lines 0 comments Download
M third_party/WebKit/Source/core/frame/Frame.h View 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/Frame.cpp View 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrame.cpp View 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp View 1 chunk +58 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrame.h View 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (8 generated)
falken
kinuko: Would you be able to review this reland? The diff from the previous patch ...
2 years, 11 months ago (2016-06-16 02:38:43 UTC) #3
kinuko
lgtm.
2 years, 11 months ago (2016-06-16 03:33:59 UTC) #4
falken
TBRing owners from previous review
2 years, 11 months ago (2016-06-16 04:25:44 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2071433003/20001
2 years, 11 months ago (2016-06-16 04:27:03 UTC) #10
commit-bot: I haz the power
Committed patchset #2 (id:20001)
2 years, 11 months ago (2016-06-16 06:10:20 UTC) #12
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/ad1850962644e19cdb040d60eb236e0ebc23c243 Cr-Commit-Position: refs/heads/master@{#400093}
2 years, 11 months ago (2016-06-16 06:12:07 UTC) #14
dcheng
Sorry, I didn't see this CL till now. I thought about it a bit more ...
2 years, 11 months ago (2016-06-16 08:59:44 UTC) #15
falken
On 2016/06/16 08:59:44, dcheng wrote: > Sorry, I didn't see this CL till now. > ...
2 years, 11 months ago (2016-06-17 00:52:34 UTC) #16
dcheng
On 2016/06/17 00:52:34, falken wrote: > On 2016/06/16 08:59:44, dcheng wrote: > > Sorry, I ...
2 years, 11 months ago (2016-06-17 06:49:24 UTC) #17
alexmos
2 years, 11 months ago (2016-06-17 06:52:19 UTC) #18
Message was sent while issue was closed.
On 2016/06/17 00:52:34, falken wrote:
> On 2016/06/16 08:59:44, dcheng wrote:
> > Sorry, I didn't see this CL till now.
> > 
> > I thought about it a bit more the last few days, and I don't think we should
> add
> > the Blink public API for this. It means we can't share the code, but I think
> > that's fine: this is a one-off thing for service workers, as far as I can
> tell.
> > If it becomes useful for other things, then we can revisit exposing it in
the
> > public API.
> 
> I don't have a strong opinion (the early patchsets on the previous review did
> that) but jww@ and alexmos@ expressed a strong preference for sharing the
code.
> 
> I'm loath to revert this now since I want it to pass through canary and get it
> merged ASAP.
> 
> I'll fire up a patch to change the public API part and see how it looks.

FWIW, I supported sharing the code before we knew that we couldn't merge the
duplicate calls in public API, but had the same concern as Daniel for that
(https://codereview.chromium.org/2009453002/diff/120001/third_party/WebKit/pub...),
so I'm ok with following Daniel's suggestion in a followup CL.

Powered by Google App Engine
This is Rietveld 408576698