Implement 'window.isSecureContext'.

Implement 'window.isSecureContext'.

The "Secure Contexts" spec defines a feature-detection mechanism for
secure contexts on the Window and WorkerGlobalScope objects[1]. This
patch implements the former. The latter will be in a subsequent patch,
as the logic to deal with Shared Workers is a bit more complicated.

It also renames the internal check from 'isPrivilegedContext' to
'isSecureContext', in line with the specification. That is a purely
mechanical change, using the following commands:

> git grep -n -l -e "isPrivilegedContext" | xargs -L1 sed -i '' -e 's/isPrivilegedContext/isSecureContext/g'
> git grep -n -l -e "PrivilegeContextCheck" | xargs -L1 sed -i '' -e 's/PrivilegeContextCheck/SecureContextCheck/g'
> git grep -n -l -e "StandardPrivilegeCheck" | xargs -L1 sed -i '' -e 's/StandardPrivilegeCheck/StandardSecureContextCheck/g'
> git grep -n -l -e "WebCryptoPrivilegeCheck" | xargs -L1 sed -i '' -e 's/WebCryptoPrivilegeCheck/WebCryptoSecureContextCheck/g'

[1]: https://w3c.github.io/webappsec/specs/powerfulfeatures/#monkey-patching-global-object

BUG=534788
Committed: https://crrev.com/fd67eed6357b2a5487c13cc159b4e843a9c91aee
Cr-Commit-Position: refs/heads/master@{#351290}

[Mike West, 2015-09-28 10:22:58]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, philipj@opera.com

[Mike West, 2015-09-28 10:22:59]

Philip, would you mind taking a look at the layout tests?

Jochen, would you mind stamping the content/ change?

You both are supremely qualified to review the small code change in
Source/core/frame/DOMWindow.{cpp,h,idl}, and the mechanical rename across the
remaining files.

Thanks!

-mike

[jochen (gone - plz use gerrit), 2015-09-28 11:37:22]

content/ lgtm

[philipj_slow, 2015-09-28 12:17:56]

Tests lgtm % title nit.

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/resources/get-host-info.js
(right):

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/get-host-info.js:23:
AUTHENTICATED_ORIGIN: 'https://' + UNAUTHENTICATED_HOST + ':' + HTTPS_PORT
This now looks a bit weird, is there a better name for 'example.test'?

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated.html
(right):

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated.html:4:
<title>Unauthenticated origin is insecure</title>
The first actual assertion amounts to "authenticated origin is secure". It looks
like this is testing top-level secure contexts, as well as insecure and insecure
contexts in iframes. A more descriptive title would be nice, and if that's too
hard perhaps the test can be split?

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/unauthenticated.html
(right):

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/unauthenticated.html:4:
<title>Unauthenticated origin is insecure</title>
I guess the other title was copied from here :)

[Mike West, 2015-09-28 13:27:24]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-09-28 13:27:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1373773003/#ps40001
(title: "Feedback.")

[commit-bot: I haz the power, 2015-09-28 13:27:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373773003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373773003/40001

[Mike West, 2015-09-28 13:28:02]

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/resources/get-host-info.js
(right):

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/get-host-info.js:23:
AUTHENTICATED_ORIGIN: 'https://' + UNAUTHENTICATED_HOST + ':' + HTTPS_PORT
On 2015/09/28 at 12:17:56, philipj wrote:
> This now looks a bit weird, is there a better name for 'example.test'?

I don't know why we're using `example.test`, honestly. We ought to be using
something that WPT supports. I'll add a TODO and change the name.

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated.html
(right):

https://codereview.chromium.org/1373773003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated.html:4:
<title>Unauthenticated origin is insecure</title>
On 2015/09/28 at 12:17:56, philipj wrote:
> The first actual assertion amounts to "authenticated origin is secure". It
looks like this is testing top-level secure contexts, as well as insecure and
insecure contexts in iframes. A more descriptive title would be nice, and if
that's too hard perhaps the test can be split?

Clarified the test name and title. Thanks!

[philipj_slow, 2015-09-28 13:33:44]

I didn't review anything except the tests, has someone looked at the actual
implementation yet?

[Mike West, 2015-09-28 13:34:35]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2015-09-28 13:35:18]

On 2015/09/28 at 13:33:44, philipj wrote:
> I didn't review anything except the tests, has someone looked at the actual
implementation yet?

Canceling the CQ. Sorry, I didn't see the "tests" qualifier on your comment.

I'd certainly appreciate it if one of the two of you could look at the
implementation as well. :)

[Mike West, 2015-09-28 13:36:17]

On 2015/09/28 at 13:35:18, Mike West (slow) wrote:
> On 2015/09/28 at 13:33:44, philipj wrote:
> > I didn't review anything except the tests, has someone looked at the actual
implementation yet?
> 
> Canceling the CQ. Sorry, I didn't see the "tests" qualifier on your comment.
> 
> I'd certainly appreciate it if one of the two of you could look at the
implementation as well. :)

(Again, third_party/WebKit/Source/core/frame/DOMWindow.* are the only
non-mechanical, non-test changes.)

[Srirama, 2015-09-28 14:42:06]

srirama.m@samsung.com changed reviewers:
+ srirama.m@samsung.com

[Srirama, 2015-09-28 14:42:07]

https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:161: if (!frame() ||
!document())
do we need the document check here? I think it is not required as it is
returning the reference and nowhere in this file there is a check for document.
Even if it is needed, atleast it should come before the frame check, as inside
frame we are using document.

[philipj_slow, 2015-09-28 15:03:46]

On 2015/09/28 13:35:18, Mike West (slow) wrote:
> On 2015/09/28 at 13:33:44, philipj wrote:
> > I didn't review anything except the tests, has someone looked at the actual
> implementation yet?
> 
> Canceling the CQ. Sorry, I didn't see the "tests" qualifier on your comment.
> 
> I'd certainly appreciate it if one of the two of you could look at the
> implementation as well. :)

Ah, and I missed your "You both are supremely qualified to review the small code
change" bit. I'll take a look now.

[philipj_slow, 2015-09-28 15:20:26]

Everything LGTM, but I agree with Srirama that the document() check is a bit
odd, it makes it not entirely obvious if there may be internal calls to
RemoteDOMWindow::document() that are supposed to silently return false due to
the frame() check or something.

The IDL thing can be left until later, just thinking out loud.

https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:161: if (!frame() ||
!document())
On 2015/09/28 14:42:07, Srirama wrote:
> do we need the document check here? I think it is not required as it is
> returning the reference and nowhere in this file there is a check for
document.
> Even if it is needed, atleast it should come before the frame check, as inside
> frame we are using document.

Right, RemoteDOMWindow::document() can return null but then we'll also have
passed an ASSERT_NOT_REACHED().

On the other hand, the only other plain document() call I can see first does an
isLocalDOMWindow() check, but to go that route it would be more reasonable,
IMHO, to just make this virtual.

https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:189: readonly attribute boolean
isSecureContext;
Why not a partial interface like in the spec, do you expect this to be merged
into HTML so that it's no longer a partial interface?

If it's going to stay a separate spec, an interface implemented by both Window
and WorkerGlobalScope like https://html.spec.whatwg.org/#atob might be nice.

[Mike West, 2015-09-28 16:52:53]

On 2015/09/28 at 15:20:26, philipj wrote:
>
https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/DOMWindow.cpp:161: if (!frame() ||
!document())
> On 2015/09/28 14:42:07, Srirama wrote:
> > do we need the document check here? I think it is not required as it is
> > returning the reference and nowhere in this file there is a check for
document.
> > Even if it is needed, atleast it should come before the frame check, as
inside
> > frame we are using document.
> 
> Right, RemoteDOMWindow::document() can return null but then we'll also have
passed an ASSERT_NOT_REACHED().
> 
> On the other hand, the only other plain document() call I can see first does
an isLocalDOMWindow() check, but to go that route it would be more reasonable,
IMHO, to just make this virtual.

The frame check is the important bit. I think I can drop the `document()` check
entirely, actually. Changing to an `isLocalDOMWindow` might be the right thing
to do, but I'll have to check to see what that actually means in the context of
the `property-access-on-cached-window-*` tests.

>
https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/Window.idl (right):
> 
>
https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Window.idl:189: readonly attribute
boolean isSecureContext;
> Why not a partial interface like in the spec, do you expect this to be merged
into HTML so that it's no longer a partial interface?
> 
> If it's going to stay a separate spec, an interface implemented by both Window
and WorkerGlobalScope like https://html.spec.whatwg.org/#atob might be nice.

There are at least three different ways of doing this between different things
in different specs. I'm happy to rewrite it however you like, if you formulate
the thing you like as a response to
https://lists.w3.org/Archives/Public/public-webappsec/2015Sep/0192.html :)

[philipj_slow, 2015-09-29 09:22:15]

On 2015/09/28 16:52:53, Mike West (slow) wrote:
> On 2015/09/28 at 15:20:26, philipj wrote:
> >
>
https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/DOMWindow.cpp:161: if (!frame() ||
> !document())
> > On 2015/09/28 14:42:07, Srirama wrote:
> > > do we need the document check here? I think it is not required as it is
> > > returning the reference and nowhere in this file there is a check for
> document.
> > > Even if it is needed, atleast it should come before the frame check, as
> inside
> > > frame we are using document.
> > 
> > Right, RemoteDOMWindow::document() can return null but then we'll also have
> passed an ASSERT_NOT_REACHED().
> > 
> > On the other hand, the only other plain document() call I can see first does
> an isLocalDOMWindow() check, but to go that route it would be more reasonable,
> IMHO, to just make this virtual.
> 
> The frame check is the important bit. I think I can drop the `document()`
check
> entirely, actually. Changing to an `isLocalDOMWindow` might be the right thing
> to do, but I'll have to check to see what that actually means in the context
of
> the `property-access-on-cached-window-*` tests.

OK, new code LGTM.

>
https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/frame/Window.idl (right):
> > 
> >
>
https://codereview.chromium.org/1373773003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/Window.idl:189: readonly attribute
> boolean isSecureContext;
> > Why not a partial interface like in the spec, do you expect this to be
merged
> into HTML so that it's no longer a partial interface?
> > 
> > If it's going to stay a separate spec, an interface implemented by both
Window
> and WorkerGlobalScope like https://html.spec.whatwg.org/#atob might be nice.
> 
> There are at least three different ways of doing this between different things
> in different specs. I'm happy to rewrite it however you like, if you formulate
> the thing you like as a response to
> https://lists.w3.org/Archives/Public/public-webappsec/2015Sep/0192.html :)

I actually don't have much of an opinion of "part of HTML's Window" vs. "partial
interface Window" vs. "Window implements SecureContextThing", I was just
wondering if it would work to express it with the same IDL as in the spec.

As for the references situation, it's too bad if you're required by process to
use some mix of obsolete and actively maintained documents, instead of just
referring to the most upstream version of every spec. I'm happy to stay far away
from that discussion, though :)

[Mike West, 2015-09-29 09:30:01]

On 2015/09/29 at 09:22:15, philipj wrote:
> I actually don't have much of an opinion of "part of HTML's Window" vs.
"partial interface Window" vs. "Window implements SecureContextThing", I was
just wondering if it would work to express it with the same IDL as in the spec.

I'd like to land it as-is, but I'm happy to come back to clean it up if you have
strong feeling about a change. :)

> As for the references situation, it's too bad if you're required by process to
use some mix of obsolete and actively maintained documents, instead of just
referring to the most upstream version of every spec. I'm happy to stay far away
from that discussion, though :)

Please, please, please don't get me started. It's depressing.

[Mike West, 2015-09-29 09:30:05]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-09-29 09:30:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1373773003/#ps60001
(title: "document.")

[commit-bot: I haz the power, 2015-09-29 09:30:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373773003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373773003/60001

[philipj_slow, 2015-09-29 09:45:26]

On 2015/09/29 09:30:01, Mike West (slow) wrote:
> On 2015/09/29 at 09:22:15, philipj wrote:
> > I actually don't have much of an opinion of "part of HTML's Window" vs.
> "partial interface Window" vs. "Window implements SecureContextThing", I was
> just wondering if it would work to express it with the same IDL as in the
spec.
> 
> I'd like to land it as-is, but I'm happy to come back to clean it up if you
have
> strong feeling about a change. :)

Not really, fine to land as-is, maybe when it's exposed on Workers it'll be
obvious which way to do it.

[commit-bot: I haz the power, 2015-09-29 10:02:17]

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

[commit-bot: I haz the power, 2015-09-29 10:02:18]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Mike West, 2015-09-29 10:27:16]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-09-29 10:27:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373773003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373773003/60001

[commit-bot: I haz the power, 2015-09-29 10:48:35]

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

[commit-bot: I haz the power, 2015-09-29 10:48:36]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Mike West, 2015-09-29 11:19:36]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-09-29 11:20:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373773003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373773003/60001

[commit-bot: I haz the power, 2015-09-29 12:05:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-29 12:06:36]

Patchset 4 (id:??) landed as
https://crrev.com/fd67eed6357b2a5487c13cc159b4e843a9c91aee
Cr-Commit-Position: refs/heads/master@{#351290}