#60 Using $(…) in desktop files fails for items other than Exec

Closed
opened 2 weeks ago by SlavekB · 22 comments
SlavekB commented 2 weeks ago

Basic information

  • TDE version: R14.0.7
  • Distribution: all
  • Hardware: all

Description

Commit 1074eb0336 caused that expansion does not work in some desktop files, where it is used for example on URL item.

Steps to reproduce

  1. Click to My Documents on the desktop.
  2. Observe the error message that ( kxdglauncher --getpath --xdgname DOCUMENTS ) is an incorrect url format.

Additional information

For parsing URLs in desktop files, the readPathEntry method is used, which automatically allows dollar expansion – without the need to use the [$e] modifier. This method is also used for many other item types: Example, ScreenShot, Path, URL, exec, TryExec, X-DocPath, i18ndir, soundfile, logfile,…

So the question is whether to look for a solution to allow external calls to be made specifically for URLs, or whether to keep the restriction as it is now and modify the desktop files that are affected – to solve the situation by doubling the $ in those desktop files.

## Basic information - TDE version: R14.0.7 - Distribution: all - Hardware: all ## Description Commit 1074eb0336 caused that expansion does not work in some desktop files, where it is used for example on URL item. ## Steps to reproduce 1. Click to My Documents on the desktop. 2. Observe the error message that `( kxdglauncher --getpath --xdgname DOCUMENTS )` is an incorrect url format. ## Additional information For parsing URLs in desktop files, the `readPathEntry` method is used, which automatically allows dollar expansion – without the need to use the `[$e]` modifier. This method is also used for many other item types: Example, ScreenShot, Path, URL, exec, TryExec, X-DocPath, i18ndir, soundfile, logfile,… So the question is whether to look for a solution to allow external calls to be made specifically for URLs, or whether to keep the restriction as it is now and modify the desktop files that are affected – to solve the situation by doubling the `$` in those desktop files.
SlavekB added this to the R14.0.8 release milestone 2 weeks ago
SlavekB added the
SL/regression
label 2 weeks ago
SlavekB added the
PR/rfc
label 2 weeks ago
MicheleC commented 2 weeks ago
Owner

I think we should not eanble external calls again, we would go back to the original problem. Instead we will need to fix .desktop and .directory files where needed

I think we should not eanble external calls again, we would go back to the original problem. Instead we will need to fix .desktop and .directory files where needed

Does doubling the $$ solve the issue on linux? For me it only worked on FreeBSD. What about adding the XDG_…-variables to the environment and use these instead of $( kxdglauncher ..) ?

Does doubling the $$ solve the issue on linux? For me it only worked on FreeBSD. What about adding the XDG_...-variables to the environment and use these instead of $( kxdglauncher ..) ?
MicheleC commented 2 weeks ago
Owner

@Dr_Nikolaus_Klepp we are seeing strange results. Using $$ seems to have no effect whatsoever on my system, while Slavek and Chris seems to see a difference. In any case we will need to analyse the problem in more details and find the correct way to fix it.

@Dr_Nikolaus_Klepp we are seeing strange results. Using $$ seems to have no effect whatsoever on my system, while Slavek and Chris seems to see a difference. In any case we will need to analyse the problem in more details and find the correct way to fix it.
SlavekB commented 2 weeks ago
Owner

For me it works on the My Documents icon on the desktop, but it does not work on the system:/documents.

Using variables instead of executing the command would be nice, but XDG_*_DIR values are not set in environment variables.

For me it works on the My Documents icon on the desktop, but it does not work on the system:/documents. Using variables instead of executing the command would be nice, but `XDG_*_DIR` values are not set in environment variables.

Well, if the startupscript would contain something like this, it would work (needs some refinements, but ..):

export XDG_DOCUMENTS_DIR=$( kxdglauncher --getpath --xdgname DOCUMENTS )

BTW: What is the reason for “kxdglauncher” ? Wouldn’t “xdg-user-dir” do the same job, e.g.:

xdg-user-dir DOCUMENTS

Well, if the startupscript would contain something like this, it would work (needs some refinements, but ..): export XDG_DOCUMENTS_DIR=$( kxdglauncher --getpath --xdgname DOCUMENTS ) BTW: What is the reason for "kxdglauncher" ? Wouldn't "xdg-user-dir" do the same job, e.g.: xdg-user-dir DOCUMENTS
Chris commented 2 weeks ago
Collaborator

Sadly this would have discovered much more early, because I noticed it instantly after my rebuild of tdelibs after the release on my working machine, which hadn’t updated the working tdelibs many months. That is very sad.

I made some personal tests and thought about the topic and re-read that linked file in the original reported issue and want to share my observations and at least my point of view about that topic.

It seems this problems has much more impacts as we could think about. For example Amarok isn’t playing any streams with that commit. It’s just one problem introduced by it. And I think there are a lot more hidden problems now introduced by this commit. I think - But I could(!) be really wrong about that, that the problem was just misinterpreted and the right thing to do was not to completely remove any support of external calls from desktop files $(xxx). Instead of this the problem is just the unintended - So without any user action - triggered external call of from desktop files (so [$e]). Because that is also the example shown and spoken about here:

64aa3d3027/kde-kdesktopfile-command-injection.txt

Because otherwise that would not make any sense. Because if some desktop file is run by some intended action of some user, it is the responsibility of every user. Exactly it is like running any unknown binary files or shell scrips from any remote places, or something like that. Therefore, any further secure than removing any possibility to trigger desktop actions without any user action would be the wrong way and would probliable do more harm than good. I think, we don’t know exactly in which situations this would be just really, really useful or is really needed. (Talking just about $(xxx) here, not the [$e]. Also, just changing out a dynamic URL (which is useful) for some exec isn’t really the solution, I think. Because the URL is a right entry for a URL (which is simply needed for good reasons to be dynamic). That shouldn’t be some exec just.

From my personal point of view, the problem here is not the call of $(xxx), for something like the URL entry. It is only a problem for something like Name, Comment and Icon and only in combination with Icon[$e]=, exactly the way it is pointed out from the file above. Because only the [$e] is the functionality of “dynamic configuration entries” as original called that feature. I tested some combinations myself with the example of the file and the commit we talk about removed. So ready to be infected.

From what I got from the document linked, that is only a problem for something like an icon, where the result of that command will end up in the config file (so []), which will be triggered automatically by some external call let’s say from Konqueror, because it will start to parse that file for (only specific) entries to know which icon should be displayed, losely said. But for URL it should be not. So you just download some desktop file from the web, drop it in some folder (you think it would not be run, if you don’t click it as some binary file), but if you just do open the folder, it will be executed. But with only URL=$(echo${IFS}0>~/Desktop/zero.lol&) that should’t happen.

So “Icon[$e]=$(echo${IFS}0>~/Desktop/zero.lol&)” will create that file on your desktop, just if you start Konqueror and you open the folder in which the file is. Or the .directory file. But “Icon=$(echo${IFS}0>~/Desktop/zero.lol&)” don’t will do anything. Same for Name and Comment. Name[$e]=$(echo${IFS}0>~/Desktop/zero.lol&) and/or Comment[$e]=$(echo${IFS}0>~/Desktop/zero.lol&) will both trigger to create that file (without any users action), if you hover that file (because Konqueror needs that information than). But, again Comment=$(echo${IFS}0>~/Desktop/zero.lol&) and/or Name=$(echo${IFS}0>~/Desktop/zero.lol&) will not and should not do that. There is no problem about them. Exactly as there should be not problem for entries not parsed by Konqueror. Exec, URL and such things, which are (should) not external triggered by Konqueror or any other Component of TDE should be safe. The “URL=$( kxdglauncher --getpath --xdgname DOCUMENTS )” is just some good example. It shouldn’t do any harm. Just try out the example. Just with the [$e] it should be nasty. And we can’t think that complicated that someone could change out your desktop file for your documents desktop file, you someone is tricked into clicking on it.

So to make it short: Either just the [$e] functionality (parsing) should be of no effect and therefore just removed (but preserving $(xxx)), or/and some whitelist only for specific entries should be added in the code. Let’s say for URL or Exec, for which is real use of it. The problems just appear because all of the $(xxx) functionality was removed fully.

So there is nothing we can do to make desktop files totaly save (if run by users), exactly as we can’t do that for binary files or shell scripts. I made some further tests, which would too much to write all here down. I might be fully wrong here. But maybe it’s something to think about, before we just come to some fast conclusion, which could be wrong. If wanted, I would really test more and report here every testing result.

About kxdglauncher: That is used also as some kind of prompter to create missing XDG dirs, for TDE to work. It looks for that dir’s if they are opened and prompts to create that dir (with some dialog) if it is missing. On new TDE installs you can see it. Or better on machines where you haven’t got any XDG dirs until this point. So removing that would be wrong. It needs further love, because it lacks other XDG dirs, I wanted to add someday in future. Also this dirs can change to any time. You can change them with some module in kcontrol, for example or with other ways and they need to be read out in real time. So just read them out with session start would be some regression. You would need to promp the user to re-login to see effects and such things.

Something to add:

The TDE functionality for creating own desktop files, for which you can define a own working dir, decide if it should run inside some terminal and such things will create [$e], for example Path[$e]=xxx, also if there is no need for, because that can work without that. But that is just one place of possible many others which create these files (using the nasty [$e]). Unrelated to what is the outcome of that question here, that would needed to find and fix too. I haven’t tested if that files are still running, if that [$e] in it can’t be parsed anymore (so with that commit in).

Sadly this would have discovered much more early, because I noticed it instantly after my rebuild of tdelibs after the release on my working machine, which hadn't updated the working tdelibs many months. That is very sad. I made some personal tests and thought about the topic and re-read that linked file in the original reported issue and want to share my observations and at least my point of view about that topic. It seems this problems has much more impacts as we could think about. For example Amarok isn't playing any streams with that commit. It's just one problem introduced by it. And I think there are a lot more hidden problems now introduced by this commit. I think - But I could(!) be really wrong about that, that the problem was just misinterpreted and the right thing to do was not to completely remove any support of external calls from desktop files $(xxx). Instead of this the problem is just the *unintended* - So without any user action - triggered external call of from desktop files (so [$e]). Because that is also the example shown and spoken about here: https://gist.githubusercontent.com/zeropwn/630832df151029cb8f22d5b6b9efaefb/raw/64aa3d30279acb207f787ce9c135eefd5e52643b/kde-kdesktopfile-command-injection.txt Because otherwise that would not make any sense. Because if some desktop file is run by some *intended* action of some user, it is the responsibility of every user. Exactly it is like running any unknown binary files or shell scrips from any remote places, or something like that. Therefore, any further secure than removing any possibility to trigger desktop actions without any user action would be the wrong way and would probliable do more harm than good. I think, we don't know exactly in which situations this would be just really, really useful or is really needed. (Talking just about $(xxx) here, not the [$e]. Also, just changing out a dynamic URL (which is useful) for some exec isn't really the solution, I think. Because the URL is a right entry for a URL (which is simply needed for good reasons to be dynamic). That shouldn't be some exec just. From my personal point of view, the problem here is not the call of $(xxx), for something like the URL entry. It is only a problem for something like Name, Comment and Icon and only in combination with Icon[$e]=, exactly the way it is pointed out from the file above. Because only the [$e] is the functionality of "dynamic configuration entries" as original called that feature. I tested some combinations myself with the example of the file and the commit we talk about removed. So ready to be infected. From what I got from the document linked, that is only a problem for something like an icon, where the result of that command will end up in the config file (so []), which will be triggered automatically by some external call let's say from Konqueror, because it will start to parse that file for (only specific) entries to know which icon should be displayed, losely said. But for URL it should be not. So you just download some desktop file from the web, drop it in some folder (you think it would not be run, if you don't click it as some binary file), but if you just do open the folder, it will be executed. But with only URL=$(echo${IFS}0>~/Desktop/zero.lol&) that should't happen. So "Icon[$e]=$(echo${IFS}0>~/Desktop/zero.lol&)" will create that file on your desktop, just if you start Konqueror and you open the folder in which the file is. Or the .directory file. But "Icon=$(echo${IFS}0>~/Desktop/zero.lol&)" don't will do anything. Same for Name and Comment. Name[$e]=$(echo${IFS}0>~/Desktop/zero.lol&) and/or Comment[$e]=$(echo${IFS}0>~/Desktop/zero.lol&) will both trigger to create that file (without any users action), if you hover that file (because Konqueror needs that information than). But, again Comment=$(echo${IFS}0>~/Desktop/zero.lol&) and/or Name=$(echo${IFS}0>~/Desktop/zero.lol&) will not and should not do that. There is no problem about them. Exactly as there should be not problem for entries not parsed by Konqueror. Exec, URL and such things, which are (should) not external triggered by Konqueror or any other Component of TDE should be safe. The "URL=$( kxdglauncher --getpath --xdgname DOCUMENTS )" is just some good example. It shouldn't do any harm. Just try out the example. Just with the [$e] it should be nasty. And we can't think that complicated that someone could change out your desktop file for your documents desktop file, you someone is tricked into clicking on it. So to make it short: Either just the [$e] functionality (parsing) should be of no effect and therefore just removed (but preserving $(xxx)), or/and some whitelist only for specific entries should be added in the code. Let's say for URL or Exec, for which is real use of it. The problems just appear because all of the $(xxx) functionality was removed fully. So there is nothing we can do to make desktop files totaly save (if run by users), exactly as we can't do that for binary files or shell scripts. I made some further tests, which would too much to write all here down. I might be fully wrong here. But maybe it's something to think about, before we just come to some fast conclusion, which could be wrong. If wanted, I would really test more and report here every testing result. About kxdglauncher: That is used also as some kind of prompter to create missing XDG dirs, for TDE to work. It looks for that dir's if they are opened and prompts to create that dir (with some dialog) if it is missing. On new TDE installs you can see it. Or better on machines where you haven't got any XDG dirs until this point. So removing that would be wrong. It needs further love, because it lacks other XDG dirs, I wanted to add someday in future. Also this dirs can change to any time. You can change them with some module in kcontrol, for example or with other ways and they need to be read out in real time. So just read them out with session start would be some regression. You would need to promp the user to re-login to see effects and such things. Something to add: The TDE functionality for creating own desktop files, for which you can define a own working dir, decide if it should run inside some terminal and such things will create [$e], for example Path[$e]=xxx, also if there is no need for, because that can work without that. But that is just one place of possible many others which create these files (using the nasty [$e]). Unrelated to what is the outcome of that question here, that would needed to find and fix too. I haven't tested if that files are still running, if that [$e] in it can't be parsed anymore (so with that commit in).
MicheleC commented 2 weeks ago
Owner

@Chris The problem is that allowing execution of commands typed in .desktop or .directory files will open the door to malignous users and security issues.
In my opinion it is better to prevent that and deal with the side-effects in a different but safer way.

@Chris The problem is that allowing execution of commands typed in .desktop or .directory files will open the door to malignous users and security issues.<br/> In my opinion it is better to prevent that and deal with the side-effects in a different but safer way.
MicheleC commented 2 weeks ago
Owner

@Dr_Nikolaus_Klepp sounds like a good idea, probably an easy and working solution. Thanks.
Before applying that though, we still need to investigatewhy it works in kdesktop but not in konqueror. Once we understand the differences, then we can apply your solution.

@Dr_Nikolaus_Klepp sounds like a good idea, probably an easy and working solution. Thanks.<br/> Before applying that though, we still need to investigatewhy it works in kdesktop but not in konqueror. Once we understand the differences, then we can apply your solution.
SlavekB commented 2 weeks ago
Owner

Exactly the way to set variables for XDG paths is one of the options I’ve been thinking about. I consider two ways how do it – to set them either at startup in the stattde script into the session environment or locally before evaluating in TDEConfigBase class.

The first way has the disadvantage that it will not work if the user runs individual applications, not a complete session using starttde.

The second way has the advantage that it can work in all situations and at the same time it does not make a fundamental change in the environment variables.

However, both have the disadvantage that such desktop files will not work properly when loaded by other desktop environments. We can see how similar icons are solved in other desktop environments.

Exactly the way to set variables for XDG paths is one of the options I've been thinking about. I consider two ways how do it – to set them either at startup in the stattde script into the session environment or locally before evaluating in TDEConfigBase class. The first way has the disadvantage that it will not work if the user runs individual applications, not a complete session using starttde. The second way has the advantage that it can work in all situations and at the same time it does not make a fundamental change in the environment variables. However, both have the disadvantage that such desktop files will not work properly when loaded by other desktop environments. We can see how similar icons are solved in other desktop environments.

How many desktop files would be affected - besides the two already mentioned?

How many desktop files would be affected - besides the two already mentioned?
SlavekB commented 2 weeks ago
Owner

@Chris reports some problems with Amarok, which are also related to running external commands. For now, I haven’t examined it any closer.

@Chris do you have any sample files related to problem in Amarok?

@Chris reports some problems with Amarok, which are also related to running external commands. For now, I haven't examined it any closer. @Chris do you have any sample files related to problem in Amarok?
Chris commented 2 weeks ago
Collaborator

We are now not talking about ripping the kxdglauncher fully, right?

For what Slavek suggested, I would prefer the second option. But, please, just add to consideration what I wrote above. There would be the need to read out this variables to any time, because otherwise some change of that dirs isn’t of much use and it would be like in Windows, just you have to restart. I don’t that is the use of having that. It’s expected someone can change the dirs (also per config file) at any time. And if you just click on some music file or you open some desktop file with that variables inside, they will open the right dir. That should be the absolute minimum.

Also - Like I wrote - There are things like Amarok which broke by that change. Because most likely your stream URL will be dynamicly opened in combination with some desktop file or something like that. I don’t know. But if so, it was and is some handy use case.

Also I understand the point here to be secure. But with that considerations there has to keep in mind that we can’t defend users from all bad in the world. If someone has some real life exmaple, how this could cause any harm, without [$e] and just for URL, for example, which is without user action what is the problem pointed out, I would got it. But if there is not such a case, about what we are talking here? We also would need to ripp of autostart file support. Because some infected USB stick could also be some security problem. Please, don’t get the route like KDE and others and be hysteric about such things. A good combination of security and users freedom would be a wonderful thing.

Also, if we just change out all URL= for example with Exec= and Type=Application, we would maybe end up again with some applications expecting URL= (which makes sense) like Dolphin or so would be confused and can’t open that files as some dirs (which should they are), because that would be some application, emulating some dir.

@SlavekB: Sample files? No. You can just open some music stream (online radio) with Amarok. It has most likely to do with the tdeioslave for http. But just from my digging around the TDE source. I have seen more use cases for $(xxxx) in desktop files. I don’t know where I have seen them sadly. But they are out there.

Also, we neither know what is the problem why $$(xxx) doesn’t work and in which cases. I suspect that is because different applications expect that different. Just some guess. But that should be researched. Also what could are possible drawbacks for using just $$(xxx).

A bit of activating the brain can be expected from TDE users. There are other options for using as security hole than just desktop files (with intended action, not unintended and without users action). The autostart functionality is also activateable for users and can be used, if they know what they do.

If you just look how this file, which explains the security problem is worded. It critics desktop enviroments for having own functionality for desktop files, not following the holy XDG fully and absolutely. Now TDE is going to just rip that of.

I am really sorry and really, that is no personal thing. I just want to give some more view to consider and to think about. I wanted that to do also to the original issue, bu decided just to let it this way, because I thought it was never really of any use, which seems to be wrong now. I just know these kind of discussions from other projects and have often seen to what route they have lead.

By the way: I also see no real point (until now) to allow that $(xxx) functionality (not [$e]) with anything other than URL. With Path that isn’t needed anyway, as Slavek showed me already. For Name, Icon, Comment, it can be handy. But yes, I see that the security hole here is much more. But again, that could be activateable exactly the same way as autostart files for external drives, if users want to use that. But TDE should not use that for applications. But, again, to loose that for Name and anything other than URL, at the moment, is reasonable. Seems URL is making the only or most problems here. So if we could whitelist it only for URL, just $(xxx), but not [$e] and ban the whole rest, all would work and TDE would be much more secure, I expect. I am wrong here?

Also: user-dirs.dirs allows to add own XDG dirs. With having the XDG dirs kcontrol module, functionality could be added for users to add own dirs over that, which would be here instantly. They could add some desktop file at own. Or it could be generated at the same time. Using kxdglauncher the same way as the documents dir. But with the read out at session start approach, that would not work in a good way. Users would need to fiddle around more.

We are now not talking about ripping the kxdglauncher fully, right? For what Slavek suggested, I would prefer the second option. But, please, just add to consideration what I wrote above. There would be the need to read out this variables to any time, because otherwise some change of that dirs isn't of much use and it would be like in Windows, just you have to restart. I don't that is the use of having that. It's expected someone can change the dirs (also per config file) at any time. And if you just click on some music file or you open some desktop file with that variables inside, they will open the right dir. That should be the absolute minimum. Also - Like I wrote - There are things like Amarok which broke by that change. Because most likely your stream URL will be dynamicly opened in combination with some desktop file or something like that. I don't know. But if so, it was and is some handy use case. Also I understand the point here to be secure. But with that considerations there has to keep in mind that we can't defend users from all bad in the world. If someone has some real life exmaple, how this could cause any harm, without [$e] and just for URL, for example, which is *without user action* what is the problem pointed out, I would got it. But if there is not such a case, about what we are talking here? We also would need to ripp of autostart file support. Because some infected USB stick could also be some security problem. Please, don't get the route like KDE and others and be hysteric about such things. A good combination of security and users freedom would be a wonderful thing. Also, if we just change out all URL= for example with Exec= and Type=Application, we would maybe end up again with some applications expecting URL= (which makes sense) like Dolphin or so would be confused and can't open that files as some dirs (which should they are), because that would be some application, emulating some dir. @SlavekB: Sample files? No. You can just open some music stream (online radio) with Amarok. It has most likely to do with the tdeioslave for http. But just from my digging around the TDE source. I have seen more use cases for $(xxxx) in desktop files. I don't know where I have seen them sadly. But they are out there. Also, we neither know what is the problem why $$(xxx) doesn't work and in which cases. I suspect that is because different applications expect that different. Just some guess. But that should be researched. Also what could are possible drawbacks for using just $$(xxx). A bit of activating the brain can be expected from TDE users. There are other options for using as security hole than just desktop files (with intended action, not unintended and without users action). The autostart functionality is also activateable for users and can be used, if they know what they do. If you just look how this file, which explains the security problem is worded. It critics desktop enviroments for having *own* functionality for desktop files, not following the holy XDG fully and absolutely. Now TDE is going to just rip that of. I am really sorry and really, that is no personal thing. I just want to give some more view to consider and to think about. I wanted that to do also to the original issue, bu decided just to let it this way, because I thought it was never really of any use, which seems to be wrong now. I just know these kind of discussions from other projects and have often seen to what route they have lead. By the way: I also see no real point (until now) to allow that $(xxx) functionality (not [$e]) with anything other than URL. With Path that isn't needed anyway, as Slavek showed me already. For Name, Icon, Comment, it *can* be handy. But yes, I see that the security hole here is much more. But again, that could be activateable exactly the same way as autostart files for external drives, if users want to use that. But TDE should not use that for applications. But, again, to loose that for Name and anything other than URL, at the moment, is reasonable. Seems URL is making the only or most problems here. So if we could whitelist it only for URL, just $(xxx), but not [$e] and ban the whole rest, all would work and TDE would be much more secure, I expect. I am wrong here? Also: user-dirs.dirs allows to add own XDG dirs. With having the XDG dirs kcontrol module, functionality could be added for users to add own dirs over that, which would be here instantly. They could add some desktop file at own. Or it could be generated at the same time. Using kxdglauncher the same way as the documents dir. But with the *read out at session start* approach, that would not work in a good way. Users would need to fiddle around more.

Ok, I just checked which files use $(…). At least on FreeBSD it’s just these 2 files:

/opt/trinity/share/apps/systemview/documents.desktop
/opt/trinity/share/apps/kdesktop/Desktop/My_Documents

All other files do not use external calls, just variable substitution. I would argue that breaking the desktop-files functionality for other DEs is acceptable.

@Chris: [$e] is not needed on FreeBSD. It’s sufficient to have URL=$$(…), and the $$(…)-part get’s executed. Just imagine the surprise of URL=$$(rm -Rf ~).

Ok, I just checked which files use $(...). At least on FreeBSD it's just these 2 files: /opt/trinity/share/apps/systemview/documents.desktop /opt/trinity/share/apps/kdesktop/Desktop/My_Documents All other files do not use external calls, just variable substitution. I would argue that breaking the desktop-files functionality for other DEs is acceptable. @Chris: [$e] is not needed on FreeBSD. It's sufficient to have URL=$$(...), and the $$(...)-part get's executed. Just imagine the surprise of URL=$$(rm -Rf ~).
SlavekB commented 2 weeks ago
Owner

The original code caused the commands to be executed at the time the configuration file was read => reading of configuration file is clearly not a situation that should cause an external command to run.

The appropriate method, where it happened, has no idea for what type of item is called. So there is no way to make an exception by item type. And there it doesn’t matter if the expand flag is set because [$e] or automatically by item type. Therefore, in this method, only two variants are possible => disable calling external commands and expand only variables for all situations or enable external commands for all situations. And the second variant is not desirable.

The original code caused the commands to be executed at the time the configuration file was read => reading of configuration file is clearly not a situation that should cause an external command to run. The appropriate method, where it happened, has no idea for what type of item is called. So there is no way to make an exception by item type. And there it doesn't matter if the expand flag is set because `[$e]` or automatically by item type. Therefore, in this method, only two variants are possible => disable calling external commands and expand only variables for all situations or enable external commands for all situations. And the second variant is not desirable.
Chris commented 2 weeks ago
Collaborator

I see the point here of the expand flag is set automatically. That is some problem, yes.

Can you show me how to trigger the read out of the configuration file? Because I played with it again. I just used the documents file and changed out the kxdglauncher command for URL, with the example from the file and tried to trigger it. If it is some problem, it should create some file on the desktop, right? From my testing not all items are readout (and therefore executed) for all actions. Hovering reads out the comment, but not preview for me. Activating icons and view some folder with the desktop file triggered to read out the Icon item.

Playing with it again, to verify.

I changed out now the URL again from My_Documents with the command shown in the example of that report. So for My_Documents, without that commit, the URL=$( kxdglauncher --getpath --xdgname DOCUMENTS ) will be URL=$(echo${IFS}0>~/Desktop/zero.lol&). And also if I refresh the desktop (which should trigger to read out at least the icon entry) the file isn’t created. So the URL is not executed. Only if I click by intention on the documents icon, the file will be created and therefore only then the command is executed. So far my testing.

EDIT: So after playing with that more, I really found a way to execute that command and create that file for the example above. If you watch the properties of that changed My_Documents file, with the changed URL. Than it will trigger the command. But that is just because the URL needs to be readout than, because the URL is shown in that dialog.

So also having a too complex parser is a bad thing too, I fully see some argument for.

So I think, the most easy way would be really just to use $$(xxx) as replacement. And fix why it is not working for all things. Therefore we can keep the kxdglauncher and the gain of still using the kcontrol module for paths with real use. But as already noticed, for this URL example for XDG things, using $$ is working. So for all, or I am wrong about that? It just doesn’t work for other things. If that is not doable for all things, we could use some alternative for things where $$ is not working. This way some URL at least would be still some URL and no crimpled Type=Application.

About Amarok: Just (with that commit) try to open some URL like http://*.mp3, for example. It will complain there is no tdeioslave module for open that. After reverting the patch, it instantly works again. So that is related.

I see the point here of the expand flag is set automatically. That is some problem, yes. Can you show me how to trigger the read out of the configuration file? Because I played with it again. I just used the documents file and changed out the kxdglauncher command for URL, with the example from the file and tried to trigger it. If it is some problem, it should create some file on the desktop, right? From my testing not all items are readout (and therefore executed) for all actions. Hovering reads out the comment, but not preview for me. Activating icons and view some folder with the desktop file triggered to read out the Icon item. Playing with it again, to verify. I changed out now the URL again from My_Documents with the command shown in the example of that report. So for My_Documents, without that commit, the *URL=$( kxdglauncher --getpath --xdgname DOCUMENTS )* will be *URL=$(echo${IFS}0>~/Desktop/zero.lol&)*. And also if I refresh the desktop (which should trigger to read out at least the icon entry) the file isn't created. So the URL is *not* executed. Only if I click by intention on the documents icon, the file will be created and therefore only then the command is executed. So far my testing. EDIT: So after playing with that more, I really found a way to execute that command and create that file for the example above. If you watch the *properties* of that changed My_Documents file, with the changed URL. Than it will trigger the command. But that is just because the URL needs to be readout than, because the URL is shown in that dialog. So also having a too complex parser is a bad thing too, I fully see some argument for. So I think, the most easy way would be really just to use $$(xxx) as replacement. And fix why it is not working for all things. Therefore we can keep the kxdglauncher and the gain of still using the kcontrol module for paths with real use. But as already noticed, for this URL example for XDG things, using $$ is working. So for all, or I am wrong about that? It just doesn't work for other things. If that is not doable for all things, we could use some alternative for things where $$ is *not* working. This way some URL at least would be still some URL and no crimpled Type=Application. About Amarok: Just (with that commit) try to open some URL like http://*.mp3, for example. It will complain there is no tdeioslave module for open that. After reverting the patch, it instantly works again. So that is related.
MicheleC commented 1 week ago
Owner

I have analysed this issue and here is a proposed solution.

  1. kxdglauncher is no longer required. We can use xdg-user-dir to get the same information. kxdg-launcher only checks for documents and desktop folder and can be safely removed.

  2. in tdecore/tdeconfigbase.cpp, we can reuse part of the code that was removed by commit 1074eb0336 and run “xdg-user-dir DOCUMENTS” or “xdg-user-dir DESKTOP” when a specially defined URL is found. For example we could have $(DOCUMENTS) and $(DESKTOP) in the .desktop files in the URL field.

This should work within TDE, will probably not work in other DE but I wouldn’t worry about it too much.

Please let me know what you think about it before I work on the code.

I have analysed this issue and here is a proposed solution. 1. kxdglauncher is no longer required. We can use xdg-user-dir to get the same information. kxdg-launcher only checks for documents and desktop folder and can be safely removed. 2. in tdecore/tdeconfigbase.cpp, we can reuse part of the code that was removed by commit 1074eb0336 and run "xdg-user-dir DOCUMENTS" or "xdg-user-dir DESKTOP" when a specially defined URL is found. For example we could have $(DOCUMENTS) and $(DESKTOP) in the .desktop files in the URL field. This should work within TDE, will probably not work in other DE but I wouldn't worry about it too much. Please let me know what you think about it before I work on the code.

Sounds good for me.

Sounds good for me.
SlavekB commented 1 week ago
Owner

My idea was as follows:

Add parsing from xdg-user-dir into the readEntry method and either export all XDG_*_DIR variables internally to the environment or add recognition that the XDG_*_DIR variable is required and use the value we have already read.

Regardless of the way XDG_*_DIR is interpreted in the readEntry method, in desktop files, using it would be the same as using an environment variable – so $XDG_DOCUMENTS_DIR.

My idea was as follows: Add parsing from xdg-user-dir into the `readEntry` method and either export all `XDG_*_DIR` variables internally to the environment or add recognition that the `XDG_*_DIR` variable is required and use the value we have already read. Regardless of the way `XDG_*_DIR` is interpreted in the `readEntry` method, in desktop files, using it would be the same as using an environment variable – so `$XDG_DOCUMENTS_DIR`.
MicheleC commented 1 week ago
Owner

Fixed by these PRs. Please test both in Konqueror and KDesktop.

  1. TDE/tdelibs#61
  2. TDE/tdebase#120

Note: if I open my document folder from my desktop, I see duplicated entries in the Konqueror side bar under “system”. This seems to happen systematically, which may be good for troubleshooting. But it does not happens if I open it from Konqueror.
Please confirm whether you see the same thing or not on your system.

Fixed by these PRs. Please test both in Konqueror and KDesktop. 1. TDE/tdelibs#61 2. TDE/tdebase#120 Note: if I open my document folder from my desktop, I see duplicated entries in the Konqueror side bar under "system". This seems to happen systematically, which may be good for troubleshooting. But it does not happens if I open it from Konqueror.<br/> Please confirm whether you see the same thing or not on your system.
MicheleC commented 1 week ago
Owner

latest version of the fix does not use xdg-user-dirs at all. Instead relies on existing code already in tdelibs.

latest version of the fix does not use xdg-user-dirs at all. Instead relies on existing code already in tdelibs.
MicheleC removed the
PR/rfc
label 1 week ago
Chris commented 1 week ago
Collaborator

Just for reference. My custom created desktop files are working fine with this like before and I couldn’t trigger that Amarok problem anymore. It might be because the streams were added back then with a playlist file, which was temporary located at the documents dir. I just re-added this streamlist and all is working fine.

Just for reference. My custom created desktop files are working fine with this like before and I couldn't trigger that Amarok problem anymore. It might be because the streams were added back then with a playlist file, which was temporary located at the documents dir. I just re-added this streamlist and all is working fine.

FreeBSD: Recompiled (without tdegraphics & tdeedu - both fail at the moment) and tested, but does not work. “system:/documents” gives a messagebox “not found”.

FreeBSD: Recompiled (without tdegraphics & tdeedu - both fail at the moment) and tested, but does not work. "system:/documents" gives a messagebox "not found".
Sign in to join this conversation.
No Milestone
No Assignees
4 Participants
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
Cancel
Save
There is no content yet.