1137245 - ServiceWorkerManager should set WorkerPrivate::LoadInfo::mIndexedDBAllowed correctly

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Current ServiceWorkerManager::CreateServiceWorker() manually creates a WorkerPrivate::LoadInfo structure.  If we want to allow IDB in ServiceWorkers (which we do), then we need to set mIndexedDBAllowed correctly when we create the LoadInfo.

Current IDBFactory has an IsAllowedForWindow() call, but the ServiceWorker does not have a window.  We do have a principal, though, which seems like it should be enough to do this check.  We probably need an IDBFactory IsAllowedForPrincipal() check.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

If anyone wants to attempt this bug, you just need to do something like this:

1) Add AllowedForPrincipal() in dom/indexeddb/IDBFactory.(h|cpp).  This would just to the principal oriented checks currently in AllowedForWindow().
2) Modify dom/workers/ServiceWorkerManager.cpp to call IDBFactory::AllowedForPrincipal() in the places where it creates a LoadInfo struct.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8571067 [details] [diff] [review]
v1

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

This looks good to me!  If you think its ready, I recommend getting a try build and flagging bent for review.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8571067 [details] [diff] [review]
v1

Thank you Ben.

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This patch actually raises some interesting questions.  We currently block using IDB in sandboxed iframes and third party documents.  Will those hit a ServiceWorker when they do loads?  If so, this would allow that to be bypassed.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> This patch actually raises some interesting questions.  We currently block
> using IDB in sandboxed iframes and third party documents.  Will those hit a
> ServiceWorker when they do loads?  If so, this would allow that to be
> bypassed.

I believe they will and we currently expose Cache as a similar storage primitive.  If that's not ok, then we should probably just disallow ServiceWorkers in this case.  I can't find anything in the spec that says we should do that, though.

What is the rationale behind disallowing IDB in these cases?

Nikhil, can you confirm if we will trigger a ServiceWorker for sandboxed iframes and third party windows?

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Ben Kelly [:bkelly] from comment #7)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> > This patch actually raises some interesting questions.  We currently block
> > using IDB in sandboxed iframes and third party documents.  Will those hit a
> > ServiceWorker when they do loads?  If so, this would allow that to be
> > bypassed.
> 
> I believe they will and we currently expose Cache as a similar storage
> primitive.  If that's not ok, then we should probably just disallow
> ServiceWorkers in this case.  I can't find anything in the spec that says we
> should do that, though.
> 
> What is the rationale behind disallowing IDB in these cases?
> 
> Nikhil, can you confirm if we will trigger a ServiceWorker for sandboxed
> iframes and third party windows?

A ServiceWorker intercepts requests created from same-origin iframes/windows. That request may be to a cross-origin resource.
It does not intercept loads where the iframe/window is from another origin.

According to jdm, we may be intercepting sandboxed iframes.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #8)
> According to jdm, we may be intercepting sandboxed iframes.

But the sandboxed iframe would still have to be same-origin to get intercepted, right?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

It seems the iframe would have to opt in to <iframe sandbox="allow-same-origin"> to get intercepted based on origin.  Perhaps we should block interception if allow-scripts is not also set.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

I opened a spec issue regarding sandboxed iframes:

  https://github.com/slightlyoff/ServiceWorker/issues/648

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #8)
> A ServiceWorker intercepts requests created from same-origin
> iframes/windows. That request may be to a cross-origin resource.
> It does not intercept loads where the iframe/window is from another origin.

Its possible, though, that there is a service worker registered for a different origin that would intercept a third party window.  For example:

1) User visits facebook.com normally and a SW is registered for facebook.com.
2) User visits site foo.com with an embedded "like button" iframe
3) The "like button" iframe would be intercepted by the SW registered in (1)

Right?

I think this is by design of the spec.

Comment 13

[(no longer active)]

The current plan is to disallow the interception of third-party iframes when the network.cookie.cookieBehavior pref is set to 1, and to allow the usage of IDB in service workers unconditionally.  That should effectively disallow the usage of IDB in SW when the user has chosen to not allow third party websites to persist information on the disk.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8571067 [details] [diff] [review]
v1

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

Thanks for diving in here Fernando! Your patch works just fine, but I'd prefer we integrate it more tightly with the existing code:

::: dom/indexedDB/IDBFactory.cpp
@@ +424,5 @@
>  }
>  
> +// static
> +bool
> +IDBFactory::AllowedForPrincipal(nsIPrincipal *aPrincipal)

AllowedForWindowInternal grabs a principal off the window, so in my mind it should then pass that principal to AllowedForPrincipal. Then you can move some of the other checks into the AllowedForPrincipal call, and we have a common flow of execution.

Make sense?

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #13)
> The current plan is to disallow the interception of third-party iframes when
> the network.cookie.cookieBehavior pref is set to 1, and to allow the usage
> of IDB in service workers unconditionally. 

Did a bug get filed for that? We shouldn't land this fix until that one gets fixed I think...

Comment 16

[(no longer active)]

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #15)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #13)
> > The current plan is to disallow the interception of third-party iframes when
> > the network.cookie.cookieBehavior pref is set to 1, and to allow the usage
> > of IDB in service workers unconditionally. 
> 
> Did a bug get filed for that? We shouldn't land this fix until that one gets
> fixed I think...

I just filed bug 1152899.

Fernando, would you like to take that bug, since that blocks this one?

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Sure

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Thanks for your feedback Ben! I think this patch addresses your review comment. Now we are calling 'AllowedForPrincipal' from 'AllowedForWindowInternal'.

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8591554 [details] [diff] [review]
v2

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

Yep, this looks almost perfect, but can you move the handling of the system principal into the new function too?

::: dom/indexedDB/IDBFactory.cpp
@@ +431,5 @@
> +
> +  bool isNullPrincipal;
> +  if (NS_WARN_IF(NS_FAILED(aPrincipal->GetIsNullPrincipal(&isNullPrincipal))) ||
> +      isNullPrincipal ||
> +      NS_WARN_IF(!IndexedDatabaseManager::GetOrCreate())) {

Let's move this check to the beginning of the function (similar to AllowedForWindowInternal())

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8607075 [details] [diff] [review]
v3

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

Thanks, this looks good!

::: dom/indexedDB/IDBFactory.h
@@ +105,5 @@
>    static bool
>    AllowedForWindow(nsPIDOMWindow* aWindow);
>  
> +  static bool
> +  AllowedForPrincipal(nsIPrincipal* aPrincipal, bool* aIsSystemPrincipal);

I might make this:

  static bool
  AllowedForPrincipal(nsIPrincipal* aPrincipal, bool* aIsSystemPrincipal = nullptr);

Then you can optionally pass a bool* here if you care (as IDBFactory does) or not (since ServiceWorkerManager doesn't care).

Then just be careful to nullcheck in the implementation.

Comment 23

[Noemí Freire (:noemi)]

José Antonio could you please check if the crash reported in Bug 1141539 is still reproduce when applying the patch (v3) available in this bug?. Thanks!

Comment 24

[José Antonio Olivera Ortega [:jaoo]]

Well the crash is still there as bug 1141539 is resolved as a DUP of bug 1132436 which is not resolved yet. The bug here is fixed by the patch ferjm provided (thanks!) so IndexedDB is allowed in service workers now.

F/MOZ_Assert( 2041): Assertion failure: NS_HasPendingEvents(mThread), at /Volumes/firefoxos/dev/mozilla-central/dom/workers/WorkerPrivate.cpp:5279
F/libc    ( 2041): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1), thread 2581 (DOM Worker)

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/219780d1c8fd

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/219780d1c8fd