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

This is a work in progress.

Prior to this commit, the get-title-tokens function was not operating as expected. It contained a bug where if the title of a resource were in single quotes, it would return all tokens prior to the COLON at the end of the title, rather than just the tokens from the beginning of the title. As well as this, the resource type the title belonged to wasn't being checked, resulting in resources other than execs having their titles checked for unsafe interpolations.

get_title_tokens now returns an array of hashes where each hash contains a title's tokens as an array and its resource_type. The resulting hash containing all of the titles is mapped to a new variable exec_title_tokens if its resource_type is an exec. Before titles are checked for unsafe interpolations, exec_title_tokens is checked incase it is empty.

Introduced the check_unsafe_title method which removes the need for nested loops (since exec_title_tokens will be an array of arrays of title tokens). This is similar to what is done to check commands for unsafe interpolations but with slight differences. There may be scope to combine check_unsafe_title and check_unsafe_interpolations.

The function interpolated? has been added to improve readability. Furthermore, this function checks if an interpolated string is present by checking if DQPRE or DQMID are present in the string, rather than checking if a :VARIABLE is present as this would flag non-interpolated variables.

In check_unsafe_interpolations(), when looking for command parameters, the iteration is skipped unless command, onlyif or unless are found in the exec but a check has been added to ensure these are of type :NAME before they are checked to prevent undesirable behaviour. E.g. if a token with value 'command' 'onlyif' or 'unless' is found, it was checked for unsafe interpolations as if a command parameter was found.

Author: GSPatton

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