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: add null check for connection.headers #2200

Merged
merged 2 commits into from Aug 20, 2019
Merged

Conversation

wood1986
Copy link
Contributor

@wood1986 wood1986 commented Aug 16, 2019

fix #2199

  • 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?

Motivation / Use-Case

Breaking Changes

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Aug 16, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Aug 16, 2019

Codecov Report

Merging #2200 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2200   +/-   ##
=======================================
  Coverage   93.91%   93.91%           
=======================================
  Files          34       34           
  Lines        1282     1282           
  Branches      370      371    +1     
=======================================
  Hits         1204     1204           
  Misses         71       71           
  Partials        7        7
Impacted Files Coverage Δ
lib/servers/SockJSServer.js 93.75% <100%> (ø) ⬆️

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 bcacacf...dca0f81. Read the comment docs.

@wood1986 wood1986 force-pushed the fix-2199 branch 2 times, most recently from d602201 to 1d05d6c Compare Aug 16, 2019
@alexander-akait
Copy link
Member

alexander-akait commented Aug 16, 2019

/cc @Loonride

Copy link
Member

@alexander-akait alexander-akait left a comment

need tests

@knagaitsev
Copy link
Collaborator

knagaitsev commented Aug 16, 2019

If the connection object is null, there is no use in passing it along to the callback since we can't do anything with it. I think it is better off being:

if (connection) {
  f(connection, connection.headers);
}

@wood1986
Copy link
Contributor Author

wood1986 commented Aug 17, 2019

If the connection object is null, there is no use in passing it along to the callback since we can't do anything with it. I think it is better off being:

if (connection) {
  f(connection, connection.headers);
}

I thought about this. As you have a check in

this.socketServer.onConnection((connection, headers) => {
if (!connection) {
return;
}
if (!headers) {
this.log.warn(
'transportMode.server implementation must pass headers to the callback of onConnection(f) ' +
'via f(connection, headers) in order for clients to pass a headers security check'
);
}

I want to reuse.

@wood1986 wood1986 changed the title fix: Cannot read property 'headers' of null at Server.socket.on fix: add null check for connection.headers Aug 17, 2019
@wood1986
Copy link
Contributor Author

wood1986 commented Aug 17, 2019

need tests

I added

@wood1986
Copy link
Contributor Author

wood1986 commented Aug 19, 2019

Any updates?

@knagaitsev
Copy link
Collaborator

knagaitsev commented Aug 20, 2019

I want to reuse.

Understood. I have added some notes on the tests.

@knagaitsev
Copy link
Collaborator

knagaitsev commented Aug 20, 2019

Looks good to me /cc @evilebottnawi @hiroppy

Copy link
Member

@alexander-akait alexander-akait left a comment

/cc @hiroppy

@hiroppy hiroppy merged commit 7964997 into webpack:master Aug 20, 2019
3 checks passed
@wood1986
Copy link
Contributor Author

wood1986 commented Aug 21, 2019

When will you guys do the npm publish?

@Sleepful
Copy link

Sleepful commented Aug 26, 2019

@wood1986 not yet I guess, you can use the master branch on your package.json for now if you want:

"webpack-dev-server": "git+https://github.com/webpack/webpack-dev-server.git"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants