CSM restrictions: Make 'LOAD_CLIENT_MODS' disable loading of 'builtin'

[paramat]

Previously, when the CSM restriction 'LOAD_CLIENT_MODS' was used a
client was still able to add CSM code to 'builtin' to bypass that
restriction, because 'builtin' is not yet verified.

Until server-sent CSM and verifying of 'builtin' are complete, make
'LOAD_CLIENT_MODS' disable the loading of builtin.

Clarify code comments and messages to distinguish between client-side
modding and client-side scripting. 'Scripting' includes 'builtin',
'modding' does not.
///////////////////////////////

A CSM restriction exists ('LOAD_CLIENT_MODS' ) that stops a client loading CSMods, this is for servers that want to stop a client using any CSM code, whether it is generally considered harmful or not.
However this restriction does not stop the client loading the 'builtin' CSM code, and it's easy to add CSM code to builtin, and builtin is not yet checked for such added code.

[p-ouellette]

Why a separate restriction instead of disabling builtin/client loading when the 'LOAD_CLIENT_MODS' restriction is active? As is 'LOAD_CLIENT_MODS' is not very useful since you could just put all your mod code in builtin.

[paramat]

Good question and is something that has confused many times.

The 'LOAD_CLIENT_MODS' CSM restriction is for when a server sends CSMods to run on the client, but does not want the client to load it's own CSMods. In this situation 'builtin' needs loading because the server-sent mods require it.

This is why the verification of 'builtin' is necessary once we have SSCSM, because it needs to be loaded.

[p-ouellette]

Restrictions probably should not apply to server-sent code. This (and the issue you mentioned in #7995 (comment)) would be solved by having separate Lua states for CSM and CPCSM as @nerzhul suggested in #5393 (comment). I think that would make a lot of sense since the APIs would probably differ.

[nerzhul]

@pauloue i think SSCSM should have restrictions from client side too. If a client doesn't want server to use some API it should have the freedom to do it. And yeah server owners will not like this suggestion, but users should have the permission to block it, and being warned about gameplay issues.

[sfan5]

Since the builtin bypass is supposed to be eventually fixed, why not make CSM_RF_LOAD_CLIENT_MODS act like the new proposed flag in this pr?

[nerzhul]

I agree with @sfan5. I think mod loading & lua stack disable must be the same flag, there is no reason (currently) to have builtin alone.
Just rename the MOD loading flag to CSM_RF_DISABLE and handle it

[paramat]

sfan5 good idea, i was concerned about adding a new restriction that may later be unnecessary.
I think it should remain LOAD_CLIENT_MODS though as that is clear about it's actual purpose.

About the default: Because restrictions can be bypassed by placing code in builtin, and because builtin won't be checked in 5.0.0, the default for 5.0.0 will now have to be restriction enabled, disabled is not safe.
There are also many requests for CSM to be 'off' in every way in the defaults.

[nerzhul]

load mods is already off by default but extendid load_mods restriction to builtin too (which is a good idea) permit to ensure it's nice. I suggest you to do that, and we can close all the issues related to CSM not restricteable

[p-ouellette]

restrictions can be bypassed by placing code in builtin

How? I tested by using send_chat_message in builtin while the CHAT_MESSAGES restriction was active and the chat message was not sent. Or do you mean something else?

[paramat]

I mean things not covered by the 4 feature restrictions, but that should be restricted by 'LOAD_CLIENT_MODS'.

[paramat]

Updated as requested (mostly).
I kept the same CSM restriction name for simplicity and because it is still applicable in this temporary role.

The default for CSM restrictions has been changed to disable the client scripting environment. Considering CSM is unfinished and considering the requests of server owners this seems essential. The default should be the safest option.
Clients can easily request that server admin enable client scripting if they haven't already. This means it is enabled conciously as an 'opt-in' instead of allowed through ignorance.
Note that the clientside setting for enabling the client environment is also disabled by default.
Sorry nerzhul, i know you won't like that but considering the situation this is a reasonable compromise and only causes very minor inconvenience for clients.

I haven't tested this thoroughly yet.

[nerzhul]

I think this doesn't work because scripts are inited in the client constructor, then the server doesn't have sent the CSM restriction it wanted.

[BluebirdGreycoat]

If a client doesn't want server to use some API it should have the freedom to do it. And yeah server owners will not like this suggestion, but users should have the permission to block it, and being warned about gameplay issues.

I'm fine with this actually, it's not any different than disabling Javascript (either completely or selectively) from running in a browser, and there are plenty of legit reasons to do that.

[Ezhh]

The default for CSM restrictions has been changed to disable the client scripting environment

Yes please. I have tried suggesting this as a compromise / discussion point a few times.

(The thing about clients being able to decide what to accept from a server is common sense, and it shouldn't be assumed server owners will disagree with it.)

[nerzhul]

One point we need to care, the default on remote server, but it should be enabled for singleplayer as it's the local player server :)

@paramat let me code this this weekend, it's more tricky than just changing the condition because you are not in the correct workflow step

[Ezhh]

No issues with singleplayer default being to allow. It should just be disabled by default for multiplayer servers.

[paramat]

I think this doesn't work because scripts are inited in the client constructor, then the server doesn't have sent the CSM restriction it wanted.

Do you mean CSM restrictions have not yet been sent from the server when the client constructor is run? I did wonder about that. I'll test this, i may have to move the restriction action back into 'loadMods'.
I'm not too bothered about the client environment being created, the critical thing is not loading builtin.

And yes, CSM should be enabled by the singleplayer server, i forgot this.

[paramat]

Do you mean CSM restrictions have not yet been sent from the server when the client constructor is run?

Tested and they are not.
Updated and simplified, now the PR simply moves the 'LOAD_CLIENT_MODS' restriction check to before the loading of builtin.

@nerzhul i don't mind you adding to this work but i would like to merge this as i've put a lot of work into it. As it is now this PR seems fine, it just doesn't load builtin.
This PR leaves the client scripting environment created but it has to be because it is created before the CSM restrictions are received from the server. I think this is fine as a tempoaray state while we work on SSCSM.

The only thing needed now is to enable CSM in singleplayer. If i can work out how i'll add this to the PR.

[rubenwardy]

or 'builtin')

This is an implementation detail, and not really needed

[nerzhul]

your pr has been changed and doesn't disable builtin loading. As i said yes restriction are created when the client connect, then it's after cllient object creation, logical :)

[paramat]

nerzhul, it does:

	if (checkCSMRestrictionFlag(CSMRestrictionFlags::CSM_RF_LOAD_CLIENT_MODS)) {
		warningstream << "Client-provided mod loading is disabled by server." <<
			std::endl;
		return;
	}

	// Load builtin
	scanModIntoMemory(BUILTIN_MOD_NAME, getBuiltinLuaPath());
	m_script->loadModFromMemory(BUILTIN_MOD_NAME);

Found a bug so WIP.

[paramat]

Fixed. Fix is:

		// This line is needed because builtin is not loaded
		m_modding_enabled = false;

To avoid running code that relies on builtin being loaded.

Still need to apply according to singleplayer mode.

[nerzhul]

It's not a fix, it's a hack. You will hack the cut for the callbacks but builtin has already be loaded in the constructor. I think i will take the ownership of this PR to rework the CSM loading workflow

[paramat]

I think this doesn't work because scripts are inited in the client constructor,

You will hack the cut for the callbacks but builtin has already be loaded in the constructor

Where in the client constructor is builtin loaded? I can't see anything in Client::Client()

The loading of builtin is done in loadMods(), and until now the disabling of loading from builtin has been done in loadMods(), which is what my PR does. So why is it now not ok to disable the loading of builtin here? As you wrote, CSM restrictions are not present in the constructor.

I'm happy to let you do this if it's complex to the point of being beyond my ability, but please clearly explain so i understand.

[paramat]

However there are 2 things that my PR doesn't do which it probably should:

• Don't apply CSM restrictions in singleplayer.
• If possible, don't create the client scripting environment when the 'LOAD_CLIENT_MODS' restriction is in effect.
  So your help is certainly needed, either building on this PR or replacing it.

[nerzhul]

@paramat for the singleplayer let it outside of your pr we will handle that in another PR.
I should reverify where loadMods is called, i remembered builtin was in Client constructor but you say i'm wrong then maybe your pr is nearly valid :)

[paramat]

for the singleplayer let it outside of your pr we will handle that in another PR.

Ok. I'm happy for you to code that, i may not be able to do it well.

To clarify: What this PR does is temporary while we work on SSCSM. It just moves the 'LOAD_CLIENT_MODS' restriction to before the loading of builtin.
Once the verifying of builtin is complete it will be moved back, because the current code in master branch is apparently fine.
So i don't see a need for a complex rewrite.

This PR is no longer WIP.

[paramat]

Rebased and updated.

• Adds the recently added CSM restriction to settingtypes.txt.
• To help get this merged i have not changed the default setting of CSM restrictions to '63', i can do that in a separate PR as it is likely to be controversial.