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

ci: security patching upgrades #156

Merged
merged 14 commits into from
Feb 3, 2022
Merged

ci: security patching upgrades #156

merged 14 commits into from
Feb 3, 2022

Conversation

xuatz
Copy link
Contributor

@xuatz xuatz commented Feb 3, 2022

Summary

Just small deps bumps which I think is pretty trivial.

Details

Had to rollback the eslint upgrade because v8 introduced some breaking changes for eslint-config-molindo.
Jest seems to be working fine as well.

Feel free to squash the commits 👍

Notes

I don't expect this PR to get merged as-is, but just want to show you the stuff I'm doing.
Let me know what do you want, or if its shorter, what you don't want, then I will clean it up to make a PR for amannn/action-semantic-pull-request

Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

Thanks for your suggestions! I've left some comments.

Also it seams like a pnpm lockfile was commited. Can you revert that and make sure the yarn lockfile is up to date?

.eslintrc.js Outdated
extends: [
"eslint:recommended",
"plugin:node/recommended"
],
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't change the ESLint config.

# the `language` matrix defined below to confirm you have the correct set of
# supported CodeQL languages.
#
name: "CodeQL"
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, I didn't know about CodeQL. Is there a preview of what kind of things this would report? I'm only concerned if it would add noise like it's often the case with npm audit and Dependabot (see e.g. npm audit: Broken by Design).

If it provides helpful suggestions then I'm open. I'm wondering though if it has to run in a cron job? Since it analyses the code, doesn't it make sense to run it when code changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I don't know too much as well ^^

I just threw all these scanners because my team is complaining about "vulnerabilities". I'm not sure if they don't raise false positives like the examples given in that blog post (Dan the man!)

Anyways it looks like this, and so far only the generated files are getting flagged. I added a config file to ignore dist so nothing should show up in the main(this) repository.

image

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, thanks for the additional info then! 🙂

Well if there's no strong reason to introduce this currently, then I'd wait with the code scanners.

# This workflow integrates njsscan with GitHub's Code Scanning feature
# nodejsscan is a static security code scanner that finds insecure code patterns in your Node.js applications

name: njsscan sarif
Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't this do the same thing as CodeQL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just threw the whole kitchen sink here ^^

I can remove it if you don't want this scanner.

"jest": "24.9.0",
"eslint": "8.8.0",
"eslint-plugin-node": "^11.1.0",
"jest": "27.4.7",
Copy link
Owner

Choose a reason for hiding this comment

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

Upgrading dependencies is fine, thanks! But please don't change the ESLint config.

dependabot bot and others added 14 commits February 3, 2022 18:26
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@xuatz xuatz force-pushed the master branch 2 times, most recently from a730baa to faf89b4 Compare February 3, 2022 10:34
@xuatz xuatz changed the title ci: security patching upgrades + some opinions ci: security patching upgrades Feb 3, 2022
@xuatz xuatz marked this pull request as ready for review February 3, 2022 10:46
@amannn amannn merged commit 16c6cc6 into amannn:master Feb 3, 2022
@amannn
Copy link
Owner

amannn commented Feb 3, 2022

Thanks a lot @xuatz!

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

🎉 This PR is included in version 4.0.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

2 participants