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

Bug: SAST is calculated incorrectly #1231

Open
eustas opened this issue Jul 13, 2023 · 3 comments
Open

Bug: SAST is calculated incorrectly #1231

eustas opened this issue Jul 13, 2023 · 3 comments

Comments

@eustas
Copy link

eustas commented Jul 13, 2023

Context: https://github.com/google/brotli/security/code-scanning/4
Quick view on actions panel reveals that report is not true: https://github.com/google/brotli/actions/workflows/codeql.yml?query=branch%3Amaster

@eustas eustas changed the title SAST is calculated incorrectly Bug: SAST is calculated incorrectly Jul 13, 2023
@spencerschrock
Copy link
Contributor

spencerschrock commented Jul 13, 2023

Scorecard does its analysis by downloading the repo's tarball. brotli uses a .gitattributes file which doesn't include the github workflows in its download, so it has trouble seeing your codeql workflow file.

This is unfortunately a known issue (ossf/scorecard#2489 (comment)), and some of the alternatives (e.g git clone) have proved slow enough that we haven't switched over. It may be worth having a Scorecard CLI argument (and a corresponding GitHub action argument) which does a git clone.

@eustas
Copy link
Author

eustas commented Jul 14, 2023

Thanks for pointing me to that. But for me it looks like a different problem.
Scorecard does not say that SAST is not detected, it says: "SAST tool detected but not run on all commmits".

@spencerschrock
Copy link
Contributor

Thanks for pointing me to that. But for me it looks like a different problem. Scorecard does not say that SAST is not detected, it says: "SAST tool detected but not run on all commmits".

My mistake, we recently changed how we look for the workflow file, so I had assumed that was it.

In terms of detecting it on all commits, I see this when running Scorecard

go run main.go --repo google/brotli --checks SAST --show-details --verbosity debug --show-details --format json | jq
    {
      "details": [
        "Debug: tool detected: github-code-scanning",
        "Debug: tool detected: github-code-scanning",
        "Warn: 2 commits out of 5 are checked with a SAST tool",
        "Warn: CodeQL tool not detected"
      ],
      "score": 4,
      "reason": "SAST tool is not run on all commits -- score normalized to 4",
      "name": "SAST",
      "documentation": {
        "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#sast",
        "short": "Determines if the project uses static code analysis."
      }
    }

The offending PRs seem to be empty PRs from Copybara:

List of pull requests without CI test: 1039, 1035, 1021

For each of the last 30 commits, Scorecard looks at the associated PR and then the last commit in that PR to look for the SAST tool. So the current logic doesn't count direct pushes to main, but rather that a SAST tool is run on a PR before merging. There has been some discussion on that here ossf/scorecard#1580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants