STR: 1. Open "about:cache" 2. Go to "List Cache Entries" 3. Click any link AR: Nothing happened. The link is not opened. ER: The link is opened. Browser Console shows >Security Error: Content at about:cache?storage=disk&context= may not load or link to about:cache-entry?storage=disk&context=&eid=&uri=... ("..." is a link clicked) workaround: Copy link location and paste it to urlbar. mozregression: 7:18.55 INFO: Last good revision: 29fd1259e299250eb26e4c83dbef8c55de612305 7:18.55 INFO: First bad revision: 5b14585f3efcd201d4552591d2bbe3acc42cc4ba 7:18.55 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=29fd1259e299250eb26e4c83dbef8c55de612305&tochange=5b14585f3efcd201d4552591d2bbe3acc42cc4ba 7:19.81 INFO: Looks like the following bug has the changes which introduced the regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1318664 It seems that 1318664 resolved 1309310 but it is insufficient. I don't know why this(or these) occurs on 51.0.1 though... (maybe uplifted?) (54.0a1 20170130030205 has same issue)

(In reply to atlanto from comment #0) > mozregression: > 7:18.55 INFO: Last good revision: 29fd1259e299250eb26e4c83dbef8c55de612305 > 7:18.55 INFO: First bad revision: 5b14585f3efcd201d4552591d2bbe3acc42cc4ba > 7:18.55 INFO: Pushlog: > https://hg.mozilla.org/integration/mozilla-inbound/ > pushloghtml?fromchange=29fd1259e299250eb26e4c83dbef8c55de612305&tochange=5b14 > 585f3efcd201d4552591d2bbe3acc42cc4ba > 7:19.81 INFO: Looks like the following bug has the changes which introduced > the regression: > https://bugzilla.mozilla.org/show_bug.cgi?id=1318664 This regression window doesn't make sense given that it's broken on 51. Before bug 1318664, if you opened about:cache?storage=disk&context= directly, were you able to click entries and have them work? Can you get a new regression window for when that *actually* broke? I expect it's bug 1309310, but confirmation would be helpful. There should really be functional automated tests for the about:cache stuff if we intend to keep shipping it and keep it working. :-\

Oh, although bug 1309310 was uplifted to 51 together with 1309310, so that explains the 51 bustage. But mozregression checks against nightly, and I think this broke on Nightly before 1318664 landed, and was simply hidden before because nobody tried the direct link. I'd still like confirmation of when this regressed on Nightly.

(In reply to :Gijs from comment #2) > Oh, although bug 1309310 was uplifted to 51 together with 1309310, Egh, copy-paste failure. Bug 1318664 was uplifted together with 1309310. Sorry for all the bugspam. > so that > explains the 51 bustage. But mozregression checks against nightly, and I > think this broke on Nightly before 1318664 landed, and was simply hidden > before because nobody tried the direct link. I'd still like confirmation of > when this regressed on Nightly.

>Before bug 1318664, if you opened about:cache?storage=disk&context= directly, were you able to click entries and have them work? No, of course not. >Can you get a new regression window for when that *actually* broke? I could not perform mozregression because of 1309310.(I couldn't reach Cache List Entries) I didn't try beyond the wall and I guessed 1318664 reveals this issue. I didn't mean 1318664 is the cause of this issue. Anyway, I'll try it again later.

I checked myself and this was indeed caused by bug 1309310. Boris, I'm not sure how to proceed. Some options I see: 1. contrive some kind of linkage scheme whereby some about: pages can link to some other about: pages 2. allow linking only based on having the same-or-lower linkability flags between about: pages (so chrome pages can link to unlinkable pages can link to linkable pages, but not in the other direction) 3. move about:cache-entry into about:cache (ie put the 'entry' bit in the query params) so it has the same about: module as about:cache (not sure why those are different pages entirely, tbh, but given they're both cpp-backed, not sure how easy it would be to actually do this) 4. hack the about module detection code so it returns the same thing for about:cache-entry and about:cache (not sure what would break - we could potentially only hack it in CAPS but that feels even worse.) 5. make about:cache privileged. (Do not want.) All else being equal I'd lean towards doing one of 2-4, probably 2 or 3 given that 4 will only break some more when we normalize about: URIs further. What do you think, and are there options I've missed?

I can't think of any bright ideas you didn't list. Option 2 sounds pretty good to me, albeit complicated. Option 3 is nice and simple conceptually; the hard part is making it work...

Comment on attachment 8832903 [details] Bug 1335272 - fix about:cache internal links, https://reviewboard.mozilla.org/r/109154/#review110318 ::: netwerk/protocol/about/nsAboutCache.cpp:568 (Diff revision 2) > } > > NS_IMETHODIMP > nsAboutCache::GetURIFlags(nsIURI *aURI, uint32_t *result) > { > - *result = 0; > + *result = nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT; Note that the documentation in the about: protocol handler says this is only OK if the module sets the originalURI correctly. I don't think an originalURI makes sense for these dynamically generated documents, but practically speaking, I *think* this is OK? Please make sure I haven't missed something, though. :-)

Comment on attachment 8832903 [details] Bug 1335272 - fix about:cache internal links, https://reviewboard.mozilla.org/r/109154/#review110316 ::: caps/nsScriptSecurityManager.cpp:761 (Diff revision 1) > bool schemesMatch = scheme.Equals(otherScheme, stringComparator); > - bool isSamePage; > + bool isSamePage = false; > // about: URIs are special snowflakes. > if (scheme.EqualsLiteral("about")) { > - nsAutoCString module, otherModule; > - isSamePage = schemesMatch && > + nsCOMPtr<nsIAboutModule> module, otherModule; > + bool knowBothModules = You need to still check schemesMatch here, right? Otherwise you can call NS_GetAboutModule on a random non-about: URI. ::: caps/nsScriptSecurityManager.cpp:769 (Diff revision 1) > - module.Equals(otherModule); > + uint32_t aboutModuleFlags = 0; > + uint32_t otherAboutModuleFlags = 0; > + knowBothModules = knowBothModules && > + NS_SUCCEEDED(module->GetURIFlags(currentURI, &aboutModuleFlags)) && > + NS_SUCCEEDED(otherModule->GetURIFlags(currentOtherURI, &otherAboutModuleFlags)); > + if (schemesMatch && knowBothModules) { And then you won't need to check schemesMatch here, since knowBothModules will be false if not. ::: caps/nsScriptSecurityManager.cpp:775 (Diff revision 1) > + // about: pages can link only from more privileged to less > + // privileged pages. URI flags will allow linking to 'public' > + // ones like about:blank, but deny everything else. Allow > + // unprivileged non-public about: pages to link to each > + // other here: > + isSamePage = What about ones that are not safe for untrusted content. Shouldn't they be able to link to self? As in, don't you still need to keep the "same module name means linking is ok" check? Your test gives about:test-about-chrome-privs the system principal, but nothing forces the "not safe" case to use system principal, right?

> I don't think an originalURI makes sense for these dynamically generated documents Well. The point is, if nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT is set then the principal will be created based on the originalURI of the channel. So it's not about whether it makes sense or not in terms of "where is this loaded from?"; it's about the principal of the resulting document. Anyway, nsAboutCache::NewChannel basically sets up an input stream channel, and that will default to having its originalURI be the same as its URI. So about:cache-whatever in this case. And the page will then get a codebase principal for this URI. That's probably OK.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14) > Comment on attachment 8832903 [details] > Bug 1335272 - fix about:cache internal links, > > https://reviewboard.mozilla.org/r/109154/#review110316 > > ::: caps/nsScriptSecurityManager.cpp:761 > (Diff revision 1) > > bool schemesMatch = scheme.Equals(otherScheme, stringComparator); > > - bool isSamePage; > > + bool isSamePage = false; > > // about: URIs are special snowflakes. > > if (scheme.EqualsLiteral("about")) { > > - nsAutoCString module, otherModule; > > - isSamePage = schemesMatch && > > + nsCOMPtr<nsIAboutModule> module, otherModule; > > + bool knowBothModules = > > You need to still check schemesMatch here, right? Otherwise you can call > NS_GetAboutModule on a random non-about: URI. > > ::: caps/nsScriptSecurityManager.cpp:769 > (Diff revision 1) > > - module.Equals(otherModule); > > + uint32_t aboutModuleFlags = 0; > > + uint32_t otherAboutModuleFlags = 0; > > + knowBothModules = knowBothModules && > > + NS_SUCCEEDED(module->GetURIFlags(currentURI, &aboutModuleFlags)) && > > + NS_SUCCEEDED(otherModule->GetURIFlags(currentOtherURI, &otherAboutModuleFlags)); > > + if (schemesMatch && knowBothModules) { > > And then you won't need to check schemesMatch here, since knowBothModules > will be false if not. Ugh, good points. > ::: caps/nsScriptSecurityManager.cpp:775 > (Diff revision 1) > > + // about: pages can link only from more privileged to less > > + // privileged pages. URI flags will allow linking to 'public' > > + // ones like about:blank, but deny everything else. Allow > > + // unprivileged non-public about: pages to link to each > > + // other here: > > + isSamePage = > > What about ones that are not safe for untrusted content. Shouldn't they be > able to link to self? As in, don't you still need to keep the "same module > name means linking is ok" check? > > Your test gives about:test-about-chrome-privs the system principal, but > nothing forces the "not safe" case to use system principal, right? This is an interesting question. I've always assumed that URI_SAFE_FOR_UNTRUSTED_CONTENT == content privileges, and things that don't have that flag => chrome privileges. The about:cache case is the first I saw to the contrary. What *does* determine if the content gets chrome privileges? Because it's not clear to me... (and without knowing the answer to that in more detail, I'm not sure about the case you're referring to...)

> What *does* determine if the content gets chrome privileges? The way it works is this: 1) The about: module creates a channel. 2) _If_ the URI_SAFE_FOR_UNTRUSTED_CONTENT flag is set, the about: protocol handler then clears the "owner" on the channel. So if the flag is not set, the principal will be whatever it would normally be for the channel. If the flag _is_ set, then the thing we do only matters if the channel creation in the about: module set the owner. In practice, for our built-in modules, this only happens if the channel created was a normal channel for a chrome:// URI, because the chrome protocol handler sets the "owner" to system in NewChannel. This setup is all kinda wonky in the new LoadInfo world, obviously.... For example, if the about: module creates a channel which says to inherit triggering principal and puts the triggering principal in the loadinfo as system, then we'll get system no matter what URI_SAFE_FOR_UNTRUSTED_CONTENT is set to.

Comment on attachment 8832903 [details] Bug 1335272 - fix about:cache internal links, https://reviewboard.mozilla.org/r/109154/#review110358 r=me

Comment on attachment 8832902 [details] Bug 1335272 - prep: factor out registering about: pages into BTU, https://reviewboard.mozilla.org/r/109152/#review111176 ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:1302 (Diff revision 3) > + this._loadedAboutContentScript = true; > + Services.ppmm.loadProcessScript(kAboutPageRegistrationContentScript, true); The order of these two should be swapped in case loadProcessScript throws.

Comment on attachment 8832904 [details] Bug 1335272 - add barebones functional test for about:cache, https://reviewboard.mozilla.org/r/109156/#review111330 Hey Gijs, sorry for the lag of response here. Anyway, the test looks good. I was wondering if it make sense to use |content.document.docShell.currentDocumentChannel.loadinfo.triggeringPrincipal and loadingPrincipal| and check for the exact principal we are expecting, similar to what we do in [1]. I am fine either way, but I think it would make sense to check the values of the loadInfo. [1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_e10s_about_page_triggeringprincipal.js#31 ::: netwerk/test/browser/browser_about_cache.js:23 (Diff revision 3) > + return uri.startsWith("about:cache?") && uri.includes("storage=disk"); > + }; > + let diskPageLoaded = BrowserTestUtils.browserLoaded(tab.linkedBrowser, false, expectedPageCheck); > + yield ContentTask.spawn(tab.linkedBrowser, null, function() { > + ok(!content.document.nodePrincipal.isSystemPrincipal, > + "about:cache should not have system principal"); Ideally I would prefer to check for the exact principal we expect here. E.g. is(content.document.nodePrincipal.URI.spec,...). Could you add that? Additionaly we could keep the is(!systemPrincipal,...) check. ::: netwerk/test/browser/browser_about_cache.js:37 (Diff revision 3) > + info("Saw load for " + uri); > + return uri.startsWith("about:cache-entry") && uri.includes("dummy.html"); > + }; > + let entryLoaded = BrowserTestUtils.browserLoaded(tab.linkedBrowser, false, expectedPageCheck); > + yield ContentTask.spawn(tab.linkedBrowser, kTestPage, function(kTestPage) { > + ok(!content.document.nodePrincipal.isSystemPrincipal, same here. ::: netwerk/test/browser/browser_about_cache.js:47 (Diff revision 3) > + }); > + yield entryLoaded; > + info("about:cache entry loaded"); > + > + yield ContentTask.spawn(tab.linkedBrowser, null, function() { > + ok(!content.document.nodePrincipal.isSystemPrincipal, and same here.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/38740bd3a50e prep: factor out registering about: pages into BTU, r=jaws https://hg.mozilla.org/integration/autoland/rev/494c40293b33 fix about:cache internal links, r=bz https://hg.mozilla.org/integration/autoland/rev/54f31b2c699e add barebones functional test for about:cache, r=ckerschb

https://hg.mozilla.org/mozilla-central/rev/38740bd3a50e https://hg.mozilla.org/mozilla-central/rev/494c40293b33 https://hg.mozilla.org/mozilla-central/rev/54f31b2c699e

Comment on attachment 8832902 [details] Bug 1335272 - prep: factor out registering about: pages into BTU, Approval Request Comment [Feature/Bug causing the regression]: bug 1309310 [User impact if declined]: about:cache doesn't work properly [Is this code covered by automated tests?]: as part of this patch, yes [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: there's automated tests, but given this is going into beta relatively late, it wouldn't hurt to test: 1) type 'about:cache' in the URL bar, hit enter 2) click the "list cache entries" link for memory/disk/appcache 3) click one of the entry link All of the links should work, inasmuch as the links are present (you may need to navigate to some webpages first if you're using a clean profile). [List of other uplifts needed for the feature/fix]: n/a, but I will need to write a slightly different version of the first patch for beta which doesn't have the tests from bug 1331686 / bug 1329032 . [Is the change risky?]: a bit [Why is the change risky/not risky?]: because we're changing caps. If there's anything I've learned it's that doing so rarely goes unpunished. I'm asking for uplift anyway because I don't think we should ship ESR with a broken about:cache, because there's automated tests that significantly reduce the risk of this causing unwanted changes, because we still have a few weeks of beta left, and because the actual code changes here (in the second patch) aren't actually *that* big. [String changes made/needed]: no.

These are the same 3 patches grafted to beta with conflicts resolved. I verified tests still pass. To be clear: uplift requested for all 3 patches.

Comment on attachment 8832902 [details] Bug 1335272 - prep: factor out registering about: pages into BTU, fix links in about:cache, aurora53+, beta52+ (3 patches)

https://hg.mozilla.org/releases/mozilla-aurora/rev/e2db8d3a5745 https://hg.mozilla.org/releases/mozilla-aurora/rev/f05860329459 https://hg.mozilla.org/releases/mozilla-aurora/rev/55e4637ae559

https://hg.mozilla.org/releases/mozilla-beta/rev/722b9597f736 https://hg.mozilla.org/releases/mozilla-beta/rev/e9ee24c88e55 https://hg.mozilla.org/releases/mozilla-beta/rev/1a04f291a9db

Backed out from Beta for browser_BrowserTestUtils.js timeouts. Yes, I used the attached Beta patch. https://treeherder.mozilla.org/logviewer.html#?job_id=75643977&repo=mozilla-beta https://hg.mozilla.org/releases/mozilla-beta/rev/bb6cc749654e

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35) > Backed out from Beta for browser_BrowserTestUtils.js timeouts. Yes, I used > the attached Beta patch. > https://treeherder.mozilla.org/logviewer.html#?job_id=75643977&repo=mozilla- > beta > > https://hg.mozilla.org/releases/mozilla-beta/rev/bb6cc749654e Ugh, it's because a newURI call fails, because 52 doesn't have bug 1329182. I've asked there to get that change uplifted because this is going to bite us repeatedly on ESR. Anyway, I'll update, test that test (I actually built that patch and ran both of the other ones and forgot about the BTU "unit" test, sigh) and then reland. Thanks!

remote: https://hg.mozilla.org/releases/mozilla-beta/rev/28a0babec23b8a90018adba8a3af545f1883f852 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/913c4c60537a3729fe3bd42e04420d74655bbe72 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/65283abdecf54206c301b209f9582d2cc72b0b21

https://hg.mozilla.org/releases/mozilla-esr52/rev/28a0babec23b https://hg.mozilla.org/releases/mozilla-esr52/rev/913c4c60537a https://hg.mozilla.org/releases/mozilla-esr52/rev/65283abdecf5