Merge pull request #48 from smortex/fix-safe-interpolation-detection

Fix safe interpolation detection

Author: david22swan

lib/puppet-lint/plugins/check_unsafe_interpolations.rb

@@ -22,18 +22,18 @@ 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
 
   # Iterates over an exec resource and if a command, onlyif or unless paramter is found, it is checked for unsafe interpolations
   def check_unsafe_interpolations(command_resources)
     command_resources[:tokens].each do |token|
       # Skip iteration if token isn't a command of type :NAME
-      next unless COMMANDS.include?(token.value) && token.type == :NAME
+      next unless COMMANDS.include?(token.value) && (token.type == :NAME || token.type == :UNLESS)
       # Don't check the command if it is parameterised
       next if parameterised?(token)
 
@@ -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
 
@@ -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|
@@ -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

spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb

@@ -33,19 +33,23 @@ class foo {
 
           exec { 'bar':
             command => "echo ${foo} ${bar}",
+            onlyif  => "${baz}",
+            unless  => "${bazz}",
           }
 
         }
         PUPPET
       end
 
       it 'detects multiple unsafe exec command arguments' do
-        expect(problems).to have(2).problems
+        expect(problems).to have(4).problems
       end
 
       it 'creates two warnings' do
         expect(problems).to contain_warning(msg)
-        expect(problems).to contain_warning(msg)
+        expect(problems).to contain_warning("unsafe interpolation of variable 'bar' in exec command")
+        expect(problems).to contain_warning("unsafe interpolation of variable 'baz' in exec command")
+        expect(problems).to contain_warning("unsafe interpolation of variable 'bazz' in exec command")
       end
     end
 
@@ -87,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

spec/spec_helper.rb

@@ -1,3 +1,4 @@
 require 'puppet-lint'
+require 'rspec/collection_matchers'
 
 PuppetLint::Plugins.load_spec_helper