1571342 - Clicking "Save all as HAR" menu entry doesn't do anything

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce

1. Navigate to a site
2. Open the netmonitor
3. Check that there are some network calls done
4. On the toolbar click the HAR button
5. In the dropdown, select Save All as HAR

Expected results

A file dialog opens and let me save to a file

Actual results

Nothing happens

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report I can reproduce the issue on my machine too.

Honza

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d08d899e5431458f7c28ca66a13e3e95dff9c7c1&tochange=2236a18eb0a91be02e72c22e370a6f5aa877f11e

Regressed by:
2236a18eb0a91be02e72c22e370a6f5aa877f11e Gijs Kruitbosch — Bug 1560178 - disallow unsafe loads in the parent, r=bzbarsky
6071c24be566a2b3f2f98152456a540e9e893377 Gijs Kruitbosch — Bug 1560178 - fix devtools tests that load untrusted URIs in the parent, r=jdescottes
e76e9e4790fbeeef75c45e587648bd176dd314b6 Gijs Kruitbosch — Bug 1560178 - fix miscellaneous tests to allow them to keep working when disallowing remote content in the parent process, r=aswan,ckerschb
3093028bdd97d359e4c1a1d150f4733e690561c4 Gijs Kruitbosch — Bug 1560178 - adjust webextension tests that rely on loading untrusted URIs in the parent process when remote webextensions are turned off, r=aswan
e95cad09512848284491e78ce42bf8fb06d028ce Gijs Kruitbosch — Bug 1560178 - fix/remove about:addons tests that load discovery pane in the parent, r=aswan

Gijs,
Your patch seems to cause the regression. Can you Please look into this?

Comment 3

[:Gijs (out for now; he/him)]

I'm on PTO until next Monday and won't be able to look until then. Honza or Nicolas, are you able to look at this sooner? The likely cause is that the HAR saving is somehow trying to execute a document load in a docshell that isn't remote (ie not using e10s). It needs remoting to the content process if it requires the document load to work. The work Andrew's been doing in bug 1569135 may also be helpful as a guide as to what'd need to happen.

Comment 4

[Andrew Swan [:aswan]]

I took a quick glance and the HAR gathering bits seem fine, I think the problem is the download code (https://searchfox.org/mozilla-central/source/devtools/client/shared/file-saver.js) This code takes all the HAR data in a Blob, creates a hidden anchor in the devtools page, sets it up to download the blob, calls .click() on it, then cleans everything up.
I'm not sure exactly how to fix this. Boris, would it be safe to allow blob: urls to be loaded in the parent process or would that open up the types of holes that bug 1560178 was meant to close?
If its not safe to load blobs in the parent process then devtools will need to display its own file picker and then write the file to disk itself or figure out how to run the existing code in a content process.

Comment 5

[Andrew Swan [:aswan]]

Eh, this might be clear from context but as I was editing comment 4 I managed to remove the detail that clicking the hidden anchor starts a load of the blob in the parent process and this stopped working with bug 1560178. So the first question is whether loading a blob: url in the parent process is actually safe or not. I'm not equipped to answer that question, hence the ni to Boris.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Not sure if this can help, but in the console we do have a mechanism to save a file to disk: devtools/client/webconsole/utils/context-menu.js#252-276.
It still works today, so maybe the har exporter could use the same methods?

Comment 7

[Andrew Swan [:aswan]]

From an offline discussion, it is intentional that the parent process refuses to load blob urls, so I think the right course here is for devtools to show its own file picker and write the file to disk (perhaps using something like the code from comment 6).
Note this is affecting 69 so if a fix is going to be uplifted its going to need to happen pretty quickly!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ugh, I thought I had commented here, but apparently not...

Whether it's safe to navigate to a blob kinda depends on where the text of the blob came from...

I think we can probably add blob: to the whitelist here if it's really needed. If system code is creating blobs from untrusted data and then loading them, it was vulnerable to problems a lot worse than bug 1560178. On the other hand, maybe we want to protect it from those problems...

We could in theory also do something more like:

  nsCOMPtr<nsIPrincipal> principal;
  if (GetBlobURLPrincipal(uri, getter_addRefs(principal)) &&
      principal->IsSystemPrincipal()) {
    // allow
  }

which would allow blobs only if system code created them. Again, not sure whether we want to allow that.

Comment 9

[Andrew Swan [:aswan]]

My 2c, this particular case seems easy enough to work around in devtools that doing that sounds much better than relaxing the limits on blobs and relying on reviews/audits/whatever to prevent unsafe use of them.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Andrew Swan [:aswan] from comment #9)

My 2c, this particular case seems easy enough to work around in devtools that doing that sounds much better than relaxing the limits on blobs and relying on reviews/audits/whatever to prevent unsafe use of them.

I agree, let's try to use the same approach as the Console panel.

---

Here is what the console panel does:

const date = new Date();
const suggestedName =
  `console-export-${date.getFullYear()}-` +
  `${date.getMonth() + 1}-${date.getDate()}_${date.getHours()}-` +
  `${date.getMinutes()}-${date.getSeconds()}.txt`;
const returnFile = await showSaveFileDialog(win, suggestedName);
const converter = Cc[
  "@mozilla.org/intl/scriptableunicodeconverter"
].createInstance(Ci.nsIScriptableUnicodeConverter);
converter.charset = "UTF-8";
const webconsoleOutput = parentNode.querySelector(".webconsole-output");
const istream = converter.convertToInputStream(
  getElementText(webconsoleOutput)
);
return saveFileStream(returnFile, istream);

This works for the Network panel & HAR as well and we can use it in this place:
https://searchfox.org/mozilla-central/rev/9775cca0a10a9b5c5f4e15c8f7b3eff5bf91bbd0/devtools/client/netmonitor/src/har/har-exporter.js#98

But, HAR can also be compressed and saved as *.zip and we can't use @mozilla.org/intl/scriptableunicodeconverter in such case.

Since the saveFileStream helper function (used by the Console panel) is expecting nsIInputStream the question is:
How can I convert Blob to nsIInputStream? Is there a converter for that?

Honza

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

We could in theory also do something more like:

  nsCOMPtr<nsIPrincipal> principal;
  if (GetBlobURLPrincipal(uri, getter_addRefs(principal)) &&
      principal->IsSystemPrincipal()) {
    // allow
  }

which would allow blobs only if system code created them. Again, not sure whether we want to allow that.

I'm not a blob: architecture expert, but my understanding is that we can't know whether the blob was parent- or content-process created, even if it has system principal (ie a content process can create a system-principal'd blob:, which is then used in the parent). This once again makes me think we should have a separate system principal pointer identity for "system principal that got sent up from the child and deserialized in the parent" so we can distinguish it when we care (like here)... but that's probably not a great short-term solution for this particular problem.

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #10)

This works for the Network panel & HAR as well and we can use it in this place:
https://searchfox.org/mozilla-central/rev/9775cca0a10a9b5c5f4e15c8f7b3eff5bf91bbd0/devtools/client/netmonitor/src/har/har-exporter.js#98

But, HAR can also be compressed and saved as *.zip and we can't use @mozilla.org/intl/scriptableunicodeconverter in such case.

How do I test this case? Is it only if the hidden pref devtools.netmonitor.har.compress is set to true?

Since the saveFileStream helper function (used by the Console panel) is expecting nsIInputStream the question is:
How can I convert Blob to nsIInputStream? Is there a converter for that?

Looks like you can make jszip output a typed array by setting type to uint8array, and that should be easy to write to a file asynchronously using e.g. OS.File.writeAtomic(). If you must use streams, you could make jszip output a blob (or just wrap the uint8array in a blob yourself) and use NetUtil.asyncFetch with an options object, passing the blob URI, setting loadWithSystemPrincipal to true and setting an appropriate content policy type.

Now that I'm back, I might as well try fixing this given that it's my fault it broke...

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to :Gijs (he/him) from comment #11)

How do I test this case? Is it only if the hidden pref devtools.netmonitor.har.compress is set to true?

Yes

Now that I'm back, I might as well try fixing this given that it's my fault it broke...

That would be great, thanks!

Honza

Comment 13

[:Gijs (out for now; he/him)]

Attachment: Bug 1571342 - unify save paths for console and netmonitor and fix the latter to not do docshell loads, r?Honza

Comment 14

[Frederik Braun [:freddy]]

(In reply to :Gijs (he/him) from comment #11)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

We could in theory also do something more like:

  nsCOMPtr<nsIPrincipal> principal;
  if (GetBlobURLPrincipal(uri, getter_addRefs(principal)) &&
      principal->IsSystemPrincipal()) {
    // allow
  }

which would allow blobs only if system code created them. Again, not sure whether we want to allow that.

I'm not a blob: architecture expert, but my understanding is that we can't know whether the blob was parent- or content-process created, even if it has system principal (ie a content process can create a system-principal'd blob:, which is then used in the parent). This once again makes me think we should have a separate system principal pointer identity for "system principal that got sent up from the child and deserialized in the parent" so we can distinguish it when we care (like here)... but that's probably not a great short-term solution for this particular problem.

FWIW; this is also my understanding. Allowing system-principal'd blob: would still allow for the content process to compromise the parent process. So +1 to providing a process-specific identity of system principals.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73659f7163fc
unify save paths for console and netmonitor and fix the latter to not do docshell loads, r=Honza,nchevobbe

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/73659f7163fc

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 9085276 [details]
Bug 1571342 - unify save paths for console and netmonitor and fix the latter to not do docshell loads, r?Honza

Beta/Release Uplift Approval Request

• User impact if declined: Saving requests as HAR from the devtools netmonitor fails silently
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment #0
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This unifies some codepaths to use the same set of utility functions to prompt for a destination and then save some data to a file. There's an automated test for one of these consumers, so code test coverage has gone down a little, it's a JS-only change, and it's been reviewed by 2 devtools peers, so we should be pretty good here.
• String changes made/needed: None

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9085276 [details]
Bug 1571342 - unify save paths for console and netmonitor and fix the latter to not do docshell loads, r?Honza

Fixes a regression in Fx69 causing saving as HAR from the Netmonitor to silently fail. Approved for 69.0b15.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a969e02f8c9c

Comment 20

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 70.0a1 (2019.08.05) on Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 70.0a1 and 69.0b15 on Win 10 x64, Ubuntu 18.04 x64 and macOS 10.14.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9085276 [details]
Bug 1571342 - unify save paths for console and netmonitor and fix the latter to not do docshell loads, r?Honza

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1560178#c34.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/3c8c9595c316

Comment 23

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

It look like that the fix is not in 68.2.0esr build , verified with steps from Description on Windows 8.1 x64, Ubuntu 18.04 and on macOS 10.14 and the issue is still reproducible for me:

• Windows 8.1 x64 using build from treeherder : https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&selectedJob=269350751, build ID: 20191002005440
• Ubuntu 18.04 x64 : https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&selectedJob=269357405
• macOS 10.14: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&selectedJob=269334627

Comment 24

[:Gijs (out for now; he/him)]

Gah, it's because I messed up rebasing around bug 1517728, which added Cc to the list of imports from require("chrome") in DevToolsUtils.js

I pushed to try to confirm this is sufficient for a fix - it works for me locally with an artifact build. There should be builds soon on https://treeherder.mozilla.org/#/jobs?repo=try&revision=7060f595f461e1616df7fc453bba4cce623e0d21 .

Ryan, how do you want to handle landing this followup? ( https://hg.mozilla.org/try/rev/a92d87a0aec63e79429f7329399296fc20da3e6d )

Comment 25

[Ryan VanderMeulen [:RyanVM]]

We can just land the follow-up as a bustage fix. That said, should we be worried that no tests caught this?

Comment 26

[:Gijs (out for now; he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)

We can just land the follow-up as a bustage fix. That said, should we be worried that no tests caught this?

There's tests for the other consumer of the function that broke - but that other consumer and the test were added in bug 1517728 so they only exist in 69+, which is how I missed this. If the same thing happened on central/beta/release right now, it'd break tests.

Comment 27

[:Gijs (out for now; he/him)]

(In reply to Timea Zsoldos [:zstimi/tzsoldos], Desktop Release QA from comment #23)

It look like that the fix is not in 68.2.0esr build , verified with steps from Description on Windows 8.1 x64, Ubuntu 18.04 and on macOS 10.14 and the issue is still reproducible for me:

• Windows 8.1 x64 using build from treeherder : https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&selectedJob=269350751, build ID: 20191002005440
• Ubuntu 18.04 x64 : https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&selectedJob=269357405
• macOS 10.14: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&selectedJob=269334627

Can you verify the windows/mac builds on https://treeherder.mozilla.org/#/jobs?repo=try&revision=7060f595f461e1616df7fc453bba4cce623e0d21&selectedJob=269436630 do work correctly? Then we can land the follow-up.

Comment 28

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

(In reply to :Gijs (he/him) from comment #27)

(In reply to Timea Zsoldos [:zstimi/tzsoldos], Desktop Release QA from comment #23)

It look like that the fix is not in 68.2.0esr build , verified with steps from Description on Windows 8.1 x64, Ubuntu 18.04 and on macOS 10.14 and the issue is still reproducible for me:

• Windows 8.1 x64 using build from treeherder : https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&selectedJob=269350751, build ID: 20191002005440
• Ubuntu 18.04 x64 : https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&selectedJob=269357405
• macOS 10.14: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&selectedJob=269334627

Can you verify the windows/mac builds on https://treeherder.mozilla.org/#/jobs?repo=try&revision=7060f595f461e1616df7fc453bba4cce623e0d21&selectedJob=269436630 do work correctly? Then we can land the follow-up.

I verified on this builds using Windows 8.1 x64 and macOS 10.14 it's work correctly, I can confirm the issue is fixed.

Comment 29

[:Gijs (out for now; he/him)]

(In reply to Timea Zsoldos [:zstimi/tzsoldos], Desktop Release QA from comment #28)

(In reply to :Gijs (he/him) from comment #27)

Can you verify the windows/mac builds on https://treeherder.mozilla.org/#/jobs?repo=try&revision=7060f595f461e1616df7fc453bba4cce623e0d21&selectedJob=269436630 do work correctly? Then we can land the follow-up.

I verified on this builds using Windows 8.1 x64 and macOS 10.14 it's work correctly, I can confirm the issue is fixed.

Excellent, thanks. Ryan, can you push the change at https://hg.mozilla.org/try/rev/a92d87a0aec63e79429f7329399296fc20da3e6d ? Thank you!

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/a5cfa16c9b47

Comment 31

[:Gijs (out for now; he/him)]

Timea, can you re-attempt verification on esr68?

Comment 32

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I verified on 68.2.0esr build from treeherder: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&revision=ead3869b6258e95415a55ab603adfbb4b0d6ac5f&selectedJob=269670904 using Windows 10 x64 and macOS 10.13 it's work correctly, I can confirm the issue is fixed.