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

Fix trim vulnerability #20371

Merged
merged 2 commits into from
Dec 23, 2022
Merged

Fix trim vulnerability #20371

merged 2 commits into from
Dec 23, 2022

Conversation

timur-svoboda
Copy link
Contributor

@timur-svoboda timur-svoboda commented Dec 21, 2022

trim@0.0.1 has a known vulnerability with high severity: https://security.snyk.io/vuln/SNYK-JS-TRIM-1017038.

Here is the path to trim in dependencies of the ./code folder before this commit:

└─ @storybook/root@workspace:.
   └─ @storybook/linter-config@npm:2.5.0 [cfb4f] (via npm:^2.5.0 [cfb4f])
      └─ remark-cli@npm:8.0.1 (via npm:^8.0.0)
         └─ remark@npm:12.0.1 (via npm:^12.0.0)
            └─ remark-parse@npm:8.0.3 (via npm:^8.0.0)
               └─ trim@npm:0.0.1 (via npm:0.0.1)

Issue: #14603

What I did

Upgrade @storybook/linter-config to the latest version (3.1.2) to fix the trim vulnerability issue.

Also, the latest version of @storybook/linter-config was used in the ./scripts folder before this commit.

How to test

I checked that there is no trim in the dependencies in the ./code folder after this commit. You can check it by running the following.

yarn why -R trim
  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

Upgrade `@storybook/linter-config` to the latest version (`3.1.2`) to
fix the `trim` vulnerability issue.

`trim@0.0.1` has a known vulnerability with high severity:
https://security.snyk.io/vuln/SNYK-JS-TRIM-1017038.

Here is the path to `trim` before this commit:

```
└─ @storybook/root@workspace:.
   └─ @storybook/linter-config@npm:2.5.0 [cfb4f] (via npm:^2.5.0 [cfb4f])
      └─ remark-cli@npm:8.0.1 (via npm:^8.0.0)
         └─ remark@npm:12.0.1 (via npm:^12.0.0)
            └─ remark-parse@npm:8.0.3 (via npm:^8.0.0)
               └─ trim@npm:0.0.1 (via npm:0.0.1)
```

I checked that there is no `trim` in the dependencies in the __./code__
folder after this commit. You can check it by running the following.

```sh
yarn why -R trim
```

The latest version of `@storybook/linter-config` is also used in the
__./scripts__ folder.
@shilman shilman added the build Internal-facing build tooling & test updates label Dec 22, 2022
@ndelangen
Copy link
Member

Thanks!

...this is only a devDependency (we do not expose this risk to users)

I'll see if I can get this upgraded soon.. I suspect some linting issues might come up?

@timur-svoboda
Copy link
Contributor Author

Thanks!

...this is only a devDependency (we do not expose this risk to users)

I'll see if I can get this upgraded soon.. I suspect some linting issues might come up?

Hi!

Issues

What kind of issues? Do you mean that Prettier, ESLint, or Remark will not work or their work will be different from the current one?

If the former, I checked that they work in the ./code folder and in the Husky pre-commit hook.

If the second one.

I found only two places in the project where the @storybook/linter-config package is used: ./code/.remarkrc.js and ./scripts/prettier.config.js. Probably, the ESLint config from @storybook/linter-config is not used.

There are no differences in Prettier and Remark configs between the latest version (3.1.2.) and the current version used in this repo (2.5.0): https://github.com/storybookjs/linter-config/compare/v2.5.0..6885fcd77e04954f91dacea3f8e18f252b035c3a

Although, there are upgradings of dependencies related to remark. But remark-preset-lint-recommended is not changed: you can compare versions here:

Given all of the above, I can assume that the work of linting in the project will not change. But I didn't find a way to verify this..

Maybe you meant some other issues?

Security

You may be right that there are no risks. But even if it is a development dependency, it may imply some risks. For example, @babel/core is a development dependency, but it transforms your source code, so it may create vulnerabilities in your production code.

In our case, I don't think that this vulnerability definitely may create some vulnerabilities in the production code. But I think it is better to eliminate vulnerabilities even in development dependencies.

@ndelangen ndelangen self-requested a review December 22, 2022 16:02
@ndelangen
Copy link
Member

I'll merge this as soon as I've verified on my machine it's all OK, and after babel/babel#15301 is released. 👍 thank you @timur-svoboda for the very detailed explanation and the effort you've put in.

Just as a heads-up:
We're only using babel for 3 packages, still. This project: #18732 is nearly complete.
The transpilation tool uses esbuild under the hood, not babel.

That doesn't refute your point, I wasn't trying to, just though I'd give you have information so you were aware, we're not using babel to convert our code.

@ndelangen ndelangen merged commit d6008c0 into storybookjs:next Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants