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

upgrade to ws@7 #26

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

upgrade to ws@7 #26

wants to merge 5 commits into from

Conversation

mixmix
Copy link
Member

@mixmix mixmix commented Jun 5, 2019

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

test/error.js Outdated Show resolved Hide resolved
@mixmix mixmix requested a review from dominictarr June 5, 2019 01:53
@mixmix
Copy link
Member Author

mixmix commented Jun 5, 2019

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

patchbay@8.0.1 /home/mix/projects/SSBC/patchbay
├─┬ ssb-server@14.1.12
│ └─┬ multiserver@3.3.2
│   └── UNMET DEPENDENCY pull-ws@3.3.2 
└─┬ ssb-ws@6.0.0
  └─┬ multiserver@3.3.1
    └── UNMET DEPENDENCY pull-ws@3.3.2 

npm ERR! missing: pull-ws@3.3.2, required by multiserver@3.3.2
npm ERR! missing: pull-ws@3.3.2, required by multiserver@3.3.1

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

everything working fine so far

@dominictarr
Copy link
Member

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
Copy link
Member

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

@dominictarr
Copy link
Member

@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
Copy link
Member Author

mixmix commented Jun 8, 2019

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
Copy link
Member Author

mixmix commented Jun 8, 2019

@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)
})

@@ -38,9 +38,10 @@ module.exports = !WebSocket.Server ? null : function (opts, onConnection) {
proxy(server, 'request')
proxy(server, 'close')

wsServer.on('connection', function (socket) {
wsServer.on('connection', function (socket, req) {
socket.upgradeReq = req // mix: kinda gross hack to preserve the API of duplex.js, but might confuse users...
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@arj03 arj03 mentioned this pull request Apr 6, 2020
@arj03
Copy link

arj03 commented Apr 6, 2020

@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
Copy link

arj03 commented Apr 6, 2020

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
Copy link
Member

DamonOehlman commented May 30, 2020

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
Copy link

arj03 commented Jun 1, 2020

@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
Copy link

arj03 commented Sep 1, 2020

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
Copy link
Member

@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
Copy link

arj03 commented Sep 2, 2020

Thank you ❤️

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

Successfully merging this pull request may close these issues.

secruity issue in dependency ws
5 participants