fix(server): respect sockPath on transportMode: 'ws' (#2310) #2311

joonas-lahtinen · 2019-11-09T12:09:32Z

For Bugs and Features; did you add new tests?

Added tests to verify that sockPath is respected.

Motivation / Use-Case

Fixes bug #2310.

jsf-clabot · 2019-11-09T12:09:34Z

All committers have signed the CLA.

codecov · 2019-11-09T12:14:26Z

Codecov Report

Merging #2311 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2311      +/-   ##
==========================================
+ Coverage   93.84%   93.86%   +0.02%     
==========================================
  Files          34       34              
  Lines        1283     1288       +5     
  Branches      366      367       +1     
==========================================
+ Hits         1204     1209       +5     
  Misses         78       78              
  Partials        1        1

Impacted Files	Coverage Δ
lib/servers/WebsocketServer.js	`91.66% <100%> (+2.19%)`	⬆️

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 3a31917...976e0cf. Read the comment docs.

alexander-akait

/cc @Loonride

alexander-akait · 2019-11-09T14:24:05Z

Please accept CLA

hiroppy · 2019-11-09T14:26:43Z

lib/servers/WebsocketServer.js

    this.wsServer.on('error', (err) => {
      this.server.log.error(err.message);
    });
  }

+  shouldUpgrade(req) {
+    const pathname = url.parse(req.url).pathname;


memo: we'll use URLSearchParams after major release.

I fixed the code to use WebSocket.Server.shouldHandle()
https://github.com/websockets/ws/blob/master/doc/ws.md#servershouldhandlerequest
Should be more futureproof?

joonas-lahtinen · 2019-11-09T14:30:44Z

Thanks for the quick reviews. I noticed a bug in the tests based on the code coverage difference report, should be fixed now and using the more forward compatible ws.Server.shouldHandle()

joonas-lahtinen · 2019-11-09T15:34:42Z

This also fixes the bug that whole webpack-dev-server process exits due to an unhandled error if the proxy target is not open:

[HPM] Rewriting path from "/api" to ""
[HPM] GET /api ~> http://localhost:8888
[HPM] Upgrading to WebSocket
events.js:174
      throw er; // Unhandled 'error' event
      ^

Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed

alexander-akait

/cc @hiroppy @Loonride wait both reviews

knagaitsev

@joonas-lahtinen I ran your new tests without the changes that you made, and they passed. How are your changes different from the existing behavior? The ws docs say for the path option - "Accept only connections matching this path" (https://github.com/websockets/ws/blob/master/doc/ws.md#new-websocketserveroptions-callback), so paths that do not match the one specified are already rejected.

Edit: I missed the explanation in #2310. Could you make a test demonstrating that proxying works?

joonas-lahtinen · 2019-11-13T20:24:20Z

Thanks for the patience. After some digging I found the spot to inject testing the different transportMode options with proxy.

knagaitsev · 2019-11-20T23:07:58Z

LGTM /cc @evilebottnawi @hiroppy

alexander-akait

/cc @hiroppy

hiroppy · 2019-11-29T07:23:53Z

thanks

joonas-lahtinen requested review from alexander-akait, hiroppy and knagaitsev as code owners November 9, 2019 12:09

alexander-akait reviewed Nov 9, 2019

View reviewed changes

hiroppy reviewed Nov 9, 2019

View reviewed changes

joonas-lahtinen force-pushed the fix/transportMode-ws-proxy branch from ee3e805 to 6bcddc9 Compare November 9, 2019 14:28

alexander-akait previously approved these changes Nov 11, 2019

View reviewed changes

knagaitsev requested changes Nov 12, 2019

View reviewed changes

fix(server): respect sockPath on transportMode: 'ws' (webpack#2310)

7d38597

joonas-lahtinen dismissed alexander-akait’s stale review via 7d38597 November 13, 2019 20:20

joonas-lahtinen force-pushed the fix/transportMode-ws-proxy branch from 6bcddc9 to 7d38597 Compare November 13, 2019 20:20

knagaitsev approved these changes Nov 20, 2019

View reviewed changes

alexander-akait approved these changes Nov 21, 2019

View reviewed changes

Merge branch 'master' into fix/transportMode-ws-proxy

976e0cf

hiroppy approved these changes Nov 29, 2019

View reviewed changes

hiroppy merged commit e188542 into webpack:master Nov 29, 2019

hiroppy mentioned this pull request Dec 6, 2019

transportMode: 'ws' interferes with proxy { ws: true } #2310

Closed

2 tasks

snyk-bot mentioned this pull request Dec 31, 2019

[Snyk] Upgrade webpack-dev-server from 3.9.0 to 3.10.1 alfill/qr-code-scanner#6

Merged

This was referenced Feb 22, 2020

[Snyk] Upgrade webpack-dev-server from 3.2.1 to 3.10.2 saurabharch/react-ecommerce#5

Open

[Snyk] Upgrade webpack-dev-server from 3.1.14 to 3.10.2 raindigi/eShopOnContainers#2

Open

snyk-bot mentioned this pull request Mar 5, 2020

[Snyk] Upgrade webpack-dev-server from 3.1.10 to 3.10.3 sarvaniupadrasta/learnReact#2

Open

vpetersson mentioned this pull request Mar 6, 2020

[Snyk] Upgrade webpack-dev-server from 3.1.14 to 3.10.3 WoTTsecurity/api#757

Closed

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade webpack-dev-server from 2.9.4 to 3.10.3 lwojcik/sc2pte-panel-frontend#85

Closed

This was referenced Apr 4, 2020

[Snyk] Upgrade webpack-dev-server from 3.7.2 to 3.10.3 CollageLabs/collagelabs.org#60

Merged

[Snyk] Upgrade webpack-dev-server from 3.9.0 to 3.10.3 cds-snc/webpack-starter#8

Open

This was referenced May 5, 2020

[Snyk] Upgrade webpack-dev-server from 3.1.8 to 3.10.3 Barnstorm-Online/barnstorm-cli#6

Open

[Snyk] Upgrade webpack-dev-server from 3.1.14 to 3.10.3 hzhz/material-ui#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(server): respect sockPath on transportMode: 'ws' (#2310) #2311

fix(server): respect sockPath on transportMode: 'ws' (#2310) #2311

joonas-lahtinen commented Nov 9, 2019

jsf-clabot commented Nov 9, 2019 •

edited

codecov bot commented Nov 9, 2019 •

edited

alexander-akait left a comment

alexander-akait commented Nov 9, 2019

hiroppy Nov 9, 2019

joonas-lahtinen Nov 9, 2019

joonas-lahtinen commented Nov 9, 2019

joonas-lahtinen commented Nov 9, 2019 •

edited

alexander-akait left a comment

knagaitsev left a comment •

edited

joonas-lahtinen commented Nov 13, 2019

knagaitsev commented Nov 20, 2019

alexander-akait left a comment

hiroppy commented Nov 29, 2019

fix(server): respect sockPath on transportMode: 'ws' (#2310) #2311

fix(server): respect sockPath on transportMode: 'ws' (#2310) #2311

Conversation

joonas-lahtinen commented Nov 9, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

jsf-clabot commented Nov 9, 2019 • edited

codecov bot commented Nov 9, 2019 • edited

Codecov Report

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Nov 9, 2019

hiroppy Nov 9, 2019

Choose a reason for hiding this comment

joonas-lahtinen Nov 9, 2019

Choose a reason for hiding this comment

joonas-lahtinen commented Nov 9, 2019

joonas-lahtinen commented Nov 9, 2019 • edited

alexander-akait left a comment

Choose a reason for hiding this comment

knagaitsev left a comment • edited

Choose a reason for hiding this comment

joonas-lahtinen commented Nov 13, 2019

knagaitsev commented Nov 20, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

hiroppy commented Nov 29, 2019

jsf-clabot commented Nov 9, 2019 •

edited

codecov bot commented Nov 9, 2019 •

edited

joonas-lahtinen commented Nov 9, 2019 •

edited

knagaitsev left a comment •

edited