Closed Bug 1359204 Opened 3 years ago Closed 3 years ago

From view-source, can't open view-source:file URL links (so local file links inside view source are broken)

Categories

(Core :: DOM: Security, defect, P1)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: vertova, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [domsecurity-active])

Attachments

(4 files, 5 obsolete files)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Attached file index.7z
Gijs, is this related to some URL handing changes you had made?
Flags: needinfo?(gijskruitbosch+bugs)
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9307d60e21ba70f46fb47c9eb43b6b5246b7e64c&tochange=d31df518cb6ec920cf9ab7accb67682298f8a9f8

Regressed by: Bug 1182569
Blocks: 1182569
Component: View Source → DOM: Security
Product: Toolkit → Core
(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. :-(
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ckerschb)
Summary: Can't open file: URLs in view source → From view-source, can't open view-source:file URL links (so local file links inside view source are broken)
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.
Flags: needinfo?(ckerschb)
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
Hey Gijs, smaug is not accepting r? requests at the moement, something you feel comfortable to r+ or should we ask someone else?
Attachment #8862364 - Flags: review?(gijskruitbosch+bugs)
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!
Attachment #8862365 - Flags: feedback?(gijskruitbosch+bugs)
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?
Attachment #8862365 - Flags: feedback?(gijskruitbosch+bugs)
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.
Attachment #8862364 - Flags: review?(gijskruitbosch+bugs) → feedback+
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?
Attachment #8862365 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
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!
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Attachment #8862364 - Flags: review+
(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.
Flags: needinfo?(gijskruitbosch+bugs)
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!
Attachment #8862434 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8863705 - Flags: feedback?(gijskruitbosch+bugs)
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.
Attachment #8863705 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Flags: needinfo?(gijskruitbosch+bugs)
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).
Attachment #8863705 - Attachment is obsolete: true
Attachment #8863818 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8863818 - Flags: review?(gijskruitbosch+bugs) → review+
> (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
Attachment #8863818 - Attachment is obsolete: true
Attachment #8864020 - Flags: review+
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
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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+.
Attachment #8864020 - Attachment is obsolete: true
Attachment #8864465 - Flags: review+
(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
Flags: needinfo?(bzbarsky)
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?
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
> 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.
Flags: needinfo?(bzbarsky)
See Also: → 1361688
(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
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(bzbarsky)
(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?
Attachment #8865603 - Flags: review?(bzbarsky)
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?
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
Attachment #8865603 - Flags: review?(bzbarsky) → review+
(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
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
Attachment #8862364 - Flags: approval-mozilla-aurora?
Comment on attachment 8864465 [details] [diff] [review]
bug_1359204_view_source_file_tests.patch

Approval Request Comment
See comment 42
Attachment #8864465 - Flags: approval-mozilla-aurora?
Comment on attachment 8865603 [details] [diff] [review]
bug_1359204_view_source_loadpage_triggeringprincipal.patch

Approval Request Comment
See comment 42
Attachment #8865603 - Flags: approval-mozilla-aurora?
Comment on attachment 8862364 [details] [diff] [review]
bug_1359204_view_source_file.patch

54 is on Beta.
Attachment #8862364 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8864465 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8865603 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Hi Daniel,
I'd like to get your feedback on the changing security checks given the risk assessment in comment #42.
Flags: needinfo?(dveditz)
Blocks: 1361688
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.
No longer blocks: 1361688
Flags: needinfo?(dveditz)
Blocks: 1361688
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.
Attachment #8862364 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8864465 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8865603 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.