Core: Replace ip function with a small helper function to address security concerns by cosieLq · Pull Request #26073 · storybookjs/storybook

cosieLq · 2024-02-16T18:01:37Z

What I did

I've listened to the suggestion from #26025 and added a small helper function to replace ip.address, so that we can remove the insecure package ip. What the helper function does is essentially the same as ip.address: it returns the first remotely accessible IPv4 address or otherwise the loopback.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

socket-security · 2024-02-16T18:04:00Z

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/@types/ip@1.1.3

View full report↗︎

AhmedMuhammedElsaid · 2024-02-18T13:12:48Z

waiting for this PR to be merged ASAP !
ThanQ

victordas · 2024-02-18T16:58:36Z

+  const allIps = Object.values(os.networkInterfaces()).flat();
+  const allFilteredIps = allIps.filter((ip) => ip && ip.family === 'IPv4' && !ip.internal);
+
+  return allFilteredIps.length ? allFilteredIps[0]?.address : '127.0.0.1';


Isn't 0.0.0.0 better suited for the else part?

Good point. I've changed this to 0.0.0.0.

valentinpalkovic · 2024-02-19T07:30:30Z

+  const allIps = Object.values(os.networkInterfaces()).flat();
+  const allFilteredIps = allIps.filter((ip) => ip && ip.family === 'IPv4' && !ip.internal);
+
+  return allFilteredIps.length ? allFilteredIps[0]?.address : '0.0.0.0';


I am curious whether returning 0.0.0.0 makes sense. Can the Storybook server be reached in the browser when 0.0.0.0 is used? If not, I would rather return null/undefined and handle it appropriately in the getServerAddresses function. It should return networkAddress: undefined. In this case, the network address shouldn't be logged at all to the user's console, after the server starts.

I'm not sure which returned value is better. The Storybook server is reachable via 0.0.0.0 in my browsers. Maybe it also depends on how to interpret 'On your network' in the log.

valentinpalkovic · 2024-02-19T08:46:26Z

@cosieLq Thank you so much for your contribution!

But I think we can close this. It seems that the security vulnerability was resolved in ip. A new version of ip was just released a couple of hours ago.

valentinpalkovic · 2024-02-19T08:46:49Z

Superseded by #26086

cosieLq · 2024-02-19T09:12:59Z

@cosieLq Thank you so much for your contribution!

But I think we can close this. It seems that the security vulnerability was resolved in ip. A new version of ip was just released a couple of hours ago.

Sure! But I sort of agree with the suggestion from #26025 . Since Storybook uses only part of one function from ip, maybe it's worthy of thinking about whether to keep it as a dependency or just replace it.

tony19 · 2024-02-21T11:49:57Z

FYI, ip 2.0.1 didn't completely fix the bug, and a new CVE might be coming. Maybe we should reconsider this PR to factor out the unstable module, especially since Storybook only uses one of its functions that can easily be recreated.

ouuan · 2024-02-21T11:59:16Z

FYI, the usage here is not affected by CVE-2023-42282, because your network interface will not provide malformed addresses. However, the current implementation of the ip.address() function is incorrect, see indutny/node-ip#61. It returns public addresses when the argument is 'private' and vice-versa. It's documented to return a private address when no argument is provided but actually it's returning a public address instead.

xfournet · 2024-05-29T15:23:49Z

Since we have again a security issue with ip (see CVE-2024-29415), wouldn't be the time to re-open this PR to definitively get rid off this lib ?
in my (relatively large) codebase only storybook still use this dependency

hobadams · 2024-06-03T10:15:43Z

Can this be re-opened? I'm still getting this issue in storybook 8.1.5. Is there a way to work around this without an official release?

Shaun1 · 2024-06-03T15:57:07Z

Also still getting this issue and the (automated) way npm wants to resolve it is to downgrade to storybook@6.5.16.

This needs to be fixed if we are to be comfortable using storybook in a production environment.

valentinpalkovic · 2024-06-03T22:10:20Z

The original repo got deleted, so I can't reopen the PR. Is someone willing to put together a new PR?

tony19 · 2024-06-04T03:17:51Z

@valentinpalkovic The PR's branch could still be checked out locally. I've copied the PR as-is to #27529.

cosieLq added 3 commits February 16, 2024 18:23

Add a helper function in server-address

58e6e4a

Remove ip from dependency

54b8d66

Make mock response from networkInterfaces correct

5370775

cosieLq mentioned this pull request Feb 16, 2024

Core: Replace ip package with internal-ip to address security concerns #26025

Closed

8 tasks

cosieLq added 2 commits February 17, 2024 10:29

Add condition check

8243b1d

Add some unit test

f4c4d55

valentinpalkovic self-assigned this Feb 17, 2024

valentinpalkovic added maintenance User-facing maintenance tasks core security labels Feb 17, 2024

AhmedMuhammedElsaid approved these changes Feb 18, 2024

View reviewed changes

victordas reviewed Feb 18, 2024

View reviewed changes

Change default address to 0.0.0.0

f839b8a

valentinpalkovic added the ci:normal Run our default set of CI jobs (choose this for most PRs). label Feb 19, 2024

valentinpalkovic reviewed Feb 19, 2024

View reviewed changes

valentinpalkovic closed this Feb 19, 2024

xfournet mentioned this pull request Jun 2, 2024

[Bug]: The latest version depends on the highly vulnerable ip package #26014

Closed

tony19 mentioned this pull request Jun 4, 2024

Core: Replace ip function to address security concerns #27529

Merged

8 tasks

Uh oh!

Uh oh!

Conversation

cosieLq commented Feb 16, 2024

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

Uh oh!

socket-security Bot commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AhmedMuhammedElsaid commented Feb 18, 2024

Uh oh!

victordas Feb 18, 2024

Choose a reason for hiding this comment

Uh oh!

cosieLq Feb 18, 2024

Choose a reason for hiding this comment

Uh oh!

valentinpalkovic Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosieLq Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

valentinpalkovic commented Feb 19, 2024

Uh oh!

valentinpalkovic commented Feb 19, 2024

Uh oh!

cosieLq commented Feb 19, 2024

Uh oh!

tony19 commented Feb 21, 2024

Uh oh!

ouuan commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xfournet commented May 29, 2024

Uh oh!

hobadams commented Jun 3, 2024

Uh oh!

Shaun1 commented Jun 3, 2024

Uh oh!

valentinpalkovic commented Jun 3, 2024

Uh oh!

tony19 commented Jun 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

socket-security Bot commented Feb 16, 2024 •

edited

Loading

valentinpalkovic Feb 19, 2024 •

edited

Loading

ouuan commented Feb 21, 2024 •

edited

Loading