New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CSM] Add flavour limits controlled by server #5930

Merged
merged 13 commits into from Jul 18, 2017

Conversation

@nerzhul
Member

nerzhul commented Jun 6, 2017

Server send flavour limits to client permitting to disable or limit some Lua calls

  • CSM_FL_LOOKUP_NODES: Limit node lookups for 8 nodes in player range
  • CSM_FL_CHAT_MESSAGES: Disable chat message sending from CSM
  • CSM_FL_READ_ITEMDEFS: Disable itemdef lookups
  • CSM_FL_READ_NODEDEFS: Disable nodedef lookups

Remove get_node_or_nil, get_node now have the get_node_or_nil behaviour

Add a parameter (server-side) to set node lookup limit (default 8) when enable LOOKUP_NODE Flavour

Show outdated Hide outdated src/server.h
@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 6, 2017

Member
Member

nerzhul commented Jun 6, 2017

@tenplus1

This comment has been minimized.

Show comment
Hide comment
@tenplus1

tenplus1 Jun 6, 2017

Contributor

+1 we need a way for servers to disable csm elements or csm completely.

Contributor

tenplus1 commented Jun 6, 2017

+1 we need a way for servers to disable csm elements or csm completely.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 6, 2017

Member

Thanks.
I strongly feel all node getters should be disableable, no range limiting, no exceptions.
Also, an option for a server to block all client-provided clientmods if it wishes, so that all clients have the same experience even if a certain feature is harmless and not usable for cheating. Opinions about what can be unregulated will differ.
Generally, servers should continue to have absolute power over CSM if they wish.

(Some players are stating that server owners are mean and will abuse this, not at all, server owners generally care about the fun of their players and are usually more mature and better behaved than the players.)

Member

paramat commented Jun 6, 2017

Thanks.
I strongly feel all node getters should be disableable, no range limiting, no exceptions.
Also, an option for a server to block all client-provided clientmods if it wishes, so that all clients have the same experience even if a certain feature is harmless and not usable for cheating. Opinions about what can be unregulated will differ.
Generally, servers should continue to have absolute power over CSM if they wish.

(Some players are stating that server owners are mean and will abuse this, not at all, server owners generally care about the fun of their players and are usually more mature and better behaved than the players.)

@red-001

This comment has been minimized.

Show comment
Hide comment
@red-001

red-001 Jun 6, 2017

Contributor

+1 to the idea but the implementation could be improved. It would be best to have a client-sided list of dangerous functions since that would allow adding new functions that could be used for cheating without having to worry about older servers not knowing about them.
A way for servers to whitelist or blacklist extra functions could of course be added.

Contributor

red-001 commented Jun 6, 2017

+1 to the idea but the implementation could be improved. It would be best to have a client-sided list of dangerous functions since that would allow adding new functions that could be used for cheating without having to worry about older servers not knowing about them.
A way for servers to whitelist or blacklist extra functions could of course be added.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 6, 2017

Member

@red-001 it's why flavour is nice because you define some sets or flavours and you can apply it on many functions without needed both side support

Member

nerzhul commented Jun 6, 2017

@red-001 it's why flavour is nice because you define some sets or flavours and you can apply it on many functions without needed both side support

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 8, 2017

Member

I added limits for reading nodedefs and itemdefs

Member

nerzhul commented Jun 8, 2017

I added limits for reading nodedefs and itemdefs

@bigfoot547

This comment has been minimized.

Show comment
Hide comment
@bigfoot547

bigfoot547 Jun 8, 2017

Contributor
Contributor

bigfoot547 commented Jun 8, 2017

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 9, 2017

Member

someone on IRC said his game was based on recipe discovery, now we don't have recipes but node and item defs, i just thought some games wants to limit this to prevent discovery of new features in a specific gameplay, i just prevented the future question on those APIs. Don't forget flavour will be disabled and should be enabled by server owners

Member

nerzhul commented Jun 9, 2017

someone on IRC said his game was based on recipe discovery, now we don't have recipes but node and item defs, i just thought some games wants to limit this to prevent discovery of new features in a specific gameplay, i just prevented the future question on those APIs. Don't forget flavour will be disabled and should be enabled by server owners

@red-001

This comment has been minimized.

Show comment
Hide comment
@red-001

red-001 Jun 9, 2017

Contributor

@nerzhul when you actually implement the chat message sending restriction could you add a heavily rate limited exception for chat commands?

Contributor

red-001 commented Jun 9, 2017

@nerzhul when you actually implement the chat message sending restriction could you add a heavily rate limited exception for chat commands?

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 9, 2017

Member

I understand that old clients will be able to get around these restrictions, so a server wishing to enforce restrictions will have to disallow old clients.

Does this PR include a way to disalble all clientmod functions?
For example, many functions will of course be considered completely 'harmless', but a server may wish that all its players have the same game experience and so wish to disallow harmless functions also.

Member

paramat commented Jun 9, 2017

I understand that old clients will be able to get around these restrictions, so a server wishing to enforce restrictions will have to disallow old clients.

Does this PR include a way to disalble all clientmod functions?
For example, many functions will of course be considered completely 'harmless', but a server may wish that all its players have the same game experience and so wish to disallow harmless functions also.

@red-001

This comment has been minimized.

Show comment
Hide comment
@red-001

red-001 Jun 9, 2017

Contributor

I do believe it has already been pointed out that disabling all functions isn't possible since we need some of them for bultin. If you disable all functions players are just not going to update if they want to use CSM which will defeat the point of this or at least increase the period of time during which people will just use old clients.

Contributor

red-001 commented Jun 9, 2017

I do believe it has already been pointed out that disabling all functions isn't possible since we need some of them for bultin. If you disable all functions players are just not going to update if they want to use CSM which will defeat the point of this or at least increase the period of time during which people will just use old clients.

@red-001

This comment has been minimized.

Show comment
Hide comment
@red-001

red-001 Jun 9, 2017

Contributor

Plus safe functions will only affect the client so by that logic we should remove texture-packs.

Contributor

red-001 commented Jun 9, 2017

Plus safe functions will only affect the client so by that logic we should remove texture-packs.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 9, 2017

Member

the idea is to control risky features, which can be part of a gameplay

Member

nerzhul commented Jun 9, 2017

the idea is to control risky features, which can be part of a gameplay

@nerzhul nerzhul added this to Feature PR in Minetest 5.0.0 Jun 10, 2017

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 12, 2017

Member

I do believe it has already been pointed out that disabling all functions isn't possible since we need some of them for bultin.

Ok, so i mean that functions that are not essential for builtin, but are not also considered harmful for a server, should still be disableable.

If you disable all functions players are just not going to update if they want to use CSM

I'm not proposing disabling all functions, 'flavours' are options for servers, only some would disable all functions including the non-harmful ones because servers care about their popularity. There is also singleplayer.
I feel that functions that are not obviously harmful should still be disableable, as a server may wish that all clients have a similar client experience.

by that logic we should remove texture-packs.

No, by that logic we should add a way for a server to disable client-chosen texture packs. This is a reasonable suggestion because a server may rely on a particular texture pack to work properly and look right.

Your arguments here are ridiculous and seem biased in order to resist the requests for restrictions, you have done this quite often during the issue discussion. By reacting this way you run the risk of losing people's trust that you are genuinely trying to fix this issue instead of misleading people.
I'm not sure i trust you with CSM restrictions, nerzhul seems more reasonable and trustworthy.
EDIT: I take that back after recent behaviour.

Member

paramat commented Jun 12, 2017

I do believe it has already been pointed out that disabling all functions isn't possible since we need some of them for bultin.

Ok, so i mean that functions that are not essential for builtin, but are not also considered harmful for a server, should still be disableable.

If you disable all functions players are just not going to update if they want to use CSM

I'm not proposing disabling all functions, 'flavours' are options for servers, only some would disable all functions including the non-harmful ones because servers care about their popularity. There is also singleplayer.
I feel that functions that are not obviously harmful should still be disableable, as a server may wish that all clients have a similar client experience.

by that logic we should remove texture-packs.

No, by that logic we should add a way for a server to disable client-chosen texture packs. This is a reasonable suggestion because a server may rely on a particular texture pack to work properly and look right.

Your arguments here are ridiculous and seem biased in order to resist the requests for restrictions, you have done this quite often during the issue discussion. By reacting this way you run the risk of losing people's trust that you are genuinely trying to fix this issue instead of misleading people.
I'm not sure i trust you with CSM restrictions, nerzhul seems more reasonable and trustworthy.
EDIT: I take that back after recent behaviour.

@red-001

This comment has been minimized.

Show comment
Hide comment
@red-001

red-001 Jun 12, 2017

Contributor

Alright in that case I suggest we add a way to force texture and sound packs in the same release.

Contributor

red-001 commented Jun 12, 2017

Alright in that case I suggest we add a way to force texture and sound packs in the same release.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 12, 2017

Member

Issue for my request #5974

Member

paramat commented Jun 12, 2017

Issue for my request #5974

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 12, 2017

Member

'send_chat_message' needs to be added to those functions considered potentially harmful, so included here as something disableable, reasons here #5915 (comment)

Member

paramat commented Jun 12, 2017

'send_chat_message' needs to be added to those functions considered potentially harmful, so included here as something disableable, reasons here #5915 (comment)

@red-001

This comment has been minimized.

Show comment
Hide comment
@red-001

red-001 Jun 12, 2017

Contributor

@paramat no-one is disputing that anymore.

Contributor

red-001 commented Jun 12, 2017

@paramat no-one is disputing that anymore.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 12, 2017

Member

@paramat just read the PR instead of complaining, the case is already in the scope.

Member

nerzhul commented Jun 12, 2017

@paramat just read the PR instead of complaining, the case is already in the scope.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 13, 2017

Member

'send_chat_message' needs to be added to those functions considered potentially harmful,

Ok, sorry i missed that.

Member

paramat commented Jun 13, 2017

'send_chat_message' needs to be added to those functions considered potentially harmful,

Ok, sorry i missed that.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 13, 2017

Member

Looks like NONE and ALL refer to the 4 restrictions only.

Member

paramat commented Jun 13, 2017

Looks like NONE and ALL refer to the 4 restrictions only.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 13, 2017

Member

yes it's a byteflag, you can combine the values for filtering using a single integer

Member

nerzhul commented Jun 13, 2017

yes it's a byteflag, you can combine the values for filtering using a single integer

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 15, 2017

Member
+	// CSM flavour limits byteflag
+	u64 m_csm_flavour_limits = CSMFlavourLimit::CSM_FL_NONE;

The default is currently no restrictions.
👎
EDIT: Sorry, i should add my request to be more constructive, please can all restrictions be enabled by default?

Member

paramat commented Jun 15, 2017

+	// CSM flavour limits byteflag
+	u64 m_csm_flavour_limits = CSMFlavourLimit::CSM_FL_NONE;

The default is currently no restrictions.
👎
EDIT: Sorry, i should add my request to be more constructive, please can all restrictions be enabled by default?

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 15, 2017

Member

yeah and i should rebase, default is currently choosen default and can be change when someone will help me to find the best restriction to nodes, if everybody prefers to complains and doesn't permit to build this specific solution the PR will remain as is.

Member

nerzhul commented Jun 15, 2017

yeah and i should rebase, default is currently choosen default and can be change when someone will help me to find the best restriction to nodes, if everybody prefers to complains and doesn't permit to build this specific solution the PR will remain as is.

@nerzhul nerzhul removed the Controversial label Jun 15, 2017

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 15, 2017

Member

Note: PR is not controversial, you just do a review man. Stop that label for everything.

Member

nerzhul commented Jun 15, 2017

Note: PR is not controversial, you just do a review man. Stop that label for everything.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 15, 2017

Member

Ok, i should have added, please can the default be all restrictions enabled?
I am reviewing and am suggesting a better default.

Member

paramat commented Jun 15, 2017

Ok, i should have added, please can the default be all restrictions enabled?
I am reviewing and am suggesting a better default.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 15, 2017

Member

if restrctions should be enable i suggest to use chat message and node limits

Member

nerzhul commented Jun 15, 2017

if restrctions should be enable i suggest to use chat message and node limits

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 15, 2017

Member

default is currently choosen default and can be change when someone will help me to find the best restriction to nodes

It's obvious all potentially problematic functions should be restricted by default, otherwise those functions would not be restrictable.
You can see from the issue that server owners want as much restriction as possible.

It is common usage and completely acceptable for a dev to -1 if that dev disapproves of a PR in it's current state, you are over-reacting. I am reviewing and stating my unhappiness with something, that is normal review and is not 'complaning' that is to be dismissed as troublemaking.

Those who cannot code C++ still have a right to complain, otherwise we would ignore most players and server owners, please do not state that those who cannot work on the code have no right to complain, you have done this to me several times.

Depending on how this PR changes this is potentially controversial, the issue certainly is.

Member

paramat commented Jun 15, 2017

default is currently choosen default and can be change when someone will help me to find the best restriction to nodes

It's obvious all potentially problematic functions should be restricted by default, otherwise those functions would not be restrictable.
You can see from the issue that server owners want as much restriction as possible.

It is common usage and completely acceptable for a dev to -1 if that dev disapproves of a PR in it's current state, you are over-reacting. I am reviewing and stating my unhappiness with something, that is normal review and is not 'complaning' that is to be dismissed as troublemaking.

Those who cannot code C++ still have a right to complain, otherwise we would ignore most players and server owners, please do not state that those who cannot work on the code have no right to complain, you have done this to me several times.

Depending on how this PR changes this is potentially controversial, the issue certainly is.

@tenplus1

This comment has been minimized.

Show comment
Hide comment
@tenplus1

tenplus1 Jun 15, 2017

Contributor

Personally I would disable CSM by default as well as having a feature to disable from server.

Contributor

tenplus1 commented Jun 15, 2017

Personally I would disable CSM by default as well as having a feature to disable from server.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jun 15, 2017

Member

Another request:
As i understand it, some functions are essential for CSM to work, in the following i am not referring to these.
I propose that the CSM 'flavours' restrictions allow all (non-essential) CSM functions to be disabled even if they are currently not considered potentially harmful to a server.

It's not easy to judge which functions are potentially harmful to a server. In the future it is likely that a function currently considered harmless will be used in a way we didn't forsee. In this situation a server will have no way to avoid the disruption and would have to wait for a PR that adds that function to those considered potentially harmful, this would take a few days.

If this is done, my request for the default flavour setting would be either disable all functions or just the 4 function types known to be potentilly harmful.

Member

paramat commented Jun 15, 2017

Another request:
As i understand it, some functions are essential for CSM to work, in the following i am not referring to these.
I propose that the CSM 'flavours' restrictions allow all (non-essential) CSM functions to be disabled even if they are currently not considered potentially harmful to a server.

It's not easy to judge which functions are potentially harmful to a server. In the future it is likely that a function currently considered harmless will be used in a way we didn't forsee. In this situation a server will have no way to avoid the disruption and would have to wait for a PR that adds that function to those considered potentially harmful, this would take a few days.

If this is done, my request for the default flavour setting would be either disable all functions or just the 4 function types known to be potentilly harmful.

Show outdated Hide outdated src/client.h
@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jul 16, 2017

Member

protocol bump done

Member

nerzhul commented Jul 16, 2017

protocol bump done

Show outdated Hide outdated src/server.cpp
// CSM flavour limits byteflag
u64 m_csm_flavour_limits = CSMFlavourLimit::CSM_FL_NONE;
u32 m_csm_noderange_limit = 8;

This comment has been minimized.

@SmallJoker

SmallJoker Jul 16, 2017

Member

Redefinition. They're set in the constructor.

@SmallJoker

SmallJoker Jul 16, 2017

Member

Redefinition. They're set in the constructor.

This comment has been minimized.

@nerzhul

nerzhul Jul 16, 2017

Member

exact but this permit to ensure on different constructor we have a default value set

@nerzhul

nerzhul Jul 16, 2017

Member

exact but this permit to ensure on different constructor we have a default value set

Show outdated Hide outdated src/client.cpp

nerzhul added some commits Jun 6, 2017

[WIP] [CSM] Add flavour limits controlled by server
Server send flavour limits to client permitting to disable or limit some Lua calls
Merge get_node_or_nil into get_node.
Sending fake node doesn't make sense in CSM, just return nil if node is not available for any reason
@rubenwardy

How about

getClient(L)->checkCSMFlavour(CSMFlavourLimit::CSM_FL_CHAT_MESSAGES)

instead of:

getClient(L)->getCSMFlavourLimits() & CSMFlavourLimit::CSM_FL_CHAT_MESSAGES

it could be inline, but makes the code more readable.

I'm also not convinced about mixing CSM things in Client methods such as getNode, however I don't mind if it's made more explicit (ie: renaming to csm_getNode)

Show outdated Hide outdated builtin/settingtypes.txt
Show outdated Hide outdated doc/client_lua_api.md
* @param p
* @param is_valid_position
* @return
*/
MapNode Client::getNode(v3s16 p, bool *is_valid_position)

This comment has been minimized.

@rubenwardy

rubenwardy Jul 16, 2017

Member

csm_getNode ?
Not sure that this is the right place for the limits

@rubenwardy

rubenwardy Jul 16, 2017

Member

csm_getNode ?
Not sure that this is the right place for the limits

This comment has been minimized.

@nerzhul

nerzhul Jul 16, 2017

Member

it has as right place as privileges :), and not we cannot specialize a function to a unrelated object feature

@nerzhul

nerzhul Jul 16, 2017

Member

it has as right place as privileges :), and not we cannot specialize a function to a unrelated object feature

Show outdated Hide outdated src/script/lua_api/l_client.cpp
Show outdated Hide outdated src/script/lua_api/l_env.cpp
@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jul 17, 2017

Member

Just noticed 'sound play' is in the CSM API, does that need a control too as it affects the server? maybe it could be soft-limited by to sounds only the local player hears?

Member

paramat commented Jul 17, 2017

Just noticed 'sound play' is in the CSM API, does that need a control too as it affects the server? maybe it could be soft-limited by to sounds only the local player hears?

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jul 17, 2017

Member

sound play permit CSM mod to play a sound, no need for server for that. It permits, for example to have sounds on formspec buttons without waiting server to send event for that, then reduce server load

Member

nerzhul commented Jul 17, 2017

sound play permit CSM mod to play a sound, no need for server for that. It permits, for example to have sounds on formspec buttons without waiting server to send event for that, then reduce server load

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jul 17, 2017

Member

So 'sound play' doesn't play a sound for other players?

Member

paramat commented Jul 17, 2017

So 'sound play' doesn't play a sound for other players?

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jul 17, 2017

Member

absolutely no, CSM is for triggering local actions, not remote actions (chat is considered as a local action). You will play local sound

Member

nerzhul commented Jul 17, 2017

absolutely no, CSM is for triggering local actions, not remote actions (chat is considered as a local action). You will play local sound

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jul 17, 2017

Member

Good :]

Member

paramat commented Jul 17, 2017

Good :]

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jul 18, 2017

Member

i will merge this evening if no more things to change. It's time to push this and close all CSM controversy

Member

nerzhul commented Jul 18, 2017

i will merge this evening if no more things to change. It's time to push this and close all CSM controversy

@nerzhul nerzhul merged commit 79f19b8 into minetest:master Jul 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Eurybot added a commit to MT-Eurythmia/minetest that referenced this pull request Jul 25, 2017

[CSM] Add flavour limits controlled by server (#5930)
* [CSM] Add flavour limits controlled by server

Server send flavour limits to client permitting to disable or limit some Lua calls

* Add limits for reading nodedefs and itemdefs

* flavour: Add lookup node limits

* Merge get_node_or_nil into get_node.

Sending fake node doesn't make sense in CSM, just return nil if node is not available for any reason

* Add node range customization when noderange flavour is enabled (default 8 nodes)

* Limit nodes range & disable chat message sending by default

* Bump protocol version

@nerzhul nerzhul moved this from Feature PR to Done in Minetest 5.0.0 Aug 16, 2017

@Lejo1

This comment has been minimized.

Show comment
Hide comment
@Lejo1

Lejo1 Jun 19, 2018

Does this include Metadatas?
They should also have a flavour limit.

Lejo1 commented Jun 19, 2018

Does this include Metadatas?
They should also have a flavour limit.

@SmallJoker

This comment has been minimized.

Show comment
Hide comment
@SmallJoker

SmallJoker Jun 19, 2018

Member

@Lejo1 What would you like to limit there? Metadata fields can already be set as private by the server, so I don't see a need for liming that too.

Member

SmallJoker commented Jun 19, 2018

@Lejo1 What would you like to limit there? Metadata fields can already be set as private by the server, so I don't see a need for liming that too.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 19, 2018

Member

meta limit is already limited by using private meta server side. Public meta are meant to be accessed by the client.

Member

nerzhul commented Jun 19, 2018

meta limit is already limited by using private meta server side. Public meta are meant to be accessed by the client.

@nerzhul nerzhul deleted the nerzhul:csm_flavour_blacklist branch Jun 19, 2018

@Lejo1

This comment has been minimized.

Show comment
Hide comment
@Lejo1

Lejo1 Jun 20, 2018

The nodemeta.
Its easily possible to check in a big radius for a chest/furnace using the meta.

Lejo1 commented Jun 20, 2018

The nodemeta.
Its easily possible to check in a big radius for a chest/furnace using the meta.

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jun 20, 2018

Member

it's not the flavour responsibility. It's mod owner responibility to make meta private

Member

nerzhul commented Jun 20, 2018

it's not the flavour responsibility. It's mod owner responibility to make meta private

@paramat paramat moved this from In Progress to Done in Server-sent Client Side Modding Jun 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment