From 8b11976ccc38ac6146a673599c910b6abf47fb2a Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Mon, 22 Jun 2020 15:16:10 -0700 Subject: [PATCH 01/11] Add ContextRegionMustBeProperSupersetOfRegion check --- .../Rules/RuleResources.Designer.cs | 16 +- src/Sarif.Multitool/Rules/RuleResources.resx | 13 +- ...sicalLocationPropertiesMustBeConsistent.cs | 122 +++++++++++++- ...onPropertiesMustBeConsistent_Invalid.sarif | 153 +++++++++++++++++- ...onPropertiesMustBeConsistent_Invalid.sarif | 150 ++++++++++++++++- 5 files changed, 444 insertions(+), 10 deletions(-) diff --git a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs index db0c92508..189a498fc 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs +++ b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs @@ -199,7 +199,17 @@ internal static string SARIF1007_RegionPropertiesMustBeConsistent_FullDescriptio } /// - /// 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. If the tool simply reversed 'region', then fix it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must n [rest of string was truncated]";. + /// + 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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does.. /// internal static string SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text { get { @@ -209,7 +219,9 @@ internal static string SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Erro } /// - /// 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 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 arti [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 8eb62cc77..7f4db6630 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.resx +++ b/src/Sarif.Multitool/Rules/RuleResources.resx @@ -154,10 +154,16 @@ Certain URIs are required to be absolute. - If the "contextRegion" property is present, the "region" property must also be present. + 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 SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply neglected to populate 'region'. 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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does. {0}: This "{1}" object contains a property "{2}" with value {3}, but "{4}" has fewer than {5} elements. @@ -204,4 +210,7 @@ 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. If the tool simply reversed 'region', then fix it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it. + \ No newline at end of file diff --git a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs index 65b445ebe..3d931551b 100644 --- a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs +++ b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs @@ -1,7 +1,10 @@ // 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.Collections.Generic; +using System.Linq.Expressions; +using System.Runtime.InteropServices.WindowsRuntime; namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules { @@ -18,15 +21,130 @@ public class PhysicalLocationPropertiesMustBeConsistent : SarifValidationSkimmer protected override IEnumerable MessageResourceNames => new string[] { - nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text) + nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text), + nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text) }; protected override void Analyze(PhysicalLocation physicalLocation, string physicalLocationPointer) { - if (physicalLocation.ContextRegion != null && physicalLocation.Region == null) + if (physicalLocation.ContextRegion == null) + { + return; + } + + if (physicalLocation.Region == null) { LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text)); + return; + } + + if (!IsRegionProperSuperset(physicalLocation.ContextRegion, physicalLocation.Region)) + { + LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text)); + } + } + + private static bool IsRegionProperSuperset(Region superRegion, Region subRegion) + { + if (IsLineColumnBasedTextRegion(superRegion) && + IsLineColumnBasedTextRegion(subRegion) && + !IsLineColumnBasedTextRegionProperSuperset(superRegion, subRegion)) + { + return false; + } + + if (IsOffsetBasedTextRegion(superRegion) && + IsOffsetBasedTextRegion(subRegion) && + !IsOffsetBasedTextRegionProperSupetSet(superRegion, subRegion)) + { + + return false; } + + if (IsBinaryRegion(superRegion) && + IsBinaryRegion(subRegion) && + !IsBinaryRegionProperSuperset(superRegion, 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 static bool IsBinaryRegionProperSuperset(Region superRegion, Region subRegion) + { + if (superRegion.ByteOffset > subRegion.ByteOffset) + { + return false; + } + + if (superRegion.ByteOffset == subRegion.ByteOffset && superRegion.ByteLength <= subRegion.ByteLength) + { + return false; + } + + return true; + } + + private static bool IsLineColumnBasedTextRegionProperSuperset(Region superRegion, Region subRegion) + { + if (superRegion.StartLine > subRegion.StartLine || superRegion.EndLine < subRegion.EndLine) + { + return false; + } + + if (superRegion.StartLine == subRegion.StartLine && superRegion.StartColumn < subRegion.StartColumn) + { + return false; + } + + if (superRegion.EndLine == subRegion.EndLine && superRegion.EndColumn > subRegion.EndColumn) + { + return false; + } + + if (superRegion.StartLine == subRegion.StartLine && + superRegion.EndLine == subRegion.EndLine && + superRegion.StartColumn == subRegion.StartColumn && + superRegion.EndColumn == subRegion.EndColumn) + { + return false; + } + + return true; + } + + private static bool IsOffsetBasedTextRegionProperSupetSet(Region superRegion, Region subRegion) + { + if (superRegion.CharOffset > subRegion.CharOffset) + { + return false; + } + + if (superRegion.CharOffset == subRegion.CharOffset && superRegion.CharLength <= subRegion.CharLength) + { + return false; + } + return true; + } + + private static bool IsLineColumnBasedTextRegion(Region region) + { + return region.StartLine >= 1; + } + + private static bool IsOffsetBasedTextRegion(Region region) + { + return region.CharOffset > 0; + } + + private static bool IsBinaryRegion(Region region) + { + return region.ByteOffset >= 0; } } } 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 bd4d56dde..1a017757e 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": "A SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'." }, "fullDescription": { - "text": "If the \"contextRegion\" property is present, the \"region\" property must also be present." + "text": "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.\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 SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply neglected to populate 'region'. 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'." }, "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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does." + }, + "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. If the tool simply reversed 'region', then fix it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it." } }, "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" @@ -63,6 +66,150 @@ } } ] + }, + { + "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": 164, + "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 e43ae2c84..dc9f946e3 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 @@ -13,7 +13,7 @@ "ruleId": "TST0001", "level": "error", "message": { - "text": "Some testing occurred." + "text": "Bad result - context region is present without region." }, "locations": [ { @@ -28,6 +28,154 @@ } } ] + }, + { + "ruleId": "TST0002", + "level": "error", + "message": { + "text": "Bad result - partially overlapping region and contextregion (line/column based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "startLine": 1, + "startColumn": 5, + "endLine": 10, + "endColumn": 5 + }, + "contextRegion": { + "startLine": 1, + "startColumn": 1, + "endLine": 10, + "endColumn":15 + } + } + } + ] + }, + { + "ruleId": "TST0003", + "level": "error", + "message": { + "text": "Bad result - non-overlapping region and contextregion (line/column based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "startLine": 5, + "endLine": 10 + }, + "contextRegion": { + "startLine": 2, + "endLine": 4 + } + } + } + ] + }, + { + "ruleId": "TST0004", + "level": "error", + "message": { + "text": "Bad result - fully overlapping region and contextregion (line/column based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "startLine": 2, + "endLine": 4 + }, + "contextRegion": { + "startLine": 2, + "endLine": 4 + } + } + } + ] + }, + { + "ruleId": "TST0005", + "level": "error", + "message": { + "text": "Bad result - text contextregion smaller than region (offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "charOffset": 15, + "charLength": 30 + }, + "contextRegion": { + "charOffset": 15, + "charLength": 20 + } + } + } + ] + }, + { + "ruleId": "TST0006", + "level": "error", + "message": { + "text": "Bad result - non overlapping text region and contextregion (offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "charOffset": 1, + "charLength": 10 + }, + "contextRegion": { + "charOffset": 15, + "charLength": 20 + } + } + } + ] + }, + { + "ruleId": "TST0007", + "level": "error", + "message": { + "text": "Bad result - fully overlapping binary region and contextregion (offset based)." + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/test.c" + }, + "region": { + "byteOffset": 10, + "byteLength": 100 + }, + "contextRegion": { + "byteOffset": 10, + "byteLength": 100 + } + } + } + ] } ], "columnKind": "utf16CodeUnits" From 8538964075c024d7f0384df8dd8e8d4a5e780e5f Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Mon, 22 Jun 2020 17:14:38 -0700 Subject: [PATCH 02/11] rc++ --- .../Rules/RuleResources.Designer.cs | 2 +- src/Sarif.Multitool/Rules/RuleResources.resx | 2 +- ...sicalLocationPropertiesMustBeConsistent.cs | 51 +++-- ...onPropertiesMustBeConsistent_Invalid.sarif | 190 +++++++++++++++--- 4 files changed, 204 insertions(+), 41 deletions(-) diff --git a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs index 189a498fc..55e3b7bee 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs +++ b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs @@ -199,7 +199,7 @@ internal static string SARIF1007_RegionPropertiesMustBeConsistent_FullDescriptio } /// - /// 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. If the tool simply reversed 'region', then fix it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must n [rest of string was truncated]";. + /// 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. If the tool simply reversed 'region' and 'contextRegion', then fix it so it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by [rest of string was truncated]";. /// internal static string SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text { get { diff --git a/src/Sarif.Multitool/Rules/RuleResources.resx b/src/Sarif.Multitool/Rules/RuleResources.resx index 7f4db6630..2d85f60a1 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.resx +++ b/src/Sarif.Multitool/Rules/RuleResources.resx @@ -211,6 +211,6 @@ If the SARIF validator reports that 'contextRegion' is present but 'region' is a 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. If the tool simply reversed 'region', then fix it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it. + {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. If the tool simply reversed 'region' and 'contextRegion', then fix it so it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it. \ No newline at end of file diff --git a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs index 3d931551b..d92990bc2 100644 --- a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs +++ b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs @@ -57,7 +57,6 @@ private static bool IsRegionProperSuperset(Region superRegion, Region subRegion) IsOffsetBasedTextRegion(subRegion) && !IsOffsetBasedTextRegionProperSupetSet(superRegion, subRegion)) { - return false; } @@ -72,45 +71,49 @@ private static bool IsRegionProperSuperset(Region superRegion, Region subRegion) // and it is not possible to judge validity without looking at the actual content. // It is a potential false negative. return true; - } - private static bool IsBinaryRegionProperSuperset(Region superRegion, Region subRegion) + private static bool IsLineColumnBasedTextRegionProperSuperset(Region superRegion, Region subRegion) { - if (superRegion.ByteOffset > subRegion.ByteOffset) + if (superRegion.StartLine > subRegion.StartLine || superRegion.EndLine < subRegion.EndLine) { return false; } - if (superRegion.ByteOffset == subRegion.ByteOffset && superRegion.ByteLength <= subRegion.ByteLength) + if (superRegion.StartLine == subRegion.StartLine && superRegion.StartColumn > subRegion.StartColumn) { return false; } - return true; - } + if (superRegion.EndLine == subRegion.EndLine && superRegion.EndColumn < subRegion.EndColumn) + { + return false; + } - private static bool IsLineColumnBasedTextRegionProperSuperset(Region superRegion, Region subRegion) - { - if (superRegion.StartLine > subRegion.StartLine || superRegion.EndLine < subRegion.EndLine) + if (superRegion.StartLine == subRegion.StartLine && + superRegion.EndLine == subRegion.EndLine && + superRegion.StartColumn == subRegion.StartColumn && + superRegion.EndColumn == subRegion.EndColumn) { return false; } - if (superRegion.StartLine == subRegion.StartLine && superRegion.StartColumn < subRegion.StartColumn) + return true; + } + + private static bool IsBinaryRegionProperSuperset(Region superRegion, Region subRegion) + { + if (superRegion.ByteOffset > subRegion.ByteOffset) { return false; } - if (superRegion.EndLine == subRegion.EndLine && superRegion.EndColumn > subRegion.EndColumn) + if (GetByteEndOffset(superRegion) < GetByteEndOffset(subRegion)) { return false; } - if (superRegion.StartLine == subRegion.StartLine && - superRegion.EndLine == subRegion.EndLine && - superRegion.StartColumn == subRegion.StartColumn && - superRegion.EndColumn == subRegion.EndColumn) + if (superRegion.ByteOffset == subRegion.ByteOffset && superRegion.ByteLength <= subRegion.ByteLength) { return false; } @@ -125,10 +128,16 @@ private static bool IsOffsetBasedTextRegionProperSupetSet(Region superRegion, Re return false; } + if (GetCharEndOffset(superRegion) < GetCharEndOffset(subRegion)) + { + return false; + } + if (superRegion.CharOffset == subRegion.CharOffset && superRegion.CharLength <= subRegion.CharLength) { return false; } + return true; } @@ -146,5 +155,15 @@ private static bool IsBinaryRegion(Region region) { return region.ByteOffset >= 0; } + + 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/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 dc9f946e3..f144d38ef 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 @@ -30,10 +30,10 @@ ] }, { - "ruleId": "TST0002", + "ruleId": "TST0001", "level": "error", "message": { - "text": "Bad result - partially overlapping region and contextregion (line/column based)." + "text": "Bad result - partially overlapping region and contextregion (text and line/column based)." }, "locations": [ { @@ -45,23 +45,23 @@ "startLine": 1, "startColumn": 5, "endLine": 10, - "endColumn": 5 + "endColumn": 15 }, "contextRegion": { "startLine": 1, "startColumn": 1, "endLine": 10, - "endColumn":15 + "endColumn": 5 } } } ] }, { - "ruleId": "TST0003", + "ruleId": "TST0001", "level": "error", "message": { - "text": "Bad result - non-overlapping region and contextregion (line/column based)." + "text": "Bad result - non-overlapping region and contextregion (text and line/column based)." }, "locations": [ { @@ -82,10 +82,10 @@ ] }, { - "ruleId": "TST0004", + "ruleId": "TST0001", "level": "error", "message": { - "text": "Bad result - fully overlapping region and contextregion (line/column based)." + "text": "Bad result - identical region and contextregion (text and line/column based)." }, "locations": [ { @@ -106,10 +106,10 @@ ] }, { - "ruleId": "TST0005", + "ruleId": "TST0001", "level": "error", "message": { - "text": "Bad result - text contextregion smaller than region (offset based)." + "text": "Bad result - contextregion smaller than region (text and line/column based)." }, "locations": [ { @@ -118,22 +118,70 @@ "uri": "src/test.c" }, "region": { - "charOffset": 15, - "charLength": 30 + "startLine": 2, + "endLine": 5 }, "contextRegion": { - "charOffset": 15, - "charLength": 20 + "startLine": 3, + "endLine": 4 + } + } + } + ] + }, + { + "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": "TST0006", + "ruleId": "TST0001", "level": "error", "message": { - "text": "Bad result - non overlapping text region and contextregion (offset based)." + "text": "Bad result - identical region and contextregion (text and offset based)." }, "locations": [ { @@ -142,22 +190,94 @@ "uri": "src/test.c" }, "region": { - "charOffset": 1, - "charLength": 10 + "charOffset": 10, + "charLength": 25 }, "contextRegion": { - "charOffset": 15, + "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 - 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": "TST0007", + "ruleId": "TST0001", "level": "error", "message": { - "text": "Bad result - fully overlapping binary region and contextregion (offset based)." + "text": "Bad result - identical region and contextregion (binary offset based)." }, "locations": [ { @@ -167,11 +287,35 @@ }, "region": { "byteOffset": 10, - "byteLength": 100 + "byteLength": 25 }, "contextRegion": { "byteOffset": 10, - "byteLength": 100 + "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 } } } From de68307fab0982a95f35d5d025cf47e5d0f59e77 Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Mon, 22 Jun 2020 17:16:09 -0700 Subject: [PATCH 03/11] regen test output --- ...onPropertiesMustBeConsistent_Invalid.sarif | 146 +++++++++++++++++- 1 file changed, 145 insertions(+), 1 deletion(-) 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 1a017757e..58fe024c9 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 @@ -21,7 +21,7 @@ "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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does." }, "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. If the tool simply reversed 'region', then fix it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it." + "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. If the tool simply reversed 'region' and 'contextRegion', then fix it so it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it." } }, "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" @@ -210,6 +210,150 @@ } } ] + }, + { + "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": 308, + "startColumn": 35 + } + } + } + ] } ], "columnKind": "utf16CodeUnits" From c58f30e2abc3c00dacc1e5fd4a327c70fb3f7d6b Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Tue, 23 Jun 2020 17:45:02 -0700 Subject: [PATCH 04/11] updated with defaults computations and related tests --- ...sicalLocationPropertiesMustBeConsistent.cs | 124 +----------- src/Sarif/Core/Region.cs | 179 ++++++++++++++++-- ...onPropertiesMustBeConsistent_Invalid.sarif | 100 +++++++++- ...onPropertiesMustBeConsistent_Invalid.sarif | 94 +++++++++ src/Test.UnitTests.Sarif/Core/RegionTests.cs | 113 +++++++++++ 5 files changed, 474 insertions(+), 136 deletions(-) create mode 100644 src/Test.UnitTests.Sarif/Core/RegionTests.cs diff --git a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs index d92990bc2..062eeeb56 100644 --- a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs +++ b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs @@ -38,132 +38,10 @@ protected override void Analyze(PhysicalLocation physicalLocation, string physic return; } - if (!IsRegionProperSuperset(physicalLocation.ContextRegion, physicalLocation.Region)) + if (!Region.IsProperSuperset(physicalLocation.ContextRegion, physicalLocation.Region)) { LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text)); } } - - private static bool IsRegionProperSuperset(Region superRegion, Region subRegion) - { - if (IsLineColumnBasedTextRegion(superRegion) && - IsLineColumnBasedTextRegion(subRegion) && - !IsLineColumnBasedTextRegionProperSuperset(superRegion, subRegion)) - { - return false; - } - - if (IsOffsetBasedTextRegion(superRegion) && - IsOffsetBasedTextRegion(subRegion) && - !IsOffsetBasedTextRegionProperSupetSet(superRegion, subRegion)) - { - return false; - } - - if (IsBinaryRegion(superRegion) && - IsBinaryRegion(subRegion) && - !IsBinaryRegionProperSuperset(superRegion, 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 static bool IsLineColumnBasedTextRegionProperSuperset(Region superRegion, Region subRegion) - { - if (superRegion.StartLine > subRegion.StartLine || superRegion.EndLine < subRegion.EndLine) - { - return false; - } - - if (superRegion.StartLine == subRegion.StartLine && superRegion.StartColumn > subRegion.StartColumn) - { - return false; - } - - if (superRegion.EndLine == subRegion.EndLine && superRegion.EndColumn < subRegion.EndColumn) - { - return false; - } - - if (superRegion.StartLine == subRegion.StartLine && - superRegion.EndLine == subRegion.EndLine && - superRegion.StartColumn == subRegion.StartColumn && - superRegion.EndColumn == subRegion.EndColumn) - { - return false; - } - - return true; - } - - private static bool IsBinaryRegionProperSuperset(Region superRegion, Region subRegion) - { - if (superRegion.ByteOffset > subRegion.ByteOffset) - { - return false; - } - - if (GetByteEndOffset(superRegion) < GetByteEndOffset(subRegion)) - { - return false; - } - - if (superRegion.ByteOffset == subRegion.ByteOffset && superRegion.ByteLength <= subRegion.ByteLength) - { - return false; - } - - return true; - } - - private static bool IsOffsetBasedTextRegionProperSupetSet(Region superRegion, Region subRegion) - { - if (superRegion.CharOffset > subRegion.CharOffset) - { - return false; - } - - if (GetCharEndOffset(superRegion) < GetCharEndOffset(subRegion)) - { - return false; - } - - if (superRegion.CharOffset == subRegion.CharOffset && superRegion.CharLength <= subRegion.CharLength) - { - return false; - } - - return true; - } - - private static bool IsLineColumnBasedTextRegion(Region region) - { - return region.StartLine >= 1; - } - - private static bool IsOffsetBasedTextRegion(Region region) - { - return region.CharOffset > 0; - } - - private static bool IsBinaryRegion(Region region) - { - return region.ByteOffset >= 0; - } - - 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/Core/Region.cs b/src/Sarif/Core/Region.cs index e67219992..c5e6eedb0 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 static bool IsProperSuperset(Region superRegion, Region subRegion) + { + superRegion.PopulateDefaults(); + subRegion.PopulateDefaults(); + + if (superRegion.IsLineColumnBasedTextRegion && + subRegion.IsLineColumnBasedTextRegion && + !IsLineColumnBasedTextRegionProperSuperset(superRegion, subRegion)) + { + return false; + } + + if (superRegion.IsOffsetBasedTextRegion && + subRegion.IsOffsetBasedTextRegion && + !IsOffsetBasedTextRegionProperSupetSet(superRegion, subRegion)) + { + return false; + } + + if (superRegion.IsBinaryRegion && + subRegion.IsBinaryRegion && + !IsBinaryRegionProperSuperset(superRegion, 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 static bool IsLineColumnBasedTextRegionProperSuperset(Region superRegion, Region subRegion) + { + if (superRegion.StartLine > subRegion.StartLine || superRegion.EndLine < subRegion.EndLine) + { + return false; + } + + if (superRegion.StartLine == subRegion.StartLine && superRegion.StartColumn > subRegion.StartColumn) + { + return false; + } + + if (superRegion.EndLine == subRegion.EndLine && superRegion.EndColumn < subRegion.EndColumn) + { + return false; + } + + if (superRegion.StartLine == subRegion.StartLine && + superRegion.EndLine == subRegion.EndLine && + superRegion.StartColumn == subRegion.StartColumn && + superRegion.EndColumn == subRegion.EndColumn) + { + return false; + } + + return true; + } + + private static bool IsBinaryRegionProperSuperset(Region superRegion, Region subRegion) + { + if (superRegion.ByteOffset > subRegion.ByteOffset) + { + return false; + } + + if (GetByteEndOffset(superRegion) < GetByteEndOffset(subRegion)) + { + return false; + } + + if (superRegion.ByteOffset == subRegion.ByteOffset && superRegion.ByteLength <= subRegion.ByteLength) + { + return false; + } + + return true; + } + + private static bool IsOffsetBasedTextRegionProperSupetSet(Region superRegion, Region subRegion) + { + if (superRegion.CharOffset > subRegion.CharOffset) + { + return false; + } + + if (GetCharEndOffset(superRegion) < GetCharEndOffset(subRegion)) + { + return false; + } + + if (superRegion.CharOffset == subRegion.CharOffset && superRegion.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/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 58fe024c9..916055e80 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 @@ -204,7 +204,7 @@ "index": 0 }, "region": { - "startLine": 164, + "startLine": 163, "startColumn": 35 } } @@ -348,7 +348,103 @@ "index": 0 }, "region": { - "startLine": 308, + "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 } } 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 f144d38ef..d04aa4583 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 @@ -129,6 +129,54 @@ } ] }, + { + "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", @@ -225,6 +273,29 @@ } ] }, + { + "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", @@ -320,6 +391,29 @@ } } ] + }, + { + "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..51fccb252 --- /dev/null +++ b/src/Test.UnitTests.Sarif/Core/RegionTests.cs @@ -0,0 +1,113 @@ +// 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(); + + } + } +} \ No newline at end of file From 4e7aee3d309a02be341295bd2c5fcbd04960eb8f Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Wed, 24 Jun 2020 08:50:39 -0700 Subject: [PATCH 05/11] pushing changes --- ...sicalLocationPropertiesMustBeConsistent.cs | 2 +- src/Sarif/Core/Region.cs | 50 +++++++++---------- src/Test.UnitTests.Sarif/Core/RegionTests.cs | 15 ++++++ 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs index 062eeeb56..5f64bf213 100644 --- a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs +++ b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs @@ -38,7 +38,7 @@ protected override void Analyze(PhysicalLocation physicalLocation, string physic return; } - if (!Region.IsProperSuperset(physicalLocation.ContextRegion, physicalLocation.Region)) + if (!physicalLocation.ContextRegion.IsProperSupersetOf(physicalLocation.Region)) { LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text)); } diff --git a/src/Sarif/Core/Region.cs b/src/Sarif/Core/Region.cs index c5e6eedb0..744eb9c76 100644 --- a/src/Sarif/Core/Region.cs +++ b/src/Sarif/Core/Region.cs @@ -12,7 +12,7 @@ public partial class Region public bool IsLineColumnBasedTextRegion => this.StartLine >= 1; - public bool IsOffsetBasedTextRegion => this.CharOffset > 0; + public bool IsOffsetBasedTextRegion => this.CharOffset >= 0; public override string ToString() { @@ -71,28 +71,28 @@ private void PopulateBinaryDefaults() } } - public static bool IsProperSuperset(Region superRegion, Region subRegion) + public bool IsProperSupersetOf(Region subRegion) { - superRegion.PopulateDefaults(); + this.PopulateDefaults(); subRegion.PopulateDefaults(); - if (superRegion.IsLineColumnBasedTextRegion && + if (this.IsLineColumnBasedTextRegion && subRegion.IsLineColumnBasedTextRegion && - !IsLineColumnBasedTextRegionProperSuperset(superRegion, subRegion)) + !IsLineColumnBasedTextRegionProperSupersetOf(subRegion)) { return false; } - if (superRegion.IsOffsetBasedTextRegion && + if (this.IsOffsetBasedTextRegion && subRegion.IsOffsetBasedTextRegion && - !IsOffsetBasedTextRegionProperSupetSet(superRegion, subRegion)) + !IsOffsetBasedTextRegionProperSupetSetOf(subRegion)) { return false; } - if (superRegion.IsBinaryRegion && + if (this.IsBinaryRegion && subRegion.IsBinaryRegion && - !IsBinaryRegionProperSuperset(superRegion, subRegion)) + !IsBinaryRegionProperSupersetOf(subRegion)) { return false; } @@ -103,27 +103,27 @@ public static bool IsProperSuperset(Region superRegion, Region subRegion) return true; } - private static bool IsLineColumnBasedTextRegionProperSuperset(Region superRegion, Region subRegion) + private bool IsLineColumnBasedTextRegionProperSupersetOf(Region subRegion) { - if (superRegion.StartLine > subRegion.StartLine || superRegion.EndLine < subRegion.EndLine) + if (this.StartLine > subRegion.StartLine || this.EndLine < subRegion.EndLine) { return false; } - if (superRegion.StartLine == subRegion.StartLine && superRegion.StartColumn > subRegion.StartColumn) + if (this.StartLine == subRegion.StartLine && this.StartColumn > subRegion.StartColumn) { return false; } - if (superRegion.EndLine == subRegion.EndLine && superRegion.EndColumn < subRegion.EndColumn) + if (this.EndLine == subRegion.EndLine && this.EndColumn < subRegion.EndColumn) { return false; } - if (superRegion.StartLine == subRegion.StartLine && - superRegion.EndLine == subRegion.EndLine && - superRegion.StartColumn == subRegion.StartColumn && - superRegion.EndColumn == subRegion.EndColumn) + if (this.StartLine == subRegion.StartLine && + this.EndLine == subRegion.EndLine && + this.StartColumn == subRegion.StartColumn && + this.EndColumn == subRegion.EndColumn) { return false; } @@ -131,19 +131,19 @@ private static bool IsLineColumnBasedTextRegionProperSuperset(Region superRegion return true; } - private static bool IsBinaryRegionProperSuperset(Region superRegion, Region subRegion) + private bool IsBinaryRegionProperSupersetOf(Region subRegion) { - if (superRegion.ByteOffset > subRegion.ByteOffset) + if (this.ByteOffset > subRegion.ByteOffset) { return false; } - if (GetByteEndOffset(superRegion) < GetByteEndOffset(subRegion)) + if (GetByteEndOffset(this) < GetByteEndOffset(subRegion)) { return false; } - if (superRegion.ByteOffset == subRegion.ByteOffset && superRegion.ByteLength <= subRegion.ByteLength) + if (this.ByteOffset == subRegion.ByteOffset && this.ByteLength <= subRegion.ByteLength) { return false; } @@ -151,19 +151,19 @@ private static bool IsBinaryRegionProperSuperset(Region superRegion, Region subR return true; } - private static bool IsOffsetBasedTextRegionProperSupetSet(Region superRegion, Region subRegion) + private bool IsOffsetBasedTextRegionProperSupetSetOf(Region subRegion) { - if (superRegion.CharOffset > subRegion.CharOffset) + if (this.CharOffset > subRegion.CharOffset) { return false; } - if (GetCharEndOffset(superRegion) < GetCharEndOffset(subRegion)) + if (GetCharEndOffset(this) < GetCharEndOffset(subRegion)) { return false; } - if (superRegion.CharOffset == subRegion.CharOffset && superRegion.CharLength <= subRegion.CharLength) + if (this.CharOffset == subRegion.CharOffset && this.CharLength <= subRegion.CharLength) { return false; } diff --git a/src/Test.UnitTests.Sarif/Core/RegionTests.cs b/src/Test.UnitTests.Sarif/Core/RegionTests.cs index 51fccb252..ef4aa1467 100644 --- a/src/Test.UnitTests.Sarif/Core/RegionTests.cs +++ b/src/Test.UnitTests.Sarif/Core/RegionTests.cs @@ -107,7 +107,22 @@ public void Region_IsOffsetBasedTextRegion_ComputesCorrectly() 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 From 2d2c9a93061960bcc1ee4af004b973cd8674f48a Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Wed, 24 Jun 2020 09:16:43 -0700 Subject: [PATCH 06/11] changing names for testcases --- src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs index 8203e0d70..5a77a4b4b 100644 --- a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs +++ b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs @@ -55,7 +55,7 @@ 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, @@ -67,7 +67,7 @@ public TestCaseData(Region inputRegion, Region outputRegion) 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 }, @@ -273,14 +273,16 @@ public TestCaseData(Region inputRegion, Region outputRegion) new ReadOnlyCollection(new TestCaseData[] { // Insertion point at beginning of binary file - new TestCaseData(outputRegion : s_Insertion_Beginning_Of_Binary_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 // 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 }), + // TODO: binary file test add + new TestCaseData(outputRegion : s_Insertion_End_Of_File, inputRegion: new Region() { CharOffset = 20 }), From 80b4f18f310fed14b0e88443a5785519ecb9bd09 Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Wed, 24 Jun 2020 09:58:21 -0700 Subject: [PATCH 07/11] following convention in the validation rules --- ...sicalLocationPropertiesMustBeConsistent.cs | 30 ++++++++++++------- src/Sarif/FileRegionsCache.cs | 2 +- .../FileRegionsCacheTests.cs | 8 ++--- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs index 5f64bf213..f5ebb84e6 100644 --- a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs +++ b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs @@ -1,30 +1,36 @@ // 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.Collections.Generic; -using System.Linq.Expressions; -using System.Runtime.InteropServices.WindowsRuntime; namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules { public class PhysicalLocationPropertiesMustBeConsistent : SarifValidationSkimmerBase { - public override MultiformatMessageString FullDescription => new MultiformatMessageString - { - Text = RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text - }; - - public override FailureLevel DefaultLevel => FailureLevel.Error; - + /// + /// SARIF1008 + /// public override string Id => RuleId.PhysicalLocationPropertiesMustBeConsistent; + /// + /// 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 SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply neglected to populate 'region'. 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 + /// + public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text }; + protected override IEnumerable MessageResourceNames => new string[] { nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text), nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text) }; + public override FailureLevel DefaultLevel => FailureLevel.Error; + protected override void Analyze(PhysicalLocation physicalLocation, string physicalLocationPointer) { if (physicalLocation.ContextRegion == null) @@ -32,14 +38,18 @@ protected override void Analyze(PhysicalLocation physicalLocation, string physic 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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does. 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)) { + // {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. If the tool simply reversed 'region' and 'contextRegion', then fix it so it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it. LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text)); } } 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.UnitTests.Sarif/FileRegionsCacheTests.cs b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs index 5a77a4b4b..8fc5e97b8 100644 --- a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs +++ b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs @@ -59,10 +59,10 @@ public TestCaseData(Region inputRegion, Region outputRegion) new Region() { Snippet = null, - StartLine = 0, - StartColumn = 0, - EndLine = 0, - EndColumn = 0, + StartLine = 1, + StartColumn = 1, + EndLine = 1, + EndColumn = 1, CharOffset = 0, CharLength = 0 }; From 186aa056243c39cb0baaacf052dca86fb3acef5e Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Wed, 24 Jun 2020 10:18:46 -0700 Subject: [PATCH 08/11] fixing test case output --- src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs index 8fc5e97b8..8df2ce5f5 100644 --- a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs +++ b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs @@ -58,7 +58,7 @@ public TestCaseData(Region inputRegion, Region outputRegion) private readonly static Region s_Insertion_Beginning_Of_OffsetBased_Text_File = new Region() { - Snippet = null, + Snippet = new ArtifactContent() { Text = INSERTION_POINT }, StartLine = 1, StartColumn = 1, EndLine = 1, From d359e9908ecb6abf5d5418d9486e5c4fade54761 Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Wed, 24 Jun 2020 10:52:43 -0700 Subject: [PATCH 09/11] final fixes (hopefully) :/ --- ...sicalLocationPropertiesMustBeConsistent.cs | 50 +++++++++++++++---- .../FileRegionsCacheTests.cs | 22 ++++++-- 2 files changed, 58 insertions(+), 14 deletions(-) diff --git a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs index f5ebb84e6..b59114f8d 100644 --- a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs +++ b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs @@ -13,21 +13,38 @@ public class PhysicalLocationPropertiesMustBeConsistent : SarifValidationSkimmer public override string Id => RuleId.PhysicalLocationPropertiesMustBeConsistent; /// - /// 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. + /// 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. + /// '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. + /// '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 SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply neglected to populate 'region'. 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 + /// If the SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's + /// possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply + /// neglected to populate 'region'. 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 /// public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text }; - 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), + nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text) + }; public override FailureLevel DefaultLevel => FailureLevel.Error; @@ -41,7 +58,12 @@ protected override void Analyze(PhysicalLocation physicalLocation, string physic // 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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does. + // {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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so + // that it populates only the 'region'. If the tool simply neglected to populate 'region', + // then fix it so that it does. LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text)); return; } @@ -49,7 +71,13 @@ protected override void Analyze(PhysicalLocation physicalLocation, string physic // ContextRegionMustBeProperSupersetOfRegion: 'contextRegion' must be a proper superset of 'region'. if (!physicalLocation.ContextRegion.IsProperSupersetOf(physicalLocation.Region)) { - // {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. If the tool simply reversed 'region' and 'contextRegion', then fix it so it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it. + // {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. If the tool simply reversed 'region' + // and 'contextRegion', then fix it so it puts the correct values in the correct + // properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is + // unnecessary, and (by the spec) the tool must not populate it. LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text)); } } diff --git a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs index 8df2ce5f5..ab568603e 100644 --- a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs +++ b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs @@ -79,6 +79,20 @@ public TestCaseData(Region inputRegion, Region outputRegion) CharLength = 0 }; + private readonly static Region s_Insertion_Beginning_Of_OffsetBased_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,16 +286,18 @@ public TestCaseData(Region inputRegion, Region outputRegion) private static readonly ReadOnlyCollection s_specExampleTestCases = new ReadOnlyCollection(new TestCaseData[] { - // Insertion point at beginning of binary file + // Insertion point at beginning of 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 line/column based text file, can only // be denoted by use of startLine new TestCaseData(outputRegion : s_Insertion_Beginning_Of_LineColumnBased_Text_File, inputRegion: new Region() { StartLine = 1, StartColumn = 1, EndColumn = 1, CharOffset = 0 }), - // TODO: binary file test add + // Insertion point at beginning of offset based text file + new TestCaseData(outputRegion : s_Insertion_Beginning_Of_OffsetBased_Binary_File, + inputRegion: new Region() { ByteOffset = 0}), new TestCaseData(outputRegion : s_Insertion_End_Of_File, inputRegion: new Region() { CharOffset = 20 }), From 5d1284f0760a889d780e997a3226f7573b5aea23 Mon Sep 17 00:00:00 2001 From: Harleen Kaur Kohli Date: Wed, 24 Jun 2020 10:57:13 -0700 Subject: [PATCH 10/11] :( --- ...F1008.PhysicalLocationPropertiesMustBeConsistent.cs | 8 ++++++-- src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs | 10 +++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs index b59114f8d..31e10f57d 100644 --- a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs +++ b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs @@ -64,7 +64,9 @@ protected override void Analyze(PhysicalLocation physicalLocation, string physic // If the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so // that it populates only the 'region'. If the tool simply neglected to populate 'region', // then fix it so that it does. - LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text)); + LogResult( + physicalLocationPointer, + nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text)); return; } @@ -78,7 +80,9 @@ protected override void Analyze(PhysicalLocation physicalLocation, string physic // and 'contextRegion', then fix it so it puts the correct values in the correct // properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is // unnecessary, and (by the spec) the tool must not populate it. - LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text)); + LogResult( + physicalLocationPointer, + nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text)); } } } diff --git a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs index ab568603e..2fb0a9e42 100644 --- a/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs +++ b/src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs @@ -79,7 +79,7 @@ public TestCaseData(Region inputRegion, Region outputRegion) CharLength = 0 }; - private readonly static Region s_Insertion_Beginning_Of_OffsetBased_Binary_File = + private readonly static Region s_Insertion_Beginning_Of_Binary_File = new Region() { Snippet = null, @@ -286,17 +286,17 @@ public TestCaseData(Region inputRegion, Region outputRegion) private static readonly ReadOnlyCollection s_specExampleTestCases = new ReadOnlyCollection(new TestCaseData[] { - // Insertion point at beginning of offset based text 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 line/column based 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_LineColumnBased_Text_File, inputRegion: new Region() { StartLine = 1, StartColumn = 1, EndColumn = 1, CharOffset = 0 }), - // Insertion point at beginning of offset based text file - new TestCaseData(outputRegion : s_Insertion_Beginning_Of_OffsetBased_Binary_File, + // 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, From 800861c03abd06d82d42a6d40a5ac71790c9de0f Mon Sep 17 00:00:00 2001 From: Larry Golding Date: Wed, 24 Jun 2020 11:22:18 -0700 Subject: [PATCH 11/11] Update user-facing strings. --- .../Rules/RuleResources.Designer.cs | 10 +-- src/Sarif.Multitool/Rules/RuleResources.resx | 10 +-- ...sicalLocationPropertiesMustBeConsistent.cs | 63 +++++++++---------- ...onPropertiesMustBeConsistent_Invalid.sarif | 8 +-- 4 files changed, 47 insertions(+), 44 deletions(-) diff --git a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs index 55e3b7bee..a6425577a 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs +++ b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs @@ -199,7 +199,7 @@ internal static string SARIF1007_RegionPropertiesMustBeConsistent_FullDescriptio } /// - /// 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. If the tool simply reversed 'region' and 'contextRegion', then fix it so it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by [rest of string was truncated]";. + /// 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 { @@ -209,7 +209,7 @@ internal static string SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Erro } /// - /// 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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does.. + /// 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 { @@ -219,9 +219,11 @@ internal static string SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Erro } /// - /// Looks up a localized string similar to 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. + /// Looks up a localized string similar to Ensure consistency among the properties of a 'physicalLocation' object. /// - ///'region' allows both users and tools to distinguish similar results within the same arti [rest of string was truncated]";. + ///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 2d85f60a1..5258c9746 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.resx +++ b/src/Sarif.Multitool/Rules/RuleResources.resx @@ -154,16 +154,18 @@ Certain URIs are required to be absolute. - 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. + 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 SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply neglected to populate 'region'. 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'. +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. 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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does. + {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. @@ -211,6 +213,6 @@ If the SARIF validator reports that 'contextRegion' is present but 'region' is a 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. If the tool simply reversed 'region' and 'contextRegion', then fix it so it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it. + {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'. \ No newline at end of file diff --git a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs index 31e10f57d..41a6ef1f0 100644 --- a/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs +++ b/src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs @@ -13,31 +13,32 @@ public class PhysicalLocationPropertiesMustBeConsistent : SarifValidationSkimmer public override string Id => RuleId.PhysicalLocationPropertiesMustBeConsistent; /// + /// 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. + /// 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. /// - /// '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 + /// '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 SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's - /// possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply - /// neglected to populate 'region'. 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 + /// + /// 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 }; @@ -58,12 +59,11 @@ protected override void Analyze(PhysicalLocation physicalLocation, string physic // 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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so - // that it populates only the 'region'. If the tool simply neglected to populate 'region', - // then fix it so that it does. + // {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)); @@ -76,10 +76,9 @@ protected override void Analyze(PhysicalLocation physicalLocation, string physic // {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. If the tool simply reversed 'region' - // and 'contextRegion', then fix it so it puts the correct values in the correct - // properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is - // unnecessary, and (by the spec) the tool must not populate it. + // '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/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 916055e80..0ec0822e0 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,17 +11,17 @@ "id": "SARIF1008", "name": "PhysicalLocationPropertiesMustBeConsistent", "shortDescription": { - "text": "A SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'." + "text": "Ensure consistency among the properties of a 'physicalLocation' object." }, "fullDescription": { - "text": "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.\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 SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply neglected to populate 'region'. 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'." + "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. 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 the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does." + "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. If the tool simply reversed 'region' and 'contextRegion', then fix it so it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it." + "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"