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

Some security updates #792

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ed-flanagan
Copy link

@ed-flanagan ed-flanagan commented Apr 25, 2023

A GitHub security advisory for yaml12 (transitive dependency of cosmiconfig < 83) originally prompted me to draft this PR. However, sometime after drafting, the security advisory has further constrained the effected version boundary4, which removes the used cosmiconfig from scope.

Nevertheless, I had to configure Actions and such to trigger on my fork and figured I'd upstream the changes I made. Feel free to take/ignore whatever. I tried to chunk discrete changes into each commit. Felt they may be useful, besides the npm audit changes, largely just refactors and using some extra GitHub features


  • Reflect current versions
    • 14.x is the min supported/tested version. Reflect that everywhere
      • Update README
      • Update package files
    • Update package version to current release
  • Update GitHub CI Action
    • Move single OS out of matrix
    • Add 20.x to node versions
    • Add 20.x-exclusive codecov field to matrix (feel this is a more extensible conditional matrix pattern5)
    • Update actions/checkout to v3
    • Remove unused windows-only step
    • Update actions/setup-node to v3
    • Remove actions/setup-node name
    • Move CI env to job level
    • Split out monolithic build step into different runs6
    • Update codecov/codecov-action to v3
    • Update codecov/codecov-action conditional to codecov
  • Address npm audit
    • Upgrade some packages:
      • cosmiconfig (uses older yaml)
      • mocha
    • Update lockfile (transient deps)
  • Extend prettier coverage
    • Update package script wrapper
      • Remove file extension restriction
      • Ignore unknown files
    • Add a graphql file to ignores
    • Run formatter. Newly covered
      • .babelrc
      • *.jsx
      • *.tsx?
      • *.vue
  • Add Dependabot config
  • Add CodeQL config. Reported one possible item

Footnotes

  1. https://github.com/advisories/GHSA-f9xv-q969-pqx4

  2. https://www.npmjs.com/package/yaml

  3. https://github.com/cosmiconfig/cosmiconfig/blob/main/CHANGELOG.md#800

  4. https://github.com/github/advisory-database/commit/0852ba747a815ccac173afe1c96360f33125bc04

  5. https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#example-expanding-configurations

  6. https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-nodejs#using-the-nodejs-starter-workflow

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2023

Codecov Report

Merging #792 (bc6b10f) into main (b7a0428) will decrease coverage by 0.41%.
The diff coverage is 93.68%.

❗ Current head bc6b10f differs from pull request most recent head f868465. Consider uploading reports for the commit f868465 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #792      +/-   ##
==========================================
- Coverage   98.29%   97.88%   -0.41%     
==========================================
  Files          50       53       +3     
  Lines        1052     1080      +28     
==========================================
+ Hits         1034     1057      +23     
- Misses         18       23       +5     
Impacted Files Coverage Δ
src/constants.js 100.00% <ø> (ø)
src/parser/typescript.js 100.00% <ø> (ø)
src/parser/storybook.js 80.96% <80.96%> (ø)
src/parser/tsconfig.js 93.34% <93.34%> (ø)
src/detector/extract.js 100.00% <100.00%> (ø)
src/parser/sass.js 100.00% <100.00%> (ø)
src/parser/vue.js 90.00% <100.00%> (+10.00%) ⬆️
src/special/jest.js 100.00% <100.00%> (ø)
src/utils/index.js 100.00% <100.00%> (ø)
src/utils/module-root.js 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da616e7...f868465. Read the comment docs.

@ed-flanagan ed-flanagan force-pushed the security-updates branch 2 times, most recently from ab352b3 to 5508ad0 Compare April 25, 2023 22:21
Ed Flanagan added 5 commits April 25, 2023 15:22
* 14.x is the min supported/tested version. Reflect that everywhere
  * Update README
  * Update package files
* Update package version to current release
* Move single OS out of matrix
* Add 20.x to node versions
* Add 20.x-exclusive `codecov` field to matrix
* Update actions/checkout to v3
* Remove unused windows-only step
* Update actions/setup-node to v3
* Remove actions/setup-node name
* Move CI env to job level
* Split out monolithic build step into different runs
* Update codecov/codecov-action to v3
* Update codecov/codecov-action conditional to `codecov`
* Upgrade some packages
  * cosmiconfig
  * mocha
* Update lockfile (transient deps)
* Update package script wrapper
  * Remove file extension restriction
  * Ignore unknown files
* Add a graphql file to ignores
* Run formatter. Newly covered
  * .babelrc
  * *.jsx
  * *.tsx?
  * *.vue
@legobeat
Copy link
Contributor

legobeat commented May 14, 2023

@ed-flanagan There are some great updates here but I'm thinking perhaps splitting this up in a couple of PRs would help facilitate review and release.

For example, bumping the engines.node field is a breaking change implying a semver-major bump, while the npm audit changes should be releasable in a non-breaking 1.x update and adding new CI workflows could be nice but independent from any of this.

Here's how I would split this up:

  • Non-breaking dependency and devDependency updates
  • Current CI workflow fixes/updates
  • Extending/improving tests and linting
  • Add CodeQL and Dependabot
  • Increase package minimum node.js version (possibly alongside any dependency updates this bump unlocks)

What do you think?

@legobeat
Copy link
Contributor

Broke out non-breaking dependency updates and audit fix into #801 .

@znarf
Copy link
Collaborator

znarf commented Jun 9, 2023

Agree that this PR could be split into smaller chunks to help reviewability.

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

4 participants