Open Bug 912202 Opened 11 years ago Updated 2 years ago

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

Categories

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

26 Branch
defect

Tracking

()

People

(Reporter: akligman, Unassigned)

References

Details

(Keywords: privacy)

Attachments

(1 file, 4 obsolete files)

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
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
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.
Having an option in about:permissions to control this would be nice.
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
In the short-term, can we enable indexeddb in third-party iframes when third-party cookies are enabled?
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)
To clarify, we want allow IndexedDB for a fist-party domain even when cookies are disabled?
Flags: needinfo?(jonas)
Attached patch idb-third-party-iframes.patch (obsolete) (deleted) — Splinter Review
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)
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+
Attached patch idb-third-party-iframes.patch (obsolete) (deleted) — Splinter Review
Removed whitespace changes from patch.
Assignee: nobody → akligman
Attachment #8472295 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8479317 - Attachment is obsolete: true
Attachment #8485369 - Attachment is obsolete: true
Attachment #8501961 - Attachment is obsolete: true
Pretty sure Alan has moved on.
Assignee: akligman → nobody
Status: ASSIGNED → NEW
Still interested in seeing this fixed, but I no longer have time to do the work.
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: