1768069 - Open image in new tab if image is webp triggers the download dialog

Status: RESOLVED FIXED

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

Description

[Troy Janda]

Attachment: error.png

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

Steps to reproduce:

I was on https://www.ghacks.net/2022/05/05/mozilla-firefox-101-beta-download-prompt-for-opening-and-saving-files/ and right clicked on a webp image and tried to select open image in new tab, a download dialog opened and a tab opened but I had to select open with Firefox within the download dialog to see the image

Actual results:

The tab opened and a download prompt appeared asking me what to do

Expected results:

Should have opened the webp image in a new tab without prompting to download.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::ImageLib' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Timothy Nikkel (:tnikkel)]

Doesn't happen with all webp images, for example this one https://upload.wikimedia.org/wikipedia/commons/thumb/a/a1/Johnrogershousemay2020.webp/1200px-Johnrogershousemay2020.webp

Another example that doesn't have this bug is https://www.gstatic.com/webp/gallery/1.webp

Comparing response headers the ones that work have content-type: image/webp, the ghacks one does not. ghacks also has x-content-type-options: nosniff. I'm not too familiar with this area, but that might be why? If we are told not to sniff and also not given a content type we have to download unless we assume it based on the url (but that's not correct either as webservers are configured to send webp with urls that end in jpg).

Comment 3

[Timothy Nikkel (:tnikkel)]

Gijs, are you familiar with area, do you know what might be going on here?

Comment 4

[:Gijs (he/him)]

We just open media URLs viewed this way in a new tab/window, cf. https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/browser/base/content/nsContextMenu.js#1517-1523 .

So yes, what happens afterwards depends on the server headers etc., as you outlined in comment #2 .

FWIW, with default prefs, the webp file downloads and displays in a new tab - webp images are set to "view in browser" by default, so we start a download due to the headers, realize it's a webp file, and then display it immediately. The fact that this doesn't happen for OP probably means that they've got downloads (and/or webp downloads specifically) configured to prompt rather than the default behaviour.

It would be a good idea to add some kind of mechanism that forced display as an image (or video/audio, where applicable), also because it'd mean SVG things viewed this way don't get "upgraded" to being viewed as documents (which will run script, which is unexpected given it's an image). But right now such a mechanism doesn't exist, so there isn't anything better frontend can do here.

Not sure that's fully answered your question here, :tnikkel - lmk if you were looking for something else? :-)

Comment 5

[Timothy Nikkel (:tnikkel)]

I guess my question was whether we take the correct action based on the server headers. Chrome does not download the image, Safari does ask to download it.

Comment 6

[:Gijs (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)

I guess my question was whether we take the correct action based on the server headers. Chrome does not download the image, Safari does ask to download it.

I don't know. This is a document load, so AIUI the nosniff header shouldn't do anything and we should still sniff, and potentially display as an image if the detected content type supports it.

I think that might be a dupe of bug 1671567 then? That bug seems stuck. Christoph/Anne, am I missing something, and/or can we get that bug un-stuck?

Comment 7

[Anne (:annevk)]

We do have to honor nosniff for navigations, but the exact details are a bit unclear. I think what happens is that Chrome still sniffs, but if they end up with a type that would execute code (such as text/html), they then do honor nosniff and move to download or display it as text/plain instead.

I think that's an angle we didn't test enough when implementing it for navigations and that's why we keep hitting these one-off bugs. Coupled with the fact that the state of the specification is messy.

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

Anne is correct, nosniff is probably not tested well enough for navigations (Meta Bug 471020).

Hey Tom, I remember you were active when we started to support nosniff in Firefox (Bug 471020). Something you are interested in sorting - maybe even together with Bug 1671567?

Comment 9

[Tom Schuster (MoCo)]

So when testing this in Chrome and going to https://www.ghacks.net/wp-content/uploads/2022/05/Firefox-101-beta-brings-back-the-download-prompt.webp they just display the picture. That means they are sniffing the content and deciding to display as an image. That just seems like the wrong behavior to me.

The question here is should we intentionally weaken no-sniff and is there some actual problem this is causing?

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:aosmond, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 11

[Tom Schuster (MoCo)]

I am investigating if we could just allow sniffing of images in top-level documents.

Comment 12

[Tom Schuster (MoCo)]

It seems like we used to support sniffing with empty Content-Types even with X-Content-Type-Options: nosniff, but changed to the more strict behavior with bug 1594766.

Comment 13

[Tom Schuster (MoCo)]

Attachment: WIP: Bug 1768069 - Allow sniffing top-level documents as image.

Comment 14

[Anne (:annevk)]

This might have been resolved offline, but I wanted to give an answer to the question in comment 9. Essentially we roughly want equivalent behavior for these kind of navigation flows to other browsers as otherwise we end up breaking user expectations. We might also be "breaking" websites if they navigate to such images in nested browsing contexts.

At some point in the past we attempted to honor nosniff for <img>, but that broke a lot of content. So at this point nosniff really means "don't execute" (which includes CSS, HTML, but unsure about PDF). For subresources it's clearly defined in Fetch, interoperable, and indeed only applicable to JavaScript and CSS, but for navigations we don't have a good handle on a definition yet. For that we need to essentially match Chromium/WebKit which I believe roughly have the "no execute" semantics, but there's a lot of cases to consider and there's no finished comprehensive test suite. There's https://github.com/web-platform-tests/wpt/pull/30403, but it has a number of unresolved issues.

Comment 15

[Tom Schuster (MoCo)]

Thanks for commenting on this Anne. We indeed have been discussing this in more detail outside this bug. I am going to try and summarize our findings.

The fetch spec only defines no-sniff handling for script-like targets (so Workers, <script> etc.) and style. As mentioned above. It doesn't say anything as far as I can tell for document or sub-document, which this bug is about.

The mimesniff spec actually talks about this:

If the supplied MIME type is undefined or if the supplied MIME type’s essence is "unknown/unknown", "application/unknown", or "/", execute the rules for identifying an unknown MIME type with the sniff-scriptable flag equal to the inverse of the no-sniff flag and abort these steps.

scriptable types are xml, html and pdf. So per that spec we should not sniff these scriptable types when we have the no-sniff flag. I think this matches Chrome's behavior as well.

Right now we don't seem to have a good way of only allowing the sniffing of specific non-scriptable types.

Comment 16

[:Gijs (he/him)]

(In reply to Tom Schuster (MoCo) from comment #15)

Thanks for commenting on this Anne. We indeed have been discussing this in more detail outside this bug. I am going to try and summarize our findings.

The fetch spec only defines no-sniff handling for script-like targets (so Workers, <script> etc.) and style. As mentioned above. It doesn't say anything as far as I can tell for document or sub-document, which this bug is about.

The mimesniff spec actually talks about this:

If the supplied MIME type is undefined or if the supplied MIME type’s essence is "unknown/unknown", "application/unknown", or "/", execute the rules for identifying an unknown MIME type with the sniff-scriptable flag equal to the inverse of the no-sniff flag and abort these steps.

scriptable types are xml, html and pdf. So per that spec we should not sniff these scriptable types when we have the no-sniff flag. I think this matches Chrome's behavior as well.

Right now we don't seem to have a good way of only allowing the sniffing of specific non-scriptable types.

Can we not use the request nsIContentPolicy to determine if we're looking for script?

Comment 17

[Tom Schuster (MoCo)]

Can we not use the request nsIContentPolicy to determine if we're looking for script?

I don't really understand the question. We already use the nsIContentPolicyType to to determine if something is a document/sub-document: https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#2752-2753. We then set the SkipContentSniffing flag. I think what we have to do is at least change nsUnknownDecoder to allow the sniffing of all non-scriptable types (so everything that isn't html, xml, pdf) even with the flag set. I will investigate this further, but I want to be really careful to not allow more sniffing than necessary.

Comment 18

[:Gijs (he/him)]

(In reply to Tom Schuster (MoCo) from comment #17)

Can we not use the request nsIContentPolicy to determine if we're looking for script?

I don't really understand the question. We already use the nsIContentPolicyType to to determine if something is a document/sub-document: https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#2752-2753. We then set the SkipContentSniffing flag. I think what we have to do is at least change nsUnknownDecoder to allow the sniffing of all non-scriptable types (so everything that isn't html, xml, pdf) even with the flag set. I will investigate this further, but I want to be really careful to not allow more sniffing the necessary.

Sorry, I think the multiple meanings of "type" here confused me. Your elaboration makes sense to me. Worth bearing in mind that image/svg+xml loaded as a standalone document is also executable. :-(

Comment 19

[Tom Schuster (MoCo)]

Attachment: WIP: Bug 1768069 - Always allow sniffing non-scriptable MIME types.

This largely reverts Bug 1594766, but still X-Content-Type-Options: no-sniff for so called
scriptable types (html, xml and pdf)

Comment 20

[Tom Schuster (MoCo)]

I tried some local testcase in Chrome and surprisingly Chrome also showed the image as text! After a lot head-scratching I think I figured out that the original test case is sending the X-Content-Type-Options: nosniff header twice and somehow this seems to make Chrome ignore it altogether?

Comment 21

[Tom Schuster (MoCo)]

I opened a Chromium bug about them not following the spec while parsing the XCTO header. Freddy tested Safari and they seem to follow Firefox and trigger a download and don't show the image.

I have a few patches that add tests and improve some of the code.

Comment 22

[Tom Schuster (MoCo)]

Attachment: Bug 1768069 - Remove unused security.requireHTMLsuffix pref. r?freddyb

This pref was never used and the function always returns early because mRequireHTMLsuffix is false.

Comment 23

[Tom Schuster (MoCo)]

Attachment: Bug 1768069 - Add test for duplicated XCTO: nosniff header. r?freddyb

Depends on D149768

Comment 24

[Tom Schuster (MoCo)]

Attachment: Bug 1768069 - Add PDF no-sniff test. r?freddyb

This also shares the server implementation.

Depends on D149769

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3124f43eda4
Remove unused security.requireHTMLsuffix pref. r=freddyb,necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/bd68a2c87342
Add test for duplicated XCTO: nosniff header. r=freddyb
https://hg.mozilla.org/integration/autoland/rev/45cbe4926347
Add PDF no-sniff test. r=freddyb

Comment 26

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/34531 for changes under testing/web-platform/tests

Comment 27

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/f3124f43eda4
https://hg.mozilla.org/mozilla-central/rev/bd68a2c87342
https://hg.mozilla.org/mozilla-central/rev/45cbe4926347

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 29

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

I could not reproduce this issue on Firefox 102.0 on Windows 10 64-bits. Would you be so kind as to verify the fix on the latest beta/nightly?
Thank you.

Comment 30

[Troy Janda]

Not using 102 I am on 103 and it still does it on there.

Comment 31

[Tom Schuster (MoCo)]

Sorry, we should specify the original issue as reported wasn't fixed. It's a bug with the website and another bug in Chrome causes this to work there.

Comment 32

[Troy Janda]

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

We just open media URLs viewed this way in a new tab/window, cf. https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/browser/base/content/nsContextMenu.js#1517-1523 .

So yes, what happens afterwards depends on the server headers etc., as you outlined in comment #2 .

FWIW, with default prefs, the webp file downloads and displays in a new tab - webp images are set to "view in browser" by default, so we start a download due to the headers, realize it's a webp file, and then display it immediately. The fact that this doesn't happen for OP probably means that they've got downloads (and/or webp downloads specifically) configured to prompt rather than the default behaviour.

It would be a good idea to add some kind of mechanism that forced display as an image (or video/audio, where applicable), also because it'd mean SVG things viewed this way don't get "upgraded" to being viewed as documents (which will run script, which is unexpected given it's an image). But right now such a mechanism doesn't exist, so there isn't anything better frontend can do here.

Not sure that's fully answered your question here, :tnikkel - lmk if you were looking for something else? :-)

I have not set any setting to prompt to download webp, also this happens on a new profile with nothing but defaults without addons.