Usage of non-local environment variables

[xathon]

Currently, the command-handling logic is not placed inside a function, making the usage of local environment variables impossible. This can cause previously set environment variables to get overwritten.

To mitigate the above issue, environmental variables get cleared before usage or directly overwritten. However, in some cases environment variables get appended, but not checked if they are empty before. This can cause external code to change the environmental variable at a specific time, causing injection of unwanted codes into the options of mscs/msctl. This can have multiple effects ranging from a confusing and bloated output of status to a spam of created folders when using start; however, as far as I have seen, there are no security implications, since variables are always enclosed in double quotes and thus not interpreted.

An example of the above:

mscs/msctl

Lines 3053 to 3058 in 8118d10

	status | show | status-json | show-json)
	# Figure out which worlds to show the status for.
	if [ "$#" -ge 1 ]; then
	for arg in "$@"; do
	if isWorldAvailable "$arg"; then
	WORLDS="$WORLDS $arg"

An example where it's done correctly (note L2668):

mscs/msctl

Lines 2667 to 2676 in 8118d10

	elif [ "$#" -ge 1 ]; then
	WORLDS=''
	for arg in "$@"; do
	if isWorldEnabled "$arg"; then
	if serverRunning "$arg"; then
	printf "Unable to start the requested worlds: world '$arg' already running.\n"
	exit 1
	else
	WORLDS="$WORLDS $arg"
	fi

[xathon]

#310 fixes the mitigation, however, rewriting the command handling in a function might still be useful.

[sandain]

Nice catch @xathon. I've looked over your PR, and it looks correct to me. Thanks for noting the issue and sending in a patch to correct it!

[sandain]

I don't have much time to work on this code, but would be happy to review patches if you are interested in moving the command handling into a function.

[xathon]

No worries! This was causing me all sorts of headaches, wondering how in the world I had injected an array into my status command 😄

I don't have much time to work on this code, but would be happy to review patches if you are interested in moving the command handling into a function.

I'll have a look. I did a short attempt earlier, which failed due to being unable to pass the correct variables to the handlers. It might be a bigger rewrite, but if I find some time I'll try my hand at it.