Skip to content

Commit

Permalink
Fix safe interpolation detection
Browse files Browse the repository at this point in the history
Any variable interpolation is currently reported as unsafe.

The stdlib feature a `stdlib::shell_escape()` function (formerly
`shell_escape()`) that escape the string passed as parameter.  In such a
case, an unsafe interpolation should not be detected.

Add detection of such escaped string and do not report an error in this
case.  `stdlib::shell_escape()` must be the last function called in the
interpolation for it to not be reported as unsafe.
  • Loading branch information
smortex committed Apr 8, 2024
1 parent 7b7b42d commit 80a991d
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 6 deletions.
56 changes: 50 additions & 6 deletions lib/puppet-lint/plugins/check_unsafe_interpolations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ def check
end
end

# Iterate over the tokens in a title and raise a warning if an interpolated variable is found
# Iterate over the tokens in a title and raise a warning if an unsafe interpolated variable is found
def check_unsafe_title(title)
title.each do |token|
notify_warning(token.next_code_token) if interpolated?(token)
notify_warning(token.next_code_token) if unsafe_interpolated?(token)
end
end

Expand Down Expand Up @@ -60,7 +60,7 @@ def check_command(token)
# Iterate through tokens in command
while current_token.type != :NEWLINE
# Check if token is a varibale and if it is parameterised
rule_violations.append(current_token.next_code_token) if interpolated?(current_token)
rule_violations.append(current_token.next_code_token) if unsafe_interpolated?(current_token)
current_token = current_token.next_token
end

Expand All @@ -78,7 +78,7 @@ def parameterised?(token)
end

# This function is a replacement for puppet_lint's title_tokens function which assumes titles have single quotes
# This function adds a check for titles in double quotes where there could be interpolated variables
# This function adds a check for titles in double quotes where there could be unsafe interpolated variables
def get_exec_titles
result = []
tokens.each_index do |token_idx|
Expand Down Expand Up @@ -114,8 +114,52 @@ def get_exec_titles
result
end

def interpolated?(token)
INTERPOLATED_STRINGS.include?(token.type)
def find_closing_brack(token)
while (token = token.next_code_token)
case token.type
when :RBRACK
return token
when :LBRACK
token = find_closing_brack(token)
end
end
raise 'not reached'
end

def find_closing_paren(token)
while (token = token.next_code_token)
case token.type
when :RPAREN
return token
when :LPAREN
token = find_closing_paren(token)
end
end
raise 'not reached'
end

def unsafe_interpolated?(token)
# XXX: Since stdlib 9.0.0 'shell_escape' is deprecated in favor or 'stdlib::shell_escape'
# When the shell_escape function is removed from stdlib, we want to remove it bellow.
return false unless INTERPOLATED_STRINGS.include?(token.type)

token = token.next_code_token

if token.type == :FUNCTION_NAME && ['shell_escape', 'stdlib::shell_escape'].include?(token.value)
token = token.next_code_token
token = find_closing_paren(token).next_code_token if token.type == :LPAREN
return ![:DQPOST, :DQMID].include?(token.type)
elsif token.type == :VARIABLE
token = token.next_code_token
token = find_closing_brack(token).next_code_token if token.type == :LBRACK
if token.type == :DOT && [:NAME, :FUNCTION_NAME].include?(token.next_code_token.type) && ['shell_escape', 'stdlib::shell_escape'].include?(token.next_code_token.value)
token = token.next_code_token.next_code_token
token = find_closing_paren(token).next_code_token if token.type == :LPAREN
return ![:DQPOST, :DQMID].include?(token.type)
end
end

true
end

# Finds the index offset of the next instance of `value` in `tokens_slice` from the original index
Expand Down
38 changes: 38 additions & 0 deletions spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,44 @@ class foo {
end
end

context 'exec with shell escaped string in command' do
let(:code) do
<<-PUPPET
class foo {
exec { 'bar':
command => "echo ${foo.stdlib::shell_escape} ${bar.stdlib::shell_escape()}",
onlyif => "${bar[1].stdlib::shell_escape}",
unless => "[ -x ${stdlib::shell_escape(reticulate($baz))} ]",
}
}
PUPPET
end

it 'detects zero problems' do
expect(problems).to have(0).problems
end
end

context 'exec with post-processed shell escaped string in command' do
let(:code) do
<<-PUPPET
class foo {
exec { 'bar':
command => "echo ${reticulate(foo.stdlib::shell_escape)} ${bar.stdlib::shell_escape().reticulate}",
onlyif => "${bar[1].stdlib::shell_escape.reticulate()}",
unless => "[ -x ${reticulate(stdlib::shell_escape($baz))} ]",
}
}
PUPPET
end

it 'detects zero problems' do
expect(problems).to have(4).problems
end
end

context 'exec that has an array of args in command' do
let(:code) do
<<-PUPPET
Expand Down

0 comments on commit 80a991d

Please sign in to comment.