script to run semgrep tests against adapter PRs #2907

onkarvhanumante · 2023-07-06T11:42:48Z

Description

PR Semgrep rules for adapters #2833 introduced two Semgrep rules for adapters i.e builder-struct-name.yml and type-bid-assignment.yml
This PR adds a GitHub action script and some helper to run above Semgrep rules as PR checks. These checks are conditioned to run only if new PR makes some adapter related changes
Findings from above semgrep rules will be reported as PR code review comment. To avoid duplication, previous comments will not be added again
Script only considers findings that are relevant to the changes made in the PR. For example, if the PR modifies line 10 in the existing file "foo.go" and Semgrep detects issues on lines 5 and 10, only the issue on line 10 will be taken into account. This approach limits the scope of the Semgrep checks to the specific changes made in the PR
Script will continue to fail until all high severity errors reported by Semgrep are fixed

Testings

Tested script on fork repo - https://github.com/onkarvhanumante/prebid-server/pulls

Scenario 1 Adapter PR created
PR: https://github.com/onkarvhanumante/prebid-server/pull/44
Expected: Semgrep result get uploaded as comment. Semgrep PR checks will fail
Scenario 2 New commits pushed to fix some errors previously reported by PR checks
PR: https://github.com/onkarvhanumante/prebid-server/pull/45
Expected: No duplicate comments added for unaddressed errors. Since all the errors are not fixed therefore Semgrep PR checks will still continue to fail
Scenario 3 Commits pushed to fix all errors previously reported by PR checks
PR: https://github.com/onkarvhanumante/prebid-server/pull/46
Expected: All errors are addressed. So Semgrep PR checks will get passed
Scenario 4 PR has no adapter related change
PR: https://github.com/onkarvhanumante/prebid-server/pull/47
Expected: Semgrep checks won't be executed

Review notes

PR introduces 2 files - semgrep.yml and pull-request-utils.js. Action script semgrep.yml makes use of helpers from pull-request-utils.js.

For example, semgrep.yml has above code block.

prebid-server/.github/workflows/semgrep.yml

Lines 21 to 27 in d78c3d2

    
                       const utils = require('./.github/workflows/helpers/pull-request-utils.js') 
        
                       // consider only non-test Go files that are part of the adapter code 
        
                       function fileNameFilter(filename) { 
        
                         return filename.startsWith("adapters/") && filename.split("/").length > 2 && filename.endsWith(".go") && !filename.endsWith("_test.go") 
        
                       } 
        
                       const helper = utils.diffHelper({github, context, fileNameFilter, event: "${{github.event.action}}", testName: "${{github.job}}"}) 
        
                       return await helper.buildDiff()

Here pull-request-utils.js is imported on line 21. Helper class diffHelper is initialised on line 26 and helper method buildDiff() is called on line 27. Therefore start reviewing from semgrep.yml file

Commit introduces script and helpers to run semgrep test against PRs and upload semgrep results as PR comment

gargcreation1992 · 2023-07-10T05:50:25Z

.github/workflows/helpers/pull-request-utils.js

+      const nonScannedCommits = await this.#getNonScannedCommits()
+      for (const commit of nonScannedCommits) {
+        const { data } = await this.github.rest.repos.getCommit({
+          owner: this.owner,
+          repo: this.repo,
+          ref: commit,
+        })
+
+        const commitDiff = await this.#getDiffForFiles(data.files)
+        const files = Object.keys(commitDiff)
+        for (const file of files) {
+          // Consider scenario where the changes made to a file in the initial commit are completely undone by subsequent commits
+          // In such cases, the modifications from the initial commit should not be taken into account
+          // If the changes were entirely removed, there should be no entry for the file in the pullRequestStats
+          const filePRDiff = pullRequestDiff[file]
+          if (!filePRDiff) {
+            continue
+          }
+
+          // Consider scenario where changes made in the commit were partially removed or modified by subsequent commits
+          // In such cases, include only those commit changes that are part of the pullRequestStats object
+          // This ensures that only the changes that are reflected in the pull request are considered
+          const changes = await this.#filterCommitDiff(commitDiff[file], filePRDiff)
+
+          if (changes.length !== 0) {
+            // Check if nonScannedCommitsDiff[file] exists, if not assign an empty array to it
+            nonScannedCommitsDiff[file] = nonScannedCommitsDiff[file] || []
+            // Combine the existing nonScannedCommitsDiff[file] array with the commit changes
+            // Remove any duplicate elements using the Set data structure
+            nonScannedCommitsDiff[file] = [
+              ...new Set([...nonScannedCommitsDiff[file], ...changes]),
+            ]
+          }
+        }
+      }
+    }


Do you think moving this code to separate function will help in code readibility? function like RetrieveNonScannedCommit?

9d06e6b

makes above sense

gargcreation1992

lgtm!
Let me know if you would like to move that part of code to a separate function.

Builder struct name should be adapter. So changing severity to error

@onkarvhanumante

authored by @onkarvhanumante

@akkapur

* Fix: deal tiers no longer ignored due to presence of tid (prebid#2829) * CAPT-787: GPP support for imds bidder. (prebid#2867) Co-authored-by: Timothy M. Ace <[email protected]> * Adsinteractive: change usersync endpoint to https (prebid#2861) Co-authored-by: Balint Vargha <[email protected]> * consumable adapter: add gpp support (prebid#2883) * feat: IX Bid Adapter - gpp support for user sync urls (prebid#2873) Co-authored-by: Chris Corbo <[email protected]> * fix: update links in readme (prebid#2888) authored by @akkapur * New Adapter: AIDEM (prebid#2824) Co-authored-by: AndreaC <[email protected]> Co-authored-by: Andrea Tumbarello <[email protected]> Co-authored-by: darkstar <[email protected]> * Improve Digital adapter: Set currency in bid response (prebid#2886) * Sharethrough: Support multiformat bid request impression (prebid#2866) * Triplelift Bid Adapter: Adding GPP Support (prebid#2887) * YahooAdvertising rebranding to Yahoo Ads. (prebid#2872) Co-authored-by: oath-jac <[email protected]> * IX: MultiImp Implementation (prebid#2779) Co-authored-by: Chris Corbo <[email protected]> Co-authored-by: Oronno Mamun <[email protected]> * Exchange unit test fix (prebid#2868) * Semgrep rules for adapters (prebid#2833) * IX: Remove glog statement (prebid#2909) * Activities framework (prebid#2844) * PWBID: Update Default Endpoint (prebid#2903) * script to run semgrep tests against adapter PRs (prebid#2907) authored by @onkarvhanumante * semgrep rule to detect undesirable package imports in adapter code (prebid#2911) * update package-import message (prebid#2913) authored by @onkarvhanumante * Bump google.golang.org/grpc from 1.46.2 to 1.53.0 (prebid#2905) --------- Co-authored-by: Brian Sardo <[email protected]> Co-authored-by: Timothy Ace <[email protected]> Co-authored-by: Timothy M. Ace <[email protected]> Co-authored-by: balintvargha <[email protected]> Co-authored-by: Balint Vargha <[email protected]> Co-authored-by: Jason Piros <[email protected]> Co-authored-by: ccorbo <[email protected]> Co-authored-by: Chris Corbo <[email protected]> Co-authored-by: Ankush <[email protected]> Co-authored-by: Giovanni Sollazzo <[email protected]> Co-authored-by: AndreaC <[email protected]> Co-authored-by: Andrea Tumbarello <[email protected]> Co-authored-by: darkstar <[email protected]> Co-authored-by: Jozef Bartek <[email protected]> Co-authored-by: Max Dupuis <[email protected]> Co-authored-by: Patrick Loughrey <[email protected]> Co-authored-by: radubarbos <[email protected]> Co-authored-by: oath-jac <[email protected]> Co-authored-by: Oronno Mamun <[email protected]> Co-authored-by: Veronika Solovei <[email protected]> Co-authored-by: Onkar Hanumante <[email protected]> Co-authored-by: Stephen Johnston <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Add github action script to run semgrep tests against adapter PRs

d78c3d2

Commit introduces script and helpers to run semgrep test against PRs and upload semgrep results as PR comment

onkarvhanumante added the do not port label Jul 6, 2023

onkarvhanumante changed the title ~~Add github action script to run semgrep tests against adapter PRs~~ script to run semgrep tests against adapter PRs Jul 6, 2023

onkarvhanumante assigned Sonali-More-Xandr and gargcreation1992 Jul 7, 2023

onkarvhanumante mentioned this pull request Jul 7, 2023

semgrep rule to flag undesirable package imports in adapter code #2911

Merged

gargcreation1992 reviewed Jul 10, 2023

View reviewed changes

gargcreation1992 previously approved these changes Jul 10, 2023

View reviewed changes

Sonali-More-Xandr previously approved these changes Jul 10, 2023

View reviewed changes

add getNonScannedCommitDiff() function

9d06e6b

onkarvhanumante dismissed stale reviews from Sonali-More-Xandr and gargcreation1992 via 9d06e6b July 10, 2023 08:59

Change severity to error

32d76c7

Builder struct name should be adapter. So changing severity to error

gargcreation1992 approved these changes Jul 10, 2023

View reviewed changes

onkarvhanumante merged commit a39f5ca into master Jul 10, 2023

onkarvhanumante deleted the semgrepPRs/run-adapter-semgrep-tests branch July 10, 2023 09:56

Peiling-Ding pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jul 14, 2023

script to run semgrep tests against adapter PRs (prebid#2907)

9df856d

authored by @onkarvhanumante

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

script to run semgrep tests against adapter PRs #2907

script to run semgrep tests against adapter PRs #2907

onkarvhanumante commented Jul 6, 2023 •

edited

Loading

gargcreation1992 Jul 10, 2023

onkarvhanumante Jul 10, 2023

gargcreation1992 left a comment

	const utils = require('./.github/workflows/helpers/pull-request-utils.js')
	// consider only non-test Go files that are part of the adapter code
	function fileNameFilter(filename) {
	return filename.startsWith("adapters/") && filename.split("/").length > 2 && filename.endsWith(".go") && !filename.endsWith("_test.go")
	}
	const helper = utils.diffHelper({github, context, fileNameFilter, event: "${{github.event.action}}", testName: "${{github.job}}"})
	return await helper.buildDiff()

script to run semgrep tests against adapter PRs #2907

script to run semgrep tests against adapter PRs #2907

Conversation

onkarvhanumante commented Jul 6, 2023 • edited Loading

Description

Testings

Review notes

gargcreation1992 Jul 10, 2023

Choose a reason for hiding this comment

onkarvhanumante Jul 10, 2023

Choose a reason for hiding this comment

gargcreation1992 left a comment

Choose a reason for hiding this comment

onkarvhanumante commented Jul 6, 2023 •

edited

Loading