8000 bump execa by y-nk · Pull Request #1216 · shelljs/shelljs · GitHub
[go: up one dir, main page]
Skip to content

bump execa #1216

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 13 commits into from
Closed

bump execa #1216

wants to merge 13 commits into from

Conversation

y-nk
Copy link
Contributor
@y-nk y-nk commented Apr 10, 2025 •
edited
Loading

following #1208

A few things to notice:

  1. i've tried my best to bump the versions one by one major so that i could follow up and fix tests, propose a list of readable commits so that it's easier to review and follow. i wanted to go as far as i could into updating deps (more on that below), as usually keeping deps fresh is less prone to this kind of event (cve and such)

  2. the fact that i had to provide a few fixes for execa@2 and @3 shows that an npm override wouldn't cut it and there's no further workaround than patch-package after this PR.

  3. i have hit a major barrier since execa moved to be a esm "only" module since v6. installing v6, running tests won't pass having issues such as:

 Uncaught exception in test/cmd.js

  ~/shelljs/src/cmd.js:1

   1: var execa = require('execa');
   2: var common = require('./common');

  Error [ERR_REQUIRE_ESM]: require() of ES Module ~/shelljs/node_modules/execa/index.js from ~/shelljs/src/cmd.js not supported.
  Instead change the require of index.js in ~/shelljs/src/cmd.js to a dynamic import() which is available in all CommonJS modules.

that being said, execa@5 does not yield any entry in the audit report, so i guess it's good to leave it at v5.

  1. After this point, I have tried to drop commits backwards and tested npm audit --omit=dev for every major and report says execa is free of vulnerabilities since its v2 (here) so if you prefer avoiding the fixes related to v3, i'm happy to drop the unnecessary commits and leave it at v2.

  2. I don't own a win32 machine to confirm that the change i made to the commandNotFound function is proper (v3 and above) so if possible to have a test made that'd be best.

Not related to PR

when running npm install, stdout shows that a few dependencies are also marked as insecure by npm audit, mostly from cross-spawn widely used as well as trim-newlines. they are devDependencies so it is less likely to be a problem (since production code should normally npm i --prod) so i think it's ok to leave them like that.

image

cross-spawn  <6.0.6
Severity: high
Regular Expression Denial of Service (ReDoS) in cross-spawn - https://github.com/advisories/GHSA-3xgq-45jj-v275
fix available via `npm audit fix --force`
Will install ava@6.2.0, which is a breaking change
node_modules/term-size/node_modules/cross-spawn
  execa  0.5.0 - 0.9.0
  Depends on vulnerable versions of cross-spawn
  node_modules/term-size/node_modules/execa
    term-size  1.0.0 - 1.2.0
    Depends on vulnerable versions of execa
    node_modules/term-size
      boxen  1.2.0 - 3.2.0
      Depends on vulnerable versions of term-size
      node_modules/boxen
        update-notifier  0.2.0 - 5.1.0
        Depends on vulnerable versions of boxen
        Depends on vulnerable versions of latest-version
        node_modules/update-notifier
          ava  0.1.0 - 4.0.0-rc.1
          Depends on vulnerable versions of meow
          Depends on vulnerable versions of update-notifier
          node_modules/ava

got  <11.8.5
Severity: moderate
Got allows a redirect to a UNIX socket - https://github.com/advisories/GHSA-pfrx-2q88-qq97
fix available via `npm audit fix --force`
Will install ava@6.2.0, which is a breaking change
node_modules/got
  package-json  <=6.5.0
  Depends on vulnerable versions of got
  node_modules/package-json
    latest-version  0.2.0 - 5.1.0
    Depends on vulnerable versions of package-json
    node_modules/latest-version

trim-newlines  <3.0.1
Severity: high
Uncontrolled Resource Consumption in trim-newlines - https://github.com/advisories/GHSA-7p7h-4mm5-852v
fix available via `npm audit fix --force`
Will install ava@6.2.0, which is a breaking change
node_modules/trim-newlines
  meow  3.4.0 - 5.0.0
  Depends on vulnerable versions of trim-newlines
  Depends on vulnerable versions of yargs-parser
  node_modules/meow

yargs-parser  6.0.0 - 13.1.1
Severity: moderate
yargs-parser Vulnerable to Prototype Pollution - https://github.com/advisories/GHSA-p9pc-299p-vxgp
fix available via `npm audit fix --force`
Will install ava@6.2.0, which is a breaking change
node_modules/yargs-parser

12 vulnerabilities (4 moderate, 8 high)

@y-nk y-nk changed the title bump of execa bump execa Apr 10, 2025
nfischer
nfischer requested changes Apr 10, 2025
View reviewed changes
@codecov Codecov
Copy link
codecov bot commented Apr 10, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.47%. Comparing base (67a26e2) to head (7e69a87).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
- Coverage   97.47%   97.47%   -0.01%     
==========================================
  Files          35       35              
  Lines        1468     1467       -1     
==========================================
- Hits         1431     1430       -1     
  Misses         37       37

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nfischer
Copy link
Member

when running npm install, stdout shows up that a few dependencies are also marked as insecure by npm audit, mostly from cross-spawn widely used as well as trim-newlines. they are devDependencies so it is less likely to be a problem (since production code should normally npm i --prod) so i think it's ok to leave them like that.

Yeah, I'm aware of those. I'm working on updating ava which will fix everything I'm aware of. But let's leave that out of the scope of this PR.

@nfischer nfischer added this to the v0.10.0 milestone Apr 10, 2025
nfischer
nfischer reviewed Apr 10, 2025
View reviewed changes
@nfischer nfischer added chore exec Issues specific to the shell.exec() API dependencies Pull requests that update a dependency file labels Apr 10, 2025
nfischer
nfischer requested changes Apr 10, 2025
View reviewed changes
y-nk added 2 commits April 20, 2025 21:08
@y-nk
bump execa to major 2
435d102 
apply fixes on execa

bump execa to v3

fix source and tests

bump execa to v4

bump execa to v5

review comments

Update src/cmd.js

use standard posix 1 as code for default unknown error exit code

fix after rebase
@y-nk
fix after rebase... again...
bf69ca0
n 8000 fischer
nfischer requested changes Apr 20, 2025
View reviewed changes
nfischer
nfischer requested changes Apr 21, 2025
View reviewed changes
y-nk
y-nk commented Apr 22, 2025
View reviewed changes
y-nk
y-nk commented Apr 22, 2025
View reviewed changes
@y-nk
Copy link
Contributor Author
y-nk commented Apr 23, 2025

@nfischer all is fixed asrequested!

nfischer
nfischer reviewed Apr 25, 2025
View reviewed changes
@nfischer
Copy link
Member

I think you can fix the test failures with something like #1223. The critical part is inside isExecaInternalError(). The failure took quite a bit of debugging and the only way I could think to solve this was to just upload a PR to test on CI. I'd still prefer to merge your PR though if you can fix it up.

@nfischer
Copy link
Member

Once you rebase, then I can retrigger CI to run

@y-nk
Copy link
Contributor Author
y-nk commented Apr 25, 2025

i've fixed the new conflicts.

nfischer
nfischer reviewed Apr 25, 2025
View reviewed changes
y-nk
y-nk commented Apr 25, 2025
View reviewed changes
nfischer pushed a commit that referenced this pull request Apr 30, 2025
@y-nk @nfischer
bump execa (#1216)
e48e919 
This update execa to v5.
@nfischer
Copy link
Member

Merged as e48e919

Thank you for the contribution!

@nfischer nfischer closed this Apr 30, 2025
@y-nk
Copy link
Contributor Author
y-nk commented Apr 30, 2025

@nfischer thank you. i noticed the package.json version is not bumped nor a new version was published on the registry. is that a separate process?

@nfischer
Copy link
Member
nfischer commented May 2, 2025

I push releases separately. I'm going to skim through some of the other GitHub issues and I expect I can release v0.10 shortly.

@nfischer
Copy link
Member
nfischer commented May 9, 2025

Released in shelljs v0.10. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfischer nfischer nfischer requested changes

Assignees
No one assigned
Labels
chore dependencies Pull requests that update a dependency file exec Issues specific to the shell.exec() API
Projects
None yet
Milestone
v0.10.0
Development

Successfully merging this pull request may close these issues.
2 participants
@y-nk @nfischer
0