From c0b2e90be1089ad2cd5deb5b80ac6368bd49a061 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Mon, 11 Dec 2023 10:51:32 -1000 Subject: [PATCH 1/5] Fix running the test suite The rspec-collection_matchers documentation advise to require rspec-collection_matchers form `spec_helper.rb`. This fix: ``` Failure/Error: expect(problems).to have(1).problems NoMethodError: undefined method `have' for # ``` --- spec/spec_helper.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 4aa9e69..7060224 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,3 +1,4 @@ require 'puppet-lint' +require 'rspec/collection_matchers' PuppetLint::Plugins.load_spec_helper From c3d5b6fe53497dd159968ba20438663b635582cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Mon, 11 Dec 2023 14:35:44 -1000 Subject: [PATCH 2/5] Fix checking of warning messages We produce 2 errors in this example. We don't want to check that the first one is present twice: we want to check that each warning is present once. --- spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb b/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb index b19dee9..22b99f4 100644 --- a/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb +++ b/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb @@ -45,7 +45,7 @@ class foo { 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") end end From b35bc5f1b83434fadcbb06f491327725a1062c17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Mon, 11 Dec 2023 14:37:23 -1000 Subject: [PATCH 3/5] Add more checks for onlyif / unless These commands are supposed to be supported, but they are not tested, so add tests to demonstrate that they work as intended. --- .../puppet-lint/plugins/check_unsafe_interpolations_spec.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb b/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb index 22b99f4..0cd088a 100644 --- a/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb +++ b/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb @@ -33,6 +33,8 @@ class foo { exec { 'bar': command => "echo ${foo} ${bar}", + onlyif => "${baz}", + unless => "${bazz}", } } @@ -40,12 +42,14 @@ class foo { 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("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 From 7b7b42d796793f4b8d2864b0f806b58d79527e79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Mon, 11 Dec 2023 14:38:25 -1000 Subject: [PATCH 4/5] Fix detection of insecure interpolations in unless parameter When using the `unless` parameter of an `exec` resource with unsafe string interpolation, the linter should warn about the issue. It happen that it currently doesn't because unless is also a keyword. Adjust the linter to cope with this. --- lib/puppet-lint/plugins/check_unsafe_interpolations.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet-lint/plugins/check_unsafe_interpolations.rb b/lib/puppet-lint/plugins/check_unsafe_interpolations.rb index 33838f1..480493c 100644 --- a/lib/puppet-lint/plugins/check_unsafe_interpolations.rb +++ b/lib/puppet-lint/plugins/check_unsafe_interpolations.rb @@ -33,7 +33,7 @@ def check_unsafe_title(title) 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) From 80a991d1bbc59f0465ae923fca1180b474c3a36c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= Date: Mon, 8 Apr 2024 13:14:38 -1000 Subject: [PATCH 5/5] Fix safe interpolation detection Any variable interpolation is currently reported as unsafe. The stdlib feature a `stdlib::shell_escape()` function (formerly `shell_escape()`) that escape the string passed as parameter. In such a case, an unsafe interpolation should not be detected. Add detection of such escaped string and do not report an error in this case. `stdlib::shell_escape()` must be the last function called in the interpolation for it to not be reported as unsafe. --- .../plugins/check_unsafe_interpolations.rb | 56 +++++++++++++++++-- .../check_unsafe_interpolations_spec.rb | 38 +++++++++++++ 2 files changed, 88 insertions(+), 6 deletions(-) diff --git a/lib/puppet-lint/plugins/check_unsafe_interpolations.rb b/lib/puppet-lint/plugins/check_unsafe_interpolations.rb index 480493c..f71ad2c 100644 --- a/lib/puppet-lint/plugins/check_unsafe_interpolations.rb +++ b/lib/puppet-lint/plugins/check_unsafe_interpolations.rb @@ -22,10 +22,10 @@ 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 @@ -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 diff --git a/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb b/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb index 0cd088a..908d70e 100644 --- a/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb +++ b/spec/puppet-lint/plugins/check_unsafe_interpolations_spec.rb @@ -91,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