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

Fix broken sarif export #80

Merged
merged 4 commits into from
Feb 10, 2021
Merged

Fix broken sarif export #80

merged 4 commits into from
Feb 10, 2021

Conversation

JarLob
Copy link
Contributor

@JarLob JarLob commented Feb 7, 2021

Plus upgrade sarif schema to 2.1.0

@JarLob JarLob requested a review from phosphore February 7, 2021 23:07
@phosphore
Copy link
Contributor

About ba73cfd, this is needed for some checks where no location is present afaik. Or is it a Sarif spec requirement to have at least 0:0 for the lin:col? issue.location could be undefined in some checks (e.g. CSP?)

@JarLob
Copy link
Contributor Author

JarLob commented Feb 9, 2021

Yes, I was testing with different Sarif viewers like VS Code and VS. Implementations differ. First I thought to skip region at all, but this combination is the best common denominator. Sarif also doesn't like 0:0, so having 1:1 when no file is available doesn't change anything. The change affects only the exported sarif json file.

@phosphore
Copy link
Contributor

Fair enough. Is issue.location always present tho?
ba73cfd#diff-e97b0ce1d9051faf00c21306911cf6bcfd8a245e5783ec1a9b448dc6c8dd8966R116-R117

@JarLob
Copy link
Contributor Author

JarLob commented Feb 9, 2021

Yes, it is 0:0 if no file. Can check location for undefined if you think it is better.
The final version btw is:

startLine: issue.location.line === 0 ? 1 : issue.location.line, // This is odd, VS and VS Code highlight the line correctly, but min value is 1
startColumn: issue.location.column + 1, // sarif columns start from 1
charLength: issue.sample ? issue.sample.length : 0

I dropped the check for N/A. Just adjust indices and set charLength to zero if sample is not available.

@phosphore
Copy link
Contributor

Thank you very much @JarLob <3
I'll merge this and create a new v1.8.1 tag here and on NPM.

@phosphore phosphore merged commit 719799f into doyensec:master Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants