upgrade to ws@7

[mixmix]

fixes #25

I'm not familiar with this module, so I've changed as little as possible

• bumped ws to 7.0.0 to close vulnerability
• got tests passing

This is relevant to ssb-server and multi-server

[mixmix]

did an "integration test" by linking this into patchbay

$ cd pull-ws
$ npm link
$ cd ../patchbay
$ npm link pull-ws
$ cd node_modules/ssb-server
$ rm -rf pull-ws && npm link pull-ws  // because ssb-server is shrink-wrapped?
$ cd ..
$ npm ls pull-ws

[email protected] /home/mix/projects/SSBC/patchbay
├─┬ [email protected]
│ └─┬ [email protected]
│   └── UNMET DEPENDENCY [email protected] 
└─┬ [email protected]
  └─┬ [email protected]
    └── UNMET DEPENDENCY [email protected] 

npm ERR! missing: [email protected], required by [email protected]
npm ERR! missing: [email protected], required by [email protected]

I think this is a good test? I'm seeing replication working fine and blobs are chill...

everything working fine so far

[dominictarr]

I think this is a good test? I'm seeing replication working fine and blobs are chill...

does patchbay use websockets for those things?

[dominictarr]

I think that ssb-server/test/bin might be the main thing that actually tests this.

[dominictarr]

@mixmix I added some commits so that the tests pass without changing the tests (went back to strictEqual) it looks like ws now gives an error event that has the error as a property, but to be consistent with everything, we want to just return the error. That makes this just a patch change.

[mixmix]

patchbay doesn't use websockets to my knowledge, good point.
Does this need more attention / modification, or is it good to go?

I will try that ssb-server test you mentioned and report back

[mixmix]

@dominictarr good call, that didn't pass. I've tried reading into what's going wrong but got lost.

It fails on https://github.com/ssbc/multiserver/blob/master/plugins/ws.js#L65-L72

      WS.createServer(Object.assign({}, opts, {server: server}), function (stream) {
        stream.address = safe_origin(
          stream.headers.origin,    // <<<  stream.headers is undefined
          stream.remoteAddress,
          stream.remotePort
        )
        onConnect(stream)
})

[mixmix]

ok this line gets the tests passing for :

• this module
• ssb-server/test/bin.js

This fix would stop us needing a breaking change ... but feels a bit ech? don't know how much duplex.js is getting used directly

[dominictarr]

I think a cleaner option would be to pass the headers directly to duplex (I don't think anything uses it directly, but client.js and server.js need it) or, at least, to pass just the origin header. A WebSocket object created in the browser doesn't even have a headers property, but multiserver sets the address from the address you passed it, in that case.

[dominictarr]

oh I also ran this against the multiserver tests and found another place it breaks.
however, in this case, I think it's multiserver that is causing the problem...

[mixmix]

So you mean we should change the code to something like :

      var opts = {
        header: req.headers,
        remoteAddress: req.connection.remoteAddress
      }
      var stream = ws(socket, opts)   // here ws is duplex.js
      emitter.emit('connection', stream)

[christianbundy]

Oops, I just opened #27 because I hadn't seen this. Anything I can do to help move this forward?

[arj03]

@christianbundy maybe push your other dev-deps updated to this branch. I'll do some tests with this branch locally and see what comes up.

[arj03]

Been testing this in ssb browser for a few hours tonight with no problems. Would be nice to get the last things fixed if they need to and merged.

[DamonOehlman]

Is it fair to say that all that is required to get this over the line is the .travis.yml updates. In the branch I played around with I found that the following worked a treat:

language: node_js

node_js:
- 14
- 13
- 12
- 11
- 10
- 9
- 8

sudo: required
services:
  - xvfb

before_script:
  - export DISPLAY=:99.0

[arj03]

@DamonOehlman as far as I can see the patch work. But it might be a good idea to have this comment cleaned up and it would be a good idea to probably also upgrade tap & tape while we are at it.

[arj03]

Is there anything I can do to help move this issue forward @DamonOehlman ? I don't have write access to this organization and would like to have this merged so that I can close an issue in multiserver. As for that comment, I think that can be done in a separate PR.

Thanks :-)

[DamonOehlman]

@arj03 Thanks for the ping. I'll admit I forgot about this and then got swamped with some general life things. I'll make myself a note to look at sorting this out in the next couple of days.

[arj03]

Thank you ❤️