-
Notifications
You must be signed in to change notification settings - Fork 361
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
Recreated Github Action PR #432
Conversation
Signed-off-by: Rex P <rexpan@google.com>
Signed-off-by: Rex P <rexpan@google.com>
Here is an example of the PR reusable workflow working: another-rex/scorecard-check-osv-e2e#1 That PR adds an additional vulnerability, which causes it to fail. You can see that only the new vuln is showing up in the code scanning report: https://github.com/another-rex/scorecard-check-osv-e2e/security/code-scanning/1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! some minor comments/questions.
- name: "Checkout target branch" | ||
run: git checkout $GITHUB_BASE_REF | ||
- name: "Run scanner" | ||
uses: another-rex/osv-scanner@markdown-output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update all of these references to google/osv-scanner@main ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we do, but only after this is already merged, otherwise I think it will prevent merging as the action will fail. Maybe a better way is to merge only the code part and have a separate PR for the workflows part.
action.yml
Outdated
default: 'sarif' | ||
recursive-search: | ||
description: 'Recursively scan though subdirectories' | ||
required: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is required: false
required if there is a default
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's required, but I think it's clearer to explicitly state it for all for all inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random note because frankly its weird imo and super easy to miss: required
isn't actually enforced "yet"
Until google/osv-scanner#432 is merged, this method seems to be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything in particular blocking this PR from merging at this point?
This PR features:
action.yaml
and it's specialized dockerfileaction.dockerfile
. This docker image runs a bash script wrapping osv-scanner, first by preprocessing the input so the last argument will be split by new line, allowing the workflow user to pass in multiple directories/files they wish to scan. The script also changes exit codes 127 and 128 to 0 as they contain errors that the user can't really do anything about.--experimental-diff
, which will only output the difference between a previous run and this run of the osv-scanner. This is for use in the PR workflow.Closes #57
Currently the reusable workflow has to point to a specific action which cannot be relative (otherwise it would point to the wrong action when reused in another repo). This means right now it's pointed to this fork/branch instead of the master branch, this will need to be updated once this PR is merged.
Example of what workflow sarif output looks like:
Here is an example of the PR reusable workflow working:
another-rex/scorecard-check-osv-e2e#1
That PR adds an additional vulnerability, which causes it to fail. You can see that only the new vuln is showing up in the code scanning report: https://github.com/another-rex/scorecard-check-osv-e2e/security/code-scanning/1
TODO after this PR is merged: