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.

Sign up for GitHub

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

Jump to bottom

Security fix 1794. WS server missing Origin validation. #2494

Closed
wants to merge 6 commits into from
Closed

Security fix 1794. WS server missing Origin validation. #2494

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8231917
fixes #1794, makes the websocket server validate the origin
Jan 1, 2019
c1d187e
adapt some tests for commit fixing #1794
Jan 2, 2019
8411254
no more breaking changes for #2494
Jan 3, 2019
243274f
Merge branch 'master' into security-fix-1794
DeMoorJasper Feb 8, 2019
4f1e289
Merge branch 'master' into security-fix-1794
DeMoorJasper Feb 8, 2019
5e7880c
Merge branch 'master' into security-fix-1794
DeMoorJasper Mar 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 44 additions & 17 deletions packages/core/integration-tests/test/hmr.js
View file
Open in desktop
Expand Up @@ -48,14 +48,17 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
await b.bundle();

ws = new WebSocket('ws://localhost:' + b.options.hmrPort);
ws = new WebSocket('ws://localhost:' + b.options.hmrPort, {
origin: 'http://localhost:1234'
});

const buildEnd = nextEvent(b, 'buildEnd');
await sleep(100);

fs.writeFile(
path.join(__dirname, '/input/local.js'),
'exports.a = 5;\nexports.b = 5;'
Expand All @@ -79,6 +82,7 @@ describe('hmr', function() {
b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true,
hmrHostname: 'localhost',
target: 'node'
});
await b.bundle();
Expand All @@ -103,7 +107,9 @@ describe('hmr', function() {
});
await b.bundle();

ws = new WebSocket('ws://localhost:' + b.options.hmrPort);
ws = new WebSocket('ws://localhost:' + b.options.hmrPort, {
origin: 'http://localhost:1234'
});

const buildEnd = nextEvent(b, 'buildEnd');

Expand All @@ -130,11 +136,14 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
await b.bundle();

ws = new WebSocket('ws://localhost:' + b.options.hmrPort);
ws = new WebSocket('ws://localhost:' + b.options.hmrPort, {
origin: 'http://localhost:1234'
});

const buildEnd = nextEvent(b, 'buildEnd');

Expand All @@ -159,11 +168,14 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
await b.bundle();

ws = new WebSocket('ws://localhost:' + b.options.hmrPort);
ws = new WebSocket('ws://localhost:' + b.options.hmrPort, {
origin: 'http://localhost:1234'
});

const buildEnd = nextEvent(b, 'buildEnd');

Expand Down Expand Up @@ -198,7 +210,8 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
await b.bundle();

Expand All @@ -210,7 +223,9 @@ describe('hmr', function() {
await nextEvent(b, 'buildEnd');
await sleep(50);

ws = new WebSocket('ws://localhost:' + b.options.hmrPort);
ws = new WebSocket('ws://localhost:' + b.options.hmrPort, {
origin: 'http://localhost:1234'
});
let msg = JSON.parse(await nextEvent(ws, 'message'));
assert.equal(msg.type, 'error');
});
Expand All @@ -223,11 +238,14 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
await b.bundle();

ws = new WebSocket('ws://localhost:' + b.options.hmrPort);
ws = new WebSocket('ws://localhost:' + b.options.hmrPort, {
origin: 'http://localhost:1234'
});

const firstBuildEnd = nextEvent(b, 'buildEnd');

Expand Down Expand Up @@ -264,7 +282,8 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
let bundle = await b.bundle();
let outputs = [];
Expand Down Expand Up @@ -295,7 +314,8 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
let bundle = await b.bundle();
let outputs = [];
Expand Down Expand Up @@ -336,7 +356,8 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
let bundle = await b.bundle();
let outputs = [];
Expand Down Expand Up @@ -369,7 +390,8 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
let bundle = await b.bundle();

Expand Down Expand Up @@ -410,7 +432,8 @@ describe('hmr', function() {

b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true
hmr: true,
hmrHostname: 'localhost'
});
let bundle = await b.bundle();

Expand Down Expand Up @@ -468,11 +491,13 @@ describe('hmr', function() {
b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true,
hmrHostname: 'localhost',
https: true
});
await b.bundle();

ws = new WebSocket('wss://localhost:' + b.options.hmrPort, {
origin: 'http://localhost:1234',
rejectUnauthorized: false
});

Expand Down Expand Up @@ -502,6 +527,7 @@ describe('hmr', function() {
b = bundler(path.join(__dirname, '/input/index.js'), {
watch: true,
hmr: true,
hmrHostname: 'localhost',
https: {
key: path.join(__dirname, '/integration/https/private.pem'),
cert: path.join(__dirname, '/integration/https/primary.crt')
Expand All @@ -510,6 +536,7 @@ describe('hmr', function() {
await b.bundle();

ws = new WebSocket('wss://localhost:' + b.options.hmrPort, {
origin: 'http://localhost:1234',
rejectUnauthorized: false
});

Expand Down
5 changes: 4 additions & 1 deletion packages/core/integration-tests/test/utils.js
View file
Open in desktop
Expand Up @@ -112,7 +112,10 @@ function prepareBrowserContext(bundle, globals) {
document: fakeDocument,
WebSocket,
console,
location: {hostname: 'localhost'},
location: {
hostname: 'localhost',
origin: 'http://localhost:1234'
},
fetch(url) {
return Promise.resolve({
arrayBuffer() {
Expand Down
12 changes: 5 additions & 7 deletions packages/core/parcel-bundler/src/HMRServer.js
View file
Open in desktop
Expand Up @@ -17,15 +17,13 @@ class HMRServer {
}

let websocketOptions = {
server: this.server
server: this.server,
verifyClient: (info) => {
const originator = new URL(info.origin);
return options.hmrHostname === originator.hostname;
}
};

if (options.hmrHostname) {
websocketOptions.origin = `${options.https ? 'https' : 'http'}://${
options.hmrHostname
}`;
}

this.wss = new WebSocket.Server(websocketOptions);
this.server.listen(options.hmrPort, resolve);
});
Expand Down
1 change: 1 addition & 0 deletions packages/core/parcel-bundler/src/cli.js
View file
Open in desktop
Expand Up @@ -214,6 +214,7 @@ async function bundle(main, command) {

command.throwErrors = false;
command.scopeHoist = command.experimentalScopeHoisting || false;
command.hmrHostname = command.hmrHostname || 'localhost';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

const bundler = new Bundler(main, command);

command.target = command.target || 'browser';
Expand Down