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

NEW
Unassigned

Status

()

P3
normal
5 years ago
8 months ago

People

(Reporter: akligman, Unassigned)

Tracking

({privacy})

26 Branch
privacy
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
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

5 years ago
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.
(Reporter)

Comment 3

5 years ago
Having an option in about:permissions to control this would be nice.
(Reporter)

Updated

5 years ago
Flags: needinfo?(mmc)
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?
Flags: needinfo?(mmc) → needinfo?(jonas)
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.
Flags: needinfo?(jonas)
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.
Summary: Allow IndexedDB in third-party iframes → Unify site-specific and third party permission across all forms of local storage
(Reporter)

Comment 7

5 years ago
In the short-term, can we enable indexeddb in third-party iframes when third-party cookies are enabled?
(Reporter)

Comment 8

4 years ago
See comment above.
Flags: needinfo?(mmc)
Jonas owns indexdb.
Flags: needinfo?(mmc) → needinfo?(jonas)
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.
Flags: needinfo?(jonas) → needinfo?(bent.mozilla)
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.
Flags: needinfo?(bent.mozilla)
(Reporter)

Comment 12

4 years ago
To clarify, we want allow IndexedDB for a fist-party domain even when cookies are disabled?
(Reporter)

Updated

4 years ago
Flags: needinfo?(jonas)
(Reporter)

Comment 13

4 years ago
Created attachment 8472295 [details] [diff] [review]
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.)
Attachment #8472295 - Flags: feedback?(jonas)
Attachment #8472295 - Flags: feedback?(bent.mozilla)
(Reporter)

Comment 14

4 years ago
The changes to CookieServiceChild are trivial, but the patch seems to work. Not sure what needs to be done there (WIP).
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.
Attachment #8472295 - Flags: feedback?(jonas) → feedback+
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.
Flags: needinfo?(jonas)
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
Attachment #8472295 - Flags: feedback?(bent.mozilla) → feedback+
(Reporter)

Comment 18

4 years ago
Created attachment 8479317 [details] [diff] [review]
idb-third-party-iframes.patch

Removed whitespace changes from patch.
Assignee: nobody → akligman
Attachment #8472295 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Reporter)

Comment 19

4 years ago
Created attachment 8485369 [details] [diff] [review]
0001-Bug-912202-Unify-site-specific-and-third-party-permi.patch
Attachment #8479317 - Attachment is obsolete: true
(Reporter)

Comment 20

4 years ago
Created attachment 8501961 [details] [diff] [review]
0001-Bug-912202-Unify-site-specific-and-third-party-permi.patch
Attachment #8485369 - Attachment is obsolete: true
(Reporter)

Comment 21

4 years ago
Created attachment 8501963 [details] [diff] [review]
0001-Bug-912202-Unify-site-specific-and-third-party-permi.patch
Attachment #8501961 - Attachment is obsolete: true
Pretty sure Alan has moved on.
Assignee: akligman → nobody
Status: ASSIGNED → NEW

Comment 23

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

Updated

8 months ago
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.