-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fuse the two desktop files #95
Comments
I believe in the past the Dæmon engine was able to detect |
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 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 |
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. |
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 :(
|
What if |
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 |
The need is to have a |
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? |
It looks like the issue surfaced with the flatpak attempt, so @necessarily-equal will probably confirm, but I suspect only one Thing is that, the For example the Firefox
If I'm right, it is expected the missing argument is not passed if empty. There is no mechanism to add So, what if the engine can receive an URL without -connect but in this case, the parsing stop?
It is needed to parse the commands before the url because on Debian/Arch packages the With that solution, we would be able to do:
or:
In the non-updater |
What is wrong with my previous proposal?
The arguments added by wrapper scripts are always |
For sure we can just remove the -connect warning… but… wait, why do we would write a mistake (useless I have hard time to see where is the security issue requiring to add |
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.
|
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.
>
|
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 In conclusion, it is incorrect to assume you will have a sane value for |
Ooh, interesting, it means you can trigger the url handler without an actual url? It's very likely that |
Use the updater, instead of Daemon, as the handler for unv:// links on Linux and Windows (protocol handler is not implemented on Mac). Fixes Unvanquished#32, Unvanquished#95.
Use the updater, instead of Daemon, as the handler for unv:// links on Linux and Windows (protocol handler is not implemented on Mac). Fixes Unvanquished#32, Unvanquished#95.
Use the updater, instead of Daemon, as the handler for unv:// links on Linux and Windows (protocol handler is not implemented on Mac). Fixes Unvanquished#32, Unvanquished#95.
Done in #120. |
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:
#95 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
Right. The Flatpak desktop file still matches the intended use case for |
Some context: the flatpack uses a patch to suppress the warning, this commit upstreams the patch. Slipher seems to be ok with this patch: Unvanquished/updater#95 (comment). This patch is (was) [added manually in the flatpack](https://github.com/flathub/net.unvanquished.Unvanquished/blob/0217843/0001-Suppress-a-warning.patch).
Some context: the flatpack uses a patch to suppress the warning, this commit upstreams the patch. Slipher seems to be ok with this patch: Unvanquished/updater#95 (comment). This patch is (was) [added manually in the flatpack](https://github.com/flathub/net.unvanquished.Unvanquished/blob/0217843/0001-Suppress-a-warning.patch).
There are no real reason to have two desktop files instead of one. Maybe we can have dæmon understand
daemon unv://…
asdaemon -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.
The text was updated successfully, but these errors were encountered: