Applications: Properly parse desktop exec keys and stop shelling out #207
Conversation
|
I just want to +1 this, as it's been sitting here a while. I have run into cases where the error in parsing means that .desktop files don't work. |
Running the sents branch of anyrun, which fixes the parsing of desktop files. anyrun-org/anyrun#207 Signed-off-by: K. Adam Christensen <pope@shifteleven.com>
|
Sorry for the inactivity, I guess after a year of inactivity I should actually do something about this. The PR needs a rebase, after which I will have a look at it. |
https://specifications.freedesktop.org/desktop-entry-spec/latest/exec-variables.html describes how commands and their arguments are stored in the exec key. As the exec key is a string type as defined in https://specifications.freedesktop.org/desktop-entry-spec/latest/value-types.html escapes code for certain characters have to be respected. After this there are rules which characters necessitate a quoted string and which characters need to be escaped in a quoted string. To use the exec key for executing we then need to unescape these characters and can collect the args in a vector. This enables us to stop shelling out when executing desktop files and instead call `Command` directly with the specified program and arguments.
|
Thank you for looking at this PR. I have just rebased on master and accidentally closed the PR in the process. I previously used a panic when a std::process:exit would suffice and added another commit to fix this. |
|
It seems with the new anyrun-provider ipc approach using |
|
If an exec key can't be parsed, it should probably just emit an error and not do any process spawning. The launcher will close as long as |
|
I have noticed the shell I've now used the shell-words crate to obtain the command and argument vector but I'm also open to:
What is your opinion @Kirottu? |
Kirottu
left a comment
Kirottu
left a comment
There was a problem hiding this comment.
I suspect using shell_words to split out the output should be good enough for most use cases. And not shelling out is probably a good idea anyways.
There are some more improvements that would definitely be good for maintainability, and I also need to test it in practice as well.
| Escape, | ||
| } | ||
|
|
||
| fn unescape_exec(s: &str) -> Result<Vec<String>, ExecKeyError> { |
There was a problem hiding this comment.
This kind of complex behavior could really benefit from some unit tests verifying that it works.
plugins/applications/src/scrubber.rs
Outdated
| let argv_without_fieldcodes = argv | ||
| .to_vec() | ||
| .into_iter() | ||
| .map(|mut c| { |
There was a problem hiding this comment.
If we are handling the exec keys to this extent already, some of the valid codes like %i for the icon name should be handled correctly as well.
| Escape, | ||
| } | ||
|
|
||
| fn substitute_escapes(s: &str) -> Result<String, ExecKeyError> { |
There was a problem hiding this comment.
This would benefit from testing as well.
| @@ -93,23 +112,23 @@ pub fn handler(selection: Match, state: &State) -> HandleResult { | |||
| let sensible_terminals = &[ | |||
| Terminal { | |||
There was a problem hiding this comment.
Do all the terminals actually accept the commands in this way?
3 participants
https://specifications.freedesktop.org/desktop-entry-spec/latest/exec-variables.html describes how commands and their arguments are stored in the exec key. As the exec key is a string type as defined in
https://specifications.freedesktop.org/desktop-entry-spec/latest/value-types.html escapes code for certain characters have to be respected. After this there are rules which characters necessitate a quoted string and which characters need to be escaped in a quoted string. To use the exec key for executing we then need to unescape these characters and can collect the args in a vector. This enables us to stop shelling out when executing desktop files and instead call
Commanddirectly with the specified program and arguments.