diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index f2348889..c2aeda6c 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2023-04-04 08:44:46 UTC using RuboCop version 1.48.1. +# on 2023-08-25 10:33:19 UTC using RuboCop version 1.48.1. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -34,12 +34,12 @@ Lint/MissingCopEnableDirective: - 'lib/puppet-lint/tasks/puppet-lint.rb' - 'spec/unit/puppet-lint/puppet-lint_spec.rb' -# Offense count: 58 +# Offense count: 60 # Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes. Metrics/AbcSize: Max: 142 -# Offense count: 34 +# Offense count: 36 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns, inherit_mode. # AllowedMethods: refine Metrics/BlockLength: @@ -55,22 +55,27 @@ Metrics/BlockNesting: Metrics/ClassLength: Max: 387 -# Offense count: 33 +# Offense count: 35 # Configuration parameters: AllowedMethods, AllowedPatterns. Metrics/CyclomaticComplexity: Max: 33 -# Offense count: 80 +# Offense count: 82 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. Metrics/MethodLength: Max: 108 -# Offense count: 26 +# Offense count: 27 # Configuration parameters: AllowedMethods, AllowedPatterns. Metrics/PerceivedComplexity: Max: 31 -# Offense count: 182 +# Offense count: 1 +Naming/AccessorMethodName: + Exclude: + - 'lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb' + +# Offense count: 186 # Configuration parameters: ForbiddenDelimiters. # ForbiddenDelimiters: (?i-mx:(^|\s)(EO[A-Z]{1}|END)(\s|$)) Naming/HeredocDelimiterNaming: @@ -92,13 +97,13 @@ Performance/CollectionLiteralInLoop: - 'lib/puppet-lint/plugins/check_resources/ensure_first_param.rb' - 'lib/puppet-lint/plugins/check_whitespace/trailing_whitespace.rb' -# Offense count: 406 +# Offense count: 417 # Configuration parameters: Prefixes, AllowedPatterns. # Prefixes: when, with, without RSpec/ContextWording: Enabled: false -# Offense count: 41 +# Offense count: 42 # Configuration parameters: IgnoredMetadata. RSpec/DescribeClass: Enabled: false @@ -122,7 +127,7 @@ RSpec/FilePath: - 'spec/unit/puppet-lint/lexer_spec.rb' - 'spec/unit/puppet-lint/puppet-lint_spec.rb' -# Offense count: 138 +# Offense count: 139 RSpec/MultipleExpectations: Max: 137 @@ -136,14 +141,21 @@ RSpec/MultipleMemoizedHelpers: RSpec/NestedGroups: Max: 5 -# Offense count: 8 +# Offense count: 10 RSpec/RepeatedExampleGroupDescription: Exclude: - 'spec/unit/puppet-lint/plugins/check_resources/file_mode_spec.rb' - 'spec/unit/puppet-lint/plugins/check_resources/unquoted_file_mode_spec.rb' + - 'spec/unit/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations_spec.rb' - 'spec/unit/puppet-lint/plugins/legacy_facts/legacy_facts_spec.rb' -# Offense count: 106 +# Offense count: 1 +# This cop supports unsafe autocorrection (--autocorrect-all). +Style/ConcatArrayLiterals: + Exclude: + - 'lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb' + +# Offense count: 108 # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: EnforcedStyle. # SupportedStyles: always, always_true, never diff --git a/lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb b/lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb new file mode 100644 index 00000000..8949672f --- /dev/null +++ b/lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb @@ -0,0 +1,130 @@ +COMMANDS = Array['command', 'onlyif', 'unless'] +INTERPOLATED_STRINGS = Array[:DQPRE, :DQMID] +USELESS_CHARS = Array[:WHITESPACE, :COMMA] + +PuppetLint.new_check(:check_unsafe_interpolations) do + def check + # Gather any exec commands' resources into an array + exec_resources = resource_indexes.filter_map do |resource| + resource_parameters = resource[:param_tokens].map(&:value) + resource if resource[:type].value == 'exec' && !(COMMANDS & resource_parameters).empty? + end + + # Iterate over title tokens and raise a warning if any are variables + unless get_exec_titles.empty? + get_exec_titles.each do |title| + check_unsafe_title(title) + end + end + + # Iterate over each command found in any exec + exec_resources.each do |command_resources| + check_unsafe_interpolations(command_resources) + end + end + + # Iterate over the tokens in a title and raise a warning if an interpolated variable is found + def check_unsafe_title(title) + title.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| + # 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) + + check_command(token).each do |t| + notify_warning(t) + end + end + end + + # Raises a warning given a token and message + def notify_warning(token) + notify :warning, + message: "unsafe interpolation of variable '#{token.value}' in exec command", + line: token.line, + column: token.column + end + + # Iterates over the tokens in a command and adds it to an array of violations if it is an input variable + def check_command(token) + # Initialise variables needed in while loop + rule_violations = [] + current_token = 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) + current_token = current_token.next_token + end + + rule_violations + end + + # A command is parameterised if its args are placed in an array + # This function checks if the current token is a :FARROW and if so, if it is followed by an LBRACK + def parameterised?(token) + current_token = token + while current_token.type != :NEWLINE + return true if current_token.type == :FARROW && current_token.next_token.next_token.type == :LBRACK + + current_token = current_token.next_token + end + 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 + def get_exec_titles + result = [] + tokens.each_with_index do |_token, token_idx| + next if tokens[token_idx].value != 'exec' + + # We have a resource declaration. Now find the title + tokens_array = [] + # Check if title is an array + if tokens[token_idx]&.next_code_token&.next_code_token&.type == :LBRACK + # Get the start and end indices of the array of titles + array_start_idx = tokens.rindex { |r| r.type == :LBRACK } + array_end_idx = tokens.rindex { |r| r.type == :RBRACK } + + # Grab everything within the array + title_array_tokens = tokens[(array_start_idx + 1)..(array_end_idx - 1)] + tokens_array.concat(title_array_tokens.reject do |token| + USELESS_CHARS.include?(token.type) + end) + result << tokens_array + # Check if title is double quotes string + elsif tokens[token_idx].next_code_token.next_code_token.type == :DQPRE + # Find the start and end of the title + title_start_idx = tokens.find_index(tokens[token_idx].next_code_token.next_code_token) + title_end_idx = title_start_idx + index_offset_for(':', tokens[title_start_idx..tokens.length]) + + result << tokens[title_start_idx..title_end_idx] + # Title is in single quotes + else + tokens_array.concat([tokens[token_idx].next_code_token.next_code_token]) + + result << tokens_array + end + end + result + end + + def interpolated?(token) + INTERPOLATED_STRINGS.include?(token.type) + end + + # Finds the index offset of the next instance of `value` in `tokens_slice` from the original index + def index_offset_for(value, tokens_slice) + tokens_slice.each_with_index do |token, i| + return i if value.include?(token.value) + end + end +end diff --git a/spec/unit/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations_spec.rb b/spec/unit/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations_spec.rb new file mode 100644 index 00000000..b19dee91 --- /dev/null +++ b/spec/unit/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations_spec.rb @@ -0,0 +1,186 @@ +require 'spec_helper' + +describe 'check_unsafe_interpolations' do + let(:msg) { "unsafe interpolation of variable 'foo' in exec command" } + + context 'with fix disabled' do + context 'exec with unsafe interpolation in command' do + let(:code) do + <<-PUPPET + class foo { + + exec { 'bar': + command => "echo ${foo}", + } + + } + PUPPET + end + + it 'detects an unsafe exec command argument' do + expect(problems).to have(1).problems + end + + it 'creates one warning' do + expect(problems).to contain_warning(msg) + end + end + + context 'exec with multiple unsafe interpolations in command' do + let(:code) do + <<-PUPPET + class foo { + + exec { 'bar': + command => "echo ${foo} ${bar}", + } + + } + PUPPET + end + + it 'detects multiple unsafe exec command arguments' do + expect(problems).to have(2).problems + end + + it 'creates two warnings' do + expect(problems).to contain_warning(msg) + expect(problems).to contain_warning(msg) + end + end + + context 'code that uses title with unsafe string as command' do + let(:code) do + <<-PUPPET + class foo { + + exec { "echo ${foo}": } + + } + PUPPET + end + + it 'detects one problem' do + expect(problems).to have(1).problems + end + + it 'creates one warning' do + expect(problems).to contain_warning(msg) + end + end + + context 'exec with a safe string in command' do + let(:code) do + <<-PUPPET + class foo { + + exec { 'bar': + command => "echo foo", + } + + } + PUPPET + end + + it 'detects zero problems' do + expect(problems).to have(0).problems + end + end + + context 'exec that has an array of args in command' do + let(:code) do + <<-PUPPET + class foo { + + exec { 'bar': + command => ['echo', $foo], + } + } + PUPPET + end + + it 'detects zero problems' do + expect(problems).to have(0).problems + end + end + + context 'exec that has an array of args in command' do + let(:code) do + <<-PUPPET + class foo { + + exec { ["foo", "bar", "baz"]: + command => echo qux, + } + } + PUPPET + end + + it 'detects zero problems' do + 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 + + context 'case statement and an exec' do + let(:code) do + <<-PUPPET + class foo { + case bar { + baz : { + echo qux + } + } + + exec { 'foo': + command => "echo bar", + } + } + PUPPET + end + + it 'detects zero problems' do + expect(problems).to have(0).problems + end + end + end +end