-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Core: Replace ip function to address security concerns #27529
Conversation
@tony19 Thank you! Indeed, let's fix this by removing 'ip'. |
@tony19 I would like to merge this. Can you fix the yarn.lock issue? |
Failed to publish canary version of this pull request, triggered by @valentinpalkovic. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/9366149836 |
@tony19 Could you please allow the contribution on this branch for maintainers? I would like to update the lock file, which is not up to date. Or just run |
@kasperpeulen @valentinpalkovic I don't see the option to enable maintainer access on this PR, but I've added you both to my fork as maintainers in case that's still needed. I'll update the lockfile... done. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 07e8bc7. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Core: Replace ip function with a small helper function to address security concerns (cherry picked from commit 70467ec)
Core: Replace ip function with a small helper function to address security concerns (cherry picked from commit 70467ec)
@valentinpalkovic is it possible to roll this out not only for Storybook V8 but for Storybook V7 as well? Given that this is a security fix to core. |
Just for context: The reported security vulnerability with the affected ip.isPublic() method is not used by Storybook. Hence, the vulnerability reported by npm audit doesn't affect Storybook users in general. You are still save to use Storybook 7. Do we want to patch this back to 7 nonetheless @shilman? |
@valentinpalkovic Thank you for your answer! For users of Storybook V7, it'd be very useful if this is patched back to V7, since any deploy pipeline consisting of dependency audit will otherwise fail. |
Core: Replace ip function with a small helper function to address security concerns (cherry picked from commit 70467ec)
I understand your concerns. We have defined here in which particular cases patches for security vulnerabilities are patched back. In this case, though, Storybook wasn't affected by the security vulnerability because we haven't used the affected API. Because of that, it is very unlikely that we will patch this fix back to Storybook 7. Can you explain for which specific reasons you're stuck on Storybook 7? |
The reasons are mainly priority of work. In my case (I believe relatable to many others), Storybook is deployed for the benefit of UX designers. Upgrade is not considered high priority especially when it involves some work, in comparison with other development work, as long as nothing breaks or goes deprecated. However, security is considered high priority and a vulnerable dependency, even slightly, will trigger a failed pipeline and be reflected in things such as dependency check dashboard etc.
|
Hi Storybook team! @valentinpalkovic et al. : I'm trying to give my Architect team an update on when the patch to 8.1.6 will be released. They are itching to remove Storybook from our repo b/c of the ip security concern. I would really like to keep it as my team in particular use Storybook a lot. Is there a chance the patch will be merged soon? I notice it has been around for a few weeks so I'm not sure if there is a schedule or cadence (or, perhaps, I could convince someone to merge it sooner rather than later 😬). Thank you! |
In addition to @cosieLq's reasoning, I'd like to add that storybook 8 requires upgrading svelte to 4+, which my team has yet to do. Until then, we cannot upgrade to Storybook 8, so a patch for Storybook 7 would be greatly appreciated. |
I would also like to request a patch for Storybook 7. Our team heavily relies on storiesOf for dynamically generated stories and currently lacks the capacity to migrate everything to the experimental indexer API. The experimental indexer API also appears to have bugs in its current state. A patch would be extremely helpful for us. Thank you. |
Hi @kuhiga. Chatting with the team about getting a Separately, regarding
I'm asking in case we need to provide better off-ramps for |
@kuhiga, @claireramming, @cosieLq v7.6.20 was just released, which contains the security hotfix! |
There were a few security concerns in storybook. Although these seem to be fixed in 8.2 and later, we're still on 7.x. More info can be found [here](storybookjs/storybook#27529). [category:Dependencies] Co-authored-by: manuel.carrera <manuel.carrera@workday.com>
Copy of #26073 by @cosieLq
Closes #26014
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:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/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-27529-sha-07e8bc7b
. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-27529-sha-07e8bc7b sandbox
or in an existing project withnpx storybook@0.0.0-pr-27529-sha-07e8bc7b upgrade
.More information
0.0.0-pr-27529-sha-07e8bc7b
refactor/ip
07e8bc7b
1717564843
)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=27529