Fuse the two desktop files

[necessarily-equal]

There are no real reason to have two desktop files instead of one. Maybe we can have dæmon understand daemon unv://… as daemon -connect unv://…

(This is a bit of an issue that should be across the repos, because the desktop files are in the updater repo, belong here and this needs a dæmon change.)

I'll do a PR in the following month for that unless someone think it's a bad idea.

[illwieckz]

I believe in the past the Dæmon engine was able to detect unv:// prefix without -connect.

[slipher]

-connect is a security feature; there is no guarantee about what kind of "URLs" might be sent by different programs, and we don't want them to be able to put arbitrary command lines. -connect guarantees that the next argument will be treated as an address (and not, say a plus command).

But that's all a moot point if we route URLs through the updater first, as you seem to suggest. In that case we can construct whatever kind of daemon command we want. And sanitize the URL if we wish. I don't understand how exactly is it supposed to work though? What happens when %u appears in a non-protocol handler line?

There is an issue with the current desktop files - custom command lines are not respected when using the URL handler. There's a commandline option (default %command%), that you could replace with optirun %command% for example. But since the protocol handler desktop file bypasses the updater, it doesn't get used. So running URLs through the updater would be a good idea in any case. It just takes a bit of work - add protocol handling on the updater command line, skip the splash screen, and probably fiddle with parsing for the custom command stuff.

[illwieckz]

If we want to make the updater the universal launcher even for distro packages like Debian or FlatPak that already handles update, then we need a mechanism to disable update download in updater for those packages.

At this point a non-updater .desktop file is needed for things like flatpak.

[necessarily-equal]

I was thinking of keeping the updater-specific desktop files for now, since one command goes through the updater and the other not. I don't think in the current state it makes any sense to have the updater with linux packages, be it distro ones, or flatpaks. And that's probably why the issue was on the other repo originally.

About the security issue, I expect MimeType=x-scheme-handler/unv to pass only unv:// urls, though I haven't checked. At least xdg-open unv://123.45.67.89 seems to keep the unv:// prefix, and is thus fine:

📦$ cat 
~/.var/app/net.unvanquished.Unvanquished/data/unvanquished/daemon.log
…
cmdline: -pakpath /app/pkg -connect unv://123.45.67.89
…

GitHub killed my markdown :(

[illwieckz]

-connect is a security feature; there is no guarantee about what kind of "URLs" might be sent by different programs.

What if unv:// is detected without -connect, we connect to that url, but we stop processing options?

[slipher]

The original post mentioned the updater desktop files, and did not mention distros or flatpaks, so I was not able to understand that it is about distro packaging for a non-updater-using Unvanquished install.

If the goal is to have a command line that can optionally have a URL argument at the end, or otherwise no argument (or an empty string? or the string %u? Still waiting for explanation of this part) then we can use -connect and simply remove the warning that Daemon emits when there is no argument.

[illwieckz]

The need is to have a .desktop file with a command running daemon when user clicks the icon, and running daemon -connect unv://something when user clicks unv://something url.

[slipher]

But is there some reason it needs to be a single file? If so, how does parameter substitution work - what is the resulting command line in each case?

[illwieckz]

It looks like the issue surfaced with the flatpak attempt, so @necessarily-equal will probably confirm, but I suspect only one .desktop file is expected to be seen from outside the flatpak?

Thing is that, the .desktop file is expect to provide one Exec= line that is used both when launching the application the usual way (clicking the icon) or whith an URL handler.

For example the Firefox Exec= line is:

Exec=firefox %u

If I'm right, it is expected the missing argument is not passed if empty. There is no mechanism to add -connect prefix to the url.

So, what if the engine can receive an URL without -connect but in this case, the parsing stop?

• daemon -libpath /var/games/unvanquished -connect unvanquished.net +delay 100f say hello there
  run the game with custom libpath, connect to server, execute a command
• daemon -libpath /var/games/unvanquished -connect unv://unvanquished.net +delay 100f say hello there
  run the game with custom libpath, connect to server, execute a command
• daemon -libpath /var/games/unvanquished unv://unvanquished.net
  run the game with custom libpath, connect to server
• daemon -libpath /var/games/unvanquished unv://unvanquished.net +delay 100f say hello there
  run the game with custom libpath, connect to server, do not execute a command

It is needed to parse the commands before the url because on Debian/Arch packages the .desktop file does not set the daemon engine directly in the Exec= line but a script that can use various options like -libpath.

With that solution, we would be able to do:

Exec=daemon

or:

Exec=unvanquished

In the non-updater .desktop file (Flatpak, Debian, Arch…).

[slipher]

What is wrong with my previous proposal?

we can use -connect and simply remove the warning that Daemon emits when there is no argument.

The arguments added by wrapper scripts are always -flags rather than +commands, so there is no problem with having them before -connect. Plus commands should only appear if someone is writing their own command line, in which case wouldn't use the desktop file. If you are clicking a desktop icon then I do not expect manually adding arguments to be possible.

[illwieckz]

For sure we can just remove the -connect warning… but… wait, why do we would write a mistake (useless -connect in a desktop file) to begin with? Why do other programs just do progname %u and why we would not be able to?

I have hard time to see where is the security issue requiring to add -connect to defeat it, on which system?

[necessarily-equal]

If I'm right, it is expected the missing argument is not passed if empty. There is no mechanism to add -connect prefix to the url.

I agree on that. We can start with the definition, %u is:

A single URL. Local files may either be passed as
file: URLs or as file path.

And %u should not be quoted. From that definition %u can ever be an url with a protocol handler (https://, file:// or unv://) or an absolute path (/home/antoine/.bashrc). I think it's safe to say it will never start with a + or a -. It will also never have several URL arguments, as we would use %U if we wanted to support that.

I think that using daemon %u which with the current desktop file can only result in daemon or daemon unv://xyz and would be perfectly safe on linux. We also don't need to stop parsing arguments IMO: it would be possible to cope with stopped argument parsing, but it's a needless surprise.

We can keep the -connect flag if others platforms need it. Also note that the current implementation of -connect stop parsing the command line arguments.

On Tue, Jun 8 2021 at 21:41:36 -0700, Thomas Debesse @.***> wrote:

for sure we can just remove the -connect warning… but… wait, why do we would write a mistake -useless -connect in a desktop file to begin with? Why do other programs just do progname %u and why we would not be able to?

I have hard time to see where is the security issue requiring to add -connect to defeat it, on which system?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[necessarily-equal]

As a rule of thumb, all of these work (nautilus is the GNOME file manager, and evince the pdf viewer):

If that's any indications, other programs don't seem to stop parsing after an URL.

$ firefox https://unvanquished.net -help Usage: firefox [ options ... ] [URL] where options include: …

$ nautilus file:///home/afontain --new-window

$ evince file:///home/afontain/MSE2_all.pdf --find="some string"

On Wed, Jun 9 2021 at 11:50:54 +0200, Antoine Fontaine @.***> wrote:

If I'm right, it is expected the missing argument is not passed if empty. There is no mechanism to add -connect prefix to the url.

I agree on that. We can start with the definition, %u is:

A single URL. Local files may either be passed as file: URLs or as file path.

And %u should not be quoted. From that definition %u can ever be an url with a protocol handler (https://, file:// or unv://) or an absolute path (/home/antoine/.bashrc). I think it's safe to say it will never start with a + or a -. It will also never have several URL arguments, as we would use %U if we wanted to support that.

I think that using daemon %u which with the current desktop file can only result in daemon or daemon unv://xyz and would be perfectly safe on linux. We also don't need to stop parsing arguments IMO: it would be possible to cope with stopped argument parsing, but it's a needless surprise.

We can keep the -connect flag if others platforms need it. Also note that the current implementation of -connect stop parsing the command line arguments.

On Tue, Jun 8 2021 at 21:41:36 -0700, Thomas Debesse @.***> wrote:

for sure we can just remove the -connect warning… but… wait, why do we would write a mistake -useless -connect in a desktop file to begin with? Why do other programs just do progname %u and why we would not be able to?

I have hard time to see where is the security issue requiring to add -connect to defeat it, on which system?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[slipher]

So with Firefox/Debian/GNOME I just tried having an HTTP server set the Content-Type header of its HTTP response to x-scheme-handler/whatever, in an attempt to exploit the fact that URI protocol registration piggybacks on the MIME type database. This works. Not only does the resulting %u argument not have unv:// in it, it doesn't even resemble a URL. The argument is like /tmp/mozilla_slipher0/osds4Jxu.

In conclusion, it is incorrect to assume you will have a sane value for %u.

[illwieckz]

Ooh, interesting, it means you can trigger the url handler without an actual url?

It's very likely that /tmp/mozilla_slipher0/osds4Jxu is a temporary file containing the body of the HTTP response.

[slipher]

Done in #120.

[necessarily-equal]

For the record, we can't really migrate to this for the flatpack as we don't bundle the updater there.

For the flatpack, we use a single file that always include -connect https://github.com/flathub/net.unvanquished.Unvanquished/blob/master/net.unvanquished.Unvanquished.desktop, then silence the warning if no argument is passed https://github.com/flathub/net.unvanquished.Unvanquished/blob/master/0001-Suppress-a-warning.patch.

In the flatpack's case, I'm not sure any action is needed to be honest, as long as the patch still applies. Since the flatpak use case is simpler, it also would seem like we don't need to move to -connect-trusted; is that right @slipher?

(CC @illwieckz I assume)

On 11 July 2024 09:02:33 CEST, slipher @.***> wrote:

Closed #95 as completed.

-- Reply to this email directly or view it on GitHub: https://github.com/Unvanquished/updater/issues/95#event-13467038387 You are receiving this because you were mentioned.

Message ID: @.***>

[slipher]

Right. The Flatpak desktop file still matches the intended use case for -connect so no action is needed. I would approve this patch in upstream BTW.