From d55670144384878b4c2998744faae37eafd98560 Mon Sep 17 00:00:00 2001 From: jabaran Date: Sun, 11 Aug 2024 18:56:19 -0400 Subject: [PATCH] Adding support for GH Advanced Security SARIF Upload Github annoucned changes to their SARIF [upload](https://github.blog/changelog/2024-05-06-code-scanning-will-stop-combining-runs-from-a-single-upload/) the impact is when a SARIF that is being uploaded, each run must have unique "category", as defined by GitHub [here](https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#runautomationdetails-object) GH's new requirement presents a new problem for when a file is removed from source because GH will not have an empty result for the previous file to close any previously opened items as would happen with `--all-projects` previously since GH would merge all results into a single result and closed items no longer present, but now that the closure is based on tool.driver.name + Category and we have unique files, this will be more problematic. "Open source's" solution is the most obvious, include the targetFile. Since Snyk-iac, is already populating the field with the static value of "snyk-iac". It seemed wise to combine riff off that and use "snyk-sca" for OpenSource. Then I separated those values with a pipe and included the `targetFile` This pattern was then applied to "snyk-iac" and added to "snyk-container". I wasn't able to find unit tests for SARIF output for "snyk-iac" or "snyk-container" or flags to test the output; however, given the simliarities in the implementation to snyk-sca I felt it was worth submitting. --- src/lib/formatters/iac-output/sarif.ts | 17 ++++++++++--- .../formatters/open-source-sarif-output.ts | 14 +++++++++++ src/lib/formatters/sarif-output.ts | 14 +++++++++++ .../open-source-sarif-output.spec.ts.snap | 3 +++ .../__snapshots__/sarif-output.spec.ts.snap | 9 +++++++ .../test/format-test-results.spec.ts | 25 +++++++++++++++++++ 6 files changed, 79 insertions(+), 3 deletions(-) diff --git a/src/lib/formatters/iac-output/sarif.ts b/src/lib/formatters/iac-output/sarif.ts index 67e18111c2..4a5fe318f7 100644 --- a/src/lib/formatters/iac-output/sarif.ts +++ b/src/lib/formatters/iac-output/sarif.ts @@ -77,9 +77,7 @@ export function createSarifOutputForIac( }, tool, - automationDetails: { - id: 'snyk-iac', - }, + automationDetails : getAutomationDetails(pathToFileURL(repoRoot).href), results: mapIacTestResponseToSarifResults(issues), }, ], @@ -153,6 +151,19 @@ function extractReportingDescriptor( return Object.values(tool); } +// Github anncouned changes to their SARIF upload -- https://github.blog/changelog/2024-05-06-code-scanning-will-stop-combining-runs-from-a-single-upload/ +// the impact is when a SARIF that is being uploaded, each run must have unique category, as defined by GitHub here, https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#runautomationdetails-object +// This presents a new problem of when a file is removed from source since GH will not have an empty result to close any previously opened items since GH open/closes +// based on the SARIF tool.driver.name + Category. Open source's solution is the most obvious, inlcude the targetFile. Snyk-iac, is using this field set to a static "snyk-iac". Combing what +// was being done there with the file name to generate the unique value. Using | as a separator to make it easier to parse out tool vs targetFile. +function getAutomationDetails(path: string) +{ + let automationId = !!process.env.SET_AUTOMATION_DETAILS_ID ? `snyk-iac|${path}/` : "snyk-iac" + return { + id : automationId, + }; +} + function mapIacTestResponseToSarifResults( issues: ResponseIssues, ): sarif.Result[] { diff --git a/src/lib/formatters/open-source-sarif-output.ts b/src/lib/formatters/open-source-sarif-output.ts index 0388674197..870043c2f8 100644 --- a/src/lib/formatters/open-source-sarif-output.ts +++ b/src/lib/formatters/open-source-sarif-output.ts @@ -27,6 +27,7 @@ export function createSarifOutputForOpenSource( 'https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json', version: '2.1.0', runs: testResults.map(replaceLockfileWithManifest).map((testResult) => ({ + automationDetails : getAutomationDetails(testResult), tool: { driver: { name: 'Snyk Open Source', @@ -44,6 +45,19 @@ export function createSarifOutputForOpenSource( }; } +// Github anncouned changes to their SARIF upload -- https://github.blog/changelog/2024-05-06-code-scanning-will-stop-combining-runs-from-a-single-upload/ +// the impact is when a SARIF that is being uploaded, each run must have unique category, as defined by GitHub here, https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#runautomationdetails-object +// This presents a new problem of when a file is removed from source since GH will not have an empty result to close any previously opened items since GH open/closes +// based on the SARIF tool.driver.name + Category. Open source's solution is the most obvious, inlcude the targetFile. Snyk-iac, is using this field set to a static "snyk-iac". Combing what +// was being done there with the file name to generate the unique value. Using | as a separator to make it easier to parse out tool vs targetFile. +function getAutomationDetails(testResult: TestResult) +{ + let automationId = !!process.env.SET_AUTOMATION_DETAILS_ID ? `snyk-sca|${testResult.displayTargetFile || testResult.targetFile }/` : "" + return { + id : automationId, + }; +} + function replaceLockfileWithManifest(testResult: TestResult): TestResult { let targetFile = testResult.displayTargetFile || ''; for (const [key, replacer] of Object.entries(LOCK_FILES_TO_MANIFEST_MAP)) { diff --git a/src/lib/formatters/sarif-output.ts b/src/lib/formatters/sarif-output.ts index 7af459f9c5..a7d0dacaa6 100644 --- a/src/lib/formatters/sarif-output.ts +++ b/src/lib/formatters/sarif-output.ts @@ -17,6 +17,7 @@ export function createSarifOutputForContainers( testResults.forEach((testResult) => { sarifRes.runs.push({ + automationDetails : getAutomationDetails(testResult), tool: getTool(testResult), results: getResults(testResult), }); @@ -94,3 +95,16 @@ export function getTool(testResult): sarif.Tool { .filter(Boolean); return tool; } + +// Github anncouned changes to their SARIF upload -- https://github.blog/changelog/2024-05-06-code-scanning-will-stop-combining-runs-from-a-single-upload/ +// the impact is when a SARIF that is being uploaded, each run must have unique category, as defined by GitHub here, https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#runautomationdetails-object +// This presents a new problem of when a file is removed from source since GH will not have an empty result to close any previously opened items since GH open/closes +// based on the SARIF tool.driver.name + Category. Open source's solution is the most obvious, inlcude the targetFile. Snyk-iac, is using this field set to a static "snyk-iac". Combing what +// was being done there with the file name to generate the unique value. Using | as a separator to make it easier to parse out tool vs targetFile. +function getAutomationDetails(testResult: TestResult) +{ + let automationId = !!process.env.SET_AUTOMATION_DETAILS_ID ? `snyk-container|${testResult.displayTargetFile || testResult.targetFile }/` : "" + return { + id : automationId, + }; +} \ No newline at end of file diff --git a/test/jest/unit/lib/formatters/__snapshots__/open-source-sarif-output.spec.ts.snap b/test/jest/unit/lib/formatters/__snapshots__/open-source-sarif-output.spec.ts.snap index d59c72574c..6dd6e7e7dc 100644 --- a/test/jest/unit/lib/formatters/__snapshots__/open-source-sarif-output.spec.ts.snap +++ b/test/jest/unit/lib/formatters/__snapshots__/open-source-sarif-output.spec.ts.snap @@ -5,6 +5,9 @@ exports[`createSarifOutputForOpenSource general 1`] = ` "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", "runs": [ { + "automationDetails": { + "id": "", + }, "results": [ { "fixes": [ diff --git a/test/jest/unit/lib/formatters/__snapshots__/sarif-output.spec.ts.snap b/test/jest/unit/lib/formatters/__snapshots__/sarif-output.spec.ts.snap index 4bfad15519..8d70b10a94 100644 --- a/test/jest/unit/lib/formatters/__snapshots__/sarif-output.spec.ts.snap +++ b/test/jest/unit/lib/formatters/__snapshots__/sarif-output.spec.ts.snap @@ -5,6 +5,9 @@ exports[`createSarifOutputForContainers general with critical severity issue wit "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", "runs": [ { + "automationDetails": { + "id": "", + }, "results": [ { "fixes": undefined, @@ -99,6 +102,9 @@ exports[`createSarifOutputForContainers general with critical severity issue wit "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", "runs": [ { + "automationDetails": { + "id": "", + }, "results": [ { "fixes": undefined, @@ -192,6 +198,9 @@ exports[`createSarifOutputForContainers general with high severity issue 1`] = ` "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", "runs": [ { + "automationDetails": { + "id": "", + }, "results": [ { "fixes": undefined, diff --git a/test/jest/unit/lib/formatters/test/format-test-results.spec.ts b/test/jest/unit/lib/formatters/test/format-test-results.spec.ts index 5db19495ba..cd0a3c68f6 100644 --- a/test/jest/unit/lib/formatters/test/format-test-results.spec.ts +++ b/test/jest/unit/lib/formatters/test/format-test-results.spec.ts @@ -1,10 +1,12 @@ import { Options } from '../../../../../../src/lib/types'; import * as fs from 'fs'; import { extractDataToSendFromResults } from '../../../../../../src/lib/formatters/test/format-test-results'; +//import exp from 'constants'; describe('extractDataToSendFromResults', () => { afterEach(() => { jest.restoreAllMocks(); + process.env.SET_AUTOMATION_DETAILS_ID = ""; }); describe('open source results', () => { @@ -116,6 +118,29 @@ describe('extractDataToSendFromResults', () => { expect(res.stringifiedData).not.toBe(''); expect(res.stringifiedJsonData).toBe(''); expect(res.stringifiedSarifData).not.toBe(''); + var sarif = JSON.parse(res.stringifiedSarifData); + expect(sarif.runs[0].automationDetails.id).toBe(''); + }); + + it('should create SARIF JSON and only SARIF JSON if `--sarif` is set in the options and SET_AUTOMATION_DETAILS_ID is set', () => { + const options = { + sarif: true, + } as Options; + process.env.SET_AUTOMATION_DETAILS_ID = "true"; + + const jsonStringifySpy = jest.spyOn(JSON, 'stringify'); + const res = extractDataToSendFromResults( + resultsFixture, + mappedResultsFixture, + options, + ); + + expect(jsonStringifySpy).toHaveBeenCalledTimes(1); + expect(res.stringifiedData).not.toBe(''); + expect(res.stringifiedJsonData).toBe(''); + expect(res.stringifiedSarifData).not.toBe(''); + var sarif = JSON.parse(res.stringifiedSarifData); + expect(sarif.runs[0].automationDetails.id).not.toBe(''); }); it('should create SARIF JSON and only SARIF JSON if `--sarif-file-output` is set in the options', () => {