From 73529284fa82cda92c8a13f74721e9623f0f0a3b Mon Sep 17 00:00:00 2001 From: Gavin Patton Date: Tue, 29 Nov 2022 17:37:51 +0000 Subject: [PATCH] (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. --- .../plugins/check_unsafe_interpolations.rb | 103 ++++++++++-------- .../check_unsafe_interpolations_spec.rb | 40 +++++++ 2 files changed, 97 insertions(+), 46 deletions(-) diff --git a/lib/puppet-lint/plugins/check_unsafe_interpolations.rb b/lib/puppet-lint/plugins/check_unsafe_interpolations.rb index 2e63096..398bd57 100644 --- a/lib/puppet-lint/plugins/check_unsafe_interpolations.rb +++ b/lib/puppet-lint/plugins/check_unsafe_interpolations.rb @@ -1,5 +1,6 @@ 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| @@ -7,13 +8,15 @@ def check 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 diff --git a/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb b/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb index 261cd27..da9dd37 100644 --- a/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb +++ b/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