Security fix 1794. WS server missing Origin validation.

[carlosgeos]

↪️ Pull Request

I believe this PR solves #1783 and amends #1794. I think the merged fix was doing nothing basically (this one --> a1a5422)

It was giving Websocket.Server an origin option which is not even accepted by the constructor of Websocket.Server: https://github.com/websockets/ws/blob/master/doc/ws.md#new-websocketserveroptions-callback

This would mean that this (apparently high) security vulnerability is still present up to the latest version as of now (1.11.0)

I used the proper verifyClient hook to check the origin.
I also included a default hmrHostname on cli.js since there was no hmrHostname by default !

💻 Examples

• Navigating to localhost:1234 works the same as before. Everything works.
• Some site running malicious JS can't connect to the websocket server to sniff the code changes, since the origin will be example.com and not localhost.

🚨 Test instructions

I adapted about half the HMR tests and they pass.

The other half use the Node vm library and I couldn't find a way for this isolated V8 environment to send an origin header with the WebSocket connection. Therefore they fail. I need some help here 😛

I used the proof of concept found in this blog to test the fix (before and after): https://blog.cal1.cn/post/Sniffing%20Codes%20in%20Hot%20Module%20Reloading%20Messages. I basically set up a remote server, serving some JS that fetches http://localhost:1234 and gets the random port number of the websocket server, then connects to it. All the details are in the blog post.

Of course the issue with the static assets (where the random port is found, actually) and CORS is still there, and I will look into it next.

✔️ PR Todo

• Added/updated unit tests for this change (PARTIALLY)
• Filled out test instructions (In case there aren't any unit tests)
• Included links to related issues/PRs

[DeMoorJasper]

This is a breaking change, so not sure if this should have a default

[carlosgeos]

Ok, it could go on Bundler.js' normalizeOptions like this:

hmrHostname:
        options.hmrHostname ||
        options.host ||
        'localhost',

Instead of:

hmrHostname:
        options.hmrHostname ||
        options.host ||
        (options.target === 'electron' ? 'localhost' : ''),

It simplifies the tests as well I think.

[DeMoorJasper]

That would still be a breaking change, but yes. If you really wanna do it than that would probably be a cleaner solution

[carlosgeos]

The problem is:

parcel/packages/core/parcel-bundler/src/cli.js

Lines 25 to 28 in c3d99de

	.option(
	'--hmr-hostname <hostname>',
	'set the hostname of HMR websockets, defaults to location.hostname of current window'
	)

It says it defaults to window.location.hostname but this is not true. It is only true, for some var hostname later in the code, that is used in the client side code for connecting to the websocket server:

parcel/packages/core/parcel-bundler/src/builtins/hmr-runtime.js

Line 26 in c3d99de

var hostname = process.env.HMR_HOSTNAME || location.hostname;

This default does not reach HMRServer.js (obviously). When the time to check the origin comes, hmrHostname is empty, and the check never takes place. And the check, even if it happened, does nothing, as explained before.

hmrHostname needs to have a default somewhere and normalizeOptions is the best place. I don't see why it's a breaking change: people using a specified hmrHostname (because they are using some complex setup) will still keep that.

Another option: deprecate hmrHostname like you mentioned in #1794 (comment) and #1482 (comment). The flags host or hostname can be used to make the origin check, and as fallback to window.location.hostname in the client.

What do you think ?

[DeMoorJasper]

Approved this too early, tests are still failing. Please fix them

[DeMoorJasper]

Closing this as it hasn't been updated in a while and the fix is included in Parcel 2's HMR