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

fix(server): fix header check for socket server #2077

Merged
merged 3 commits into from
Jul 1, 2019

Conversation

knagaitsev
Copy link
Collaborator

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Not yet

Motivation / Use-Case

Headers are not passed in the same way on connection for ws and sockjs, so the socket server implementation needs to reflect that, then the server can check the headers.

Breaking Changes

None

Additional Info

@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #2077 into master will increase coverage by 0.35%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2077      +/-   ##
==========================================
+ Coverage   92.92%   93.27%   +0.35%     
==========================================
  Files          32       32              
  Lines        1201     1205       +4     
  Branches      335      333       -2     
==========================================
+ Hits         1116     1124       +8     
+ Misses         81       77       -4     
  Partials        4        4
Impacted Files Coverage Δ
lib/Server.js 93.62% <100%> (+0.84%) ⬆️
lib/servers/WebsocketServer.js 94.11% <100%> (+1.26%) ⬆️
lib/servers/SockJSServer.js 96.66% <100%> (+0.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96b5ab9...e0b0027. Read the comment docs.

});
}

onConnectionClose(connection, f) {
Copy link
Member

Choose a reason for hiding this comment

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

Why we need this?

Copy link
Collaborator Author

@knagaitsev knagaitsev Jun 28, 2019

Choose a reason for hiding this comment

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

@evilebottnawi As I say later, I want to not assume anything about the connection object. Probably most connection objects will work like connection.on('close', f), which is the case with both sockjs and ws. But maybe some other library implements it like connection.onclose = f.

onConnection(f) {
this.socket.on('connection', f);
this.socket.on('connection', (connection) => {
f(connection, connection.headers);
Copy link
Member

Choose a reason for hiding this comment

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

Why not use only connection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi Because ws connection object does not work the same, it does not have a headers property (https://github.com/webpack/webpack-dev-server/pull/2077/files#diff-b111fb9e1bc06a00edb8d0416cabf86dR33). The goal is to not assume anything about the connection object, since it could differ between socket server types.

@@ -677,25 +677,22 @@ class Server {
const SocketServerImplementation = this.socketServerImplementation;
this.socketServer = new SocketServerImplementation(this);

this.socketServer.onConnection((connection) => {
this.socketServer.onConnection((connection, headers) => {
Copy link
Member

Choose a reason for hiding this comment

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

connection already has headers, i think we don't need extra argument

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Only questions

@alexander-akait
Copy link
Member

/cc @hiroppy

@knagaitsev
Copy link
Collaborator Author

/cc @evilebottnawi @hiroppy please review this first because it is needed for ws mode to work

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @hiroppy

@alexander-akait
Copy link
Member

/cc @hiroppy feel free to merge when CI green, i am on mobile

@hiroppy hiroppy merged commit 7f51859 into webpack:master Jul 1, 2019
!this.checkHost(connection.headers) ||
!this.checkOrigin(connection.headers)
) {
if (headers && (!this.checkHost(headers) || !this.checkOrigin(headers))) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it's too easy to accidentally skip the security check.

Maybe changing it to if (!headers || ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sokra Good point. I think if users do not implement onConnection correctly though, it will be hard to figure out what is wrong. Maybe before that I'll log a warning if headers are missing.

Copy link
Member

Choose a reason for hiding this comment

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

@Loonride it should be no warning, we should drop connection if headers are not present

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi If !headers is true it means that the user made a socket server implementation that did not call the onConnection(f) callback correctly, so I think that could cause confusion if there is little explanation for failed connection. Let me send a PR and I'll show you what I mean

Copy link
Member

Choose a reason for hiding this comment

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

👍

@alexander-akait
Copy link
Member

/cc @Loonride

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Google Summer of Code scope: ws(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants