(CAT-1301) Add check unsafe interpolations check #142
Conversation
| end | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
wow, Rubocop really isn't happy with me... thanks @ekohl. I'll fix this up
There was a problem hiding this comment.
also, what is PUP-5704? Do you mean a specific version of puppet?
GSPatton
force-pushed
the
CAT-1301-add_unsafe_interpolations_check
branch
from
0b3812c
to
8e11a99
Compare
In puppetlabs/puppetlabs-apache#2444 I was corrected and Puppet 7.9.0 introduced the PUP-5704 feature this new style needs. That means if this lint rule is applied and a user runs Puppet < 7.9 (or Puppet < 6.24) the code will be invalid. It needs puppetlabs/puppet@59d045b. |
|
Perhaps submit the RuboCop fixes in a separate PR first to make reviewing a bit easier? |
GSPatton
force-pushed
the
CAT-1301-add_unsafe_interpolations_check
branch
2 times, most recently
from
8e11a99
to
1c645d6
Compare
lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb
Outdated
Show resolved
Hide resolved
lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb
Outdated
Show resolved
Hide resolved
lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb
Outdated
Show resolved
Hide resolved
lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb
Outdated
Show resolved
Hide resolved
| 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 |
There was a problem hiding this comment.
I don't think this is valid. There may be multiple tokens between, like whitespace.
| # 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 |
There was a problem hiding this comment.
Puppet doesn't care about whitespace, so why does this matter? Don't you want to look for a comma or } instead, which signifies that the next attribute comes or the definition is over.
There was a problem hiding this comment.
i'm not sure what you mean. This function is used before checking the contents of the command to check if the command is parameterised. Because if so, the command won't need checked. Parameterising the command is one of the ways of making string interpolation safer
lib/puppet-lint/plugins/check_unsafe_interpolations/check_unsafe_interpolations.rb
Outdated
Show resolved
Hide resolved
GSPatton
force-pushed
the
CAT-1301-add_unsafe_interpolations_check
branch
from
1c645d6
to
0f76677
Compare
GSPatton
changed the title
GSPatton
force-pushed
the
CAT-1301-add_unsafe_interpolations_check
branch
from
0f76677
to
31b72b8
Compare
This commit adds the check unsafe interpolations check to core puppet-lint insted of having it as a standalone gem. It corrects rubocop warnings in the plugin and adds the update .rubocop_todo which will be addressed in a separate PR
GSPatton
force-pushed
the
CAT-1301-add_unsafe_interpolations_check
branch
from
31b72b8
to
d3c9d9f
Compare
|
This does give false positives. If command is present the title doesn't need to be validated. An example: Or simplified: exec { "validate ${filename}":
command => "openssl x509 -in ${filename} -noout",
} |
background information: puppetlabs/puppet-lint#142
This commit adds the check unsafe interpolations check to core puppet-lint insted of having it as a standalone gem
Summary
Provide a detailed description of all the changes present in this pull request.
Additional Context
Add any additional context about the problem here.
Related Issues (if any)
Mention any related issues or pull requests.
Checklist