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

Low severity vulnerability in @storybooks/cli #4241

Closed
mkretschek opened this issue Sep 27, 2018 · 13 comments
Closed

Low severity vulnerability in @storybooks/cli #4241

mkretschek opened this issue Sep 27, 2018 · 13 comments

Comments

@mkretschek
Copy link

Bug or support request summary

When installing my project's dependencies or running npm audit I get a vulnerability alert. The vulnerability is caused by the merge-dirs dependency which doesn't seem to be maintained anymore (last release was 3 years ago and the issues posted there doesn't seem to get any attention).

I get the following from npm install @storybook/cli:

$ npm install @storybook/cli
...
+ @storybook/cli@3.4.11
added 312 packages from 143 contributors and audited 7418 packages in 12.676s
found 1 low severity vulnerability
  run `npm audit fix` to fix them, or `npm audit` for details

From npm audit:

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low           │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ lodash                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=4.17.5                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @storybook/cli [dev]                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @storybook/cli > merge-dirs > inquirer > lodash              │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/577

Steps to reproduce

npm install @storybook/cli

Please specify which version of Storybook and optionally any affected addons that you're running

  • @storybook/cli 3.4.11
  • @storybook/cli 4.0.0-alpha.23

Affected platforms

@ndelangen
Copy link
Member

Thanks for reporting this!

We'll be looking for an alternative that's secure, do you happen to have any recommendations?

@ndelangen
Copy link
Member

Maybe this:
https://www.npmjs.com/package/merge-trees

@stale
Copy link

stale bot commented Oct 18, 2018

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Oct 18, 2018
@stale
Copy link

stale bot commented Nov 17, 2018

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!

@stale stale bot closed this as completed Nov 17, 2018
@JaceHensley
Copy link

I'm still seeing this problem. Any update on this? Thanks

@shilman shilman reopened this Apr 16, 2019
@stale stale bot removed the inactive label Apr 16, 2019
@shilman shilman added the cli label Apr 16, 2019
@stale
Copy link

stale bot commented May 7, 2019

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label May 7, 2019
@Reeska
Copy link

Reeska commented May 29, 2019

Hello,

Any news on this ?
There is a new vulnerability with moderate level now, this is going to be critical.

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ lodash                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=4.17.11                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @storybook/cli [dev]                                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @storybook/cli > merge-dirs > inquirer > lodash              │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/782                             │

@stale stale bot removed the inactive label May 29, 2019
@Stephanemw
Copy link
Contributor

I've updated Storybook's direct dependencies on lodash in another PR, will take a look at this transitive one as well if no-one else is on it yet

@Stephanemw Stephanemw self-assigned this May 29, 2019
@debel27
Copy link
Contributor

debel27 commented Jun 15, 2019

I think it would be better to get rid of this merge-dirs dependency altogether. It has not updated in 3 years and is still at version 0.2.1.

Looking at the code, the dependency is used only to synchronously copy some directory A in some directory B (the content of A overriding in case of conflicts).

I know fs-extra to exist for quite a while now. It might have some issues, but has no known vulnerabilities. If the library fits, a drop-in replacement with this library would be something in the lines of

from (existing code)

import path from 'path';
import mergeDirs from 'merge-dirs';
// ...
mergeDirs(path.resolve(__dirname, 'template/'), '.', 'overwrite');

to (new code)

import path from 'path';
import fse from 'fs-extra';
// ...
fse.copySync(path.resolve(__dirname, 'template/'), '.', {overwrite: true});

@shilman shilman added this to the 5.2.0 milestone Jun 15, 2019
@ndelangen
Copy link
Member

@debel27 Sounds like you would be able to open a PR changing this? Would be appreciated a lot!

@debel27
Copy link
Contributor

debel27 commented Jun 15, 2019

You got it!
PR

@shilman
Copy link
Member

shilman commented Jun 16, 2019

Huzzah!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.2.0-alpha.27 containing PR #7100 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Jun 16, 2019
@shilman
Copy link
Member

shilman commented Jul 31, 2019

Whoopee!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.1.10 containing PR #7100 that references this issue. Upgrade today to try it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants