(maint) Codebase Hardening #2313
Conversation
| command => "/bin/rm ${confdir}/${_filename}", | ||
| unless => $verify_command, | ||
| command => $remove_command, | ||
| unless => [$verify_command], |
There was a problem hiding this comment.
unless accepts an array of arrays. What do you think about using shellsplit here? That way we can ensure that the command will be handled as expected by the exec provider.
For example:
[shellsplit($verify_command)]
There was a problem hiding this comment.
Bit iffy on doing this.
The verify command can be manually set and we know that splitting the command can cause errors in certain situations, i.e. the use of pipes
Not sure I like the idea of splitting the command when I don't know what it may be.
Probably other thinking it, but still
There was a problem hiding this comment.
Yes that is a good point.. but to me bolsters the fact that we should be guiding people to properly structure their commands.
unless takes an array of arrays. Each inner array is interpreted as a command by the provider. When multiple commands are passed each one needs to evaluate as true for the overall success of `unless.
Though the above would be a breaking change for some which adds to the complication.
There was a problem hiding this comment.
Could split the default command into an array. Then set verify_command to be: Variant[String, Array[String], Array[Array[String]]
It's a bit messy, but is technically what it should accept
There was a problem hiding this comment.
Then maybe remove the string in the next major release
There was a problem hiding this comment.
Could split the default command into an array. Then set verify_command to be: Variant[String, Array[String], Array[Array[String]]
It's a bit messy, but is technically what it should accept
I like this, but would it make sense to (also) introduce a Stdlib::Exec::Unless data type? While we couldn't use it immediately, it does signal to the user how it should be used. Just like there's Stdlib::Ensure::*.
There was a problem hiding this comment.
also rebased onto main
There was a problem hiding this comment.
and squashed down all my corrective changes into one commit
There was a problem hiding this comment.
@ekohl that would be nice yes. I'm a fan of anything we can do to help users on their journey.
|
Just a thought: can we develop a puppet-lint plugin that detects string interpolation in |
Yes! This is actually something that we were planning when we had our team meet up last week. Should be a nice piece of work for someone on the team to run with. |
david22swan
force-pushed
the
maint/codebase_hardening
branch
3 times, most recently
from
dbba2ae
to
a14c834
Compare
david22swan
force-pushed
the
maint/codebase_hardening
branch
3 times, most recently
from
6d5815f
to
a3eb11f
Compare
There was a problem hiding this comment.
Looking great @david22swan
Your branch could do with a rebase before merge though please 👍
Changes made to ensure that no malformed commands are passed through to the system.
- Fixes made to spec tests - onlyif commands corrected - remove unnecessary shell_escapes - variable used in place of replicated assignment - $_file_path neatened
The unless variable in an exec manifest, which $verify_command is directly passed too, accepts either an Array or and Array of Arrays as input. Updating the variables type and default values to match this. In the case where it is passed a String it proceeds to wrap it in an Array and then treat it as such.
david22swan
force-pushed
the
maint/codebase_hardening
branch
from
a3eb11f
to
a565ea9
Compare
Changes made to ensure that no malformed commands are passed through to the system.
Includes an update the the $verify_command variable type
The unless variable in an exec manifest, which $verify_command is directly passed too, accepts either an Array or and Array of Arrays as input.
Updating the variables type and default values to match this.
In the case where it is passed a String it proceeds to wrap it in an Array and then treat it as such.