After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 610586 - Open download action often fails
Open download action often fails
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
unspecified
Other All
: Normal normal
: ---
Assigned To: Carlos Garcia Campos
Epiphany Maintainers
: 651117 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-02-21 08:25 UTC by Diego Escalante Urrelo (not reading bugmail)
Modified: 2016-05-09 14:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Don't check mime again in ephy_file_launch_handler (1.16 KB, patch)
2010-02-21 08:25 UTC, Diego Escalante Urrelo (not reading bugmail)
none Details | Review
download: Browse to file if it cannot be opened (1.07 KB, patch)
2016-02-27 19:02 UTC, Michael Catanzaro
none Details | Review
download: Allow users to open "unsafe" files (1.67 KB, patch)
2016-02-27 19:02 UTC, Michael Catanzaro
needs-work Details | Review
download: Browse to file if it cannot be opened (1.07 KB, patch)
2016-03-05 19:48 UTC, Michael Catanzaro
committed Details | Review
Improve some FIXMEs (1.72 KB, patch)
2016-03-05 19:48 UTC, Michael Catanzaro
committed Details | Review
download: Get rid of EPHY_DOWNLOAD_ACTION_DO_NOTHING (2.77 KB, patch)
2016-03-05 19:48 UTC, Michael Catanzaro
committed Details | Review
download: Remove EPHY_DOWNLOAD_ACTION_AUTO (4.29 KB, patch)
2016-03-05 19:48 UTC, Michael Catanzaro
committed Details | Review
download: Add an assertion (806 bytes, patch)
2016-03-05 19:48 UTC, Michael Catanzaro
committed Details | Review
download: Further simplify ephy_download_do_download_action() (1.74 KB, patch)
2016-03-05 19:48 UTC, Michael Catanzaro
none Details | Review
download: Handle focus stealing prevention properly (4.52 KB, patch)
2016-03-05 19:48 UTC, Michael Catanzaro
committed Details | Review

Description Diego Escalante Urrelo (not reading bugmail) 2010-02-21 08:25:54 UTC
This is causing "Open" action in download dialog to fail sometimes since webkit
-for example- says the file is text but locally we see x-python or something.
So if you click on a download link to a script and select Open to see it in
gedit (as the dialog tells you) it will fail silently.
I'm not completely sure how unsafe this would be, it's 3 AM here. Opinions
welcome.
Comment 1 Diego Escalante Urrelo (not reading bugmail) 2010-02-21 08:25:56 UTC
Created attachment 154308 [details] [review]
Don't check mime again in ephy_file_launch_handler

ephy_file_launch_handler is used to launch downloaded files which mime types we
already know and have checked against our safe mime database (selecting Open on
the download file dialog) or when the situation is safe enough (right click on
a image and select Open image).
We shouldn't expect this to check safeness anyway.

Bug #610586
Comment 2 Diego Escalante Urrelo (not reading bugmail) 2010-02-21 08:37:01 UTC
Mmm, I'm not really that sure that is the right place to fix this. My concern is that I'm not really sure what happens if you download an executable that was reported as text. Locally g_app_info would find something to run that?
Comment 3 Gustavo Noronha (kov) 2010-02-24 13:59:39 UTC
If the server says "hey, this is text/plain", and sends you a binary file, libsoup's content sniffing will mutate its mime type into application/octect-stream. I guess there may be other vectors, though.
Comment 4 Michael Catanzaro 2016-02-27 18:23:01 UTC
The patch is obsolete, but the bug is not.
Comment 5 Michael Catanzaro 2016-02-27 19:02:08 UTC
Created attachment 322547 [details] [review]
download: Browse to file if it cannot be opened

Instead of failing silently in response to the user's attempts to open a
download.
Comment 6 Michael Catanzaro 2016-02-27 19:02:11 UTC
Created attachment 322548 [details] [review]
download: Allow users to open "unsafe" files

We have a map of safe vs. unsafe MIME types, intended to prevent
Epiphany from automatically opening potentially-unsafe downloads without
user interaction. It's not designed to prevent the user from ever
opening these files, but that's what's happening: currently attempting
to open an "unsafe" file always fails silently, confusing users.

We need to pass the MIME type of the file to
ephy_embed_shell_launch_handler. Passing the MIME type means "open it no
matter what" while passing NULL means "open it if it's safe."
Comment 7 Michael Catanzaro 2016-02-27 19:06:52 UTC
(In reply to Diego Escalante Urrelo (not reading bugmail) from comment #2)
> Mmm, I'm not really that sure that is the right place to fix this. My
> concern is that I'm not really sure what happens if you download an
> executable that was reported as text. Locally g_app_info would find
> something to run that?

I think Epiphany would open the file in gedit... if there's a vulnerability in gedit, then you're out of luck, but I think that's reasonable behavior. The point of the safe types list is to avoid opening files in unusually-vulnerable applications, not to protect against everything that's suspicious.
Comment 8 Michael Catanzaro 2016-02-28 05:39:24 UTC
*** Bug 651117 has been marked as a duplicate of this bug. ***
Comment 9 Carlos Garcia Campos 2016-02-28 08:33:48 UTC
Review of attachment 322547 [details] [review]:

::: embed/ephy-download.c
@@ +481,3 @@
                                                destination, NULL, download->start_time);
+        if (!ret)
+          ret = ephy_file_browse_to (destination, download->start_time);

I'm thinking that maybe we should let ephy_embed_shell_launch_handler handle this, because here we don't know if it failed because it didn't find an app, because the app was ephy itself, or because launching the app failed. Do we want to browse the file in all those cases?
Comment 10 Carlos Garcia Campos 2016-02-28 08:55:05 UTC
Review of attachment 322548 [details] [review]:

::: embed/ephy-download.c
@@ +481,3 @@
+                                               destination,
+                                               ephy_download_get_content_type (download),
+                                               download->start_time);

I think this doesn't work. In case of EPHY_DOWNLOAD_ACTION_AUTO, if we find a valid app for the download mime type, ephy_download_do_download_action is called recursively with EPHY_DOWNLOAD_ACTION_OPEN, that would call ephy_embed_shell_launch_handler with a mime type so we would always allow running unsafe mime types. In the current code, we check the download mime type in auto just to make a first filter and browse to directly if don't have any app to handle it, but we have an app, we don't trust the mime type and we sniff the actual content type to decide whether to open it or not. I think ephy_embed_shell_launch_handler is a bit confusing, so I we could make it receive the GAppInfo instead, or add a variant that receives a GAppInfo instead of a GFile. This way we could make EPHY_DOWNLOAD_ACTION_AUTO use ephy_file_launcher_get_app_info_for_file directly and either call phy_embed_shell_launch_handler or ephy_file_browse_to without doing any recursive calls. I would also change ephy_file_launcher_get_app_info_for_file to not trust the given mime type, so that if we don't find an app for the proposed mime type, we return early, but if we find one we check it's safe.
Comment 11 Michael Catanzaro 2016-03-01 00:48:54 UTC
Also we should fix the code duplication between ephy_embed_shell_launch_handler and ephy_file_launch_handler, looks like ephy_embed_shell_launch_handler does the same thing but with an extra desktop file safety check that's definitely wanted in both places. Also that check is broken on Debian....
Comment 12 Michael Catanzaro 2016-03-05 18:10:58 UTC
(In reply to Carlos Garcia Campos from comment #10)
> I would also change
> ephy_file_launcher_get_app_info_for_file to not trust the given mime type,
> so that if we don't find an app for the proposed mime type, we return early,
> but if we find one we check it's safe.

I'm not sure. A malicious server sending a bogus MIME type could only cause the file to open in an app that claims to handle the safe type. I suppose that could be bad if the same app handles both a safe and an unsafe type....

(In reply to Michael Catanzaro from comment #11)
> Also we should fix the code duplication between
> ephy_embed_shell_launch_handler and ephy_file_launch_handler, looks like
> ephy_embed_shell_launch_handler does the same thing but with an extra
> desktop file safety check that's definitely wanted in both places.

Looks like Xan intentionally added it as a separate function, but I don't know why.

> Also that
> check is broken on Debian....

It's not, Debian doesn't rename the desktop file.
Comment 13 Michael Catanzaro 2016-03-05 19:48:19 UTC
Created attachment 323162 [details] [review]
download: Browse to file if it cannot be opened

Instead of failing silently in response to the user's attempts to open a
download.
Comment 14 Michael Catanzaro 2016-03-05 19:48:23 UTC
Created attachment 323163 [details] [review]
Improve some FIXMEs

 * Let's not rename downloaded files
 * g_application_get_application_id is a thing
Comment 15 Michael Catanzaro 2016-03-05 19:48:27 UTC
Created attachment 323164 [details] [review]
download: Get rid of EPHY_DOWNLOAD_ACTION_DO_NOTHING

This differs from EPHY_DOWNLOAD_ACTION_NONE in that it explicitly
informs EphyDownload that it should not perform any download action.
Maybe that was actually important in the past, but nowadays there's
absolutely no difference, so just get rid of it.
Comment 16 Michael Catanzaro 2016-03-05 19:48:31 UTC
Created attachment 323165 [details] [review]
download: Remove EPHY_DOWNLOAD_ACTION_AUTO

Let's always take the same action when auto-opening a file as when
opening manually, instead of allowing ephy_download_do_download_action()
to confusingly invoke itself recursively.

At first, I thought it would be good to treat automatic downloads
differently, and perform MIME type safety checking on them, but not on
downloads opened manually. But I'm not so sure anymore, since it is easy
for a server to give the file a deceptive file extension but report an
accurate MIME type, which could trick the user into opening a harmful
file (e.g. a script) if we were to stop performing MIME type safety
checks on manually-opened files. So let's continue to perform that check
in both cases. But now that EPHY_DOWNLOAD_ACTION_OPEN browses to the
file if opening fails, there's no need to have special behavior for
EPHY_DOWNLOAD_ACTION_AUTO anymore. The only difference that currently
implies is that we check the safety of the MIME type once for the
server's reported MIME type, fall back to browsing to the file if it's
unsafe, then sniff the MIME type ourselves, check safety again, and
decide to browse to. There's no point in checking the safety of the
server's reported type, since we're just going to sniff it ourselves.

So just get rid of it.
Comment 17 Michael Catanzaro 2016-03-05 19:48:36 UTC
Created attachment 323166 [details] [review]
download: Add an assertion
Comment 18 Michael Catanzaro 2016-03-05 19:48:39 UTC
Created attachment 323167 [details] [review]
download: Further simplify ephy_download_do_download_action()

Let's just fall through to EPHY_DOWNLOAD_ACTION_BROWSE_TO instead of
calling ephy_file_browse_to() twice in a row.
Comment 19 Michael Catanzaro 2016-03-05 19:48:44 UTC
Created attachment 323168 [details] [review]
download: Handle focus stealing prevention properly

Currently, we always pass the start time of the download to
ephy_embed_shell_launch_handler for focus stealing preventing. This is
wrong as it causes apps to not be focused when opening files manually.
For example, I became confused today when I tried to open an image file,
and it opened in eog, but eog appeared beneath the Epiphany window and
did not receive focus, so I could not see it.

Instead, use the current time for focus stealing prevention when the
download action is activated from somewhere external to this file (e.g.
the downloads popover), and only use the start time when the download
action is set in advance via ephy_download_set_download_action(), or
when opening the download automatically without user interaction.
Comment 20 Michael Catanzaro 2016-03-12 02:19:17 UTC
Attachment 323162 [details] pushed as fb3e7f4 - download: Browse to file if it cannot be opened
Attachment 323163 [details] pushed as 5ac5606 - Improve some FIXMEs
Comment 21 Michael Catanzaro 2016-05-09 14:22:38 UTC
Attachment 323164 [details] pushed as 5a5584f - download: Get rid of EPHY_DOWNLOAD_ACTION_DO_NOTHING
Attachment 323165 [details] pushed as d0ad997 - download: Remove EPHY_DOWNLOAD_ACTION_AUTO
Attachment 323166 [details] pushed as c33e83c - download: Add an assertion
Attachment 323168 [details] pushed as 0283906 - download: Handle focus stealing prevention properly