Skip to content

Add Windows Path Sanity Checks#3097

Merged
briaguya0 merged 1 commit intoHarbourMasters:developfrom
Malkierian:path-sanity
Nov 6, 2023
Merged

Add Windows Path Sanity Checks#3097
briaguya0 merged 1 commit intoHarbourMasters:developfrom
Malkierian:path-sanity

Conversation

@Malkierian
Copy link
Contributor

@Malkierian Malkierian commented Jul 28, 2023

Add path checks for Windows for temp directory usage and write/modify permissions to prevent execution. Temp path uses the environment variable for TEMP and checks the runtime path to see if it's in the temp path, which would imply trying to run SoH out of the .zip. This has a backup of recreating the default TEMP path in the case TEMP is garbage or nonexistent using the USERPROFILE variable which, if missing or broken, would break Windows itself.

For permissions, a file and a folder are both created and deleted to check write and modify permissions respectively, properly encapsulated in try...catch statements where necessary.

In the case either of these fails, the user is notified that they're trying to run SoH in a way that would break things and instructed on how to proceed to get it running, then SoH exits.

Build Artifacts

@Malkierian
Copy link
Contributor Author

Malkierian commented Jul 28, 2023

As a side note, this is only necessary because Windows is setup to use the program directory instead of an AppData or User data directory. I don't know why we do this.

@Spodi
Copy link
Contributor

Spodi commented Jul 28, 2023

Better way for program files check would be to check for write permissions instead. This would also make the message appear if they're somewhere else without permissions.

Buuut I don't know myself how to look for permissions yet.

@Malkierian
Copy link
Contributor Author

The problem I've seen is that it's primarily rewrite permissions that are blocked. Like, an initial save works, but subsequent saves fail. So you'd have to write the same test file twice to truly test the issue.

@Archez
Copy link
Contributor

Archez commented Aug 11, 2023

The problem I've seen is that it's primarily _re_write permissions that are blocked. Like, an initial save works, but subsequent saves fail. So you'd have to write the same test file twice to truly test the issue.

I'm not sure I completely understand _re_write as a permission here. Quick searching suggests that you might have to do both a directory and file check. Also maybe the difference is between system write vs user write. I'm not sure what the default permissions are when the save directory/a save file is first created.

@Malkierian
Copy link
Contributor Author

I'm not sure I completely understand _re_write as a permission here.

I'm just saying, at least half the time I've seen someone who put SoH in, say, Program Files, they didn't have trouble with creating an OTR, and their first game save worked, it just suddenly stopped saving later. I don't know the actual mechanism behind why that happens.

@leggettc18
Copy link
Contributor

I feel like this issue could be solved instead with a proper installer for Windows (should be doable via CPack). That way it would have permissions already for the folder in Program Files that the installer creates, and the installer should also be able to install to AppData instead if desired.

@Malkierian
Copy link
Contributor Author

I agree that would be the ideal end goal, but how long before that happens would we have to continue to deal with some people doing these things?

Copy link
Contributor

@garrettjoecox garrettjoecox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s get this in, it sounds like there are better solutions to this problem but none have been submitted in the 5 months this has sat around

…hive), and proper file permissions (write/modify, to prevent things like Program Files or Windows, or other folders we couldn't know about that don't have proper file permissions). Instructs users as to what it discovered and how to fix, then exits.
@briaguya0 briaguya0 merged commit 8e00265 into HarbourMasters:develop Nov 6, 2023
@Malkierian Malkierian deleted the path-sanity branch November 6, 2023 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants