Add minetest.settings to CSM API and allow CSMs to provide settingtypes.txt

[AFCMS]

Add compact, short information about your PR for easier understanding:

Fix #10382
Allow CSMs to use minetest.settings.
Allow CSMs to provide settingtypes.txt

To do

This PR is Ready for Review.

• Expose minetest.settings to CSM API
• Expose minetest.setting_get_pos to CSM API
• Add test to preview CSM
• Allow client mods to provide settingtypes.txt
• Add documentation

How to test

Use this simple script inside a CSM and check the console output:

print(minetest.settings:get_bool("enable_client_modding", false))

Open the settings tab and check is there is a Client Mods tab.

[rubenwardy]

The original issue for this was rejected: #5454

I'm hesitant to add features to CSM without any consensus on what the future of SSCSM and CSM will be, especially when the features are potentially security sensitive like exposing the entire settings object (which may contain secrets)

[AFCMS]

@rubenwardy

The original issue for this was rejected: #5454

#10382 is still open, just low priority

#5454 was closed mostly because of no core devs interested into implementing it as far as I can see.

I'm hesitant to add features to CSM without any consensus on what the future of SSCSM and CSM will be

Its literally a 4 lines feature, and its very useful for CSMs like keystrokes which require us to edit its files to change where is the HUD on the screen.

Does it make any sense to have keystrokes as a SSCSM instead of a CSM? No of course.
This feature just makes easier for players to change their client settings without messing with conf files.
In the past few days I had to explain on Discord how to configure this specific CSM to a kid and it took 5mn for him to understand the right way to do it.

Its also useful for people who need SSCSM: https://git.minetest.land/MineClone2/MineClone2/issues/1882

especially when the features are potentially security sensitive like exposing the entire settings object (which may contain secrets)

CSMs are user installed, so it shouldn't cause any problem. Its exactly the same risk as user that install random minetest mods to play in singleplayer. Yes, there is a risk, but its only with people who doesn't even know what they are doing. (also, you need to connect to a malicious server for a CSM to be able to drop your secrets)

Exposing all settings to SSCSM would be a very bad idea, but its just fine (and useful!) for CSM.

[TurkeyMcMac]

Also, if settings support is added and is later found to be a bad idea for whatever reason, it can just be removed, since the CSM API is unstable.

[kneekoo]

I'm hesitant to add features to CSM without any consensus on what the future of SSCSM and CSM will be,

Lack of consensus (and focus) can last for a long time, so that's not a good enough reason to turn down useful features.

especially when the features are potentially security sensitive like exposing the entire settings object (which may contain secrets)

That's indeed critical, and this can be treated as a whitelist/blacklist of settings that can be safely exposed or not, or only expose an object key ("safe"?), where the publicly safe settings can live, so there would no need to maintain any list.

[AFCMS]

especially when the features are potentially security sensitive like exposing the entire settings object (which may contain secrets)

That's indeed critical, and this can be treated as a whitelist/blacklist of settings that can be safely exposed or not, or only expose an object key ("safe"?), where the publicly safe settings can live, so there would no need to maintain any list.

As I said above, CSMs are user installed.
It doesn't make sense for SSMs to have access to all your settings but not CSMs.

[rubenwardy]

The CSM API and sandbox is designed for SSCSMs, which is why it's so restricted

[AFCMS]

The CSM API and sandbox is designed for SSCSMs, which is why it's so restricted

But why? CSM and SSCSM will never be executed in the same sandbox after all.

CSMs and SSCSM have completely different usecases.

[rubenwardy]

The current plan is to execute CPCSMs and SSCSMs in the same sandbox

Client-provided CSMs exist as a way to test this sandbox

Now, they could be given different sandboxes. But this goes back to what I said about needing to determine the future of SSCSM

[sfan5]

The current plan is to execute CPCSMs and SSCSMs in the same sandbox

This is not feasible.
To ease implementation during the transition period CPCSM could be automatically disabled when SSCSMs are in use but if the final goal is to have both then they must be in separate environments.

for more context see also this and following messages #10594 (comment)

[AFCMS]

I just added the client_lua_api.txt entry for minetest.settings

[AFCMS]

I just exposed minetest.setting_get_pos to the CSM API (by moving its definition to misc_helpers.lua)

[sfan5]

Approved for inclusion by coredev meeting.

[appgurueu]

Needs changes & isn't a possible close anymore

[AFCMS]

Rebased

[sfan5]

Tested

[SmallJoker]

Does not protect against writing of secure. settings; should be guarded by checkSettingSecurity.

[sfan5]

relevant code:

minetest/src/script/lua_api/l_settings.cpp

Lines 45 to 46 in d400a98

	if (ScriptApiSecurity::isSecure(L) && name.compare(0, 7, "secure.") == 0)
	throw LuaError("Attempted to set secure setting.");

[AFCMS]

Does not protect against writing of secure. settings; should be guarded by checkSettingSecurity.

But what is exactly to change?
Doesn't checkSettingSecurity works for both environments by checking for secure.?
Or is it a problem with ScriptApiSecurity::isSecure()?

[SmallJoker]

print(dump(minetest.settings:get("secure.http_mods")))
print(dump(minetest.settings:set("secure.http_mods", "test")))

The problem is that any CSM could write to secure. settings, even disabling mod security for local servers.
I did use those two lines with the PR to check whether writing these settings is possible, and minetest.conf did indeed save the "test" value.

[sfan5]

The fix is probably this but someone will have to verify it:

diff --git a/src/script/cpp_api/s_security.cpp b/src/script/cpp_api/s_security.cpp
--- a/src/script/cpp_api/s_security.cpp
+++ b/src/script/cpp_api/s_security.cpp
@@ -417,6 +417,12 @@ void ScriptApiSecurity::setLuaEnv(lua_State *L, int thread)
 
 bool ScriptApiSecurity::isSecure(lua_State *L)
 {
+#ifndef SERVER
+       auto script = ModApiBase::getScriptApiBase(L);
+       // CSM keeps no globals backup but is always secure
+       if (script->getType() == ScriptingType::Client)
+               return true;
+#endif
        lua_rawgeti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_GLOBALS_BACKUP);
        bool secure = !lua_isnil(L, -1);
        lua_pop(L, 1);

[AFCMS]

I go tomorrow on vacation and will not able to push/test something for about two weeks. 😓

You can edit the pull request if someone test the fix.

[SmallJoker]

Works.