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.

Sign up for GitHub

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

Jump to bottom

bin-wrapper is problematic #134

Closed
boneskull opened this issue Jan 5, 2022 · 4 comments · Fixed by #135
Closed

bin-wrapper is problematic #134

boneskull opened this issue Jan 5, 2022 · 4 comments · Fixed by #135
Assignees
@wswebcreation
Labels
bug

Comments

@boneskull
Copy link
Contributor

bin-wrapper is seemingly unmaintained. There's a ReDoS vuln in a deep transitive dependency (semver-regex). Sending a PR to address this problem (to bin-wrapper) is probably futile. Here are our options:

  1. Remove bin-wrapper and replace it with something else. That something else might mean "DIY".
  2. Look for a fork of bin-wrapper which has already addressed the vuln, and use that instead (probably unlikely).
  3. Fork bin-wrapper, update the bin-version-check dependency to v5 or newer, and re-publish (under e.g., @saucelabs/bin-wrapper). However, bin-version-check@5 is an ESM-only module, which will probably be painful to deal with.

Thoughts? We are getting complaints.
@boneskull boneskull added the bug label Jan 5, 2022
@boneskull
Copy link
Contributor Author

FWIW, using [email protected] would allow either us or a consumer (not sure) to manually override the version of semver-regex, but that version does not ship with LTS yet.

@boneskull
Copy link
Contributor Author

Further:

  • bin-wrapper uses an old version of bin-version-check
  • bin-version-check attempts to parse the version displayed by <executable> --version using semver
  • but it does not appear we actually need to parse the version; all we need to do is grep for the required version as returned by sc.

bin-version is pretty straightforward, and we can simplify it if rolling our own, which I don't believe is unreasonable.

@wswebcreation
Copy link
Contributor

I can take a look at it tomorrow @boneskull

@wswebcreation wswebcreation self-assigned this Jan 6, 2022
@boneskull
Copy link
Contributor Author

@wswebcreation Any thoughts? If you don't have the cycles, I am happy to work on this.

@wswebcreation wswebcreation mentioned this issue Jan 7, 2022
Merged
boneskull added a commit that referenced this issue Jan 10, 2022
@wswebcreation @boneskull
replace bin-wrapper; closes #134 (#135)
d79c01b 
* initial fix for issue 134

* chore(binWrapper): fix formatting w/ eslint

* chore: ignore .vscode

* chore: add .prettierrc

* test: provide instances export

* chore: fix formatting in src/index.js; remove unused var

* test: fix binWrapper tests

This change removes the `bin-wrapper` mock file, which we no longer need since we no longer use `bin-wrapper`.  Instead, it moves the mock for `src/binWrapper.js` into the test file, since this seems to be how we mock local modules (as opposed to dependencies).

This is not ideal, because now that `bin-wrapper` is _our_ code, we should not be mocking it at all.  It'd make sense to then create a mock for `download`.  As a result, `src/binWrapper.js` _has no test coverage_, which is bad.  If we want to keep it, we should consider copying (and likely converting to jest) `bin-wrapper`'s tests into our repo.

* chore(pkg): update lockfile

* test: fix reference to fs/promises, which is different in node v12

* Update src/index.js

Co-authored-by: Christopher Hiller <[email protected]>

* chore: refactor after feedback

* chore: remove consoles

* fix: broken var reference

- fix bad formatting
- refactor generation of `scData` to use `reduce()`
- if e2e tests are run outside of CI, use `(local)` instead of `undefined` in the tunnel name

* chore: add .eslintignore

* fix: simplify and fix chmod behavior

1. we don't need to iterate through `SAUCE_CONNECT_PLATFORM_DATA` because we know what platform we're on.  just use the platform as the key and reference it.
2. we don't need to chmod _everything_ to 755 (I think).  we _do_ need to ensure the `sc` bin is executable (but not on windows)
3. we don't need to retain the `options` object for anything
4. added a guard against unsupported platforms and a test
5. further simplify `sauceConnectLoader`

* test: increase coverage to threshold

* test: try to simplify some things

Co-authored-by: Christopher Hiller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@wswebcreation wswebcreation

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

initial fix for issue 134 saucelabs/node-saucelabs
2 participants
@boneskull @wswebcreation