AppCache is very unsafe for users when deployed on http://. Unfortunately the feature was implemented before it became common to require secure contexts for powerful features. So there is non-trivial amount of insecure AppCache in the wild. There has been talk of deprecating and removing AppCache on http in blink: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/UKF8cK0EwMI/discussion AFAIK, however, it has not happened yet due to usage in the wild. We should consider rolling out some kind of UX warning to the user for sites deploying AppCache on http://. It can be similarly dangerous as password fields over insecure connections and that UX warning seems to have been quite effective (anecdotally). This UX could be: * Turning the lock icon a new shade of scary. * A banner warning asking the user if they want to permit offline features on the insecure site. * Something else? The goal here is to communicate the issue to the end users since they are the ones bearing the risk of AppCache-over-http on their device. This might help drive usage down over time so we can then disable the feature at the DOM level.

Tanvi, do you know who would be best to look at this? I'm not sure what team did the password UX change and if it would fall in the same bucket. Thanks!

Our bug to wholesale remove AppCache is bug 1237782. The appcache-over-http issue is perhaps a bit more critical for users than the other reasons for removing AppCache in total.

I don't know a lot about AppCache but I can tell you that there are a lot of people asking for "a new shade of scary" lock to warn against the security fail they find most threatening. IMO that is not helpful to users. I don't want to ship a security indicators cheat sheet with the browser. It is hard enough to convey the dangers of insecure HTTP or mixed content to the average user. A warning also sounds ineffective to me in this case. Isn't it too late once the user has visited a website on a compromised network? So, if we want the warning to be effective we'd need to include a button to clear the AppCache entry as well. That's just a cheap form of deprecating the feature on insecure contexts, because any sane user will click that button, no matter if the website is insecure or not. We should not use UX warnings as a replacement for deprecating features on insecure contexts. I think HTTP is already pretty scary for any serious web app with the in-content warning on login fields, AppCache or not. Personally I don't see a way to make this happen, sorry :/

Do we have a web console warning for this already?

Apparently we have a permission already offline caching. Perhaps we could alter that for http:// offline caching to indicate the additional risk. The risk of passwords on insecure connections is losing a credential. The risk of appcache on insecure connections is *permanently* losing control of that site on that users device, meaning all future passwords/forms could be lost. It seems really weird to warn for one, but then just shrug about the other. History has shown the deprecation and spamming the console means nothing. The people at risk are the users, not the devs. The devs can easily ignore this as long as users are unaware.

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #5) > Apparently we have a permission already offline caching. Perhaps we could > alter that for http:// offline caching to indicate the additional risk. Not sure what you mean? Would you like to show a permission prompt for using AppCache? Then you might as well just remove the feature IMO. > The risk of passwords on insecure connections is losing a credential. The > risk of appcache on insecure connections is *permanently* losing control of > that site on that users device, meaning all future passwords/forms could be > lost. It seems really weird to warn for one, but then just shrug about the > other. I never said it's not important. I acknowledge that AppCache is dangerous, but that doesn't invalidate any of my other points. Comparing the AppCache warning to the HTTP warning is apples to oranges. What we should really be looking at is showing the warning vs just removing the feature on platform side. I really can't think of any advantages to the former vs. the latter. > > History has shown the deprecation and spamming the console means nothing. > The people at risk are the users, not the devs. The devs can easily ignore > this as long as users are unaware. Spamming users isn't more effective than spamming developers, the "success" (a lot of people were really upset about it) of the insecure login warning comes from the fact that it's an extreme measure and that it is placed in the right context. As soon as people start to find these messages annoying or confusing you can be sure they will simply ignore them.

Clearing needinfo for Tanvi since she's on leave and I cc'ed a couple of people who worked on the insecure login warning.

(In reply to Johann Hofmann [:johannh] from comment #6) > Not sure what you mean? Would you like to show a permission prompt for using > AppCache? Then you might as well just remove the feature IMO. I saw something that suggested we had a permission prompt at one point, but I just tested and don't see a prompt. So you can ignore that suggestion. > > The risk of passwords on insecure connections is losing a credential. The > > risk of appcache on insecure connections is *permanently* losing control of > > that site on that users device, meaning all future passwords/forms could be > > lost. It seems really weird to warn for one, but then just shrug about the > > other. > > I never said it's not important. I acknowledge that AppCache is dangerous, > but that doesn't invalidate any of my other points. Comparing the AppCache > warning to the HTTP warning is apples to oranges. What we should really be > looking at is showing the warning vs just removing the feature on platform > side. I really can't think of any advantages to the former vs. the latter. That's like saying we should just remove forms on http://. Its dangerous, so why warn instead of just removing it? Of course the answer is its in use today and we try not to break existing content. > > History has shown the deprecation and spamming the console means nothing. > > The people at risk are the users, not the devs. The devs can easily ignore > > this as long as users are unaware. > > Spamming users isn't more effective than spamming developers, the "success" > (a lot of people were really upset about it) of the insecure login warning > comes from the fact that it's an extreme measure and that it is placed in > the right context. As soon as people start to find these messages annoying > or confusing you can be sure they will simply ignore them. Developers I talk to care when they get mixed content and other UX warnings on their site. Their users do notice. Do many ignore? Sure. But it does show up in their adoption metrics and therefore they do what they can to avoid it. Anyway, all I ask is that this be considered. Your immediate negative response doesn't give me much confidence the risk-vs-benefit is actually being weighed here.

It seems like our two man options here are either: 1. Use the insecure padlock 2. Require HTTPS for AppCache We should probably get telemetry first on deciding how often it is used over HTTP vs HTTPS. We can do what Chrome does for it's deprecation of APIs by warning in the console first for option 2. If we can't deprecate the API with expediency over HTTP we might look into option 1 however I agree with Johann about 1. seems the wrong tool here really. Insecure locks for forms was a rare choice in warning the users being exploited. I'm not sure AppCache has the same immediate risks as this does (happy to hear STRs of exploits though). As per Geolocation, we discovered it's likely safe to break some sites based on benefit of security; I think we should aim to move down that path. Googles bug is actually more up to date: https://bugs.chromium.org/p/chromium/issues/detail?id=588931 with this seemingly one of the slowest deprecations they have done for insecureContext. However we shouldn't let that stop us, lets get some data on usage first. Looks like the page usage is 2% if 50% of those are over HTTP that still seems too high: https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-04-03&keys=__none__!__none__!__none__&max_channel_version=nightly%252F55&measure=USE_COUNTER2_DEPRECATED_AppCache_PAGE&min_channel_version=null&processType=*&product=Firefox!Fennec!Nightly&sanitize=0&sort_keys=submissions&start_date=2017-03-08&table=0&trim=1&use_submission_date=0

Also worth noting Google appears to be using a third option. Pretending over HTTP there is no cache so sites fall back to getting the content. See the bug again: https://bugs.chromium.org/p/chromium/issues/detail?id=588931

Chrome stats for this are actually really low also: https://www.chromestatus.com/metrics/feature/timeline/popularity/1248

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #8) > Developers I talk to care when they get mixed content and other UX warnings on their site. Their users do notice. Do many ignore? Sure. But it does show up in their adoption metrics and therefore they do what they can to avoid it. The developers we're talking about here are running a site on HTTP. They get the absolutely worst security indicators we can offer already. You want to make it even scarier. I don't want to come off as dismissive, it's simply an ambitious idea :) > That's like saying we should just remove forms on http://. Its dangerous, so why warn instead of just removing it? > > Of course the answer is its in use today and we try not to break existing content. Warnings like these are always also a WebCompat issue, we can't pretend this will not break websites for users in practice. We were doing this extreme measure for HTTP logins because they have a marketshare of over 25% and a removal is not even remotely an option. (In reply to Jonathan Kingston [:jkt] from comment #9) > I'm not sure AppCache has the same immediate risks as this does (happy to hear STRs of exploits though). I think Ben is right in that this can be really bad for a potential victim. (Not sure if as bad as compromised credentials but I don't want to discuss that here). (In reply to Jonathan Kingston [:jkt] from comment #10) > Also worth noting Google appears to be using a third option. Pretending over > HTTP there is no cache so sites fall back to getting the content. > > See the bug again: > https://bugs.chromium.org/p/chromium/issues/detail?id=588931 Oh, nice idea.

(In reply to Jonathan Kingston [:jkt] from comment #10) > Also worth noting Google appears to be using a third option. Pretending over > HTTP there is no cache so sites fall back to getting the content. > > See the bug again: > https://bugs.chromium.org/p/chromium/issues/detail?id=588931 That has been discussed, but is not actually shipping in chrome. Also, note comment 3 in that bug: "Per today's F2F Service Worker meeting we are wondering if we might be able to apply negative security indicators to use of AppCache over insecure HTTP as a first step to AppCache deprecation." Those "negative security indicators" is what I am asking for consideration in this bug.

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #13) > Those "negative security indicators" is what I am asking for consideration > in this bug. What about just shipping bug 1310447 (crossed padlock without text for http; let the mixed content pref on false at first) with Photon and showing a deprecation message with date X in the console? http:// is arbitrariness and might be "a funny UX" as long you don't type in passwords or credit card credentials. Photon hides "https://" by default which is vice versa as done today.

> Also, note comment 3 in that bug: > > "Per today's F2F Service Worker meeting we are wondering if we might be able > to apply negative security indicators to use of AppCache over insecure HTTP > as a first step to AppCache deprecation." > > Those "negative security indicators" is what I am asking for consideration > in this bug. Now that I have had my coffee I see they continued from comment 3 to conclude that falling back to network is probably better. Another factor is that security indicators won't help with some of the iframe issues. We would need to add security indicators and disable appcache in insecure cross-origin iframes.

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #15) > Another factor is that security indicators won't help with some of the > iframe issues. We would need to add security indicators and disable > appcache in insecure cross-origin iframes. Insecure iframes are blocked as mixed active content. I also have to take a step back and revisit my earlier statement: > The developers we're talking about here are running a site on HTTP. They get the absolutely worst security indicators we can offer already. You want to make it even scarier. I don't want to come off as dismissive, it's simply an ambitious idea :) I was assuming that pages that use AppCache are automatically web apps that would have a login form and would get the insecure UI indicators (since bug 1310447 hasn't landed yet). That's obviously not necessarily the case. Ben, do you think a) There's a large portion of pages that currently do not receive the "broken lock" (e.g. don't have a login form) but are using AppCache over HTTP? b) That the information whether a site is using AppCache is trival to expose to the frontend? If both cases are true, we could consider simply making breaking the lock like we do for HTTP logins and soon hopefully for all of HTTP. If this is a big task, however, or if there are likely few sites that this affects, we might as well try to get bug 1310447 enabled by default

I don't think changing the lock when a site happens to use appcache will make sense for users since appcache is simply an implementation detail to them. Contrast that to login forms which are something the user can actually see and therefore draw a connection between the insecure lock and the login form (especially with the Control Center text). I definitely think the first step is a console warning with evangelism. Before that, we should make sure a transition from http to https can actually be done seamlessly (like we did for saved logins[1]) since IIRC AppCache handles redirects as failures for manifest contents (and maybe the manifest file itself?) which could be why uptake is slow. [1] https://matthew.noorenberghe.com/blog/2016/08/filling-saved-http-logins-over-https-same-domain

Appcache is an invisible feature to the user. Unless we have some kind of text saying why, "randomly" showing the broken lock (or whatever) on some sites will confuse users. I like the suggestion of https://bugs.chromium.org/p/chromium/issues/detail?id=588931#c4 to simply act as if the cache is empty. Or maybe make it a session cache if that helps any.

Which image ends up being mixed content is also not exposed to users and would already be random for users, no? A broken lock seems much more a signal to sites that they need to get their act together.

We appear to be at 0.13% usage of this, can we just block it's usage over insecure?

If we cannot do outright removal right now (and I guess we still want to wait until all browsers ship service workers in final releases?) I suggest we write a Hacks post that informs folks AppCache will be restricted to secure contexts by current release + 2 and spread that around and at the same time start a console warning. I don't think we should do something that bothers the end user with this.

Console warning + hacks post sounds good to me. Is there a team that owns AppCache now or should we just do it?

We agreed in our latest meeting that Comment 21 is the best approach: - Deprecate in stable with a warning - Make a pref for disabling over insecure - Enable pref in Nightly+Beta to remove AppCache over insecure - Blog about the removal

Also I just want to point out we have been warning about AppCache since September 2015 Firefox 44: https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/dom/locales/en-US/chrome/dom/dom.properties#189 Which became default in 1st March 2016 Firefox 47 when "dom.serviceWorkers.interception.enabled" which became default in 1st March pref was removed in Bug 1251875 :bkelly I see a reference to "dom.serviceWorkers.interception.enabled" in mobile prefs: https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/mobile/android/app/mobile.js#853 can we remove that? So my patch doesn't do any warning as we agreed only to console warn.

(In reply to Jonathan Kingston [:jkt] from comment #30) > :bkelly I see a reference to "dom.serviceWorkers.interception.enabled" in > mobile prefs: > https://searchfox.org/mozilla-central/rev/ > 48cbb200aa027a0a379b6004b6196a167344b865/mobile/android/app/mobile.js#853 > can we remove that? Yea, that can be removed.

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. https://reviewboard.mozilla.org/r/212562/#review219314 Before r+'ing this, can you please verify what happens when you do the following: 1. Allow insecure appcache 2. Visit an insecure site that uses appcache 3. Disable insecure appcache 4. Visit site from (2) and see if it uses appcache 5. Does site from (2) still trigger updates on an insecure appcache? Also, in the future it might be helpful to separate mechanical changes (like the test modifications to https) in a separate patch, etc. ::: dom/base/nsGlobalWindowInner.cpp:2972 (Diff revision 6) > return xpc::WindowOrNull(aObj)->IsChromeWindow() && > nsContentUtils::ObjectPrincipal(aObj) == nsContentUtils::GetSystemPrincipal(); > } > > /* static */ bool > +nsGlobalWindowInner::InsecureOfflineCacheEnabled(JSContext* aCx, JSObject* aObj) nit: I found this name a bit confusing. It says "Insecure" in the name, but then returns true if its a secure context. Maybe just change it to `OfflineCacheAllowedForContext()`? ::: dom/base/nsGlobalWindowInner.cpp:2975 (Diff revision 6) > > /* static */ bool > +nsGlobalWindowInner::InsecureOfflineCacheEnabled(JSContext* aCx, JSObject* aObj) > +{ > + return mozilla::dom::IsSecureContextOrObjectIsFromSecureContext(aCx, aObj) || > + Preferences::GetBool("browser.cache.offline.insecure.enable"); nit: You don't need the `mozilla::dom::` here. This file has a `using namespace mozilla::dom` at the top. Indenting. Please align `Preferences` with the start of the function call above it. ::: modules/libpref/init/all.js:92 (Diff revision 6) > // and files are left open given up to the OS to do the cleanup. > pref("browser.cache.max_shutdown_io_lag", 2); > > pref("browser.cache.offline.enable", true); > + > +#ifdef EARLY_BETA_OR_EARLIER Uh, why are you allowing insecure appcache on nightly, but blocking it on beta and release? ::: testing/web-platform/meta/html/browsers/offline/introduction-4/event_cached.html.ini:2 (Diff revision 6) > +[event_cached.html] > + prefs: [browser.cache.offline.insecure.enable:true] It might be better to change these WPT tests to use https instead of http. ::: testing/web-platform/meta/html/browsers/the-window-object/security-window/window-security.html.ini:2 (Diff revision 6) > [window-security.html] > + prefs: [browser.cache.offline.insecure.enable:true] Setting the insecure appcache pref instead of just marking things expected fail with our default settings seems perhaps wrong. Maybe we can't do that until all.js has a single pref for all channels, though.

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. https://reviewboard.mozilla.org/r/212562/#review219320

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. https://reviewboard.mozilla.org/r/212562/#review219314 > Uh, why are you allowing insecure appcache on nightly, but blocking it on beta and release? It's the inverse, sorry for the confusion here. As noted in the draft unship I messed up the description. Anyway browser.cache.offline.insecure.enable true keeps it behaving like it currently does and false removes the API ver insecure. This follows the same naming as browser.cache.offline.enable > It might be better to change these WPT tests to use https instead of http. My rationale to keep the wpt tests working is that we want to continue these tests working in insecure until 61. I think we should remove the pref here once 61 lands and we remove the pref, we can flip these tests to be https only. Alternatively we could submit these to wpt itself then. > Setting the insecure appcache pref instead of just marking things expected fail with our default settings seems perhaps wrong. Maybe we can't do that until all.js has a single pref for all channels, though. We can do this, however for web-compat sake I think it's worth continuing the testing at least until 61.

(In reply to Jonathan Kingston [:jkt] from comment #34) > > It might be better to change these WPT tests to use https instead of http. > > My rationale to keep the wpt tests working is that we want to continue these > tests working in insecure until 61. I think we should remove the pref here > once 61 lands and we remove the pref, we can flip these tests to be https > only. Alternatively we could submit these to wpt itself then. Well, I was trying to suggest we move the appcache tests to https since that works both today and in the future. We would just sort of leave insecure appcache untested in these wpt tests for now.

:flod due to how the code works for deprecation warnings, there isn't an ability to pass substrings to warnings. Is there an issue if I hard-code "Firefox" instead of using substring replacing in the following string: # LOCALIZATION NOTE: Do not translate "Application Cache API", "AppCache". Firefox is the release brandShortName of Firefox AppCacheInsecureWarning=Use of the Application Cache API (AppCache) for insecure connections will be removed in Firefox 61. I could instead just use the word "version"? I don't really think it's worth checking for a substring in all deprecations or adding arguments to those functions.

(In reply to Jonathan Kingston [:jkt] from comment #37) > :flod due to how the code works for deprecation warnings, there isn't an > ability to pass substrings to warnings. Is there an issue if I hard-code > "Firefox" instead of using substring replacing in the following string: > > # LOCALIZATION NOTE: Do not translate "Application Cache API", "AppCache". > Firefox is the release brandShortName of Firefox > AppCacheInsecureWarning=Use of the Application Cache API (AppCache) for > insecure connections will be removed in Firefox 61. > > I could instead just use the word "version"? I don't really think it's worth > checking for a substring in all deprecations or adding arguments to those > functions. I feel like "version 61" is safer, potentially "Gecko 61" given the target? This is not much about localization, that string will say "Firefox" in any repackaged and rebranded version of the Firefox source code. Having said that, I don't have a strong opinion against using "Firefox 61", if you think that's clearer. I agree that it's not worth the effort of passing the parameter to a relatively short-lived alert.

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. https://reviewboard.mozilla.org/r/212562/#review219354 ::: dom/locales/en-US/chrome/dom/dom.properties:190 (Diff revision 7) > HittingMaxWorkersPerDomain2=A Worker could not be started immediately because other documents in the same origin are already using the maximum number of workers. The Worker is now queued and will be started after some of the other workers have completed. > # LOCALIZATION NOTE: Do not translate "setVelocity", "PannerNode", "AudioListener", "speedOfSound" and "dopplerFactor" > PannerNodeDopplerWarning=Use of setVelocity on the PannerNode and AudioListener, and speedOfSound and dopplerFactor on the AudioListener are deprecated and those members will be removed. For more help https://developer.mozilla.org/en-US/docs/Web/API/AudioListener#Deprecated_features > # LOCALIZATION NOTE: Do not translate "Application Cache API", "AppCache" and "ServiceWorker". > AppCacheWarning=The Application Cache API (AppCache) is deprecated and will be removed at a future date. Please consider using ServiceWorker for offline support. > +# LOCALIZATION NOTE: Do not translate "Application Cache API", "AppCache". Firefox is the release brandShortName of Firefox I think it's safe to remove the second part of the note.

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. https://reviewboard.mozilla.org/r/212562/#review219706 do we have a test proving that we don't load appcached http: content when the new pref is switched off? and that we don't cache with it off? ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp:643 (Diff revision 8) > return NS_OK; > } > + } else { > + if (!sAllowInsecureOfflineCache) { > + return NS_OK; > + } nit: indent ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp:728 (Diff revision 8) > + if (!innerURI) > + return NS_ERROR_NOT_AVAILABLE; > + > + // if http then we should prevent this cache > + bool match; > + nsresult rv = innerURI->SchemeIs("http", &match); nit: re-declaration ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp:732 (Diff revision 8) > + bool match; > + nsresult rv = innerURI->SchemeIs("http", &match); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (match) > + return NS_ERROR_NOT_AVAILABLE; if () {} please! (all over this block)

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. https://reviewboard.mozilla.org/r/212562/#review219712 r=me with comments addressed. ::: modules/libpref/init/all.js:93 (Diff revision 8) > pref("browser.cache.max_shutdown_io_lag", 2); > > pref("browser.cache.offline.enable", true); > + > +#ifdef EARLY_BETA_OR_EARLIER > +pref("browser.cache.offline.insecure.enable", false); Can you add a comment that we are disabling insecure appcache in nightly and "early" beta, but release continues to have it enabled until release 62? I find the "EARLY_BETA_OR_EARLIER" confusing since "earlier" suggests lower version numbers, but nightly is a larger version number. ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp:641 (Diff revision 8) > NS_ENSURE_SUCCESS(rv, rv); > if (!match) { > return NS_OK; > } > + } else { > + if (!sAllowInsecureOfflineCache) { This prevents us from serving the stale appcache data to the user, right? Should we do something to purge it from disk or just not worry about it? ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp:720 (Diff revision 8) > + if (!sAllowInsecureOfflineCache) { > + nsCOMPtr<nsIURI> uri; > + aPrincipal->GetURI(getter_AddRefs(uri)); > + > + if (!uri) > + return NS_ERROR_NOT_AVAILABLE; Please use enclosing braces `{ }` on single line if-statements. If you want a short way of returning error here you can do: `NS_ENSURE_TRUE(uri, NS_ERROR_NOT_AVAILABLE);` etc. ::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp:728 (Diff revision 8) > + if (!innerURI) > + return NS_ERROR_NOT_AVAILABLE; > + > + // if http then we should prevent this cache > + bool match; > + nsresult rv = innerURI->SchemeIs("http", &match); Would it be a bit safer to return error if its not https:// instead of returning error if its only http://? Is it possible to do other schemes like file:// for appcache?

(In reply to Ben Kelly [:bkelly] from comment #42) > This prevents us from serving the stale appcache data to the user, right? > Should we do something to purge it from disk or just not worry about it? I think it's a stage two to the cleanup. I would do that when we remove the pref. It would be bad for some users that may decide to use insecure appcache to remove their cached data suddenly. > `NS_ENSURE_TRUE(uri, NS_ERROR_NOT_AVAILABLE);` not that this will plague the text console (not good) > > + nsresult rv = innerURI->SchemeIs("http", &match); > > Would it be a bit safer to return error if its not https:// instead of > returning error if its only http://? > > Is it possible to do other schemes like file:// for appcache? only http or https ever has and ever will support appcaching. there is no support for file to be appcached. and this patch wants to disable (via a pref) support in http:, so I think this is right.

(In reply to Honza Bambas (:mayhemer) from comment #43) > > `NS_ENSURE_TRUE(uri, NS_ERROR_NOT_AVAILABLE);` > > not that this will plague the text console (not good) Well, for checks like if nsIPrincipal->GetURI() returns a nullptr URI I think warning is reasonable. But yet, you probably don't want to warn on !match.

Hey Honza, I updated the patch to add a test (note it's a little noisy as I rebased): https://reviewboard.mozilla.org/r/212562/diff/8-9/ Could you check it over just in case, essentially the test does the following: - Loads the file with everything enabled - Goes offline - Clears the HTTP cache just in case - Loads the file again to ensure it loads - Turns the pref off - The file then doesn't load as it's over HTTP - Goes online - The file loads but appcache api is disabled I just wanted you to have another check before I land the code. (Looks like I might have lint errors since it got rebased too?, will fix in a follow up) Thanks!

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. https://reviewboard.mozilla.org/r/212562/#review221744 ::: dom/tests/mochitest/ajax/offline/browser_disableAppcache.js:85 (Diff revision 12) > + ] > + }); > + > + // turn network back on and check that the APIs are disabled > + > + await BrowserTestUtils.openNewForegroundTab(gBrowser, URL, false); should be done after switching offline? ::: dom/tests/mochitest/ajax/offline/file_simpleManifest.cacheManifest^headers^:1 (Diff revision 12) > +Content-Type: text/cache-manifest no longer needed, I believe ::: dom/tests/mochitest/ajax/offline/file_simpleManifest.html:8 (Diff revision 12) > +<title>load manifest test</title> > + > +<script type="text/javascript"> > + window.addEventListener("load", () => { > + const hasAppcache = document.getElementById("hasAppcache"); > + hasAppcache.textContent = "applicationCache" in window ? "yes" : "no"; this is not a test whether this has loaded from appcache. it only tests the property is there, which just reflects the state of the preference to enable appcache. you should offline cache something with "evil" content in it and then when the pref is off, check you load again from the originating server with "good" content. you'll need a statefull sjs or something returning some random number or something that changes with every request made.

c53

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. https://reviewboard.mozilla.org/r/212562/#review221744 > should be done after switching offline? We are already offline at this point, I'm checking that the whole file doesn't ever load (which I verified before does work when appcache is enabled) which verifies or not if appcache was used. I wanted to ensure the code is suffering the offline scenario here, then clicking the try again button once the network comes back. > this is not a test whether this has loaded from appcache. it only tests the property is there, which just reflects the state of the preference to enable appcache. > > you should offline cache something with "evil" content in it and then when the pref is off, check you load again from the originating server with "good" content. you'll need a statefull sjs or something returning some random number or something that changes with every request made. This part isn't the parent file verifies what was loaded over appcache. This is to ensure if the API has been disabled. I couldn't find a way to stuff the cache easily with something else without using a python script and a tonne more complexity. If you have any pointers I'm open to doing so, my priority was checking the cached file never loaded with the pref off.

(In reply to Jonathan Kingston [:jkt] from comment #55) > Comment on attachment 8942278 [details] > Bug 1354175 - Disable AppCache in insecure contexts. > > https://reviewboard.mozilla.org/r/212562/#review221744 > > > should be done after switching offline? > > We are already offline at this point, I'm checking that the whole file > doesn't ever load (which I verified before does work when appcache is > enabled) which verifies or not if appcache was used. I wanted to ensure the > code is suffering the offline scenario here, then clicking the try again > button once the network comes back. good, maybe add a comment? overall, it's good to describe steps the test takes at top, so it's easier to follow the test. > > > this is not a test whether this has loaded from appcache. it only tests the property is there, which just reflects the state of the preference to enable appcache. > > > > you should offline cache something with "evil" content in it and then when the pref is off, check you load again from the originating server with "good" content. you'll need a statefull sjs or something returning some random number or something that changes with every request made. > > This part isn't the parent file verifies what was loaded over appcache. This > is to ensure if the API has been disabled. > I couldn't find a way to stuff the cache easily with something else without > using a python script and a tonne more complexity. If you have any pointers > I'm open to doing so, my priority was checking the cached file never loaded > with the pref off. ok, that's probably enough (however, I missed the spot in the test doing - probably because the comments are weak) there is a planty of sjs examples (server js) around the tree, and even in this directory you are adding the tests to. what I want is a check that when an appcached content from an insecure context cached before we've added this preference doesn't load (when either offline or online!) when the pref is set to TRUE - to disable the insecure appcache. this is the whole point - to prevent cache poisoning. that is IMO missing and is the most (if not only) important thing to test. and of course, that we don't cache insecure appcache when the pref is on.

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. Hey Honza, as mentioned would you be able to give a feedback review on this test. Thanks!

I'm getting intermittent fails which I'm working through which I can't replicate even through --verify. Anyway I don't think this will impact the test itself much whatever the fix is.

Comment on attachment 8942278 [details] Bug 1354175 - Disable AppCache in insecure contexts. Yes! Thanks, that's it! Nice work.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 1323577f5d53542f53f54af7917f8eb22bdaac1c -d 7574b1758561: rebasing 444976:1323577f5d53 "ug 1354175 - Disable AppCache in insecure contexts. r=baku,bkelly,mayhemer" (tip) merging dom/base/nsDeprecatedOperationList.h merging dom/base/nsGlobalWindowInner.cpp merging dom/base/nsGlobalWindowInner.h merging modules/libpref/init/all.js merging netwerk/protocol/http/nsHttpChannel.cpp merging testing/web-platform/meta/html/dom/interfaces.html.ini warning: conflicts while merging testing/web-platform/meta/html/dom/interfaces.html.ini! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e401076359c6 Disable AppCache in insecure contexts. r=baku,bkelly,mayhemer

Backed out changeset e401076359c6 (bug 1354175) for M(8) failures on a CLOSED TREE https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=62e956da98b9b145347f5658aa48697921e54d5d&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-searchStr=M(8) https://hg.mozilla.org/integration/autoland/rev/62e956da98b9b145347f5658aa48697921e54d5d

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee72b754dd19 Disable AppCache in insecure contexts. r=baku,bkelly,mayhemer

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/support-for-application-cache-on-insecure-sites-has-been-deprecated/

I have documented this, by adding notes to relevant pages: https://developer.mozilla.org/en-US/docs/Web/API/SharedWorkerGlobalScope/applicationCache https://developer.mozilla.org/en-US/docs/Web/HTML/Using_the_application_cache https://developer.mozilla.org/en-US/Firefox/Experimental_features#Security https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts/features_restricted_to_secure_contexts I've not added this to the release notes, as it is only in Nightly/Beta for now. Let me know if this looks OK. Thanks!

I added the word "make" to https://developer.mozilla.org/en-US/docs/Web/API/SharedWorkerGlobalScope/applicationCache "Don't use it to make offline websites" - feel free to change. Everything else looks good, shall I file dev-doc-needed again when we do this for stable? As always, thanks a lot!

(In reply to Jonathan Kingston [:jkt] from comment #74) > I added the word "make" to > https://developer.mozilla.org/en-US/docs/Web/API/SharedWorkerGlobalScope/ > applicationCache "Don't use it to make offline websites" - feel free to > change. "offline" is fairly commonly used as a verb in this context these days, but I am honestly fine with either. > > Everything else looks good, shall I file dev-doc-needed again when we do > this for stable? Please do. > As always, thanks a lot! You're welcome!