Make secure context requirements more explicit

[mkruisselbrink]

It is not clear to me what the current spec actually requires when it comes to secure contexts. Other than for the cache API, the only mention on restrictions wrt secure contexts seems to be this paragraph:

Service workers should execute in secure contexts, and the controlled service worker clients should also be secure contexts. This effectively means that service workers and their service worker clients should be hosted over HTTPS. The user agents may allow localhost, 127.0.0.0/8, and ::1/128 for development purpose. (Note that they are still secure contexts.) The primary reason for this restriction is to protect users from the risks associated with insecure contexts.

The first thing I'm confused about is why this is only a "should", while secure context checks for the cache API are phrased as a "must"?

But more importantly, I'm not sure what requirements this is actually trying to impose on which parts of the API. For one this paragraph doesn't seem to say anything at all about "uncontrolled service worker clients" (not sure if there is such a thing), or more in general which parts of the API should be available to contexts that run in the same origin as a service worker, but that are not secure contexts. From just reading that paragraph I'm led to believe that insecure contexts should be able to call pretty much every method, get access to existing SW registrations, postMessage to them, etc.

But I've been told (in WICG/background-sync#90) that the intention of the paragraph is to completely disallow access to any API surface from insecure contexts. If that is indeed the intended behavior, could that maybe be specified somehow?

[jungkees]

WICG/background-sync#90 (comment)

This should be clarified first. When writing the secure context section, I think I didn't take into account the case where the client having a potentially trustworthy origin is embedded in an insecure context.

I think the minimum requirement is "A SW should be executed in its registering client's origin which is potentially secure." Is there any reason that such clients embedded in insecure context should not access the SW?

/cc @mikewest @annevk @slightlyoff @jakearchibald

[jungkees]

The first thing I'm confused about is why this is only a "should", while secure context checks for the cache API are phrased as a "must"?

I had discussed this with @slightlyoff when I wrote the text. I forgot the reason we got to that exactly. Will find out and clarify it.

/cc @slightlyoff

[jungkees]

But more importantly, I'm not sure what requirements this is actually trying to impose on which parts of the API.

I think it depends on this discussion: #754 (comment). If it turns out the minimum requirement there is enough, additional guards in the method for secure context would not be required. Otherwise, I can add them where they make sense.

"uncontrolled service worker clients" (not sure if there is such a thing)

A service worker client as a spec concept is just an environment settings object. An uncontrolled client is a client within the same origin but currently uncontrolled as like it's out-of-scope. One such client can be gotten by e.g.:
self.clients.matchAll({ includeUncontrolled: true }).

I agree this text ("the controlled service worker clients should also be secure contexts") should be clarified too. Thanks for having pointed them out.

/cc @slightlyoff @jakearchibald

[slightlyoff]

I'd like to understand what @mikewest thinks.

The plain reading of the Secure Contexts doc is that an https (secure) document w/ potentially matching SW but which is loaded as an iframe that's a child of an http (insecure) can't use SWs. I'm not sure how much value there is in not adopting this stronger definition, but I'd like to hear from more people about it.

[wanderview]

I guess this means embedded social media frames (for like, follow, etc buttons) couldn't use service workers if they are embedded in an html document. Right?

I guess this seems ok to me and its probably something foreign-fetch could solve in the future.

[jakearchibald]

From https://code.google.com/p/chromium/issues/detail?id=489670

@mikewest

This is expected behavior, as per https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-secure.

We didn't do this for WebCrypto, and the result was Netflix writing a 1:1 postMessage-based wrapper. That's not something we can support while maintaining the guarantees that locking an API to HTTPS is supposed to provide

Related bug #700 - this feels like a dupe?

[annevk]

Well the reason this was raised is because it's unclear in the specification. So in that sense it's not a duplicate.

[mkruisselbrink]

Yes, this was raised because 1) the current spec doesn't specify what should and shouldn't be available to a potential client in an insecure context (I agree that probably nothing should be available there, to be least surprising, and avoid the trivial postMessage wrapper), and 2) the current spec doesn't specify what should happen if an insecure context tries to access any of the API.

I assume for 2) that controller always returns undefined, ready returns a promise that always rejects, and all the methods also return promises that always reject? Although for ready the spec says that it will never reject, so maybe in this case it should also just return a promise that never resolves nor rejects?

[jungkees]

So, it was answered by #754 (comment) and the Secure Context spec has this: https://w3c.github.io/webappsec-secure-contexts/#examples-service-workers.

I wonder if we want to use "should" for register and messaging? Or should we use "must" for all?

• Service workers must execute in secure contexts.
• Register: the registering client should be a secure context.
• Control: the service worker clients must be secure contexts.
• Messaging: the service worker clients should be secure contexts.

For calling APIs in insecure contexts, locking the following APIs seems enough?

// Disallow the access to registrations and service workers
ServiceWorkerContainer.controller // throw a SecurityError exception
 - @mkruisselbrink suggested it return undefined. Not sure which way is better.
ServiceWorkerContainer.ready // reject with SecurityError exception 
 - @mkruisselbrink concerned about ready never rejecting but I think it's okay here.
ServiceWorkerContainer.register() // reject with SecurityError exception
ServiceWorkerContainer.getRegistration() // reject with SecurityError exception
ServiceWorkerContainer.getRegistrations() // reject with SecurityError exception

// Disallow posting a message to a client in insecure context
Client.postMessage() // throw a SecurityError exception if the target client is not a secure context

// Disallow the access to cache objects (which the spec already does).
CacheStorage.match() // reject with SecurityError exception
CacheStorage.has() // reject with SecurityError exception
CacheStorage.open() // reject with SecurityError exception
CacheStorage.delete() // reject with SecurityError exception
CacheStorage.keys() // reject with SecurityError exception

[annevk]

Must for all.

[slightlyoff]

I don't understand what must for all gets us here over what @jungkees outlined. Implementers will block based on should. LGTM

[annevk]

What's the rationale for should?

[jungkees]

Applied the requirements as discussed so far: 002eba5. (Discussed further with @slightlyoff and decided to use "must" for all):

Service workers must execute in secure contexts. Service worker clients must also be secure contexts to register a service worker registration, to get access to the service worker registrations and the service workers, to do messaging with the service workers, and to be manipulated by the service workers.

• Summary

// Disallow register
ServiceWorkerContainer.register() // reject with SecurityError exception

// Disallow the access to registrations and service workers
ServiceWorkerContainer.controller // return undefined.
ServiceWorkerContainer.ready // reject with SecurityError exception 
ServiceWorkerContainer.getRegistration() // reject with SecurityError exception
ServiceWorkerContainer.getRegistrations() // reject with SecurityError exception

// Disallow getting a controller
Handle Fetch step 12.1 returns null when request.client is not a secure context
Clients.claim() // skip the clients which is not secure contexts

// Disallow the access to non-secure clients
Clients.matchAll() // skip the clients which is not secure contexts
Clients.get(id) // reject with SecurityError exception

// Disallow the access to cache objects
CacheStorage.match() // reject with SecurityError exception
CacheStorage.has() // reject with SecurityError exception
CacheStorage.open() // reject with SecurityError exception
CacheStorage.delete() // reject with SecurityError exception
CacheStorage.keys() // reject with SecurityError exception

// Disallow messaging between non-secure SW and non-secure clients
Done by essentially preventing the access to those objects

[jungkees]

Blink: tracked in https://crbug.com/542499.

[jungkees]

I came up with a question while implementing this change:
Is ServiceWorkerContainer.controller returning null instead of undefined in non-secure context okay? If we'd want to return undefined, the IDL type should be changed from ServiceWorker? to any and some custom getter should be written for V8 in Blink case. If null in the script surface is fine, I'd suggest using null here. Or, I wonder if throwing a SecurityError exception would be better than using null?

/cc @slightlyoff @annevk @jakearchibald @domenic

[annevk]

Null or undefined does not matter much in that sense, since either way it's discoverable that a property is defined.

[jungkees]

Will discuss hiding the properties entirely or not-hiding in the web app sec meeting.

[wanderview]

Is it intended that .register() now allows file:// URLs? It simply says:

• If the result of running Is origin potentially trustworthy with the origin of scriptURL as the argument is Not Trusted, then:
  • Return a promise rejected with a "SecurityError" exception.

But the Is origin potentially trustworthy algorithm now allows file schemes:

If origin's scheme component is file, return "Potentially Trustworthy".

Note, Cache API still does explicitly requires http or https URLs for all Requests. So its not really feasible to load a site from file:// and have the service worker function correctly.

[jakearchibald]

F2F: update spec. Property won't exist in non-secure contexts.

Make sure SW doesn't work for file:// urls.

Remaining question: Should navigator.serviceWorker be there for file:// origins? @annevk do you have strong feelings here?

[annevk]

@jakearchibald I think it's fine for it to exist and throw, if a user agent considers file:// a secure context (not sure whether a user agent should do that).

[jungkees]

Closed by 5264a72

Make sure SW doesn't work for file:// urls.
Currently, we ensure this in register() method step 6 (step 5 after the above commit).