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

Add codeql-analysis.yml GitHub Actions Workflow #7961

Merged
merged 1 commit into from Jun 30, 2022

Conversation

JLLeitschuh
Copy link
Contributor

Hello! Your friendly neighborhood security researcher here!

GitHub is slowly (not entirely there yet) migrating away from LGTM.com
to establishing a way for security researchers to work with CodeQL databases
fully through GitHub.com. This 'new' way requires that databases get built
via GitHub actions and uploaded to GitHub.

The old way, supported by the .lgtm.yaml file will continue to exist for a while
(and I'll continue to use it), but as a researcher, if the Jetty project also built a
CodeQL database and uploaded it, I'd find it incredibly useful.

In summary, this PR adds a GitHub action to build a CodeQL database and runs
GitHub's CodeQL analysis tool which audits the code for security vulnerabilities.


on:
push:
branches: [ 'jetty-[1-9]?[0-9].[0-9].x', 'feat/JLL/codeql-support' ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just hardcode jetty-10.0.x, jetty-11.0.x, and jetty-12.0.x
Note: Jetty 9.x is being deprecated soon. See: #7958

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you support branching names like 9.4.x, and will use that pattern for future development, wouldn't it be this?
[ 'jetty-10.[1-9]?[0-9].x', 'jetty-11.[1-9]?[0-9].x', 'jetty-12.[1-9]?[0-9].x' ]

@JLLeitschuh JLLeitschuh force-pushed the feat/JLL/codeql-support branch 3 times, most recently from 221e5c4 to 584cb28 Compare May 6, 2022 19:54
@JLLeitschuh JLLeitschuh marked this pull request as ready for review May 6, 2022 20:04
@JLLeitschuh
Copy link
Contributor Author

Once this is merged, the maintainers of the eclipse jetty project will be able to see the alerts here.
https://github.com/eclipse/jetty.project/security/code-scanning

Also, merging this will establish a new baseline, and any PRs that unintentionally introduce a new security vulnerability will be flagged 😄

@JLLeitschuh JLLeitschuh requested a review from joakime May 6, 2022 20:07
@JLLeitschuh
Copy link
Contributor Author

I think this PR is in a complete state. The one check 'Code scanning results / CodeQL' will not pass this time, but for other PRs it will pass because it will compare against what's on the default branch as a baseline.

@joakime
Copy link
Contributor

joakime commented May 6, 2022

This is interesting, but we'll need some time to absorb what this means to our processes.

@JLLeitschuh
Copy link
Contributor Author

If you don't want to run this job on PRs in general, we can always remove these lines:

https://github.com/eclipse/jetty.project/pull/7961/files#diff-63bd641104d10e25f141d518a16b22a151d125e12701df2f9e79734b23b90188R6-R8

strategy:
fail-fast: false
matrix:
language: [ 'java', 'javascript' ]
Copy link
Member

Choose a reason for hiding this comment

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

we do not have any javascript here. So there is no real need of a matrix

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have javascript, in the demo apps. Check the bottom of the "files changed" tab in this PR to see where it's finding bugs.

Copy link
Member

Choose a reason for hiding this comment

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

ah yup demos 🤦

branches: [ 'jetty-10.[1-9]?[0-9].x', 'jetty-11.[1-9]?[0-9].x', 'jetty-12.[1-9]?[0-9].x' ]
pull_request:
# The branches below must be a subset of the branches above
branches: [ 'jetty-10.[1-9]?[0-9].x', 'jetty-11.[1-9]?[0-9].x', 'jetty-12.[1-9]?[0-9].x' ]
Copy link
Member

Choose a reason for hiding this comment

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

not sure we want to run this on PRs.
just a weekly on hardcoded branches might be enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with PRs, it will catch accidentally introduced bugs.

@olamy
Copy link
Member

olamy commented May 8, 2022

This is interesting, but we'll need some time to absorb what this means to our processes.

correct because we already some reports we do nothing about.
so here we will have to mark the reported errors or they will be sitting here and we will do nothing and this will be YASRWDC (Yet Another Static Report We Don't Care :) )

@JLLeitschuh
Copy link
Contributor Author

so here we will have to mark the reported errors or they will be sitting here and we will do nothing and this will be YASRWDC (Yet Another Static Report We Don't Care :) )

Not a bad thing, but yes. I'm currently discussing with the GitHub team if there's a way to build the database without reporting query results. They say that there is, but it's a bit of a hack.

@joakime joakime merged commit ea3dd1e into jetty:jetty-10.0.x Jun 30, 2022
@JLLeitschuh JLLeitschuh deleted the feat/JLL/codeql-support branch July 1, 2022 14:04
@olamy
Copy link
Member

olamy commented Jul 21, 2022

is it possible to exclude src/test from the scanning?

@JLLeitschuh
Copy link
Contributor Author

It is, but I don't remember how to off hand. I suggest consulting the documentation

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

Successfully merging this pull request may close these issues.

None yet

3 participants