Throw a SecurityError when navigator.serviceWorker is accessed in a sandboxed iframe.

Throw a SecurityError when navigator.serviceWorker is accessed in a sandboxed iframe.

BUG=486308
TEST=./blink/tools/run_layout_tests.sh http/tests/serviceworker/chromium/sandboxed-iframe-navigator-serviceworker.html

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

[horo, 2015-06-23 07:55:30]

Patchset #1 (id:1) has been deleted

[horo, 2015-06-23 07:59:28]

Patchset #1 (id:20001) has been deleted

[horo, 2015-06-23 07:59:58]

horo@chromium.org changed reviewers:
+ nhiroki@chromium.org

[horo, 2015-06-23 07:59:59]

nhiroki@
Could you please review this?

[nhiroki, 2015-06-23 08:28:14]

> Throw a SecurityError when navigator.serviceWorker is accessed in a sandboxed
iframe.

Question) I couldn't find a consensus about this behavior in chromium/spec
issues. Where is this defined?

[horo, 2015-06-23 10:05:49]

On 2015/06/23 08:28:14, nhiroki wrote:
> > Throw a SecurityError when navigator.serviceWorker is accessed in a
sandboxed
> iframe.
> 
> Question) I couldn't find a consensus about this behavior in chromium/spec
> issues. Where is this defined?

There is no clarified definition about this behavior yet.
But there seems to be a consensus that "Sandboxed iframes without
allow-same-origin shouldn't be intercepted".
I think we should not wait for the spec to be settled and keep this security
bypasses bug.
And it is reasonable to make the same behavior as "window.caches" does.

[nhiroki, 2015-06-24 02:47:11]

On 2015/06/23 10:05:49, horo wrote:
> On 2015/06/23 08:28:14, nhiroki wrote:
> > > Throw a SecurityError when navigator.serviceWorker is accessed in a
> sandboxed
> > iframe.
> > 
> > Question) I couldn't find a consensus about this behavior in chromium/spec
> > issues. Where is this defined?
> 
> There is no clarified definition about this behavior yet.
> But there seems to be a consensus that "Sandboxed iframes without
> allow-same-origin shouldn't be intercepted".
> I think we should not wait for the spec to be settled and keep this security
> bypasses bug.
> And it is reasonable to make the same behavior as "window.caches" does.

Sounds reasonable. Thank you for the explanation.

[nhiroki, 2015-06-24 02:48:17]

Looks pretty good.

https://codereview.chromium.org/1199183002/diff/40001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-navigator-serviceworker.html
(right):

https://codereview.chromium.org/1199183002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-navigator-serviceworker.html:37:
async_test(function(t) {
promise_test?

https://codereview.chromium.org/1199183002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-navigator-serviceworker.html:85:
}, 'Accessing navigator.serviceWorker in sandboxed iframe with allow-same-origin
flag should not throw.');
nit: Can you wrap this at 80 column?

https://codereview.chromium.org/1199183002/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/1199183002/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:40: if
(navigator.frame() &&
navigator.frame()->securityContext()->securityOrigin()->canAccessServiceWorkers())
{
Is this necessary? Aren't checks in NavigatorServiceWorker::serviceWorker
sufficient?

[horo, 2015-06-24 03:39:20]

https://codereview.chromium.org/1199183002/diff/40001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-navigator-serviceworker.html
(right):

https://codereview.chromium.org/1199183002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-navigator-serviceworker.html:37:
async_test(function(t) {
On 2015/06/24 02:48:17, nhiroki wrote:
> promise_test?

Done.

https://codereview.chromium.org/1199183002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/sandboxed-iframe-navigator-serviceworker.html:85:
}, 'Accessing navigator.serviceWorker in sandboxed iframe with allow-same-origin
flag should not throw.');
On 2015/06/24 02:48:17, nhiroki wrote:
> nit: Can you wrap this at 80 column?

Done.

https://codereview.chromium.org/1199183002/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/1199183002/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:40: if
(navigator.frame() &&
navigator.frame()->securityContext()->securityOrigin()->canAccessServiceWorkers())
{
On 2015/06/24 02:48:17, nhiroki wrote:
> Is this necessary? Aren't checks in NavigatorServiceWorker::serviceWorker
> sufficient?

We need this check.
Otherwise ASSERT_NOT_REACHED() in
NonThrowableExceptionState::throwSecurityError() kills the process.

[nhiroki, 2015-06-24 04:38:26]

lgtm

[falken, 2015-06-24 04:40:43]

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

[falken, 2015-06-24 04:40:44]

https://codereview.chromium.org/1199183002/diff/60001/Source/modules/servicew...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/1199183002/diff/60001/Source/modules/servicew...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:69:
exceptionState.throwSecurityError("Access to service worker is denied.");
Can we make this message more detailed? A developer seeing "Access to service
worker is denied." is likely to not understand how to fix the problem.

[nhiroki, 2015-06-24 04:46:38]

Sorry, one more nit. Can you add TEST= line in the CL description?

[horo, 2015-06-24 05:58:31]

https://codereview.chromium.org/1199183002/diff/60001/Source/modules/servicew...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/1199183002/diff/60001/Source/modules/servicew...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:69:
exceptionState.throwSecurityError("Access to service worker is denied.");
On 2015/06/24 04:40:44, falken wrote:
> Can we make this message more detailed? A developer seeing "Access to service
> worker is denied." is likely to not understand how to fix the problem.

Done.

[horo, 2015-06-24 05:59:14]

horo@chromium.org changed reviewers:
+ tkent@chromium.org

[horo, 2015-06-24 05:59:15]

tkent@
Could you please review Source/platform/weborigin/SecurityOrigin.h?

[tkent, 2015-06-24 06:22:17]

lgtm

https://codereview.chromium.org/1199183002/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/1199183002/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:41:
NonThrowableExceptionState exceptionState;
probably |supplement->serviceWorker(ASSERT_NO_EXCEPTION);| is simpler.

[horo, 2015-06-24 06:31:02]

Thank you!!

https://codereview.chromium.org/1199183002/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/1199183002/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:41:
NonThrowableExceptionState exceptionState;
On 2015/06/24 06:22:17, tkent wrote:
> probably |supplement->serviceWorker(ASSERT_NO_EXCEPTION);| is simpler.

Done.

[horo, 2015-06-24 06:32:10]

The CQ bit was checked by horo@chromium.org

[horo, 2015-06-24 06:32:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1199183002/#ps100001
(title: "use ASSERT_NO_EXCEPTION")

[commit-bot: I haz the power, 2015-06-24 06:32:15]

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

[commit-bot: I haz the power, 2015-06-24 07:45:30]

Committed patchset #4 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197711