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.

Sign up for GitHub

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

Jump to bottom

✨Add LGTM to the SAST check #1232

Merged
merged 3 commits into from
Nov 10, 2021
Merged

✨Add LGTM to the SAST check #1232

merged 3 commits into from
Nov 10, 2021

Conversation

evverx
Copy link
Contributor

@evverx evverx commented Nov 10, 2021

According to https://github.com/apps/lgtm-com
"LGTM is a code analysis platform for identifying vulnerabilities early and preventing
them from reaching production". It's used by systemd, lxc and a lot of other large
open source projects. The check is
still kind of broken in the sense that it fails to detect
projects where every PR is analyzed by LGTM before getting merged
but it's better than nothing I guess.

@evverx
Add LGTM to the SAST check
43b37a4 
According to https://github.com/apps/lgtm-com
"LGTM is a code analysis platform for identifying vulnerabilities early and preventing
them from reaching production". It's used by `systemd`, `lxc` and a lot of other large
open source projects. The check is
still kind of broken in the sense that it fails to detect
projects where every PR is analyzed by LGTM before getting merged
but it's better than nothing I guess.
@evverx evverx changed the title Add LGTM to the SAST check ✨Add LGTM to the SAST check Nov 10, 2021
@evverx evverx mentioned this pull request Nov 10, 2021
Open
@naveensrinivasan
Copy link
Member

/ok-to-test sha=03c92279e12aeb4b0235d5cd878008fdf89e3013

naveensrinivasan
naveensrinivasan approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@naveensrinivasan naveensrinivasan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @evverx!

@olivekl Could you please review?

@github-actions
Copy link

Integration tests success for 03c92279e12aeb4b0235d5cd878008fdf89e3013

@evverx
Copy link
Contributor Author

evverx commented Nov 10, 2021

I'm not sure what TestSARIFOutput is but judging by https://github.com/ossf/scorecard/runs/4146238436?check_suite_focus=true it seems to have been failing since the latest commit was merged so I don't think it has anything to do with this PR.

azeemshaikh38
azeemshaikh38 approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@azeemshaikh38 azeemshaikh38 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@laurentsimon laurentsimon mentioned this pull request Nov 10, 2021
Merged
@laurentsimon
Copy link
Contributor

laurentsimon commented Nov 10, 2021 •
edited
Loading

I'm not sure what TestSARIFOutput is but judging by https://github.com/ossf/scorecard/runs/4146238436?check_suite_focus=true it seems to have been failing since the latest commit was merged so I don't think it has anything to do with this PR.

Thanks. I've merged a PR to fix it #1233

@laurentsimon
Copy link
Contributor

According to https://github.com/apps/lgtm-com "LGTM is a code analysis platform for identifying vulnerabilities early and preventing them from reaching production". It's used by systemd, lxc and a lot of other large open source projects. The check is still kind of broken in the sense that it fails to detect projects where every PR is analyzed by LGTM before getting merged but it's better than nothing I guess.

Can you elaborate? Function sastToolInCheckRuns does list merged PRs and look for Slug, see https://github.com/ossf/scorecard/blob/main/checks/sast.go#L138 Is this insufficient for LGTM? Is there some improvement we should make?

@evverx
Copy link
Contributor Author

evverx commented Nov 10, 2021

Is this insufficient for LGTM?

I'm not sure. I need to figure out why scorecard says that "23 commits out of 30 are checked with a SAST tool". It could be it has something to do with "neutral" checks where the source code wasn't changed and LGTM was skipped in a way.

@laurentsimon
Copy link
Contributor

Is this insufficient for LGTM?

I'm not sure. I need to figure out why scorecard says that "23 commits out of 30 are checked with a SAST tool". It could be it has something to do with "neutral" checks where the source code wasn't changed and LGTM was skipped in a way.

Thanks. Let's not block this PR on this problem. Can you create a tracking issue for it so we don't forget? If you're interested in looking into, help is appreciated!

@evverx
Copy link
Contributor Author

evverx commented Nov 10, 2021 •
edited
Loading

Looks like in PRs like systemd/systemd#21266 the status of LGTM is "neutral" because "LGTM.com did not detect changes to any C/C++ files". It can be fixed by not skipping PRs where cr.Conclusion is neutral:

+                       if cr.Conclusion != "success" && cr.Conclusion != "neutral" {
                                continue
                        }

With that patch applied scorecard says that "29 commits out of 30 are checked with a SAST tool", which is correct because apparently in systemd/systemd#21268 LGTM got lost somehow. Though I'm not sure flukes like that should affect the aggregate score.

@laurentsimon
Copy link
Contributor

Looks like in PRs like systemd/systemd#21266 the status of LGTM is "neutral" because "LGTM.com did not detect changes to any C/C++ files". It can be fixed by not skipping PRs where cr.Conclusion is neutral:

+                       if cr.Conclusion != "success" && cr.Conclusion != "neutral" {
                                continue
                        }

With that patch applied scorecard says that "29 commits out of 30 are checked with a SAST tool", which is correct because apparently in systemd/systemd#21268 LGTM got lost somehow. Though I'm not sure flukes like that should affect the aggregate score.

That sounds good. Please send us a PR and cc me on it, I'll LGTM it.

@laurentsimon
Copy link
Contributor

This PR is ready to merge code-wise. @olivekl we only need your ack for the documentation.

olivekl
olivekl approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@olivekl olivekl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks, @evverx!

@laurentsimon
Copy link
Contributor

LGTM! Thanks, @evverx!

Thanks.

@laurentsimon laurentsimon merged commit 6a2fb2e into ossf:main Nov 10, 2021
@evverx evverx mentioned this pull request Nov 10, 2021
Merged
@evverx evverx deleted the lgtm-SAST branch November 10, 2021 19:29
laurentsimon pushed a commit that referenced this pull request Nov 10, 2021
@evverx
SAST: no longer skip "neutral" checks (#1237)
5524c97 
Some SASTs like LGTM don't analyze PRs where code hasn't been changed,
which leads to their status being "neutral" there.

It's a follow up to #1232 (comment)

I'm not sure what to do about one-offs like the one
mentioned in #1232 (comment)
that shouldn't affect the aggregate score but it can probably
be fixed later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olivekl olivekl olivekl approved these changes

@laurentsimon laurentsimon laurentsimon approved these changes

@azeemshaikh38 azeemshaikh38 azeemshaikh38 approved these changes

@naveensrinivasan naveensrinivasan naveensrinivasan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@evverx @naveensrinivasan @laurentsimon @azeemshaikh38 @olivekl