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

Backport Security Fix to 6.2.1 #1895

Closed
1 task done
loren138 opened this issue Jun 1, 2021 · 15 comments
Closed
1 task done

Backport Security Fix to 6.2.1 #1895

loren138 opened this issue Jun 1, 2021 · 15 comments

Comments

@loren138
Copy link

loren138 commented Jun 1, 2021

  • I've searched for any related issues and avoided creating a duplicate
    issue.

Description

Any chances the security fix patch 00c425e could be backported to 6.2.1 to release a 6.2.2 with the fix?

Webpack-dev-server currently uses 6.2.1 which has caused a flagged security issue in a lot of repos that can't be fixed until people can upgrade to the not yet stable webpack-dev-server 4. webpack/webpack-dev-server#3360 (Incidentally, we are using webpack-dev-server as a dependency of react-scrips so it will probably be a long time before react-scripts updates to webpack-dev-server v4.)

Admittedly being a dev server, this is (hopefully) only local, but it would be nice not to have a security alert stuck on our github repository.

Reproducible in:

  • version: 6.2.1
  • Node.js version(s):
  • OS version(s):

Steps to reproduce:

  1. Install webpack-dev-server

Expected result:

No security issue

Actual result:

Flagged security issues

@lpinca
Copy link
Member

lpinca commented Jun 1, 2021

ws@6 is no longer maintained. Is there anything that prevents them from upgrading to ws@7? [email protected] was release more than two years ago.

@lpinca
Copy link
Member

lpinca commented Jun 1, 2021

@lpinca
Copy link
Member

lpinca commented Jun 1, 2021

I've released [email protected] and updated the security advisory. I don't know if the change will be automatically reflected to the GitHub Advisory Database and processed accordingly.

@dacevedo12
Copy link

Will the npm advisory be updated too? https://www.npmjs.com/advisories/1748/versions

@G-Rath
Copy link

G-Rath commented Jun 2, 2021

@dacevedo12 it should do eventually yes - sending a ticket to npm support advising them about the advisory update, it can help expedite matters.

@lpinca I don't mind doing this, but I think it could have more weight if you send it as a maintainer of the package :)

@danishsatkut
Copy link

@G-Rath I am not sure if this is the right place to ask, but do you know the process of updating the npm advisory db and how it works?

@G-Rath
Copy link

G-Rath commented Jun 2, 2021

@danishsatkut The advisory database is managed by npm (it's not something that's stored locally like ruby-advisory-db or security-advisories), so updates are performed by their security team.

So the process on the sides of us devs is to contact their support if we have new information about an advisory, as I said in my previous comment :)

I believe there is an actual security channel somewhere for submitting actual security advisories, but I'm not well versed in that side of the security world so don't know what that actually is 🤷

This was referenced Jun 6, 2021
@glasser
Copy link

glasser commented Jun 8, 2021

I'm sure you're even less excited by this request, but: any chance of a backport to ws@5?

(Context: I am a maintainer of Apollo Server. We are in the process of a big major release that entirely removes our core dependency on ws by moving websocket support to be something external from our library. However, our current release depends indirectly on ws@5 via an old library (subscriptions-transport-ws) that we haven't been actively maintaining. I'm thus not super inspired to put a bunch of effort into figuring out if subscriptions-transport-ws needs work to be upgraded to ws@7 given that we are planning to entirely drop the dependency soon... but in the meantime our current users are seeing the CVE. I realize this is more of an us problem than a you problem... npm stats makes it look like subscriptions-transport-ws has about 4% of the downloads of ws, not sure if that's a large number or a small number.)

@lpinca
Copy link
Member

lpinca commented Jun 8, 2021

@glasser the 6.x release line had only small breaking changes from 5.x. See https://github.com/websockets/ws/releases/tag/6.0.0. Are there any blockers?

@glasser
Copy link

glasser commented Jun 8, 2021

Mostly just not wanting to deal with pushing any breaking changes to users when we're trying to reduce our investment in this part of our system. For example, maybe the new default maxPayload limit of 100MB is a breaking change for some users, so we would have to give a way to thread that option through our API? It looks like the patch would apply directly to version-5. Would it help if I file a PR?

@lpinca
Copy link
Member

lpinca commented Jun 8, 2021

For example, maybe the new default maxPayload limit of 100MB is a breaking change for some users, so we would have to give a way to thread that option through our API?

Yes but it's trivial no? Just add the maxPayload option in the options object

const ws = new WebSocket(url, { maxPayload: Infinity });

I prefer to help you upgrade to ws@6 or ws@7 and cut a new patch version of subscriptions-transport-ws rather than backporting this patch to the 5.x release line.

@lpinca
Copy link
Member

lpinca commented Jun 8, 2021

@glasser I've released [email protected] and updated the GitHub security advisory. Please contact npm support to make them update their advisory yourself.

@glasser
Copy link

glasser commented Jun 8, 2021

Thank you! I did also cut a new patch version of subscriptions-transport-ws in parallel with your work based on your previous comment, but I think both are helpful.

@glasser
Copy link

glasser commented Jun 8, 2021

And I contacted npm support.

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

No branches or pull requests

7 participants