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

Incorrect query object when connection to different namespace #3495

Closed
sebamarynissen opened this issue Oct 13, 2019 · 3 comments
Closed

Incorrect query object when connection to different namespace #3495

sebamarynissen opened this issue Oct 13, 2019 · 3 comments
Milestone

Comments

@sebamarynissen
Copy link
Contributor

sebamarynissen commented Oct 13, 2019

Current behaviour

What is actually happening?

When one connects to a different namespace with multiplexing enabled such as

let one = io.connect('/one', {
  'force new connection': false,
  query: { param: 'one' }
});

let two = io.connect('/two', {
  'force new connection': false,
  query: { param: 'two' }
});

the updated query is not parsed correctly on the server. The server side code

let one = io.of('one').on('connection', socket => {
  let { query } = socket.handshake;
  console.log('one', query.param, query.param === 'one');
});
let two = io.of('two').on('connection', socket => {
  let { query } = socket.handshake;
  console.log('two', query.param, query.param === 'two');
});

logs

one one true
two one false

When multiplexing is explicitly disabled by setting

{ 'force new connection': true }

the server logs

one one true
two two true

as expected.

Steps to reproduce

Fork https://github.com/sebamarynissen/socket.io-fiddle. Run npm start, open up a browser and go to http://localhost:3000.

Expected behaviour

The query object should be the same, regardless of whether multiplexing is enabled or not.

Setup

  • OS: Windows 10
  • browser: Chrome
  • socket.io version: 2.3.0

Other information

When I open up the WS network tab in Chrome devtools, it logs that

40/two?param=two

is sent to the server, so it seems that at least the client sends the correct query to the server. Hence I guess that this is a server related issue.

@sebamarynissen
Copy link
Contributor Author

I have found the cause of the bug, it is located in socket.js:

Socket.prototype.buildHandshake = function(query){
  var self = this;
  function buildQuery(){
    var requestQuery = url.parse(self.request.url, true).query;
    //if socket-specific query exist, replace query strings in requestQuery
    // The order of query & requestQuery should be swapped! The general
    // `requestQuery` currently overrides socket-specific `query`, but this
    // should be the other way around.
    // return Object.assign({}, query, requestQuery);

    // It should be:
    return Object.assign({}, requestQuery, query);
  }
  return {
    // ...
    query: buildQuery()
  };
};

I'm going to file a PR for this.

@sebamarynissen
Copy link
Contributor Author

For people having this issue as well, until #3496 is accepted and published on npm, you can fix it by including the following in your server code:

const url = require('url');
const Socket = require('socket.io/lib/socket');
Socket.prototype.buildHandshake = function(query) {
	let self = this;
	function buildQuery() {
		let requestQuery = url.parse(self.request.url, true).query;
		return Object.assign({}, requestQuery, query);
	}
	return {
		headers: this.request.headers,
		time: (new Date) + '',
		address: this.conn.remoteAddress,
		xdomain: !!this.request.headers.origin,
		secure: !!this.request.connection.encrypted,
		issued: +(new Date),
		url: this.request.url,
		query: buildQuery()
	};
};

I can confirm that this fixed my issue which led me to discover this bug.

darrachequesne pushed a commit that referenced this issue Jan 4, 2021
The `query` option of the Manager had the priority over the one of the
Socket instance, which meant updating the Socket#query object on the
client-side was not reflected in the Socket#handshake object on the
server-side.

Please note that the behavior of the `query` option is still a bit
weird in Socket.IO v2, as it only applies to non-default namespace.
This is fixed in v3:

- https://socket.io/docs/v3/migrating-from-2-x-to-3-0/#Add-a-clear-distinction-between-the-Manager-query-option-and-the-Socket-query-option
- https://socket.io/docs/v3/middlewares/#Sending-credentials

Fixes #3495
@darrachequesne
Copy link
Member

Fixed by d33a619 (included in [email protected]).

Please note that this does not apply to Socket.IO v3, since the query option has been updated (more information here).

@darrachequesne darrachequesne added this to the 2.4.0 milestone Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants