sarif1008: ContextRegionMustBeProperSupersetOfRegion check

src/Sarif.Multitool/Rules/RuleResources.resx

@@ -154,10 +154,16 @@
     <value>Certain URIs are required to be absolute.</value>
   </data>
   <data name="SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text" xml:space="preserve">
-    <value>If the "contextRegion" property is present, the "region" property must also be present.</value>
+    <value>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'.</value>
   </data>
   <data name="SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text" xml:space="preserve">
-    <value>{0}: This "physicalLocation" object contains a "contextRegion" property, but it does not contain a "region" property.</value>
+    <value>{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.</value>
   </data>
   <data name="SARIF1009_IndexPropertiesMustBeConsistentWithArrays_Error_TargetArrayMustBeLongEnough_Text" xml:space="preserve">
     <value>{0}: This "{1}" object contains a property "{2}" with value {3}, but "{4}" has fewer than {5} elements.</value>
@@ -204,4 +210,7 @@
   <data name="SARIF1007_RegionPropertiesMustBeConsistent_FullDescription_Text" xml:space="preserve">
     <value>Placeholder_SARIF1007_RegionPropertiesMustBeConsistent_FullDescription_Text</value>
   </data>
+  <data name="SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text" xml:space="preserve">
+    <value>{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.</value>
+  </data>
 </root>

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<string> 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;
         }
     }
 }