Merge pull request #21 from puppetlabs/CONT-234_fix_bug_in_get-title-tokens_function

(CONT-234) Fix bug in get-title-tokens function

Author: chelnak

lib/puppet-lint/plugins/check_unsafe_interpolations.rb

@@ -1,19 +1,22 @@
 PuppetLint.new_check(:check_unsafe_interpolations) do
   COMMANDS = Array['command', 'onlyif', 'unless']
+  INTERPOLATED_STRINGS = Array[:DQPRE, :DQMID]
   def check
     # Gather any exec commands' resources into an array
     exec_resources = resource_indexes.map { |resource|
       resource_parameters = resource[:param_tokens].map(&:value)
       resource if resource[:type].value == 'exec' && !(COMMANDS & resource_parameters).empty?
     }.compact
 
-    # Get title tokens with modified puppet lint function
-    title_tokens_list = get_title_tokens
+    # Filter the list of titles returned by get_title_tokens for those contained in an exec
+    exec_title_tokens = get_title_tokens.select { |title|
+      title if title[:resource_type].value == 'exec'
+    }.compact
 
     # Iterate over title tokens and raise a warning if any are variables
-    title_tokens_list[:tokens].each do |token|
-      if token.type == :VARIABLE
-        notify_warning(token)
+    unless exec_title_tokens.empty?
+      exec_title_tokens.each do |title|
+        check_unsafe_title(title)
       end
     end
 
@@ -23,11 +26,18 @@ def check
     end
   end
 
-  # Iterates over tokens in a command and raises a warning if an interpolated variable is found
+  # Iterate over the tokens in a title and raise a warning if an interpolated variable is found
+  def check_unsafe_title(title)
+    title[:tokens].each do |token|
+      notify_warning(token.next_code_token) if 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|
-      # We are only interested in tokens from command onwards
-      next unless token.type == :NAME
+      # Skip iteration if token isn't a command of type :NAME
+      next unless COMMANDS.include?(token.value) && token.type == :NAME
       # Don't check the command if it is parameterised
       next if parameterised?(token)
 
@@ -54,9 +64,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
-      if current_token.type == :VARIABLE
-        rule_violations.append(current_token)
-      end
+      rule_violations.append(current_token.next_code_token) if interpolated?(current_token)
       current_token = current_token.next_token
     end
 
@@ -76,45 +84,48 @@ def parameterised?(token)
   # 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
   def get_title_tokens
-    tokens_array = []
-    result = {}
+    result = []
     tokens.each_index do |token_idx|
-      if tokens[token_idx].type == :COLON
-        # Check if title is an array
-        if tokens[token_idx - 1].type == :RBRACK
-          array_start_idx = tokens.rindex do |r|
-            r.type == :LBRACK
-          end
-          title_array_tokens = tokens[(array_start_idx + 1)..(token_idx - 2)]
-          tokens_array.concat(title_array_tokens.select do |token|
-            { STRING: true, NAME: true }.include?(token.type)
-          end)
-          result = {
-            tokens: tokens_array,
-            resource_type: tokens[array_start_idx].prev_code_token.prev_code_token
-          }
-        # Check if title is double quotes string
-        elsif tokens[token_idx - 1].type == :DQPOST
-          # Find index of the start of the title
-          title_start_idx = tokens.rindex do |r|
-            r.type == :DQPRE
-          end
-          result = {
-            # Title is tokens from :DQPRE to the index before :COLON
-            tokens: tokens[title_start_idx..(token_idx - 1)],
-            resource_type: tokens[title_start_idx].prev_code_token.prev_code_token
-          }
-        # Title is in single quotes
-        else
-          next_token = tokens[token_idx].next_code_token
-          tokens_array.concat(tokens[..token_idx - 1]) unless next_token.type == :LBRACE
-          result = {
-            tokens: tokens_array,
-            resource_type: tokens[token_idx - 1].prev_code_token.prev_code_token
-          }
+      next unless tokens[token_idx].type == :COLON
+      tokens_array = []
+      # Check if title is an array
+      if tokens[token_idx - 1].type == :RBRACK
+        array_start_idx = tokens.rindex do |r|
+          r.type == :LBRACK
+        end
+        title_array_tokens = tokens[(array_start_idx + 1)..(token_idx - 2)]
+        tokens_array.concat(title_array_tokens.select do |token|
+          { STRING: true, NAME: true }.include?(token.type)
+        end)
+        result << {
+          tokens: tokens_array,
+          resource_type: tokens[array_start_idx].prev_code_token.prev_code_token
+        }
+      # Check if title is double quotes string
+      elsif tokens[token_idx - 1].type == :DQPOST
+        # Find index of the start of the title
+        title_start_idx = tokens.rindex do |r|
+          r.type == :DQPRE
         end
+        result << {
+          # Title is tokens from :DQPRE to the index before :COLON
+          tokens: tokens[title_start_idx..(token_idx - 1)],
+          resource_type: tokens[title_start_idx].prev_code_token.prev_code_token
+        }
+      # Title is in single quotes
+      else
+        next_token = tokens[token_idx].next_code_token
+        tokens_array.concat([tokens[token_idx - 1]]) unless next_token.type == :LBRACE
+        result << {
+          tokens: tokens_array,
+          resource_type: tokens[token_idx - 1].prev_code_token.prev_code_token
+        }
       end
     end
     result
   end
+
+  def interpolated?(token)
+    INTERPOLATED_STRINGS.include?(token.type)
+  end
 end

spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb

@@ -120,5 +120,45 @@ class foo {
         expect(problems).to have(0).problems
       end
     end
+
+    context 'file resource' do
+      let(:code) do
+        <<-PUPPET
+        class foo {
+          file { '/etc/bar':
+            ensure  => file,
+            backup  => false,
+            content => $baz,
+          }
+        }
+        PUPPET
+      end
+
+      it 'detects zero problems' do
+        expect(problems).to have(0).problems
+      end
+    end
+
+    context 'file resource and an exec with unsafe interpolation in command' do
+      let(:code) do
+        <<-PUPPET
+        class foo {
+          file { '/etc/bar':
+            ensure  => file,
+            backup  => false,
+            content => $baz,
+          }
+
+          exec { 'qux':
+            command => "echo ${foo}",
+          }
+        }
+        PUPPET
+      end
+
+      it 'detects one problem' do
+        expect(problems).to have(1).problems
+      end
+    end
   end
 end