Use [SecureContext] for navigator.storage

Use [SecureContext] for navigator.storage

StorageManager (navigator.storage) is specified [1] as having
[SecureContext] on the interface. Replace per-method checks
with [SecureContext] on the navigator.storage attribute so
it simply doesn't exist in non-secure contexts as intended.

(The spec has [SecureContext] on a non-partial interface and
the interface is implemented by Navigator. We don't support
core interfaces implementing module interfaces, so we use a
partial interface instead. [SecureContext] does not yet
work when defined on the partial interface itself.)

[1] https://storage.spec.whatwg.org/#api

BUG=690720, 634270
Review-Url: https://codereview.chromium.org/2692633002
Cr-Commit-Position: refs/heads/master@{#454073}
Committed: https://chromium.googlesource.com/chromium/src/+/b1b9e6d3db16818dfd5b76199136f8745f005144

[jsbell, 2017-02-10 23:32:53]

Description was changed from

==========
Use [SecureContext] for navigator.storage

StorageManager (navigator.storage) is specified [1] as having
[SecureContext] on the interface. Replace per-method checks
with [SecureContext] on the navigator.storage attribute so
it simply doesn't exist in non-secure contexts as intended.

TODO: The spec has [SecureContext] on a non-partial interface and
the interface is implemented by Navigator. We don't support
core interfaces implementing module interfaces, so we use a
partial interface instead. Does [SecureContext] work when
defined on the partial interface itself?

[1] https://storage.spec.whatwg.org/#api

BUG=690720,634270
==========

to

==========
Use [SecureContext] for navigator.storage

StorageManager (navigator.storage) is specified [1] as having
[SecureContext] on the interface. Replace per-method checks
with [SecureContext] on the navigator.storage attribute so
it simply doesn't exist in non-secure contexts as intended.

(The spec has [SecureContext] on a non-partial interface and
the interface is implemented by Navigator. We don't support
core interfaces implementing module interfaces, so we use a
partial interface instead. [SecureContext] does not yet
work when defined on the partial interface itself.)

[1] https://storage.spec.whatwg.org/#api

BUG=690720,634270
==========

[jsbell, 2017-02-10 23:33:09]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 23:34:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2017-02-11 01:01:43]

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

[jsbell, 2017-02-11 01:01:44]

mkwst@ - what do you think about this change?

And more specifically...

https://codereview.chromium.org/2692633002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/2692633002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/quota/StorageManager.cpp:65: // TODO(jsbell):
Is the isUnique() check covered by [SecureContext]? If so,
I'm keeping this TODO but maybe mkwst@ has an answer? 

Tracing through the code, the isPotentiallyTrustworthy() check would allow
unique origins that have had setUniqueOriginIsPotentiallyTrustworthy() called,
which apparently happens sometimes (e.g. Document::initSecurityContext). If I'm
reading the code (and [HTML]) correctly, you can get a secure context for a
sandboxed iframe.

The Storage spec says nothing about unique origins, but they make no sense as
far as storage-related APIs are concerned. Our existing storage APIs block them
as this code does. Maybe we want another attribute to restrict this from being
used in unique origins?

[commit-bot: I haz the power, 2017-02-11 01:16:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 01:16:20]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-02-14 12:38:31]

LGTM.

On 2017/02/11 at 01:01:44, jsbell wrote:
> mkwst@ - what do you think about this change?
> 
> And more specifically...
> 
>
https://codereview.chromium.org/2692633002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/quota/StorageManager.cpp (right):
> 
>
https://codereview.chromium.org/2692633002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/quota/StorageManager.cpp:65: //
TODO(jsbell): Is the isUnique() check covered by [SecureContext]? If so,
> I'm keeping this TODO but maybe mkwst@ has an answer? 
> 
> Tracing through the code, the isPotentiallyTrustworthy() check would allow
unique origins that have had setUniqueOriginIsPotentiallyTrustworthy() called,
which apparently happens sometimes (e.g. Document::initSecurityContext). If I'm
reading the code (and [HTML]) correctly, you can get a secure context for a
sandboxed iframe.

Yes. We consider a frame "secure" if it was transported securely. There's a
metric floating around to see if we can get away with changing that (by adding a
new sandbox flag:
https://w3c.github.io/webappsec-secure-contexts/#sandboxed-secure-browsing-co...),
but I need to check up on it.

Basically, `[SecureContext]` should handle things for you and you shouldn't have
to care about it.

> The Storage spec says nothing about unique origins, but they make no sense as
far as storage-related APIs are concerned. Our existing storage APIs block them
as this code does. Maybe we want another attribute to restrict this from being
used in unique origins?

Our existing code doesn't hide the `localStorage` interface in a sandbox, it
throws. Whether or not that's a good idea, I don't know, but it matches the spec
(see step 2 of
https://html.spec.whatwg.org/#storage-object-initialisation-steps).

I'd suggest filing a bug against the Storage spec to see if carrying over that
restriction directly makes sense, or whether matching `[SecureContext]` makes
sense.

[Mike West, 2017-02-14 12:39:25]

https://codereview.chromium.org/2692633002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/durable-storage-on-insecure-origin.html
(right):

https://codereview.chromium.org/2692633002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/durable-storage-on-insecure-origin.html:13:
}, 'Requires secure context');
Nit: Are there Web Platform Tests for this feature? If so, could this test live
there? :)

[jsbell, 2017-02-14 18:33:56]

On 2017/02/14 12:38:31, Mike West (sloooooow) wrote:
> I'd suggest filing a bug against the Storage spec to see if carrying over that
> restriction directly makes sense, or whether matching `[SecureContext]` makes
> sense.

Done: https://github.com/whatwg/storage/issues/41

[jsbell, 2017-02-14 18:34:47]

https://codereview.chromium.org/2692633002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/durable-storage-on-insecure-origin.html
(right):

https://codereview.chromium.org/2692633002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/durable-storage-on-insecure-origin.html:13:
}, 'Requires secure context');
On 2017/02/14 12:39:25, Mike West (sloooooow) wrote:
> Nit: Are there Web Platform Tests for this feature? If so, could this test
live
> there? :)

We should add a suite/migrate our tests; I'll do that as a follow-up.

[jsbell, 2017-02-14 18:35:28]

> > Nit: Are there Web Platform Tests for this feature? If so, could this test
> live
> > there? :)
> 
> We should add a suite/migrate our tests; I'll do that as a follow-up.

To clarify: no, not currently. We should add an idlharness test and the rest.

[jsbell, 2017-02-14 18:36:51]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2017-02-14 18:37:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 18:37:22]

This CL has an open dependency (Issue 2687263002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[jsbell, 2017-02-14 18:38:17]

I'll also add the unique opaque origin test in a follow-on, to address those
TODOs.

[jsbell, 2017-02-14 18:38:22]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2017-02-14 18:39:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 18:39:12]

This CL has an open dependency (Issue 2687263002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[jsbell, 2017-02-14 22:46:29]

On 2017/02/14 18:39:12, commit-bot: I haz the power wrote:
> This CL has an open dependency (Issue 2687263002 Patch 1). Please resolve the
> dependency and try again.

Dangit. :)

[Mike West, 2017-02-15 06:36:38]

On 2017/02/14 at 18:35:28, jsbell wrote:
> > > Nit: Are there Web Platform Tests for this feature? If so, could this test
> > live
> > > there? :)
> > 
> > We should add a suite/migrate our tests; I'll do that as a follow-up.
> 
> To clarify: no, not currently. We should add an idlharness test and the rest.

SGTM!

[jsbell, 2017-03-01 18:22:39]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2017-03-01 18:22:39]

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/2692633002/#ps20001
(title: "TypeError and rebased tests")

[commit-bot: I haz the power, 2017-03-01 18:22:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 22:31:40]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488392559508060,
"parent_rev": "e8d21447d207616130a7d68e4b7b98c8d9fdca9c", "commit_rev":
"b1b9e6d3db16818dfd5b76199136f8745f005144"}

[commit-bot: I haz the power, 2017-03-01 22:34:07]

Description was changed from

==========
Use [SecureContext] for navigator.storage

StorageManager (navigator.storage) is specified [1] as having
[SecureContext] on the interface. Replace per-method checks
with [SecureContext] on the navigator.storage attribute so
it simply doesn't exist in non-secure contexts as intended.

(The spec has [SecureContext] on a non-partial interface and
the interface is implemented by Navigator. We don't support
core interfaces implementing module interfaces, so we use a
partial interface instead. [SecureContext] does not yet
work when defined on the partial interface itself.)

[1] https://storage.spec.whatwg.org/#api

BUG=690720,634270
==========

to

==========
Use [SecureContext] for navigator.storage

StorageManager (navigator.storage) is specified [1] as having
[SecureContext] on the interface. Replace per-method checks
with [SecureContext] on the navigator.storage attribute so
it simply doesn't exist in non-secure contexts as intended.

(The spec has [SecureContext] on a non-partial interface and
the interface is implemented by Navigator. We don't support
core interfaces implementing module interfaces, so we use a
partial interface instead. [SecureContext] does not yet
work when defined on the partial interface itself.)

[1] https://storage.spec.whatwg.org/#api

BUG=690720,634270

Review-Url: https://codereview.chromium.org/2692633002
Cr-Commit-Position: refs/heads/master@{#454073}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1b9e6d3db16818dfd5b76199136...
==========

[commit-bot: I haz the power, 2017-03-01 22:34:13]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b1b9e6d3db16818dfd5b76199136...