8000 execa 1.0.0 flagged as insecure · Issue #1208 · shelljs/shelljs · GitHub
[go: up one dir, main page]
Skip to content

execa 1.0.0 flagged as insecure #1208

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
y-nk opened this issue Apr 8, 2025 · 14 comments
Closed

execa 1.0.0 flagged as insecure #1208

y-nk opened this issue Apr 8, 2025 · 14 comments
Labels
dependencies Pull requests that update a dependency file exec Issues specific to the shell.exec() API help wanted
Milestone
v0.10.0

Comments

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

we're now at execa@9.5.2 - a bump of 8 majors seems risky with overrides. any chance to work at this?

PS: i'm willing to help if needed.
@nfischer
Copy link
Member
nfischer commented Apr 8, 2025

Can you share details of the security vulnerability? If it's been disclosed, then please share a link. If the vulnerability is not yet disclosed, then please raise this with the execa maintainers first.

I won't say no to a PR to update execa as long as everything still works.

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

the vulnerability was found by aws docker image scanner, so it's all ok to share.

https://scout.docker.com/vulnerabilities/id/GMS-2020-2

Showed up in our report like this (nb: tw-email-bridge is our docker container name):

Image

after running pnpm why execa, shelljs was found as direct dependent.

Image

@nfischer
Copy link
Member
nfischer commented Apr 8, 2025

I don't think this is a vulnerability in shelljs. The report says:

Attackers could trick execa into executing arbitrary binaries. This behaviour is caused by the setting preferLocal=true which makes execa search for locally installed binaries and executes them. This vulnerability is usually only exploitable when using execa on a client-side LOCAL application.

We don't use { preferLocal: true } so I don't think our project is vulnerable.

If some other project is using shelljs and providing { preferLocal: true }, then that would be on them. Nothing we can do to stop this.

@nfischer
Copy link
Member
nfischer commented Apr 8, 2025

Also, how do you know that upgrading execa will fix the warning? It seems to me like execa still supports preferLocal the same as it always has. Are you sure that execa@2.0.0 doesn't trigger the same warning?

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

that's a good question, for which i don't have yet an answer. i can only try to build a project with updated execa and have it passed in the aws scanner again and see.

i would like to highlight that i have read the repo's source code and indeed i agree that shelljs itself don't use anything which would put any user at risk... but it is marked as such, raising issue in a few security dashboard products ; so it's not really that the code is insecure itself, but the auditors are quite basic and won't discard based on usage (as it's not really scalable from their PoV). their policy is "you have it in package-lock, you're at risk" which is what i wanted to report in this issue.

for FOSS projects i don't think it matters ; for SaaS like the one i work for, we have some security agreement which force us to fix those issues within a given time window or to seek help/support to do it.

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

after looking more in detail of the report in above url, they do provide the version which resolved the issue. that being said, 2.0.0 have dependency which also have other CVE attached, so maybe trying to upgrade to latest would be a better approach.

Image

@nfischer
Copy link
Member
nfischer commented Apr 8, 2025

Ok I found the execa change: sindresorhus/execa#314. Changelog says this did in fact go out in 2.0.0. https://github.com/sindresorhus/execa/releases/tag/v2.0.0

I will accept a patch to upgrade execa. To set expectations though, I will not commit to a timeline when I will release the patch. "Due in 1 day" is an absolutely insane timeline for any OSS bug.

@nfischer nfischer added help wanted exec Issues specific to the shell.exec() API dependencies Pull requests that update a dependency file labels Apr 8, 2025
@y-nk
Copy link
Contributor Author
y-nk commented Apr 8, 2025

@nfischer oh sorry i didn't mean to put pressure on you - the due in 1 day is on our side and is not something which reflects to you or this package. i/we do not expect such a short answer.

i will look into providing the patch PR, although i will look at doing it so that npm audit does not yield any vulnerability anymore - if it appears to be impossible (lots of breaking changes etc) i will let you know in this issue.

@nfischer
Copy link
Member
nfischer commented Apr 9, 2025

No worries, I understand that's an internal deadline. Just want to clarify that this probably won't be changed on our side for days or weeks. If the 1 day deadline on your side is a firm deadline, then you may want to prioritize finding a quick workaround to meet your deadline and then come back later if you're still interested in contributing the package update.

Unfortunately this would be a breaking change for us. I can still accept the PR, but you'll need to chase down the mailin package to have them upgrade shelljs once the update goes out.

@nfischer
Copy link
Member
nfischer commented Apr 9, 2025

Wait, something is wrong. You said the dependency path is mailin > shelljs > execa. This can't be right. mailin is 8 years old and it depends on shelljs "^0.5.1" https://github.com/Flolagale/mailin/blob/master/package.json#L45C17-L45C23

The "^0.5.1" semver pattern is not supposed to match version "0.9.1". "^" means you can only get patch updates if the major version is still zero. If your package manager installed shelljs v0.9.1, then your package manager is doing the wrong thing.

It's a miracle the mailin package still runs... we've shipped some breaking changes since 0.5.3, but I guess they only use shell.which() and shell.mkdir() so they lucked out. https://github.com/search?q=repo%3AFlolagale%2Fmailin+shell.&type=code

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

you're right, it's a miracle indeed ; or maybe not :) i have read its entire source code to address "can we use a npm override or not" and it was ok to force their shelljs^0.5.1 to be shelljs^0.9.2 and get the latest bug fixes and improvements.

we could also have made a fork, but looking at how many fork currently existing and still none of them are properly maintained, i made the call not to add one more of them to the mix and confuse the community further.

(if you have a time) you can check their issues and will see i've been looking into solutions to solve this as well as running mailin with node^20 (it was node^14 compliant). mailin itself is unmaintained for about 8y i think so we had no other choice than adding a override entry to our package.json. nobody reads the issue, nobody pushes updates.

i could very well add one more override of execa but execa is not a direct dependency of mailin but rather shelljs and since it's still maintained i thought there's a chance to kindly ask for an upgrade and make the whole community beneficiate from this bump, while it was not possible for mailin because nobody seems to care anymore.

@y-nk y-nk mentioned this issue Apr 10, 2025
Closed
@nfischer
Copy link
Member

you're right, it's a miracle indeed ; or maybe not :) i have read its entire source code to address "can we use a npm override or not" and it was ok to force their shelljs^0.5.1 to be shelljs^0.9.2 and get the latest bug fixes and improvements.

Well, I'm impressed. I didn't know you could even do that!

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

@nfischer it was added in npm since v9 (i think) under packageJson.overrides and still exists in pnpm as packageJson.pnpm.overrides. very neat last resort solution!

@nfischer nfischer added this to the v0.10.0 milestone May 9, 2025
@nfischer
Copy link
Member
nfischer commented May 9, 2025

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

@nfischer nfischer closed this as completed May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file exec Issues specific to the shell.exec() API help wanted
Projects
None yet
Milestone
v0.10.0
Development

No branches or pull requests
2 participants
@y-nk @nfischer
0