Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating docs and fixing command #2181

Merged
merged 8 commits into from
Dec 14, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ado-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ jobs:
inputs:
targetType: filePath
filePath: ./scripts/BuildAndTest.ps1

- task: PublishBuildArtifacts@1
inputs:
pathToPublish: 'msbuild.binlog'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msbuild.binlog [](start = 22, length = 14)

Our build is failing some times. This will help us investigate

artifactName: 'binlog'

- job: npm_pipeline
steps:
Expand Down
60 changes: 27 additions & 33 deletions docs/ValidationRules.md
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,7 @@ The two identity-related properties of a SARIF rule must be consistent. The requ

### Description

Specify a valid URI reference for every URI-valued property.

URIs must conform to [RFC 3986](https://tools.ietf.org/html/rfc3986). In addition, 'file' URIs must not include '..' segments. If symbolic links are present, '..' might have different meanings on the machine that produced the log file and the machine where an end user or a tool consumes it.
Specify a valid URI reference for every URI-valued property. URIs must conform to [RFC 3986](https://tools.ietf.org/html/rfc3986). In addition, 'file' URIs must not include '..' segments. If symbolic links are present, '..' might have different meanings on the machine that produced the log file and the machine where an end user or a tool consumes it.

### Messages

Expand All @@ -140,13 +138,7 @@ URIs must conform to [RFC 3986](https://tools.ietf.org/html/rfc3986). In additio

### Description

When using the 'uriBaseId' property, obey the requirements in the SARIF specification [3.4.4](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317431) that enable it to fulfill its purpose of resolving relative references to absolute locations. In particular:

If an 'artifactLocation' object has a 'uriBaseId' property, its 'uri' property must be a relative reference, because if 'uri' is an absolute URI then 'uriBaseId' serves no purpose.

Every URI reference in 'originalUriBaseIds' must resolve to an absolute URI in the manner described in the SARIF specification [3.14.14](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317498).

Finally, a relative reference in 'artifactLocation.uri' must not begin with a slash, because that prevents it from combining properly with the absolute URI specified by a 'uriBaseId'.
When using the 'uriBaseId' property, obey the requirements in the SARIF specification [3.4.4](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317431) that enable it to fulfill its purpose of resolving relative references to absolute locations. In particular: If an 'artifactLocation' object has a 'uriBaseId' property, its 'uri' property must be a relative reference, because if 'uri' is an absolute URI then 'uriBaseId' serves no purpose. Every URI reference in 'originalUriBaseIds' must resolve to an absolute URI in the manner described in the SARIF specification [3.14.14](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317498). Finally, a relative reference in 'artifactLocation.uri' must not begin with a slash, because that prevents it from combining properly with the absolute URI specified by a 'uriBaseId'.

### Messages

Expand Down Expand Up @@ -194,9 +186,7 @@ Certain URIs are required to be absolute. For the most part, these are URIs that

### Description

The properties of an 'invocation' object must be consistent.

If the 'invocation' object specifies both 'startTimeUtc' and 'endTimeUtc', then 'endTimeUtc' must not precede 'startTimeUtc'. To allow for the possibility that the duration of the run is less than the resolution of the string representation of the time, the start time and the end time may be equal.
The properties of an 'invocation' object must be consistent. If the 'invocation' object specifies both 'startTimeUtc' and 'endTimeUtc', then 'endTimeUtc' must not precede 'startTimeUtc'. To allow for the possibility that the duration of the run is less than the resolution of the string representation of the time, the start time and the end time may be equal.

### Messages

Expand All @@ -210,9 +200,7 @@ If the 'invocation' object specifies both 'startTimeUtc' and 'endTimeUtc', then

### Description

The properties of a 'region' object must be consistent.

SARIF can specify a 'region' (a contiguous portion of a file) in a variety of ways: with line and column numbers, with a character offset and count, or with a byte offset and count. The specification states certain constraints on these properties, both within each property group (for example, the start line cannot be greater than end line) and between the groups (for example, if more than one group is present, they must independently specify the same portion of the file). See the SARIF specification ([3.30](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317685)).
The properties of a 'region' object must be consistent. SARIF can specify a 'region' (a contiguous portion of a file) in a variety of ways: with line and column numbers, with a character offset and count, or with a byte offset and count. The specification states certain constraints on these properties, both within each property group (for example, the start line cannot be greater than end line) and between the groups (for example, if more than one group is present, they must independently specify the same portion of the file). See the SARIF specification ([3.30](https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317685)).

### Messages

Expand All @@ -234,15 +222,7 @@ SARIF can specify a 'region' (a contiguous portion of a file) in a variety of wa

### Description

Ensure consistency among the properties of a 'physicalLocation' object.

A SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'. If 'contextRegion' is present, then 'region' must also be present, and 'contextRegion' must be a "proper superset" of 'region'. That is, 'contextRegion' must completely contain 'region', and it must be larger than 'region'. To understand why this is so we must understand the roles of the 'region' and 'contextRegion' properties.

'region' allows both users and tools to distinguish similar results within the same artifact. If a SARIF viewer has access to the artifact, it can display it, and highlight the location identified by the analysis tool. If the region has a 'snippet' property, then even if the viewer doesn't have access to the artifact (which might be the case for a web-based viewer), it can still display the faulty code.

'contextRegion' provides users with a broader view of the result location. Typically, it consists of a range starting a few lines before 'region' and ending a few lines after. Again, if a SARIF viewer has access to the artifact, it can display it, and highlight the context region (perhaps in a lighter shade than the region itself). This isn't terribly useful since the user can already see the whole file, with the 'region' already highlighted. But if 'contextRegion' has a 'snippet' property, then even a viewer without access to the artifact can display a few lines of code surrounding the actual result, which is helpful to users.

If the validator reports that 'contextRegion' is not a proper superset of 'region', then it's possible that the tool reversed 'region' and 'contextRegion'. If 'region' and 'contextRegion' are identical, the tool should simply omit 'contextRegion'.
Ensure consistency among the properties of a 'physicalLocation' object. A SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'. If 'contextRegion' is present, then 'region' must also be present, and 'contextRegion' must be a "proper superset" of 'region'. That is, 'contextRegion' must completely contain 'region', and it must be larger than 'region'. To understand why this is so we must understand the roles of the 'region' and 'contextRegion' properties. 'region' allows both users and tools to distinguish similar results within the same artifact. If a SARIF viewer has access to the artifact, it can display it, and highlight the location identified by the analysis tool. If the region has a 'snippet' property, then even if the viewer doesn't have access to the artifact (which might be the case for a web-based viewer), it can still display the faulty code. 'contextRegion' provides users with a broader view of the result location. Typically, it consists of a range starting a few lines before 'region' and ending a few lines after. Again, if a SARIF viewer has access to the artifact, it can display it, and highlight the context region (perhaps in a lighter shade than the region itself). This isn't terribly useful since the user can already see the whole file, with the 'region' already highlighted. But if 'contextRegion' has a 'snippet' property, then even a viewer without access to the artifact can display a few lines of code surrounding the actual result, which is helpful to users. If the validator reports that 'contextRegion' is not a proper superset of 'region', then it's possible that the tool reversed 'region' and 'contextRegion'. If 'region' and 'contextRegion' are identical, the tool should simply omit 'contextRegion'.

### Messages

Expand Down Expand Up @@ -496,9 +476,7 @@ A SARIF log file should contain, on the root object, a '$schema' property that r

### Description

Adopt uniform naming conventions for rule ids.

Many tools follow a conventional format for the 'reportingDescriptor.id' property: a short string identifying the tool concatenated with a numeric rule number, for example, 'CS2001' for a diagnostic from the Roslyn C# compiler. For uniformity of experience across tools, we recommend this format.
Adopt uniform naming conventions for rule ids. Many tools follow a conventional format for the 'reportingDescriptor.id' property: a short string identifying the tool concatenated with a numeric rule number, for example, 'CS2001' for a diagnostic from the Roslyn C# compiler. For uniformity of experience across tools, we recommend this format.

### Messages

Expand Down Expand Up @@ -542,24 +520,32 @@ Provide context regions to enable users to see a portion of the code that surrou

Rule metadata should provide information that makes it easy to understand and fix the problem.

Provide the 'name' property, which contains a "friendly name" that helps users see at a glance the purpose of the rule. For uniformity of experience across all tools that produce SARIF, the friendly name should be a single Pascal identifier, for example, 'ProvideRuleFriendlyName'.
Provide the 'name' property, which contains a "friendly name" that helps users see at a glance the purpose of the rule. For uniformity of experience across all tools that produce SARIF, the friendly name should be a single Pascal-case identifier, for example, 'ProvideRuleFriendlyName'.

Provide the 'helpUri' property, which contains a URI where users can find detailed information about the rule. This information should include a detailed description of the invalid pattern, an explanation of why the pattern is poor practice (particularly in contexts such as security or accessibility where driving considerations might not be readily apparent), guidance for resolving the problem (including describing circumstances in which ignoring the problem altogether might be appropriate), examples of invalid and valid patterns, and special considerations (such as noting when a violation should never be ignored or suppressed, noting when a violation could cause downstream tool noise, and noting when a rule can be configured in some way to refine or alter the analysis).

### Messages

#### `ProvideFriendlyName`: Note
#### `FriendlyNameNotAPascalIdentifier`: Note

{0}: The rule '{1}' does not provide a "friendly name" in its 'name' property. The friendly name should be a single Pascal identifier, for example, 'ProvideRuleFriendlyName', that helps users see at a glance the purpose of the analysis rule.
{0}: '{1}' is not a Pascal-case identifier. For uniformity of experience across all tools that produce SARIF, the friendly name should be a single Pascal-case identifier, for example, 'ProvideRuleFriendlyName'.

#### `FriendlyNameNotAPascalIdentifier`: Note
#### `ProvideFriendlyName`: Note

{0}: '{1}' is not a Pascal identifier. For uniformity of experience across all tools that produce SARIF, the friendly name should be a single Pascal identifier, for example, 'ProvideRuleFriendlyName'.
{0}: The rule '{1}' does not provide a "friendly name" in its 'name' property. The friendly name should be a single Pascal-case identifier, for example, 'ProvideRuleFriendlyName', that helps users see at a glance the purpose of the analysis rule.

#### `ProvideHelpUri`: Note

{0}: The rule '{1}' does not provide a help URI. Providing a URI where users can find detailed information about the rule helps users to understand the result and how they can best address it.

#### `ProvideMetadataForAllViolatedRules`: Note

'{0}' does not provide a 'rules' property. 'rules' contain information that helps users understand why each rule fires and what the user can do to fix it.

#### `ProvideRuleMetadata`: Note

'{0}' does not provide metadata for rule '{1}'. Rule metadata contains information that helps the user understand why each rule fires and what the user can do to fix it.

---

## Rule `SARIF2013.ProvideEmbeddedFileContent`
Expand Down Expand Up @@ -630,5 +616,13 @@ Semantics: Assuming the reader of the log file (an end user or another tool) has

{0}: The file location '{1}' is specified with absolute URI. Prefer a relative reference together with a uriBaseId property.

#### `ShouldNotContainBackSlash`: Note

{0}: The relative file URL '{1}' contains one or more backslashes, which will be preserved when concatenating to an absolute URL. This can result in inconsistent representations, compared to URLs created from an absolute file path, which may be regarded as not equivalent. Replace all backslashes with forward slashes.

#### `ShouldNotStartWithSlash`: Note

{0}: The relative file URL '{1}' is prefixed with a leading slash, which can lead to unintended behavior when concatenating with absolute URLs. Remove the leading slash.

---

21 changes: 15 additions & 6 deletions docs/multitool-usage.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@ Use the SARIF Multitool to transform, enrich, filter, result match, and do other
## Modes
| Mode | Purpose |
| ---- | ------- |
| help | See Usage |
| absoluteuri | Turn all relative Uris into absolute URIs (to be used after rebaseUri is run) |
| apply-policy | Apply policies from SARIF log |
| convert | Convert a tool output log to SARIF format |
| match-results-forward | Match Results run over run to identify New, Absent, and Unchanged Results |
| export-validation-config | Export validation rule options to an XML or JSON file that can be edited and used to configure subsequent analysis |
| export-validation-rules | Export validation rules metadata to a SARIF or SonarQube XML file |
| file-work-items | Send SARIF results to a work item tracking system such as GitHub or Azure DevOps |
| rewrite | Transform a SARIF file to a reformatted version |
| transform | Transform a SARIF log to a different SARIF version |
| match-results-forward | Match Results run over run to identify New, Absent, and Unchanged Results |
| merge | Merge multiple SARIF files into one |
| rebaseuri | Rebase the URIs in one or more sarif files. |
| absoluteuri | Turn all relative Uris into absolute URIs (to be used after rebaseUri is run) |
| page | Extract a subset of results from a source SARIF file. |
| query | Find the matching subset of a SARIF file and output it or log it. |
| rebaseuri | Rebase the URIs in one or more sarif files. |
| rewrite | Transform a SARIF file to a reformatted version |
| transform | Transform a SARIF log to a different SARIF version |
| validate | Validate a SARIF File against the schema and against additional correctness rules. |
| help | See Usage |
| version | Display version information |

## Examples
Expand Down Expand Up @@ -43,6 +46,12 @@ Sarif.Multitool rebaseuri Current.sarif --base-path-value "C:\Local\Path\To\Repo
: Compare to previous baseline to identify new Results
Sarif.Multitool match-results-forward Current.sarif --previous Baseline.sarif --output-file-path NewBaseline.sarif

: Export validation config (this can be used to validate and rewrite the default policies)
Sarif.Multitool export-validation-config validation.xml

: Export validation rules metadata
Sarif.Multitool export-validation-rules ValidationRules.md

: Extract new Results only from New Baseline
Sarif.Multitool query NewBaseline.sarif --expression "BaselineState == 'New'" --output Current.NewResults.sarif

Expand Down
2 changes: 2 additions & 0 deletions lgtm.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,7 @@ extraction:
csharp:
index:
solution: "src/Sarif.Sdk.CodeQL.sln"
dotnet:
version: 3.1.10
msbuild:
configuration: "Release"
2 changes: 1 addition & 1 deletion scripts/BuildAndTest.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function Invoke-DotNetBuild($solutionFileRelativePath) {
Write-Information "Building $solutionFileRelativePath..."

$solutionFilePath = Join-Path $SourceRoot $solutionFileRelativePath
& dotnet build $solutionFilePath --configuration $Configuration -v $BuildVerbosity --no-incremental
& dotnet build $solutionFilePath --configuration $Configuration --verbosity $BuildVerbosity --no-incremental -bl
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-bl [](start = 112, length = 4)

add binlog parameter


if ($LASTEXITCODE -ne 0) {
Exit-WithFailureMessage $ScriptName "Build of $solutionFilePath failed."
Expand Down
7 changes: 7 additions & 0 deletions src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,9 @@ private SupportedPlatform GetCurrentRunningOS()
DefaultDriverOptions.CreateRuleSpecificOption(skimmer, DefaultDriverOptions.RuleEnabled);

RuleEnabledState ruleEnabled = rootContext.Policy.GetProperty(ruleEnabledProperty);
FailureLevel failureLevel = (ruleEnabled == RuleEnabledState.Default || ruleEnabled == RuleEnabledState.Disabled)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failureLevel [](start = 29, length = 12)

generate failureLevel from the file. With that, we can replace our default value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok! but we need more unit tests here, clearly. :) maybe yong can help?


In reply to: 541631128 [](ancestors = 541631128)

? default
: (FailureLevel)Enum.Parse(typeof(FailureLevel), ruleEnabled.ToString());

if (ruleEnabled == RuleEnabledState.Disabled)
{
Expand All @@ -555,6 +558,10 @@ private SupportedPlatform GetCurrentRunningOS()
// So disable it, but don't complain that the rule was explicitly disabled.
disabledSkimmers.Add(skimmer.Id);
}
else if (skimmer.DefaultConfiguration.Level != failureLevel)
{
skimmer.DefaultConfiguration.Level = failureLevel;
}
}

if (disabledSkimmers.Count == skimmers.Count())
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using CommandLine;
using System.Collections.Generic;
using System.Reflection;

using Microsoft.CodeAnalysis.Sarif.Driver;

namespace Microsoft.CodeAnalysis.Sarif.Multitool
{
public class ExportValidationConfigurationCommand : ExportConfigurationCommandBase
{
public override IEnumerable<Assembly> DefaultPlugInAssemblies => new Assembly[] {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefaultPlugInAssemblies [](start = 46, length = 23)

fixing a null reference

this.GetType().Assembly
};
}
}
1 change: 0 additions & 1 deletion src/build.props
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@

<PropertyGroup Condition=" '$(Configuration)' == 'Release' ">
<ContinuousIntegrationBuild>true</ContinuousIntegrationBuild>
<TargetFrameworkMonikerAssemblyAttributesPath>$([System.IO.Path]::Combine('$(IntermediateOutputPath)','$(TargetFrameworkMoniker).AssemblyAttributes$(DefaultLanguageSourceExtension)'))</TargetFrameworkMonikerAssemblyAttributesPath>
<Deterministic>true</Deterministic>
</PropertyGroup>
<ItemGroup Condition=" '$(Configuration)' == 'Release' ">
Expand Down