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

High severity audit report from npm: browser-sync -> socket.io -> engine.io #2213

Closed
zachleat opened this issue Feb 10, 2022 · 8 comments
Closed
Assignees
Labels
high-priority npm-audit Security audits from npm waiting-to-close Issue that is probably resolved, waiting on OP confirmation.

Comments

@zachleat
Copy link
Member

zachleat commented Feb 10, 2022

Filed upstream at BrowserSync/browser-sync#1926

Possible upstream home base at BrowserSync/browser-sync#1850

@zachleat zachleat added the npm-audit Security audits from npm label Feb 10, 2022
@zachleat zachleat self-assigned this Feb 10, 2022
@zachleat
Copy link
Member Author

One alternative here for folks that are more npm audit friendly—we could potentially provide a version of Eleventy that does not have the dev server bundled to avoid some of these auditing issues moving forward?

@johnhunter
Copy link

That's a good point @zachleat - maybe it could work as a separate npm module, although similar problems could emerge with other dependencies.

There is a general problem with npm audit because it cannot tell if a JS vulnerability is going to end up in deployed production code, or in a build process (on cloud CI), or in this case a local development machine. Dan Abramov has voiced strong opinions on this. I'm not sure best answer but hopefully npm can come up with a more nuanced way of identifying vulnerabilities.

@besenwagen
Copy link

besenwagen commented Feb 15, 2022

I would love to see that decoupled. First time I looked at 11ty, there was a high severity issue in browser-sync, so I moved on, and this was the second time I had a look. 🤷

Arguing that it does not matter if it is not used in production should not be a choice that developers should have to make. Home directories are very attractive targets for stealing ssh keys, authorization tokens, or just mining the web number 3. 🙄

@zachleat
Copy link
Member Author

Alright, I’m starting to lean towards a new code structure to better solve this problem, probably in a few steps.

Notably I should say up front that @11ty/eleventy will stay as-is, and even though some dependencies may move out of the repo, it will consume these other dependencies as needed so that no breaking changes are made.

  1. Publish a @11ty/eleventy-build that exposes the programmatic API and core build-code
  2. Not sold on this one yet but optionally separate out template languages from eleventy-build as plugins, related: Move some of the default plugins into plugin land #1103 Decouple 11ty.js rendering from Eleventy core #668
  3. Publish a @11ty/eleventy-serve-browsersync package for folks that want to use browser-sync for --start. Might also be nice to have a standard serve API so we can easily plug in other servers (maybe vite?)
  4. Update @11ty/eleventy to point to these new deps.

@zachleat
Copy link
Member Author

Couple of updates here:

I do want to emphasize that there is very little production risk from these audits. Eleventy --serve is a development only tool and we do not bundle browser-sync with serverless functions bundles (it’s too big to fit in the 50MB limit).

The approach documented above #2213 (comment) won’t solve the problem for new and current 1.x users, it will only allow some folks to opt-in to an escape hatch.

I’m leaning towards an Eleventy 2.0 release that either moves browser-sync to be an optional peer dependency or removes it altogether. The core --serve experience will instead by-default use an (ideally smaller) alternative. Some background on that in #1305

Want to also keep in mind this comment, too:

Might also be nice to have a standard serve API so we can easily plug in other servers (maybe vite?)

@zachleat zachleat added this to the Eleventy 2.0.0 milestone Feb 28, 2022
@GrimLink
Copy link

GrimLink commented Mar 8, 2022

@zachleat good news, PR BrowserSync/browser-sync#1936 has been merged.
This fixes the high severity issue.
The wait is just for a new release of browserSync.

@zachleat
Copy link
Member Author

zachleat commented Mar 9, 2022

Great @GrimLink!

We are also solving this on our end at #1305, now available for testing on the 2.0 canary release

I’ll leave this one open until 2.0 stable launches or browser-sync cuts a new release with the audits fixed

@zachleat zachleat added the waiting-to-close Issue that is probably resolved, waiting on OP confirmation. label Mar 10, 2022
@zachleat
Copy link
Member Author

zachleat commented Apr 5, 2022

Forgot to close this one but this issue specifically was fixed on Eleventy 1.0. https://twitter.com/eleven_ty/status/1504531174642880515

Going to move it to the 1.0 milestone as Browsersync fixed it upstream.

Also note that Eleventy Dev Server will be the default moving forward in 2.0: https://www.11ty.dev/docs/watch-serve/#eleventy-dev-server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high-priority npm-audit Security audits from npm waiting-to-close Issue that is probably resolved, waiting on OP confirmation.
Projects
None yet
Development

No branches or pull requests

4 participants