Disable some unsafe CLI arguments in template builds by default.

[bruvzg]

Follow up to #108818

Splits disable_overrides into two:

• disable_overrides - disabled by default, only affect project config override (which can be also disabled via project setting).
• disable_path_overrides - enabled by default, disables project path/pack overrides and script CLI arguments, and forces PCK/project loading from executable or resource directory instead of CWD to ensure only project files bundled with executable are loaded.

[Calinou]

Tested locally, it works as expected.

However, I'd make the existing arguments print an error when run, so that it doesn't fail silently like it currently does:

$ path/to/template --path ~/d/3d/truck_town/
Error: Couldn't load project data at path ".". Is the .pck file missing?
If you've renamed the executable, the associated .pck file should also be renamed to match the executable's name (without the extension).

See an example of this here:

godot/main/main.cpp

Lines 932 to 934 in 9cd297b

	ERR_PRINT(
	"`--test` was specified on the command line, but this Godot binary was compiled without support for unit tests. Aborting.\n"
	"To be able to run unit tests, use the `tests=yes` SCons option when compiling Godot.\n");

Also, note that if you cd to a project folder (not a PCK), you can still run it directly with an export template binary:

$ ~/d/3d/platformer: ~/g/bin/godot.linuxbsd.template_release.x86_64

Therefore, I'm not sure the CWD handling is working correctly.

(Moving or symlinking the binary to the project folder and running it there will keep working, but I think that part is to be expected.)

[Calinou]

Note that this PR will impact https://github.com/GodotModding/godot-mod-loader due to the removal of --script, unless the project uses a custom export template compiled with disable_path_overrides=no. It will have to find another means of injecting its code into projects.

godotengine/godot-proposals#6137 could make this possible using override.cfg, as long as the project uses official export templates (or doesn't change disable_overrides from its default no state in custom templates).

[bruvzg]

godotengine/godot-proposals#6137 could make this possible using override.cfg.

There's already a way to use it without any changes (see comment in the proposal), which looks a bit hacky but should work.

[bruvzg]

Also, note that if you cd to a project folder (not a PCK), you can still run it directly with an export template binary

Seems like get_bundle_resource_dir on Linux is ., so this is likely the reason, I will add some extra checks.

[bruvzg]

Some notes:

Options disabled by disable_path_overrides (enabled by default for templates, always false for editor):

• --path and --scene - used to run editor instances (including project running form editor), so should not be needed for the export templates.
• --upwards - used for unit test in the editor, so not needed for templates.
• -s, --main-loop - intended primarily for the engine development (but also used by mod loader, so will break its current installation method).
• --main-pack - seems to be used only on Web, so enabled for this platform only.

Options disabled in release templates:

• --remote-fs and --remote-fs-password - can be used by editor to run project on remote device (editor always use debug template).

[akien-mga]

--upwards - not sure why this option exist, seems completely useless, maybe we should remove in entirely?

Yeah that's a legacy option, it lets you run a Godot project while in a subfolder by looking up the project.godot file recursively. It's dates back to Godot being open sourced.

Let's keep it for now but deprecated (printing a warning about it), so if users rely on it in some scripts, they'll have until 4.7 to adapt their scripts, and we can drop it then.

Edit: Actually scratch that, let's just drop it straight and document it in the migration guide. It's simple enough to work around when upgrading.

[bruvzg]

--upwards - not sure why this option exist, seems completely useless, maybe we should remove in entirely?

Actually, it is used for a bunch (GLTF, DDS, GDScript) of unit tests (which make sense, tests are upwards of bin sub-folder), so I guess it should stay at least in the editor builds. Not as argument, but underlaying code is used.

[bruvzg]

Removed --upwards argument, but not ProjectSetting code for it, since it used (by directly calling setup) by unit tests.

[Calinou]

CWD handling seems to be working now, and a message is printed when using --path in an unsupported binary. I agree with the removal of --upwards as an exposed option, I haven't seen anyone rely on it recently.

Code looks good to me (same for the CLI help changes).

[Ivorforce]

The feature sounds good to me. If I understand it correctly, this would require users to start editing the overrides.cfg to inject mods? If yet, that would be a good indicator of improved security, while still keeping mods pretty accessible.

I've also reviewed the code. I'm only a bit familiar with the main interfaces, but from what I can judge this looks fairly straight-forward. Looks good to me anyway.

When we merge this, we should definitely document the compile options (compile and project settings), so people can make an active choice about whether to support mods.

[Ivorforce]

It feels a bit weird to me that disable_path_overrides is silently ignored when building an editor build. But I'm not sure if it's important to fix this.

[bruvzg]

If I understand it correctly, this would require users to start editing the overrides.cfg to inject mods?

Or editing PCK, or both.

It feels a bit weird to me that disable_path_overrides is silently ignored when building an editor build.

It is, but I'm not sure how to handle it, since it's both enabled for editor and disabled for templates by default.

[Ivorforce]

The main solution in my mind would be to have it 'unspecified' by default, and if it's specified it overrides the defaults. But I'm not sure if we do this for any other options, plus the editor wouldn't function as expected with the option explicitly enabled anyway. So it's probably fine to keep it as is.

[Repiteo]

We have some options that will automatically infer their status based on if it's an editor build or not, but those are more options for the SCons environment itself rather than actual arguments iirc. Unsure how much effort it would take to make that "just work" natively

[Ivorforce]

Looks like at least one person is actively (trying to) use --upwards.
Do we have any indication of how popular it is as part of people's workflows?
#111647

[Repiteo]

I'd recommend removing the --upwards changes from this PR in that case. I still think its removal makes sense, but it can be handled separately

EDIT: Actually, the OP says it's not critical, so we'll go ahead with removing it.

[Repiteo]

Thanks!

[clayjohn]

With this PR I can no longer run an exported Android project. Using the default config for Android exports, I get an endless spam of

ERROR: Scene path was specified on the command line, but this Godot binary was compiled without support for path overrides. Aborting. To be able to use it, use the `disable_path_overrides=no` SCons option when compiling Godot.

I have not done anything to specify Scene path on the command line.

It sounds like Android exports were implicitly relying on path overrides somewhere.

[bruvzg]

It sounds like Android exports were implicitly relying on path overrides somewhere.

Will check it in a moment, it should not depend on scene path.

[clayjohn]

ERROR: Scene path was specified on the command line, but this Godot binary was compiled without support for path overrides. Aborting. To be able to use it, use the disable_path_overrides=no SCons option when compiling Godot.

Looks like the logic is just incorrect

		} else if (E->get().length() && E->get()[0] != '-' && positional_arg.is_empty() && game_path.is_empty()) {
#if defined(OVERRIDE_PATH_ENABLED)
			positional_arg = E->get();

			String scene_path = ResourceUID::ensure_path(E->get());
			if (scene_path.ends_with(".scn") ||
					scene_path.ends_with(".tscn") ||
					scene_path.ends_with(".escn") ||
					scene_path.ends_with(".res") ||
					scene_path.ends_with(".tres")) {
				// Only consider the positional argument to be a scene path if it ends with
				// a file extension associated with Godot scenes. This makes it possible
				// for projects to parse command-line arguments for custom CLI arguments
				// or other file extensions without trouble. This can be used to implement
				// "drag-and-drop onto executable" logic, which can prove helpful
				// for non-game applications.
				game_path = scene_path;
			}
#else
			ERR_PRINT(
					"Scene path was specified on the command line, but this Godot binary was compiled without support for path overrides. Aborting.\n"
					"To be able to use it, use the `disable_path_overrides=no` SCons option when compiling Godot.\n");
			return EXIT_FAILURE;
#endif // defined(OVERRIDE_PATH_ENABLED)

It currently fails without even checking if scene paths are used

In other words, when override path is disabled it just always fails if it reaches this block. But it reaches this block if there is an argument starting with "-", so it catches way too much

[bruvzg]

Yes, it's likely it, and should only disable in the game_path setting section.

[clayjohn]

Yes, it's likely it, and should only disable in the game_path setting section.

Ya, it should only fail if the user attempts to use a scene_path. If scene_path is empty, it shouldn't fail

[FeldrinH]

It seems I'm a bit late to the party, but I wanted to add that I have developed some mod loaders for Godot-based games (https://github.com/FeldrinH/TASmaniac, https://github.com/FeldrinH/Luckloader) and the --script entrypoint was extremely clean and powerful for this.

I'm not sure I understand why this needed to be disabled by default, while override.cfg based injection points remain enabled by default.

[FeldrinH]

Also, am I correct that with this change all options to override the main loop are disabled by default? TASmaniac (linked above) currently overrides the main loop to sleep between frames (this is used together with --fixed-fps to get fully deterministic frame times while running the game in real time).

[bruvzg]

I'm not sure I understand why this needed to be disabled by default, while override.cfg based injection points remain enabled by default.

Unlike --script, override.cfg require adding files directly to the game folder, which (in case of reasonable setup) is only possible if deliberately done by the user or app with admin privileges.

Also, override.cfg can be disabled in the project settings as well.

Also, am I correct that with this change all options to override the main loop are disabled by default?

Yes, main loop override is disabled as well.

All these options existed for development purpose only, are still available in the editor builds, and can be enabled for a custom build.

[FeldrinH]

All these options existed for development purpose only, are still available in the editor builds, and can be enabled for a custom build.

That's kind of unfortunate for me, since I'm trying to mod games using release builds. I guess I'll have to see if I can make things work without control over the main loop.

[Ivorforce]

That's kind of unfortunate for me, since I'm trying to mod games using release builds. I guess I'll have to see if I can make things work without control over the main loop.

Sorry about making things more difficult for you!
While we're removing features from release builds for security purposes, we are interested in working with mod (loader) authors such as you to bring those back in a secure way.

If you have been relying on main loop replacement, I want to encourage you to open a proposal to re-implement this in a way more similar to override.cfg, i.e. with the qualities that bruvzg mentioned above.

[FeldrinH]

Sorry about making things more difficult for you! While we're removing features from release builds for security purposes, we are interested in working with mod (loader) authors such as you to bring those back in a secure way.

Thanks, I appreciate it. I'll have to take some time to experiment, right now I'm not fully clear on what has been restricted and how many of the restricted features I strictly need.

[FeldrinH]

Just to be clear, can I use override.cfg to change application/run/main_loop_type to a custom type defined in a gdscript file located next to the game executable or is that now restricted in release builds?

[bruvzg]

Just to be clear, can I use override.cfg to change application/run/main_loop_type to a custom type defined in a gdscript file located next to the game executable or is that also restricted in release builds?

Yes, this should work (have not tested, but this kind of overrides was not disabled).

[Cykyrios]

I was just hit with this while exporting demos for a project of mine (non-game project, but runs a TCP connection to interact with a non-Godot game). Instead of exporting a full executable + pck for each demo, I was exporting a single executable, all pck files, and made small .sh/.bat files that were calling --main-pck, to save space on those 5 demos.

It looks like I can work around the issue with a little hack: renaming the executable to correspond to each pck in the corresponding .sh/.bat file seems to do the trick.