User Agent: Mozilla/5.0 (Windows NT 6.1; rv:53.0) Gecko/20100101 Firefox/53.0 Build ID: 20170413192749 Steps to reproduce: In FF 53, try opening a link to a local file in view source. I was thinking of a simple test case for STR such as <a href="file:///foobar.html">Click here</a>, but I suspect you can only reproduce this if you click on a link to an existing local file, otherwise the behaviour I describe is expected. Actual results: If you click on the link, nothing happens. If you ctrl-click, FF opens a blank new tab. Links to file: URLs in normal navigation open as usual. Expected results: In previous versions, if you opened a link to a local file in view source, Firefox would display the source of the destination file.

Security Error: Content at view-source:file:///D:/pathTo/index.html may not load or link to file:///D:/pathTo/foobar.html.

Gijs, is this related to some URL handing changes you had made?

Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9307d60e21ba70f46fb47c9eb43b6b5246b7e64c&tochange=d31df518cb6ec920cf9ab7accb67682298f8a9f8 Regressed by: Bug 1182569

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3) > Gijs, is this related to some URL handing changes you had made? Based on comment #4 (thanks, Alice!), I don't think so. This looks like a bug in nsContentSecurityManager::CheckChannel: https://dxr.mozilla.org/mozilla-central/rev/e17cbb839dd225a2da7e5d5bec43cf94e11749d8/dom/security/nsContentSecurityManager.cpp#559-565 if (contentPolicyType == nsIContentPolicy::TYPE_DOCUMENT || contentPolicyType == nsIContentPolicy::TYPE_SUBDOCUMENT) { // query the nested URI for security checks like in the case of view-source nsCOMPtr<nsINestedURI> nestedURI = do_QueryInterface(uri); if (nestedURI) { nestedURI->GetInnerURI(getter_AddRefs(uri)); } This looks buggy to me. Why do we do this, Christoph? The CheckLoadURI code should be doing the right things for nested URIs already, and unnesting them might make it do the wrong thing. :-(

Francesco, thanks for filing; Alison thanks for the testcase and Gijs thanks for locating the problem. (In reply to :Gijs from comment #5) > This looks buggy to me. Why do we do this, Christoph? Now that I look at it I think you are right, but there was a testcase that was failing. I browsed through Bug 1182569 but I couldn't figure out why we added it anymore. I guess there is only one way to find out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48586a9c9377f25c8cef7d4671e95810ad70c08c Once we have results from TRY we can take it from there.

Hey Gijs, smaug is not accepting r? requests at the moement, something you feel comfortable to r+ or should we ask someone else?

Gijs, it seems that we can't query 'content.document.getElementById("testlink");' within view-source and hence 'link' is null. Any idea how we could simulate the link click in a view-source page? Thanks!

Comment on attachment 8862365 [details] [diff] [review] bug_1359204_view_source_file_tests.patch Review of attachment 8862365 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/browser/browser_click_link_within_view_source.js @@ +26,5 @@ > + info("set href of testlink and click link within view-source page"); > + let loadPromise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, LINK_URI); > + yield ContentTask.spawn(gBrowser.selectedBrowser, LINK_URI, function(uri) { > + let link = content.document.getElementById("testlink"); > + link.href = uri; Err, no, wait. This test isn't testing the right thing. What's supposed to happen is: 1) you have a page with a link to file:///.../foo.html 2) you view-source on that page. 3) you click that link in view-source. But in view-source, all the links are prefixed with 'view-source' as well, so the link will point to view-source:file:///.../foo.html, not to file:///.../foo.html To make this work right, I think we need to put the dummy.html link hardcoded in the test file (which is unfortunate, but there we are). @@ +28,5 @@ > + yield ContentTask.spawn(gBrowser.selectedBrowser, LINK_URI, function(uri) { > + let link = content.document.getElementById("testlink"); > + link.href = uri; > + }); > + BrowserTestUtils.synthesizeMouseAtCenter("#testlink", {}, There's only 1 link, so I expect just using "a[href]" as the selector should work?

Comment on attachment 8862364 [details] [diff] [review] bug_1359204_view_source_file.patch Review of attachment 8862364 [details] [diff] [review]: ----------------------------------------------------------------- Given that I also suggested this change, and that I'm not actually a peer here or all that familiar with this specific code, I think it would be good if someone else looked at this as well. But yes, this change looks good to me.

I did also test the change and it seems that this works fine for normal view source usage on both file: and http(s): pages, and that links from view-source:file:... to view-source:http(s) still work, while links from view-source:http(s) to view-source:file: are still blocked as expected.

Gijs, thanks for your continuous help when writing browser tests. (In reply to :Gijs from comment #9) > Err, no, wait. This test isn't testing the right thing. Indeed. We need to load a local file://, then view-source on that file and then click the link within that view-source page. > To make this work right, I think we need to put the dummy.html link > hardcoded in the test file (which is unfortunate, but there we are). How hardcoded? Using something like: let TEST_FILE_URI = getChromeDir(getResolvedURI(gTestPath)); TEST_FILE_URI.append("file_click_link_within_view_source.html"); TEST_FILE_URI = Services.io.newFileURI(TEST_FILE_URI).spec; The other thing I realized is that we can't really wait for a tab to load or also test that the right tab is loaded because of the difference of representation of our generated URI and the one that gBrowser.selectedBrowser.currentURI.spec spits out, see following example: Got view-source:file:///home/ckerschb/source/mc/docshell/test/browser/file_click_link_within_view_source.html expected view-source:file:///home/ckerschb/source/mc-obj-ff-dbg/_tests/testing/mochitest/browser/docshell/test/browser/file_click_link_within_view_source.html > @@ +28,5 @@ > > + yield ContentTask.spawn(gBrowser.selectedBrowser, LINK_URI, function(uri) { > > + let link = content.document.getElementById("testlink"); > > + link.href = uri; The problem is that 'link' is null at this point, so I suppose content.document.getElementById() does not work on a view-source: page. Or am I missing something?

Comment on attachment 8862364 [details] [diff] [review] bug_1359204_view_source_file.patch Smaug, since you helped me getting Bug 1182569 landed I was wondering if you could review that regression fix for me. Please have a look at comment 5 where Gijs already summarizes the problem. Thanks!

Comment on attachment 8862364 [details] [diff] [review] bug_1359204_view_source_file.patch And you're adding a testcase for this? Do we have testcase also for that wyciwyg case, if not, please add.

(In reply to Olli Pettay [:smaug] from comment #14) > And you're adding a testcase for this? Yes. > Do we have testcase also for that wyciwyg case, if not, please add. Will do.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12) > Created attachment 8862434 [details] [diff] [review] > bug_1359204_view_source_file_tests.patch > > Gijs, thanks for your continuous help when writing browser tests. > > (In reply to :Gijs from comment #9) > > Err, no, wait. This test isn't testing the right thing. > > Indeed. We need to load a local file://, then view-source on that file and > then click the link within that view-source page. > > > To make this work right, I think we need to put the dummy.html link > > hardcoded in the test file (which is unfortunate, but there we are). > > How hardcoded? Using something like: > let TEST_FILE_URI = getChromeDir(getResolvedURI(gTestPath)); > TEST_FILE_URI.append("file_click_link_within_view_source.html"); > TEST_FILE_URI = Services.io.newFileURI(TEST_FILE_URI).spec; No. I mean, load the file_click_link_within_view_source.html file however you're doing it now. Then open view source. Then click the link in view-source. Don't set the target of the link you're clicking dynamically. Your current test is then trying to change the link while we're already in view source. Instead, the actual HTML on disk should have the right URL for the link from the start. You can just use a relative URL there, so if you load a document that has: <a href="dummy.html">Link</a> Then the corresponding view-source document will automatically end up with a link to view-source:file:///.../dummy.html. > The other thing I realized is that we can't really wait for a tab to load or > also test that the right tab is loaded because of the difference of > representation of our generated URI and the one that > gBrowser.selectedBrowser.currentURI.spec spits out, see following example: > > Got > > view-source:file:///home/ckerschb/source/mc/docshell/test/browser/ > file_click_link_within_view_source.html > expected > > view-source:file:///home/ckerschb/source/mc-obj-ff-dbg/_tests/testing/ > mochitest/browser/docshell/test/browser/file_click_link_within_view_source. > html I think you could just omit the url argument to waitFornewTab and that would work. > > @@ +28,5 @@ > > > + yield ContentTask.spawn(gBrowser.selectedBrowser, LINK_URI, function(uri) { > > > + let link = content.document.getElementById("testlink"); > > > + link.href = uri; > > The problem is that 'link' is null at this point, so I suppose > content.document.getElementById() does not work on a view-source: page. Or > am I missing something? The DOM of the view-source page isn't at all the DOM of the original page. It just shows the source of the original page. So the link you want to click here doesn't have an ID at all, which is why document.getElementById() can find it. You can try to use the devtools inspector on a view-source document to see what I mean.

Hey Gijs, can you take another look at this test please? Unfortunately all that view-source related stuff is not straight forward hence I would like to make sure we are testing the right thing. Two things are worth mentioning: a) It seems that document.getElementById() does not work on a view-source page, hence we use document.links which returns the correct link. I added a sanity check to make sure there is only 1 link on the page. b) We are facing that discrepancy between getChromeDir() and gBrowser.selectedBrowser.currentURI.spec > file:///home/ckerschb/source/mc/docshell/test/browser/file_click_link_within_view_source.html VS > file:///home/ckerschb/source/mc-obj-ff-dbg/_tests/testing/mochitest/browser/docshell/test/browser/file_click_link_within_view_source.html hence I am using startsWith() and endsWith() as a workaround. So while the beginning of the test is working I am failing the last check to make sure the dummy_page.html is loaded correctly, I am getting: > view-source:file:///home/ckerschb/source/mc/docshell/test/browser/file_click_link_within_view_source.html but I should get > view-source:file:///home/ckerschb/source/mc/docshell/test/browser/dummy_page.html Am I testing the wrong tab? But then why would the above tests succeed? I am confused. Any idea what might be wrong the that final test? Thanks!

Comment on attachment 8863705 [details] [diff] [review] bug_1359204_view_source_file_tests.patch Review of attachment 8863705 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/browser/browser_click_link_within_view_source.js @@ +5,5 @@ > + let TEST_FILE_URI = getChromeDir(getResolvedURI(gTestPath)); > + TEST_FILE_URI.append(TEST_FILE); > + TEST_FILE_URI = Services.io.newFileURI(TEST_FILE_URI).spec; > + > + let DUMMY_FILE = "dummy_page.html"; Your HTML has "dummy.html", not "dummy_page.html". Not sure which is wrong! @@ +25,5 @@ > + let tabPromise = BrowserTestUtils.waitForNewTab(gBrowser); > + let vSrcItem = vSrcCtxtMenu.getElementsByAttribute("id", "context-viewsource")[0]; > + vSrcItem.click(); > + vSrcCtxtMenu.hidePopup(); > + let tab = yield tabPromise; To be honest, I would replace this whole context menu stuff with just: document.getElementById("View:PageSource").doCommand(); @@ +42,5 @@ > + let myLink = content.document.links[0]; > + myLink.click(); > + }); > + > + yield loadPromise; I suspect the reason this is resolving with the wrong load is that the waitForNewTab() promise doesn't wait for the new tab to finish loading, and so apparently you're managing to activate the link before that page is fully loaded. The simplest fix is to pass the third param to browserLoaded: https://dxr.mozilla.org/mozilla-central/rev/2e7c10a9b86e30691f67855f6c8f98d984508d7c/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#185 so: let loadPromise = BrowserTestUtils.browserLoaded(tab.linkedBrowser, false, url => url.endsWith("dummy.html")); that should help.

Gijs, thanks for the feedback. dummy_page.html is the right file to use and updating > BrowserTestUtils.browserLoaded(tab.linkedBrowser, false, url => url.endsWith("dummy_page.html")); caused the test finally to work as expected. (Really appreciate your help with those browser tests - thanks).

Comment on attachment 8863818 [details] [diff] [review] bug_1359204_view_source_file_tests.patch Review of attachment 8863818 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/browser/browser_click_link_within_view_source.js @@ +1,3 @@ > +"use strict"; > + > +add_task(function*() { Please name the function and add a line or two of docstring comment above it indicating what it's testing. @@ +24,5 @@ > + yield popupPromise; > + let tabPromise = BrowserTestUtils.waitForNewTab(gBrowser); > + let vSrcItem = vSrcCtxtMenu.getElementsByAttribute("id", "context-viewsource")[0]; > + vSrcItem.click(); > + vSrcCtxtMenu.hidePopup(); Was there a reason not to replace the context menu stuff with document.getElementById("View:PageSource").doCommand(); as I previously suggested?

> (In reply to Olli Pettay [:smaug] from comment #14) > > Do we have testcase also for that wyciwyg case, if not, please add. In fact, we have multiple tests verifying wyciwyg channels succeed, e.g.: docshell/test/test_bug540462.html dom/html/test/test_bug172261.html dom/html/test/test_bug255820.html In that case there is no need to add an additional test.

(In reply to :Gijs from comment #20) > Was there a reason not to replace the context menu stuff with > document.getElementById("View:PageSource").doCommand(); as I previously > suggested? Nope, there was no reason, but I looked at that part again and I followed your suggestion. Carrying over r+. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dfb6db2590a3a01175f88005d24bd867da2c7fb

Gijs, any chance you could take a look at the test again? It seems it's failing on TRY, see bc2. I have no idea what's wrong now. If you have any insights please let me know. Thanks! https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dfb6db2590a3a01175f88005d24bd867da2c7fb&selectedJob=96162818

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #23) > Gijs, any chance you could take a look at the test again? It seems it's > failing on TRY, see bc2. I have no idea what's wrong now. If you have any > insights please let me know. Thanks! > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=3dfb6db2590a3a01175f88005d24bd867da2c7fb&selectedJob=9 > 6162818 I think that the issue is that we're not waiting for the view-source document to have loaded before trying to click the link. This is annoying to check for because the new tab event happens in the parent and view-source loads in the child, so there's effectively no guarantee either way whether the load has or hasn't happened by the time the content task spins up. The simplest solution is likely to do this in the content task: if (content.document.readyState != "complete") { yield ContentTaskUtils.waitForEvent(content.document, "readystatechange", false, () => return content.document.readyState == "complete"); } You'll need to make the content task function be a function*() in order for the yield to work, or make it an async function and use 'await' instead of 'yield' in the snippet I just gave. However, I also feel obliged to point out that that trypush shows that another view-source test is breaking because of these changes. toolkit/components/viewsource/test/browser/browser_srcdoc.js is orange in all the runs - and while there's an intermittent on file, that trypush looks like perma-orange.

(In reply to :Gijs from comment #24) > The simplest solution is likely to do this in the content task: > if (content.document.readyState != "complete") { > yield ContentTaskUtils.waitForEvent(content.document, "readystatechange", > false, () => return content.document.readyState == "complete"); > } Thanks Gijs, works locally, I'll verify it works on TRY as well before pushing. Carrying over r+.

(In reply to :Gijs from comment #24) > However, I also feel obliged to point out that that trypush shows that > another view-source test is breaking because of these changes. > toolkit/components/viewsource/test/browser/browser_srcdoc.js is orange Yes, that's correct - the test is permanently failing now. It originates from Bug 1201535 and with the patch from this bug applied it spits out the following error in the console: > Security Error: Content at moz-nullprincipal:{0c032f03-aca6-49f9-9c28-dd7c974035c7} may not load or link to view-source:about:srcdoc. When looking at the security manager I get the following arguments for that channel load: > doContentSecurityCheck { > channelURI: view-source:about:srcdoc > loadingPrincipal: nullptr > triggeringPrincipal: NullPrincipal > principalToInherit: NullPrincipal > contentPolicyType: 6 > securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, SEC_FORCE_INHERIT_PRINCIPAL, > } Since we now don't strip the view-source from the URI, the function CheckLoadURIWithPrincipal enters CheckLoadURIFlags here [1] using the following args: > CheckLoadURIFlags { > aSourceURI: moz-nullprincipal:{0c032f03-aca6-49f9-9c28-dd7c974035c7} > aTargetURI: view-source:about:srcdoc > aSourceBaseURI: moz-nullprincipal:{0c032f03-aca6-49f9-9c28-dd7c974035c7} > aTargetBaseURI: moz-safe-about:srcdoc > } which ultimately returns false and blocks the load. I assume the NullPrincipal comes from the data: URI load which is not allowed to access view-source:about:srcdoc. I actually don't know what the right path forward is. Boris, what are we missing? [1] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#807

So the proximate failure is that view-source: is URI_DANGEROUS_TO_LOAD (as of bug 1172165), and we hacked it to work if one view-source: thing links to another one... but in the case of viewing source on something that has nullprincipal the origin principal is also a nullprincipal, not a view-source: codebase principal or something? The fundamental problem here is that CheckLoadURI checks use a principal to represent the actor and a URI to represent the action being taken. And then at some point they start messing with the URI stored in the principal and comparing to the URI being loaded, so we can get mismatches like the above. So here's the first question: what should the principal of view-source on that data: URI be?

Boris, I ran a few tests to compare principals for similar view-source loads: 1) URI: data:text/html,<body>hi</body> channelURI: view-source:data:text/html,<body>hi</body> loadingPrincipal: nullptr triggeringPrincipal: SystemPrincipal principalToInherit: NullPrincipal contentPolicyType: 6 2) URI: https://www.example.com/ channelURI: view-source:https://www.example.com/ loadingPrincipal: nullptr triggeringPrincipal: SystemPrincipal principalToInherit: NullPrincipal contentPolicyType: 6 3) URI: data:text/html,<html><iframe%20srcdoc='<a%20href="about:mozilla">good</a>'></iframe></html> channelURI: view-source:data:text/html,<html><iframe%20srcdoc='<a%20href="about:mozilla">good</a>'></iframe></html> loadingPrincipal: nullptr triggeringPrincipal: SystemPrincipal principalToInherit: NullPrincipal contentPolicyType: 6 4) (Viewing source of frame from example (3) channelURI: view-source:about:srcdoc loadingPrincipal: nullptr triggeringPrincipal: NullPrincipal principalToInherit: NullPrincipal contentPolicyType: 6 Is it correct that the triggeringPrincipal is a NullPrincipal in case (4)? In all other cases the view-source is triggered by the SystemPrincipal, shouldn't that be the case for (4) as well? (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #27) > So here's the first question: what should the principal of view-source on > that data: URI be? I think view-source on a data URI should have the arguments from example (1). The SystemPrincipal triggers it, but the principalToInherit should be the NullPrincipal. That seems to be the correct behavior to me.

> 4) (Viewing source of frame from example (3) This is via the "view frame source" context menu item? > Is it correct that the triggeringPrincipal is a NullPrincipal in case (4)? If this is coming from the context menu item, then no. > I think view-source on a data URI should have the arguments from example (1). I'm not talking about the arguments. I'm talking about the principal of the resulting document, which is then presumably used as the principal for CheckLoadURI checks for links from it.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #29) > > 4) (Viewing source of frame from example (3) > > This is via the "view frame source" context menu item? Yes, it is coming from the context menu item (right-click->this-frame->view-frame-source). > > Is it correct that the triggeringPrincipal is a NullPrincipal in case (4)? > > If this is coming from the context menu item, then no. See stacktrace underneath. > > I think view-source on a data URI should have the arguments from example (1). > > I'm not talking about the arguments. I'm talking about the principal of the > resulting document, which is then presumably used as the principal for > CheckLoadURI checks for links from it. What I wanted to say is that the load should be triggered by the systemPrincipal but the principalToInherit should be the NullPrincipal. The triggeringPrincipal is the one that is used for CheckLoadURI. In fact the following little patch would make the test succeed again. But I am not sure if that is the right fix. diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -5691,16 +5691,17 @@ nsDocShell::LoadPage(nsISupports* aPageD newSpec.Append(spec); rv = NS_NewURI(getter_AddRefs(newUri), newSpec); if (NS_FAILED(rv)) { return rv; } shEntry->SetURI(newUri); shEntry->SetOriginalURI(nullptr); + shEntry->SetTriggeringPrincipal(nsContentUtils::GetSystemPrincipal()); } %---- snip ---- #0 0x00007f314761c75d in nanosleep () at ../sysdeps/unix/syscall-template.S:84 #1 0x00007f314761c6aa in __sleep (seconds=0) at ../sysdeps/posix/sleep.c:55 #2 0x00007f31392f8a5f in ah_crap_handler (signum=11) at /home/ckerschb/source/mc/toolkit/xre/nsSigHandlers.cpp:103 #3 0x00007f31392d6a0a in nsProfileLock::FatalSignalHandler (signo=11, info=0x7ffefb54f370, context=0x7ffefb54f240) at /home/ckerschb/source/mc/toolkit/profile/nsProfileLock.cpp:191 #4 0x00007f3139d3af5e in js::UnixExceptionHandler (signum=11, info=0x7ffefb54f370, context=0x7ffefb54f240) at /home/ckerschb/source/mc/js/src/ds/MemoryProtectionExceptionHandler.cpp:263 #5 0x00007f313a16bd36 in WasmFaultHandler<(Signal)0> (signum=11, info=0x7ffefb54f370, context=0x7ffefb54f240) at /home/ckerschb/source/mc/js/src/wasm/WasmSignalHandlers.cpp:1299 #6 <signal handler called> #7 0x00007f3136a55e05 in nsContentSecurityManager::doContentSecurityCheck (aChannel=0x7f31119e24f0, aInAndOutListener=[(nsViewSourceChannel *) 0x7f310f4f42c8]) at /home/ckerschb/source/mc/dom/security/nsContentSecurityManager.cpp:553 #8 0x00007f3133700f7a in nsBaseChannel::AsyncOpen2 (this=0x7f31119e24a0, aListener=0x7f310f4f42c8) at /home/ckerschb/source/mc/netwerk/base/nsBaseChannel.cpp:728 #9 0x00007f3133c673f5 in nsViewSourceChannel::AsyncOpen (this=0x7f310f4f42c0, aListener=0x7f3108acc5e0, ctxt=0x0) at /home/ckerschb/source/mc/netwerk/protocol/viewsource/nsViewSourceChannel.cpp:311 #10 0x00007f3133c675f4 in nsViewSourceChannel::AsyncOpen2 (this=0x7f310f4f42c0, aListener=0x7f3108acc5e0) at /home/ckerschb/source/mc/netwerk/protocol/viewsource/nsViewSourceChannel.cpp:341 #11 0x00007f31347e0a57 in nsURILoader::OpenURI (this=0x7f311caad8c0, channel=0x7f310f4f42c0, aFlags=0, aWindowContext=0x7f3111ab1030) at /home/ckerschb/source/mc/uriloader/base/nsURILoader.cpp:857 #12 0x00007f3138cf62ce in nsDocShell::DoChannelLoad (this=0x7f3111ab1000, aChannel=0x7f310f4f42c0, aURILoader=0x7f311caad8c0, aBypassClassifier=false) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:11561 #13 0x00007f3138cf5971 in nsDocShell::DoURILoad (this=0x7f3111ab1000, aURI=0x7f3117fe87a0, aOriginalURI=0x0, aLoadReplace=false, aReferrerURI=0x0, aSendReferrer=true, aReferrerPolicy=0, aTriggeringPrincipal=0x7f31071b74a0, aPrincipalToInherit=0x7f31071b74a0, aTypeHint=0x7f313aad200c <gNullChar> "", aFileName=<gNullChar> u"", aPostData=0x0, aHeadersData=0x0, aFirstParty=true, aDocShell=0x0, aRequest=0x7ffefb5500d0, aIsNewWindowTarget=false, aBypassClassifier=false, aForceAllowCookies=false, aSrcdoc=u"<a href=\"about:mozilla\">good</a>", aBaseURI=0x7f31181b93e0, aContentPolicyType=6) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:11375 #14 0x00007f3138cf2f3c in nsDocShell::InternalLoad (this=0x7f3111ab1000, aURI=0x7f3117fe87a0, aOriginalURI=0x0, aLoadReplace=false, aReferrer=0x0, aReferrerPolicy=0, aTriggeringPrincipal=0x7f31071b74a0, aPrincipalToInherit=0x7f31071b74a0, aFlags=64, aWindowTarget=<empty_buffer> u"", aTypeHint=0x7f313aad200c <gNullChar> "", aFileName=<gNullChar> u"", aPostData=0x0, aHeadersData=0x0, aLoadType=4, aSHEntry=0x7f31128ee530, aFirstParty=true, aSrcdoc=u"<a href=\"about:mozilla\">good</a>", aSourceDocShell=0x0, aBaseURI=0x7f31181b93e0, aCheckForPrerender=false, aDocShell=0x0, aRequest=0x0) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:10808 #15 0x00007f3138cfbf64 in nsDocShell::LoadHistoryEntry (this=0x7f3111ab1000, aEntry=0x7f31128ee530, aLoadType=4) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:12734 #16 0x00007f3138cde4aa in nsDocShell::LoadPage (this=0x7f3111ab1000, aPageDescriptor=0x7f31128ee488, aDisplayType=1) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:5701 #17 0x00007f3133622082 in NS_InvokeByIndex () at /home/ckerschb/source/mc/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30) > What I wanted to say is that the load should be triggered by the > systemPrincipal but the principalToInherit should be the NullPrincipal. The > triggeringPrincipal is the one that is used for CheckLoadURI. In fact the > following little patch would make the test succeed again. But I am not sure > if that is the right fix. > > diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp > --- a/docshell/base/nsDocShell.cpp > +++ b/docshell/base/nsDocShell.cpp > @@ -5691,16 +5691,17 @@ nsDocShell::LoadPage(nsISupports* aPageD > newSpec.Append(spec); > > rv = NS_NewURI(getter_AddRefs(newUri), newSpec); > if (NS_FAILED(rv)) { > return rv; > } > shEntry->SetURI(newUri); > shEntry->SetOriginalURI(nullptr); > + shEntry->SetTriggeringPrincipal(nsContentUtils::GetSystemPrincipal()); > } Webpages can call history.go(-1) and stuff like that. If that hits this codepath, this might not be right?

> What I wanted to say is that the load should be triggered by the > systemPrincipal but the principalToInherit should be the NullPrincipal. That's fine. > See stacktrace underneath. Thanks. So I think what we may want to do is restrict the LoadPage triggering principal thing to the nsIWebPageDescriptor::DISPLAY_AS_SOURCE case, but in that case we should certainly reset the triggering principal to system. We should also document the behavior of loadPage clearly! That said, you didn't answer the questions I was asking above... > would make the test succeed again Which test? The concerns in this bug are about clicking links in view-source, not the original view-source thing which happens via loadPage, right? > If that hits this codepath It doesn't. loadPage is only invoked directly by chrome code.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #32) > Which test? The concerns in this bug are about clicking links in > view-source, not the original view-source thing which happens via loadPage, > right? Boris, sorry for not providing the big picture, let me summarize what's happening in this bug: The bug itself is about the problem that clicking links on a file:// URI openend in view-source: do not work. We fixed that problem by not stripping the view-source: (or also jar:) part from the URI before performing checkLoadURI checks. What happenend next is that with this change the test toolkit/components/viewsource/test/browser/browser_srcdoc.js stopped working. Then I started to debug browser_srcdoc.js to figure out whether the old behavior was correct or the new behavior is. I tested a few things (e.g. comment 28) which makes me believe the new behavior is correct and that right-click->view-source-of-frame on: > data:text/html,<html><iframe%20srcdoc='<a%20href="about:mozilla">good</a>'></iframe></html> should use the systemPrincpal as the triggeringPrincipal which we now do in this patch. Does that make sense?

Ah. Yes, I think that makes sense, thanks!

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #34) > Ah. Yes, I think that makes sense, thanks! Boris, did you want to r+ that patch and just forgot to set the flag or is there something else we are missing?

Comment on attachment 8865603 [details] [diff] [review] bug_1359204_view_source_loadpage_triggeringprincipal.patch Hrm. I didn't see this review request... I think the comment is too-specific: this would be a problem for any page which was loaded other than via the URL bar. So perhaps something like this: shEntry's current triggering principal is whoever loaded that page initially. But now we're doing another load of the page, via an API that is only exposed to system code. The triggering principal for this load should be the system principal. or something. Maybe explicitly mention that this is needed in the DISPLAY_AS_SOURCE case because web things can't link to view-source:. And again, maybe we should only reset the triggering principal in the DISPLAY_AS_SOURCE case.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #37) > again, maybe we should only reset the triggering principal in the > DISPLAY_AS_SOURCE case. Maybe I am missing something here, or it's splinter, but the > shEntry->SetTriggeringPrincipal(nsContentUtils::GetSystemPrincipal()); is within the |if (nsIWebPageDescriptor::DISPLAY_AS_SOURCE == aDisplayType|. I'll update the comment - thanks!

> is within the |if (nsIWebPageDescriptor::DISPLAY_AS_SOURCE == aDisplayType| Ah, so it is. Great!

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a7df04f9ba Do not query nested URI within CheckChannel in ContentSecurityManager. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0a18eacde8 Use SystemPrincipal as TriggeringPrincipal when loading page as view-source. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/4346875ba9c3 Test view-source can open link is not blocked by security policies. r=gijs

https://hg.mozilla.org/mozilla-central/rev/e7a7df04f9ba https://hg.mozilla.org/mozilla-central/rev/3e0a18eacde8 https://hg.mozilla.org/mozilla-central/rev/4346875ba9c3

Comment on attachment 8862364 [details] [diff] [review] bug_1359204_view_source_file.patch Approval Request Comment Opening a local file:// in view-source doesn't permit to click links. Similar opening a local file using jar: (See Bug 1361688, which is a duplicate of this bug) is denied by security checks. [Feature/Bug causing the regression]: Bug 1182569 [User impact if declined]: See approval comment [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: no, landed an automated test [List of other uplifts needed for the feature/fix]: --- [Is the change risky?]: low-medium risk [Why is the change risky/not risky?]: low to medium risk because changing security checks for view-source and jar: files might introduce another problem (related inner URIs), which are view-source: and jar:. [String changes made/needed]: no

Comment on attachment 8864465 [details] [diff] [review] bug_1359204_view_source_file_tests.patch Approval Request Comment See comment 42

Comment on attachment 8865603 [details] [diff] [review] bug_1359204_view_source_loadpage_triggeringprincipal.patch Approval Request Comment See comment 42

Comment on attachment 8862364 [details] [diff] [review] bug_1359204_view_source_file.patch 54 is on Beta.

Hi Daniel, I'd like to get your feedback on the changing security checks given the risk assessment in comment #42.

In addition to jar: and view-source: there are other nested types such as the feed: protocol and some about: urls. The worry wouldn't be breaking view-source (in a different way than the current brokenness) but breaking something to do with these URLs elsewhere. The GetInnerURI() call that's being removed was troubling because multiply-nested URIs are possible (simple case: view-source of a jar: file). It might be valid to have special-case code that is "if view-source: get the next level down", but for generic nested URIs either you leave them alone or you drill down to the "Innermost" URI. This patch is better in than out.

Comment on attachment 8862364 [details] [diff] [review] bug_1359204_view_source_file.patch Fix a regression that local file links inside view source are broken. Beta54+. Should be in 54 beta 9.

https://hg.mozilla.org/releases/mozilla-beta/rev/3ee9312263c7 https://hg.mozilla.org/releases/mozilla-beta/rev/1c5f3f6b6dc2 https://hg.mozilla.org/releases/mozilla-beta/rev/a2a06bf22ead

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #42) > [Is this code covered by automated tests?]: > yes > > [Has the fix been verified in Nightly?]: > not yet > > [Needs manual test from QE? If yes, steps to reproduce]: > no, landed an automated test Setting qe-verify- based on Christoph's assessment on manual testing needs and the fact that this fix has automated coverage.