Closed Bug 1057933 Opened 5 years ago Closed 5 years ago

ServiceWorkerRegistrations should use scope and not window to obtain correct worker

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(2 files)

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.
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....  :(
Attached file 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
Closed: 5 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.