fix: address npm audit security report #42
Conversation
.travis.yml
Outdated
| deploy: | ||
| - provider: script | ||
| script: ./bin/nlm.js release | ||
| script: ./node_modules/.bin/nlm release |
|
lib/git/commits.js
Outdated
| @@ -54,14 +54,14 @@ function parseCommit(commit) { | |||
| const parentSha = meta.shift() || null; | |||
|
|
|||
| const data = commitParser.sync(message, { | |||
| issuePrefixes: ['#', 'https?://\\w[\\w.-]*[\\w/-]+?'], | |||
| issuePrefixes: ['#', 'https?://\\w[\\w.-]*[\\D/-]+'], | |||
There was a problem hiding this comment.
\D includes literally anything that isn't a digit, including e.g. ]
| }); | ||
| const prMatch = message.match(PR_MERGE_PATTERN); | ||
| if (prMatch) { | ||
| const prId = prMatch[1]; | ||
| data.type = 'pr'; | ||
| data.pullId = prId; | ||
| data.references.push({ | ||
| Object.assign(data.references[0], { |
There was a problem hiding this comment.
No problem. After updating the conventional-commits-parser library it seemed that there was already an array index of 0 present, and this test was expecting the original code to push those values into an empty array.
I thought it best to not disturb the original functionality
There was a problem hiding this comment.
As I parse it, the test was expecting there to be no references, and hence our push()ed one is [0].
What's changed is that there now are some... I'd vote:
- figure out why (what are the references we're getting now that we weren't before)
- if they're good things, change the test to search for our push()ed item, rather than assuming it's at
[0] - if they're things we don't want, delete them and push ours
package.json
Outdated
| @@ -16,7 +16,11 @@ | |||
| "scripts": { | |||
| "pretest": "eslint lib test", | |||
| "test": "mocha", | |||
| "posttest": "./bin/nlm.js verify" | |||
| "posttest": "nlm verify" | |||
package.json
Outdated
| "eslint-plugin-node": "^5.1.1", | ||
| "eslint-plugin-prettier": "^2.2.0", | ||
| "mocha": "^3.1.2", | ||
| "mocha": "^5.2.0", | ||
| "nlm": "^3.0.0", |
There was a problem hiding this comment.
+1 we want to self-publish, explicitly. That way we can't accidentally release something that can't release.
|
I'm not seeing the "vendor package" you mentioned |
|
@dbushong the vendor package was removed as I found an updated version of the library in another repo 👍 |
lib/git/commits.js
Outdated
| @@ -54,7 +54,7 @@ function parseCommit(commit) { | |||
| const parentSha = meta.shift() || null; | |||
|
|
|||
| const data = commitParser.sync(message, { | |||
| issuePrefixes: ['#', 'https?://\\w[\\w.-]*[\\D/-]+'], | |||
| issuePrefixes: ['#', 'https?://[a-zA-Z\\./-]*'], | |||
There was a problem hiding this comment.
Can we have a test case for what this fixes? Is this about multi-digit issue numbers..?
There was a problem hiding this comment.
A story of how I got here:
https?://\\w[\\w.-]*[\\w/-]+?--> was the original regex before updating the conventional-commits-parser library, and did not capture a full portional of a JIRA url after updating the libraryhttps?://\\w[\\w.-]*[\\D/-]+--> did capture the JIRA url appropriately, but as @dbushong pointed out \D is very inclusivehttps?://[a-zA-Z\\./-]*--> I decided to start over and attempt to match the two types of URLS in question
An example of a test failing after updating conventional-commits-parser is: https://github.com/groupon/nlm/blob/master/test/steps/pending-changes.test.js#L58-L64
It was failing because after the update it failed to capture https://jira.atlassian.com/browse/REPO- from https://jira.atlassian.com/browse/REPO-2001.
The result was prefix equalling REPO instead of REPO- as intended.
I didn't write any new tests about what this change fixes but ensured the 3 failing tests passed after the library update.
There was a problem hiding this comment.
Can we add a test case for a URL like https://gh3.internal:4000/foo/bar/issues/42? IIRC the previous one was meant to be a non-greedy capture of "anything goes". Maybe just being more explicit about the final character (- or /) would make the difference..?
Running
npm auditresulted in some vulnerabilities being detected.Other than some normal package updates the archived
conventional-commits-parserlibrary was imported as a vendor package, and its lodash dependency was updated locally to a security compliant version.