Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SARIF support #40

Merged
merged 31 commits into from
Jun 14, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
60e3ac2
add SARIF support
shaopeng-gh Mar 23, 2022
283382b
pretty print the whole json at the end
shaopeng-gh Mar 24, 2022
8627248
SARIF base path uri should end with `/`
shaopeng-gh Mar 28, 2022
8d15b84
Use better sample data in SARIF template
shaopeng-gh Mar 30, 2022
b2e0dce
Add help_uri and description to SARIF output
shaopeng-gh Mar 30, 2022
40cfdd4
update example values in sarif template
shaopeng-gh Apr 4, 2022
d916b98
test workflow
shaopeng-gh Apr 5, 2022
db93036
test puppet-lint-action
shaopeng-gh Apr 6, 2022
6e42257
test workflow
shaopeng-gh Apr 6, 2022
f9555df
test
shaopeng-gh Apr 6, 2022
40c367a
test path
shaopeng-gh Apr 6, 2022
99c91a3
continue-on-error: true
shaopeng-gh Apr 6, 2022
8af4705
use .
shaopeng-gh Apr 6, 2022
427333f
change order
shaopeng-gh Apr 6, 2022
369fadd
Revert "change order"
shaopeng-gh Apr 6, 2022
36e1fbd
change order
shaopeng-gh Apr 6, 2022
747505a
remove warning lines
shaopeng-gh Apr 6, 2022
d145c7f
clean up
shaopeng-gh Apr 6, 2022
0123103
use STDERR.put for warnings
shaopeng-gh Apr 8, 2022
ef1b694
Merge branch 'master' of https://github.com/shaopeng-gh/puppet-lint
shaopeng-gh Apr 8, 2022
16d625d
remove clean step
shaopeng-gh Apr 8, 2022
7fcf324
new line
shaopeng-gh Apr 8, 2022
1b2da02
fix test with stderr
shaopeng-gh Apr 8, 2022
01f8bd8
use Pathname.new
shaopeng-gh Apr 8, 2022
17ad7a8
use delete_if
shaopeng-gh Apr 8, 2022
119f16f
Fix comments
shaopeng-gh Apr 11, 2022
57c7af8
remove not needed strategy matrix
shaopeng-gh Apr 19, 2022
7232414
remove not needed strategy matrix
shaopeng-gh Apr 19, 2022
74b39e6
fix rule index
shaopeng-gh May 10, 2022
9d91452
upgrade to use upload-sarif@v2 instead of upload-sarif@v1
shaopeng-gh May 17, 2022
eca7fe0
removed example scan.yml
shaopeng-gh May 31, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .github/workflows/scan.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
name: puppet-lint scan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting thing. Do you have an example of where this ran so I can see the resulting report?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a sample (or sometimes called a starter workflow) that the user of the puppet-lint can add to their own repo, after we got all things done. User can use it to scan their repo and get code scanning alerts of the .pp files in their repo.
GitHub support this and the format used is SARIF format.

this is a run in my fork as example: (it is not intended to run against the puppet-lint repo itself, but the user's repo)
https://github.com/shaopeng-gh/puppet-lint/security/code-scanning

for people don't have permission:
image


on:
# Allows you to run this workflow manually from the Actions tab
workflow_dispatch:

jobs:

security:

strategy:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels odd to have a matrix here if there's just a single entry. Is that really needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matrix is not needed, I have removed it.

matrix:
operating-system: [ubuntu-latest]

runs-on: ${{ matrix.operating-system }}

steps:
- name: Checkout
uses: actions/checkout@v2

- name: Setup Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: 2.7
bundler-cache: true

- name: Scan
continue-on-error: true
run: bin/puppet-lint . --sarif > results.sarif

- name: Upload artifact
uses: actions/upload-artifact@v2
with:
path: results.sarif

- name: Upload SARIF to GitHub
uses: github/codeql-action/upload-sarif@v1
with:
sarif_file: results.sarif
7 changes: 3 additions & 4 deletions lib/puppet-lint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def print_context(message)
# problems - An Array of problem Hashes as returned by
# PuppetLint::Checks#run.
#
# Returns nothing.
# Returns array of problem.
def report(problems)
json = []
problems.each do |message|
Expand All @@ -171,7 +171,7 @@ def report(problems)

next unless message[:kind] == :fixed || [message[:kind], :all].include?(configuration.error_level)

if configuration.json
if configuration.json || configuration.sarif
message['context'] = get_context(message) if configuration.with_context
json << message
else
Expand All @@ -180,9 +180,8 @@ def report(problems)
print_github_annotation(message) if configuration.github_actions
end
end
puts JSON.pretty_generate(json) if configuration.json
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could affect usage via Rake. I don't know if people do this, but there are effectively 2 entry points: the bin file which you already found and Rake:
https://github.com/puppetlabs/puppet-lint/blob/020143b705b023946739eb44e7c7d99fcd087527/lib/puppet-lint/tasks/puppet-lint.rb#L88-L107=

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for all the inputs! I will set the PR as draft for now and look into them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have followed this instruction:
https://github.com/puppetlabs/puppet-lint/blob/master/README.md#testing-with-puppet-lint-as-a-rake-task

then running rake lint seems not having issue and output result normally.
I think running in rake currently only have normal
output does not support json option in the configuration, so the line highlighted will not be run in rake lint command, so should be good.


$stderr.puts 'Try running `puppet parser validate <file>`' if problems.any? { |p| p[:check] == :syntax }
json
end

# Public: Determine if PuppetLint found any errors in the manifest.
Expand Down
72 changes: 68 additions & 4 deletions lib/puppet-lint/bin.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require 'pathname'
require 'uri'
require 'puppet-lint/optparser'

# Internal: The logic of the puppet-lint bin script, contained in a class for
Expand Down Expand Up @@ -46,6 +48,21 @@ def run

begin
path = @args[0]
full_path = File.expand_path(path, ENV['PWD'])
full_base_path = if File.directory?(full_path)
full_path
else
File.dirname(full_path)
end

full_base_path_uri = if full_base_path.start_with?('/')
'file://' + full_base_path
else
'file:///' + full_base_path
end

full_base_path_uri += '/' unless full_base_path_uri.end_with?('/')

path = path.gsub(File::ALT_SEPARATOR, File::SEPARATOR) if File::ALT_SEPARATOR
path = if File.directory?(path)
Dir.glob("#{path}/**/*.pp")
Expand All @@ -57,15 +74,14 @@ def run

return_val = 0
ignore_paths = PuppetLint.configuration.ignore_paths
all_problems = []

puts '[' if PuppetLint.configuration.json
path.each do |f|
next if ignore_paths.any? { |p| File.fnmatch(p, f) }
l = PuppetLint.new
l.file = f
l.run
l.print_problems
puts ',' if f != path.last && PuppetLint.configuration.json
all_problems << l.print_problems

if l.errors? || (l.warnings? && PuppetLint.configuration.fail_on_warnings)
return_val = 1
Expand All @@ -76,7 +92,17 @@ def run
fd.write(l.manifest)
end
end
puts ']' if PuppetLint.configuration.json

if PuppetLint.configuration.sarif
report_sarif(all_problems, full_base_path, full_base_path_uri)
elsif PuppetLint.configuration.json
all_problems.each do |problems|
problems.each do |problem|
problem.delete_if { |key, _| [:description, :help_uri].include?(key) }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the description must be excluded. The help URI also doesn't hurt IMHO. Perhaps a maintainer can weigh in on that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a maintainer can weigh in on that. ---- yes the only reason for this code block is I am trying to maintain the same JSON output as before, maintainer can advise if also want these 2 new properties in JSON and let me know to change accordingly.

end
end
puts JSON.pretty_generate(all_problems)
end

return return_val
rescue PuppetLint::NoCodeError
Expand All @@ -85,4 +111,42 @@ def run
return 1
end
end

# Internal: Print the reported problems in SARIF format to stdout.
#
# problems - An Array of problem Hashes as returned by
# PuppetLint::Checks#run.
#
# Returns nothing.
def report_sarif(problems, base_path, base_path_uri)
sarif_file = File.read(File.join(__dir__, 'report', 'sarif_template.json'))
sarif = JSON.parse(sarif_file)
sarif['runs'][0]['originalUriBaseIds']['ROOTPATH']['uri'] = base_path_uri
rules = sarif['runs'][0]['tool']['driver']['rules'] = []
results = sarif['runs'][0]['results'] = []
problems.each do |messages|
messages.each do |message|
relative_path = Pathname.new(message[:fullpath]).relative_path_from(Pathname.new(base_path))
rules = sarif['runs'][0]['tool']['driver']['rules']
rule_exists = rules.any? { |r| r['id'] == message[:check] }
unless rule_exists
new_rule = { 'id' => message[:check] }
new_rule['fullDescription'] = { 'text' => message[:description] } unless message[:description].nil?
new_rule['fullDescription'] = { 'text' => 'Check for any syntax error and record an error of each instance found.' } if new_rule['fullDescription'].nil? && message[:check] == :syntax
new_rule['defaultConfiguration'] = { 'level' => message[:KIND].downcase } if %w[error warning].include?(message[:KIND].downcase)
new_rule['helpUri'] = message[:help_uri] unless message[:help_uri].nil?
rules << new_rule
end
rule_index = rules.count - 1
result = {
'ruleId' => message[:check],
'ruleIndex' => rule_index,
'message' => { 'text' => message[:message] },
'locations' => [{ 'physicalLocation' => { 'artifactLocation' => { 'uri' => relative_path, 'uriBaseId' => 'ROOTPATH' }, 'region' => { 'startLine' => message[:line], 'startColumn' => message[:column] } } }],
}
results << result
end
end
puts JSON.pretty_generate(sarif)
end
end
10 changes: 6 additions & 4 deletions lib/puppet-lint/checkplugin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,12 @@ def default_info
#
# kind - The Symbol problem type (:warning or :error).
# problem - A Hash containing the attributes of the problem
# :message - The String message describing the problem.
# :line - The Integer line number of the location of the problem.
# :column - The Integer column number of the location of the problem.
# :check - The Symbol name of the check that detected the problem.
# :message - The String message describing the problem.
# :line - The Integer line number of the location of the problem.
# :column - The Integer column number of the location of the problem.
# :check - The Symbol name of the check that detected the problem.
# :description - The description of the check that detected the problem.
# :help_uri - The help web page of the check that detected the problem.
#
# Returns nothing.
def notify(kind, problem)
Expand Down
1 change: 1 addition & 0 deletions lib/puppet-lint/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ def defaults
self.with_context = false
self.fix = false
self.json = false
self.sarif = false
self.show_ignored = false
self.ignore_paths = ['vendor/**/*.pp']
self.github_actions = ENV.key?('GITHUB_ACTION')
Expand Down
4 changes: 2 additions & 2 deletions lib/puppet-lint/data.rb
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ def parse_control_comments
if top_override.nil?
# TODO: refactor to provide a way to expose problems from
# PuppetLint::Data via the normal problem reporting mechanism.
puts "WARNING: lint:endignore comment with no opening lint:ignore:<check> comment found on line #{token.line}"
$stderr.puts "WARNING: lint:endignore comment with no opening lint:ignore:<check> comment found on line #{token.line}"
else
top_override.each do |start|
next if start.nil?
Expand All @@ -607,7 +607,7 @@ def parse_control_comments
end

stack.each do |control|
puts "WARNING: lint:ignore:#{control[0][2]} comment on line #{control[0][0]} with no closing lint:endignore comment"
$stderr.puts "WARNING: lint:ignore:#{control[0][2]} comment on line #{control[0][0]} with no closing lint:endignore comment"
end
end
end
Expand Down
4 changes: 4 additions & 0 deletions lib/puppet-lint/optparser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ def self.build(args = [])
PuppetLint.configuration.json = true
end

opts.on('--sarif', 'Log output as SARIF') do
PuppetLint.configuration.sarif = true
end

opts.on('--list-checks', 'List available check names.') do
PuppetLint.configuration.list_checks = true
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ def check

notify(
:warning,
:message => "arrow should be on the right operand's line",
:line => token.line,
:column => token.column,
:token => token
:message => "arrow should be on the right operand's line",
:line => token.line,
:column => token.column,
:token => token,
:description => 'Test the manifest tokens for chaining arrow that is on the line of the left operand when the right operand is on another line.',
:help_uri => 'https://puppet.com/docs/puppet/latest/style_guide.html#chaining-arrow-syntax'
)
end
end
Expand Down
8 changes: 5 additions & 3 deletions lib/puppet-lint/plugins/check_classes/autoloader_layout.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ def check

notify(
:error,
:message => "#{title_token.value} not in autoload module layout",
:line => title_token.line,
:column => title_token.column
:message => "#{title_token.value} not in autoload module layout",
:line => title_token.line,
:column => title_token.column,
:description => 'Test the manifest tokens for any classes or defined types that are not in an appropriately named file for the autoloader to detect and record an error of each instance found.',
:help_uri => 'https://puppet.com/docs/puppet/latest/style_guide.html#separate-files'
)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ def check

notify(
:warning,
:message => 'class inheriting from params class',
:line => class_idx[:inherited_token].line,
:column => class_idx[:inherited_token].column
:message => 'class inheriting from params class',
:line => class_idx[:inherited_token].line,
:column => class_idx[:inherited_token].column,
:description => 'Check the manifest tokens for any classes that inherit a params subclass and record a warning for each instance found.',
:help_uri => nil
)
end
end
Expand Down
8 changes: 5 additions & 3 deletions lib/puppet-lint/plugins/check_classes/code_on_top_scope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ def check

notify(
:warning,
:message => "code outside of class or define block - #{token.value}",
:line => token.line,
:column => token.column
:message => "code outside of class or define block - #{token.value}",
:line => token.line,
:column => token.column,
:description => 'Test that no code is outside of a class or define scope.',
:help_uri => nil
)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ def check

notify(
:warning,
:message => 'class inherits across module namespaces',
:line => class_idx[:inherited_token].line,
:column => class_idx[:inherited_token].column
:message => 'class inherits across module namespaces',
:line => class_idx[:inherited_token].line,
:column => class_idx[:inherited_token].column,
:description => 'Test the manifest tokens for any classes that inherit across namespaces and record a warning for each instance found.',
:help_uri => 'https://puppet.com/docs/puppet/latest/style_guide.html#class-inheritance'
)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ def check

notify(
:error,
:message => "#{obj_type} name containing a dash",
:line => class_idx[:name_token].line,
:column => class_idx[:name_token].column
:message => "#{obj_type} name containing a dash",
:line => class_idx[:name_token].line,
:column => class_idx[:name_token].column,
:description => 'Check the manifest tokens for any classes or defined types that have a dash in their name and record an error for each instance found.',
:help_uri => nil
)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ def check

notify(
:error,
:message => "#{obj_type} '#{class_idx[:name_token].value}' contains illegal uppercase",
:line => class_idx[:name_token].line,
:column => class_idx[:name_token].column,
:token => class_idx[:name_token]
:message => "#{obj_type} '#{class_idx[:name_token].value}' contains illegal uppercase",
:line => class_idx[:name_token].line,
:column => class_idx[:name_token].column,
:token => class_idx[:name_token],
:description => 'Find and warn about module names with illegal uppercase characters.',
:help_uri => 'https://puppet.com/docs/puppet/latest/modules_fundamentals.html#allowed-module-names'
)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ def check

notify(
:warning,
:message => "#{type} defined inside a class",
:line => token.line,
:column => token.column
:message => "#{type} defined inside a class",
:line => token.line,
:column => token.column,
:description => 'Test the manifest tokens for any classes or defined types that are defined inside another class.',
:help_uri => 'https://puppet.com/docs/puppet/latest/style_guide.html#nested-classes-or-defined-types'
)
end
end
Expand Down
8 changes: 5 additions & 3 deletions lib/puppet-lint/plugins/check_classes/parameter_order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,11 @@ def check
msg = 'optional parameter listed before required parameter'
notify(
:warning,
:message => msg,
:line => token.line,
:column => token.column
:message => msg,
:line => token.line,
:column => token.column,
:description => 'Test the manifest tokens for any parameterised classes or defined types that take parameters and record a warning if there are any optional parameters listed before required parameters.',
:help_uri => 'https://puppet.com/docs/puppet/latest/style_guide.html#display-order-of-parameters'
)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ def check
tokens.select { |r| r.type == :OUT_EDGE }.each do |token|
notify(
:warning,
:message => 'right-to-left (<-) relationship',
:line => token.line,
:column => token.column
:message => 'right-to-left (<-) relationship',
:line => token.line,
:column => token.column,
:description => 'Test the manifest tokens for any right-to-left (<-) chaining operators and record a warning for each instance found.',
:help_uri => 'https://puppet.com/docs/puppet/latest/style_guide.html#chaining-arrow-syntax'
)
end
end
Expand Down
Loading