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

Fix failing OSSAR static analysis reporting #5681

Closed
martgil opened this issue Apr 17, 2024 · 8 comments · Fixed by #5716 or #5735
Closed

Fix failing OSSAR static analysis reporting #5681

martgil opened this issue Apr 17, 2024 · 8 comments · Fixed by #5716 or #5735
Assignees
Milestone

Comments

@martgil
Copy link
Collaborator

martgil commented Apr 17, 2024

From Roma:

By the way, we have also some automated scanning configured in flowcrypt-browser repo, can also check opened issues at https://github.com/FlowCrypt/flowcrypt-browser/security/code-scanning and find why ESLint fails there.

Reference: https://github.com/FlowCrypt/flowcrypt-security/issues/277

@martgil martgil self-assigned this Apr 17, 2024
@martgil
Copy link
Collaborator Author

martgil commented Apr 18, 2024

Hello @sosnovsky, I believe I have already found the root cause of this issue. The GitHub action OSSAR is still using 'node 16', from which ESLint's requirements doesn't support.

OSSAR action file:
https://github.com/github/ossar-action/blob/fae13e456b9973657a670eef6bccc3a4c2b5153d/action.yml#L16

@sosnovsky
Copy link
Collaborator

Hello @martgil, thanks for your investigation, hope they'll get it merged soon!

@martgil
Copy link
Collaborator Author

martgil commented May 16, 2024

I noticed that there's something wrong going on only for push event's where checking out the repo doesn't seems so well and marked as not finished if we take a look at the GitHub action overview.

Consider the following example:

https://github.com/FlowCrypt/flowcrypt-browser/actions/runs/9095908805/job/25036158252 - its from the master branch.

But in general most PRs should experience such issue, where OSSAR with "push" event tags does have the same issues where tags "OSSAR / OSSAR-Scan (pull_request)" finishes correctly.

image

There's no way we can review the logs for that step.

@martgil
Copy link
Collaborator Author

martgil commented May 16, 2024

I think the GitHub security tab considers a "skipped" job/tasks as an error so it fails -- I'll check that case too. On top of this, a push event OSSAR shouldn't be running on pull_request event.

sosnovsky pushed a commit that referenced this issue May 22, 2024
* Update ossar-analysis.yml

* Checkout github.sha on push

* Add condition for push event

* Fix typo

* Make checkout repository failsafe

* Update ossar-analysis.yml based on the update template from GitHub

* Limit OSSAR push check on master branch

* Enforce latest updates

* Update OSSAR to run on pull_requests

* Use ubuntu-latest

* Specify master branch

---------

Co-authored-by: martgil <[email protected]>
@martgil martgil reopened this May 22, 2024
@martgil
Copy link
Collaborator Author

martgil commented May 22, 2024

Hello @sosnovsky I had to re-open this GitHub issue since the error on the Security tab didn't disappears. Meanwhile, I have completely tried to create the same OSSAR GitHub action on a test organization that I created to verify this and while both has the same similar OSSAR configuration, the error only appears on the FlowCrypt browser extension report. So I think a maintainance should have done on the repo's Settings under code scanning, specially it would help if we could have remove other define workflows where file's didn't exists in .github/workflows within the FlowCrypt browser extension's report - https://github.com/FlowCrypt/flowcrypt-browser/security/code-scanning/tools/CodeQL/status

I have invited you the test organization that i made to cross check this - https://github.com/An-Example-Org/sample-ossar/security.

@martgil
Copy link
Collaborator Author

martgil commented May 22, 2024

Another option to try could be re-initializing the OSSAR Github Action - since it works very well with against my test repo under the test organization.

@sosnovsky
Copy link
Collaborator

It seems eslint fails with some errors when running in OSSAR Action
Screenshot 2024-05-22 at 16 21 14

Maybe it happens because OSSAR Action uses outdated eslint 7.32.0, while our project is configured to work with eslint v8. I also re-checked OSSAR and we need only eslint from this action, as other tools there are for checking Python and C code. So it'll should be simpler to use just ESLint action - https://github.com/FlowCrypt/flowcrypt-browser/new/master?filename=.github%2Fworkflows%2Feslint.yml&workflow_template=code-scanning%2Feslint

@martgil
Copy link
Collaborator Author

martgil commented May 23, 2024

Thanks @sosnovsky, I understand. I'll be more efficient by using ESLint action instead of OSSAR for a well-defined project like ours. I'll check it out.

ioanmo226 pushed a commit that referenced this issue May 27, 2024
* Update ossar-analysis.yml

* Checkout github.sha on push

* Add condition for push event

* Fix typo

* Make checkout repository failsafe

* Update ossar-analysis.yml based on the update template from GitHub

* Limit OSSAR push check on master branch

* Enforce latest updates

* Update OSSAR to run on pull_requests

* Use ubuntu-latest

* Specify master branch

---------

Co-authored-by: martgil <[email protected]>
@sosnovsky sosnovsky added this to the 8.5.6 milestone May 27, 2024
sosnovsky pushed a commit that referenced this issue May 27, 2024
* Delete ossary-analysis.yml

* Add ESLint GitHub workflow

* Install ESLint version 8.57.0

* Install eslint-formatter-sarif version 3.1.0

* Fix syntax error

* Use upload-sarif version 3

* Use existing npm script "test_eslint"

* Update "Run ESLint" commands

* Temporary hotfix: add eslint-sarif-formatter.js

* Temporary hotfix: use modified version of @microsoft/eslint-formatter-sarif

* Add debug code

* Add write permission on actions tab

* Add write permissions to contents

* Add debug code: check file writing capability

* Add debug code: add continue-on-error

* Fix typo

* Add debug code: add alternative output writing method

* Add debug code: read eslint-results.sarif

* Add debug code: try other file writing method

* Add debug code: add more debug code

* Install utf8 module

* Add reported missing node module

* cleanup

* Specify pull requests on master branch

* cleanup

* Add write permissions on contents

* cleanup

* cleanup

* Replace reduce() with for...of

* PR review: add SARIF_ESLINT_IGNORE_SUPPRESSED parameter for eslint sarif formatter

* PR review: Add test_eslint_ci for ESLint test

* Cleanup

* Install eslint-formatter-sarif

---------

Co-authored-by: martgil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment