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

Support allowlisting private packages by module #252

Closed
quinnturner opened this issue May 4, 2022 · 7 comments
Closed

Support allowlisting private packages by module #252

quinnturner opened this issue May 4, 2022 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@quinnturner
Copy link
Member

From @gimyboya #170 (comment):

I am getting the following error which makes me question the purpose of the allowlist:

Modules to allowlist: proxy-client-ts, tenant-client-ts.
Consider not allowlisting modules: proxy-client-ts, tenant-client-ts.
Found vulnerable advisory paths:
GHSA-pw2r-vq6v-hr8c|proxy-client-ts>axios>follow-redirects
GHSA-pw2r-vq6v-hr8c|tenant-client-ts>axios>follow-redirects
GHSA-74fj-2j2h-c42q|proxy-client-ts>axios>follow-redirects
GHSA-74fj-2j2h-c42q|tenant-client-ts>axios>follow-redirects
Failed security audit due to high, moderate vulnerabilities.
Vulnerable advisories are:
https://github.com/advisories/GHSA-pw2r-vq6v-hr8c
https://github.com/advisories/GHSA-74fj-2j2h-c42q
Exiting...

How can I exclude those two libraries?

This behaviour is because proxy-client-ts and tenant-client-ts are not on the registry. Accordingly, they are not present in the audit response(? TBD, I don't know). This makes allowlisting the modules more difficult as we have to use information outside the scope of just the audit response.

@quinnturner quinnturner added the enhancement New feature or request label May 4, 2022
@andrewdetorres
Copy link

Hey @quinnturner, I'm interested on working on this if thats okay? 😄

@quinnturner
Copy link
Member Author

Hey @andrewdetorres, yep, I would accept a PR that adds this functionality! If you have questions, feel free to drop them here 😄

@quinnturner quinnturner added the good first issue Good for newcomers label May 6, 2022
@andrewdetorres
Copy link

andrewdetorres commented May 6, 2022

Hey @quinnturner,

After taking a look into this issue, the way that the code works with module name allowlist is that the package name with the vulnerability should be used. In the example above GHSA-pw2r-vq6v-hr8c|proxy-client-ts>axios>follow-redirects this should be follow-redirects instead of proxy-client-ts.

Having the root package in the allowlist would be a cool feature so you don't have to write out the full path. Is this something you would like to consider as a feature? 😄

@quinnturner
Copy link
Member Author

Root module advisory allow listing is supported with *|module-name>*. I believe the problem is that since these are private packages, the private package is not listed in the audit. Unfortunately, that means adding context of the private dependency in the audit response.

I haven't reviewed how the package manager's audit responds to private dependencies. If they still handle the transitive dependencies, there may we a way to workaround it with a different allowlist without much or any code.

@andrewdetorres
Copy link

It appears that the above syntax *|module-name>* works with private packages. Having tested this with our own private package and the advisories were correctly ignored.

The root module advisory allow listing support seems slightly hidden in the docs. I'm happy to make a contribution to make this more clear if you're happy for me to go ahead with that?

@quinnturner
Copy link
Member Author

quinnturner commented May 6, 2022

I am always interested in making the documentation better! Allowlisting all transitive dependencies of a package is not usually recommended workflow because legitimate advisories may slip through, so the wording would have to be considered. However, I am open to PRs!

@quinnturner
Copy link
Member Author

I am closing for now because of the improved documentation. While this is still technically an issue, it can be worked around and "fixing it" involves mutating the received audit. If "natively" solving this receives a lot of positive desire, I will consider reopening!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants