1398229 - "Save Link As..." on a link that requires auth doesn't work the same in a container tab

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P1)

Description

[(not currently active) Ted Mielczarek]

I noticed this on crash-stats.mozilla.com when trying to download minidumps. Getting the link to a raw minidump requires you to log in (and have specific permissions), and then it presents you a link. Clicking on the link works fine, but I like to "Save Link As..." to save them in a specific place (which works really well because Firefox remembers the last directory I chose per-site). This wasn't working properly for me recently. The "Save As" dialog would show the content-type as text/html, and give me a junk .html filename. I thought maybe it was a regression from e10s, but a little more testing showed that it's because I've been opening crash-stats links in container tabs recently (to keep my work Google login separate from my personal one).

Looking at the network requests in the browser console, what seems to be happening is that we do some sort of preflight network request when you click "Save Link As...", presumably to get the filename and content-type, but in this scenario that request doesn't have my session cookie for crash-stats and so crash-stats sends me a 302 redirect to the login page (which is text/html). I assume the network request is happening outside of the container context.

Comment 1

[Jonathan Kingston [:jkt] he/him]

I'm going to restrict access to this whilst we audit this just because it might be a contextual identity escape that impacts PB mode too.

:baku if you have any cycles (likely no one does at the moment though), this would be good to look at I think.

Comment 2

[Andrea Marchesini [:baku]]

We are currently using system principal. No OriginAttributes, no correct principal involved.
See https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1146-1155

ckerschb, you have been working on bug bug 1195555 and bug 1136055. Any thought?

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrea Marchesini [:baku] from comment #2)
> ckerschb, you have been working on bug bug 1195555 and bug 1136055. Any
> thought?

Mhm, the reason we used SystemPrincipal is to bypass some mixed content restrictions. What we could do (though ugly) is to use TYPE_DOCUMENT to bypass mixed content and also take the OA of this.principal, but create a new codebasePrincipal from the URI we are about to download:

let OA = this.principal.originAttributes;
let principal =
  Services.scriptSecurityManager.createCodebasePrincipal(makeURI(linkURL), OA);

I know it's not great, but that should work. Open for any discussion, this is just one potential way forward.
What do you think?

Comment 4

[Tanvi Vyas[:tanvi]]

What is the current content type?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas[:tanvi] from comment #4)
> What is the current content type?

It's TYPE_OTHER, see:
https://hg.mozilla.org/releases/mozilla-release/rev/3f64bf12d5f8#l1.28

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Tanvi, this is still marked tracking for FF57. Do we need to fix it or can we remove the tracking flag?

Comment 7

[Tanvi Vyas[:tanvi]]

Ted, can you check if this is also an issue in regular and private mode?

Comment 8

[Tanvi Vyas[:tanvi]]

Given how close we are to 57 launch, I think we have to wait for 58 on this one.

Comment 9

[Tanvi Vyas[:tanvi]]

baku, can you take this bug?  Probably the fix is using TYPE_DOCUMENT instead of TYPE_OTHER and using this.principal - https://hg.mozilla.org/releases/mozilla-release/rev/3f64bf12d5f8#l1.28

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to Tanvi Vyas[:tanvi] from comment #7)
> Ted, can you check if this is also an issue in regular and private mode?

What would you like me to test exactly? Logging in to the site in regular mode and then trying to save the link from private mode?

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> (In reply to Tanvi Vyas[:tanvi] from comment #7)
> > Ted, can you check if this is also an issue in regular and private mode?
> 
> What would you like me to test exactly? Logging in to the site in regular
> mode and then trying to save the link from private mode?

Yes, exactly.  And vice versa.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: mochitest

This is the test. Now we have to write the fix :)

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 1 - mochitest

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - save-as

Note that I'm still using TYPE_OTHER. Using TYPE_DOCUMENT breaks LoadInfo, which cannot be created from JS, since bug 1105556.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8926987 [details] [diff] [review]
part 2 - save-as

Review of attachment 8926987 [details] [diff] [review]:
-----------------------------------------------------------------

Baku, this seems like the wrong fix. Using TYPE_OTHER will be blocked as mixed content and now we are back to why we needed to fix Bug 1136055 in the first place (see also comment 3). But I see your problem, we can't create a channel with TYPE_DOCUMENT, because we have that special constructor for loadInfo with TYPE_DOCUMENT which we only allow from docshell - we also shouldn't change that. We could add a new contentPolicyType TYPE_DOWNLOAD or something and return early within the mixed content blocker? I don't know of other ways to bypass mixed content blocker (besides using systemPrincipal). Looking at nsMixedContentBlocker.cpp, it seems that only TYPE_WEBSOCKET gets a free pass at the moment (because it can't be created from within insecure context).

Tanvi, any other suggestions on how to bypass mixed content blocker? Using TYPE_MEDIA for example would allow the load but would update the UI, right? Maybe that is desireable in the end and we don't even want to return early which we would for TYPE_DOCUMENT, but rather have an indicator that something was loaded using an insecure connection.

I think I am for adding a new contentType, TYPE_DOWNLOAD which is treated as eMixedDisplay.

Comment 16

[Andrea Marchesini [:baku]]

Note that my mochitest implements a redirect and it passes correctly with this patch applied.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8926985 [details] [diff] [review]
part 1 - mochitest

Review of attachment 8926985 [details] [diff] [review]:
-----------------------------------------------------------------

Holy Moly, what an effort to write a test - thanks for doing that. Please address my questions and flag me again for review.

::: browser/components/contextualidentity/test/browser/browser_saveLink.js
@@ +1,5 @@
> +"use strict";
> +
> +const BASE_ORIGIN = "http://example.com";
> +const URI = BASE_ORIGIN +
> +  "/browser/browser/components/contextualidentity/test/browser/saveLink.sjs";

A more robust way of writing this could be:

const kTestPath = getRootDirectory(gTestPath)
                  .replace("chrome://mochitests/content", "http://example.com")
const kTestURI = kTestPath + "saveLink.sjs";

@@ +10,5 @@
> +add_task(async function setup() {
> +  // make sure userContext is enabled.
> +  await SpecialPowers.pushPrefEnv({"set": [
> +    ["privacy.userContext.enabled", true]
> +  ]});

I think you can just take that whole pushPrefEnv and place it at the beginning of add_task(async function test() {

@@ +21,5 @@
> +  info("Let's open a new window");
> +  let win = await BrowserTestUtils.openNewBrowserWindow();
> +
> +  info("Opening tab with UCI: 1");
> +  let tab = BrowserTestUtils.addTab(win.gBrowser, URI + "?UCI=1", {userContextId: 1});

Doesn't that end up creating a Systemprncipal within addTab() which in turn causes the this.principal within nsContextMenu.js to use that SystemPrincipal? I think we should pass a principal explicitly here, no?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrea Marchesini [:baku] from comment #16)
> Note that my mochitest implements a redirect and it passes correctly with
> this patch applied.

Please see previous comment, I guess there is a flaw in the test.

Comment 19

[Andrea Marchesini [:baku]]

> Doesn't that end up creating a Systemprncipal within addTab() which in turn
> causes the this.principal within nsContextMenu.js to use that
> SystemPrincipal? I think we should pass a principal explicitly here, no?

No. The principal is related to what is loaded in the tab. The tab will run a ContentPrincipal with OriginAttributes.UCI = 1.
Note that SystemPrincipal has always UCI = 0.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrea Marchesini [:baku] from comment #19)
> No. The principal is related to what is loaded in the tab. The tab will run
> a ContentPrincipal with OriginAttributes.UCI = 1.
> Note that SystemPrincipal has always UCI = 0.

Are you sure? Have a look at:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#1463

Comment 21

[Andrea Marchesini [:baku]]

This is the triggering principal, not the loading principal.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 1 - mochitest

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 2 - browser

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 3 - TYPE_INTERNAL_DOWNLOAD

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8927770 [details] [diff] [review]
part 1 - mochitest

Review of attachment 8927770 [details] [diff] [review]:
-----------------------------------------------------------------

This test (starting out with https and then redirecting to http) looks good to me now. thanks for updating.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8927771 [details] [diff] [review]
part 2 - browser

Review of attachment 8927771 [details] [diff] [review]:
-----------------------------------------------------------------

that also looks good.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8927772 [details] [diff] [review]
part 3 - TYPE_INTERNAL_DOWNLOAD

Review of attachment 8927772 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsMixedContentBlocker.cpp
@@ +496,5 @@
> +  // Quick return for TYPE_INTERNAL_DOWNLOAD. This is always alloed.
> +  if (aContentType == nsIContentPolicy::TYPE_INTERNAL_DOWNLOAD) {
> +    *aDecision = ACCEPT;
> +    return NS_OK;
> +  }

I don't think we should give it a free pass, we should rather treat it the same as TYPE_MEDIA, so just add it here:

https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#579

Comment 28

[Andrea Marchesini [:baku]]

Attachment: part 3 - TYPE_DOWNLOAD

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8927853 [details] [diff] [review]
part 3 - TYPE_DOWNLOAD

Review of attachment 8927853 [details] [diff] [review]:
-----------------------------------------------------------------

Ok. Thanks for updating. Just for the record, initially we were considering passing TYPE_DOCUMENT. Using TYPE_DOWNLOAD seems like the best option now. thanks for fixing.

Comment 30

[Tanvi Vyas[:tanvi]]

Comment on attachment 8927853 [details] [diff] [review]
part 3 - TYPE_DOWNLOAD

>diff --git a/dom/base/nsIContentPolicy.idl b/dom/base/nsIContentPolicy.idl
>--- a/dom/base/nsIContentPolicy.idl
>+++ b/dom/base/nsIContentPolicy.idl
>@@ -177,16 +177,21 @@ interface nsIContentPolicy : nsISupports
>   const nsContentPolicyType TYPE_IMAGESET = 21;
> 
>   /**
>    * Indicates a web manifest.
>    */
>   const nsContentPolicyType TYPE_WEB_MANIFEST = 22;
> 
>   /**
>+   * Indicates an save-as link download from the front-end code.
>+   */
>+  const nsContentPolicyType TYPE_DOWNLOAD = 43;
>+
Can we be more descriptive here?  We aren't talking about all downloads, just this very specific save-link-as download.  That should be clear in the content type.  TYPE_SAVE-AS_DOWNLOAD, for example?  And also should be detailed in the comments in nsMixedCotnetnBlocker.

Comment 31

[Andrea Marchesini [:baku]]

Attachment: part 3 - TYPE_SAVEAS_DOWNLOAD

I cannot use '-'. TYPE_SAVEAS_DOWNLOAD seems a good name to me. Alternatively: TYPE_SAVE_LINK_AS_DOWNLOAD.
Comment improved.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

This was pushed to inbound and then backed out for browser_saveLink.js leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b58d7dbfcf60e3dabbca270ff0c4c4d1b233edf0

https://treeherder.mozilla.org/logviewer.html#?job_id=145055417&repo=mozilla-inbound

Comment 33

[Tanvi Vyas[:tanvi]]

(In reply to Andrea Marchesini [:baku] from comment #31)
> Created attachment 8928016 [details] [diff] [review]
> part 3 - TYPE_SAVEAS_DOWNLOAD
> 
> I cannot use '-'. TYPE_SAVEAS_DOWNLOAD seems a good name to me.
> Alternatively: TYPE_SAVE_LINK_AS_DOWNLOAD.
> Comment improved.

Add one more comment in this block and you are good to go:
https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/dom/security/nsMixedContentBlocker.cpp#509

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6b4cebf12e3f
https://hg.mozilla.org/mozilla-central/rev/e7d2101e1aa9
https://hg.mozilla.org/mozilla-central/rev/314c6024ea27

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c7a66beb6674

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you're comfortable doing so.

Comment 37

[Andrea Marchesini [:baku]]

Comment on attachment 8927770 [details] [diff] [review]
part 1 - mochitest

Approval Request Comment
[Feature/Bug causing the regression]: Containers
[User impact if declined]: save-link-as loads the page using a SystemPrincipal and the wrong OriginAttributes.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: This set of patches introduces a new nsIContentPolicy type. This is used for save a link. It should be relatively low risk.
[String changes made/needed]: none.

Comment 38

[Gerry Chang [:gchang]]

Comment on attachment 8927770 [details] [diff] [review]
part 1 - mochitest

This is sec-low. Let's let it ride the train. Beta58-. Feel free to nominate if you disagree.

Comment 39

[Tanvi Vyas[:tanvi]]

baku, we are still missing a comment here:
https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/dom/security/nsMixedContentBlocker.cpp#509