Service-Worker-Allowed header

[mfalken]

Spinning off of #468. I have some comments about the spec change in ddb21be.

1. Suppose a site changes the Service-Worker-Allowed header after a worker installs. E.g., at install time it was "/" and later the site change it to "/restricted_path. What's the expected behavior? I would think upon "24 hour" update you re-evaluate the path restriction using the new max scope against the newest worker's script URL. If that fails, you terminate the workers and clear the registration. Does that make sense?
2. The spec language needs some proofreading. I think "Let" and "Set" got mixed up in some places.

[mfalken]

We should also specify what happens if the HTTP response contains multiple values for "Service-Worker-Allowed". If I understand correctly, there is no ABNF for "Service-Worker-Allowed" defined so the parse step https://fetch.spec.whatwg.org/#concept-header-parse won't fail.

I think Chromium will just take the first value.

[jungkees]

Suppose a site changes the Service-Worker-Allowed header after a worker installs. E.g., at install time it was "/" and later the site change it to "/restricted_path. What's the expected behavior? I would think upon "24 hour" update you re-evaluate the path restriction using the new max scope against the newest worker's script URL.

Yes, for all the Update/Soft Update cases (ServiceWorkerContainer.register(), ServiceWorkerRegistration.update(), 24h update, navigation matching), the given registration's scope (NOT the script URL) is re-evaluated against the new max scope.

If that fails, you terminate the workers and clear the registration. Does that make sense?

I think so as the site is trying to give it a new restriction upon those updates.

The spec language needs some proofreading. I think "Let" and "Set" got mixed up in some places.

I double-checked and couldn't find misuse of those. Probably, the use of similar names, maxScope and maxScopeString, made the confusion?

[mfalken]

Yes, for all the Update/Soft Update cases (ServiceWorkerContainer.register(), ServiceWorkerRegistration.update(), 24h update, navigation matching), the given registration's scope (NOT the script URL) is re-evaluated against the new max scope.

Cool. Do you mean the current spec says this, or you plan to add it? AFAIK it's not yet in the spec. Currently if the path restriction fails you only delete the registration if there is no installed worker.

I double-checked and couldn't find misuse of those. Probably, the use of similar names, maxScope and maxScopeString, made the confusion?

It should be "Let xxx be" and "Set xxx to". Sometime there is "Let xxx to" and "Set xxx be". Minor quibble but reading spec language is hard enough as it is :)

[jungkees]

Currently if the path restriction fails you only delete the registration if there is no installed worker.

You're right! I added a step which invokes Unregister algorithm when there's been a worker in the registration: 1613f9f.

Sometime there is "Let xxx to" and "Set xxx be".

Fixed. Thanks!

[mfalken]

Hmm.. I have second thoughts about this, what if you do:

register("resources/sw.js", { scope: '/' });  // OK, ServiceWorker-Allowed = "/"
register("resources/some_other_sw.js", { scope: '/' }); // no ServiceWorker-Allowed header

It would mean the registration is uninstalled, that doesn't look right.

I guess the uninstallation should only happen for same-script URL update?

[jungkees]

I agree. Service-Worker-Allowed says, "I would allow this script to be running even in the designated scope". Will fix this.

[jungkees]

the uninstallation should only happen for same-script URL update

5617e9c

[mfalken]

Thanks Jungkee. I thought about this a little more last night.

1. We don't unregister in any other case, like MIME type changed or response code became 403, etc. Maybe it's more consistent to not unregister here then?
2. I think the header value should be relative to the script URL (the HTTP request URL), rather than the client's base URL. That's how Content-Location and Referer works. WDYT?

[mfalken]

Also,
3. Shouldn't an invalid Service-Worker-Allowed value be a SecurityError rather than a NetworkError?
4. An invalid Service-Worker-Allowed value does not uninstall. Only a valid one that no longer satisfies path restriction does. That seems a bit strange, intended?

[mfalken]

Maybe 3-4 above are OK. If you can't even parse the Service-Worker-Allowed header, it's is probably a network error. I was confused between URL parsing and HTTP header parsing.

Which brings me to the next question, can URL parsing fail? For the step "Let maxScope be the result of parsing serviceWorkerAllowed with client's API base URL.", is it possible for maxScope to be failure?

[jungkees]

[1]. We don't unregister in any other case, like MIME type changed or response code became 403, etc. Maybe it's more consistent to not unregister here then?
[4]. An invalid Service-Worker-Allowed value does not uninstall. Only a valid one that no longer satisfies path restriction does. That seems a bit strange, intended?

Agreed. Let's keep it more consistent to other cases (i.e. not unregister.) Yes, it's a SecurityError so we stop updating it right here but the one already installed had had a permission to do so then. If authors wanted to unregister this, they should explicitly do so.

[2]. I think the header value should be relative to the script URL (the HTTP request URL), rather than the client's base URL. That's how Content-Location and Referer works. WDYT?

Good catch. If we parse it against the client's base URL, the value would change based on the document locations in which the methods are invoked. Will fix this.

[3]. Shouldn't an invalid Service-Worker-Allowed value be a SecurityError rather than a NetworkError?

It's aligned to Fetch spec. A header parsing failure leads to a network error there.

[jungkees]

[1][2][4]: 4c4fec3.

[jungkees]

@mattto on second thought, "[3]. Shouldn't an invalid Service-Worker-Allowed value be a SecurityError rather than a NetworkError?" seems a right thing to do. A network error from fetch can be caused by many things including MIX, CSP check, etc.

Fixed: 4c318ae. Thanks for reviewing the steps!

[slightlyoff]

As a perhaps-unrelated issue, I a conversation I had with @metromoxie makes me think we should also add a special none or blank value to Service-Worker-Allowed to disable SWs entirely. Many users want a similar kill-switch.

[jungkees]

a special none or blank value to Service-Worker-Allowed to disable SWs entirely

To my understanding, Service-Worker-Allowed header as-is actually means Scope-Allowed-For-This-Service-Worker.

That said, if it's something we wanted to kill the SW registration this script has been bound to, then I think it sort of makes sense we use this header for that. The expected behavior would be to invoke Clear Registration algorithm no matter whether some pages are currently being controlled. (or to invoke Unregister if we take more conservative approach.)

But if it's something we wanted to kill all the registrations or something like that, then it seems we need another header (directive).

[slightlyoff]

I'd be happy with a different header to kill things, but something seems necessary.

@jakearchibald @annevk: thoughts?

[annevk]

I had the impression unregister() was the way to remove a single service worker and there would be no protocol equivalent.

If we want a protocol solution to removing a service worker, using HTTP 410 seems much more applicable.

Using none as keyword to forbid service workers of any kind seems fine although it's a bit unclear to me how you disambiguate that from a relative URL. Would we require URLs in that header to start with a slash?

[KenjiBaheux]

I've filed #614 regarding the discussion on the protocol solution for a kill-switch.

I've checked with @matto that he's happy with @jungkees latest update. Let me close.

[wanderview]

Tracked in gecko here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1130101