1269052 - Implement WorkerGlobalScope.isSecureContext

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P2)

Description

[Jonathan Watt [:jwatt]]

Now that bug 1162772 (Implement Window.isSecureContext) is fixed we can leverage that code to implement WorkerGlobalScope.isSecureContext.

Comment 1

[Jonathan Watt [:jwatt]]

For future reference: if [SecureContext] doesn't appear to work for interfaces in Workers, check CreateInterfaceObjects in the generated *Bindings.cpp files to see if sMethods is being used when !NS_IsMainThread(). (sMethods contains the PrefableDisablers object.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Implement isSecureContext for worker scopes

Ben, would you mind reviewing?

I still need to write a serviceworker test for this.  Need to figure out what the right way to test that is.

I also still need to write a test for the case of a sharedworker shared across contexts with different security status...  I did test it manually, I believe.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Actually, maybe window.open() works to open a non-https thing from an https page even with mixed content blocking.  I'll give that a shot...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Implement isSecureContext for worker scopes

Still no serviceworker test, but the sharedworker bits are tested now...

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8794445 [details] [diff] [review]
Implement isSecureContext for worker scopes

Review of attachment 8794445 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/secure-contexts/basic-dedicated-worker.html
@@ +75,5 @@
> +      }
> +
> +      var ifr = document.createElement("iframe");
> +      ifr.src = https_dir + "support/https-subframe-dedicated.html";
> +      document.body.appendChild(ifr);                                

extra spaces here.

::: testing/web-platform/tests/secure-contexts/basic-dedicated-worker.https.html
@@ +75,5 @@
> +      }
> +
> +      var ifr = document.createElement("iframe");
> +      ifr.src = https_dir + "support/https-subframe-dedicated.html";
> +      document.body.appendChild(ifr);                                

here as well.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8794445 [details] [diff] [review]
Implement isSecureContext for worker scopes

Review of attachment 8794445 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good.  Thanks for adding WPT tests!  r+ with comments addressed.  I also wouldn't object to looking at it again depending on what changes.  I'll leave my review queue open.

::: dom/webidl/WorkerGlobalScope.webidl
@@ +41,5 @@
>  readonly attribute CacheStorage caches;
>  };
>  
> +// https://w3c.github.io/webappsec-secure-contexts/#monkey-patching-global-object
> +partial interface WorkerGlobalScope {

Is there a reason you didn't use the `WindowOrWorkerGlobalScope` mixin like the spec?  The html spec has been updated to include it AFAICT.

::: dom/workers/RuntimeService.cpp
@@ +2463,5 @@
> +    if (workerPrivate->IsSecureContext() !=
> +        JS_GetIsSecureContext(js::GetContextCompartment(aCx))) {
> +      // We can't mutate the secure context state of our shared worker, and
> +      // shouldn't per spec.  Spec says to throw a SecurityError.
> +      return NS_ERROR_DOM_SECURITY_ERR;

I think we should implement the async error event immediately instead of synchronously throwing.  Code should already be listening for async error events to handle things like script load failures, etc.

::: dom/workers/WorkerPrivate.cpp
@@ +31,5 @@
>  #include "nsIWeakReferenceUtils.h"
>  #include "nsIWorkerDebugger.h"
>  #include "nsIXPConnect.h"
>  #include "nsPIDOMWindow.h"
> +#include "nsGlobalWindow.h"

I think including nsGlobalWindow.h out of dom/base should be a red flag.  (Although I know its done in other places...)

@@ +2206,5 @@
>      aParent->CopyJSSettings(mJSSettings);
>  
> +    // And manually set our mIsSecureContext, though it's not really relevant to
> +    // dedicated workers...
> +    mIsSecureContext = aParent->IsSecureContext();

MOZ_ASSERT_IF(mIsChromeWorker, mIsSecureContext);

@@ +2225,5 @@
> +      // Shared and dedicated workers both inherit the loading window's secure
> +      // context state.  Shared workers then prevent windows with a different
> +      // secure context state from attaching to them.
> +      mIsSecureContext =
> +        nsGlobalWindow::Cast(mLoadInfo.mWindow)->IsSecureContext();

Seems like IsSecureContext() should be exposed on nsPIDOMWindow, not nsGlobalWindow.  If we're going to have this interface, lets use it.

@@ +2229,5 @@
> +        nsGlobalWindow::Cast(mLoadInfo.mWindow)->IsSecureContext();
> +    } else {
> +      // XXXbz Are all cases like this chrome stuff like JSMs and hence secure
> +      // contexts?  I _think_ they are...
> +      mIsSecureContext = true;

Should we maybe have an `if (mIsChromeWorker) { mIsSecureContext = true; }` case before the window check?  Then you can assert MOZ_ASSERT_UNREACHABLE() in this final else case to ensure we either have a window or its a chrome worker.

::: dom/workers/WorkerPrivate.h
@@ +210,5 @@
> +   *
> +   * It's a bit unfortunate that we have to have an out-of-band boolean for
> +   * this, but we need access to this state from the parent thread, and we can't
> +   * use our global object's secure state there.
> +   */

nit: This file uses // comments for block comments, not /* */.

::: dom/workers/WorkerScope.cpp
@@ +176,5 @@
> +bool
> +WorkerGlobalScope::IsSecureContext() const
> +{
> +  return
> +    JS_GetIsSecureContext(js::GetObjectCompartment(GetWrapperPreserveColor()));

MOZ_ASSERT() that mIsSecureContext matches?

::: testing/web-platform/tests/secure-contexts/basic-shared-worker.html
@@ +12,5 @@
> +    <script>
> +      var t1 = async_test("Shared worker");
> +      var t2 = async_test("Nested worker in shared worker");
> +      var t3 = async_test("Shared worker from https subframe");
> +      var t4 = async_test("Nested worker from shared worker from https subframe");

I don't think any browser implements it yet, but in theory SharedWorker can be nested under other workers as well.  Maybe add a comment that those tests would go here?

::: testing/web-platform/tests/secure-contexts/shared-worker-insecure-first.https.html
@@ +12,5 @@
> +    <script>
> +      var t1 = async_test("Shared worker in subframe");
> +      var t2 = async_test("Nested worker in shared worker in subframe");
> +      var t3 = async_test("Shared worker in popup");
> +      var t4 = async_test("Nested worker from shared worker in popup");

Can you add some comments about how this test works?  I think the way the test conditions intermix below is a bit confusing.

@@ +50,5 @@
> +          } else {
> +            t2.step(function() {
> +              assert_true(data.exception);
> +            });
> +            t2.done();

For example we have t4 and t2 in this nested case.  Having a roadmap would help.

@@ +82,5 @@
> +          document.body.appendChild(ifr);
> +        }
> +      }
> +
> +      Math.sin();

What dark magic is this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is there a reason you didn't use the `WindowOrWorkerGlobalScope` mixin

Mostly that we don't have it yet and I was trying to scope down the changes.  I can add it in general; it's probably not that big a deal.  Will need re-review.

> I think we should implement the async error event immediately 

This part will _certainly_ need re-review.  ;)  OK, will do, if we're sure that's where the spec is going to end up.

> I think including nsGlobalWindow.h out of dom/base should be a red flag

Maybe.  The nsPIDOMWindow thing is a bit silly in some ways; it's mostly useful as a tool to reduce the set of things included, since nsGlobalWindow.h includes all sorts of stuff.

> Seems like IsSecureContext() should be exposed on nsPIDOMWindow

We could do that, at the price of making it virtual and hence a tiny bit bigger and slower when called from both here and the binding code.  I guess it's not fatal if we do that.

> Then you can assert MOZ_ASSERT_UNREACHABLE() in this final else case to
> ensure we either have a window or its a chrome worker.

Is it actually unreachable?  mIsChromeWorker is true if the worker was created with |new ChromeWorker|.  What if a JSM does |new Worker|, though?  This is the part the needinfo is for...

> MOZ_ASSERT() that mIsSecureContext matches?

You mean mWorkerPrivate->IsSecureContext()?  I can do that, sure.

> What dark magic is this?

Left over from debugging the test.  I'll take it out.  I'll apply the rest of the comments.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> > I think we should implement the async error event immediately 
> 
> This part will _certainly_ need re-review.  ;)  OK, will do, if we're sure
> that's where the spec is going to end up.

I mean, this is my opinion.  Obviously if others disagree I can be persuaded.  But since I think no one else implements this for shared workers yet it seems we could lead a bit here.

I'm partly thinking of the compat issues we had with pdfjs's worker when we moved some worker script load errors from being sync to async.  Obviously some script load errors have to be async, but we previously threw an exception if the initial URL is cross-origin.  If we had made all errors async from the start this would not have been a problem.  (Obviously, pdfjs was bugged in that it only looked for exception and ignored async errors completely.)

> > I think including nsGlobalWindow.h out of dom/base should be a red flag
> 
> Maybe.  The nsPIDOMWindow thing is a bit silly in some ways; it's mostly
> useful as a tool to reduce the set of things included, since
> nsGlobalWindow.h includes all sorts of stuff.

I think there is some benefit to separating public API from private implementation.  Not just for avoiding clobber builds when touching a header, but for helping people find the right thing to use.

I guess this is partly me trying to make sense of nsPIDOMWindow and nsGlobalWindow.  Previously I didn't understand the separation very well and when something should go in one place or another.  After working on the timer thing, though, this rationale is where I've been leaning.

Another way to separate public and private would be to make the public API in nsGlobalWindow.h, but move all the implementation details into an anonymous inner class in nsGlobalWindow.cpp.  That seems like a big change, though.

> > Then you can assert MOZ_ASSERT_UNREACHABLE() in this final else case to
> > ensure we either have a window or its a chrome worker.
> 
> Is it actually unreachable?  mIsChromeWorker is true if the worker was
> created with |new ChromeWorker|.  What if a JSM does |new Worker|, though? 
> This is the part the needinfo is for...

Hmm, maybe s/mIsChromeWorker/IsSystemPrincipal()/g then?  Workers have to be same-origin in general, but I have no idea how `new Worker()` in a chrome script differs from `new ChromeWorker()`.  That seems... weird.  I can try to look tomorrow.

> > MOZ_ASSERT() that mIsSecureContext matches?
> 
> You mean mWorkerPrivate->IsSecureContext()?  I can do that, sure.

Yea, just something to try to ensure the js settings and the external flag stay in sync.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

So it appears you are correct we can do `new Worker()` in a jsm.  In that case we should hit this:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4215

Which means `UsesSystemPrincipal()` should be true:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.h#609

So I think we can make the code look like:

  if (UsesSystemPrincipal() || IsServiceWorker()) {
    mIsSecureContext = true;
  } else if (mLoadInfo.mWindow) {
    mIsSecureContext = mLoadInfo.mWindow->IsSecureContext();
  } else {
    MOZ_ASSERT_UNREACHABLE("non-chrome worker that is not a service worker without a parent");
  }

And then in the parent case we can do:

  MOZ_ASSERT_IF(UsesSystemPrincipal(), mIsSecureContext);

Does that all sound reasonable?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, that sounds reasonable.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Introduce the WindowOrWorkerGlobalScope mixin in webidl

Note that this is not as safe as it might seem, because it changes enumeration order of things on the window.  I expect it's fine, though.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8795844 [details] [diff] [review]
part 1.  Introduce the WindowOrWorkerGlobalScope mixin in webidl

Review of attachment 8795844 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: dom/webidl/WindowOrWorkerGlobalScope.webidl
@@ +10,5 @@
> +
> +// https://html.spec.whatwg.org/multipage/webappapis.html#windoworworkerglobalscope-mixin
> +[NoInterfaceObject, Exposed=(Window,Worker)]
> +interface WindowOrWorkerGlobalScope {
> +  // XXXbz We don't implement 'origin' yet on either window or worker globals.

Do we have a follow-up bug?

@@ +14,5 @@
> +  // XXXbz We don't implement 'origin' yet on either window or worker globals.
> +  // [Replaceable] readonly attribute USVString origin;
> +
> +  // base64 utility methods
> +  [Throws] DOMString btoa(DOMString btoa);

Can we put the [Throws] annotation on its own line to make it easier to see that the webidl matches the spec?

@@ +19,5 @@
> +  [Throws] DOMString atob(DOMString atob);
> +
> +  // timers
> +  // NOTE: We're using overloads where the spec uses a union.  Should
> +  // be black-bos the same.

nit: s/bos/box/g

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Do we have a follow-up bug?

Filed bug 1306170 and adjusted the comment accordingly.

> Can we put the [Throws] annotation on its own line to make it easier to see that the webidl matches the spec?

Sure, done.

> nit: s/bos/box/g

Good catch.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add IsSecureContext on nsPIDOMWindowInner

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff addressing all the review comments except the exception/error-event bit

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff switching from firing exceptions to firing error events at the worker

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated with both interdiffs, mostly for reference. This is what I'll check in as part 3 of this bug

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8795958 [details] [diff] [review]
part 2.  Add IsSecureContext on nsPIDOMWindowInner

Review of attachment 8795958 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8795960 [details] [diff] [review]
Interdiff addressing all the review comments except the exception/error-event bit

Review of attachment 8795960 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: dom/workers/WorkerPrivate.h
@@ +208,5 @@
> +  // of state (loadinfo, worker type, parent).
> +  //
> +  // It's a bit unfortunate that we have to have an out-of-band boolean for
> +  // this, but we need access to this state from the parent thread, and we can't
> +  // use our global object's secure state there.

Do you want to assert in WorkerPrivate::IsSecureContext() that its only being called from the main thread?  If the caller is on the worker thread then they should be using WorkerGlobalScope::IsSecureContext() instead, right?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #19)
> Do you want to assert in WorkerPrivate::IsSecureContext() that its only
> being called from the main thread?  If the caller is on the worker thread
> then they should be using WorkerGlobalScope::IsSecureContext() instead,
> right?

Or if your on the worker thread you could forward to the WorkerGlobalScope version.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Do you want to assert in WorkerPrivate::IsSecureContext() that
> its only being called from the main thread?

That wouldn't allow me to have an assert in WorkerGlobalScope::IsSecureContext() that the two values match.  Likewise for the forwarding approach.

I could try to rejigger things so that WorkerPrivate::IsSecureContext() is the "canonical" value and WorkerGlobalScope::IsSecureContext() asserts that the two are equal, but other things already treat the JSCompartment boolean as canonical...  So I'm not sure we'd get any better clarity.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8795961 [details] [diff] [review]
Interdiff switching from firing exceptions to firing error events at the worker

Review of attachment 8795961 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for pushing this through now.

::: dom/workers/RuntimeService.cpp
@@ +2490,5 @@
>                                                         channel->Port1());
>  
> +  if (!shouldAttachToWorkerPrivate) {
> +    // We're done here.  Just queue up our error event and return our
> +    // dead-on-arrival SharedWorker.

Perhaps a dom/workers issue, but its surprising to me we don't track this dead state anywhere.  I guess there is no way for js code to detect it other than postMessage() calls never getting received on the channel?

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52868b2911e9
part 1.  Introduce the WindowOrWorkerGlobalScope mixin in webidl.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/7740dfd728b6
part 2.  Add IsSecureContext on nsPIDOMWindowInner.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a41bf128f85
part 3.  Implement isSecureContext for worker scopes.  r=bkelly

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/52868b2911e9
https://hg.mozilla.org/mozilla-central/rev/7740dfd728b6
https://hg.mozilla.org/mozilla-central/rev/0a41bf128f85

Comment 25

[Chris Mills [:cmills]]

I've documented this new property:

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/isSecureContext

And added a note about it to the Fx 52 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM

As a bonus, I've also documented the whole WindowOrWorkerGlobalScope mixin, as we were missing it:

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope

This includes updating Window and WorkerGlobalScope to show the new places where these features are implemented from:

https://developer.mozilla.org/en-US/docs/Web/API/Window
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope

And I've marked out-of-date interfaces as obsolete:

https://developer.mozilla.org/en-US/docs/Web/API/WindowTimers
https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64
https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmapFactories
https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch
https://developer.mozilla.org/en-US/docs/Web/API/IDBEnvironment

I hope this is all OK; let me know if anything needs updating ;-)