search

ossf / scorecard

OpenSSF Scorecard - Security health metrics for Open Source
https://scorecard.dev
Apache License 2.0
4.29k stars 465 forks source link

BUG SAST tool check runs on doc-only commits. #2487

Open ianlewis opened 1 year ago

ianlewis commented 1 year ago

The slsa-github-generator repo currently omits running the CodeQL action on commits that are documentation or yaml only as the CodeQL action is slow doesn't support markdown or yaml anyway.

However, scorecard dings us for not running CodeQL on all of our commits. It would be nice if the scorecard could detect if the commits were documentation-only changes or not.

azeemshaikh38 commented 1 year ago

Thanks @ianlewis. IIUC, there are static analysis tools for config/workflow files. So checking for SAST tools on commits which only change YAML would be a valid usecase?

If we were to limit ourselves to doc files (like .md) only, my concern is:

ianlewis commented 1 year ago

Thanks @ianlewis. IIUC, there are static analysis tools for config/workflow files. So checking for SAST tools on commits which only change YAML would be a valid usecase?

I think that's a fair point. It would be nice if scorecard had support for them.

If we were to limit ourselves to doc files (like .md) only, my concern is:

  • retrieving the changed files through GitHub API might significantly increase API token usage
  • would not work for projects which use Gerrit or other such code review tools
  • and is checking for commits which only modify .md file a strong enough usecase to merit investing into?

Maybe the solution is expanding the number of SAST tools that scoreccard checks for?

Scorecard itself has SAST like behavior for its permissions checks on GitHub workflows. I know several projects that have found those valuable.

azeemshaikh38 commented 1 year ago

@laurentsimon do you know if we consider scorecard-action itself as a SAST tool?

laurentsimon commented 1 year ago

We currently do not, but we should revive https://github.com/ossf/scorecard/pull/1487 and improve the SAST check to include it. The PR is too API intensive, and we need to solve this problem before merging it.