1411579 - [flatpak] "Open with" list is empty and and application chooser is empty

Status: RESOLVED FIXED

(Firefox :: File Handling, enhancement, P5)

Description

[Jan Horak [:jhorak]]

Since the application in flatpak does not have access to the list of applications on the host, there's currently no way how to get list of handler apps for specific mime type.

Comment 1

[Jan Horak [:jhorak]]

Attachment: Bug 1411579 - add system handler when Firefox runs in flatpak;

Firefox in Flatpak sandboxed environment does not get the list
of installed applications on the system because application should
know about the environment as little as possible. Introducing
nsSystemHandlerApp forwards requests for opening downloaded files
to the system by utilizing xdg-open command.

As a sidenote the /tmp directory is only accessible in the sandbox,
so opening files instead of downloading ends by failure.
Setting TMPDIR environment variable to location accessible also
by the system solves that.

Review commit: https://reviewboard.mozilla.org/r/207296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207296/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review213094


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: toolkit/system/gnome/nsGIOService.cpp:44
(Diff revision 1)
> +class nsSystemHandlerApp : public nsIGIOMimeApp
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIGIOMIMEAPP
> +  nsSystemHandlerApp() {};

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

  nsSystemHandlerApp() {};
  ^
                       = default;

::: toolkit/system/gnome/nsGIOService.cpp:46
(Diff revision 1)
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIGIOMIMEAPP
> +  nsSystemHandlerApp() {};
> +private:
> +  ~nsSystemHandlerApp() { };

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~nsSystemHandlerApp() { };
  ^
                        = default;

Comment 3

[Jan Horak [:jhorak]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207296/diff/1-2/

Comment 4

[:Paolo Amadini]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review215158

I'm not familiar with flatpak, so I'd like Karl to validate the approach or point us to the right direction or reviewer. In the meantime, I have a few comments.

::: toolkit/system/gnome/nsGIOService.cpp:21
(Diff revision 2)
> +// The program is running in flatpak when /.flatpak-info file exists
> +static bool GetIsRunningInFlatpak() {
> +  bool isRunningInFlatpak;
> +  nsCOMPtr<nsIFile> flatpakInfoFile;
> +  nsresult rv = NS_NewLocalFile(NS_LITERAL_STRING("/.flatpak-info"), false,
> +                                getter_AddRefs(flatpakInfoFile));
> +
> +  NS_ENSURE_SUCCESS(rv, false);
> +  rv = flatpakInfoFile->Exists(&isRunningInFlatpak);
> +  NS_ENSURE_SUCCESS(rv, false);
> +  return isRunningInFlatpak;
> +}
> +
> +static bool IsRunningInFlatpak() {
> +  static bool sIsRunningInFlatpak = GetIsRunningInFlatpak();
> +  return sIsRunningInFlatpak;
> +}

We should avoid main-thread I/O if possible, especially considering that we have to run this code regardless of whether the application is running in flatpak or not. Is file access the only API to know in which environment the application is running?

Regardless, we should do this only once, and cache the result until the application is shut down.

::: toolkit/system/gnome/nsGIOService.cpp:68
(Diff revision 2)
> +}
> +
> +NS_IMETHODIMP
> +nsSystemHandlerApp::GetCommand(nsACString& aCommand)
> +{
> +  aCommand.SetLength(0);

I wonder if this should be "xdg-open".

::: toolkit/system/gnome/nsGIOService.cpp:83
(Diff revision 2)
> +  nsCString cmdLine("xdg-open ");
> +  cmdLine.Append(aUri);

I assume we can safely append any URL here without escaping special characters and/or adding quotes.

The xdg-open documentation says that only a limited number of schemes are supported, and if we pass an URI it is opened with the default browser, but I assume that the custom flatpak implementation of xdg-open actually does what we expect.

I'd rename the class to nsFlatpakHandlerApp since it apparently relies on these assumptions.

::: toolkit/system/gnome/nsGIOService.cpp:527
(Diff revision 2)
>    dbus_g_connection_unref(dbusGConnection);
>    g_free(uri);
>  
>    if (!rv_dbus_call) {
>      org_freedesktop_FileManager1_exists = false;
> -    return NS_ERROR_NOT_AVAILABLE;
> +      return NS_ERROR_NOT_AVAILABLE;

Leftover whitespace change.

Comment 5

[Jan Horak [:jhorak]]

Paolo: Thanks for looking into it. 
Karl: If you can't do the review because of whatever, could you let stransky do the review?

Comment 6

[Karl Tomlinson (:karlt)]

Some background that'll I'll write down as it took me some time to work out:

The Flatpak sandbox is not intended to support non-trivial apps without
explicit support for the Flatpak sandbox.
Portals are provided and are the intented mechanism for interacting with the
rest of the system.

A few toolkit functions in GTK provide the portal support transparently, but
many do not.  Even some of those that do provide support do not function as
desired and so new functions, such as g_app_info_launch_default_for_uri()
and gtk_show_uri(), were added to function as intended.

http://docs.flatpak.org/en/latest/working-with-the-sandbox.html#portals
https://github.com/flatpak/flatpak/wiki/Sandbox#portals

I'll write down some review comments.
I don't mind who takes review from here as long as my comments are addressed.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review215158

The approach to implement a system/dumb nsIHandlerApp makes sense to me.
It is only vaguely related to GIO, but subclassing nsIGIOMimeApp seems to fit
the need well.

> We should avoid main-thread I/O if possible, especially considering that we have to run this code regardless of whether the application is running in flatpak or not. Is file access the only API to know in which environment the application is running?
> 
> Regardless, we should do this only once, and cache the result until the application is shut down.

From a quick look at https://github.com/flatpak/flatpak/wiki/Sandbox#portals
I didn't see anything better, and so testing for this file seems safest, as
GIO is already depending on this.

The static sIsRunningInFlatpak already ensures this happens only once.

> I assume we can safely append any URL here without escaping special characters and/or adding quotes.
> 
> The xdg-open documentation says that only a limited number of schemes are supported, and if we pass an URI it is opened with the default browser, but I assume that the custom flatpak implementation of xdg-open actually does what we expect.
> 
> I'd rename the class to nsFlatpakHandlerApp since it apparently relies on these assumptions.

We can't safely append a URL without quoting because this is interpreted as a
shell command line.  g_spawn_command_line_async() may avoid the worst security
implications, but I expect this can be done much better.

Yes, flatpak-xdg-utils xdg-open uses the portal for this, which would provide
the expected behavior.  Well, it seems it always opens a dialog to confirm,
but that's what you'd expect in a sandbox.

nsFlatpakHandlerApp probably is a better name.
Even though it is just a system handler, it is special because we can't
query for any details.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review216926

::: toolkit/system/gnome/nsGIOService.cpp:21
(Diff revision 2)
>  #ifdef MOZ_ENABLE_DBUS
>  #include <dbus/dbus-glib.h>
>  #include <dbus/dbus-glib-lowlevel.h>
>  #endif
>  
> +// The program is running in flatpak when /.flatpak-info file exists

Please include a comment to indicate why this file is expected to exist when
running in Flatpak.  Either link to documentation, which I'm not able to find,
or just mention that this is how GIO tests for this situation.  Oddly GTK
tests for a different file, and so a comment is helpful to explain why this
filename is used.

Is anyone likely to run the flatpak without the sandbox, with
--filesystem=host for example?

That may not be the usual situation, but I wonder whether that would allow
Firefox to pick an application to launch and avoid the need for the dialog
every time.  I guess the flatpak-info file could be parsed to detect that, 
but this need not be in initial support, if at all.

::: toolkit/system/gnome/nsGIOService.cpp:25
(Diff revision 2)
>  
> +// The program is running in flatpak when /.flatpak-info file exists
> +static bool GetIsRunningInFlatpak() {
> +  bool isRunningInFlatpak;
> +  nsCOMPtr<nsIFile> flatpakInfoFile;
> +  nsresult rv = NS_NewLocalFile(NS_LITERAL_STRING("/.flatpak-info"), false,

Use NS_NewNativeLocalFile() in UNIX-specific code, such as here.  Paths are
bytes, not UTF-16 on UNIX.  It probably doesn't make any difference when the
characters are ASCII, but code gets copied around without understanding, and
passing the bytes as an NS_LITERAL_CSTRING() saves a conversion.

::: toolkit/system/gnome/nsGIOService.cpp:52
(Diff revision 2)
> +};
> +
> +NS_IMPL_ISUPPORTS(nsSystemHandlerApp, nsIGIOMimeApp)
> +
> +NS_IMETHODIMP
> +nsSystemHandlerApp::GetId(nsACString& aId)

I don't see nsGIOMimeApp::GetId() used anywhere, and so I wonder whether nsIGIOMimeApp.id should just be removed.

::: toolkit/system/gnome/nsGIOService.cpp:73
(Diff revision 2)
> +  aCommand.SetLength(0);
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +nsSystemHandlerApp::GetExpectsURIs(int32_t* aExpects)

Is nsIGIOMimeApp.expectsURIs unused?

::: toolkit/system/gnome/nsGIOService.cpp:83
(Diff revision 2)
> +  nsCString cmdLine("xdg-open ");
> +  cmdLine.Append(aUri);
> +  if (!g_spawn_command_line_async(cmdLine.get(), &error)) {

Would gtk_show_uri() provide the intended behavior here?
That would remove the dependency on flatpak-xdg-utils.
GIO uses org.freedesktop.portal.Documents
but flatpak-xdg-utils uses
OpenFile() on org.freedesktop.portal.OpenURI.  I don't know what the
difference in behavior would be.

I assume the window is not available for gtk_show_uri_on_window().

::: toolkit/system/gnome/nsGIOService.cpp:374
(Diff revision 2)
> +  // list of installed applications on the system. We use
> +  // generic nsSystemHandlerApp which forwards launch call to the
> +  // system
> +  if (IsRunningInFlatpak()) {
> +    nsSystemHandlerApp *mozApp = new nsSystemHandlerApp();
> +    NS_ENSURE_TRUE(mozApp, NS_ERROR_OUT_OF_MEMORY);

new won't return null, so no need for the check.

::: toolkit/system/gnome/nsGIOService.cpp:399
(Diff revision 2)
> +    nsSystemHandlerApp *mozApp = new nsSystemHandlerApp();
> +    NS_ENSURE_TRUE(mozApp, NS_ERROR_OUT_OF_MEMORY);

new won't return null, so no need for the check.

Comment 9

[Jan Horak [:jhorak]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207296/diff/2-3/

Comment 10

[Jan Horak [:jhorak]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review217088

::: toolkit/system/gnome/nsGIOService.cpp:21
(Diff revision 2)
>  #ifdef MOZ_ENABLE_DBUS
>  #include <dbus/dbus-glib.h>
>  #include <dbus/dbus-glib-lowlevel.h>
>  #endif
>  
> +// The program is running in flatpak when /.flatpak-info file exists

Running with --filesystem=host does not allow the 'sandboxed' application to get appinfos of the host system, because these are not exported to runtime (/usr/share/applications/ contains only runtime .desktop files).

The need for a dialog actually depends on the system setup. If only one handler application is installed, the portal should use it without showing the list. The portal could later add some 'Use as default' setting like Android does, but that's out of our reach. Android approach by delegating which application to use to the OS, makes sense to me.

::: toolkit/system/gnome/nsGIOService.cpp:25
(Diff revision 2)
>  
> +// The program is running in flatpak when /.flatpak-info file exists
> +static bool GetIsRunningInFlatpak() {
> +  bool isRunningInFlatpak;
> +  nsCOMPtr<nsIFile> flatpakInfoFile;
> +  nsresult rv = NS_NewLocalFile(NS_LITERAL_STRING("/.flatpak-info"), false,

I've found out how gtk determines the need for portal. The gtk_should_use_portal is not public, but still better than my construction (which was suggested on irc only):
https://github.com/GNOME/gtk/blob/e0ce028c88858b96aeda9e41734a39a3a04f705d/gtk/gtkprivate.c#L272
I would prefer to use this, is that okay to you?

::: toolkit/system/gnome/nsGIOService.cpp:52
(Diff revision 2)
> +};
> +
> +NS_IMPL_ISUPPORTS(nsSystemHandlerApp, nsIGIOMimeApp)
> +
> +NS_IMETHODIMP
> +nsSystemHandlerApp::GetId(nsACString& aId)

Good point. Can we do that in different bug together with .expectsURIs?

::: toolkit/system/gnome/nsGIOService.cpp:83
(Diff revision 2)
> +  nsCString cmdLine("xdg-open ");
> +  cmdLine.Append(aUri);
> +  if (!g_spawn_command_line_async(cmdLine.get(), &error)) {

I'll use gtk_show_uri to avoid calling shell command, that's a good idea.

Comment 11

[Jan Horak [:jhorak]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207296/diff/3-4/

Comment 12

[Code Review Bot [:reviewbot]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review217548


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/system/gnome/nsGIOService.cpp:51
(Diff revision 3)
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIHANDLERAPP
> +  NS_DECL_NSIGIOMIMEAPP
> +  nsFlatpakHandlerApp() {};

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

  nsFlatpakHandlerApp() {};
  ^
                        = default;

::: toolkit/system/gnome/nsGIOService.cpp:53
(Diff revision 3)
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIHANDLERAPP
> +  NS_DECL_NSIGIOMIMEAPP
> +  nsFlatpakHandlerApp() {};
> +private:
> +  ~nsFlatpakHandlerApp() { };

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~nsFlatpakHandlerApp() { };
  ^
                         = default;

Comment 13

[Code Review Bot [:reviewbot]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review217550


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/system/gnome/nsGIOService.cpp:51
(Diff revision 4)
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIHANDLERAPP
> +  NS_DECL_NSIGIOMIMEAPP
> +  nsFlatpakHandlerApp() {};

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

  nsFlatpakHandlerApp() {};
  ^
                        = default;

::: toolkit/system/gnome/nsGIOService.cpp:53
(Diff revision 4)
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIHANDLERAPP
> +  NS_DECL_NSIGIOMIMEAPP
> +  nsFlatpakHandlerApp() {};
> +private:
> +  ~nsFlatpakHandlerApp() { };

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~nsFlatpakHandlerApp() { };
  ^
                         = default;

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review217616

When rebasing and making changes to a patch, please push each stage to
reviewboard separately so that the changes to the patch are clearly visible.

::: toolkit/system/gnome/nsGIOService.cpp:30
(Diff revisions 2 - 4)
> -// The program is running in flatpak when /.flatpak-info file exists
> -static bool GetIsRunningInFlatpak() {
> -  bool isRunningInFlatpak;
> -  nsCOMPtr<nsIFile> flatpakInfoFile;
> -  nsresult rv = NS_NewLocalFile(NS_LITERAL_STRING("/.flatpak-info"), false,
> -                                getter_AddRefs(flatpakInfoFile));
> +// We use the same code as gtk_should_use_portal() to detect if we're in flatpak env
> +// https://github.com/GNOME/gtk/blob/e0ce028c88858b96aeda9e41734a39a3a04f705d/gtk/gtkprivate.c#L272
> +static bool GetShouldUsePortal() {
> +  bool shouldUsePortal;
> +  char *path;
> +  path = g_build_filename (g_get_user_runtime_dir (), "flatpak-info", NULL);

NULL is 0 in C++, so use nullptr for a void* sentinel.  No space after g_build_filename().

::: toolkit/system/gnome/nsGIOService.cpp:51
(Diff revisions 2 - 4)
>  {
>  public:
>    NS_DECL_ISUPPORTS
> +  NS_DECL_NSIHANDLERAPP
>    NS_DECL_NSIGIOMIMEAPP
> -  nsSystemHandlerApp() = default;
> +  nsFlatpakHandlerApp() {};

I wonder whether you picked up an old version of the patch.  Use = default to silence the static review tool.

::: toolkit/system/gnome/nsGIOService.cpp:53
(Diff revisions 2 - 4)
>    NS_DECL_ISUPPORTS
> +  NS_DECL_NSIHANDLERAPP
>    NS_DECL_NSIGIOMIMEAPP
> -  nsSystemHandlerApp() = default;
> +  nsFlatpakHandlerApp() {};
>  private:
> -  virtual ~nsSystemHandlerApp() = default;
> +  ~nsFlatpakHandlerApp() { };

= default.

::: toolkit/system/gnome/nsGIOService.cpp:45
(Diff revision 4)
> +static bool ShouldUsePortal() {
> +  static bool sShouldUsePortal = GetShouldUsePortal();
> +  return sShouldUsePortal;
> +}
> +
> +class nsFlatpakHandlerApp : public nsIGIOMimeApp

I'm no longer sure that implementing nsIGIOMimeApp is helpful.
launch(uri) is the only method supported but I don't know why
nsMIMEInfoUnix::LaunchDefaultWithFile(nsIFile *aFile) doesn't use
nsIHandlerApp::LaunchWithURI().

I wonder whether making the few callers that need nsIGIOMimeApp QueryInterface
would be simpler than stubbing almost all of nsIGIOMimeApp with
not_implemented.  What do you think?

::: toolkit/system/gnome/nsGIOService.cpp:96
(Diff revision 4)
> +  *aExpects = true;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsFlatpakHandlerApp::Launch(const nsACString& aUri)

What is the appropriate way to launch other apps from the sandbox when there is no URI?

Should nsGNOMEShellService::OpenApplication() just be unsupported?

::: toolkit/system/gnome/nsGIOService.cpp:99
(Diff revision 4)
> +
> +NS_IMETHODIMP
> +nsFlatpakHandlerApp::Launch(const nsACString& aUri)
> +{
> +  GError *error = nullptr;
> +  gtk_show_uri(nullptr, aUri.BeginReading(), GDK_CURRENT_TIME, &error);

BeginReading() is for use with EndReading().  It and nsACString do not necessarily provide nul-terminated strings.
Use get() for a nul-terminated string, which requires PromiseFlatCString.

::: toolkit/system/gnome/nsGIOService.cpp:142
(Diff revision 4)
> +nsFlatpakHandlerApp::Equals(nsIHandlerApp* aHandlerApp, bool* _retval)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

What uses this?

If the portal is required, then are all handlers equal?

::: toolkit/system/gnome/nsGIOService.cpp:147
(Diff revision 4)
> +nsFlatpakHandlerApp::LaunchWithURI(nsIURI* aUri, nsIInterfaceRequestor* aRequestor)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

Shouldn't this method be implemented?

::: toolkit/system/gnome/nsGIOService.cpp:526
(Diff revision 4)
>  
>  NS_IMETHODIMP
>  nsGIOService::GetAppsForURIScheme(const nsACString& aURIScheme,
>                                    nsIMutableArray** aResult)
>  {
>    nsCOMPtr<nsIMutableArray> handlersArray =

Can you add a comment please explaining why this need not return any nsFlatpakHandlerApps when using the portal?

::: widget/gtk/mozgtk/mozgtk.c:610
(Diff revision 4)
>  STUB(gtk_color_chooser_dialog_get_type)
>  STUB(gtk_color_chooser_get_type)
>  STUB(gtk_color_chooser_set_rgba)
>  STUB(gtk_color_chooser_get_rgba)
>  STUB(gtk_color_chooser_set_use_alpha)
> +STUB(gtk_show_uri)

This symbol exists in GTK2, and so the stub is in the wrong list.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review217088

> Running with --filesystem=host does not allow the 'sandboxed' application to get appinfos of the host system, because these are not exported to runtime (/usr/share/applications/ contains only runtime .desktop files).
> 
> The need for a dialog actually depends on the system setup. If only one handler application is installed, the portal should use it without showing the list. The portal could later add some 'Use as default' setting like Android does, but that's out of our reach. Android approach by delegating which application to use to the OS, makes sense to me.

How does --filesystem=host provide "Access all files" in a way that doesn't make the appinfos available?

Having handlers launch without user permission is OK if no handlers have permissions.  If any handlers have permissions to modify the system, then this subverts the sandbox.  Consider a shell file or mono executable handler, but even a jpeg handler with system permissions would be subverting the sandbox if it is not designed to be exposed to untrusted input.

> I've found out how gtk determines the need for portal. The gtk_should_use_portal is not public, but still better than my construction (which was suggested on irc only):
> https://github.com/GNOME/gtk/blob/e0ce028c88858b96aeda9e41734a39a3a04f705d/gtk/gtkprivate.c#L272
> I would prefer to use this, is that okay to you?

Yes, I'm happier with that thanks.  (I don't know why I thought GIO used /.flatpak-info, as it looks similar to GTK now.)

> Good point. Can we do that in different bug together with .expectsURIs?

I guess that's OK.  Adding the stubs just to be removed later is a bit of noise.

Comment 16

[Jan Horak [:jhorak]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207296/diff/4-5/

Comment 17

[Jan Horak [:jhorak]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review217784

::: toolkit/system/gnome/nsGIOService.cpp:45
(Diff revision 4)
> +static bool ShouldUsePortal() {
> +  static bool sShouldUsePortal = GetShouldUsePortal();
> +  return sShouldUsePortal;
> +}
> +
> +class nsFlatpakHandlerApp : public nsIGIOMimeApp

Okay, I've made a broader change hoping I didn't break more stuff than actually fixed.

::: toolkit/system/gnome/nsGIOService.cpp:96
(Diff revision 4)
> +  *aExpects = true;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsFlatpakHandlerApp::Launch(const nsACString& aUri)

The gtk_show_uri only accept uris, the check if the file is local is done there: https://github.com/GNOME/glib/blob/84350cb5666feb1cd459562e438d50cca08c6405/gio/gopenuriportal.c#L84
(the g_openuri_portal_open_uri is called indirectly by gtk_show_uri).


I'm not sure if openApplication() is executed in Linux at all. At least I'm unable to find it. Maybe there: https://searchfox.org/mozilla-central/source/browser/components/shell/content/setDesktopBackground.js#200
but APPLICATION_DESKTOP is not implemented by NSGNOMEShellService anyway.

::: toolkit/system/gnome/nsGIOService.cpp:142
(Diff revision 4)
> +nsFlatpakHandlerApp::Equals(nsIHandlerApp* aHandlerApp, bool* _retval)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

Equals is used to get rid of duplicates in the list of application handlers for specific url scheme, like magnet://. This happens when you pick some gio handler app and then this app is added to handlers.json file to the profile. Then the gio app handler is added only if it is not already loaded from handlers.json.

The introduced nsFlatpakHandlerApp is considered as default handler because system does not provide any other. Default handlers are not stored to handlers.json. I can put there correct implementation for the sake of completeness.

Comment 18

[Jan Horak [:jhorak]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207296/diff/5-6/

Comment 19

[Jan Horak [:jhorak]]

(In reply to Karl Tomlinson (back Jan 30 :karlt) from comment #15)
> > Running with --filesystem=host does not allow the 'sandboxed' application to get appinfos of the host system, because these are not exported to runtime (/usr/share/applications/ contains only runtime .desktop files).
> > 
> > The need for a dialog actually depends on the system setup. If only one handler application is installed, the portal should use it without showing the list. The portal could later add some 'Use as default' setting like Android does, but that's out of our reach. Android approach by delegating which application to use to the OS, makes sense to me.
> 
> How does --filesystem=host provide "Access all files" in a way that doesn't
> make the appinfos available?
The --filesystem=host mounts /usr to the /run/host/usr. The /usr contains libraries and other data relevant to the runtime, so they cannot be replaced by host /usr. The .desktop files from /run/host/usr/share/applications are not loaded by g_app_info_get_default_for_type or similar functions because they would not start in the sandbox anyway. The only way to open application is to use portal by calling gtk_show_uri or executing xdg-open.

> Having handlers launch without user permission is OK if no handlers have
> permissions.  If any handlers have permissions to modify the system, then
> this subverts the sandbox.  Consider a shell file or mono executable
> handler, but even a jpeg handler with system permissions would be subverting
> the sandbox if it is not designed to be exposed to untrusted input.
Hm, good point. I was not able to open file without portal application chooser dialog even when there is only one application in the list. So that information was incorrect. User is always asked for the application to choose. Sorry for the confusion. I hit different issue though: https://bugzilla.gnome.org/show_bug.cgi?id=792611 - the xdg-open differs from gtk_show_uri, but I'd like to let gtk upstream to resolve that. The gtk_show_uri use the application handler from the runtime if there's is some without asking portal.

Comment 20

[Jan Horak [:jhorak]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review217616

> I wonder whether you picked up an old version of the patch.  Use = default to silence the static review tool.

Yes, I've messed the rebasing.

> Can you add a comment please explaining why this need not return any nsFlatpakHandlerApps when using the portal?

Sorry, I've missed that, will push it in a sec.

Comment 21

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

https://reviewboard.mozilla.org/r/207296/#review219624

::: commit-message-ba283:15
(Diff revision 6)
> +because it can be fully replaced with LaunchWithUri from nsIHandlerApp
> +interface.
> +
> +As a sidenote the /tmp directory is only accessible in the sandbox,
> +so opening files instead of downloading ends by failure.
> +Setting TMPDIR environment variable to location accessible also

Please be more specific how to set the TMPDIR under sandbox.

::: toolkit/system/gnome/nsGIOService.cpp:30
(Diff revision 6)
> +// We use the same code as gtk_should_use_portal() to detect if we're in flatpak env
> +// https://github.com/GNOME/gtk/blob/e0ce028c88858b96aeda9e41734a39a3a04f705d/gtk/gtkprivate.c#L272
> +static bool GetShouldUsePortal() {
> +  bool shouldUsePortal;
> +  char *path;
> +  path = g_build_filename (g_get_user_runtime_dir (), "flatpak-info", nullptr);

space between function name and brace

::: toolkit/system/gnome/nsGIOService.cpp:31
(Diff revision 6)
> +// https://github.com/GNOME/gtk/blob/e0ce028c88858b96aeda9e41734a39a3a04f705d/gtk/gtkprivate.c#L272
> +static bool GetShouldUsePortal() {
> +  bool shouldUsePortal;
> +  char *path;
> +  path = g_build_filename (g_get_user_runtime_dir (), "flatpak-info", nullptr);
> +  if (g_file_test (path, G_FILE_TEST_EXISTS)) {

nit: space between function name and brace

::: toolkit/system/gnome/nsGIOService.cpp:34
(Diff revision 6)
> +  char *path;
> +  path = g_build_filename (g_get_user_runtime_dir (), "flatpak-info", nullptr);
> +  if (g_file_test (path, G_FILE_TEST_EXISTS)) {
> +    shouldUsePortal = true;
> +  } else {
> +    shouldUsePortal = g_getenv("GTK_USE_PORTAL") != nullptr;

please use braces () for bool expression for better readibility.

::: toolkit/system/gnome/nsGIOService.cpp:36
(Diff revision 6)
> +  if (g_file_test (path, G_FILE_TEST_EXISTS)) {
> +    shouldUsePortal = true;
> +  } else {
> +    shouldUsePortal = g_getenv("GTK_USE_PORTAL") != nullptr;
> +  }
> +  g_free (path);

space between function name and brace

::: toolkit/system/gnome/nsGIOService.cpp:40
(Diff revision 6)
> +  }
> +  g_free (path);
> +  return shouldUsePortal;
> +}
> +
> +static bool ShouldUsePortal() {

Please rename "ShouldUsePortal" to "ShouldUseFlatpakPortal" to explictly state it's flatpak related.

Also for other variables with "Portal" only substring.

Comment 22

[Jan Horak [:jhorak]]

Comment on attachment 8936555 [details]
Bug 1411579 - add system handler when Firefox runs in flatpak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207296/diff/6-7/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/317b11e45292
add system handler when Firefox runs in flatpak; r=stransky

Comment 24

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/317b11e45292