[Research]: Posting only findings in diffs to PR

guardrailsio / guardrails-engine-output-schema-validator

Schema Validator for GuardRails Engines

0 stars 1 forks source link

[Research]: Posting only findings in diffs to PR #14

Closed streichsbaer closed 6 years ago

streichsbaer commented 6 years ago

Research on how to implement it.

Nice to have:

make it toggleable via config

ghost commented 6 years ago

The easy solution I see at the moment:

Run engines as usual
Get list of added/edited files in the pull requests (via https://developer.github.com/v3/pulls/#list-pull-requests-files)
Filter out engines findings where findings path don't match pull requests files

I explored couple of other things, but I didn't see right away how we could make use of diff / patch. Exploring how pronto does it might be interesting.

ghost commented 6 years ago

Feedback welcome @streichsbaer @pxlpnk

streichsbaer commented 6 years ago

Yeah, I think this is good enough for the first round of improvements. Hopefully should be a quick enough fix for now. We can further tweak in the next round.

pxlpnk commented 6 years ago

I can not really understand how this would work with dependencies. Should we show them always then?

Looking forward to a bitbucket/gitLab integration, I think we should make sure that we can apply the same mechanism there as well.

Otherwise, this could be something to explore as a first iteration. How much work (for GH only) do you think this would be?

ghost commented 6 years ago

@pxlpnk we can have a language/process-to-dependency-file mapping for the dependencies and display dependencies findings when there's a change to the associated dependency file (package.json, Gemfile, etc.). Shouldn't be too long to implement.

streichsbaer commented 6 years ago

yeah, that makes sense.

I think it can be done in two iterations:

Only show findings that are in changed files for the PR. Even if that means showing all insecure dependencies if the e.g package.json as changed. That's the default behaviour, but as discussed previously, the users can overwrite it via guardrails config file - to show all findings in each pr.
Then as a next step improve the logic to what amine described above

streichsbaer commented 6 years ago

After a quick chat with Andy, I agree that it would make sense to show dependencies always for now. We could make that configurable later, but for now it would be as simple as:

Only filter out sast/secrets findings based on PR changed files
Show all dependencies

Then with that in place, we can get feedback from people and come up with the next set of improvements.