1502448 - save pdf issue with pdf viewer

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect)

Description

[bugdjilla]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Steps to reproduce:

- open a pdf file on my disk with file/open
- save this file in another or same folder, with same or another name


Actual results:

- the file appears in pdfviewer, but the download window say fail
- no file appears in explorer after file-save or ctrl-s
- if i click on reload in download window, the file is correctly downloaded and appear suddenly in the folder in windows explorer


Expected results:

the file is saved

Comment 1

[Matthias Versen [:Matti]]

confirming with the latest nightly on windows 10 and this is a regression:

 4:18.64 INFO: No more inbound revisions, bisection finished.
 4:18.64 INFO: Last good revision: 5d23b6e0b74d74eee52d7af5fa47573ef8afe979
 4:18.64 INFO: First bad revision: 9a2b02fe351bd65f6850a5c80b91dd0eec4a878a
 4:18.64 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5d23b6e0b74d74eee52d7af5fa47573ef8afe979&tochange=9a2b02fe351bd65f6850a5c80b91dd0eec4a878a

Comment 2

[Alice0775 White]

Error in browser console:

Security Error: Content at resource://pdf.js/web/viewer.html may not load or link to file:///D:/aaa/original.pdf.

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebBrowserPersist.savePrivacyAwareURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 523"  data: no]
internalPersist
chrome://global/content/contentAreaUtils.js:523:5
continueSave
chrome://global/content/contentAreaUtils.js:438:5
internalSave/<
chrome://global/content/contentAreaUtils.js:392:7

Comment 3

[:Gijs (he/him)]

Security-sensitive out of an abundance of caution.

(In reply to bugdjilla from comment #0)
> Steps to reproduce:
> 
> - open a pdf file on my disk with file/open
> - save this file in another or same folder, with same or another name

Save how? Using "File > save as...", or a button in the pdfjs UI, or something else?

> Actual results:
> 
> - the file appears in pdfviewer,

I don't understand how this can even work in this case. :bdahl, can you expand on that? Why is the resource: doc allowed to load the file URI in the first place?

> but the download window say fail
> - no file appears in explorer after file-save or ctrl-s

> - if i click on reload in download window, the file is correctly downloaded
> and appear suddenly in the folder in windows explorer

This doesn't make sense either. Why is the triggering principal correct the first time but wrong the second time? Paolo, how does that work?

Comment 4

[Matthias Versen [:Matti]]

> > - open a pdf file on my disk with file/open
> > - save this file in another or same folder, with same or another name
> 
> Save how? Using "File > save as...", or a button in the pdfjs UI, or
> something else?

I used Ctrl+S in my regression test and that is the same as File->save as
Using the download button in the pdf.js UI works as expected.

Comment 5

[:Gijs (he/him)]

(In reply to Matthias Versen [:Matti] from comment #4)
> > > - open a pdf file on my disk with file/open
> > > - save this file in another or same folder, with same or another name
> > 
> > Save how? Using "File > save as...", or a button in the pdfjs UI, or
> > something else?
> 
> I used Ctrl+S in my regression test and that is the same as File->save as
> Using the download button in the pdf.js UI works as expected.

OK. Maybe we just need to write a special case into that codepath so it does whatever pdfjs UI does, but that does feel a bit hacky.

Comment 6

[Brendan Dahl [:bdahl]]

(In reply to :Gijs (he/him) from comment #3)
> Save how? Using "File > save as...", or a button in the pdfjs UI, or
> something else?
> 
> > Actual results:
> > 
> > - the file appears in pdfviewer,
> 
> I don't understand how this can even work in this case. :bdahl, can you
> expand on that? Why is the resource: doc allowed to load the file URI in the
> first place?

The resource URL doesn't really load the file. The chrome privileged mime type handler opens the file and pipes the data into pdf.js (content privileged).

Comment 8

[:Paolo Amadini]

As far as I know, "Save As" should try to load the source "file:" URL directly, in the same way that re-saving a local web page as "HTML only" would do. From the error, it seems that savePrivacyAwareURI is incorrectly trying to save the resource using some principal from the document that is actually loaded in the page, that in this case is different from the origin of the content that is displayed. Gijs, does this ring any bells in relation to bug 1469916?

Comment 9

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #8)
> As far as I know, "Save As" should try to load the source "file:" URL
> directly, in the same way that re-saving a local web page as "HTML only"
> would do. From the error, it seems that savePrivacyAwareURI is incorrectly
> trying to save the resource using some principal from the document that is
> actually loaded in the page, that in this case is different from the origin
> of the content that is displayed. Gijs, does this ring any bells in relation
> to bug 1469916?

I'll have a look, but if this was really an issue with it being the 'wrong' principal for the toplevel item being saved, then I'd expect 'save as' to be broken for local HTML files, too, and it's not.

Meanwhile, I'd still like an answer to my question in comment #3:

(In reply to :Gijs (he/him) from comment #3)
> > - if i click on reload in download window, the file is correctly downloaded
> > and appear suddenly in the folder in windows explorer
> 
> This doesn't make sense either. Why is the triggering principal correct the
> first time but wrong the second time? Paolo, how does that work?

What principal is the download panel using, and why isn't it the same as the one used the first time?

Comment 10

[:Gijs (he/him)]

So the issue in the original save is that we call frameLoader.startPersistence, here:

https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/toolkit/content/contentAreaUtils.js#172

and that creates an nsIWebBrowserPersistDocument document instance for the remote (ie non-same-process) document, which gets the document principal, which it gets off the loaded document's node principal:

https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp#185-192

which is (arguably) correct. PDF.JS is *weird* in that it loads a resource: thing that still gets the file: data - as comment 6 implies, pdfjs gets the data via some indirection via chrome code.


We could potentially work around this by creating a special case for the pdf mimetype or pdfjs specifically, but that feels pretty hacky - I just don't really have better ideas. Documents that don't have access to themselves are, uh, not common.

Meanwhile, I'm still concerned that the downloads panel seems to just make up its own principal, and I want to know how that happens.

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #10)
> We could potentially work around this by creating a special case for the pdf
> mimetype or pdfjs specifically, but that feels pretty hacky - I just don't
> really have better ideas. Documents that don't have access to themselves
> are, uh, not common.

:bdahl, :ckerschb, do you have ideas about how/where we should fix this? :-)

Comment 12

[:Paolo Amadini]

(In reply to :Gijs (he/him) from comment #9)
> What principal is the download panel using, and why isn't it the same as the
> one used the first time?

There are still two different download architectures that we run in parallel, and they are handled in Downloads.jsm by different Saver objects, the DownloadCopySaver and the DownloadLegacySaver. We eventually want to remove DownloadLegacySaver, since it simplifies code and helps with Chrome extensions compatibility, but that requires a lot of work.

At the moment, downloads started by nsIWebBrowserPersist use DownloadLegacySaver through the nsITransfer interface. This interface assumes that any security checks have already been performed by implementations before creating the nsITransfer object. We don't pass or store any security meta-data about the request, like the triggering principal or the referrer policy, but we store the processed information like the final computed referrer. This is because we want to make all the meta-data safely serializable and the download resumable in a different browser session. The "Web Page, Complete" and text file cases are exceptions since we can't resume those properly anyways.

If the network request doesn't complete successfully, the next invocation of the methods on DownloadLegacySaver behaves just like DownloadCopySaver, using the same meta-data that would be used in a different browser session. This object is also created directly in various cases, like downloads started by extensions, possibly pasting raw URLs in the Downloads View in the Library window or in the Downloads Panel, and when retrying an old history download from the Library window. In some of these cases we may only know the URL without any extra meta-data. All of these network requests are executed with the system principal.

Comment 13

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #12)
> All of these network requests are executed with the system principal.

OK. I filed bug 1507773 for fixing this.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #11)
> (In reply to :Gijs (he/him) from comment #10)
> > We could potentially work around this by creating a special case for the pdf
> > mimetype or pdfjs specifically, but that feels pretty hacky - I just don't
> > really have better ideas. Documents that don't have access to themselves
> > are, uh, not common.
> 
> :bdahl, :ckerschb, do you have ideas about how/where we should fix this? :-)

How did this work before? I suppose we were using the SystemPrincipal as the TriggeringPrincipal, right? Now after Bug 1469916 we are using the 'correct' ContentPrincipal. To me it seems there is no hacky way around this problem and if possible we should try to isolate the corner case at the source of the problem. So special casing, hacking pdfjs rather than doing something hacky somewhere else in the codebase which could have other side effects.

Comment 15

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> (In reply to :Gijs (he/him) from comment #11)
> > (In reply to :Gijs (he/him) from comment #10)
> > > We could potentially work around this by creating a special case for the pdf
> > > mimetype or pdfjs specifically, but that feels pretty hacky - I just don't
> > > really have better ideas. Documents that don't have access to themselves
> > > are, uh, not common.
> > 
> > :bdahl, :ckerschb, do you have ideas about how/where we should fix this? :-)
> 
> How did this work before? I suppose we were using the SystemPrincipal as the
> TriggeringPrincipal, right?

Yes.

> Now after Bug 1469916 we are using the 'correct'
> ContentPrincipal. To me it seems there is no hacky way around this problem
> and if possible we should try to isolate the corner case at the source of
> the problem. So special casing, hacking pdfjs rather than doing something
> hacky somewhere else in the codebase which could have other side effects.

Unfortunately, pdfjs code doesn't get a look in on this codepath. The browser 'save as' code calls into the DOM webbrowserpersist code without checking anything to do with pdfjs. The only way we can fix this from the pdf js side is to change its principal, but I'm pretty sure the current setup is intentional to avoid other non-pdfjs content being able to touch the pdfjs content...

If we do need to add specialcasing in the non-pdfjs content, where do you think the best place would be? :-)

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #15)
> If we do need to add specialcasing in the non-pdfjs content, where do you
> think the best place would be? :-)

Hey Smaug,

we recently updated the TriggeringPrincipal from using a SystemPrincipal, which basically bypassed those security checks to using a CodeBasePrincipal which is no subject of Security Checks (for the changes see Bug 1469916). While using a ContentPrincipal in that case solves other issues it introduces a new one, namely:

> Security Error: Content at resource://pdf.js/web/viewer.html may not load or link to file:///D:/aaa/original.pdf.

Since it seems that we can't fix the problem within PDF-JS (see comment 15) it seems we would have to add some special casing within the ScriptSecuritymanager to allow that case. Now the question is, would we even be willing to accept an update there. If yes, where would we add that case so that resource:// can access file:///D:/aaa/original.pdf?

Comment 17

[Brendan Dahl [:bdahl]]

(In reply to :Gijs (he/him) from comment #11)
> :bdahl, :ckerschb, do you have ideas about how/where we should fix this? :-)

I don't know of any easy ways. The way pdf.js sets up all it's channels has always been kind of hacky. It'd be nice if we had some better way of supporting use case where we want to show the original url in the awesome bar, but behind the scenes it's totally different channel running things.

Comment 18

[:Gijs (he/him)]

(In reply to Brendan Dahl [:bdahl] from comment #17)
> (In reply to :Gijs (he/him) from comment #11)
> > :bdahl, :ckerschb, do you have ideas about how/where we should fix this? :-)
> 
> I don't know of any easy ways. The way pdf.js sets up all it's channels has
> always been kind of hacky. It'd be nice if we had some better way of
> supporting use case where we want to show the original url in the awesome
> bar, but behind the scenes it's totally different channel running things.

Hm. Short-term, maybe the easiest way to fix this is to make the front-end "save as" code do something else in the pdf.js case for any document URLs that don't have URI_LOADABLE_BY_ANYONE. Does the parent process know whether the document is being loaded by pdfjs?

Comment 19

[:Gijs (he/him)]

Bah, meant to ni for my last comment.

Comment 20

[Brendan Dahl [:bdahl]]

(In reply to :Gijs (he/him) from comment #18)
> Hm. Short-term, maybe the easiest way to fix this is to make the front-end
> "save as" code do something else in the pdf.js case for any document URLs
> that don't have URI_LOADABLE_BY_ANYONE. Does the parent process know whether
> the document is being loaded by pdfjs?

It's been awhile since I looked at this, but I don't think the parent process knows that pdf.js is going to load the PDF.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

I don't think I have useful input here. I would need to review all of bug 1469916 to understand this issue.

Could we use expanded principal for pdf.js, or use expanded principal for saving?

Comment 22

[:Gijs (he/him)]

Attachment: Bug 1502448 - work around principal issues with pdfjs and saving local files, r?jkt!,jaws!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0b51a908bec
work around principal issues with pdfjs and saving local files, r=jkt,jaws

Comment 24

[Natalia Csoregi [:nataliaCs]]

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

Comment 25

[:Gijs (he/him)]

Comment on attachment 9033940 [details]
Bug 1502448 - work around principal issues with pdfjs and saving local files, r?jkt!,jaws!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1469916

User impact if declined: Can't use "Save page as" to save PDF that is already on disk

Is this code covered by automated tests?: No

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 is a targeted fix in the file saving code, so it should be pretty safe.

We should verify the bug on nightly before uplift.

The main alternative is to just let this ride with 66; this has also affected 63 and 64, and there are workarounds (using the in-pdfjs button to save the file, or using the downloads panel UI to restart the "download") so in that sense waiting another release wouldn't be a terrible prospect.

String changes made/needed: none

Comment 26

[:Gijs (he/him)]

I don't really know why 64 was set unaffected; this was reported on 63, the regressing bug landed there, and I'm 99% sure this affects both 63 and 64. So marking 64 wontfix as we won't be able to uplift this fix there.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9033940 [details]
Bug 1502448 - work around principal issues with pdfjs and saving local files, r?jkt!,jaws!

[Triage Comment]
Seems pretty edge-case to me, but given that all the new code is special-cased for just pdf.js (which makes the risk pretty low IMO), I'm not particularly worried about taking it at this point in the cycle. Agreed that we should get some QA verification. Approved for 65.0b10.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/274d22447873

Comment 29

[Vlad Lucaci, Desktop QA (:vlucaci)]

I have managed to reproduce this issue using Nightly 65.0a1 (BuildId:20181026220839).

This issue is verified fixed using Firefox 66.0a1 (BuildId:20190109214248) and Firefox 65.0b19 (BuildID:20190107180200) on Windows 10 64bit.