Hotfix: Backport host/origin validation to requests and websocket connections#34009
Merged
Conversation
ghengeveld
added
maintenance
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds host validation middleware and tests; refactors WebSocket server-channel APIs to accept an options object (token, allowedHosts, localAddress, networkAddress); wires allowedHosts into Vite and core dev-server flows; updates startup output and types; adds runtime dependency and removes root Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant DevServer as Dev Server
participant HostValidation as Host Validation
participant Channel as Server Channel
participant Upgrader as Upgrade Handler
Client->>DevServer: HTTP Upgrade (Host, Origin, Token)
DevServer->>HostValidation: validate Host (allowedHosts, localAddress, networkAddress)
alt Host invalid
HostValidation-->>DevServer: invalid
DevServer->>Client: HTTP 403
else Host valid
HostValidation-->>DevServer: ok
DevServer->>Channel: validate Origin & Token (options)
alt Token/Origin invalid
Channel-->>DevServer: reject (403)
DevServer->>Client: Connection rejected
else Token/Origin valid
Channel->>Upgrader: perform upgrade
Upgrader->>Client: WebSocket established
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
code/core/src/core-server/utils/__tests__/server-channel.test.ts (1)
34-40: Consolidate mock patterns to align with repo's spy-mocking standard.This test mixes direct global spying (
vi.spyOn(console, 'warn')) with inline socket mock implementations (socket.write = vi.fn(),socket.destroy = vi.fn()). Move all package/file mocks to top-levelvi.mock(..., { spy: true })and consolidate mock behaviors inbeforeEachblocks usingvi.mocked(...). This pattern repeats across multiple test cases (lines 95–96, 120–121, 145–146, 174–175, 199–200, 222–223, 251–252, 280–281) and should be unified to match repo standards per the spy-mocking rules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/utils/__tests__/server-channel.test.ts` around lines 34 - 40, Consolidate test mocking by replacing ad-hoc spies and inline mock assignments with top-level package/file mocks and centralized setup: add vi.mock(..., { spy: true }) for modules that are currently spied (e.g., console) and move socket method mocks (socket.write, socket.destroy) into the beforeEach using vi.mocked(...) to set their behaviors; remove inline assignments like socket.write = vi.fn() and socket.destroy = vi.fn(), restore behaviors in afterEach with vi.restoreAllMocks(), and ensure all repeated occurrences (the socket mocks at the referenced test blocks) follow this single pattern so that beforeEach manages mocks and top-level vi.mock(..., { spy: true }) enables spying consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/builders/builder-vite/src/vite-server.ts`:
- Around line 35-40: The fallback that sets config.server.allowedHosts = true
only checks for IPv4 wildcard '0.0.0.0' and misses IPv6 wildcard '::'; update
the conditional in vite-server.ts to treat both '0.0.0.0' and '::' as wildcard
binds (e.g., check options.host === '0.0.0.0' || options.host === '::' or
normalize options.host into a set of wildcard values) while keeping the existing
allowedHosts empty-array check so that when options.host is an IPv6 wildcard and
allowedHosts is absent/empty you set config.server.allowedHosts = true.
In `@code/core/src/core-server/build-dev.ts`:
- Around line 139-140: The code destructures allowedHosts from the first
presets.apply('core', {}) result and then later reloads presets with
builder/renderer, so allowedHosts can become stale; update the logic to read
core.allowedHosts (or re-destructure allowedHosts) after the second presets pass
(the reload that applies builder/renderer presets) before emitting the startup
warning/output; specifically adjust the use of the originally captured
allowedHosts variable and reference presets.apply('core', {}) / builder /
renderer so the startup message always uses the post-reload value.
- Around line 147-151: The code calls logger.warn(...) but logger is not
imported into build-dev.ts; add the missing import for the logger used (e.g.,
import { logger } from the Storybook logger module) at the top of the file so
logger is in scope before the call in the function that contains the
host/allowedHosts check (the warning around logger.warn in build-dev.ts).
In `@code/core/src/core-server/utils/get-server-channel.ts`:
- Around line 16-18: The upgrade handling currently calls new URL(request.url,
options.localAddress) which throws if options.localAddress is undefined because
upgrade request.url is often relative; update both occurrences (the new URL(...)
usages) to build a safe base when options.localAddress is missing by falling
back to a host-derived base (e.g., use request.headers.host with "http://" or
"ws://" as appropriate) or by wrapping the URL construction in a try/catch and
constructing new URL(request.url, `http://${request.headers.host}`) when the
first call fails, ensuring relative upgrade URLs are parsed instead of causing a
403 rejection.
---
Nitpick comments:
In `@code/core/src/core-server/utils/__tests__/server-channel.test.ts`:
- Around line 34-40: Consolidate test mocking by replacing ad-hoc spies and
inline mock assignments with top-level package/file mocks and centralized setup:
add vi.mock(..., { spy: true }) for modules that are currently spied (e.g.,
console) and move socket method mocks (socket.write, socket.destroy) into the
beforeEach using vi.mocked(...) to set their behaviors; remove inline
assignments like socket.write = vi.fn() and socket.destroy = vi.fn(), restore
behaviors in afterEach with vi.restoreAllMocks(), and ensure all repeated
occurrences (the socket mocks at the referenced test blocks) follow this single
pattern so that beforeEach manages mocks and top-level vi.mock(..., { spy: true
}) enables spying consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35231fc7-2de2-4e16-ae0e-1821a55a1007
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
.gitignorecode/builders/builder-vite/src/vite-server.tscode/core/package.jsoncode/core/src/core-server/build-dev.tscode/core/src/core-server/dev-server.tscode/core/src/core-server/utils/__tests__/getHostValidationMiddleware.test.tscode/core/src/core-server/utils/__tests__/server-channel.test.tscode/core/src/core-server/utils/__tests__/validate-token.test.tscode/core/src/core-server/utils/get-server-channel.tscode/core/src/core-server/utils/getHostValidationMiddleware.tscode/core/src/core-server/utils/output-startup-information.tscode/core/src/core-server/utils/server-init.tscode/core/src/core-server/utils/validate-token.tscode/core/src/types/modules/core-common.ts
💤 Files with no reviewable changes (1)
- .gitignore
Contributor
|
Failed to publish canary version of this pull request, triggered by @ghengeveld. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/22670820120 |
Contributor
|
Failed to publish canary version of this pull request, triggered by @ghengeveld. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/22674106283 |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/core/src/core-server/build-dev.ts (1)
139-140:⚠️ Potential issue | 🟡 MinorUse post-second-pass
allowedHoststo avoid stale security messaging.
allowedHostsis read from the firstpresets.apply('core', {})(Line 139), then reused for warning/output after the second preset load. If builder/renderer presets altercore.allowedHosts, Lines 143-146 and Line 267 can be inaccurate.Suggested fix
- const { allowedHosts, renderer, builder, disableTelemetry } = await presets.apply('core', {}); + const { renderer, builder, disableTelemetry } = await presets.apply('core', {}); - // '0.0.0.0' binds to all interfaces, which is useful for Docker and other containerized environments. - // By default we allow requests from all hosts in this case, but the user should be made aware of the risk. - if ( - options.host === '0.0.0.0' && - (!allowedHosts || (allowedHosts !== true && allowedHosts.length === 0)) - ) { - logger.warn(dedent` - --host is set to 0.0.0.0 but no allowedHosts are defined. Allowing all hosts. - To restrict allowed hosts, set core.allowedHosts in your main Storybook config. - See: https://storybook.js.org/docs/api/main-config/main-config-core - `); - } @@ presets = await loadAllPresets({ @@ }); + + const { allowedHosts } = await presets.apply('core', {}); + // '0.0.0.0' binds to all interfaces, which is useful for Docker and other containerized environments. + // By default we allow requests from all hosts in this case, but the user should be made aware of the risk. + if ( + options.host === '0.0.0.0' && + (!allowedHosts || (allowedHosts !== true && allowedHosts.length === 0)) + ) { + logger.warn(dedent` + --host is set to 0.0.0.0 but no allowedHosts are defined. Allowing all hosts. + To restrict allowed hosts, set core.allowedHosts in your main Storybook config. + See: https://storybook.js.org/docs/api/main-config/main-config-core + `); + }Also applies to: 143-146, 267-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/build-dev.ts` around lines 139 - 140, The code reads core.allowedHosts from the first presets.apply('core', {}) and then uses it later for warnings/output, which can be stale if renderer/builder presets mutate core.allowedHosts; update the logic to re-read allowedHosts after the second presets.apply that loads/merges renderer and builder presets (i.e., call presets.apply('core', {}) again or read the merged presets result used after secondary load) and use that refreshed allowedHosts value for the warning/output code paths (the places referencing allowedHosts after renderer/builder processing such as the warning block and the use at the later output point).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@code/core/src/core-server/build-dev.ts`:
- Around line 139-140: The code reads core.allowedHosts from the first
presets.apply('core', {}) and then uses it later for warnings/output, which can
be stale if renderer/builder presets mutate core.allowedHosts; update the logic
to re-read allowedHosts after the second presets.apply that loads/merges
renderer and builder presets (i.e., call presets.apply('core', {}) again or read
the merged presets result used after secondary load) and use that refreshed
allowedHosts value for the warning/output code paths (the places referencing
allowedHosts after renderer/builder processing such as the warning block and the
use at the later output point).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2fc3710-2879-445b-90d6-90b0d2c7569c
📒 Files selected for processing (1)
code/core/src/core-server/build-dev.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 3: Update the changelog entry to explicitly state the security-related
backport: replace the vague "Add request validation" with a sentence that
details host/origin validation was added for HTTP requests and WebSocket
connections, mention that this may block external access and that users can
configure core.allowedHosts to allow external hostnames (for example ngrok), and
include a short note about the purpose (preventing host header/origin spoofing).
…ss and add detailed steps for canary testing
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/publish.md (1)
14-17: Consider adding a failure-path note for canary cleanup.Given canary publish failures are common in practice, add a short note that the deployment branch entry must be removed even when Step 2/3 fails (not only on success), plus a quick pointer on where to verify failure cause (workflow run logs).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.md around lines 14 - 17, Add a brief failure-path note to the publish-canary instructions: update the steps around the "deployment branch" entry (Step 1) to say that the deployment branch must be removed even if the canary publish (Step 2/3 invoking the publish-canary workflow/publish.yml) fails, and add a short pointer to check the publish-canary workflow run logs to diagnose the failure; reference the "deployment branch" entry, the "publish-canary" workflow and "publish.yml" so readers know where to remove the branch and where to verify failure cause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/publish.md:
- Around line 14-17: Add a brief failure-path note to the publish-canary
instructions: update the steps around the "deployment branch" entry (Step 1) to
say that the deployment branch must be removed even if the canary publish (Step
2/3 invoking the publish-canary workflow/publish.yml) fails, and add a short
pointer to check the publish-canary workflow run logs to diagnose the failure;
reference the "deployment branch" entry, the "publish-canary" workflow and
"publish.yml" so readers know where to remove the branch and where to verify
failure cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b208196-9487-4796-bd77-b6dd640a7f08
📒 Files selected for processing (1)
.github/workflows/publish.md
|
View your CI Pipeline Execution ↗ for commit 725c81b
☁️ Nx Cloud last updated this comment at |
…ific test failures and errors encountered during CI processes.
…failures and acceptable issues.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
This is a backport of #33835 for Storybook v8.6.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
npx ngrok http 6006.storybook/main.tsto setcore.allowedHoststo include the ngrok hostname (no protocol), then restart StorybookDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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-34009-sha-c90626e7. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34009-sha-c90626e7 sandboxor in an existing project withnpx storybook@0.0.0-pr-34009-sha-c90626e7 upgrade.More information
0.0.0-pr-34009-sha-c90626e7origin-validation-v8c90626e71772636654)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34009Summary by CodeRabbit
New Features
Refactor
Chores
Tests