Add SARIF processing and basic alert rendering#1171
Conversation
adityasharad
left a comment
adityasharad
left a comment
There was a problem hiding this comment.
Nice. Chiming in with a few suggestions on SARIF processing.
|
@aeisenberg @shati-patel I've just pushed some code changes to make the code snippets a bit less ugly. The original plan was to use monaco editor but I'm having too much trouble with it so for now I'll just try and make it look decent with plain React/CSS. Apologies for the churn! |
| if (highlightedRegion.endLine === highlightedRegion.startLine && | ||
| highlightedRegion.endColumn < highlightedRegion.startColumn) { | ||
| errors.push('The highlighted region end column is greater than the start column'); | ||
| } |
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?
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:
- How far do we want to go, considering we own the whole toolchain?
- Adding custom validation code like I've started doing here seems a pain. I'm wondering if we can replace that with something automated e.g. using a JSON schema validator like ajv.
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. :)
|
@aeisenberg are you happy enough with this change to be merged? |
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
As mentioned, let's get this in and we can iterate on making this more robust later.
4 participants
Updated logic that reads the SARIF file in order to extract information about analysis results and then used that data to start rendering a list of results.
This is quite basic at the minute and will be fleshed out in future PRs. More information in the internal issue.
Please let me know if I'm using incorrect terminology.
To view the changes, enable rending or analysis results in RemoteQueries.tsx L333
Checklist
N/A:
ready-for-doc-reviewlabel there.