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

Fix incorrect vulnerable manifest path check #186

Conversation

SalimBensiali
Copy link
Contributor

@SalimBensiali SalimBensiali commented Mar 24, 2022

Closes #185

@SalimBensiali SalimBensiali requested a review from a team as a code owner Mar 24, 2022
@mwaddell
Copy link
Contributor

mwaddell commented Mar 27, 2022

@SalimBensiali I don't believe that this PR addresses the issue you reported:

image

@SalimBensiali
Copy link
Contributor Author

SalimBensiali commented Mar 27, 2022

This b31caa4 does expose a bug in how getAlert works. There was no coverage for when the manifest file is at the root of the repo

@@ -78,7 +78,7 @@ export async function getAlert (name: string, version: string, directory: string

const nodes = alerts?.repository?.vulnerabilityAlerts?.nodes
const found = nodes.find(a => (version === '' || a.vulnerableRequirements === `= ${version}`) &&
trimSlashes(a.vulnerableManifestPath) === `${trimSlashes(directory)}/${a.vulnerableManifestFilename}` &&
Copy link
Contributor Author

@SalimBensiali SalimBensiali Mar 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwaddell without the propsed change, when package.json is at root, you get the following:

  • a.vulnerableManifestPath equals /package.json which makes trimSlashes(a.vulnerableManifestPath) resolve to package.json
  • `${trimSlashes(directory)}/${a.vulnerableManifestFilename}` is equivalent to `${trimSlashes('/')}/package.json` which yields /package.json

And so basically the slash trimmed paths never match, ending up making getAlert never return a matching security alert

Copy link
Contributor

@mwaddell mwaddell Mar 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now - thank you for the clarification

@SalimBensiali
Copy link
Contributor Author

SalimBensiali commented Mar 27, 2022

@mwaddell I will run the dry-run command to verify on my end

mwaddell
mwaddell previously approved these changes Mar 27, 2022
Copy link
Contributor

@mwaddell mwaddell left a comment

@brrygrdn please merge

brrygrdn
brrygrdn previously approved these changes Mar 29, 2022
@brrygrdn brrygrdn enabled auto-merge Mar 29, 2022
@brrygrdn
Copy link
Member

brrygrdn commented Mar 29, 2022

Thanks for this @SalimBensiali - it looks like the dist/ folder may be out of sync - can you run npm run build and check in any generated changes?

auto-merge was automatically disabled Mar 29, 2022

Head branch was pushed to by a user without write access

@SalimBensiali SalimBensiali dismissed stale reviews from brrygrdn and mwaddell via aa4ffba Mar 29, 2022
@SalimBensiali
Copy link
Contributor Author

SalimBensiali commented Mar 29, 2022

Done

Copy link
Member

@brrygrdn brrygrdn left a comment

🚀

@brrygrdn brrygrdn enabled auto-merge Mar 30, 2022
@brrygrdn brrygrdn merged commit 7e50846 into dependabot:main Mar 30, 2022
2 checks passed
@SalimBensiali SalimBensiali deleted the fix-incorrect-vulnerable-manifest-path-check branch Mar 30, 2022
@brrygrdn brrygrdn mentioned this pull request Apr 20, 2022
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.

3 participants