diff --git a/src/ReleaseHistory.md b/src/ReleaseHistory.md index 5290e2aee..d9a1842e4 100644 --- a/src/ReleaseHistory.md +++ b/src/ReleaseHistory.md @@ -1,10 +1,10 @@ # SARIF Package Release History (SDK, Driver, Converters, and Multitool) -## **v2.3.7** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.3.7) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.3.7) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.3.7) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.3.7) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.3.7) * DEPENDENCY BREAKING: SARIF now requires Newtonsoft.JSON 11.0.2 (rather than 10.0.3) * DEPENDENCY: SARIF TypeScript package now requires minimist 1.2.3 or later (rather than >=1.2.0) * FEATURE: Add a converter for FlawFinder's CSV output format. [#2092](https://github.com/microsoft/sarif-sdk/issues/2092) * FEATURE: Multitool SARIF output is now pretty-printed by default. To remove white space, specify `--minify`. [#2098](https://github.com/microsoft/sarif-sdk/issues/2098) +* FEATURE: The Multitool `query` command can now evaluate properties in the result and rule property bags, for example `sarif query "properties.confidence:f > 0.95 AND rule.properties.category == 'security'"` * FEATURE: The validation rule `SARIF1004.ExpressUriBaseIdsCorrectly` now verifies that if an `artifactLocation.uri` is a relative reference, it does not begin with a slash. [#2090](https://github.com/microsoft/sarif-sdk/issues/2090) * FEATURE: Add a setter to `GitHelper.GitExePath`. [#2105](https://github.com/microsoft/sarif-sdk/issues/2105) * FEATURE: `GitHelper` will search in %PATH% variable if `git.exe` isn't located in ProgramFiles folder [#2106](https://github.com/microsoft/sarif-sdk/issues/2106) diff --git a/src/Sarif/Query/Evaluators/PropertyBagPropertyEvaluator.cs b/src/Sarif/Query/Evaluators/PropertyBagPropertyEvaluator.cs new file mode 100644 index 000000000..5498f9bc3 --- /dev/null +++ b/src/Sarif/Query/Evaluators/PropertyBagPropertyEvaluator.cs @@ -0,0 +1,108 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Globalization; +using System.Linq; +using System.Text.RegularExpressions; +using Newtonsoft.Json; + +namespace Microsoft.CodeAnalysis.Sarif.Query.Evaluators +{ + public class PropertyBagPropertyEvaluator : IExpressionEvaluator + { + internal const string ResultPropertyPrefix = "properties."; + internal const string RulePropertyPrefix = "rule.properties."; + + private readonly string _propertyName; + private readonly bool _propertyBelongsToRule; + private readonly IExpressionEvaluator _evaluator; + + private static readonly Regex s_propertyNameRegex = new Regex( + @$"^ + (?properties\.|rule\.properties\.) + (?.+?) + $", + RegexOptions.CultureInvariant | RegexOptions.Compiled | RegexOptions.ExplicitCapture | RegexOptions.IgnorePatternWhitespace); + + public PropertyBagPropertyEvaluator(TermExpression term) + { + Match match = s_propertyNameRegex.Match(term.PropertyName); + _propertyName = match.Groups["name"].Value; + + string prefix = match.Groups["prefix"].Value; + if (prefix.Equals(RulePropertyPrefix)) + { + _propertyBelongsToRule = true; + } + else if (!prefix.Equals(ResultPropertyPrefix)) + { + throw new ArgumentException( + string.Format( + CultureInfo.CurrentCulture, + SdkResources.ErrorInvalidQueryPropertyPrefix, + "'" + string.Join("', '", ResultPropertyPrefix, RulePropertyPrefix)) + "'", + nameof(term)); + } + + _evaluator = CreateEvaluator(term); + } + + // Create an appropriate evaluator object for the specified term. For operators that apply + // only to strings (":" (contains), "|>" (startswith), or ">|" (endswith), always create + // a string evaluator. All other operators (==, !=, >, <, etc.) can apply to both strings + // and numbers. If the value being compared parses as a number, assume that a numeric + // comparison was intended, and create a numeric evaluator. Otherwise, create a string evaluator. + // This could cause problems if the comparand is string that happens to look like a number. + private IExpressionEvaluator CreateEvaluator(TermExpression term) => + IsStringComparison(term) + ? new StringEvaluator(GetProperty, term, StringComparison.OrdinalIgnoreCase) as IExpressionEvaluator + : new DoubleEvaluator(GetProperty, term); + + private static readonly ReadOnlyCollection s_stringSpecificOperators = + new ReadOnlyCollection( + new CompareOperator[] + { + CompareOperator.Contains, + CompareOperator.EndsWith, + CompareOperator.StartsWith + }); + + private bool IsStringComparison(TermExpression term) + => s_stringSpecificOperators.Contains(term.Operator) || !double.TryParse(term.Value, out _); + + private T GetProperty(Result result) + { + PropertyBagHolder holder = GetPropertyBagHolder(result); + return GetPropertyFromHolder(holder); + } + + private PropertyBagHolder GetPropertyBagHolder(Result result) => + _propertyBelongsToRule + ? result.GetRule() as PropertyBagHolder + : result; + + private T GetPropertyFromHolder(PropertyBagHolder holder) + { + try + { + return holder.TryGetProperty(_propertyName, out T value) ? value : default; + } + catch (JsonReaderException) + { + // Catch exceptions due to trying to perform a numeric comparison on a + // property that turns out to have a string value that can't be parsed + // as a number. The result will be that in such a case, the property + // will be treated as if its value were numeric zero. + } + + return default; + } + + public void Evaluate(ICollection results, BitArray matches) + => _evaluator.Evaluate(results, matches); + } +} diff --git a/src/Sarif/Query/Evaluators/ResultEvaluators.cs b/src/Sarif/Query/Evaluators/SarifEvaluators.cs similarity index 75% rename from src/Sarif/Query/Evaluators/ResultEvaluators.cs rename to src/Sarif/Query/Evaluators/SarifEvaluators.cs index 237b297b0..24e121e43 100644 --- a/src/Sarif/Query/Evaluators/ResultEvaluators.cs +++ b/src/Sarif/Query/Evaluators/SarifEvaluators.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using System.Globalization; using System.Linq; namespace Microsoft.CodeAnalysis.Sarif.Query.Evaluators @@ -10,7 +11,8 @@ public static class SarifEvaluators { public static IExpressionEvaluator ResultEvaluator(TermExpression term) { - switch (term.PropertyName.ToLowerInvariant()) + string propertyNameLower = term.PropertyName.ToLowerInvariant(); + switch (propertyNameLower) { case "baselinestate": return new EnumEvaluator(r => r.BaselineState, term); @@ -43,7 +45,17 @@ public static IExpressionEvaluator ResultEvaluator(TermExpression term) }, term); default: - throw new QueryParseException($"Property Name {term.PropertyName} unrecognized. Known Names: baselineState, correlationGuid, guid, hostedViewerUri, kind, level, message.text, occurrenceCount, rank, ruleId"); + if (propertyNameLower.StartsWith(PropertyBagPropertyEvaluator.ResultPropertyPrefix) || + propertyNameLower.StartsWith(PropertyBagPropertyEvaluator.RulePropertyPrefix)) + { + return new PropertyBagPropertyEvaluator(term); + } + + throw new QueryParseException( + string.Format( + CultureInfo.CurrentCulture, + SdkResources.ErrorInvalidQueryPropertyName, + term.PropertyName)); } } } diff --git a/src/Sarif/SdkResources.Designer.cs b/src/Sarif/SdkResources.Designer.cs index a6d9298f2..2625437ec 100644 --- a/src/Sarif/SdkResources.Designer.cs +++ b/src/Sarif/SdkResources.Designer.cs @@ -249,6 +249,26 @@ public static string ERR999_UnhandledEngineException { } } + /// + /// Looks up a localized string similar to The property name '{0}' is unrecognized. + ///Known property names: baselineState, correlationGuid, guid, hostedViewerUri, kind, level, message.text, occurrenceCount, rank, ruleId + ///You can also refer to properties in the result's property bag with 'properties.<propertyName>', or to properties in the associated rule's property bag with 'properties.rule.<propertyName>'.. + /// + public static string ErrorInvalidQueryPropertyName { + get { + return ResourceManager.GetString("ErrorInvalidQueryPropertyName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Property name must start with one of: {0}. + /// + public static string ErrorInvalidQueryPropertyPrefix { + get { + return ResourceManager.GetString("ErrorInvalidQueryPropertyPrefix", resourceCulture); + } + } + /// /// Looks up a localized string similar to When using the static 'Default' instance of GitHelper, you must pass the argument useCache: false to GetRepositoryRoot, which degrades performance. If the performance is not acceptable, create a separate GitHelper instance. You need not pass useCache: true because that is the default.. /// diff --git a/src/Sarif/SdkResources.resx b/src/Sarif/SdkResources.resx index a6bfa828b..10a125ba8 100644 --- a/src/Sarif/SdkResources.resx +++ b/src/Sarif/SdkResources.resx @@ -255,4 +255,12 @@ When using the static 'Default' instance of GitHelper, you must pass the argument useCache: false to GetRepositoryRoot, which degrades performance. If the performance is not acceptable, create a separate GitHelper instance. You need not pass useCache: true because that is the default. + + The property name '{0}' is unrecognized. +Known property names: baselineState, correlationGuid, guid, hostedViewerUri, kind, level, message.text, occurrenceCount, rank, ruleId +You can also refer to properties in the result's property bag with 'properties.<propertyName>', or to properties in the associated rule's property bag with 'properties.rule.<propertyName>'. + + + Property name must start with one of: {0} + \ No newline at end of file diff --git a/src/Test.UnitTests.Sarif.Multitool/QueryCommandPropertyBagTests.cs b/src/Test.UnitTests.Sarif.Multitool/QueryCommandPropertyBagTests.cs new file mode 100644 index 000000000..bf240f122 --- /dev/null +++ b/src/Test.UnitTests.Sarif.Multitool/QueryCommandPropertyBagTests.cs @@ -0,0 +1,104 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +using System.IO; +using FluentAssertions; +using Microsoft.CodeAnalysis.Sarif.Query; +using Microsoft.CodeAnalysis.Sarif.Query.Evaluators; +using Newtonsoft.Json; +using Xunit; + +namespace Microsoft.CodeAnalysis.Sarif.Multitool +{ + public class QueryCommandPropertyBagTests : IClassFixture + { + private const string FilePath = "property-bag-queries.sarif"; + + public class TestFixture + { + public TestFixture() + { + ResourceExtractor extractor = new ResourceExtractor(typeof(QueryCommandPropertyBagTests)); + File.WriteAllText(FilePath, extractor.GetResourceText($"QueryCommand.{FilePath}")); + } + } + + [Fact] + public void QueryCommand_CanAccessResultAndRulePropertyBags() + { + RunAndVerifyCount(2, "properties.name == 'Terisa'"); + RunAndVerifyCount(2, "rule.properties.Category == 'security'"); + } + + [Fact] + public void QueryCommand_ComparesPropertyBagPropertyNamesCaseSensitively() + { + RunAndVerifyCount(0, "properties.nAmE == 'Terisa'"); + RunAndVerifyCount(0, "rule.properties.CaTegORy == 'security'"); + } + + [Fact] + public void QueryCommand_ReadsIntegerProperty() + { + RunAndVerifyCount(1, "properties.count == 42"); + } + + [Fact] + public void QueryCommand_ReadsFloatProperty() + { + RunAndVerifyCount(2, "properties.confidence >= 0.95"); + } + + [Fact] + public void QueryCommand_PerformsStringSpecificComparisons() + { + RunAndVerifyCount(1, "properties.confidence : 95"); // contains + RunAndVerifyCount(3, "properties.confidence |> 0."); // startswith + RunAndVerifyCount(1, "properties.confidence >| 9"); // endswith + } + + [Fact] + public void QueryCommand_TreatsUnparseableValueAsHavingTheDefaultValue() + { + // In this test, all the results will match, so we need to know how many there are. + SarifLog sarifLog = JsonConvert.DeserializeObject(File.ReadAllText(FilePath)); + int numResults = sarifLog.Runs[0].Results.Count; + + // 'name' is a string-valued property that doesn't parse to an integer. The + // PropertyBagPropertyEvaluator sees a number on the right-hand side of the comparison + // and decides that a numeric comparison is intended. When it encounters a property + // value that can't be parsed as a number, it treats it as numeric 0 rather than + // throwing. + RunAndVerifyCount(numResults, "properties.name == 0"); + } + + // The above tests cover all but one code block in the underlying PropertyBagPropertyEvaluator. + // It doesn't cover the case of an invalid "prefix" (that is, a property name that doesn't + // start with "properties." or "rule.properties"), because QueryCommand never creates a + // PropertyBagPropertyEvaluator for a term with such a property name. So we cover that + // case here: + [Fact] + public void PropertyBagPropertyEvaluator_RejectsPropertyNameWithInvalidPrefix() + { + var expression = new TermExpression(propertyName: "invalid.prefix", op: CompareOperator.Equals, value: string.Empty); + + Action action = () => new PropertyBagPropertyEvaluator(expression); + + action.Should().Throw(); + } + + private void RunAndVerifyCount(int expectedCount, string expression) + { + var options = new QueryOptions + { + Expression = expression, + InputFilePath = FilePath, + ReturnCount = true + }; + + int exitCode = new QueryCommand().RunWithoutCatch(options); + exitCode.Should().Be(expectedCount); + } + } +} diff --git a/src/Test.UnitTests.Sarif.Multitool/Test.UnitTests.Sarif.Multitool.csproj b/src/Test.UnitTests.Sarif.Multitool/Test.UnitTests.Sarif.Multitool.csproj index d2ce02f08..17814d487 100644 --- a/src/Test.UnitTests.Sarif.Multitool/Test.UnitTests.Sarif.Multitool.csproj +++ b/src/Test.UnitTests.Sarif.Multitool/Test.UnitTests.Sarif.Multitool.csproj @@ -13,16 +13,25 @@ true + + + + + + + + - + + diff --git a/src/Test.UnitTests.Sarif.Multitool/TestData/QueryCommand/property-bag-queries.sarif b/src/Test.UnitTests.Sarif.Multitool/TestData/QueryCommand/property-bag-queries.sarif new file mode 100644 index 000000000..e997aea19 --- /dev/null +++ b/src/Test.UnitTests.Sarif.Multitool/TestData/QueryCommand/property-bag-queries.sarif @@ -0,0 +1,173 @@ +{ + "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json", + "version": "2.1.0", + "runs": [ + { + "tool": { + "driver": { + "name": "Query Tests", + "rules": [ + { + "id": "TEST1001", + "name": "NoPropertyBag" + }, + { + "id": "TEST1002", + "name": "PropertyBagWithNoMatchingProperty", + "properties": { + "someOtherProperty": "x" + } + }, + { + "id": "TEST1003", + "name": "PropertyBagWithMatchingPropertyMismatchedValue", + "properties": { + "Category": "privacy" + } + }, + { + "id": "TEST1004", + "name": "PropertyBagWithMatchingValue", + "properties": { + "Category": "security" + } + }, + { + "id": "TEST1005", + "name": "PropertyBagWithCaseInsensitiveMatchingValue", + "properties": { + "Category": "Security" + } + } + ] + } + }, + "results": [ + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "message": { + "text": "Message." + }, + "properties": { + "comment": "Exact match.", + "name": "Terisa" + } + }, + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "message": { + "text": "Message." + }, + "properties": { + "comment": "Case-insensitive match.", + "name": "terisa" + } + }, + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "message": { + "text": "Message." + }, + "properties": { + "comment": "No 'name' property.", + "SomeOtherProperty": "5.0" + } + }, + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "message": { + "text": "Message." + } + }, + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "message": { + "text": "Message." + }, + "properties": { + "count": 42 + } + }, + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "message": { + "text": "Message." + }, + "properties": { + "count": 54 + } + }, + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "message": { + "text": "Message." + }, + "properties": { + "confidence": 0.89 + } + }, + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "message": { + "text": "Message." + }, + "properties": { + "confidence": 0.95 + } + }, + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "message": { + "text": "Message." + }, + "properties": { + "confidence": 0.96 + } + }, + { + "ruleId": "TEST1002", + "ruleIndex": 1, + "message": { + "text": "Message." + } + }, + { + "ruleId": "TEST1003", + "ruleIndex": 2, + "message": { + "text": "Message." + } + }, + { + "ruleId": "TEST1004", + "ruleIndex": 3, + "message": { + "text": "Message." + } + }, + { + "ruleId": "TEST1005", + "ruleIndex": 4, + "message": { + "text": "Message." + } + }, + { + "ruleId": "TEST1006", + "message": { + "text": "This result doesn't point to a rule, so a rule.properties query on it should return no results." + } + } + ] + } + ] +} \ No newline at end of file diff --git a/src/Test.UnitTests.Sarif/Query/EvaluatorTests.cs b/src/Test.UnitTests.Sarif/Query/EvaluatorTests.cs index f766748f5..50cfc748a 100644 --- a/src/Test.UnitTests.Sarif/Query/EvaluatorTests.cs +++ b/src/Test.UnitTests.Sarif/Query/EvaluatorTests.cs @@ -1,4 +1,7 @@ -using System; +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; using System.Collections; using System.Collections.Generic; using System.Linq;