DavidoTek / ProtonUp-Qt

Install and manage GE-Proton, Luxtorpeda & more for Steam and Wine-GE & more for Lutris with this graphical user interface.
https://davidotek.github.io/protonup-qt
GNU General Public License v3.0
1.16k stars 39 forks source link

util: Use os.path.abspath for is_valid_launcher_installation #381

Closed sonic2kk closed 2 months ago

sonic2kk commented 2 months ago

Fix #378.

If the compatibilitytools.d dir is a symlink, realpath will resolve the symlink before expanding and moving up a directory, meaning we will end up looking for the Steam data files in the symlink's parent directory instead of the Steam root, as we assumed the parent folder of compatibilitytools.d would always be a Steam root directory.

To fix this, we use abspath which will resolve the path without expanding symlinks.

We probably use realpath in a lot of places where symlinks might be used, so we might want to consider revisiting these. Anywhere we might expand an install directory path before moving up a directory is likely unsafe. I thought we did this for some cases with Lutris/Heroic (to install in the runtimes folder instead of the runners folder, for example) but I didn't spot any with a quick glance. It's also pretty late so I might've just missed it :smile:

Reference:

sonic2kk commented 2 months ago

Not sure if it's worth adding a comment on why we're using abspath instead of realpath here... I didn't really know the difference between them until tonight and the Python docs, to my eyes, weren't as clear about the differences. Then again maybe this is not worth noting.

DavidoTek commented 2 months ago

Thanks!

That's interesting, I wouldn't have expected this to cause an issue. I thought if there was a symlink, it would include the entire Steam folder and not just the compatibilitytools.d. Good that it is fixed now.

We probably use realpath in a lot of places where symlinks might be used, so we might want to consider revisiting these

There are cases where this behavior is actually desired: If there is a Steam installation at ~/.steam/root and a symlink ~/.local/share/Steam that points to the actual folder, os.path.realpath in constants.py will resolve both paths to the actual path at ~/.steam/root and get rid of the duplicates using list(dict.fromkeys(...))* As far as I can remember, this is also the reason why we put ~/.local/share/Steam first in the list because it is more likely that a symlink at ~/.steam/root will point to ~/.local/share/Steam and not the other way around like I described above (I think we already had the second situation in some issue).

https://github.com/DavidoTek/ProtonUp-Qt/blob/23b60df1ee379103d6652af08630d89a2a6ae1b6/pupgui2/constants.py#L28-L33

Not sure if it's worth adding a comment on why we're using abspath instead of realpath here...

Might actually be worth adding it, there are already a few other comments, so it should be fine. I think it is easy to google the difference, but it could be helpful to know why exactly we don't want to resolve symlinks here (I guess we should also link/ID this PR in the comment then).


*there is a funny behavior: If you create a symlink ~/.steam/root -> ~/.var/app/com.valvesoftware.Steam/data/Steam, it will display the correct path (~/.var/app/...), but it detects it as a native/non-Flatpak installation. I don't think that is anything we need to worry about though :smile: