Closed Bug 1112134 Opened 9 years ago Closed 2 years ago

CacheStorage/Cache should require SecureContext

Categories

(Core :: Storage: Cache API, defect, P2)

defect

Tracking

()

RESOLVED FIXED
103 Branch
Tracking Status
firefox103 --- fixed

People

(Reporter: bkelly, Assigned: saschanaz)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: DWS_NEXT)

Attachments

(3 files, 4 obsolete files)

Currently the window.caches and worker self.caches globals will throw if you attempt to use them from an illegal principal.  We should instead detect this condition in a webidl [Func] method and hide the globals.  This will allow normal feature detection to work.
I believe we should also hide caches when in private browsing mode.
(In reply to Ben Kelly [:bkelly] from comment #1)
> I believe we should also hide caches when in private browsing mode.

Nope.  We should just ensure that we don't write anything to the disk when caches is used in a page that is loaded in a PB window.
(Note that we have gone to great lengths to make sure pages cannot easily determine whether you are in PB mode or not.)
What is cache supposed to do in PB mode then?  Its an API for writing to disk.

The previous comments in this bug are basically things Jonas told me I need to do.
Flags: needinfo?(jonas)
Flags: needinfo?(ehsan)
More specifically, what should cache.put() do in PB mode?
I don't have any strong feelings about if we hide these properties or just make sure that they don't write to disk.

Though I think the IndexedDB code might hide the IDB API when in PB mode. So sounds like that should be fixed too then.
Flags: needinfo?(jonas)
Well, I'm waiting for someone to define "don't write to disk".

If this means "provide a second, memory-backed implementation solely for use in PB mode", then I think I might have some objections:

1) A memory-backed implementation is non-trivial.  It would have to maintain all semantics including stored values being visible across windows and workers within the same origin (within the same PB session).
2) Content is going to use Cache with the (reasonable) assumption that it has a small memory footprint.  After all, its storing things on disk.  A memory-backed implementation would force everything into memory.  Sites that work well in normal browsing might be more prone to OOMs in PB mode.

If we really want to support this, then I think the better way to do this would be to fix it at the QuotaManager level.  When in PB mode, default storage would point to a separate, per-session directory.  QuotaManager would then delete the directory once the session ends.

Jonas, what do you think of this approach?
Flags: needinfo?(jonas)
Yup, I share your concerns.

I think using the QM approach sounds like a good solution. Though we'd need to make sure to clear that area is also cleared on startup in case we crashed while in PB.

Do OSs provide any help here? Can we get a memory backed filesystem which automatically swaps to disk?

It's really Ehsan's call here though since he has been more involved in PB. But effort of implementation does put limits on what we can do.
Flags: needinfo?(jonas)
Writing to a temporary directory in PB mode is not good enough since we won't be able to clean up after ourselves if we crash or otherwise don't go through the cleanup code.  It also leaves the data prone to examination when Firefox is running.

Can we open the files we write to using nsIFile::DELETE_ON_CLOSE?  That should do the right thing both on Windows and *nix, by making sure the file won't survive us closing it, but it would mean that we wouldn't be able to close the fd unless we're sure we're done with it.  We will also need to open a memory backed sqlite database, which is relatively easy.  If we have a good way of not closing the fd's too soon, this seems to me like a viable implementation approach.

The desired semantics should be to keep the caches API working in PB windows, but the cached content would be separate than the cached content in non-PB windows, and after the last PB window is closed, the cached content will go away forever.  To a website, it will seem as if the user has cleared the cache after closing the web page.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> Can we open the files we write to using nsIFile::DELETE_ON_CLOSE?  That
> should do the right thing both on Windows and *nix, by making sure the file
> won't survive us closing it, but it would mean that we wouldn't be able to
> close the fd unless we're sure we're done with it.  We will also need to
> open a memory backed sqlite database, which is relatively easy.  If we have
> a good way of not closing the fd's too soon, this seems to me like a viable
> implementation approach.

Cache only stores some data in sqlite.  It also uses flat files.  So memory sqlite is not enough here.

Close-on-delete is also inadequate without a major redesign of Cache.  Because a file is not kept open the entire time its in the cache.  Content can reasonably expect to put something in the cache, not touch it for a while, and then get it back out.  We don't hold the file open this whole time.  And if we delete immediately then that's pretty observable by content.

Also, delete-on-close does not work reliably on windows.  You can only delete a file after all handles to it have been closed.  Our file streams try to make this work, but its only best effort:

  http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsFileStreams.cpp?from=nsFileStreams.cpp&case=true#431
  http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsFileStreams.cpp?from=nsFileStreams.cpp&case=true#477

> The desired semantics should be to keep the caches API working in PB
> windows, but the cached content would be separate than the cached content in
> non-PB windows, and after the last PB window is closed, the cached content
> will go away forever.  To a website, it will seem as if the user has cleared
> the cache after closing the web page.

Keep in mind people will be storing things in Cache required to keep an entire site offline.  This is more than is rendered on any single page.  So it will be a large footprint to hold this in memory.

Further, I expect people will (reasonable) use Cache to make things like photo galleries, music players, and things like dropbox offline.  These could end up storing huge amounts of data in Cache.  This will work just fine on disk, but will rapidly OOM.

Also, all of this extra memory will be in the *parent* process.  Where OOM'ing is extra bad.

We could put some kind of arbitrary limit on how much can be stored in memory, but of course that would then be observable by content.

I think this is a non-trivial change and will take some time to think through.  Had I known this was a requirement up front, I would have designed things differently.

Ehsan, can you open a separate bug to implement a non-disk Cache for PB mode.  If you think it should block pref'ing Cache on, then make it block bug 1110144.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> Writing to a temporary directory in PB mode is not good enough since we
> won't be able to clean up after ourselves if we crash or otherwise don't go
> through the cleanup code.  It also leaves the data prone to examination when
> Firefox is running.

Also, what level of guarantee do we actually provide for features like PB and the new "forget"?  For example, I doubt "forget" deletes files such that they can't be forensically recovered from the disk.  If its between crashing the whole browser from an OOM vs writing a temp file that might be recoverable, do we really want to favor the OOM?  (Keep in mind OOM'ing may lose data in other non-PB tabs.)
(In reply to Ben Kelly (PTO, back Mon Jan 5) [:bkelly] from comment #11)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #9)
> > Writing to a temporary directory in PB mode is not good enough since we
> > won't be able to clean up after ourselves if we crash or otherwise don't go
> > through the cleanup code.  It also leaves the data prone to examination when
> > Firefox is running.
> 
> Also, what level of guarantee do we actually provide for features like PB
> and the new "forget"?

I don't know about the new "forget" feature, but for PB we do not attempt to protect against forensic level analysis.  We do attempt to protect against filesystem level analysis, IOW we ensure that the data represented by the filesystem would not reflect your browsing history (that is, we do not store anything on the filesystem that can give away where you have browsed.)

> For example, I doubt "forget" deletes files such that
> they can't be forensically recovered from the disk.  If its between crashing
> the whole browser from an OOM vs writing a temp file that might be
> recoverable, do we really want to favor the OOM?  (Keep in mind OOM'ing may
> lose data in other non-PB tabs.)

We should never favor an OOM, obviously.  :-)  Let's take a step back.

How about this:

1. Ensure that if you access the cache API from within a private session, we will not return anything from the "normal" cache.  That is, caches should give you the illusion that there is nothing stored from the origin initially.
2. Pretend that an origin that accesses the API from a private session has 0 amount of storage left from the quota manager's perspective, and thus fail any request that tries to put anything into the cache.

This will obviously make it possible to detect whether you are in PB mode, but I guess that ship has sailed with IDB (and of course with other side channel attacks that make it possible to detect this today.)  But at least it keeps the same API surface exposed to content, so if (when) sites do stupid stuff such as assuming Firefox N+ has caches implemented, they won't break when loaded in PB mode.

I really can't think of another easier to implement solution, but I don't think we should just hide the API entry points in PB mode.  Do you think the above is a feasible solution?

Also, can you please look to see what Chrome has done here?  My Chrome doesn't seem to have window.caches at all, not sure what I need to do in order to examine it myself.
Flags: needinfo?(ehsan)
This sounds ok to me. Ideally we could make the QuotaManager grant 0 bytes of temporary storage to PB pages.

It sounds like that shouldn't be too complex implementation-wise, but Ben needs to confirm.
Blocks: 1117808
Lets move the private browsing discussion over to bug 1117808.
I took a stab at this, but I'm hitting a problem.  By the time that we're setting up the worker global, it seems that the WorkerPrivate's PrincipalInfo is not initialized, so we end up crashing when trying to access it.  The stack looks like this:

* thread #63: tid = 0x6f992, 0x0000000102d92b2c XUL`mozilla::ipc::PrincipalInfo::type(this=0x0000000000000000) const + 12 at PBackgroundSharedTypes.h:312, name = 'DOM Worker', stop reason = EXC_BAD_ACCESS (code=1, address=0x18)
  * frame #0: 0x0000000102d92b2c XUL`mozilla::ipc::PrincipalInfo::type(this=0x0000000000000000) const + 12 at PBackgroundSharedTypes.h:312
    frame #1: 0x0000000104ed3705 XUL`mozilla::dom::cache::CacheStorage::EnabledOnPrincipal(aPrincipalInfo=0x0000000000000000) + 21 at CacheStorage.cpp:128
    frame #2: 0x0000000104ed36cf XUL`mozilla::dom::cache::CacheStorage::DOMCacheEnabled(aCx=0x0000000130a47880, aObject=0x0000000136603060) + 143 at CacheStorage.cpp:97
    frame #3: 0x0000000104ec48fb XUL`mozilla::dom::Prefable<JSPropertySpec const>::isEnabled(this=0x000000010b53ebc8, cx=0x0000000130a47880, obj=0x0000000136604080) const + 235 at DOMJSClass.h:64
    frame #4: 0x0000000104ec112c XUL`bool mozilla::dom::DefinePrefable<JSPropertySpec const>(cx=0x0000000130a47880, obj=Handle<JSObject *> at 0x00000001364427d0, props=0x000000010b53ebc8) + 252 at BindingUtils.cpp:345
    frame #5: 0x0000000104eacb55 XUL`mozilla::dom::DefineProperties(cx=0x0000000130a47880, obj=Handle<JSObject *> at 0x0000000136442840, properties=0x000000010b22beb8, chromeOnlyProperties=0x0000000000000000) + 149 at BindingUtils.cpp:637
    frame #6: 0x0000000104ead639 XUL`mozilla::dom::CreateInterfacePrototypeObject(cx=0x0000000130a47880, global=Handle<JSObject *> at 0x00000001364428f0, parentProto=Handle<JSObject *> at 0x00000001364428e8, protoClass=0x000000010b22bc08, properties=0x000000010b22beb8, chromeOnlyProperties=0x0000000000000000) + 217 at BindingUtils.cpp:618
    frame #7: 0x0000000104ead273 XUL`mozilla::dom::CreateInterfaceObjects(cx=0x0000000130a47880, global=Handle<JSObject *> at 0x0000000136442ab0, protoProto=Handle<JSObject *> at 0x0000000136442aa8, protoClass=0x000000010b22bc08, protoCache=0x000000011ad85af0, constructorProto=Handle<JSObject *> at 0x0000000136442aa0, constructorClass=0x000000010b22bd60, constructor=0x0000000000000000, ctorNargs=0, namedConstructors=0x0000000000000000, constructorCache=0x000000011ad85bd0, properties=0x000000010b22beb8, chromeOnlyProperties=0x0000000000000000, name=0x0000000108ab7f08, defineOnGlobal=true) + 1491 at BindingUtils.cpp:707
    frame #8: 0x0000000104986269 XUL`mozilla::dom::WorkerGlobalScopeBinding_workers::CreateInterfaceObjects(aCx=0x0000000130a47880, aGlobal=Handle<JSObject *> at 0x0000000136442bc0, aProtoAndIfaceCache=0x0000000118020cb0, aDefineOnGlobal=true) + 441 at WorkerGlobalScopeBinding.cpp:1375
    frame #9: 0x0000000104986321 XUL`mozilla::dom::WorkerGlobalScopeBinding_workers::GetProtoObjectHandle(aCx=0x0000000130a47880, aGlobal=Handle<JSObject *> at 0x0000000136442c20) + 161 at WorkerGlobalScopeBinding.cpp:1399
    frame #10: 0x0000000104ba0216 XUL`mozilla::dom::DedicatedWorkerGlobalScopeBinding_workers::CreateInterfaceObjects(aCx=0x0000000130a47880, aGlobal=Handle<JSObject *> at 0x0000000136442d28, aProtoAndIfaceCache=0x0000000118020cb0, aDefineOnGlobal=true) + 54 at DedicatedWorkerGlobalScopeBinding.cpp:458
    frame #11: 0x0000000104b9fdf1 XUL`mozilla::dom::DedicatedWorkerGlobalScopeBinding_workers::GetProtoObjectHandle(aCx=0x0000000130a47880, aGlobal=Handle<JSObject *> at 0x0000000136442d70) + 161 at DedicatedWorkerGlobalScopeBinding.cpp:494
    frame #12: 0x0000000104c25535 XUL`JS::Handle<JSObject*> mozilla::dom::CreateGlobal<mozilla::dom::workers::DedicatedWorkerGlobalScope, &(aCx=0x0000000130a47880, aNative=0x000000011a7d4580, aCache=0x000000011a7d4588, aClass=0x000000010b254678, aOptions=0x0000000136442fe0, aPrincipal=0x000000010b5dd9f0, aInitStandardClasses=true, aGlobal=MutableHandle<JSObject *> at 0x0000000136442ea0))>(JSContext*, mozilla::dom::workers::DedicatedWorkerGlobalScope*, nsWrapperCache*, JSClass const*, JS::CompartmentOptions&, JSPrincipals*, bool, JS::MutableHandle<JSObject*>) + 661 at BindingUtils.h:3100
    frame #13: 0x0000000104ba00e4 XUL`mozilla::dom::DedicatedWorkerGlobalScopeBinding_workers::Wrap(aCx=0x0000000130a47880, aObject=0x000000011a7d4580, aCache=0x000000011a7d4588, aOptions=0x0000000136442fe0, aPrincipal=0x000000010b5dd9f0, aInitStandardClasses=true, aReflector=MutableHandle<JSObject *> at 0x0000000136442f88) + 660 at DedicatedWorkerGlobalScopeBinding.cpp:429
    frame #14: 0x00000001059c7400 XUL`mozilla::dom::workers::DedicatedWorkerGlobalScope::WrapGlobalObject(this=0x000000011a7d4580, aCx=0x0000000130a47880, aReflector=MutableHandle<JSObject *> at 0x0000000136443028) + 256 at WorkerScope.cpp:395
    frame #15: 0x00000001059c18b3 XUL`mozilla::dom::workers::WorkerPrivate::GetOrCreateGlobalScope(this=0x000000011a197100, aCx=0x0000000130a47880) + 515 at WorkerPrivate.cpp:6606
    frame #16: 0x00000001059e23b4 XUL`(anonymous namespace)::CompileScriptRunnable::WorkerRun(this=0x0000000127b9c140, aCx=0x0000000130a47880, aWorkerPrivate=0x000000011a197100) + 36 at WorkerPrivate.cpp:897
    frame #17: 0x00000001059c4588 XUL`mozilla::dom::workers::WorkerRunnable::Run(this=0x0000000127b9c140) + 1784 at WorkerRunnable.cpp:350
    frame #18: 0x000000010270d14f XUL`nsThread::ProcessNextEvent(this=0x000000011b2370c0, aMayWait=false, aResult=0x000000013644365e) + 2095 at nsThread.cpp:855

Boris, is the PrincipalInfo expected to be available at the time of setting up the global?
Assignee: nobody → ehsan
Flags: needinfo?(bzbarsky)
I'd think so, yes.

Normally, I'd think this is handled by WorkerPrivateParent<Derived>::SetPrincipal which is called by the OnStreamCompleteInternal in workers/ScriptLoader.cpp.

Are service workers loaded through there?  Or do they go through the OnStreamComplete in ServiceWorkerManager.cpp which does quite different things?  :(
Flags: needinfo?(bzbarsky)
(In reply to Not doing reviews right now from comment #17)
> I'd think so, yes.
> 
> Normally, I'd think this is handled by
> WorkerPrivateParent<Derived>::SetPrincipal which is called by the
> OnStreamCompleteInternal in workers/ScriptLoader.cpp.
> 
> Are service workers loaded through there?  Or do they go through the
> OnStreamComplete in ServiceWorkerManager.cpp which does quite different
> things?  :(

Yes, but this is not a service worker in question, it's a normal dedicated worker.

So here is what happens.  When we construct a WorkerPrivate, WorkerPrivate::GetLoadInfo() fills in mLoadInfo.mPrincipal, but we don't fill in mLoadInfo.mPrincipal until WorkerPrivateParent<Derived>::SetPrincipal() is called on the main thread when loading the script finishes.  However, the creation of the global happens as a result of CompileScriptRunnable running on the worker, which calls scriptloader::LoadWorkerScript() to actually load the script after the gloabl is created.  So if I'm understanding things correctly, we wait for the actual script load to be finished in order to set mPrincipalInfo, which means mPrincipalInfo is unusable before that point (hence during global setup), and I think the reason is that we want the principal to represent the principal of the actual channel being loaded.

Now, the question is: is this the right way of doing things?  Or should we set up an interim PrincipalInfo while the script is being loaded so that we can use the principal to decide if APIs should be exposed to the global or not.

(Note that I can just hack around this by calling CacheStorage::EnabledOnPrincipal during WorkerPrivate::GetLoadInfo where we do have access to the principal, and stick the result as a bit on the WorkerLoadInfo object, but I'd like to understand if this setup is working as expected first...)
Flags: needinfo?(bzbarsky)
(In reply to Not doing reviews right now from comment #17)
> Are service workers loaded through there?  Or do they go through the
> OnStreamComplete in ServiceWorkerManager.cpp which does quite different
> things?  :(

I believe this is just used to create the initial ServiceWorker and set its first principal.  The ScriptLoader is still used to fully load the script just like SharedWorkers in the past.  This path just calls into CreateSharedWorkerForLoadInfo().

WorkerPrivateParent<Derived>::SetPrincipal() should definitely be getting called.

If it wasn't, I think CacheStorage would previously trigger an assertion at this line when it causes the LoadInfo::mPrincipalInfo auto ptr to be de-referenced by calling this:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/CacheStorage.cpp#114

When is the global created?  Is it prior to loading the scripts?  That seems not unreasonable since the scripts might try to synchronously access the global, right?
As far as I understand things, we need to create the global before loading the script in order to get a JSContext* out of it that we can pass to AddFeature() and RemoveFeature().  Those functions end up passing this context to WorkerRunnable::Dispatch() which in turn uses it to call JS_ReportError if NS_DispatchToMainThread fails.  So if we make that error reporting optional, it seems like we may be able to pass a null JSContext* and setup the global when the load has been finished.  Thoughts?
> we need to create the global before loading the script

Um... that seems bad to me, from first principles.  In particular, at least in theory we don't know the final principal until we get the OnStartRequest, but we can't mutate the principal of an existing global very nicely.

That said, for workers the final principal is same-origin with the original one, no?  So we should be able to just set the principal up initially, then set it up again if we really need the updated principal for something.  With an assert that the two are in fact same-origin.
Flags: needinfo?(bzbarsky)
(In reply to Not doing reviews right now from comment #21)
> That said, for workers the final principal is same-origin with the original
> one, no?

I don't think that is guaranteed.  For example, a data URI worker will have a different final principal from the originating principal.
> For example, a data URI worker will have a different final principal from the originating
> principal.

How so?
I believe a data URI worker always ends up with a null principal.  I had to deal with here:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?from=ScriptLoader.cpp&case=true#558

By exempting the null principal in NS_LoadGroupMatchesPrincipal().
> I believe a data URI worker always ends up with a null principal.

Er, why do we not inherit the loader's principal in that case?  We really should, imo.
I think that's a question for Jonas or others.  From what I understand we restrict a data URI to a null principal for safety reasons since its kind of like eval(), etc.
(In reply to Ben Kelly [:bkelly] from comment #26)
> I think that's a question for Jonas or others.  From what I understand we
> restrict a data URI to a null principal for safety reasons since its kind of
> like eval(), etc.

Needinfoing Jonas.
Flags: needinfo?(jonas)
Ben Kelly said Ben Turner has thoughts here (based on an IRC conversation).
(Knowing Ben Turner is unavailable this week, setting needinfo on him for comment 37).
Flags: needinfo?(bent.mozilla)
I'm missing some context, so apologies if this is nonsense.

I don't think that we should permit a service worker to be registered using a data-uri. Mainly because that breaks any ability to update the SW.

Maybe that addresses most of the issue discussed here. If so, please feel free to ignore the below.



Allowing data-urls to be indiscriminately inheriting the principal "of the loader" is a bad idea I think. We've had various instances where this has allowed websites to XSS our chrome code, and so we've put restrictions in various places on data-urls inheriting chrome principals. I don't see why websites would be more immune to this problem than our code is.

I.e. I would imagine that there are places where website A can trick website B to load a particular URL in an <iframe> or webworker. If that URL is a data-url, and data-urls inherit, then website B just got XSSed.

This is why I think we should enable something like:

new Worker({ url: "data:...", allowInheritURL: true });
<iframe src="data:..." allowinheriturl>

And only if 'allowIneritURL' is set do we actually allow schemes like data: to inherit. Otherwise the channel returns a null principal. Which for workers would mean that it's cross-origin and thus not allowed.



Right now, gecko uses a null-principal for |new Worker("data:...")|. This is definitely weird. Since that means that the worker is cross-origin which isn't allowed by spec, and isn't expected by authors.

But it does actually fix some of the XSS issues since the worker can't directly access IDB data. There's still some risk though in that the page might trust data coming from the worker in bad ways.

But I'd rather not simply make data: inherit in workers for the reasons stated above.

As long as we have the current handling for data:, I think we should be consistent about it though. Since it's running with a null principal I think we indeed should not let it have access to the Cache API.
Flags: needinfo?(jonas)
Hopefully Jonas' response cleared this up.
Flags: needinfo?(bent.mozilla)
FWIW I did give this a shot a while ago but I ran into issues since I don't think I completely understand the semantics of the worker event loops.  I got stuck looking for a way to inject one new step in the worker initialization sequence.
Whiteboard:
Whiteboard:
Blocks: 1110136
No longer blocks: serviceworker-cache
FYI, I don't think this will be implemented any time soon.  Also, its unclear to me if it should be implemented as spec's seem to lean towards rejecting with SecurityError when the CacheStorage object is used.
Now that bug 1333573 has landed in theory we can fix this, but comment 33 makes me hesitate.  Ben, what do you think?
Depends on: 1333573
Flags: needinfo?(bkelly)
I'm not sure.  I think we probably have latitude since things like private browsing are not fully spec'd.
Flags: needinfo?(bkelly)
I actually think it's really nice if we did this.  This is more in line with asking developers to feature detect especially as we are considering shipping different APIs on the ESR channel.  Choosing to throw instead of hiding means that developers need to feature detect two things.
Component: DOM → DOM: Service Workers
Priority: -- → P3
Ehsan: okay if i unassign you from this bug?
Flags: needinfo?(ehsan)
Of course, I should have done that years ago!
Assignee: ehsan → nobody
Flags: needinfo?(ehsan)
Whiteboard: DWS_NEXT
Assignee: nobody → perry
Priority: P3 → P2
To confirm, do we want to throw a SecurityError in PB (which is what dom/cache/test/mochitest/test_cache_untrusted.html appears to be expecting) or just have caches/CacheStorage be undefined?
Flags: needinfo?(ehsan)
We generally translate StorageAccess::ePrivateBrowsing <https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/dom/base/nsContentUtils.h#2942> to a SecurityError exception everywhere, so I think doing the same here also makes sense.

(Note that we originally tried to hide the property here, but right now we're planning to expose more of the above behavior to the Web, so I think the thinking here has changed a bit since this bug was originally filed, so the importance of hiding the caches property is less now...)
Flags: needinfo?(ehsan)
Update: CacheStorages, Cache, and global "caches" will only be exposed on secure contexts, and using any of those in private browsing (in secure context) will throw a SecurityError. Exposing those interfaces/attributes on secure contexts in turn broke many tests, which are in the process of being fixed
Assignee: perry → nobody
Component: DOM: Service Workers → Storage: Cache API
No longer blocks: 1110136

(In reply to Perry Jiang [:perry] from comment #41)

Update: CacheStorages, Cache, and global "caches" will only be exposed on
secure contexts, and using any of those in private browsing (in secure
context) will throw a SecurityError. Exposing those interfaces/attributes on
secure contexts in turn broke many tests, which are in the process of being
fixed

Is this a description of the current state of what we implemented (and thus we are ok) or of the desired behavior (and thus we should work and change this bug's title/description)?

Flags: needinfo?(perry)

Ah, no, this is the desired behavior and not what's current implemented. IIRC the broken tests were never fully fixed.

Flags: needinfo?(perry)
Summary: Hide window.caches and self.caches if the nsIPrincipal can't use Service Worker Cache → CacheStorage/Cache should require SecureContext

using any of those in private browsing (in secure context) will throw a SecurityError.

Can this be tracked separately? It shouldn't block adding SecureContext.

Attachment #9256619 - Attachment description: WIP: Bug 1112134 - Limit Cache/CacheStorage to SecureContext → Bug 1112134 - Limit Cache/CacheStorage to SecureContext
Assignee: nobody → krosylight
Status: NEW → ASSIGNED
Pushed by krosylight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/582722ee7fc8
Limit Cache/CacheStorage to SecureContext r=edenchuang,webidl,smaug

Backed out for causing xpcshell failures on test_originInit.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/3b63167e1ba02d11da697044acfb3e9da79b2041

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=IbqmLvRwTuWyUW5NZYRCsw.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=582722ee7fc82ce54a4a188a0212659818417c44

Failure log(s): https://treeherder.mozilla.org/logviewer?job_id=363586908&repo=autoland&lineNumber=3407 // https://treeherder.mozilla.org/logviewer?job_id=363586448&repo=autoland

Failure line(s): TEST-UNEXPECTED-FAIL | dom/cache/test/xpcshell/test_originInit.js | xpcshell return code: 0 // TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_partitionedDOMCache.js | Test timed out

Flags: needinfo?(krosylight)
Attachment #9256619 - Attachment description: Bug 1112134 - Limit Cache/CacheStorage to SecureContext → Bug 1112134 - Part 1: Limit Cache/CacheStorage to SecureContext
Flags: needinfo?(krosylight)

Depends on D135548

Depends on D135565

Attachment #9256619 - Attachment is obsolete: true
Attachment #9258392 - Attachment is obsolete: true
Attachment #9258416 - Attachment is obsolete: true
Attachment #9258417 - Attachment is obsolete: true

Some report: There are TONS OF tests that assumes existence of caches on non-secure context. Probably better to add something like dom.caches.non_secure_context.enabled or just reuse dom.caches.testing.enabled (although this one has other potentially unwanted effects), and use [Func] to conditionally expose cache things by checking mGlobal->IsSecureContext() and the flag.

Pushed by krosylight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c1b395cec03f
Part 1: Add [SecureContext] to Cache/CacheStorage r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/97a7a79776de
Part 2: Adjust tests r=dom-storage-reviewers,asuth
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

FF103 docs work for this can be tracked in https://github.com/mdn/content/issues/17475

My understanding is that this change means that the global caches object is only available in secure contexts (and this is what is use to get a CacheStorage object and associated Cache objects. So if you you're not in a secure context then the caches object won't exist.

Further, the preference dom.caches.testing.enabled and dom.serviceWorkers.testing.enabled disable this requirement for a secure context to allow things to be tested more easily.

  1. Is that about it?

  2. The existing docs for CacheStorage (only, not caches or Cache) indicate secure context support was added in FF44. Is that an error? If not, what is the difference between what happened in FF44 and now?

  3. There are some comments about about Private Browsing. Is there anything we need to say about that? If so, what happens for PB in a secure context with caches?

  4. The docs for CacheStorage have a note:

    Note: Chrome and Safari only expose CacheStorage to the windowed context over HTTPS. caches will be undefined unless an SSL certificate is configured.

    How does that differ from "in a secure context"? I.e. does that mean that even if localhost would normally be a secure context then it wouldn't work for Chrome because it requires HTTPS and a valid certificate in all cases? If so, I assume FF does not have this limitation.

Thanks!

Flags: needinfo?(krosylight)

Hi Hamish,

Is that about it?

Yup.

The existing docs for CacheStorage (only, not caches or Cache) indicate secure context support was added in FF44. Is that an error? If not, what is the difference between what happened in FF44 and now?

That kinda makes sense. Before this patch CacheStorage is the one that has secure-context-related logic (its methods threw exception in non-secure context), and caches just returned CacheStorage without any error. But it certainly is confusing to devs, I'd rather mark them as supported and add notes about this.

There are some comments about about Private Browsing. Is there anything we need to say about that? If so, what happens for PB in a secure context with caches?

The work is happening but it's Nightly only and is actively changing. Please ignore that part for now. (Bug 1776109)

How does that differ from "in a secure context"? I.e. does that mean that even if localhost would normally be a secure context then it wouldn't work for Chrome because it requires HTTPS and a valid certificate in all cases? If so, I assume FF does not have this limitation.

I feel like that part is misleading, I don't think SecureContext checks certificates. caches is available on https://expired.badssl.com/ on both Firefox and Chrome, so I'd just remove that note.

Thanks as always, and feel free to ask more whenever needed.

Flags: needinfo?(krosylight)

Thanks very much! I've taken your advice:

  • Added secure context info to browser compat data for caches. The info is still there for CacheStorage, but IMO will do not harm since it still sends the same signal "don't use outside of a secure context".
  • Removed the note about HTTPS/certificate
  • Added a release note mentioning what changed (caches et al require secure context, while previously you'd be handed a CacheStorage that would then throw in secure context). Did not mention the preference for testing - IMO it is "not intended for the arbitrary use".
  • Did not comment on private browsing.

So very minimal changes. YOu can see them in https://github.com/mdn/content/issues/17475#issuecomment-1179904034 . Hope I got this right! Thanks again.

Regressions: 1859532
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: