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

Update package-lock.json parsing strategy #1666

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

eleftherias
Copy link
Contributor

  • Start by finding any version changes
  • Then find the package name whose version was changed

Fix #1631

@eleftherias eleftherias requested a review from jhrozek November 15, 2023 16:05
- Start by finding any version changes
- Then find the package name whose version was changed

Fix mindersec#1631
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

This is a clear and nice improvement thank you.

While reading the PR I was wondering if the approach that I took with the original code where we parse the patch is just too fragile though and we'd find even more issues later. What if we did the following instead (later of course):

  • grab the whole file that was patched and parse all dependencies to get a slice of pb.Dependency
  • grab the original and parse it, too
  • see which dependencies changed
  • find them in the patch to get context
  • use that instead to add reviews at the right places

The reason I started thinking about this was really this test case:

+   },
+    "node_modules/undici-types": {
+      "version": "5.26.5",
+      "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-5.26.5.tgz",
+      "integrity": "sha512-JlCMO+ehdEIKqlFxk6IfVoAUVmgz7cU7zD/h9XZ0qzeosSHmUJVOzSQvvYSYWXkFXC+IfLKSIffhv0sVZup6pA=="

and the fact that package-lock.json might be just so easy to parse otherwise and I bet there's also libraries we could use.

@eleftherias
Copy link
Contributor Author

This is a clear and nice improvement thank you.

While reading the PR I was wondering if the approach that I took with the original code where we parse the patch is just too fragile though and we'd find even more issues later. What if we did the following instead (later of course):

  • grab the whole file that was patched and parse all dependencies to get a slice of pb.Dependency
  • grab the original and parse it, too
  • see which dependencies changed
  • find them in the patch to get context
  • use that instead to add reviews at the right places

The reason I started thinking about this was really this test case:

+   },
+    "node_modules/undici-types": {
+      "version": "5.26.5",
+      "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-5.26.5.tgz",
+      "integrity": "sha512-JlCMO+ehdEIKqlFxk6IfVoAUVmgz7cU7zD/h9XZ0qzeosSHmUJVOzSQvvYSYWXkFXC+IfLKSIffhv0sVZup6pA=="

and the fact that package-lock.json might be just so easy to parse otherwise and I bet there's also libraries we could use.

@jhrozek I agree that's a cleaner way to do it. We might need to rethink how we define the rule, because if we're getting the whole file instead of the patch we need something other than the diff ingester.

@eleftherias eleftherias merged commit 9a391fd into mindersec:main Nov 16, 2023
13 checks passed
@eleftherias eleftherias deleted the vuln-json-parse branch November 16, 2023 11:30
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.

Unmarshalling package-lock.json can cause errors
2 participants