Closed Bug 1173467 Opened 5 years ago Closed 5 years ago

Cache API should at least reject in private browsing mode

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(5 files, 1 obsolete file)

While bug 1117808 is intended to solve private browsing for storage in the correct long term way, we need to at least block Cache usage in the short term.  For now, lets reject.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Modify how CacheStorage creation triggers SecurityErr.  Instead of throwing on the caches attribute access, instead create a CacheStorage that simply rejects all method calls.
Attachment #8626036 - Flags: review?(ehsan)
Some refactoring in nsGlobalWindow to make it easier to determine if we're in private browsing.
Attachment #8626037 - Flags: review?(ehsan)
Add flag on WorkerPrivate so we can determine off-main-thread if the worker was created in private browsing mode.
Attachment #8626038 - Flags: review?(ehsan)
Require passing private browsing flag to CacheStorage::CreateOnMainThread().  Read the WOrkerPrivate flag in the worker case.  For the constructor inspect the global's document for a load context to determine private browsing.

For the ServiceWorkerManager and worker ScriptLoader cases we simply prevent ServiceWorkers from loading in private browsing mode.  Then we assume all later ServiceWorkerManager usage of cache is not private browsing.  This is necessary because the ServiceWorkerRegistrationInfo and friends have no idea about private browsing.

Otherwise its fairly straightforward to get the flag passed in correctly.
Attachment #8626039 - Flags: review?(ehsan)
I will add a test tomorrow.
So the current sandboxed-iframes.https.html wpt test expects this to throw:

  self.caches

But the spec does not say this.  In fact, it now says that CacheStorage methods should do this:

  "If the result of running is settings object a secure context with the incumbent settings object is Not Secure, return a new promise rejected with a "SecurityError" exception."

Arguably a sandboxed iframe qualifies as failing this check.  So we should reject the promise instead of throwing from the global getter.

James, I would like to disable the test for now until I can get it fixed upstream in WPT.
Attachment #8626398 - Flags: review?(james)
Comment on attachment 8626398 [details] [diff] [review]
P1.5 Disable wpt sandboxed-iframes.https.html test until it can be updated. r=jgraham

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

Can you fix up the commit message to say that you're marking as expected fail rather than disabling?
Attachment #8626398 - Flags: review?(james) → review+
Comment on attachment 8626036 [details] [diff] [review]
P1 Modify CacheStorage to reject with SecurityErr instead of throwing on creation. r=ehsan

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

::: dom/cache/CacheStorage.cpp
@@ +174,5 @@
>  
> +CacheStorage::CacheStorage(nsresult aFailureResult)
> +  : mNamespace(INVALID_NAMESPACE)
> +  , mActor(nullptr)
> +  , mStatus(aFailureResult)

Please initialize the rest of the members as well.
Attachment #8626036 - Flags: review?(ehsan) → review+
Comment on attachment 8626037 [details] [diff] [review]
P2 Add nsGlobalWindow utility method to get private browsing boolean. r=ehsan

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

This is fine, but we already have a nsContentUtils::IsInPrivateBrowsing helper which takes a document.  It would be much nicer if you added an overload that takes a window, but if you don't want to do that in this bug, it can wait for a follow-up.
Attachment #8626037 - Flags: review?(ehsan) → review+
See Also: → 1177621
Comment on attachment 8626038 [details] [diff] [review]
P3 Add a WorkerPrivate private browsing flag. r=ehsan

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

It seems like right now, if there is a private and a non-private window from the same origin and they both try to create a shared worker, they will get the same shared worker, which is broken.  I filed bug 1177621 about that and baku promised to fix!

But about this patch, this flag will be wrong depending on who gets to create a shared worker first.  I personally think since we're going to fix bug 1177621 soon, that is not a huge issue, but please ask a worker peer about this.
Attachment #8626038 - Flags: review?(ehsan) → feedback+
Comment on attachment 8626038 [details] [diff] [review]
P3 Add a WorkerPrivate private browsing flag. r=ehsan

Kyle, can you look at this one for me tomorrow?

Basically I need to know if a particular worker is for a private browsing session or not.  I set a flag based on the nsILoadGroup context used to create the worker.

Ehsan correctly notes that this is a bit broken for SharedWorker since a private and non-private window could attach to the same worker.

Ehsan wrote bug 1177621 to get the SharedWorker sharing to be fixed.  Since that will be worked separately, are you ok with this flag landing in the meantime?

(Note, we do not plan on supporting ServiceWorker interception for private browsing.)
Attachment #8626038 - Flags: review?(khuey)
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #10)
> Comment on attachment 8626036 [details] [diff] [review]
> P1 Modify CacheStorage to reject with SecurityErr instead of throwing on
> creation. r=ehsan
> 
> Review of attachment 8626036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/CacheStorage.cpp
> @@ +174,5 @@
> >  
> > +CacheStorage::CacheStorage(nsresult aFailureResult)
> > +  : mNamespace(INVALID_NAMESPACE)
> > +  , mActor(nullptr)
> > +  , mStatus(aFailureResult)
> 
> Please initialize the rest of the members as well.

I believe all of the other members have default values.  Their types are nsCOMPtr, UniquePtr, nsRefPtr, and nsTArray.

Correct if I'm wrong, but typically we don't require explicit initialization of these types, right?
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #11)
> Comment on attachment 8626037 [details] [diff] [review]
> P2 Add nsGlobalWindow utility method to get private browsing boolean. r=ehsan
> 
> Review of attachment 8626037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine, but we already have a nsContentUtils::IsInPrivateBrowsing
> helper which takes a document.  It would be much nicer if you added an
> overload that takes a window, but if you don't want to do that in this bug,
> it can wait for a follow-up.

Well, I was trying to just de-dupe code that was in this file.  I'd prefer to avoid more wide-ranging refactoring right now if I can.
(In reply to Ben Kelly [:bkelly] from comment #14)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #10)
> > Comment on attachment 8626036 [details] [diff] [review]
> > P1 Modify CacheStorage to reject with SecurityErr instead of throwing on
> > creation. r=ehsan
> > 
> > Review of attachment 8626036 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/cache/CacheStorage.cpp
> > @@ +174,5 @@
> > >  
> > > +CacheStorage::CacheStorage(nsresult aFailureResult)
> > > +  : mNamespace(INVALID_NAMESPACE)
> > > +  , mActor(nullptr)
> > > +  , mStatus(aFailureResult)
> > 
> > Please initialize the rest of the members as well.
> 
> I believe all of the other members have default values.  Their types are
> nsCOMPtr, UniquePtr, nsRefPtr, and nsTArray.
> 
> Correct if I'm wrong, but typically we don't require explicit initialization
> of these types, right?

No you're right, my mistake!

(In reply to Ben Kelly [:bkelly] from comment #15)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #11)
> > Comment on attachment 8626037 [details] [diff] [review]
> > P2 Add nsGlobalWindow utility method to get private browsing boolean. r=ehsan
> > 
> > Review of attachment 8626037 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is fine, but we already have a nsContentUtils::IsInPrivateBrowsing
> > helper which takes a document.  It would be much nicer if you added an
> > overload that takes a window, but if you don't want to do that in this bug,
> > it can wait for a follow-up.
> 
> Well, I was trying to just de-dupe code that was in this file.  I'd prefer
> to avoid more wide-ranging refactoring right now if I can.

OK, that's fine.
Attachment #8626039 - Flags: review?(ehsan) → review+
Comment on attachment 8626350 [details] [diff] [review]
P5 Add a test to validate Cache in private browsing window. r=ehsan

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

::: dom/cache/test/mochitest/browser_cache_pb_window.js
@@ +61,5 @@
> +}
> +
> +function test() {
> +  waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({'set': [['browser.privatebrowing.autostart', true],

You don't need to set this pref.  This turns on permanent private browsing mode which requires a restart to take effect, and doesn't help with this test anyway.
Attachment #8626350 - Flags: review?(ehsan) → review+
Comment on attachment 8626038 [details] [diff] [review]
P3 Add a WorkerPrivate private browsing flag. r=ehsan

Obsolete this patch in favor of Andrea's solution.
Attachment #8626038 - Attachment is obsolete: true
> ::: dom/cache/test/mochitest/browser_cache_pb_window.js
> > +function test() {
> > +  waitForExplicitFinish();
> > +  SpecialPowers.pushPrefEnv({'set': [['browser.privatebrowing.autostart', true],
> 
> You don't need to set this pref.  This turns on permanent private browsing
> mode which requires a restart to take effect, and doesn't help with this
> test anyway.

I'm sorry, but I missed removing this.  I'll do it in a follow-up.
Blocks: 1177965
This had to be backed out for:

Assertion failure: NS_IsMainThread(), at /builds/slave/m-in-lx-d-00000000000000000000/build/src/dom/bindings/BindingUtils.cpp:831
TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_app_protocol.html | application terminated with exit code 11
PROCESS-CRASH | dom/workers/test/serviceworkers/test_app_protocol.html | application crashed [@ mozilla::dom::NativeInterface2JSObjectAndThrowIfFailed(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>, xpcObjectHelper&, nsID const*, bool)]
Looking at this:

  https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=30cefdf8d020

I think its pretty clear this failure was not caused by this bug.  The stack suggests its URLSearchParams which was bug 1177916 (and was also backed out).

I intend to reland this later tonight.
I think this landing has made https://bugzilla.mozilla.org/show_bug.cgi?id=1174977 more intermittent. I'm not confident enough to back it out, but definitely worth a look before it gets merged around.
(In reply to nigelbabu@gmail.com [:nigelb] from comment #26)
> I think this landing has made
> https://bugzilla.mozilla.org/show_bug.cgi?id=1174977 more intermittent. I'm
> not confident enough to back it out, but definitely worth a look before it
> gets merged around.

The "cache" in bug 1174977 is unrelated to the Cache API here.  The dom/cache and dom/workers/test/serviceworkers tests don't run in the same chunk as the test in bug 1174977 either.  I don't see how these two could be connected.  Also, bug 1174977 has been extremely frequent and intermittent for weeks now.

Please talk to me before backing out over this issue.
I figured it was actually just intermittent rather than anything to do with your landing. Eventually confirmed with Tomcat that it was definitely not your fault.

Thanks for the detailed explanation :)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.