Closed
Description
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
Activity
dimitry-ishenko commentedon Dec 14, 2024
Looking at
configng/tools/modules/functions/config_interface.sh
Lines 480 to 491 in 0a257fd
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 asconfigng/bin/armbian-config
Line 94 in 0a257fd
or
configng/bin/armbian-config
Line 106 in 0a257fd
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 commentedon Dec 14, 2024
Suggestions to resolve the bug:
When reporting an error, direct it at stderr and not stdout (which is why we have it 😉 ), eg:
Check for sub-shell exit code:
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:
Then the above code becomes something like:
and
dimitry-ishenko commentedon Dec 14, 2024
And, obviously,
sanitize_input
should allow dashes in the input.Tearran commentedon Dec 15, 2024
@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 commentedon Dec 15, 2024
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 commentedon Dec 15, 2024
The intent was precaution more than anything for when armbian-config was not strictly an admin tool as it is now.
dimitry-ishenko commentedon Dec 15, 2024
Maybe it should be removed for now, and if the need arises later, it can be resurrected. Just my 2 cents.
dimitry-ishenko commentedon Jan 29, 2025
@Tearran if you don't mind, I'll do a small PR that drops
sanitize_input
like you did in #326?I want to get it in, so I can finish my service wrapper PR (#323) and then Samba. Thanks.