ServiceWorkerRegistrations should use scope and not window to obtain correct worker

RESOLVED FIXED in mozilla35

Status

()

Core
DOM: Workers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

unspecified
mozilla35
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

ServiceWorkerRegistration attempts to get a worker for the window it is running in, when it should be using it's scope.

// index.html
navigator.serviceWorker.register('foo.js', { scope: '/someotherscope/' }).then(function(swr) {
  swr.active/waiting/installing should be non-null because someotherscope does indeed have foo.js as it's worker.
});

Currently they are null because swr uses index.html's window to [[MatchScope]] on the list of registrations.
Created attachment 8478089 [details] [diff] [review]
ServiceWorkerRegistrations should use scope and not window to obtain workers

I'm not sure if the security check is required, since all the APIs to obtain a ServiceWorkerRegistration already perform those.
Attachment #8478089 - Flags: review?(amarchesini)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment on attachment 8478089 [details] [diff] [review]
ServiceWorkerRegistrations should use scope and not window to obtain workers

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +1795,3 @@
>  {
> +  AssertIsOnMainThread();
> +  nsCOMPtr<nsIDocument> doc = GetEntryDocument();

I want to have the opinion of somebody else on this. I don't think it's safe enough to have a service that uses the stack to retrieve the document. It seems fragile. Can we have another approach? bz, feedback?

@@ +1800,5 @@
> +  nsCString scope = NS_ConvertUTF16toUTF8(aScope);
> +
> +  ///////////////////////////////////////////
> +  // Security check
> +  nsCOMPtr<nsIURI> scopeURI;

the indentation is a bit strange. Move nsCOMPtr<nsIURI> after the comment.
Attachment #8478089 - Flags: feedback?(bzbarsky)
Comment on attachment 8478089 [details] [diff] [review]
ServiceWorkerRegistrations should use scope and not window to obtain workers

Why is this looking at the entry document's principal, instead of, say, the subject principal?  And for the window the entry window instead of the incumbent one or the current realm?

Can someone please point me to the spec for the behavior here?
Attachment #8478089 - Flags: feedback?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #3)
> Comment on attachment 8478089 [details] [diff] [review]
> ServiceWorkerRegistrations should use scope and not window to obtain workers
> 
> Why is this looking at the entry document's principal, instead of, say, the
> subject principal?  And for the window the entry window instead of the
> incumbent one or the current realm?
> 
> Can someone please point me to the spec for the behavior here?

This part isn't in the spec. What I wanted to do was make sure that the page that calls serviceworkerRegistration.active (see bug description) is allowed to load the scope the registration is for, before handing it a ServiceWorker.

But similar other functions like ServiceWorkerManager::(Un)Register use EntryGlobal() as defined by their specs (entry settings object's API base URL) [1]. Is that correct?

[1]: http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#register-algorithm
Using the entry global for base URLs makes sense in that it matches how other web APIs work for base URLs, though it's a pretty messed up behavior.  Pretty much any other use of entry globals is almost certainly wrong.

> What I wanted to do was make sure that the page that calls
> serviceworkerRegistration.active 

That would be the incumbent global.

Let me illustrate the difference between the two.

Say a user clicks a link in window A, which calls a function in window B, which then gets .active on a ServiceWorkerRegistration object from window C.

In this situation, the entry global is A, the incumbent global is B, the current realm is C.  Using A for security checks is a bit weird, imo.  Even using it for base URIs is weird moderately weird, unless A is the thing that passes a URL to B that we're trying to resolve relative to a base URI....  :(
Created attachment 8480692 [details]
interdiff

Just uses owner of the registration instead of mucking with entry document.
Andrea, please go ahead and land this if it is satisfactory (with interdiff).
Comment on attachment 8478089 [details] [diff] [review]
ServiceWorkerRegistrations should use scope and not window to obtain workers

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

I think you can get rid of:

already_AddRefed<ServiceWorkerRegistrationInfo>
ServiceWorkerManager::GetServiceWorkerRegistrationInfo(nsPIDOMWindow* aWindow)
Attachment #8478089 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/a5d98477ddc9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.