Skip to content

Commit

Permalink
Fix #1594: FxCopConverter produces invalid SARIF. (#1639)
Browse files Browse the repository at this point in the history
FxCop XML `Message` elements whose `Status` attribute is `"Active"` contain an `Issue` sub-element whose inner text provides the SARIF `result.message.text`. But `Message` elements whose `Status` attribute is `ExcludedInSource` (and possibly other values; I don't know) do not contain an `Issue` sub-element. For those FxCop `Message` elements, the `FxCopConverter` produces a SARIF `result` object whose `message` property is an empty object `{}`. This is invalid SARIF because a `message` object requires a `text` property.

Each FxCop `Message` element contains a `CheckId` attribute whose value points to a `Rule` element, which in turn contains a `Resolution` element from which `result.message.text` might seemingly be constructed. But in general, the FxCop XML file does not contain enough information to reconstruct the message. For example, this `Message`:

```
    <Member Name="#RemoveIf`1(!!0[]&amp;,System.Func`2&lt;!!0,System.Boolean&gt;)"
            Kind="Method" Static="True" Accessibility="Public" ExternallyVisible="True">
      <Messages>
        <Message Id="0#" TypeName="DoNotPassTypesByReference" Category="Microsoft.Design"
                         CheckId="CA1045" Status="ExcludedInSource"
                         Created="2018-08-23 13:07:11Z" FixCategory="Breaking"
                         LastSeen="0001-01-01 00:00:00Z" />
        ...
      </Messages>
    </Member>
```

points to this `Rule`:

```
    <Rule TypeName="DoNotPassTypesByReference" Category="Microsoft.Design" CheckId="CA1045">
      <Name>Do not pass types by reference</Name>
      <Description>...</Description>
      <Resolution Name="Default">Consider a design that does not require that {0} be a reference parameter.</Resolution>
      ...
    </Rule>
```

The sequence `{0}` should be replaced by the name of the first parameter to `RemoveIf`, which happens to be `array`. But "`array`" does not appear in the XML file. And even if all the necessary information were present, there would be no way for the converter to know which piece of information from the XML file would go into which replacement sequence in the `Resolution` string. Finally, some `Rule`s have more than one `Resolution` string, and the converter has no way to know which one to use.

That's all a long way of saying that there's no way to synthesize a valid `result.message.text` string for FxCop `Message`s that don't provide one. And the converter can't just omit `result.message`, because it's required. The best we can do is to provide a canned message: `"FxCop does not provide messages for suppressed results."`

Also:

- Upgrade SarifSdkTest.xml.sarif (which had been generated by a pre-release version of the SDK) to the final version of the SARIF 2.1.0 format. This is necessary because if this file is down-level, the `FxCopConverter`'s call to the `PrereleaseCompatibilityTransformer` produces valid SARIF, masking the bug in the converter.
  • Loading branch information
Larry Golding committed Aug 12, 2019
1 parent feb1953 commit 6e194ec
Show file tree
Hide file tree
Showing 6 changed files with 33,302 additions and 36,992 deletions.
3 changes: 3 additions & 0 deletions src/ReleaseHistory.md
Original file line number Diff line number Diff line change
Expand Up @@ -443,3 +443,6 @@
* BUGFIX: FxCop converter produced logicalLocation.index but did not produce the run.logicalLocations array. https://github.com/microsoft/sarif-sdk/issues/1571
* BUGFIX: Include Sarif.WorkItemFiling.dll in the Sarif.Multitool NuGet package. https://github.com/microsoft/sarif-sdk/issues/1636
* FEATURE: Add validation rule to ensure that all array-index-valued properties are consistent with their respective arrays.

** **v2.1.15** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.15) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.15) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.15) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.15)
* BUGFIX: FxCop converter produced empty `result.message` objects . https://github.com/microsoft/sarif-sdk/issues/1594
9 changes: 9 additions & 0 deletions src/Sarif.Converters/ConverterResources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/Sarif.Converters/ConverterResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -210,4 +210,7 @@
<data name="ContrastSecuritySiteRootDescription" xml:space="preserve">
<value>The root URI of the web site that was analyzed.</value>
</data>
<data name="FxCopNoMessage" xml:space="preserve">
<value>FxCop does not provide messages for suppressed results.</value>
</data>
</root>
3 changes: 2 additions & 1 deletion src/Sarif.Converters/FxCopConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ internal Result CreateResult(FxCopLogReader.Context context)
}

result.RuleId = context.CheckId;
result.Message = new Message { Arguments = context.Items, Id = context.ResolutionName, Text = context.Message };
string messageText = context.Message ?? ConverterResources.FxCopNoMessage;
result.Message = new Message { Arguments = context.Items, Id = context.ResolutionName, Text = messageText };
var location = new Location();

string sourceFile = GetFilePath(context);
Expand Down
70,272 changes: 33,283 additions & 36,989 deletions src/Test.FunctionalTests.Sarif/v2/ConverterTestData/FxCop/SarifSdkTest.xml.sarif

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions src/build.props
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<Copyright Condition=" '$(Copyright)' == '' ">© Microsoft Corporation. All rights reserved.</Copyright>

<!-- VersionPrefix denotes the current Semantic Version for the SDK. Must be updated before every nuget drop. -->
<VersionPrefix>2.1.14</VersionPrefix>
<VersionPrefix>2.1.15</VersionPrefix>

<!-- SchemaVersionAsPublishedToSchemaStoreOrg identifies the current published version on json schema store at https://schemastore.azurewebsites.net/schemas/json/ -->
<SchemaVersionAsPublishedToSchemaStoreOrg>2.1.0-rtm.4</SchemaVersionAsPublishedToSchemaStoreOrg>
Expand All @@ -30,7 +30,7 @@
place. These properties are actually used by the PowerShell script that
hides the previous package versions on nuget.org.
-->
<PreviousVersionPrefix>2.1.13</PreviousVersionPrefix>
<PreviousVersionPrefix>2.1.14</PreviousVersionPrefix>
<PreviousSchemaVersionAsPublishedToSchemaStoreOrg>2.1.0-rtm.3</PreviousSchemaVersionAsPublishedToSchemaStoreOrg>
<PreviousStableSarifVersion>2.1.0</PreviousStableSarifVersion>
</PropertyGroup>
Expand Down

0 comments on commit 6e194ec

Please sign in to comment.