Add SARIF processing and basic alert rendering #1171
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| import * as sarif from 'sarif'; | ||
|
charisk marked this conversation as resolved.
|
||
|
|
||
| import { AnalysisAlert, ResultSeverity } from './shared/analysis-result'; | ||
|
|
||
| const defaultSeverity = 'Warning'; | ||
|
|
||
| export function extractAnalysisAlerts( | ||
| sarifLog: sarif.Log | ||
| ): { | ||
| alerts: AnalysisAlert[], | ||
| errors: string[] | ||
| } { | ||
| if (!sarifLog) { | ||
| return { alerts: [], errors: ['No SARIF log was found'] }; | ||
| } | ||
|
|
||
| if (!sarifLog.runs) { | ||
| return { alerts: [], errors: ['No runs found in the SARIF file'] }; | ||
| } | ||
|
|
||
| const errors: string[] = []; | ||
| const alerts: AnalysisAlert[] = []; | ||
|
|
||
| for (const run of sarifLog.runs) { | ||
| if (!run.results) { | ||
| errors.push('No results found in the SARIF run'); | ||
| continue; | ||
| } | ||
|
|
||
| for (const result of run.results) { | ||
| const message = result.message?.text; | ||
| if (!message) { | ||
| errors.push('No message found in the SARIF result'); | ||
| continue; | ||
| } | ||
|
|
||
| const severity = tryGetSeverity(run, result) || defaultSeverity; | ||
|
|
||
| if (!result.locations) { | ||
| errors.push('No locations found in the SARIF result'); | ||
| continue; | ||
| } | ||
|
|
||
| for (const location of result.locations) { | ||
| const contextRegion = location.physicalLocation?.contextRegion; | ||
| if (!contextRegion) { | ||
| errors.push('No context region found in the SARIF result location'); | ||
| continue; | ||
| } | ||
| if (contextRegion.startLine === undefined) { | ||
| errors.push('No start line set for a result context region'); | ||
| continue; | ||
| } | ||
| if (contextRegion.endLine === undefined) { | ||
| errors.push('No end line set for a result context region'); | ||
| continue; | ||
| } | ||
| if (!contextRegion.snippet?.text) { | ||
| errors.push('No text set for a result context region'); | ||
| continue; | ||
| } | ||
|
|
||
| const region = location.physicalLocation?.region; | ||
| if (!region) { | ||
| errors.push('No region found in the SARIF result location'); | ||
| continue; | ||
| } | ||
| if (region.startLine === undefined) { | ||
| errors.push('No start line set for a result region'); | ||
| continue; | ||
| } | ||
| if (region.startColumn === undefined) { | ||
| errors.push('No start column set for a result region'); | ||
| continue; | ||
| } | ||
| if (region.endColumn === undefined) { | ||
| errors.push('No end column set for a result region'); | ||
| continue; | ||
| } | ||
|
|
||
| const filePath = location.physicalLocation?.artifactLocation?.uri; | ||
| if (!filePath) { | ||
| errors.push('No file path found in the SARIF result location'); | ||
| continue; | ||
| } | ||
|
|
||
| const analysisAlert = { | ||
| message, | ||
| filePath, | ||
| severity, | ||
| codeSnippet: { | ||
| startLine: contextRegion.startLine, | ||
| endLine: contextRegion.endLine, | ||
| text: contextRegion.snippet.text | ||
| }, | ||
| highlightedRegion: { | ||
| startLine: region.startLine, | ||
| startColumn: region.startColumn, | ||
| endLine: region.endLine, | ||
| endColumn: region.endColumn | ||
| } | ||
| }; | ||
|
|
||
| const validationErrors = getAlertValidationErrors(analysisAlert); | ||
| if (validationErrors.length > 0) { | ||
| errors.push(...validationErrors); | ||
| continue; | ||
| } | ||
|
|
||
| alerts.push(analysisAlert); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return { alerts, errors }; | ||
| } | ||
|
|
||
| export function tryGetSeverity( | ||
| sarifRun: sarif.Run, | ||
| result: sarif.Result | ||
| ): ResultSeverity | undefined { | ||
| if (!sarifRun || !result) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const rule = tryGetRule(sarifRun, result); | ||
| if (!rule) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const severity = rule.properties?.['problem.severity']; | ||
| if (!severity) { | ||
| return undefined; | ||
| } | ||
|
|
||
| switch (severity.toLowerCase()) { | ||
| case 'recommendation': | ||
| return 'Recommendation'; | ||
| case 'warning': | ||
| return 'Warning'; | ||
| case 'error': | ||
| return 'Error'; | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| export function tryGetRule( | ||
| sarifRun: sarif.Run, | ||
| result: sarif.Result | ||
| ): sarif.ReportingDescriptor | undefined { | ||
| if (!sarifRun || !result) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const resultRule = result.rule; | ||
| if (!resultRule) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // The rule can found in two places: | ||
| // - Either in the run's tool driver tool component | ||
| // - Or in the run's tool extensions tool component | ||
|
|
||
| const ruleId = resultRule.id; | ||
| if (ruleId) { | ||
| const rule = sarifRun.tool.driver.rules?.find(r => r.id === ruleId); | ||
| if (rule) { | ||
| return rule; | ||
| } | ||
| } | ||
|
|
||
| const ruleIndex = resultRule.index; | ||
| if (ruleIndex != undefined) { | ||
| const toolComponentIndex = result.rule?.toolComponent?.index; | ||
| const toolExtensions = sarifRun.tool.extensions; | ||
| if (toolComponentIndex !== undefined && toolExtensions !== undefined) { | ||
| const toolComponent = toolExtensions[toolComponentIndex]; | ||
| if (toolComponent?.rules !== undefined) { | ||
| return toolComponent.rules[ruleIndex]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Couldn't find the rule. | ||
| return undefined; | ||
| } | ||
|
|
||
| function getAlertValidationErrors(alert: AnalysisAlert): string[] { | ||
| const errors = []; | ||
|
|
||
| if (alert.codeSnippet.startLine > alert.codeSnippet.endLine) { | ||
| errors.push('The code snippet start line is greater than the end line'); | ||
| } | ||
|
|
||
| const highlightedRegion = alert.highlightedRegion; | ||
| if (highlightedRegion.endLine === highlightedRegion.startLine && | ||
| highlightedRegion.endColumn < highlightedRegion.startColumn) { | ||
| errors.push('The highlighted region end column is greater than the start column'); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. There are other kinds of errors to handle, too. Eg- negative values, or values larger than the code itself. I don't think you need to be adding error messages for all of these. You just need to make sure that the UI can do something reasonable when these errors occur. It looks like here you are adding error messages, and still adding the region to the list of alerts. I think that's fine as long as there's somewhere else that can clean up invalid regions, or the web view is robust enough to do something reasonable for them. Does the web view already handle this?
Contributor
Author
There was a problem hiding this comment. TBH I wanted to focus on happy path first so that we can start seeing the view populated. I just added the errors because the compiler was shouting at me 😂 the web view doesn't handle errors gracefully atm, but remember that all this is behind a "feature flag". WRT validation, here are some thoughts I've had:
Contributor
There was a problem hiding this comment. We don't really own the whole toolchain since we are running queries created by others and it is the query writers who inject source locations. Most of the time (but not always), the source locations come from codeql core libraries and most of the time (but not always) these libraries produce valid source locations. I bring this up because we've hit errors before for local queries. Bad source locations usually means a bug in the query or one of the query libraries. However, the user running the query may not have any control over that, so we should be handling this gracefully. Also, ajv won't be much help here since invalid source locations are not checkable in the json schema for sarif. The way we've handled this for local queries is that we do some reasonable things to normalize source locations (eg- negative numbers are changed to 0, values after the end of the line are changed to the end of the line, and if an end location comes before the start location we swap them). I don't think we are even logging these invalid source locations (but maybe we should). I agree that we don't need to handle it now, but it is something we should put in for a general release. So, let's get this in and handle invalid cases later. I just wanted to make sure you are aware of the difficulties so we don't repeat the same problems we had with local queries. :) |
||
|
|
||
| return errors; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.