Hotfix: Backport host/origin validation to requests and websocket connections#34011
Merged
Conversation
ghengeveld
requested review from
JReinhold,
kasperpeulen and
ndelangen
as code owners
ghengeveld
requested review from
shilman,
tmeasday and
valentinpalkovic
as code owners
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 for HTTP requests and WebSocket upgrades, propagates allowedHosts/localAddress/networkAddress into Vite and startup outputs, changes server-channel APIs to accept an options object (token + host context), updates related types, tests, and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as HTTP/WS Server
participant Middleware as Host Validation Middleware
participant Upgrade as WS Upgrade Handler
participant Logger
Client->>Server: HTTP upgrade request (Host header, Origin, token in URL)
Server->>Middleware: validate Host header
alt Host valid
Middleware-->>Server: next()
Server->>Upgrade: handleUpgrade(request)
Upgrade->>Upgrade: parse URL & extract requestToken
Upgrade->>Upgrade: isValidHost(originHost, options)
Upgrade->>Upgrade: isValidToken(requestToken, expectedToken)
alt Token & Origin valid
Upgrade-->>Client: complete WS upgrade
Upgrade->>Server: emit 'connection'
else invalid token/origin
Upgrade-->>Client: 403 Forbidden, terminate socket
Upgrade->>Logger: warn(...)
end
else Host invalid
Middleware-->>Client: 403 Forbidden "Invalid host"
Middleware->>Logger: warn(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 3
🤖 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/lib/core-server/src/build-dev.ts`:
- Around line 104-117: The warning uses the stale allowedHosts value captured
from the first presets.apply('core', {}) call; update the code to re-read
core.allowedHosts from presets.apply('core', {}) (or from the second-pass
presets where core is finalized) immediately before emitting the logger.warn and
any startup output so the check uses the final allowedHosts value; locate the
allowedHosts variable and the logger.warn block and replace its condition to
reference the freshly obtained allowedHosts (or a helper like
resolveAllowedHosts()) used later at startup to ensure accurate warnings about
--host 0.0.0.0.
In `@code/lib/core-server/src/utils/getHostValidationMiddleware.ts`:
- Around line 51-53: Update the misleading comment in
getHostValidationMiddleware: change the sentence that claims "Requests with no
Host header (e.g. same-origin navigation, GET from address bar) are not allowed"
to accurately state that modern browsers and address-bar navigations normally
include a Host header (HTTP/1.1+), and clarify that missing Host headers occur
only for older/non-HTTP/1.1 clients or malformed requests; ensure this revised
note appears alongside the getHostValidationMiddleware validation description so
future maintainers are not confused.
- Around line 35-39: The current early-return in getHostValidationMiddleware
that treats options.host === '0.0.0.0' and allowedHosts.length === 0 as valid
must be removed; instead, do not bypass host validation for 0.0.0.0—either
remove that conditional entirely or change the logic to only allow all hosts
when an explicit wildcard entry (e.g., '*' or a documented allow-all marker)
exists in allowedHosts; update the validation flow around options.host and the
allowedHosts check so Host headers are always validated unless explicitly
configured to allow all.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c94bf7ad-279e-467b-a00e-cf4fa4d033e4
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
code/builders/builder-vite/src/vite-server.tscode/lib/core-server/src/build-dev.tscode/lib/core-server/src/dev-server.tscode/lib/core-server/src/utils/get-server-channel.tscode/lib/core-server/src/utils/getHostValidationMiddleware.tscode/lib/core-server/src/utils/output-startup-information.tscode/lib/core-server/src/utils/validate-websocket-token.tscode/lib/types/src/modules/core-common.tscode/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/lib/core-server/src/utils/__tests__/server-channel.test.ts (1)
136-136: Fix stale test comment for clarity.This case is invalid-origin, not wrong-token.
✏️ Suggested edit
- // Simulate upgrade request with wrong token + // Simulate upgrade request with invalid origin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/core-server/src/utils/__tests__/server-channel.test.ts` at line 136, Update the stale test comment in server-channel.test.ts: locate the comment "// Simulate upgrade request with wrong token" in the test around the invalid-origin case and change it to accurately reflect the scenario (e.g. "// Simulate upgrade request with invalid origin" or "// Simulate upgrade request with wrong origin") so the comment matches the test behavior in the invalid-origin case.
🤖 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/lib/core-server/src/utils/__tests__/server-channel.test.ts`:
- Around line 11-15: The test creates a transport options object with a unsafe
cast ("as any") which hides type mismatches; replace the cast by typing the
object with the proper interface used by the ServerChannel constructor
(ServerChannelTransportOptions) or the constructor parameter type (e.g., use:
const options: ServerChannelTransportOptions = { localAddress: ...,
networkAddress: ..., token: ... }), import that type if necessary, and remove
the "as any" so TypeScript will validate the option shape in
server-channel.test.ts (look for the options variable and the
ServerChannel/ServerChannelTransportOptions symbol to locate the code to
change).
---
Nitpick comments:
In `@code/lib/core-server/src/utils/__tests__/server-channel.test.ts`:
- Line 136: Update the stale test comment in server-channel.test.ts: locate the
comment "// Simulate upgrade request with wrong token" in the test around the
invalid-origin case and change it to accurately reflect the scenario (e.g. "//
Simulate upgrade request with invalid origin" or "// Simulate upgrade request
with wrong origin") so the comment matches the test behavior in the
invalid-origin case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 899964aa-179b-4f8d-8809-30dd7653fce2
📒 Files selected for processing (2)
code/jest.config.base.jscode/lib/core-server/src/utils/__tests__/server-channel.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
code/lib/core-server/src/utils/getHostValidationMiddleware.ts (2)
52-54:⚠️ Potential issue | 🟡 MinorCorrect the Host-header note in the middleware docs.
Line 53 is inaccurate: normal browser HTTP/1.1+ requests (including address-bar navigation) generally include a Host header.
Suggested wording
- * Validates the Host header against known local/network addresses and allowed hosts. Requests with - * no Host header (e.g. same-origin navigation, GET from address bar) are not allowed. + * Validates the Host header against known local/network addresses and allowed hosts. + * Missing Host headers are typically malformed/legacy client cases and are rejected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/core-server/src/utils/getHostValidationMiddleware.ts` around lines 52 - 54, Update the middleware comment in getHostValidationMiddleware to correct the inaccurate note about Host headers: replace the sentence claiming "no Host header (e.g. same-origin navigation, GET from address bar) are not allowed" with wording that correctly states modern browser HTTP/1.1+ requests (including address-bar navigations) generally include a Host header, and clarify that the middleware rejects requests that are missing or malformed Host headers while still allowing recognized local/network and allowed hosts; locate and edit the comment block above the getHostValidationMiddleware function to make this change.
36-40:⚠️ Potential issue | 🟠 MajorRemove the implicit allow-all path for
0.0.0.0.Line 38 currently disables host validation when bound to
0.0.0.0and no explicitallowedHostsare set, which undermines this security hotfix in common container setups.Suggested fix
- // Setting host to 0.0.0.0 binds to all hosts, which implies allowing all hosts. - // This is common in containerized environments like Docker. - if (options.host === '0.0.0.0' && allowedHosts.length === 0) { - return true; - } + // Keep validation active even when binding to 0.0.0.0. + // Use `allowedHosts: true` only when intentionally opting into allow-all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/core-server/src/utils/getHostValidationMiddleware.ts` around lines 36 - 40, Remove the implicit allow-all branch that returns true when options.host === '0.0.0.0' and allowedHosts.length === 0 so host validation always runs; specifically, delete the if (options.host === '0.0.0.0' && allowedHosts.length === 0) { return true; } path in getHostValidationMiddleware and ensure the middleware continues to validate requests against allowedHosts (or fail when none are configured) so binding to 0.0.0.0 no longer bypasses checks for host validation.
🤖 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/lib/core-server/src/utils/getHostValidationMiddleware.ts`:
- Around line 52-54: Update the middleware comment in
getHostValidationMiddleware to correct the inaccurate note about Host headers:
replace the sentence claiming "no Host header (e.g. same-origin navigation, GET
from address bar) are not allowed" with wording that correctly states modern
browser HTTP/1.1+ requests (including address-bar navigations) generally include
a Host header, and clarify that the middleware rejects requests that are missing
or malformed Host headers while still allowing recognized local/network and
allowed hosts; locate and edit the comment block above the
getHostValidationMiddleware function to make this change.
- Around line 36-40: Remove the implicit allow-all branch that returns true when
options.host === '0.0.0.0' and allowedHosts.length === 0 so host validation
always runs; specifically, delete the if (options.host === '0.0.0.0' &&
allowedHosts.length === 0) { return true; } path in getHostValidationMiddleware
and ensure the middleware continues to validate requests against allowedHosts
(or fail when none are configured) so binding to 0.0.0.0 no longer bypasses
checks for host validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c2ddb64-63fd-4fd4-8c94-fa1e0f08f6b1
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
code/builders/builder-manager/src/utils/template.tscode/lib/core-server/package.jsoncode/lib/core-server/src/dev-server.tscode/lib/core-server/src/utils/getHostValidationMiddleware.tscode/lib/core-server/src/utils/output-startup-information.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CHANGELOG.md (2)
7-7: Fix typo in previous release notes.There's a typo on line 7: "websocke" should be "websocket".
-- Harden websocke connection +- Harden websocket connectionWhile this is pre-existing content and not part of the current changes, it would be good to fix it for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 7, Fix the typo in the changelog: replace the string "websocke" with "websocket" in the CHANGELOG.md entry (the line containing "Harden websocke connection") so the release note reads "Harden websocket connection".
1-3: Consider adding more detail to the changelog entry.The changelog entry is quite minimal compared to other entries in this file. Consider adding:
- A reference to the source PR (e.g., "[
#33835]")- A brief description of what request validation covers (host/origin validation for HTTP and WebSocket)
- Contributor attribution if applicable
Example:
## 7.6.24 -- Add request validation +- Core: Add host/origin validation for HTTP requests and WebSocket connections [`#33835`](https://github.com/storybookjs/storybook/pull/33835)This would help users better understand the scope of the change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 1 - 3, Update the CHANGELOG.md entry under "## 7.6.24" to expand the minimal line "- Add request validation": include the PR reference (e.g., "[`#33835`]"), a short summary that request validation covers host/origin validation for both HTTP and WebSocket, and add contributor attribution (e.g., "Contributed by `@username`") so the entry matches the detail level of other changelog items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CHANGELOG.md`:
- Line 7: Fix the typo in the changelog: replace the string "websocke" with
"websocket" in the CHANGELOG.md entry (the line containing "Harden websocke
connection") so the release note reads "Harden websocket connection".
- Around line 1-3: Update the CHANGELOG.md entry under "## 7.6.24" to expand the
minimal line "- Add request validation": include the PR reference (e.g.,
"[`#33835`]"), a short summary that request validation covers host/origin
validation for both HTTP and WebSocket, and add contributor attribution (e.g.,
"Contributed by `@username`") so the entry matches the detail level of other
changelog items.
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
1 participant
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 v7.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-34011-sha-c45b0f3f. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34011-sha-c45b0f3f sandboxor in an existing project withnpx storybook@0.0.0-pr-34011-sha-c45b0f3f upgrade.More information
0.0.0-pr-34011-sha-c45b0f3forigin-validation-v7c45b0f3f1772790546)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=34011Summary by CodeRabbit
New Features
Tests