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

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

Closed
3 changes: 1 addition & 2 deletions code/lib/core-server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
"express": "^4.17.3",
"fs-extra": "^11.1.0",
"globby": "^11.0.2",
"ip": "^2.0.0",
"internal-ip": "^8.0.0",
"lodash": "^4.17.21",
"open": "^8.4.0",
"pretty-hrtime": "^1.0.3",
Expand All @@ -101,7 +101,6 @@
"devDependencies": {
"@storybook/addon-docs": "workspace:*",
"@types/compression": "^1.7.0",
"@types/ip": "^1.1.0",
"@types/node-fetch": "^2.5.7",
"@types/ws": "^8",
"boxen": "^7.1.1",
Expand Down
2 changes: 1 addition & 1 deletion code/lib/core-server/src/dev-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export async function storybookDevServer(options: Options) {
const { port, host, initialPath } = options;
invariant(port, 'expected options to have a port');
const proto = options.https ? 'https' : 'http';
const { address, networkAddress } = getServerAddresses(port, host, proto, initialPath);
const { address, networkAddress } = await getServerAddresses(port, host, proto, initialPath);

const listening = new Promise<void>((resolve, reject) => {
// @ts-expect-error (Following line doesn't match TypeScript signature at all 🤔)
Expand Down
18 changes: 9 additions & 9 deletions code/lib/core-server/src/utils/__tests__/server-address.test.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
import { describe, beforeEach, it, expect, vi } from 'vitest';
import ip from 'ip';
import { getServerAddresses } from '../server-address';

vi.mock('ip');
const mockedIp = vi.mocked(ip);
vi.mock('internal-ip', () => ({
internalIpV4Sync: vi.fn(),
}));

describe('getServerAddresses', () => {
beforeEach(() => {
mockedIp.address.mockReturnValue('192.168.0.5');
beforeEach(async () => {
vi.mocked(await import('internal-ip')).internalIpV4Sync.mockReturnValue('192.168.0.5');
});

it('builds addresses with a specified host', () => {
const { address, networkAddress } = getServerAddresses(9009, '192.168.89.89', 'http');
it('builds addresses with a specified host', async () => {
const { address, networkAddress } = await getServerAddresses(9009, '192.168.89.89', 'http');
expect(address).toEqual('http://localhost:9009/');
expect(networkAddress).toEqual('http://192.168.89.89:9009/');
});

it('builds addresses with local IP when host is not specified', () => {
const { address, networkAddress } = getServerAddresses(9009, '', 'http');
it('builds addresses with local IP when host is not specified', async () => {
const { address, networkAddress } = await getServerAddresses(9009, '', 'http');
expect(address).toEqual('http://localhost:9009/');
expect(networkAddress).toEqual('http://192.168.0.5:9009/');
});
Expand Down
33 changes: 25 additions & 8 deletions code/lib/core-server/src/utils/server-address.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { describe, it, expect, vi } from 'vitest';
import detectPort from 'detect-port';
import { getServerAddresses, getServerPort, getServerChannelUrl } from './server-address';
import detectPort from 'detect-port';

vi.mock('ip');
vi.mock('internal-ip', () => ({
internalIpV4Sync: vi.fn(),
}));
vi.mock('detect-port');
vi.mock('@storybook/node-logger');

Expand All @@ -11,35 +13,50 @@ describe('getServerAddresses', () => {
const host = 'localhost';
const proto = 'http';

it('should return server addresses without initial path by default', () => {
it('should return server addresses without initial path by default', async () => {
const expectedAddress = `${proto}://localhost:${port}/`;
const expectedNetworkAddress = `${proto}://${host}:${port}/`;

const result = getServerAddresses(port, host, proto);
const result = await getServerAddresses(port, host, proto);

expect(result.address).toBe(expectedAddress);
expect(result.networkAddress).toBe(expectedNetworkAddress);
});

it('should return server addresses with initial path', () => {
it('should return server addresses with initial path', async () => {
const initialPath = '/foo/bar';

const expectedAddress = `${proto}://localhost:${port}/?path=/foo/bar`;
const expectedNetworkAddress = `${proto}://${host}:${port}/?path=/foo/bar`;

const result = getServerAddresses(port, host, proto, initialPath);
const result = await getServerAddresses(port, host, proto, initialPath);

expect(result.address).toBe(expectedAddress);
expect(result.networkAddress).toBe(expectedNetworkAddress);
});

it('should return server addresses with initial path and add slash if missing', () => {
it('should return server addresses with initial path and add slash if missing', async () => {
const initialPath = 'foo/bar';

const expectedAddress = `${proto}://localhost:${port}/?path=/foo/bar`;
const expectedNetworkAddress = `${proto}://${host}:${port}/?path=/foo/bar`;

const result = getServerAddresses(port, host, proto, initialPath);
const result = await getServerAddresses(port, host, proto, initialPath);

expect(result.address).toBe(expectedAddress);
expect(result.networkAddress).toBe(expectedNetworkAddress);
});

it('should return the internal ip if host is not specified', async () => {
const initialPath = 'foo/bar';
const mockedNetworkIP = '192.168.0.5';

const expectedAddress = `${proto}://localhost:${port}/?path=/foo/bar`;
const expectedNetworkAddress = `${proto}://${mockedNetworkIP}:${port}/?path=/foo/bar`;

vi.mocked(await import('internal-ip')).internalIpV4Sync.mockReturnValue('192.168.0.5');

const result = await getServerAddresses(port, undefined, proto, initialPath);

expect(result.address).toBe(expectedAddress);
expect(result.networkAddress).toBe(expectedNetworkAddress);
Expand Down
10 changes: 6 additions & 4 deletions code/lib/core-server/src/utils/server-address.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
import ip from 'ip';

import { logger } from '@storybook/node-logger';
import detectFreePort from 'detect-port';

export function getServerAddresses(
export async function getServerAddresses(
port: number,
host: string | undefined,
proto: string,
initialPath?: string
) {
const address = new URL(`${proto}://localhost:${port}/`);
const networkAddress = new URL(`${proto}://${host || ip.address()}:${port}/`);

const networkAddress = new URL(
// importing purely ESM 'internal-ip' package asynchronously to support CommonJS outputs
`${proto}://${host || (await import('internal-ip')).internalIpV4Sync()}:${port}/`
);

if (initialPath) {
const searchParams = `?path=${decodeURIComponent(
Expand Down