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

Feat: Adding support for GH Advanced Security SARIF Upload #5408

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/lib/formatters/iac-output/sarif.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,7 @@ export function createSarifOutputForIac(
},

tool,
automationDetails: {
id: 'snyk-iac',
},
automationDetails : getAutomationDetails(pathToFileURL(repoRoot).href),
results: mapIacTestResponseToSarifResults(issues),
},
],
Expand Down Expand Up @@ -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[] {
Expand Down
14 changes: 14 additions & 0 deletions src/lib/formatters/open-source-sarif-output.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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)) {
Expand Down
14 changes: 14 additions & 0 deletions src/lib/formatters/sarif-output.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export function createSarifOutputForContainers(

testResults.forEach((testResult) => {
sarifRes.runs.push({
automationDetails : getAutomationDetails(testResult),
tool: getTool(testResult),
results: getResults(testResult),
});
Expand Down Expand Up @@ -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,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
25 changes: 25 additions & 0 deletions test/jest/unit/lib/formatters/test/format-test-results.spec.ts
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down