Created attachment 346646 [details] [review]
gio-tool: Do not alter uris before use

Uris may be altered by the following code, which breaks xdg-open:
file = g_file_new_for_commandline_arg (arg[i])
uri = g_file_get_uri (file);

Examples of possible uri changes:
mailto:email -> mailto:///email
magnet:?xt=urn:hash -> magnet:///?xt=urn:hash
ssh://user@host -> sftp://user@host

This patch causes that uris aren't preprocessed for locations with
scheme, however absolute and relative paths are still preprocessed.

It may be also fixed another way, please see the original gvfs-open bug report:
https://bugzilla.gnome.org/show_bug.cgi?id=738690#c8

Looks to me like GIO should be fixed to not produce invalid URIs. afaik, “mailto:///email” is not valid (https://tools.ietf.org/html/rfc6068#section-2). It should be fine for g_file_get_uri() to produce a different URI from what was passed in to g_file_new_for_commandline_arg(), since it can canonicalise the URI. But what’s produced must be valid.

We can probably add some exceptions in URI generator e.g. for mailto, however, AFAIK e.g. magnet is not standardized and hard to say, what all other schemes are also affected...

So it would be probably better to not touch unknown schemes at all. See Attachment 302563 [details]. It should solve mailto, magnet... but not sure we don't break anything else with that patch.

However, it doesn't solve the problem with SSH -> SFTP. It is caused because SSH is handled like an alias for SFTP:
https://git.gnome.org/browse/gvfs/tree/daemon/sftp.mount.in#n6

We can probably remove this alias and provide separate mount file with SSH scheme, but AFAIK this is because we want to avoid duplicated mounts like sftp://localhost/ and ssh://localhost/. 

So not sure what is the best solution here. Alex?

Just a note that we should fix this for 3.24, because gvfs-open is redirected to "gio open" since 3.23, so xdg-open can't be used with gvfs-open as a workaround...

Urls get modified because of some round-tripping through GFile that is happening somewhere. That is a known, long-standing issue.

That being said, the xdg-open shell script should just be avoided.

*** Bug 773022 has been marked as a duplicate of this bug. ***

GFile uris are for uris that follow rfc 2396, not generic URIs, which have no particular structure other than at a very high level. There is not way we can parse e.g. mailto uris as GFiles and return them back unmodified, as we expect GFiles to have path-like semantics.

Not touching the uri:s like in the patch seems to be right here, and its something we do in other cases. In fact, its why g_app_info_launch_default_for_uri was added i believe.

I dunno what to do about the sftp alias issue though.

I big "+1" for a fix for this, or more information about exactly where the breakage is so that I may attempt a patch.

My usecase is that it breaks my KeePassX ssh:// URL opening, which I have launching an SSH session in a terminal. Needless to say, with this broken, my daily workflow with many dozens of hosts is impeded.

Here is the console output from KeePassX (yes, Qt still calls xdg-open):

---
This tool has been deprecated, use 'gio open' instead.
See 'gio help open' for more info.

gio: sftp://root@172.17.0.17/: The specified location is not mounted
---

I might be able to create a wrapper for xdg-open further up the path that avoids gio altogether, but that's obviously not a general solution.

As a fix, I'm not sure why GIO or any other component is ever altering a URI. I can imagine that a string should be decomposed and it tries to parse what the protocol is, host, user, whatever, but it should never take it apart and put it back together when I'm trying to launch a mime- or url-handler.

A temporary workaround was, in fact, to remove the ssh alias in sftp.mount, restart the gvfs backend, and alter my SSH launch script to strip the trailing slash.

Clearly, any real solution to this issue should address that a user's x-scheme-handler should override gvfs' opinion of mount points. They are logically different applications.

Created attachment 362472 [details] [review]
Fixes gio open for non-file paths

I've applied this patch which fixes this bug for me.

Ondrej, has there been any progress on this elsewhere in GIO/GVFS, or is the status of things here still correct? It would be good to get this fixed.

Review of attachment 362472 [details] [review]:

This looks very similar to attachment #346646 [details].

Yes, no progress on other places, the issue is still valid and attachment 346646 [details] [review] should fully fix that.

Using the patch from attachment 346646 [details] [review] doesn't work as you get the following error when compiling:

    ../glib/gio/gio-tool-open.c: In function ‘handle_open’:
    ../glib/gio/gio-tool-open.c:102:29: error: ‘file’ undeclared (first use in this function); did you mean ‘nice’?
               print_file_error (file, error->message);

This is due to the `file` variable declaration being moved inside the new if-statement.

Created attachment 363051 [details] [review]
gio-tool: Do not alter uris before use

Uris may be altered by the following code, which breaks xdg-open:
file = g_file_new_for_commandline_arg (arg[i])
uri = g_file_get_uri (file);

Examples of possible uri changes:
mailto:email -> mailto:///email
magnet:?xt=urn:hash -> magnet:///?xt=urn:hash
ssh://user@host -> sftp://user@host

This patch causes that uris aren't preprocessed for locations with
scheme, however absolute and relative paths are still preprocessed.

Fixed Comment 15 and rebased to master.

Review of attachment 363051 [details] [review]:

OK, let’s go with this. Thanks Ondrej.

Attachment 363051 [details] pushed as 796599a - gio-tool: Do not alter uris before use

Pushed to glib-2-54 as well.

*** Bug 790664 has been marked as a duplicate of this bug. ***

*** Bug 793014 has been marked as a duplicate of this bug. ***