1923368 - Extend browser.download.open_pdf_attachments_inline to other file types

Status: NEW

(Core :: DOM: Navigation, enhancement)

Description

[Pier Angelo Vendrame]

In Tor Browser, we've extended browser.download.open_pdf_attachments_inline to cover all the types that are set to be opened in the browser instead of using a third-party application.
However, we would like to upstream our patch because we think that also some Firefox users would appreaciate it.
I don't know if the approach works well for Firefox: I added an additional pref to keep also the current behavior, but removing the special handling for PDFs would simplify the code.

Comment 1

[Pier Angelo Vendrame]

Attachment: WIP: Bug 1923368 - Extend browser.download.open_pdf_attachments_inline to other file types.

Comment 2

[Pier Angelo Vendrame]

Hi, you were the author of the patch I've been working on, could you give me an advice for this?
Otherwise do you know who I could ask to?
Thanks in advance.

Comment 3

[:Gijs (he/him)]

Removing the PDF-specificness would make sense to me, though I suspect that some users will want per-filetype configuration... So unless you're willing to tackle that (and that would be a larger project with some UX input etc.) then perhaps it's best to leave it around for now?

The pref name might do with being slightly more specific (open_attachments_inline reflects a bit more precisely what this does than "ignore content disposition", IMO - that could also mean that we treat inline files as attachments! :-) ).

Finally, we would want to make sure that we don't do this for HTML files as that would mean files that get served with CD: attachment get rendered direct from the server which the sending websites do not expect, and can potentially cause security issues as script will run on that origin.

(In fact, similar concern for SVG files, potentially...)

So I think this will need a little more consideration. Does that help? :-)

Comment 4

[Pier Angelo Vendrame]

Thanks for your reply!

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

Removing the PDF-specificness would make sense to me, though I suspect that some users will want per-filetype configuration... So unless you're willing to tackle that (and that would be a larger project with some UX input etc.) then perhaps it's best to leave it around for now?

Actually the UX is already implemented 😄.
open_pdf_attachments_inline is triggered only when PDFs are set as "Open in Firefox" in the "Applications" section of about:preferences#general.
Otherwise, if it's "Save file", the pref is ignored and the file is downloaded immediately (or the system file dialog is shown, if you activated "Always ask you where to save files").
With the patch I provided, the behavior would be the same.

It it's "Always ask", the download confirmation dialog appears, and there's the possibility to open the file in the browser.
Which reminds me: this option currently skips browser.download.open_pdf_attachments_inline. I noticed that while developing my patch.
We would like the behavior to be consistent with "Open in Firefox" (I created Bug 1923835 for that).

I also tried to address it, but it wasn't trivial (I had a PoC that worked more or less, but my debug build complained that it resulted in a memory leak - I got a message like "You're leaking the world" 😓; sadly, I don't have that code anymore).

The pref name might do with being slightly more specific (open_attachments_inline reflects a bit more precisely what this does than "ignore content disposition", IMO - that could also mean that we treat inline files as attachments! :-) ).

Will do in the diff I created, thanks!

Finally, we would want to make sure that we don't do this for HTML files as that would mean files that get served with CD: attachment get rendered direct from the server which the sending websites do not expect, and can potentially cause security issues as script will run on that origin.

(In fact, similar concern for SVG files, potentially...)

So I think this will need a little more consideration. Does that help? :-)

Does my point from above answer this?
HTML is "Save File" by default (I believe, at least it is for me, and I'm pretty sure I haven't customized any per-type settings).
Only AVIF and WebP are the only files which are set to "Open with Firefox" on my system, in addition to PDFs.

Comment 5

[:Gijs (he/him)]

(In reply to Pier Angelo Vendrame from comment #4)

Thanks for your reply!

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

Removing the PDF-specificness would make sense to me, though I suspect that some users will want per-filetype configuration... So unless you're willing to tackle that (and that would be a larger project with some UX input etc.) then perhaps it's best to leave it around for now?

Actually the UX is already implemented 😄.
open_pdf_attachments_inline is triggered only when PDFs are set as "Open in Firefox" in the "Applications" section of about:preferences#general.

Well, but users who have PDFs set to "open in Firefox" may want CD: attachment PDFs to download to disk (and then either open from disk, which is the default, or not, for which there is a different about:config pref...). The open_pdf_attachments_inline changes this so PDFs open directly from the internet instead of downloading to disk at all.

If we instead had a pref that governed all file types, I'd expect that people may want to make different decisions for different filetypes, even if all those file types are set to "open in Firefox".

Does that make sense?

Finally, we would want to make sure that we don't do this for HTML files as that would mean files that get served with CD: attachment get rendered direct from the server which the sending websites do not expect, and can potentially cause security issues as script will run on that origin.

(In fact, similar concern for SVG files, potentially...)

So I think this will need a little more consideration. Does that help? :-)

Does my point from above answer this?
HTML is "Save File" by default (I believe, at least it is for me, and I'm pretty sure I haven't customized any per-type settings).

You have definitely customized something because in a clean profile HTML is not in the list. :-)

Only AVIF and WebP are the only files which are set to "Open with Firefox" on my system, in addition to PDFs.

Yes, but it's possible to configure SVGs and XML files like this (but it isn't the default) and both of those can include script.

Comment 6

[Pier Angelo Vendrame]

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

(In reply to Pier Angelo Vendrame from comment #4)

Thanks for your reply!

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

Removing the PDF-specificness would make sense to me, though I suspect that some users will want per-filetype configuration... So unless you're willing to tackle that (and that would be a larger project with some UX input etc.) then perhaps it's best to leave it around for now?

Actually the UX is already implemented 😄.
open_pdf_attachments_inline is triggered only when PDFs are set as "Open in Firefox" in the "Applications" section of about:preferences#general.

Well, but users who have PDFs set to "open in Firefox" may want CD: attachment PDFs to download to disk (and then either open from disk, which is the default, or not, for which there is a different about:config pref...). The open_pdf_attachments_inline changes this so PDFs open directly from the internet instead of downloading to disk at all.

If we instead had a pref that governed all file types, I'd expect that people may want to make different decisions for different filetypes, even if all those file types are set to "open in Firefox".

Does that make sense?

Sorry, in all of this I forgot about browser.download.start_downloads_in_tmp_dir isn't the default for Firefox, but is set in my Firefox profile and the default on Tor Browser.
So, I wrote so far was on the assumption that the download happens in a temporary directory, and you're not interested in keeping it eventually.
Maybe I need to think about all of this with fresher mind.

Finally, we would want to make sure that we don't do this for HTML files as that would mean files that get served with CD: attachment get rendered direct from the server which the sending websites do not expect, and can potentially cause security issues as script will run on that origin.

(In fact, similar concern for SVG files, potentially...)

So I think this will need a little more consideration. Does that help? :-)

Does my point from above answer this?
HTML is "Save File" by default (I believe, at least it is for me, and I'm pretty sure I haven't customized any per-type settings).

You have definitely customized something because in a clean profile HTML is not in the list. :-)

I tried on a fresh profile... And I admit I don't know how HTML ended on that list in my main profile in the first place 😅.

Comment 7

[Pier Angelo Vendrame]

I thought about this Bug a bit more.
I think the feature of choosing what to do with each file type would be very niche, and keeping two preferences would work for me.
So, I think my next actionable items are to exclude the various formats that can run scripts (HTML, SVG..., I wonder if there's already a list) and to add tests.