diff --git a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs index 27074b9f3..e536b68de 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs +++ b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs @@ -199,7 +199,17 @@ internal class RuleResources { } /// - /// Looks up a localized string similar to {0}: This "physicalLocation" object contains a "contextRegion" property, but it does not contain a "region" property.. + /// Looks up a localized string similar to {0}: This 'physicalLocation' object contains both a 'region' and a 'contextRegion' property, but 'contextRegion' is not a proper superset of 'region'. This is invalid because the purpose of 'contextRegion' is to provide a viewing context around the 'region' which is the location of the result. It's possible that the tool reversed 'region' and 'contextRegion'. If 'region' and 'contextRegion' are identical, the tool must omit 'contextRegion'.. + /// + internal static string SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text { + get { + return ResourceManager.GetString("SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBePro" + + "perSupersetOfRegion_Text", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to {0}: This 'physicalLocation' object contains a 'contextRegion' property, but it does not contain a 'region' property. This is invalid because the purpose of 'contextRegion' is to provide a viewing context around the 'region' which is the location of the result. If a tool associates only one region with a result, it must populate 'region', not 'contextRegion'.. /// internal static string SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text { get { @@ -209,7 +219,11 @@ internal class RuleResources { } /// - /// Looks up a localized string similar to If the "contextRegion" property is present, the "region" property must also be present.. + /// Looks up a localized string similar to Ensure consistency among the properties of a 'physicalLocation' object. + /// + ///A SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'. If 'contextRegion' is present, then 'region' must also be present, and 'contextRegion' must be a "proper superset" of 'region'. That is, 'contextRegion' must completely contain 'region', and it must be larger than 'region'. To understand why this is so we must understand the roles of the 'region' and 'contextRegion' properties. + /// + ///'region' allo [rest of string was truncated]";. /// internal static string SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text { get { diff --git a/src/Sarif.Multitool/Rules/RuleResources.resx b/src/Sarif.Multitool/Rules/RuleResources.resx index 79ca87dd7..c725fae71 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.resx +++ b/src/Sarif.Multitool/Rules/RuleResources.resx @@ -154,10 +154,18 @@ Certain URIs are required to be absolute. - If the "contextRegion" property is present, the "region" property must also be present. + Ensure consistency among the properties of a 'physicalLocation' object. + +A SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'. If 'contextRegion' is present, then 'region' must also be present, and 'contextRegion' must be a "proper superset" of 'region'. That is, 'contextRegion' must completely contain 'region', and it must be larger than 'region'. To understand why this is so we must understand the roles of the 'region' and 'contextRegion' properties. + +'region' allows both users and tools to distinguish similar results within the same artifact. If a SARIF viewer has access to the artifact, it can display it, and highlight the location identified by the analysis tool. If the region has a 'snippet' property, then even if the viewer doesn't have access to the artifact (which might be the case for a web-based viewer), it can still display the faulty code. + +'contextRegion' provides users with a broader view of the result location. Typically, it consists of a range starting a few lines before 'region' and ending a few lines after. Again, if a SARIF viewer has access to the artifact, it can display it, and highlight the context region (perhaps in a lighter shade than the region itself). This isn't terribly useful since the user can already see the whole file, with the 'region' already highlighted. But if 'contextRegion' has a 'snippet' property, then even a viewer without access to the artifact can display a few lines of code surrounding the actual result, which is helpful to users. + +If the validator reports that 'contextRegion' is not a proper superset of 'region', then it's possible that the tool reversed 'region' and 'contextRegion'. If 'region' and 'contextRegion' are identical, the tool should simply omit 'contextRegion'. - {0}: This "physicalLocation" object contains a "contextRegion" property, but it does not contain a "region" property. + {0}: This 'physicalLocation' object contains a 'contextRegion' property, but it does not contain a 'region' property. This is invalid because the purpose of 'contextRegion' is to provide a viewing context around the 'region' which is the location of the result. If a tool associates only one region with a result, it must populate 'region', not 'contextRegion'. {0}: This "{1}" object contains a property "{2}" with value {3}, but "{4}" has fewer than {5} elements. @@ -204,6 +212,9 @@ Placeholder_SARIF1007_RegionPropertiesMustBeConsistent_FullDescription_Text + + {0}: This 'physicalLocation' object contains both a 'region' and a 'contextRegion' property, but 'contextRegion' is not a proper superset of 'region'. This is invalid because the purpose of 'contextRegion' is to provide a viewing context around the 'region' which is the location of the result. It's possible that the tool reversed 'region' and 'contextRegion'. If 'region' and 'contextRegion' are identical, the tool must omit 'contextRegion'. + Placeholder diff --git a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs index 65b445ebe..41a6ef1f0 100644 --- a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs +++ b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs @@ -7,25 +7,81 @@ namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules { public class PhysicalLocationPropertiesMustBeConsistent : SarifValidationSkimmerBase { - public override MultiformatMessageString FullDescription => new MultiformatMessageString - { - Text = RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text - }; + /// + /// SARIF1008 + /// + public override string Id => RuleId.PhysicalLocationPropertiesMustBeConsistent; - public override FailureLevel DefaultLevel => FailureLevel.Error; + /// + /// Ensure consistency among the properties of a 'physicalLocation' object. + /// + /// A SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'. + /// If 'contextRegion' is present, then 'region' must also be present, and 'contextRegion' + /// must be a "proper superset" of 'region'. That is, 'contextRegion' must completely contain + /// 'region', and it must be larger than 'region'. To understand why this is so we must + /// understand the roles of the 'region' and 'contextRegion' properties. + /// + /// 'region' allows both users and tools to distinguish similar results within the same + /// 'artifact. If a SARIF viewer has access to the artifact, it can display it, and highlight + /// the location identified by the analysis tool.If the region has a 'snippet' property, + /// then even if the viewer doesn't have access to the artifact (which might be the case + /// for a web-based viewer), it can still display the faulty code. + /// + /// 'contextRegion' provides users with a broader view of the result location. Typically, + /// it consists of a range starting a few lines before 'region' and ending a few lines after. + /// Again, if a SARIF viewer has access to the artifact, it can display it, and highlight + /// the context region (perhaps in a lighter shade than the region itself). This isn't + /// terribly useful since the user can already see the whole file, with the 'region' + /// already highlighted. But if 'contextRegion' has a 'snippet' property, then even a + /// viewer without access to the artifact can display a few lines of code surrounding + /// the actual result, which is helpful to users. + /// + /// If the validator reports that 'contextRegion' is not a proper superset of 'region', + /// then it's possible that the tool reversed 'region' and 'contextRegion'. If 'region' + /// and 'contextRegion' are identical, the tool should simply omit 'contextRegion'. + /// + public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text }; - public override string Id => RuleId.PhysicalLocationPropertiesMustBeConsistent; + protected override IEnumerable MessageResourceNames => new string[] { + nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text), + nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text) + }; - protected override IEnumerable MessageResourceNames => new string[] - { - nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text) - }; + public override FailureLevel DefaultLevel => FailureLevel.Error; protected override void Analyze(PhysicalLocation physicalLocation, string physicalLocationPointer) { - if (physicalLocation.ContextRegion != null && physicalLocation.Region == null) + if (physicalLocation.ContextRegion == null) + { + return; + } + + // ContextRegionRequiresRegion: If 'contextRegion' is present, then 'region' must also be present. + if (physicalLocation.Region == null) + { + // {0}: This 'physicalLocation' object contains a 'contextRegion' property, but it + // does not contain a 'region' property. This is invalid because the purpose of + // 'contextRegion' is to provide a viewing context around the 'region' which is the + // location of the result. If a tool associates only one region with a result, it + // must populate 'region', not 'contextRegion'. + LogResult( + physicalLocationPointer, + nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text)); + return; + } + + // ContextRegionMustBeProperSupersetOfRegion: 'contextRegion' must be a proper superset of 'region'. + if (!physicalLocation.ContextRegion.IsProperSupersetOf(physicalLocation.Region)) { - LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text)); + // {0}: This 'physicalLocation' object contains both a 'region' and a 'contextRegion' + // property, but 'contextRegion' is not a proper superset of 'region'. This is invalid + // because the purpose of 'contextRegion' is to provide a viewing context around the + // 'region' which is the location of the result. It's possible that the tool reversed + // 'region' and 'contextRegion'. If 'region' and 'contextRegion' are identical, the + // tool must omit 'contextRegion'. + LogResult( + physicalLocationPointer, + nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text)); } } } diff --git a/src/Sarif/Core/Region.cs b/src/Sarif/Core/Region.cs index e67219992..744eb9c76 100644 --- a/src/Sarif/Core/Region.cs +++ b/src/Sarif/Core/Region.cs @@ -1,27 +1,184 @@ // 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.Runtime.CompilerServices; + namespace Microsoft.CodeAnalysis.Sarif { public partial class Region { - public bool IsBinaryRegion + public bool IsBinaryRegion => this.ByteOffset >= 0; + + public bool IsLineColumnBasedTextRegion => this.StartLine >= 1; + + public bool IsOffsetBasedTextRegion => this.CharOffset >= 0; + + public override string ToString() + { + return this.FormatForVisualStudio(); + } + + public void PopulateDefaults() { - get + if (this.IsLineColumnBasedTextRegion) + { + this.PopulateLineColumnBasedTextDefaults(); + } + + if (this.IsOffsetBasedTextRegion) { - // Is this right? What about an insertion point right after a BOM in a text file?? - // Do we need to just bite the bullet and make these Nullable type so that we have a - // clear indicator of whether the region is binary vs. textual? I tend to think so. - return - this.StartLine == 0 && - this.CharLength == 0 && - this.CharOffset == 0; + this.PopulateOffsetBasedTextDefaults(); + } + + if (this.IsBinaryRegion) + { + this.PopulateBinaryDefaults(); } } - public override string ToString() + private void PopulateLineColumnBasedTextDefaults() { - return this.FormatForVisualStudio(); + if (this.EndLine == 0) + { + this.EndLine = this.StartLine; + } + + if (this.StartColumn == 0) + { + this.StartColumn = 1; + } + + if (this.EndColumn == 0) + { + this.EndColumn = int.MaxValue; + } + } + + private void PopulateOffsetBasedTextDefaults() + { + if (this.CharLength == -1) + { + this.CharLength = 0; + } + } + + private void PopulateBinaryDefaults() + { + if (this.ByteLength == -1) + { + this.ByteLength = 0; + } + } + + public bool IsProperSupersetOf(Region subRegion) + { + this.PopulateDefaults(); + subRegion.PopulateDefaults(); + + if (this.IsLineColumnBasedTextRegion && + subRegion.IsLineColumnBasedTextRegion && + !IsLineColumnBasedTextRegionProperSupersetOf(subRegion)) + { + return false; + } + + if (this.IsOffsetBasedTextRegion && + subRegion.IsOffsetBasedTextRegion && + !IsOffsetBasedTextRegionProperSupetSetOf(subRegion)) + { + return false; + } + + if (this.IsBinaryRegion && + subRegion.IsBinaryRegion && + !IsBinaryRegionProperSupersetOf(subRegion)) + { + return false; + } + + // if we reach here, the region and context region have been expressed as different property sets, + // and it is not possible to judge validity without looking at the actual content. + // It is a potential false negative. + return true; + } + + private bool IsLineColumnBasedTextRegionProperSupersetOf(Region subRegion) + { + if (this.StartLine > subRegion.StartLine || this.EndLine < subRegion.EndLine) + { + return false; + } + + if (this.StartLine == subRegion.StartLine && this.StartColumn > subRegion.StartColumn) + { + return false; + } + + if (this.EndLine == subRegion.EndLine && this.EndColumn < subRegion.EndColumn) + { + return false; + } + + if (this.StartLine == subRegion.StartLine && + this.EndLine == subRegion.EndLine && + this.StartColumn == subRegion.StartColumn && + this.EndColumn == subRegion.EndColumn) + { + return false; + } + + return true; + } + + private bool IsBinaryRegionProperSupersetOf(Region subRegion) + { + if (this.ByteOffset > subRegion.ByteOffset) + { + return false; + } + + if (GetByteEndOffset(this) < GetByteEndOffset(subRegion)) + { + return false; + } + + if (this.ByteOffset == subRegion.ByteOffset && this.ByteLength <= subRegion.ByteLength) + { + return false; + } + + return true; + } + + private bool IsOffsetBasedTextRegionProperSupetSetOf(Region subRegion) + { + if (this.CharOffset > subRegion.CharOffset) + { + return false; + } + + if (GetCharEndOffset(this) < GetCharEndOffset(subRegion)) + { + return false; + } + + if (this.CharOffset == subRegion.CharOffset && this.CharLength <= subRegion.CharLength) + { + return false; + } + + return true; + } + + private static int GetCharEndOffset(Region region) + { + return region.CharOffset + region.CharLength; + } + + private static int GetByteEndOffset(Region region) + { + return region.ByteOffset + region.ByteLength; } } } diff --git a/src/Sarif/FileRegionsCache.cs b/src/Sarif/FileRegionsCache.cs index 76f2b4f6b..201eaac95 100644 --- a/src/Sarif/FileRegionsCache.cs +++ b/src/Sarif/FileRegionsCache.cs @@ -132,7 +132,7 @@ private void PopulatePropertiesFromCharOffsetAndLength(NewLineIndex newLineIndex { Assert(!region.IsBinaryRegion); Assert(region.StartLine == 0); - Assert(region.CharLength > 0 || region.CharOffset > 0); + Assert(region.CharLength >= 0 || region.CharOffset >= 0); int startLine, startColumn, endLine, endColumn; diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1008.PhysicalLocationPropertiesMustBeConsistent_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1008.PhysicalLocationPropertiesMustBeConsistent_Invalid.sarif index 40e384836..fff04b9d8 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1008.PhysicalLocationPropertiesMustBeConsistent_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1008.PhysicalLocationPropertiesMustBeConsistent_Invalid.sarif @@ -11,14 +11,17 @@ "id": "SARIF1008", "name": "PhysicalLocationPropertiesMustBeConsistent", "shortDescription": { - "text": "If the \"contextRegion\" property is present, the \"region\" property must also be present." + "text": "Ensure consistency among the properties of a 'physicalLocation' object." }, "fullDescription": { - "text": "If the \"contextRegion\" property is present, the \"region\" property must also be present." + "text": "Ensure consistency among the properties of a 'physicalLocation' object.\r\n\r\nA SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'. If 'contextRegion' is present, then 'region' must also be present, and 'contextRegion' must be a \"proper superset\" of 'region'. That is, 'contextRegion' must completely contain 'region', and it must be larger than 'region'. To understand why this is so we must understand the roles of the 'region' and 'contextRegion' properties.\r\n\r\n'region' allows both users and tools to distinguish similar results within the same artifact. If a SARIF viewer has access to the artifact, it can display it, and highlight the location identified by the analysis tool. If the region has a 'snippet' property, then even if the viewer doesn't have access to the artifact (which might be the case for a web-based viewer), it can still display the faulty code.\r\n\r\n'contextRegion' provides users with a broader view of the result location. Typically, it consists of a range starting a few lines before 'region' and ending a few lines after. Again, if a SARIF viewer has access to the artifact, it can display it, and highlight the context region (perhaps in a lighter shade than the region itself). This isn't terribly useful since the user can already see the whole file, with the 'region' already highlighted. But if 'contextRegion' has a 'snippet' property, then even a viewer without access to the artifact can display a few lines of code surrounding the actual result, which is helpful to users.\r\n\r\nIf the validator reports that 'contextRegion' is not a proper superset of 'region', then it's possible that the tool reversed 'region' and 'contextRegion'. If 'region' and 'contextRegion' are identical, the tool should simply omit 'contextRegion'." }, "messageStrings": { "Error_ContextRegionRequiresRegion": { - "text": "{0}: This \"physicalLocation\" object contains a \"contextRegion\" property, but it does not contain a \"region\" property." + "text": "{0}: This 'physicalLocation' object contains a 'contextRegion' property, but it does not contain a 'region' property. This is invalid because the purpose of 'contextRegion' is to provide a viewing context around the 'region' which is the location of the result. If a tool associates only one region with a result, it must populate 'region', not 'contextRegion'." + }, + "Error_ContextRegionMustBeProperSupersetOfRegion": { + "text": "{0}: This 'physicalLocation' object contains both a 'region' and a 'contextRegion' property, but 'contextRegion' is not a proper superset of 'region'. This is invalid because the purpose of 'contextRegion' is to provide a viewing context around the 'region' which is the location of the result. It's possible that the tool reversed 'region' and 'contextRegion'. If 'region' and 'contextRegion' are identical, the tool must omit 'contextRegion'." } }, "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" @@ -63,6 +66,390 @@ } } ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[1].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 40, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[2].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 68, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[3].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 92, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[4].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 116, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[5].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 140, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[6].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 163, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[7].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 188, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[8].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 212, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[9].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 236, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[10].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 260, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[11].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 284, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[12].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 307, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[13].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 331, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[14].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 355, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[15].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 379, + "startColumn": 35 + } + } + } + ] + }, + { + "ruleId": "SARIF1008", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_ContextRegionMustBeProperSupersetOfRegion", + "arguments": [ + "runs[0].results[16].locations[0].physicalLocation" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 403, + "startColumn": 35 + } + } + } + ] } ], "columnKind": "utf16CodeUnits" diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1008.PhysicalLocationPropertiesMustBeConsistent_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1008.PhysicalLocationPropertiesMustBeConsistent_Invalid.sarif index b77d8b77e..9eb86c41d 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1008.PhysicalLocationPropertiesMustBeConsistent_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1008.PhysicalLocationPropertiesMustBeConsistent_Invalid.sarif @@ -14,7 +14,7 @@ "ruleId": "TST0001", "level": "error", "message": { - "text": "Some testing occurred." + "text": "Bad result - context region is present without region." }, "locations": [ { @@ -29,6 +29,392 @@ } } ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - partially overlapping region and contextregion (text and line/column based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "startLine": 1, + "startColumn": 5, + "endLine": 10, + "endColumn": 15 + }, + "contextRegion": { + "startLine": 1, + "startColumn": 1, + "endLine": 10, + "endColumn": 5 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - non-overlapping region and contextregion (text and line/column based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "startLine": 5, + "endLine": 10 + }, + "contextRegion": { + "startLine": 2, + "endLine": 4 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - identical region and contextregion (text and line/column based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "startLine": 2, + "endLine": 4 + }, + "contextRegion": { + "startLine": 2, + "endLine": 4 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - contextregion smaller than region (text and line/column based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "startLine": 2, + "endLine": 5 + }, + "contextRegion": { + "startLine": 3, + "endLine": 4 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - default populated endLine (same as startLine) (text and line/column based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "startLine": 5 + }, + "contextRegion": { + "startLine": 5, + "endLine": 5 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - default populated endColumn (int.maxvalue) (text and line/column based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "startLine": 2, + "startColumn": 5 + }, + "contextRegion": { + "startLine": 2, + "startColumn": 5, + "endColumn": 8 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - partially overlapping region and contextregion (text and offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "charOffset": 5, + "charLength": 25 + }, + "contextRegion": { + "charOffset": 10, + "charLength": 25 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - non-overlapping region and contextregion (text and offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "charOffset": 10, + "charLength": 25 + }, + "contextRegion": { + "charOffset": 37, + "charLength": 25 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - identical region and contextregion (text and offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "charOffset": 10, + "charLength": 25 + }, + "contextRegion": { + "charOffset": 10, + "charLength": 25 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - contextregion smaller than region (text and offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "charOffset": 5, + "charLength": 20 + }, + "contextRegion": { + "charOffset": 10, + "charLength": 5 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - default populated charLength (0) (text and offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "charOffset": 5, + "charLength": 20 + }, + "contextRegion": { + "charOffset": 5 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - partially overlapping region and contextregion (binary offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "byteOffset": 5, + "byteLength": 25 + }, + "contextRegion": { + "byteOffset": 10, + "byteLength": 25 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - non-overlapping region and contextregion (binary offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "byteOffset": 10, + "byteLength": 25 + }, + "contextRegion": { + "byteOffset": 35, + "byteLength": 25 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - identical region and contextregion (binary offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "byteOffset": 10, + "byteLength": 25 + }, + "contextRegion": { + "byteOffset": 10, + "byteLength": 25 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - contextregion smaller than region (binary offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "byteOffset": 10, + "byteLength": 25 + }, + "contextRegion": { + "byteOffset": 15, + "byteLength": 5 + } + } + } + ] + }, + { + "ruleId": "TST0001", + "level": "error", + "message": { + "text": "Bad result - default populated byteLength (0) (binary offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "byteOffset": 5, + "byteLength": 20 + }, + "contextRegion": { + "byteOffset": 5 + } + } + } + ] } ], "columnKind": "utf16CodeUnits" diff --git a/src/Test.UnitTests.Sarif/Core/RegionTests.cs b/src/Test.UnitTests.Sarif/Core/RegionTests.cs new file mode 100644 index 000000000..ef4aa1467 --- /dev/null +++ b/src/Test.UnitTests.Sarif/Core/RegionTests.cs @@ -0,0 +1,128 @@ +// Copyright (c) Microsoft. All Rights Reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using FluentAssertions; +using Microsoft.CodeAnalysis.Sarif; +using Xunit; + +namespace Microsoft.CodeAnalysis.Test.UnitTests.Sarif.Core +{ + public class RegionTests + { + [Fact] + public void Region_PopulateDefaults_EndLineColumnDefaultsPopulatedCorrectly() + { + var region = new Region + { + StartLine = 5, + StartColumn = 20 + }; + + region.PopulateDefaults(); + + region.StartLine.Should().Be(5); + region.StartColumn.Should().Be(20); + region.EndLine.Should().Be(5); + region.EndColumn.Should().Be(int.MaxValue); + } + + [Fact] + public void Region_PopulateDefaults_StartColumnDefaultsPopulatedCorrectly() + { + var region = new Region + { + StartLine = 5, + }; + + region.PopulateDefaults(); + + region.StartLine.Should().Be(5); + region.StartColumn.Should().Be(1); + region.EndLine.Should().Be(5); + region.EndColumn.Should().Be(int.MaxValue); + } + + [Fact] + public void Region_PopulateDefaults_TextOffsetDefaultsPopulatedCorrectly() + { + var region = new Region + { + CharOffset = 5 + }; + + region.PopulateDefaults(); + + region.CharOffset.Should().Be(5); + region.CharLength.Should().Be(0); + } + + [Fact] + public void Region_PopulateDefaults_BinaryOffsetDefaultsPopulatedCorrectly() + { + var region = new Region + { + ByteOffset = 15 + }; + + region.PopulateDefaults(); + + region.ByteOffset.Should().Be(15); + region.ByteLength.Should().Be(0); + } + + [Fact] + public void Region_IsBinaryRegion_ComputesCorrectly() + { + var region = new Region + { + ByteOffset = 15 + }; + + region.IsLineColumnBasedTextRegion.Should().BeFalse(); + region.IsOffsetBasedTextRegion.Should().BeFalse(); + region.IsBinaryRegion.Should().BeTrue(); + } + + [Fact] + public void Region_IsLineColumnBasedTextRegion_ComputesCorrectly() + { + var region = new Region + { + StartLine = 23 + }; + + region.IsLineColumnBasedTextRegion.Should().BeTrue(); + region.IsOffsetBasedTextRegion.Should().BeFalse(); + region.IsBinaryRegion.Should().BeFalse(); + } + + [Fact] + public void Region_IsOffsetBasedTextRegion_ComputesCorrectly() + { + var region = new Region + { + CharOffset = 15 + }; + + region.IsLineColumnBasedTextRegion.Should().BeFalse(); + region.IsOffsetBasedTextRegion.Should().BeTrue(); + region.IsBinaryRegion.Should().BeFalse(); + } + + [Fact] + public void Region_HasAllRegionTypes_ComputesPropertiesCorrectly() + { + var region = new Region + { + StartLine = 23, + CharOffset = 15, + ByteOffset = 15 + + }; + + region.IsLineColumnBasedTextRegion.Should().BeTrue(); + region.IsOffsetBasedTextRegion.Should().BeTrue(); + region.IsBinaryRegion.Should().BeTrue(); + } + } +} \ No newline at end of file diff --git a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs index 8203e0d70..2fb0a9e42 100644 --- a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs +++ b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs @@ -55,19 +55,19 @@ public TestCaseData(Region inputRegion, Region outputRegion) private const string LINES_2_AND_3 = "efg\r\nhijk"; private const string CARRIAGE_RETURN_NEW_LINE = "\r\n"; - private readonly static Region s_Insertion_Beginning_Of_Binary_File = + private readonly static Region s_Insertion_Beginning_Of_OffsetBased_Text_File = new Region() { - Snippet = null, - StartLine = 0, - StartColumn = 0, - EndLine = 0, - EndColumn = 0, + Snippet = new ArtifactContent() { Text = INSERTION_POINT }, + StartLine = 1, + StartColumn = 1, + EndLine = 1, + EndColumn = 1, CharOffset = 0, CharLength = 0 }; - private readonly static Region s_Insertion_Beginning_Of_Text_File = + private readonly static Region s_Insertion_Beginning_Of_LineColumnBased_Text_File = new Region() { Snippet = new ArtifactContent() { Text = INSERTION_POINT }, @@ -79,6 +79,20 @@ public TestCaseData(Region inputRegion, Region outputRegion) CharLength = 0 }; + private readonly static Region s_Insertion_Beginning_Of_Binary_File = + new Region() + { + Snippet = null, + StartLine = 0, + StartColumn = 0, + EndLine = 0, + EndColumn = 0, + CharOffset = -1, + CharLength = 0, + ByteOffset = 0, + ByteLength = 0 + }; + private readonly static Region s_Insertion_End_Of_File = new Region() { @@ -272,15 +286,19 @@ public TestCaseData(Region inputRegion, Region outputRegion) private static readonly ReadOnlyCollection s_specExampleTestCases = new ReadOnlyCollection(new TestCaseData[] { - // Insertion point at beginning of binary file - new TestCaseData(outputRegion : s_Insertion_Beginning_Of_Binary_File, + // Insertion point at beginning of an offset based text file + new TestCaseData(outputRegion : s_Insertion_Beginning_Of_OffsetBased_Text_File, inputRegion: new Region() { CharOffset = 0}), - // Insertion point at beginning of text file, can only + // Insertion point at beginning of a line/column based text file, can only // be denoted by use of startLine - new TestCaseData(outputRegion : s_Insertion_Beginning_Of_Text_File, + new TestCaseData(outputRegion : s_Insertion_Beginning_Of_LineColumnBased_Text_File, inputRegion: new Region() { StartLine = 1, StartColumn = 1, EndColumn = 1, CharOffset = 0 }), + // Insertion point at beginning of a based binary file + new TestCaseData(outputRegion : s_Insertion_Beginning_Of_Binary_File, + inputRegion: new Region() { ByteOffset = 0}), + new TestCaseData(outputRegion : s_Insertion_End_Of_File, inputRegion: new Region() { CharOffset = 20 }),