1336364 - Off-origin requests are not handled by service worker if 3rd-party cookie is disabled

Status: RESOLVED FIXED

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR:

1. about:preferences#privacy > Set "Nightly will [Use custom settings for history]" and "Accept third-party cookies: [Never]"
2. Go to a service worker activated page that links to an off-origin external stylesheet

Expected:

1. A fetch event is dispatched inside service worker for the worker to handle the stylesheet request

Actual:

1. fetch event is not dispatched and therefore the request is handled by network. The page will break when there isn't network connectivity.

Note:

I am not sure if this is by design and if so it's not documented anywhere. I also don't really know what we are trying to defend here if this is by design.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Do you have an example of a site that does this?

We intend to disable service workers in 3rd-party iframes when 3rd-party cookies are disabled, but I think a top level document loading cross-origin should work.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://timdream.org/jszhuyin/

Set a breakpoint inside the fetch event handler you will see it will only be invoked for 12 times, instead of 14.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

The code is intentionally disallowing intercept of "third party URI" sub-resources, not sure the rationale holds up:
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#14681

The check also happens before checking and without consulting aIsNonSubresourceRequest which seems a bit sketchy too.  It seems like it might end up inhibiting a different-origin scope from being controlled on navigation across origins as well.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

So I read through bug 1152899 and it seems the intend is to disallow requests from 3rd-party iframes from being intercepted, not 3rd-party (off-origin) resources. Actually, the whole "feature" implemented in bug 1152899 could be regarded as a compat issue.

Comment 5

[Andrew Overholt [:overholt]]

Maybe Ehsan can comment given his involvement with bug 1152899.

Comment 6

[(no longer active)]

(In reply to Andrew Overholt [:overholt] from comment #5)
> Maybe Ehsan can comment given his involvement with bug 1152899.

My only involvement in that bug was filing it.  The patch that landed there clearly didn't implement what the bug asked for, and I agree that it is broken.  Forwarding to Fernando (the author) and smaug (the reviewer).

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Andrew Sutherland [:asuth] from comment #3)
> The code is intentionally disallowing intercept of "third party URI"
> sub-resources, not sure the rationale holds up:
> http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#14681
> 
> The check also happens before checking and without consulting
> aIsNonSubresourceRequest which seems a bit sketchy too.  It seems like it
> might end up inhibiting a different-origin scope from being controlled on
> navigation across origins as well.

I did a quick test on Chrome: if iframe.src points to a 3rd-party origin, service worker will not intercept it. service worker can only intercept the requests if the iframe.src points to the same origin. This behavior remain unchanged regardless of 3rd-party cookie settings.

Does what bug 1152899 asks for really holds up? Given that Chrome do that regardless of 3rd-party cookie setting, should we just follow what it does instead?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

BTW the permlink of the link in comment 3 would be

http://searchfox.org/mozilla-central/rev/ea31c4b64f34a29415a69fb768f8051495547315/docshell/base/nsDocShell.cpp#14730-14738

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Based on the discussion at https://lists.mozilla.org/pipermail/dev-platform/2015-August/011405.html and its corresponding bug 1184973, it seems like the behavior of the preference as it relates to service workers should be:

- Do not allow existing service workers to intercept navigation requests made for a 3rd-party iframe.
- Do now allow pages in iframes to access ServiceWorker API's, namely navigator.serviceWorker.  So ServiceWorkerContainer::IsEnabled should return false.
- Do not allow serviceworkers to see 3rd-party iframe pages as clients using Clients.matchAll(), etc.

It sounds suspiciously like we want OriginAttributes to grow an mThirdPartyIframe or mStorageForbidden member.

Comment 10

[(no longer active)]

(In reply to Andrew Sutherland [:asuth] from comment #9)
> Based on the discussion at
> https://lists.mozilla.org/pipermail/dev-platform/2015-August/011405.html and
> its corresponding bug 1184973, it seems like the behavior of the preference
> as it relates to service workers should be:
> 
> - Do not allow existing service workers to intercept navigation requests
> made for a 3rd-party iframe.
> - Do now allow pages in iframes to access ServiceWorker API's, namely
> navigator.serviceWorker.  So ServiceWorkerContainer::IsEnabled should return
> false.
> - Do not allow serviceworkers to see 3rd-party iframe pages as clients using
> Clients.matchAll(), etc.

The caches API is subject to the storage rules: <http://searchfox.org/mozilla-central/rev/59cd73fbfa14384a81a4e3eb17d3881b372702c4/dom/base/nsGlobalWindow.cpp#11201> and it seems to me that we should match that exactly for service workers.  Is that not what we want?

We also have this weirdness in that we disable the Cache API by throwing exceptions from its methods, and hiding the SW API in those cases is debatable.

(I defer the final decision to bkelly and asuth as they have been involved with SWs much more than me recently.)

> It sounds suspiciously like we want OriginAttributes to grow an
> mThirdPartyIframe or mStorageForbidden member.

No, this isn't a property of the origin, it's the property of the window hierarchy.  See for example the iframe sandbox checks.  OAs are only useful for things that are the properties of the origin.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to :Ehsan Akhgari from comment #10)
> The caches API is subject to the storage rules:
> <http://searchfox.org/mozilla-central/rev/
> 59cd73fbfa14384a81a4e3eb17d3881b372702c4/dom/base/nsGlobalWindow.cpp#11201>
> and it seems to me that we should match that exactly for service workers. 
> Is that not what we want?
> 
> We also have this weirdness in that we disable the Cache API by throwing
> exceptions from its methods, and hiding the SW API in those cases is
> debatable.

Huh.  That's weird.  I was assuming our strategy was to fail feature detection rather than making everything explode, but I have zero practical experience with our established patterns here.  Consistency seems most important.  (Although I would expect we have some latitude since it's a non-standard feature that intentionally breaks/limits things.)  I suspect :bkelly has an authoritative answer, but if not, I will be sure to do more extensive research/asking of :bz before I state any more opinions on this :)

> > It sounds suspiciously like we want OriginAttributes to grow an
> > mThirdPartyIframe or mStorageForbidden member.
> 
> No, this isn't a property of the origin, it's the property of the window
> hierarchy.  See for example the iframe sandbox checks.  OAs are only useful
> for things that are the properties of the origin.

So, I guess I get that, but it seems like the "disable 3rd-party cookies" functionality would be harder to screw up if we could ensure that the principal of a 3rd party iframe for origin A was not equal to the principal for a top-level window for origin A.  For example, it looks like BroadcastChannel's implementation allows 3rd-party iframe for origin A to communicate with top-level window for origin A (originChannelKey is defined as "<channelName>|<origin+OriginAttributes>").  If there was an OriginAttribute to namespace them separately, code would have to actively screw up to allow them communicate.  Quota manager would be able to provide defense-in-depth by refusing to provide storage for an origin with the StorageForbidden OA bit set, etc.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I fielded some user concerns this weekend about 3rd party scripts and privacy.  I'd like to fix this to make sure its working properly.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Refact nsDocShell::ShouldPrepareForIntercept() to short-circuit on subresource instead of non-subresource. r=smaug

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix the SW interception 3rd party cookie check to only apply to non-subresource requests and to properly check top window URI. r=smaug

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Restrict service worker interception for more cookie policies. r=asuth

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Disallow service worker register() is storage is forbidden for the window. r=asuth

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Disable service worker getRegistrations() if storage is disallowed for a window. r=asuth

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Ensure that we don't control a document if its window cannot access storage. r=asuth

This patch is work-in-progress.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Ensure that we don't control a document if its window cannot access storage. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1961805e08488a92079137a1ba82f25f92630433

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

I still need to add a test to verify the behavior from comment 0 is fixed.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8909511 [details] [diff] [review]
P1 Refact nsDocShell::ShouldPrepareForIntercept() to short-circuit on subresource instead of non-subresource. r=smaug

This is a refactoring patch which just reverses the order of the short-circuit logic in nsDocShell::ShouldPrepareForIntercept().  Instead of short-circuiting on non-subresource requests, we now short-circuit on subresource requests.  The next patch is going to add code to the non-subresource path so its going to be longer.

No functional change in this patch.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8909512 [details] [diff] [review]
P2 Fix the SW interception 3rd party cookie check to only apply to non-subresource requests and to properly check top window URI. r=smaug

Olli, in this patch I try to fix some basic problems with our 3rd party iframe request checking.  To do this the patch:

1. Move the check down in the method so it only happens for non-subresource requests.  Subresource request interception is not involved here since it is purely dictated by if the initiating document is controlled.
2. Change the logic to use the top window URI instead of mCurrentURI in order to determine 3rd party status.

I have some follow-up patches to change what cookie policies we trigger this on, but I plan to ask :asuth for that review since its more tied to service worker stuff.  I was hoping you could review this, though, since I'm only 80% sure I did the top window checking correctly.  Thanks!

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8909513 [details] [diff] [review]
P3 Restrict service worker interception for more cookie policies. r=asuth

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

Andrew, this patch tightens how our cookie policy blocks service workers.  Previously we were only stopping service worker interception in some cases for REJECTFOREIGN.  If the cookie policy blocked *all* cookies, though, we just went ahead and did service worker interception anyway.  This doesn't make sense to me.

So this patch:

1. Blocks all service worker interception if the policy is REJECT
2. Blocks 3rd party iframe interception if the policy is REJECTFOREIGN or LIMITFOREIGN

Tanvi, does this sound reasonable to you?

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8909514 [details] [diff] [review]
P4 Disallow service worker register() is storage is forbidden for the window. r=asuth

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

Next, we need to stop 3rd party iframes from registering new service workers.  Since the window is constructed here I use the nsContentUtils::StorageAllowedForWindow() utility method.  This will disable serviceWorker.register() any time storage is disabled, which includes for 3rd party windows.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8909516 [details] [diff] [review]
P5 Disable service worker getRegistrations() if storage is disallowed for a window. r=asuth

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

I think we should also prevent serviceWorker.getRegistration(s) from working when storage is blocked.  If we let once of these windows get a ServiceWorker DOM object then they can postMessage() it to escape the storage block on the window.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8909570 [details] [diff] [review]
P6 Ensure that we don't control a document if its window cannot access storage. r=asuth

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

Also, I added an assertion that we never control a window that has storage access blocked.  This triggered pretty quickly, because we totally didn't prevent that before.  So this patch adds both the assertion and checks in ServiceWorkerManager::GetServiceWorkerRegistrationInfo() to prevent these windows from finding the registration when being controlled.

Note, since MaybeStartControlling() and GetServiceWorkerRegistrationInfo() get called during document creation the inner window does not have the document set yet.  So I could not just use nsContentUtils::StorageAllowedForWindow().  Instead I created a slightly different version that operates directly on the document.  With only one small nullptr fix this lets the checking work on these documents.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8909512 [details] [diff] [review]
P2 Fix the SW interception 3rd party cookie check to only apply to non-subresource requests and to properly check top window URI. r=smaug

>+  if (nsContentUtils::CookiesBehavior() == nsICookieService::BEHAVIOR_REJECT_FOREIGN) {
>+    nsCOMPtr<nsIDocShellTreeItem> topItem;
>+    GetSameTypeRootTreeItem(getter_AddRefs(topItem));
>+    if (topItem && !SameCOMIdentity(GetAsSupports(this), topItem)) {
>+      nsPIDOMWindowOuter* outer = topItem->GetWindow();
>+      nsPIDOMWindowInner* inner = outer ? outer->GetCurrentInnerWindow()
>+                                         : nullptr;
>+      nsIURI* uri = inner ? inner->GetDocumentURI() : nullptr;
>+      if (uri) {

I don't know why the change to uri handling (add some comment here)....

>+        nsresult rv = NS_OK;
>+        nsCOMPtr<mozIThirdPartyUtil> thirdPartyUtil =
>+          do_GetService(THIRDPARTYUTIL_CONTRACTID, &rv);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        bool isThirdPartyURI = true;
>+        rv = thirdPartyUtil->IsThirdPartyURI(mCurrentURI, aURI,
>+                                             &isThirdPartyURI);
but at least I assume you meant to use uri here and not mCurrentURI

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Disable service worker getRegistrations() if storage is disallowed for a window. r=asuth

Minor update to fix static analysis failure from try.  Again, this patch blocks access to serviceWorker.getRegistration calls so 3rd party iframes can't postMessage() a service worker to wake it up and do stuff.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #27)
> >+  if (nsContentUtils::CookiesBehavior() == nsICookieService::BEHAVIOR_REJECT_FOREIGN) {
> >+    nsCOMPtr<nsIDocShellTreeItem> topItem;
> >+    GetSameTypeRootTreeItem(getter_AddRefs(topItem));
> >+    if (topItem && !SameCOMIdentity(GetAsSupports(this), topItem)) {
> >+      nsPIDOMWindowOuter* outer = topItem->GetWindow();
> >+      nsPIDOMWindowInner* inner = outer ? outer->GetCurrentInnerWindow()
> >+                                         : nullptr;
> >+      nsIURI* uri = inner ? inner->GetDocumentURI() : nullptr;
> >+      if (uri) {
> 
> I don't know why the change to uri handling (add some comment here)....

Comparing to mCurrentURI seemed wrong here.  We don't want to know if we are 3rd party to the window we are navigating from, but instead we want to know if we are 3rd party to a parent.  My code here tries to fix this by comparing to the top window URI.

I'm not sure this is adequate, either, though.  Looking at ThirdPartyUtil it seems we walk the entire parent tree when checking if a window is 3rd party or not.  I should probably do that here as well.  I guess I will make a new routine in ThirdPartyUtil to do this for a docshell.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #29)
> I'm not sure this is adequate, either, though.  Looking at ThirdPartyUtil it
> seems we walk the entire parent tree when checking if a window is 3rd party
> or not.  I should probably do that here as well.  I guess I will make a new
> routine in ThirdPartyUtil to do this for a docshell.

Actually, no, I don't think I need this.  I think I can just get the parent window and do:

  IsThirdPartyWindow(parentWindow, aURIToCheck, &isThirdParty);

I think this is the most correct and avoids adding any new ThirdPartyUtil code.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix the SW interception 3rd party cookie check to only apply to non-subresource requests and to properly check top window URI. r=smaug

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8909799 [details] [diff] [review]
P2 Fix the SW interception 3rd party cookie check to only apply to non-subresource requests and to properly check top window URI. r=smaug

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

Ok, let's try this again.  I believe the correct thing to do here is use IsThirdPartyWindow() instead of what I was trying before:

http://searchfox.org/mozilla-central/source/netwerk/base/mozIThirdPartyUtil.idl#43

I call this with the parent window, if it exists, to compare to the requested URL.  IsThirdPartyWindow() will automatically walk the parent tree correctly.

Again, we don't want to use mCurrentURI or the current window.  If a frame is being navigated, we don't really care if the new URL is 3rd party to its old URL.  We only care if its 3rd party to the parent tree.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Add a mochitest to verify subresource fetch events still work with 3rd party iframe cookies disabled. r=asuth

This adds a test that runs the test_fetch_event.html code with the third party pref set to REJECTFOREIGN.  It fails without the other patches and passes with them.  This is testing the problem from comment 0.

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8909513 [details] [diff] [review]
P3 Restrict service worker interception for more cookie policies. r=asuth

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

Posterity note: Although at this time nsICookieService::BEHAVIOR_LIMIT_FOREIGN's documentation might imply through omission we should allow interception if a SW already exists, nsContentUtils::InternalStorageAllowedForPrincipal explicitly disallows access to storage for BEHAVIOR_LIMIT_FOREIGN with the following comment at https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/dom/base/nsContentUtils.cpp#9212

      // XXX For non-cookie forms of storage, we handle BEHAVIOR_LIMIT_FOREIGN by
      // simply rejecting the request to use the storage. In the future, if we
      // change the meaning of BEHAVIOR_LIMIT_FOREIGN to be one which makes sense
      // for non-cookie storage types, this may change.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Restrict service worker interception for more cookie policies. r=asuth

I had to rebase this on top of my P2 changes.

Again, I want to block all service worker interception when cookies are set to reject.

Also, this blocks 3rd party iframe interception if cookie policy is REJECT_FOREIGN or LIMIT_FOREIGN.

I believe these changes are consistent with our cookie-based storage restrictions.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8909825 [details] [diff] [review]
P3 Restrict service worker interception for more cookie policies. r=asuth

Propagate r+ from mid-air collision review.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb17bc8dd695e5c490fe7e685c98aac619193b1b

Comment 38

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8909514 [details] [diff] [review]
P4 Disallow service worker register() is storage is forbidden for the window. r=asuth

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

Nice test cleanup!

Restating:
- We only allow SW registration if storage is fully allowed.  We deny not only for eDeny but also:
  - ePrivateBrowsing: We disable ServiceWorkerContainer if nsContentUtils::IsInPrivateBrowsing() already, so ServiceWorkerContainer::Register() is already not accessible.  But it's also worth noting that our ServiceWorkers/Cache API implementation has no in-memory or magical encrypted backing storage that would make this okay.
  - eSessionScope: No QuotaManager clients support evaporating storage either yet.  Only the cookie service handles this (via nsICookieService::ACCEPT_SESSION, not the StorageAccess enum), plus LocalStorage which has surprising special-casing for this.  (LocalStorage loads the existing persisted-to-disk state into a special memory-only "kSessionSet" slot, so the session state starts from the previously persisted state.)
- We currently don't allow registration from a Worker, which is why this patch can get away with (only) using nsContentUtils::StorageAllowedForWindow.  Bug 1131324 tracks exposing ServiceWorkerContainer on WorkerNavigator.

::: dom/workers/ServiceWorkerManager.cpp
@@ +839,2 @@
>    nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
> +  MOZ_ASSERT(doc);

Restating: My understanding is that this change is safe/proper because mDoc only ever gets nulled out in outer windows (by nsGlobalWindow::DropOuterWindowDocs).  nsGlobalWindow::FreeInnerObjects() would be the place to do it if it was gonna, but it does not.  (I'm commenting because there's no stated invariant at the definition on nsPIDOMWindow.h.  However I do understand this to be consistent with the underlying semantic inner window invariants.  And I get that the inner/outer stuff is getting cleaned up on bug 1400984 and the resurgent bug 558192.)

::: dom/workers/test/serviceworkers/test_third_party_iframes.html
@@ +82,5 @@
>  
> +// Verify that we can register and intercept a 3rd party iframe with
> +// the given cookie policy.
> +function testShouldIntercept(policy, done) {
> +  SpecialPowers.pushPrefEnv({"set": [

I wonder if these helpers should invoke SpecialPowers.popPrefEnv() in a balanced fashion to keep SpecialPowers._prefEnvUndoStack's size under control.  As written, there will be a pref-storm during test cleanup as the changes are effectively replayed in reverse order.  It's conceptually harmless, but the pref changes do get broadcast to the content processes and it might be less of an intermittent risk to just pay the pop price as the test cases happen.  Your call.  Certainly, if it turns out to be a problem, SpecialPowers could just get smarter and learn to coalesce changes at shutdown.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Sutherland [:asuth] from comment #38)
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +839,2 @@
> >    nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
> > +  MOZ_ASSERT(doc);
> 
> Restating: My understanding is that this change is safe/proper because mDoc
> only ever gets nulled out in outer windows (by
> nsGlobalWindow::DropOuterWindowDocs).  nsGlobalWindow::FreeInnerObjects()
> would be the place to do it if it was gonna, but it does not.  (I'm
> commenting because there's no stated invariant at the definition on
> nsPIDOMWindow.h.  However I do understand this to be consistent with the
> underlying semantic inner window invariants.  And I get that the inner/outer
> stuff is getting cleaned up on bug 1400984 and the resurgent bug 558192.)

I mainly did this because the StorageAllowedForWindow() will return eDeny if the document doesn't exist.

Comment 40

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8909780 [details] [diff] [review]
P5 Disable service worker getRegistrations() if storage is disallowed for a window. r=asuth

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

non-patch-related: Thanks for explaining the doc invariant assumption better!

nit: You only add test coverage for getRegistration(), not getRegistrations().  I don't think this really matters since it's the same logic and by inspection I can see it, but if you want to have full coverage, maybe unregister.html can parameterize it by its hash and you can do unregister twice, once with #plural or #registrations instead.  You don't actually need to make that case work, but if you wanted to, we know from our test invariants that there's only one registration anyways.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1091,5 @@
>    }
>  
>    // Don't allow service workers to register when the *document* is chrome for
>    // now.
> +  MOZ_ASSERT(!nsContentUtils::IsSystemPrincipal(window->GetExtantDoc()->NodePrincipal()));

restating: code consolidation relying on window->GetExtantDoc() invariant.  If that doesn't hold, then this MOZ_ASSERT crashes in a different way for free.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Ben Kelly [:bkelly] from comment #32)
> Comment on attachment 8909799 [details] [diff] [review]
> P2 Fix the SW interception 3rd party cookie check to only apply to
> non-subresource requests and to properly check top window URI. r=smaug
> 
> Review of attachment 8909799 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, let's try this again.  I believe the correct thing to do here is use
> IsThirdPartyWindow() instead of what I was trying before:
> 
> http://searchfox.org/mozilla-central/source/netwerk/base/mozIThirdPartyUtil.
> idl#43
> 
> I call this with the parent window, if it exists, to compare to the
> requested URL.  IsThirdPartyWindow() will automatically walk the parent tree
> correctly.


Could you explain why we want this behavior, and not just compare to the uri of the parent?
Why we need to check all the ancestors?

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #41)
> Could you explain why we want this behavior, and not just compare to the uri
> of the parent?
> Why we need to check all the ancestors?

Essentially I want to match what we do for 3rd party cookie blocking today.  That is dictated by the algorithms in ThirdPartyUtils here:

http://searchfox.org/mozilla-central/source/netwerk/base/mozIThirdPartyUtil.idl#16
http://searchfox.org/mozilla-central/source/dom/base/ThirdPartyUtil.cpp#147

AFAICT these have checked all ancestors for at least the last 7 years.  I don't fully understand the rationale why, but I think that changing that would be a separate bug.

Service worker interception allows the 3rd party site to run code in the service worker with storage they could store an effective "super cookie" in IDB/Cache API/etc.  I think we need to match what our current cookie policy does elsewhere.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix the SW interception 3rd party cookie check to only apply to non-subresource requests and to properly check top window URI. r=smaug

Update this patch to use GetSameTypeParent() instead of GetParentDocshell().  Try showed that on non-e10s we were comparing to the browser.xul chrome window which is wrong.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Restrict service worker interception for more cookie policies. r=asuth

Rebase.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Expand test_third_party_iframes.html to verify worker scripts are intercepted. r=asuth

This test verifies that dedicated worker scripts are similarly intercepted or not depending on the cookie policy setting.  This coverage will help when we improve our service worker support of workers.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd6490676a5d7a472dd571c07e48f8c271c95fcd

Comment 46

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8909823 [details] [diff] [review]
P7 Add a mochitest to verify subresource fetch events still work with 3rd party iframe cookies disabled. r=asuth

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

Since this is just test_fetch_event.html with 2 lines added and it doesn't look like the "hg cp" meta-info is in here, it would be great to add a comment to the top of the file like:

This is just test_fetch_event.html but with an alternate cookie mode preference set to make sure that setting the preference does not break interception as observed in bug 1336364.
TODO: refactor this test so it doesn't duplicate so much code logic.

::: dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html
@@ +68,5 @@
> +        SimpleTest.finish();
> +      });
> +  }
> +
> +  const COOKIE_BEHAVIOR_REJECTFOREIGN = 1;

I think you can just use SpecialPowers.Components.interfaces.nsICookieService.BEHAVIOR_REJECT_FOREIGN below instead of adding this, though it is a tongue twister.  (Worse case, you might need to SpecialPowers.wrap(SpecialPowers.Components)?

Comment 47

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8909993 [details] [diff] [review]
P8 Expand test_third_party_iframes.html to verify worker scripts are intercepted. r=asuth

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

woo, Worker interception!

::: dom/workers/test/serviceworkers/thirdparty/sw.js
@@ +11,5 @@
>        headers: {'Content-Type': 'text/html'}
>      }));
> +    return;
> +  }
> +  if (event.request.url.indexOf("worker.js") >= 0) {

the more you know of the day, no changes needed: We've had ES2015 str.includes() since Firefox 40, previously it was contains() (18-48).

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Expand test_third_party_iframes.html to verify worker scripts are intercepted. r=asuth

Forgot to hg add the worker.js.  I also s/indexOf/includes/g as requested.

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Expand test_third_party_iframes.html to verify worker scripts are intercepted. r=asuth

There were more problems with the test.  Sorry, I must not have run it like I thought I did.  I'm keeping the r+, though, since I think you agree with the gist of it.  Feel free to re-review if you wish.

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Block storage denied windows from ServiceWorker.postMessage() and clients.matchAll(). r=asuth

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8910055 [details] [diff] [review]
P9 Block storage denied windows from ServiceWorker.postMessage() and clients.matchAll(). r=asuth

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

This patch adds some extra preventative checks to:

1. Clients.matchAll() and Clients.get().  We should hide storage-denied windows from these to avoid communication with SW.  (This would be easier with a principal attribute, but thats a bigger change.)
2. Fail ServiceWorker.postMessage() from a storage-denied window.  This should be impossible now since we block ways of getting a ServiceWorker, but lets do this just to be safe.

Its tricky to test these at the moment, so I'm probably not going to include test coverage here.

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec1387b8d480688db94e679aead6faa939e933ef

Comment 53

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8910055 [details] [diff] [review]
P9 Block storage denied windows from ServiceWorker.postMessage() and clients.matchAll(). r=asuth

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +3329,5 @@
> +  // access.  We don't want these to communicate.
> +  auto storageAccess =
> +    nsContentUtils::StorageAllowedForWindow(doc->GetInnerWindow());
> +  if (storageAccess != nsContentUtils::StorageAccess::eAllow) {
> +    return clientInfo;

Aside: This is a confusing convention for returning nullptr.  UniquePtr has a constructor like so...
  MOZ_IMPLICIT
  UniquePtr(decltype(nullptr))
    : mTuple(nullptr, DeleterType())
...that allows us to directly return nullptr.  Perhaps in the future we can clean this up.  No need to do it now.

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Expand test_third_party_iframes.html to verify worker scripts are intercepted. r=asuth

Another worker test problem found by try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11bf95a2696c593668d4d8db689ed8a2810755ab

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Add a mochitest to verify subresource fetch events still work with 3rd party iframe cookies disabled. r=asuth

Add the comment requested.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Expand test_third_party_iframes.html to verify worker scripts are intercepted. r=asuth

This is a more complete fix for that last try problem.  We should wait for the worker messages to come through in all cases so we don't race the worker against navigating to the 3rd party URL.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=334949e190c9a89afde5591f928d77c4b3cd59e5

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9caa08b97a7d
P1 Refact nsDocShell::ShouldPrepareForIntercept() to short-circuit on subresource instead of non-subresource. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/269f242c98dc
P2 Fix the SW interception 3rd party cookie check to only apply to non-subresource requests and to properly check top window URI. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4326596d4880
P3 Restrict service worker interception for more cookie policies. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/864019cce2c9
P4 Disallow service worker register() is storage is forbidden for the window. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/33e7c326e42f
P5 Disable service worker getRegistrations() if storage is disallowed for a window. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea1993fd683d
P6 Ensure that we don't control a document if its window cannot access storage. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/33e5d35a8df4
P7 Add a mochitest to verify subresource fetch events still work with 3rd party iframe cookies disabled. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d44c48ca49e
P8 Expand test_third_party_iframes.html to verify worker scripts are intercepted. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/776eb784874a
P9 Block storage denied windows from ServiceWorker.postMessage() and clients.matchAll(). r=asuth

Comment 58

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9caa08b97a7d
https://hg.mozilla.org/mozilla-central/rev/269f242c98dc
https://hg.mozilla.org/mozilla-central/rev/4326596d4880
https://hg.mozilla.org/mozilla-central/rev/864019cce2c9
https://hg.mozilla.org/mozilla-central/rev/33e7c326e42f
https://hg.mozilla.org/mozilla-central/rev/ea1993fd683d
https://hg.mozilla.org/mozilla-central/rev/33e5d35a8df4
https://hg.mozilla.org/mozilla-central/rev/6d44c48ca49e
https://hg.mozilla.org/mozilla-central/rev/776eb784874a