912202 - Unify site-specific and third party permission across all forms of local storage

Status: NEW

(Core :: Storage: IndexedDB, defect, P3)

Description

[Alan K [:ack]]

The current policy of blocking all access to IndexedDB from third-party iframes puts excessive limits on composability of web apps. Denying access by default is fine, as long as there is a way for the user to add exceptions for third-party domains that they trust.

Related: #595307

Comment 1

[Brian Kardell]

Sanboxed iframes with allow-same-origin, specifically, should be allowed or an alternative standard mechanism proposed.  the disadvantage of the later is that the former is per spec and implemented elsewhere

Comment 2

[simon.kaegi]

This restriction is also causing my team (Orion) problems as we use iframes to let third parties contribute functionality back to our browser-based IDE. In particular this prevents our plugin authors from providing some basic offline support on FF only.

We're using sandboxed iframes and could also do something on the iframe side if some sort of trust handshake was needed.

Comment 3

[Alan K [:ack]]

Having an option in about:permissions to control this would be nice.

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

I would welcome unification of site-specific permissions and third-party permissions across all forms of local storage. The current site-specific permissions are a mess, and it's very confusing for developers to have cookie and indexdb requests treated differently.

Jonas, what do you think?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Agreed! Would you be able to work on this, or know of anyone who would? If so we could get together an come up with a comprehensive design.

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Ok, good :) I wouldn't be able to work on this anytime soon, but we should come up with a plan anyway -- I may be able to recruit someone who has capacity.

Comment 7

[Alan K [:ack]]

In the short-term, can we enable indexeddb in third-party iframes when third-party cookies are enabled?

Comment 8

[Alan K [:ack]]

See comment above.

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Jonas owns indexdb.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't actually. Ben Turner does these days.

But yes, I agree that we should enable IndexedDB in cross-origin iframes when third-party cookies are enabled.

Comment 11

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

We're all in basic agreement that all forms of temporary storage should behave the same as cookies. ack is going to take a look here.

Comment 12

[Alan K [:ack]]

To clarify, we want allow IndexedDB for a fist-party domain even when cookies are disabled?

Comment 13

[Alan K [:ack]]

Attachment: idb-third-party-iframes.patch

Added a check for cookie permissions when testing third-partyness. Check is essentially the same as the one for third-party cookies.

(Ignore the whitespace changes.)

Comment 14

[Alan K [:ack]]

The changes to CookieServiceChild are trivial, but the patch seems to work. Not sure what needs to be done there (WIP).

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8472295 [details] [diff] [review]
idb-third-party-iframes.patch

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

::: netwerk/cookie/CookieServiceChild.cpp
@@ +213,5 @@
> +CookieServiceChild::CheckCookiesAllowed(nsIURI      *aHostURI,
> +                                        nsIChannel  *aChannel,
> +                                        bool        *result)
> +{
> +  return NS_OK;

This doesn't seem right. Note that FirefoxOS uses child processes a lot and we want to make sure that this bug is fixed there too.

::: netwerk/cookie/nsCookieService.cpp
@@ +1638,5 @@
>  
>  NS_IMETHODIMP
> +nsCookieService::CheckCookiesAllowed(nsIURI     *aHostURI,
> +                                     nsIChannel *aChannel,
> +                                     bool       *result)

Someone other than me needs to look at this.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think that if cookies are completely disabled, we should also disable IndexedDB. Eventually we should enable IndexedDB in the persistent storage area even when cookies are disabled, but let's not worry about that for now.

Comment 17

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

Comment on attachment 8472295 [details] [diff] [review]
idb-third-party-iframes.patch

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

This looks generally fine, but I agree with sicking that the child process impl has to work too.

Also, your editor has made this patch hard to follow with all the unrelated whitespace changes. You'll want to revert those before getting a full review.

::: dom/base/nsGlobalWindow.cpp
@@ +10639,5 @@
>          bool isThirdParty;
>          aError = thirdPartyUtil->IsThirdPartyWindow(this, nullptr,
>                                                      &isThirdParty);
> +
> +        if (isThirdParty) {

You're not checking aError.Failed() here, 'isThirdParty' could be uninitialized.

@@ +10644,5 @@
> +          if (!codebaseURI) {
> +            // Document's principal is not a codebase (may be system), so
> +            // can't set cookies
> +
> +            aError.Throw(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

Previously we didn't throw but just made the return value null. I'd keep that approach.

@@ +10648,5 @@
> +            aError.Throw(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +            return nullptr;
> +          }
> +
> +          // not having a cookie service isn't an error

Really? In what cases would we not have a cookie service?

@@ +10650,5 @@
> +          }
> +
> +          // not having a cookie service isn't an error
> +          nsCOMPtr<nsICookieService> service = do_GetService(NS_COOKIESERVICE_CONTRACTID);
> +          nsIURI* documentURI = GetDocument()->GetDocumentURI();

What happens if documentURI is null?

::: netwerk/cookie/nsCookieService.cpp
@@ +1638,5 @@
>  
>  NS_IMETHODIMP
> +nsCookieService::CheckCookiesAllowed(nsIURI     *aHostURI,
> +                                     nsIChannel *aChannel,
> +                                     bool       *result)

I'd suggest someone in network, jduell or jdm perhaps

Comment 18

[Alan K [:ack]]

Attachment: idb-third-party-iframes.patch

Removed whitespace changes from patch.

Comment 19

[Alan K [:ack]]

Attachment: 0001-Bug-912202-Unify-site-specific-and-third-party-permi.patch

Comment 20

[Alan K [:ack]]

Attachment: 0001-Bug-912202-Unify-site-specific-and-third-party-permi.patch

Comment 21

[Alan K [:ack]]

Attachment: 0001-Bug-912202-Unify-site-specific-and-third-party-permi.patch

Comment 22

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

Pretty sure Alan has moved on.

Comment 23

[Alan Kligman]

Still interested in seeing this fixed, but I no longer have time to do the work.