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

[fyodorio]

Closes #26014

What I did

The ip package has been switched out for internal-ip (with identical functionality coverage) because of the high-level security vulnerability in the former. The change is done at the consuming utility function and the corresponding test cases.

My research shown that the internal-ip package by @sindresorhus provides sufficiently identical (to ip.address()) functionality via the internalIpV4Sync() function. So the suggestion is to go with this package further on instead of the poorly maintained (and vulnerable, see the linked issue details) ip package.

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 pull request has been released as version 0.0.0-pr-26025-sha-9ddb0c81. Try it out in a new sandbox by running npx storybook@0.0.0-pr-26025-sha-9ddb0c81 sandbox or in an existing project with npx storybook@0.0.0-pr-26025-sha-9ddb0c81 upgrade.

More information

Published version	0.0.0-pr-26025-sha-9ddb0c81
Triggered by	@valentinpalkovic
Repository	fyodorio/storybook
Branch	fyodorio/26014-fix-ip-vulnerability
Commit	9ddb0c81
Datetime	Thu Feb 15 12:53:49 UTC 2024 (1708001629)
Workflow run	7916213544

To request a new release of this pull request, mention the @storybookjs/core team.

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

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/internal-ip@8.0.0	Transitive: environment, filesystem, shell	+30	324 kB	sindresorhus
npm/p-timeout@5.1.0	None	0	11.3 kB	sindresorhus

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

View full report↗︎

[valentinpalkovic]

internal-ip seems to be a pure ESM package. Storybook currently outputs ESM and CommonJS files. To support also CommonJS outputs we have to change the import to a dynamic import. Please be aware that top-level await isn’t supported yet AFAIK. So within the function where functions from internal-ip are called, try to dynamically import import-ip (const importIp = await import('import-ip'))

[fyodorio]

@valentinpalkovic I have updated the lib tests and they seem to pass successfully in CI/CD env:

But the suite for cli fails because of the similar internal-ip ESM import issues I guess:

Do you have any hints on what could I do with that to make it pass?

[valentinpalkovic]

Sorry if I wasn’t precise enough. I meant to change the actual implementation instead of the test. As you can see, sandboxes cannot be generated because of the ESM issue.

[fyodorio]

@valentinpalkovic thanks a lot for the Midas touch, I got pretty confused tbh 👍😅

[valentinpalkovic]

Hehe. :)

I have pushed another commit. Let's see whether we get CI finally green.

[valentinpalkovic]

@fyodorio Thank you so much for your contribution so far!

[victordas]

Great to see this is fixed. Is there any ETA for this MR?
Thanks.

[valentinpalkovic]

It’s planned for tomorrow to do some final testing, get it merged, and patched back to 7.6.

[valentinpalkovic]

During tests, I stumbling upon an issue where actually the internal address wasn't returned.

With ip dependency:

With internal-ip dependency:

An issue has already been reported: sindresorhus/internal-ip#48, but it's from June 2023. I don't think, that the maintainers of internal-ip will fix it at any time soon.

So we have three possibilities:

a) We downgrade internal-ip to version 7.0.0, which is from 2021! A user reported that the issue doesn't exist in 7.0.0.
b) We use another package.
c) We use a fork of ip, which includes the security patch: https://www.npmjs.com/package/@seal-security/ip

@fyodorio WDYT?

cc @shilman

[shilman]

Fork SGTM @valentinpalkovic -- are there any other packages that are better maintained?

[cosieLq]

Maybe using ip-address like socks?

[fyodorio]

Hi guys,

I was reviewing the ip-address code during work on this PR but looks like there's no directly suitable method there to swap ip.address with.

I wouldn't go for a fork either, just for a single method. What about just implementing that as a new utility function inside code/lib/core-server/src/utils/server-address.ts? It doesn't seems to be a big deal (I know, famous last words 😅). But at least it would be an independent small util further on, with a proper test(s). As this method inside ip didn't cause any vulnerabilities, it can be just extracted [more or less] directly (and rewritten for simplicity, as not all parts are neccessary, including the attributes).

WDYT? If you support that, I would try to come up with a suggestion during the weekend.

[cosieLq]

@fyodorio I've followed your suggestion and created a PR #26073 , just have part of the same logic as ip.address implemented as a small function. Not sure if it's good enough.

[fyodorio]

@cosieLq cool, looks reasonable, I'd only suggest to add the proper test coverage using one of existing functions there as an example — also if there are (should be, I believe) tests for the corresponding methods in ip and internal-ip repos, I'd reuse them to start with.

[cosieLq]

@fyodorio thank you for your suggestion. I've added a unit test to server-address.test.ts.

[valentinpalkovic]

@fyodorio

thank you so much for your contribution and your fast reaction!

I am closing this PR in favor of #26073. I guess this is also in your interest!