Implement experimental Bandit scan check #762
Conversation
sourabhsparkala
force-pushed
the
bandit
branch
from
d2ff50b to
3bbe84c
Compare
sourabhsparkala
requested review from
ManjunathMS35,
SebastianWolf-SAP and
mibo
There was a problem hiding this comment.
No detailed knowledge about Bandit and also not that much time to dig deeper here, so unfortunately I can't give an explicit approval.
However, code looks very good, I agree with the weighting of the Bandit score, also the test seem to cover the most relevant aspects. As the checks ran through properly, it should be OK...
| = Arrays.asList(".yaml", ".yml"); | ||
|
|
||
| /** | ||
| * A step in a GitHub action that triggers analysis with CodeQL. |
There was a problem hiding this comment.
I guess, you need to update this comment.
| * <li>{@link OssFeatures#USES_BANDIT_SCAN_CHECKS}</li> | ||
| * </ul> | ||
| */ | ||
| public class BanditDataProvider extends GitHubCachingDataProvider { |
There was a problem hiding this comment.
I guess this provider is based on some existing one related to CodeQL. There may be opportunities to reduce duplicate code.
There was a problem hiding this comment.
Refactored it :)
| /** | ||
| * Shows if an open-source project runs Bandit scans. | ||
| * | ||
| * @see <a href="https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#jobsjob_idstepsrun">GitHub workflow action job config to run Bandit code scanning for a repository</a> |
There was a problem hiding this comment.
I'd use a link to Bandit instead of GitHub Actions.
| } | ||
|
|
||
| @Override | ||
| protected ValueSet fetchValuesFor(GitHubProject project) throws IOException { |
There was a problem hiding this comment.
Bandit can be run without GitHub actions. This data provider may be updated to catch it.
| "The score value is unknown because all required features are unknown."); | ||
| } | ||
|
|
||
| if (!languages.isUnknown() && !SUPPORTED_LANGUAGES.containsAnyOf(languages.get())) { |
There was a problem hiding this comment.
If the languages feature is unknown, but the rest two features are false, then the scoring function produces 0 which may not be always correct. In this case, you may consider returning N/A but I am not sure though.
There was a problem hiding this comment.
It could also be that
- The languages data provider fails to collect the language information but the project still triggers the Bandit scan.
- I guess it is up to us to decide how we want to take this up.
There was a problem hiding this comment.
I have added a check for unknown language. I decided that it doesn't make sense to check for a Bandit scan if there is no mention that the project uses Python scripts.
| @@ -97,7 +97,7 @@ private void testCodeqlRuns(String filename, InputStream content, Value<?>... ex | |||
| CodeqlDataProvider provider = new CodeqlDataProvider(fetcher); | |||
| ValueSet values = provider.fetchValuesFor(PROJECT); | |||
|
|
|||
| assertEquals(2, expectedValues.length); | |||
| assertEquals(2, values.size()); | |||
There was a problem hiding this comment.
I guess, this update is not necessary.
There was a problem hiding this comment.
There is already a Test confirming that the possible features that will be available here https://github.com/SAP/fosstars-rating-core/pull/762/files/f9d5d97620fce7554c112f9a58b257816e56aa96#diff-7567ba8d1e196c5107745af1dcfa55c3f64d94c9b85201c05247500f2c7944a8L59
But you are right. As we are ourselves passing the expected values. The test is the same.
sourabhsparkala
force-pushed
the
bandit
branch
from
f9d5d97 to
f00fe3e
Compare
| * A step in a GitHub action that triggers analysis with Bandit. | ||
| */ | ||
| private static final Pattern RUN_STEP_BANDIT_REGEX_PATTERN | ||
| = Pattern.compile("^.*bandit .*$", Pattern.DOTALL); |
There was a problem hiding this comment.
I guess, the whitespace is here for a reason.
There was a problem hiding this comment.
yes, the syntax goes bandit -r. Just to indicate that bandit is a function call during run: in GitHub actions and not randomly occuring command word. Maybe I should include all the parameters possible which comes with bandit run.
However to improve this, let me work on this here #789
Fixes #729