-
Notifications
You must be signed in to change notification settings - Fork 9
CVE dependencies and github actions #15
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
base: master
Are you sure you want to change the base?
Changes from all commits
77881ea
fb8278f
14409da
6062597
ebf5c32
e1ffc95
05b52f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # To get started with Dependabot version updates, you'll need to specify which | ||
| # package ecosystems to update and where the package manifests are located. | ||
| # Please see the documentation for all configuration options: | ||
| # https://docs.github.com/github/administering-a-repository/configuration-options-for-dependency-updates | ||
|
|
||
| version: 2 | ||
| updates: | ||
| - package-ecosystem: "npm" | ||
| directory: "/" # Location of package manifests | ||
|
nmccready marked this conversation as resolved.
|
||
| schedule: | ||
| interval: "daily" | ||
| cooldown: | ||
| default-days: 7 | ||
| groups: | ||
| all-dependencies: | ||
| patterns: | ||
| - "*" | ||
| commit-message: | ||
| prefix: fix | ||
| prefix-development: chore | ||
| include: scope | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's drop this completely. Publishing from CI is an attack vector for the spreading of worms in package registries. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,28 @@ | ||||||
| # disabling until we get 2FA working in github | ||||||
| name: publish | ||||||
|
|
||||||
| on: | ||||||
| push: | ||||||
| tags: | ||||||
| - "v*" | ||||||
|
|
||||||
| jobs: | ||||||
| tests: | ||||||
| uses: ./.github/workflows/tests.yml | ||||||
| publish-npm: | ||||||
| needs: [tests] | ||||||
| runs-on: ubuntu-latest | ||||||
| permissions: | ||||||
| id-token: write # Required for OIDC | ||||||
| steps: | ||||||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||||||
| - name: Use Node.js ${{ matrix.node-version }} | ||||||
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | ||||||
| with: | ||||||
| node-version: '20.x' | ||||||
| registry-url: 'https://registry.npmjs.org' | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
typo
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Were you meaning to get rid of both quotes? |
||||||
| - name: Publish to npm | ||||||
| run: | # npm 11.15.1 for OIDC support | ||||||
| npm install -g npm@11 | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps use a newer node to publish to avoid needing to install an unpinned npm version
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, currently still running with mainly 20 for most things and older npm, only recently moved to 11 NPM for OIDC support. |
||||||
| npm ci | ||||||
| npm publish | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can set up github branch protection rules that do this instead of adding workflows and dependencies.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Example? I know this works, been using this for a while. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand branch protection rules can validate commit messages, but the workflow approach has a few advantages: it's visible in the repo, works consistently across forks, and provides clear CI feedback. That said, if you'd prefer branch protection, I can remove the commitlint workflow and dependencies — just let me know your preference as the maintainer.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this workflow |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| name: commitlint | ||
|
|
||
| on: | ||
| workflow_call: | ||
| push: | ||
| pull_request: | ||
| branches: ["master"] | ||
|
|
||
| jobs: | ||
| commitlint: | ||
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| - name: Install required dependencies | ||
| run: | | ||
| sudo apt update | ||
| sudo apt install -y sudo | ||
| sudo apt install -y git curl | ||
|
|
||
| - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3 | ||
| with: | ||
| node-version: 20 | ||
| - name: node setup | ||
| run: npm i | ||
| - name: Print versions | ||
| run: | | ||
| git --version | ||
| node --version | ||
| npm --version | ||
| npx commitlint --version | ||
|
|
||
| - name: Validate current commit (last commit) with commitlint | ||
| if: github.event_name == 'push' | ||
| run: npx commitlint --last --verbose | ||
|
|
||
| - name: Validate PR commits with commitlint | ||
| if: github.event_name == 'pull_request' | ||
| run: npx commitlint --from ${{ github.event.pull_request.head.sha }}~${{ github.event.pull_request.commits }} --to ${{ github.event.pull_request.head.sha }} --verbose |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this. We'll add release-please if it becomes necessary. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| name: release | ||
|
|
||
| on: | ||
| push: | ||
| branches: ["master"] | ||
| tags-ignore: ['**'] | ||
|
|
||
| jobs: | ||
| tests: | ||
| uses: ./.github/workflows/tests.yml | ||
| tag-release: | ||
| runs-on: ubuntu-latest | ||
| needs: [tests] | ||
| steps: | ||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||
| with: # important, must be defined on checkout to kick publish (defining in setup/node doesn't work) | ||
| token: ${{ secrets.GH_TOKEN }} | ||
|
Comment on lines
+16
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All my projects use release-please to avoid this. It opens a PR so the merge commit triggers CI correctly.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. URL to copy? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked into release-please — it's a solid option! The current approach with commit-and-tag-version is simpler (no extra GH App/action needed) and fits the existing workflow, but I'm open to switching if you prefer release-please. For reference: https://github.com/googleapis/release-please-action The trade-off is release-please creates a 'release PR' which requires an extra merge step, while the current approach auto-tags on master push. Both are valid — your call as maintainer. |
||
| - name: Use Node.js ${{ matrix.node-version }} | ||
| uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3 | ||
| with: | ||
| node-version: '20.x' | ||
| cache: "npm" | ||
|
|
||
| - name: tag release | ||
| run: | | ||
| # ignore if commit message is chore(release): ... | ||
| if [[ $(git log -1 --pretty=%B) =~ ^chore\(release\):.* ]]; then | ||
| echo "Commit message starts with 'chore(release):', skipping release" | ||
| exit 0 | ||
| fi | ||
| git config --local user.email "creadbot@github.com" | ||
| git config --local user.name "creadbot_github" | ||
| set -v | ||
| npm ci | ||
| npx commit-and-tag-version | ||
| git push | ||
| git push --tags | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,24 @@ | ||||
| name: tests | ||||
|
|
||||
| on: | ||||
| workflow_call: | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This can be useful for debugging but let us keep this super focused. |
||||
| pull_request: | ||||
| branches: ["master"] | ||||
|
|
||||
| jobs: | ||||
| test: | ||||
| strategy: | ||||
| matrix: | ||||
| node-version: ['20.x', '22.x', '24.x'] | ||||
| runs-on: ubuntu-latest | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want Windows and Mac in the matrix since we are losing appveyor |
||||
| steps: | ||||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||||
| - name: Use Node.js ${{ matrix.node-version }} | ||||
| uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3 | ||||
| with: | ||||
| node-version: ${{ matrix.node-version }} | ||||
| cache: "npm" | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No cache please. While uncommon, cache poisoning has been used in supply chain attacks |
||||
| - run: npm ci | ||||
| - run: npm run lint | ||||
| - run: npm audit --omit dev | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could take or leave the audit. npm audit is really shitty and flags things that aren't actually an issue. Socket does it better but that is out of scope for this PR. |
||||
| - run: npm test | ||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -34,3 +34,5 @@ node_modules | |||||
| test/fixtures/tmp | ||||||
| test/fixtures/out | ||||||
| .nyc_output | ||||||
|
|
||||||
| # package-lock.json # OIDC | ||||||
|
Comment on lines
+37
to
+38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think it is common practice to check in the package-lock these days so there is no need to add it commented out with a comment to why. |
||||||
This file was deleted.
This file was deleted.
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete this |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| module.exports = { | ||
| extends: ['@commitlint/config-conventional'], | ||
| rules: { | ||
| 'body-max-line-length': [2, 'always', 200], | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| const gulpConfig = require('eslint-config-gulp'); | ||
|
|
||
| module.exports = [ | ||
| { | ||
| files: ["test/fixtures/*.js"], | ||
| rules: { | ||
| "no-unused-vars": "off", | ||
| }, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this from this PR specifically. I am not a fan of the dependabot workflow and think Renovate Dependency Dashboard is a better pattern but that requires a lot more effort and a longer discussion.