Skip to content

Commit

Permalink
Fix aggregated pull requests with multi project/working dirs (#386)
Browse files Browse the repository at this point in the history
  • Loading branch information
omerzi authored Jul 23, 2023
1 parent 58ef270 commit ecc49ca
Show file tree
Hide file tree
Showing 75 changed files with 829 additions and 365 deletions.
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ coverage
.idea
.vscode
*.iml
.DS_Store
.DS_Store
commands/testdata/**/hooks/
2 changes: 1 addition & 1 deletion action/node_modules/.package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

373 changes: 235 additions & 138 deletions commands/createfixpullrequests.go

Large diffs are not rendered by default.

268 changes: 195 additions & 73 deletions commands/createfixpullrequests_test.go

Large diffs are not rendered by default.

4 changes: 1 addition & 3 deletions commands/scanandfixrepos.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ import (
type ScanAndFixRepositories struct {
// dryRun is used for testing purposes, mocking part of the git commands that requires networking
dryRun bool
// When dryRun is enabled, dryRunRepoPath specifies the repository local path to clone
dryRunRepoPath string
}

func (saf *ScanAndFixRepositories) Run(repoAggregator utils.RepoAggregator, client vcsclient.VcsClient) error {
Expand Down Expand Up @@ -56,6 +54,6 @@ func (saf *ScanAndFixRepositories) downloadAndRunScanAndFix(repository *utils.Re
err = errors.Join(err, restoreDir())
}()

cfp := CreateFixPullRequestsCmd{dryRun: saf.dryRun, dryRunRepoPath: saf.dryRunRepoPath}
cfp := CreateFixPullRequestsCmd{dryRun: saf.dryRun, dryRunRepoPath: wd}
return cfp.scanAndFixRepository(repository, branch, client)
}
2 changes: 1 addition & 1 deletion commands/scanandfixrepos_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func TestScanAndFixRepos(t *testing.T) {
configAggregator, err := utils.BuildRepoAggregator(configData, &gitTestParams, &serverParams)
assert.NoError(t, err)

var cmd = ScanAndFixRepositories{dryRun: true, dryRunRepoPath: tmpDir}
var cmd = ScanAndFixRepositories{dryRun: true}
assert.NoError(t, cmd.Run(configAggregator, client))
}

Expand Down
12 changes: 4 additions & 8 deletions commands/scanpullrequest.go
Original file line number Diff line number Diff line change
Expand Up @@ -343,14 +343,14 @@ func getNewViolations(targetScan, sourceScan services.ScanResponse, auditResults
return violationsRows, err
}
for _, violation := range violationsRows {
existsViolationsMap[getUniqueID(violation)] = violation
existsViolationsMap[utils.GetUniqueID(violation)] = violation
}
violationsRows, _, _, err = xrayutils.PrepareViolations(sourceScan.Violations, auditResults.ExtendedScanResults, auditResults.IsMultipleRootProject, true)
if err != nil {
return newViolationsRows, err
}
for _, violation := range violationsRows {
if _, exists := existsViolationsMap[getUniqueID(violation)]; !exists {
if _, exists := existsViolationsMap[utils.GetUniqueID(violation)]; !exists {
newViolationsRows = append(newViolationsRows, violation)
}
}
Expand All @@ -364,24 +364,20 @@ func getNewVulnerabilities(targetScan, sourceScan services.ScanResponse, auditRe
return newVulnerabilitiesRows, err
}
for _, vulnerability := range targetVulnerabilitiesRows {
targetVulnerabilitiesMap[getUniqueID(vulnerability)] = vulnerability
targetVulnerabilitiesMap[utils.GetUniqueID(vulnerability)] = vulnerability
}
sourceVulnerabilitiesRows, err := xrayutils.PrepareVulnerabilities(sourceScan.Vulnerabilities, auditResults.ExtendedScanResults, auditResults.IsMultipleRootProject, true)
if err != nil {
return newVulnerabilitiesRows, err
}
for _, vulnerability := range sourceVulnerabilitiesRows {
if _, exists := targetVulnerabilitiesMap[getUniqueID(vulnerability)]; !exists {
if _, exists := targetVulnerabilitiesMap[utils.GetUniqueID(vulnerability)]; !exists {
newVulnerabilitiesRows = append(newVulnerabilitiesRows, vulnerability)
}
}
return
}

func getUniqueID(vulnerability formats.VulnerabilityOrViolationRow) string {
return vulnerability.ImpactedDependencyName + vulnerability.ImpactedDependencyVersion + vulnerability.IssueId
}

func createPullRequestMessage(vulnerabilitiesRows []formats.VulnerabilityOrViolationRow, iacRows []formats.IacSecretsRow, writer utils.OutputWriter) string {
if len(vulnerabilitiesRows) == 0 && len(iacRows) == 0 {
return writer.NoVulnerabilitiesTitle() + writer.UntitledForJasMsg() + writer.Footer()
Expand Down
2 changes: 1 addition & 1 deletion commands/scanpullrequests.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func isFrogbotRescanComment(comment string) bool {
return strings.Contains(strings.ToLower(strings.TrimSpace(comment)), utils.RescanRequestComment)
}

func downloadAndScanPullRequest(pr vcsclient.PullRequestInfo, repo utils.Repository, client vcsclient.VcsClient) error {
func downloadAndScanPullRequest(pr vcsclient.PullRequestInfo, repo utils.Repository, client vcsclient.VcsClient) (err error) {
// Download the pull request source ("from") branch
params := utils.Params{
Git: utils.Git{
Expand Down
4 changes: 2 additions & 2 deletions commands/scanpullrequests_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func TestScanAllPullRequestsMultiRepo(t *testing.T) {
err := scanAllPullRequestsCmd.Run(configAggregator, client)
assert.NoError(t, err)
assert.Len(t, frogbotMessages, 4)
expectedMessage := "[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/vulnerabilitiesBannerPR.png)](https://github.com/jfrog/frogbot#readme)\n## 📦 Vulnerable Dependencies \n\n### ✍️ Summary\n\n<div align=\"center\">\n\n| SEVERITY | CONTEXTUAL ANALYSIS | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/notApplicableCritical.png)<br>Critical | Not Applicable |minimist:1.2.5 | minimist:1.2.5 | [0.2.4]<br><br>[1.2.6] |\n\n</div>\n\n## 👇 Details\n\n\n\n\n- **Severity** 💀 Critical\n- **Contextual Analysis:** Not Applicable\n- **Package Name:** minimist\n- **Current Version:** 1.2.5\n- **Fixed Version:** [0.2.4],[1.2.6]\n- **CVE:** CVE-2021-44906\n\n**Description:**\n\n[Minimist](https://github.com/substack/minimist) is a simple and very popular argument parser. It is used by more than 14 million by Mar 2022. This package developers stopped developing it since April 2020 and its community released a [newer version](https://github.com/meszaros-lajos-gyorgy/minimist-lite) supported by the community.\n\n\nAn incomplete fix for [CVE-2020-7598](https://nvd.nist.gov/vuln/detail/CVE-2020-7598) partially blocked prototype pollution attacks. Researchers discovered that it does not check for constructor functions which means they can be overridden. This behavior can be triggered easily when using it insecurely (which is the common usage). For example:\n```\nvar argv = parse(['--_.concat.constructor.prototype.y', '123']);\nt.equal((function(){}).foo, undefined);\nt.equal(argv.y, undefined);\n```\nIn this example, `prototype.y` is assigned with `123` which will be derived to every newly created object. \n\nThis vulnerability can be triggered when the attacker-controlled input is parsed using Minimist without any validation. As always with prototype pollution, the impact depends on the code that follows the attack, but denial of service is almost always guaranteed.\n\n**Remediation:**\n\n##### Development mitigations\n\nAdd the `Object.freeze(Object.prototype);` directive once at the beginning of your main JS source code file (ex. `index.js`), preferably after all your `require` directives. This will prevent any changes to the prototype object, thus completely negating prototype pollution attacks.\n\n\n\n\n<div align=\"center\">\n\n[JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n</div>\n"
expectedMessage := "[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/vulnerabilitiesBannerPR.png)](https://github.com/jfrog/frogbot#readme)\n## 📦 Vulnerable Dependencies \n\n### ✍️ Summary\n\n<div align=\"center\">\n\n| SEVERITY | CONTEXTUAL ANALYSIS | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/notApplicableCritical.png)<br>Critical | Not Applicable |minimist:1.2.5 | minimist:1.2.5 | [0.2.4]<br><br>[1.2.6] |\n\n</div>\n\n## 👇 Details\n\n\n\n\n- **Severity** 💀 Critical\n- **Contextual Analysis:** Not Applicable\n- **Package Name:** minimist\n- **Current Version:** 1.2.5\n- **Fixed Versions:** [0.2.4],[1.2.6]\n- **CVE:** CVE-2021-44906\n\n**Description:**\n\n[Minimist](https://github.com/substack/minimist) is a simple and very popular argument parser. It is used by more than 14 million by Mar 2022. This package developers stopped developing it since April 2020 and its community released a [newer version](https://github.com/meszaros-lajos-gyorgy/minimist-lite) supported by the community.\n\n\nAn incomplete fix for [CVE-2020-7598](https://nvd.nist.gov/vuln/detail/CVE-2020-7598) partially blocked prototype pollution attacks. Researchers discovered that it does not check for constructor functions which means they can be overridden. This behavior can be triggered easily when using it insecurely (which is the common usage). For example:\n```\nvar argv = parse(['--_.concat.constructor.prototype.y', '123']);\nt.equal((function(){}).foo, undefined);\nt.equal(argv.y, undefined);\n```\nIn this example, `prototype.y` is assigned with `123` which will be derived to every newly created object. \n\nThis vulnerability can be triggered when the attacker-controlled input is parsed using Minimist without any validation. As always with prototype pollution, the impact depends on the code that follows the attack, but denial of service is almost always guaranteed.\n\n**Remediation:**\n\n##### Development mitigations\n\nAdd the `Object.freeze(Object.prototype);` directive once at the beginning of your main JS source code file (ex. `index.js`), preferably after all your `require` directives. This will prevent any changes to the prototype object, thus completely negating prototype pollution attacks.\n\n\n\n\n<div align=\"center\">\n\n[JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n</div>\n"
assert.Equal(t, expectedMessage, frogbotMessages[0])
expectedMessage = "[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/noVulnerabilityBannerPR.png)](https://github.com/jfrog/frogbot#readme)\n<div align=\"center\">\n\n[JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n</div>\n"
assert.Equal(t, expectedMessage, frogbotMessages[1])
Expand Down Expand Up @@ -188,7 +188,7 @@ func TestScanAllPullRequests(t *testing.T) {
err := scanAllPullRequestsCmd.Run(paramsAggregator, client)
assert.NoError(t, err)
assert.Len(t, frogbotMessages, 2)
expectedMessage := "**🚨 Frogbot scanned this pull request and found the below:**\n\n---\n## 📦 Vulnerable Dependencies\n---\n\n### ✍️ Summary \n\n| SEVERITY | CONTEXTUAL ANALYSIS | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | \n| Critical | Not Applicable | minimist:1.2.5 | minimist:1.2.5 | [0.2.4], [1.2.6] |\n\n---\n### 👇 Details\n---\n\n\n#### minimist 1.2.5\n\n\n- **Severity** 💀 Critical\n- **Contextual Analysis:** Not Applicable\n- **Package Name:** minimist\n- **Current Version:** 1.2.5\n- **Fixed Version:** [0.2.4],[1.2.6]\n- **CVE:** CVE-2021-44906\n\n**Description:**\n\n[Minimist](https://github.com/substack/minimist) is a simple and very popular argument parser. It is used by more than 14 million by Mar 2022. This package developers stopped developing it since April 2020 and its community released a [newer version](https://github.com/meszaros-lajos-gyorgy/minimist-lite) supported by the community.\n\n\nAn incomplete fix for [CVE-2020-7598](https://nvd.nist.gov/vuln/detail/CVE-2020-7598) partially blocked prototype pollution attacks. Researchers discovered that it does not check for constructor functions which means they can be overridden. This behavior can be triggered easily when using it insecurely (which is the common usage). For example:\n```\nvar argv = parse(['--_.concat.constructor.prototype.y', '123']);\nt.equal((function(){}).foo, undefined);\nt.equal(argv.y, undefined);\n```\nIn this example, `prototype.y` is assigned with `123` which will be derived to every newly created object. \n\nThis vulnerability can be triggered when the attacker-controlled input is parsed using Minimist without any validation. As always with prototype pollution, the impact depends on the code that follows the attack, but denial of service is almost always guaranteed.\n\n**Remediation:**\n\n##### Development mitigations\n\nAdd the `Object.freeze(Object.prototype);` directive once at the beginning of your main JS source code file (ex. `index.js`), preferably after all your `require` directives. This will prevent any changes to the prototype object, thus completely negating prototype pollution attacks.\n\n\n\n\n\n[JFrog Frogbot](https://github.com/jfrog/frogbot#readme)"
expectedMessage := "**🚨 Frogbot scanned this pull request and found the below:**\n\n---\n## 📦 Vulnerable Dependencies\n---\n\n### ✍️ Summary \n\n| SEVERITY | CONTEXTUAL ANALYSIS | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | \n| Critical | Not Applicable | minimist:1.2.5 | minimist:1.2.5 | [0.2.4], [1.2.6] |\n\n---\n### 👇 Details\n---\n\n\n#### minimist 1.2.5\n\n\n- **Severity** 💀 Critical\n- **Contextual Analysis:** Not Applicable\n- **Package Name:** minimist\n- **Current Version:** 1.2.5\n- **Fixed Versions:** [0.2.4],[1.2.6]\n- **CVE:** CVE-2021-44906\n\n**Description:**\n\n[Minimist](https://github.com/substack/minimist) is a simple and very popular argument parser. It is used by more than 14 million by Mar 2022. This package developers stopped developing it since April 2020 and its community released a [newer version](https://github.com/meszaros-lajos-gyorgy/minimist-lite) supported by the community.\n\n\nAn incomplete fix for [CVE-2020-7598](https://nvd.nist.gov/vuln/detail/CVE-2020-7598) partially blocked prototype pollution attacks. Researchers discovered that it does not check for constructor functions which means they can be overridden. This behavior can be triggered easily when using it insecurely (which is the common usage). For example:\n```\nvar argv = parse(['--_.concat.constructor.prototype.y', '123']);\nt.equal((function(){}).foo, undefined);\nt.equal(argv.y, undefined);\n```\nIn this example, `prototype.y` is assigned with `123` which will be derived to every newly created object. \n\nThis vulnerability can be triggered when the attacker-controlled input is parsed using Minimist without any validation. As always with prototype pollution, the impact depends on the code that follows the attack, but denial of service is almost always guaranteed.\n\n**Remediation:**\n\n##### Development mitigations\n\nAdd the `Object.freeze(Object.prototype);` directive once at the beginning of your main JS source code file (ex. `index.js`), preferably after all your `require` directives. This will prevent any changes to the prototype object, thus completely negating prototype pollution attacks.\n\n\n\n\n\n[JFrog Frogbot](https://github.com/jfrog/frogbot#readme)"
assert.Equal(t, expectedMessage, frogbotMessages[0])
expectedMessage = "**👍 Frogbot scanned this pull request and found that it did not add vulnerable dependencies.** \n\n\n[JFrog Frogbot](https://github.com/jfrog/frogbot#readme)"
assert.Equal(t, expectedMessage, frogbotMessages[1])
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
- params:
git:
branches:
- main
repoName: "aggregate-multi-dir"
scan:
projects:
- workingDirs:
- npm1
- npm2
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fix tests
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ref: refs/heads/main
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[core]
repositoryformatversion = 0
filemode = true
bare = false
logallrefupdates = true
ignorecase = true
precomposeunicode = true
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Unnamed repository; edit this file 'description' to name the repository.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# git ls-files --others --exclude-from=.git/info/exclude
# Lines that start with '#' are comments.
# For a project mostly in C, the following would be a good set of
# exclude patterns (uncomment them if you want to use them):
# *.[oa]
# *~
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
0000000000000000000000000000000000000000 20cfb0cbabf22f1c12d626eec6e6dd834f4e43d1 Omer Zidkoni <[email protected]> 1689852711 +0300 commit (initial): fix tests
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
0000000000000000000000000000000000000000 20cfb0cbabf22f1c12d626eec6e6dd834f4e43d1 Omer Zidkoni <[email protected]> 1689852711 +0300 commit (initial): fix tests
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x��K
1 @]�� �4�tD��{w��Z�:���+�|/IkU��ig��y��.T��5P)���c��sF��ل�ޥåq�k�OyU8ʠ��Q��I� p^�er���5Î����Ҕ��M7� �7�
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
20cfb0cbabf22f1c12d626eec6e6dd834f4e43d1
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"name": "aggregate",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no tsest specified\" && exit 1"
},
"author": "",
"license": "ISC",
"dependencies": {
"uuid": "^9.0.0",
"minimatch":"3.0.2",
"mpath": "0.7.0"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"minimist": "1.2.5"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
- params:
git:
branches:
- main
repoName: "aggregate-multi-dir"
scan:
projects:
- workingDirs:
- npm
- workingDirs:
- pip
pipRequirementsFile: requirements.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fix tests
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ref: refs/heads/main
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[core]
repositoryformatversion = 0
filemode = true
bare = false
logallrefupdates = true
ignorecase = true
precomposeunicode = true
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Unnamed repository; edit this file 'description' to name the repository.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# git ls-files --others --exclude-from=.git/info/exclude
# Lines that start with '#' are comments.
# For a project mostly in C, the following would be a good set of
# exclude patterns (uncomment them if you want to use them):
# *.[oa]
# *~
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
0000000000000000000000000000000000000000 db63220ddfc3c49fd55d611720d5423ac966a1d0 Omer Zidkoni <[email protected]> 1689852747 +0300 commit (initial): fix tests
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
0000000000000000000000000000000000000000 db63220ddfc3c49fd55d611720d5423ac966a1d0 Omer Zidkoni <[email protected]> 1689852747 +0300 commit (initial): fix tests
Binary file not shown.
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
xK��OR02g(H�(HM.��5ѳ�3�*��*r ��� �}
K
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x}�;1 D�9��}(�jkDI��`C~x������W��ӌm�����@A�صk��Y��
&w��L9��P�'��B�� yT2����ҌT�0}��9��{f�s���<5�碡�X�|A�z��ѳP��ݑC}P�F_��D@
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x���
!@;�sbt]G!�?��msƲp� ���������ZJ� w������ ��,�0ِ�>�ifb�V ������H�k�W]3����Z�b-'��?�{�հ��G�o�R�@��o��G7�
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
db63220ddfc3c49fd55d611720d5423ac966a1d0
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"name": "aggregate",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no tsest specified\" && exit 1"
},
"author": "",
"license": "ISC",
"dependencies": {
"uuid": "^9.0.0",
"minimatch":"3.0.2",
"mpath": "0.7.0"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pexpect==4.8.0
pyjwt==1.7.1
Original file line number Diff line number Diff line change
@@ -1 +1 @@
init commit
fix tests
Binary file modified commands/testdata/createfixpullrequests/aggregate/git/index
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0000000000000000000000000000000000000000 0b0bc897474ade0656ffb59bde78495cbb0215ed Omer Zidkoni <[email protected]> 1689690889 +0300 commit (initial): init commit
0000000000000000000000000000000000000000 48181e917e611cb6c95db9119329859b7f39cc2a Omer Zidkoni <[email protected]> 1689861913 +0300 commit (initial): fix tests
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0000000000000000000000000000000000000000 0b0bc897474ade0656ffb59bde78495cbb0215ed Omer Zidkoni <[email protected]> 1689690889 +0300 commit (initial): init commit
0000000000000000000000000000000000000000 48181e917e611cb6c95db9119329859b7f39cc2a Omer Zidkoni <[email protected]> 1689861913 +0300 commit (initial): fix tests
Loading

0 comments on commit ecc49ca

Please sign in to comment.