BREAKING: Removed SARIF 1.0 support from BinSkim. Now option -v | --sarif-output-version does not accept value OneZeroZero.
#719
Conversation
…sarif-output-version` does not accept value `OneZeroZero`.
| if (!Environment.GetCommandLineArgs().Any(arg => arg.Equals("--sarif-output-version"))) | ||
| if (!Environment.GetCommandLineArgs(). | ||
| Any(arg => arg.Equals("--sarif-output-version", StringComparison.OrdinalIgnoreCase) || | ||
| arg.Equals("-v", StringComparison.OrdinalIgnoreCase))) |
There was a problem hiding this comment.
this is not related to the change,
the code clearly missed the -v variant so I added it btw. #Closed
src/BinSkim.Driver/AnalyzeOptions.cs
Outdated
| HelpText = | ||
| "The SARIF version of the output log file. The only valid value is Current.", | ||
| Default = BinSkimSarifVersion.Current)] | ||
| public new BinSkimSarifVersion SarifOutputVersion { get; set; } = BinSkimSarifVersion.Current; |
src/BinSkim.Driver/AnalyzeOptions.cs
Outdated
| @@ -48,5 +48,13 @@ public class AnalyzeOptions : AnalyzeOptionsBase | |||
| "ignorePdbLoadError", | |||
| HelpText = "If enabled, BinSkim won't break if we have a 'PdbLoadingException'.")] | |||
| public bool IgnorePdbLoadError { get; set; } | |||
|
|
|||
| [Option( | |||
There was a problem hiding this comment.
This is the main change of the PR.
The support of the v1 in Binskim, actually comes from SARIF SDK. Without remove the support in SDK we can not really remove it.
The change is to disable it in BinSkim. Please see more details in PR desc.
Now option -v | --sarif-output-version does not accept value OneZeroZero in BinSkim. #Resolved
There was a problem hiding this comment.
I don't think you need this. We should delete it.
Instead, just find the right place in our code and throw an InvalidOperationException in cases where someone passes SarifOneZeroZero on the command-line.
There was a problem hiding this comment.
changed accordingly.
|
|
||
| namespace Microsoft.CodeAnalysis.IL | ||
| { | ||
| public enum BinSkimSarifVersion |
| { | ||
| analyzeOptions.SarifOutputVersion = Sarif.SarifVersion.Current; |
There was a problem hiding this comment.
put a check here for use of Sarif.SarifVersion.OneZeroZero and just throw an ArgumentException or InvalidOperationException in that case.
'BinSkim no longer supports emitting SARIF 1.0 (an obsolete format). Pass 'Current' on the command-line or omit this argument entirely.' #Resolved
There was a problem hiding this comment.
I thought about this simple throw as well, I think my way is better because simple throw will still have the wrong info in the tool help.
Let me change to simple throw.
There was a problem hiding this comment.
I see your point! We should be thinking about where this area is going. I think the right thing to do moving forward is to actually eliminate this argument entirely, i.e., we don't want the driver SDK itself to provide for SARIF v1 support by default. And so that argues for taking this approach: the help will be broken, but a future update of the driver SDK could drop this argument entirely.
| { | ||
| analyzeOptions.SarifOutputVersion = Sarif.SarifVersion.Current; | ||
| } | ||
|
|
||
| if (s_UnitTestOutputVersion != Sarif.SarifVersion.Unknown) |
BREAKING: Removed SARIF 1.0 support from BinSkim. Now option
-v | --sarif-output-versiondoes not accept valueOneZeroZero.I found most of the code is from SARIF SDK and unless we also zap the support in SDK we can not change those codes.
for example, the option itself is from SDK, and the help text will say "Valid values are OneZeroZero and Current".
We do not want that false information.
So a simple way of fix I tried working is to new a same name parameter to hide the underlying one, this way we can show something like "The only valid value is Current." and also only support Current instead of supporting both.
The error message user get this way is unified with other parameter not supported and will print the help page with the info on what is supported.
Added tests show the actual value will be
Currentas we want for various input.