-
Notifications
You must be signed in to change notification settings - Fork 361
Perform consistent diff-informed alert filtering in the action #2765
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import * as fs from "fs"; | ||
import * as path from "path"; | ||
|
||
import * as actionsUtil from "./actions-util"; | ||
import { Logger } from "./logging"; | ||
|
||
export interface DiffThunkRange { | ||
path: string; | ||
startLine: number; | ||
endLine: number; | ||
} | ||
|
||
function getDiffRangesJsonFilePath(): string { | ||
return path.join(actionsUtil.getTemporaryDirectory(), "pr-diff-range.json"); | ||
} | ||
|
||
export function writeDiffRangesJsonFile( | ||
logger: Logger, | ||
ranges: DiffThunkRange[], | ||
): void { | ||
const jsonContents = JSON.stringify(ranges, null, 2); | ||
const jsonFilePath = getDiffRangesJsonFilePath(); | ||
fs.writeFileSync(jsonFilePath, jsonContents); | ||
logger.debug( | ||
`Wrote pr-diff-range JSON file to ${jsonFilePath}:\n${jsonContents}`, | ||
); | ||
} | ||
|
||
export function readDiffRangesJsonFile( | ||
logger: Logger, | ||
): DiffThunkRange[] | undefined { | ||
const jsonFilePath = getDiffRangesJsonFilePath(); | ||
if (!fs.existsSync(jsonFilePath)) { | ||
logger.debug(`Diff ranges JSON file does not exist at ${jsonFilePath}`); | ||
return undefined; | ||
} | ||
const jsonContents = fs.readFileSync(jsonFilePath, "utf8"); | ||
logger.debug( | ||
`Read pr-diff-range JSON file from ${jsonFilePath}:\n${jsonContents}`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be more robust to log only the first n entries, in case we have a very large PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will keep the logging as-is for now. Since the JSON file contains only the line ranges, hopefully it will still be manageable even for a very large PR. |
||
); | ||
return JSON.parse(jsonContents) as DiffThunkRange[]; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ import * as api from "./api-client"; | |
import { getGitHubVersion, wrapApiConfigurationError } from "./api-client"; | ||
import { CodeQL, getCodeQL } from "./codeql"; | ||
import { getConfig } from "./config-utils"; | ||
import { readDiffRangesJsonFile } from "./diff-filtering-utils"; | ||
import { EnvVar } from "./environment"; | ||
import { FeatureEnablement } from "./feature-flags"; | ||
import * as fingerprints from "./fingerprints"; | ||
|
@@ -578,6 +579,7 @@ export async function uploadFiles( | |
features, | ||
logger, | ||
); | ||
sarif = filterAlertsByDiffRange(logger, sarif); | ||
sarif = await fingerprints.addFingerprints(sarif, checkoutPath, logger); | ||
|
||
const analysisKey = await api.getAnalysisKey(); | ||
|
@@ -848,3 +850,50 @@ export class InvalidSarifUploadError extends Error { | |
super(message); | ||
} | ||
} | ||
|
||
function filterAlertsByDiffRange(logger: Logger, sarif: SarifFile): SarifFile { | ||
const diffRanges = readDiffRangesJsonFile(logger); | ||
if (!diffRanges?.length) { | ||
return sarif; | ||
} | ||
|
||
const checkoutPath = actionsUtil.getRequiredInput("checkout_path"); | ||
|
||
for (const run of sarif.runs) { | ||
if (run.results) { | ||
run.results = run.results.filter((result) => { | ||
const locations = [ | ||
...(result.locations || []).map((loc) => loc.physicalLocation), | ||
...(result.relatedLocations || []).map((loc) => loc.physicalLocation), | ||
]; | ||
|
||
return locations.some((physicalLocation) => { | ||
const locationUri = physicalLocation?.artifactLocation?.uri; | ||
const locationStartLine = physicalLocation?.region?.startLine; | ||
if (!locationUri || locationStartLine === undefined) { | ||
return false; | ||
} | ||
// CodeQL always uses forward slashes as the path separator, so on Windows we | ||
// need to replace any backslashes with forward slashes. | ||
const locationPath = path | ||
.join(checkoutPath, locationUri) | ||
.replaceAll(path.sep, "/"); | ||
// Alert filtering here replicates the same behavior as the restrictAlertsTo | ||
// extensible predicate in CodeQL. See the restrictAlertsTo documentation | ||
// https://codeql.github.com/codeql-standard-libraries/csharp/codeql/util/AlertFiltering.qll/predicate.AlertFiltering$restrictAlertsTo.3.html | ||
// for more details, such as why the filtering applies only to the first line | ||
// of an alert location. | ||
return diffRanges.some( | ||
(range) => | ||
range.path === locationPath && | ||
((range.startLine <= locationStartLine && | ||
range.endLine >= locationStartLine) || | ||
Comment on lines
+889
to
+890
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about if the location's start line isn't within the range but its end line is? If this is intentional, it might be worth adding a comment to make explicit that this case is filtered. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion! I added comments to document the intended behavior of the filtering, by way of referencing the |
||
(range.startLine === 0 && range.endLine === 0)), | ||
); | ||
}); | ||
}); | ||
} | ||
} | ||
|
||
return sarif; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here