Split wrapper string into separate command and arguments

[webbgamers]

Fixes #1035 by splitting the wrapper string by whitespace using split_whitespace and only passing the first element as the main command with all subsequent elements passed as arguments. This allows usage of multiple wrappers and passing arguments to wrapper scripts.

Example:
gamemoderun gamescope -f -w 2560 -h 1440 -W 3840 -H 2160 -- (changes fullscreen resolution)
In this example, gamemoderun is the base command and everything else is passed as an argument. gamemoderun runs gamescope which in turn runs the game.

I don't think Modrinth needs to use a special identifier for the game executable like how Steam uses %command% since that is primarily used for passing arguments to the game which Modrinth already provides in the Java arguments option.

[AlexTMjugador]

Thank you for your contribution!

However, naively splitting the command string into its executable and argument parts by only taking into account whitespace characters is prone to cause further issues in cases where the command path contains spaces, or when arguments themselves have to include spaces.

To avoid this class of problems, I recommend the following approach:

• Use a crate like shlex for cross-platform, POSIX-shell-like command word splitting. This will correctly handle quotations and backslash escapes, such as in 'C:\Program Files\My Program.exe' an argument "with multiple words". (Windows users will need to quote paths with backslashes properly, but this seems like a reasonable trade-off for a techy feature on an OS known for its inconvenient path separator character. A consistent interpretation across platforms seems more worthwhile, as the application data could be shared among operating systems in e.g. dual boot scenarios. Incidentally, PrismLauncher implements a shlex-like logic for this.)
• Note that the point above may cause existing wrapper commands to no longer be treated as literal paths to an executable. To ensure a smooth transition, I propose implementing a database migration that would add a new column to the settings table for tracking the format version of command strings. Then, application logic can quote all wrapper commands in the database to retain their original meaning and update the format version in a single transaction.

I think it's also worth mentioning that pre- and post-launch hook commands currently use a similar whitespace-splitting logic to the one originally proposed in this PR, which is an inconsistency likely to be noticed by users and also cause problems. Ideally, we should extend the use of shlex and apply the same migration strategy to these hook commands as well.

[triphora]

Mostly looks good just one thing

[webbgamers]

Thanks, I will check that. I still want to play around with how I handle migration of Path to Command WrapperType. When a profile is loaded, I want to use shlex::quote to safely quote old wrappers and save them as new wrappers. At the very least, I'd want to perform the migration before calling launch_minecraft and removing the WrapperType arg and associated logic.

I don't quite understand how Profile::perform_launcher_feature_migration works or if that would be a good place to put migration code like what I want to do.
https://github.com/modrinth/code/blob/main/packages/app-lib/src/state/profiles.rs#L738

[AlexTMjugador]

It's great to see some progress on this, @webbgamers! 🚀

I agree that we should remove WrapperType entirely and start treating all wrappers as commands from now on, without any type distinction.

I wasn't aware of the launcher_feature_version profile field when I wrote my earlier comment, but using a new value of it to track this migration instead of a new settings field is the best approach I can think of right now. Well spotted!

Profile::perform_launcher_feature_migration is called for every profile at application startup, so it runs at exactly the right time in the right context. You'll just need to add a new variant to the LauncherFeatureVersion enum, update its MOST_RECENT associated constant, and add a case for it to the match in Profile::perform_launcher_feature_migration. This new case should handle the migration by querying the profile, escaping its hook paths to commands, and updating the feature version.

Does that clear things up for you? Please let us know if you have any other questions 🙂

Edit: by the way, @triphora's shout about running sqlx prepare in the wrong folder is also something that should be taken care of. For app migrations, you should run sqlx prepare on the app-lib package folder.

[webbgamers]

Thanks for clarifying, this is definitely a much better solution. Should be good to go now!

[AlexTMjugador]

This is looking great! Hopefully, these are my final review comments before we can merge the PR 😄

(Just a reminder: all CI checks must pass before the PR can be merged, so please keep an eye out for any lints Clippy might raise and fix them.)

[AlexTMjugador]

Several notes here:

• A migration for the pre_launch and post_exit hooks seems to be missing here. Such migrations must be added. (To avoid repetitive code, you could try iterating over an array of mutable references to the different command strings.)
• As noted in the shlex documentation, NUL characters have no place in the middle of proper command strings. They will cause the command to fail to launch, or even worse, introduce pretty odd behavior or security vulnerabilities. That said, treating their presence as a fatal migration error could render the application unusable (which is a pretty heavy-handed approach when only one of tens of instances may have odd data), while ignoring them (as the app currently seems to do) will break command hook functionality for the affected profiles. I propose stripping out NUL bytes from the string before quoting it with shlex, and logging a warning if such a replacement occurs. This way, in the unlikely event that NUL bytes are present (perhaps due to database corruption or similar anomalies), we provide the best possible automated recovery in most cases that cleans up the wrong data, while keeping users and support people informed about what correction we applied.

[webbgamers]

1. pre_launch and post_exit both previously used the "naive" whitespace splitting method so switching to shlex splitting won't break existing configs.

2. I don't think ignoring NUL chars will break any existing wrapper hooks since whatever was there was just being run that way before. All we are really doing is running shlex::quote so they can be later parsed by shlex::split which doesn't care about NUL chars as far as I can tell. I figure if there wasn't any validation before, no need to add it now as part of this PR. If anything I would suggest making a new issue for it to allow for further discussion on what kind of validation should be done and adding the same validation to pre_launch and post_exit. For a thorough validation, there are other problematic characters that would need to be addressed properly.

[AlexTMjugador]

Actually, there's a slight chance that switching to shlex could break existing pre-launch and post-exit hooks if they aren't properly migrated, despite them already having a naive argument splitting logic. To illustrate, let's consider a hook string like /home/user/"stuff"/program arg. Previously, the app would launch /home/user/"stuff"/program, passing arg to it. But with shlex, it would interpret the program to run as /home/user/stuff/program, which would point to a completely different directory lacking quotation characters. So I think this migration should be handled; while it might only affect, say, 0.01% of users, the Modrinth App is deployed at such a scale that even such a tiny percentage still translates to a non-zero and significant number of people.

As for the issue with NUL characters, to rephrase what I said before: if they were present in the past, they shouldn't have been, since they wouldn't have worked anyway. While shlex::quote, shlex::split, and most Rust string functions handle NULs fine as you mentioned, the operating system and other external programs typically don't: they treat NUL as the end of a string, as is standard in the C world.

That's why I proposed doing a one-time NUL cleanup during this migration: it's a good opportunity to fix some odd behavior that may have existed previously. And indeed, this isn't a proper validation step: we're not enforcing that new shlex-quoted wrapper commands are free of interior NULs, which leaves room for advanced users to deliberately use NUL bytes if they really need to.

Eventually, though, I believe we should move toward either validating the complete absence of NULs or stripping them out before use, depending on how this migration plays out. Having clean, migrated data now will give us a solid foundation for whatever we decide later: garbage in, garbage out, after all.

[webbgamers]

Thanks for the example, I would argue that anyone with quotes in their filenames deserves to have their config broken... jk
Anyways, I will need to handle migrations differently for pre-launch and post-exit hooks since they were previously parsed differently, but should be straight forwards.

I have also realized that this system will not migrate the global setting hooks so I need to come up with a way to do that. Probably will involve creating a migration system similar to the profile launcher_feature_version system... 🫠 Any suggestions for this would be appreciated.

On the NUL issue, I'm not sure if just stripping them from hook strings is necessarily the best approach. If there is a NUL in the string, as you mentioned it is almost certainly the result of some kind of database corruption and in that case you can't make any assumptions about the integrity of the rest of the wrapper string or really the entire database. SQLite is very robust to corruption and my guess is that you would get SQL errors before getting invalid data from the database. If there is a need to be resilient from invalid data in the database I don't think it is best addressed by this PR as the original goal was just bringing wrapper parsing to parity with pre-launch and post-exit parsing, and that has already expanded in scope. I would really prefer to just assume whatever was in old string is valid and handle the migration without trying to perform any kind of validation.

[AlexTMjugador]

Hmm, I see your point about not bothering with the NUL fixups. You're right that once the database is corrupted, trying to salvage that particular piece of data is probably a lost cause. Let's not do it in this PR; we can always revisit that later if needed.

Regarding the global hooks, I agree that something along the lines of the migration approach I've previously suggested may be necessary. It sounds like the cleanest approach would be to migrate both global and instance-specific hooks using such a migration. You could add a new function to the migrate_legacy_data module, similar to the already existing migrate_legacy_data, and call it from within State::initialize_state.

not qualified enough to rev