[Bug]: sanitize_input does not work as expected

[dimitry-ishenko]

What happened?

While testing PR #320 I would get the following error:

admin@armbian:~/configng$ bin/armbian-config --api pkg_install vim-airline

Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
E: Unable to locate package Invalid argument: vim-airline

Upon further testing, any package containing dash (-) in the name would produce similar error.

It turns out the error is generated by the sanitize_input function which does not allow dashes. However, the pipeline doesn't seem to work as expected, since instead of just reporting the error "Invalid argument: vim-airline," this output is passed further down the pipeline.

In the end the pkg_install function tries to install a package called Invalid argument: vim-airline, which results in a confusing error.

How to reproduce?

See above.

On which host OS are you running the build script and observing this problem?

Ubuntu 24.04 Noble

Code of Conduct

• I agree to follow this project's Code of Conduct

[dimitry-ishenko]

Looking at

configng/tools/modules/functions/config_interface.sh

Lines 480 to 491 in 0a257fd

	sanitize_input() {
	local sanitized_input=()
	for arg in "$@"; do
	if [[ $arg =~ ^[a-zA-Z0-9_=]+$ ]]; then
	sanitized_input+=("$arg")
	else
	echo "Invalid argument: $arg"
	exit 1
	fi
	done
	echo "${sanitized_input[@]}"
	}

My guess is that the intended behavior was to terminate program execution and print out an error message. However, in most cases sanitize_input will be used in a sub-shell such as

configng/bin/armbian-config

Line 94 in 0a257fd

args=$(sanitize_input "$@")

or

configng/bin/armbian-config

Line 106 in 0a257fd

args=$(sanitize_input "$@")

This will terminate the sub-shell with exit code 1, but unless checked in the super-shell, will continue normal execution, which is what's happening here.

[dimitry-ishenko]

Suggestions to resolve the bug:

• When reporting an error, direct it at stderr and not stdout (which is why we have it 😉 ), eg:

  echo "Invalid argument: $arg" >&2
  exit 1
  

• Check for sub-shell exit code:

  args=$(sanitize_input "$@") || exit 1
  

• We can also leverage set -e option, which will automatically abort script execution on error.

• To add consistency and reduce boiler-plate we could introduce commands such as:

  error() { echo "$@" >&2; }
  die() { error "$@"; exit 1; }
  

  Then the above code becomes something like:

  sanitize_input() {
      ...
          die "Invalid argument: $arg"
      ...
  }

  and

  args=$(sanitize_input "$@") || die

[dimitry-ishenko]

And, obviously, sanitize_input should allow dashes in the input.

[Tearran]

@dimitry-ishenko Sanitise input contribution was aimed primarily at removing escape sequences.
This is one of the placeholder implementations that is incomplete and needs refinement or replacement. Another example you are familiar with is process_input.

This has been a place for expanding and learning: If a may ask :) Are you saying that while the implementation is flawed (BUG), the concept is not, and sanitize_input should handle input validation more comprehensively rather than be removed?

For later implementation and though.
If we validate, should we not include a manner to use the database to actually verify inputs?

[dimitry-ishenko]

Are you saying that while the implementation is flawed (BUG), the concept is not, and sanitize_input should handle input validation more comprehensively rather than be removed?

It's hard to tell without knowing the original intent, but I would say it should be removed... I am not a security specialist, but I don't see what problem it's trying to solve... Is there a case of using an escape sequence to compromise the system?

Or, if there is a valid need for it, then maybe it can be modified to use a blacklist to detect escape char in the input instead.

[Tearran]

The intent was precaution more than anything for when armbian-config was not strictly an admin tool as it is now.

[dimitry-ishenko]

Maybe it should be removed for now, and if the need arises later, it can be resurrected. Just my 2 cents.