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

Security Vulnerability, openSSL Unit-Testing #1925

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

Conversation

Segmentational
Copy link

Hello! I know I'm new here, but the following change(s) are quite non-invasive and I thought I'd contribute back :D.

Overview

got had a moderate security issue that was thrown. The following PR aims to fix that.

Additionally, I've provided solutions to other larger open source npm packages relating to the minimist issues (a dependency of the very popular mocha package), but as this is my first time around nodegit, I thought it best to first see how willing the maintainers are to accept my PR (while limited, it seems some either copy and replicate my changes, or haven't bothered to look into the proposals).

Discussion

It seems that got, a dependency, is getting used only in the util directory.

Further, specific usage of the module where got is initialized is limited to acquireOpenSSL.js.

I thought to also create some unit testing as I saw that was a point of needed contributions.

Further diving down into the rabbit hole, I found that the binding for node-gyp auto-generates with darwin specified
to 10.11 as the minimum version; because I'm unable to verify (10.15) on the lower versions, I set the test to it.skip.

However, I was unable to successfully compile openSSL v1.1.1l. I updated the reference (a magic constant in acquireOpenSSL.js) to v1.1.1p. Thereafter I was able to successfully build wooohoooooo.

I believe usage of these directives is still yet limited to electron applications; but I'm not 100% certain on this one.


Note that while my personal testing included changing the v1.1.1l version to v1.1.1p, I did set it back.

@Segmentational
Copy link
Author

Segmentational commented Jul 14, 2022

Note that the following PR is also open:

Accepting that PR will break usage. It seems our friend didn't test his changes.

I'm actually unsure if version 11 was the version where ESM was implemented. This PR's change does account for ESM, and is of the latest got version.

@gpetrov
Copy link

gpetrov commented Sep 1, 2022

Please update openSSL to 1.1.1p as the current 1.1.1l has compilation problems due to failing tests @ianhattendorf

astiob added a commit to astiob/digimon-rearise-js that referenced this pull request Sep 7, 2022
AFAICT NodeGit's only uses of Got are trivial and indeed limited
to a script in its util directory, which is only used when compiling
NodeGit from source. So an override shouldn't break anything
(and indeed any Got vulnerability here might not matter at all,
but let's keep Dependabot happy).

These findings are corroborated by:
nodegit/nodegit#1925
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.

None yet

2 participants