FEATURE: Add `--normalize-for-ghas` argument to the `rewrite` command #2581

shaopeng-gh · 2022-12-06T17:44:36Z

This adds the function to rewrite a SARIF file to meet requirement of GitHub / GHAS.

shaopeng-gh · 2022-12-06T17:47:36Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

-                    }
-
-                    node.Results = errors;
-                }


please also see the test file "NonErrorResults.sarif'.
This does not seem right, why do we want to remove warning results?
#Closed

Perhaps GitHub only used to process errors? Have you tested this in their system?

This is from the current SDK repo:

and also those linters works I had been using warning to upload to GHAS, shows similar to above.

shaopeng-gh · 2022-12-06T17:50:36Z

src/Sarif.Multitool.Library/RewriteOptions.cs

@@ -16,5 +16,33 @@ public class RewriteOptions : SingleFileOptionsBase
            Default = false,
            HelpText = "Sort results in the final output file.")]
        public bool SortResults { get; set; }
+
+        [Option(


[Option(

All these options are copied from other file with existing name and HelpText. #Closed

KalleOlaviNiemitalo · 2022-12-06T20:04:10Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

+                    this.run.Tool.Driver.GlobalMessageStrings?.TryGetValue(node.Id, out formatString) == true)
+                {
+                    node.Text = node.Arguments?.Count > 0
+                        ? string.Format(CultureInfo.CurrentCulture, formatString.Text, node.Arguments.ToArray())


Seems like a false alarm to me

src/Test.UnitTests.Sarif.Multitool.Library/RewriteCommandTests.cs

shaopeng-gh · 2022-12-06T18:53:39Z

...Test.UnitTests.Sarif.Multitool.Library/TestData/RewriteCommand/Inputs/RunWithArtifacts.sarif

+          }
+        }
+      ],
+      "artifacts": [


artifacts

3 things changed,
after rewrite:

baseuri is created

messages are flatten

artifacts are removed #Closed

KalleOlaviNiemitalo · 2022-12-06T19:59:41Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

+        {
+            if (node.Text == null)
+            {
+                ReportingDescriptor rule = this.ruleIndex != -1 ? this.run.Tool.Driver.Rules[this.ruleIndex] : null;


This doesn't fully implement SARIF-2.1.0 §3.11.7 Message string lookup. I think this won't find messages of tool extensions. #Resolved

you are correct, created an issue to track.

KalleOlaviNiemitalo · 2022-12-06T20:02:15Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

+                    node.Text = node.Arguments?.Count > 0
+                        ? rule.Format(node.Id, node.Arguments)
+                        : formatString?.Text;


If the result of string.Format still contains substrings like "{0}", should this double the braces, in order to prevent the SARIF consumer from treating those as placeholders and attempting to expand them again? Also, should this reset node.Arguments to null? #Resolved

the result is put in .Text.
.Text is for plain text, consumer should not try to expand any placeholders in it.

should this reset node.Arguments to null ---- currently, .text is added side by side with .id and .arguments.
The information is duplicated, the benefit is we can un-flatten if needed by simply remove the .text field.

.Text is for plain text, consumer should not try to expand any placeholders in it.

That's not how I read the standard.

SARIF-2.1.0 §3.11.8 (text property): "A message object MAY contain a property named text whose value is a non-empty string containing a plain text message (§3.11.3)."

SARIF-2.1.0 §3.11.3 (Plain text messages): "The message string MAY contain placeholders (§3.11.5) and embedded links (§3.11.6)."

So the text property of a message object may contain placeholders. Does the SARIF consumer in GitHub Advanced Security deviate from the standard in this respect?

With more reading I think you are right! The Text can contain placeholder again!

Within both plain text and formatted message strings, the characters “{” and “}” SHALL be represented by the character sequences “{{” and “}}” respectively. ----- so, I think I will replace { with {{ and } with }}, let me know if sounds good to you.

I will replace { with {{ and } with }} -- this is done and updated the test case.

KalleOlaviNiemitalo · 2022-12-06T20:04:10Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

+                    this.run.Tool.Driver.GlobalMessageStrings?.TryGetValue(node.Id, out formatString) == true)
+                {
+                    node.Text = node.Arguments?.Count > 0
+                        ? string.Format(CultureInfo.CurrentCulture, formatString.Text, node.Arguments.ToArray())


Seems like a false alarm to me

KalleOlaviNiemitalo · 2022-12-06T20:04:51Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

@@ -161,6 +138,32 @@ public override Result VisitResult(Result node)
            return base.VisitResult(node);
        }

+        public override Message VisitMessage(Message node)
+        {
+            if (node.Text == null)


Does this need the same for node.Markdown? #Closed

this is form GitHub, I think GitHub requires .Text only

michaelcfanning · 2022-12-06T21:07:57Z

src/ReleaseHistory.md

@@ -12,6 +12,7 @@
 * BUGFIX: Eliminate `IOException` and `DirectoryNotFoundException` exceptions thrown by `merge` command when splitting by rule (due to invalid file characters in rule ids). [#2513](https://github.com/microsoft/sarif-sdk/pull/2513)
 * BUGFIX: Fix classes inside NotYetAutoGenerated folder missing `virtual` keyword for public methods and properties, by regenerate and manually sync the changes. [#2537](https://github.com/microsoft/sarif-sdk/pull/2537)
 * FEATURE: Allow external set of `MaxFileSizeInKilobytes`, which will allow SDK users to change the value. (Default value is 1024) [#2578](https://github.com/microsoft/sarif-sdk/pull/2578)
+* FEATURE: Add `--normalize-for-github` argument to the `rewrite` command to get GitHub supported SARIF format. [#2581](https://github.com/microsoft/sarif-sdk/pull/2581)


Good release note item but let me add a bit more detail, try this:

FEATURE: Add --normalize-for-github argument to the rewrite command to ensure rewritten SARIF is compatible with GitHub Advanced Security ingestion requirements. #2581 #Closed

thanks, fixed

michaelcfanning · 2022-12-06T21:09:41Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

            this.artifacts = node.Artifacts;
            this.threadFlowLocations = node.ThreadFlowLocations;

            if (node.Results != null)
            {
-                int errorsCount = 0;


errorsCount

I wouldn't delete this until you confirm that GH can accept warnings. Also, if this visitor was correct, the validator should also fire on any warnings in a log file (warning that they will be stripped). #Closed

yes i think it supports warning, please see my other reply with screenshot.

michaelcfanning · 2022-12-06T21:11:10Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

@@ -144,6 +119,8 @@

        public override Result VisitResult(Result node)
        {
+            this.ruleIndex = node.RuleIndex;
+
            if (node.Fingerprints != null)
            {
                // GitHub appears to require that fingerprints be emitted to the


s to require

We have a message string look-up helper that you should use. As Kalle notes, this is a pretty complex operation.

OK, I see you grabbed this code from the insert optional data visitor. We shouldn't copy code like this, minimally, it should be in a common helper somewhere (you could have created a static in the other visitor, for example).

Let me keep looking for a better helper, I could swear we have one. #Closed

OK, nope. It appears that we don't. So the correct course here is to please create a common helper for both this code and the insert optional data visitor to call. Second, create an issue for the problem that Kalle notes: 'Message creation logic does not properly search tool extensions for format strings.'

got it, will work on this.

done both add helper method and create an issue
#2582

michaelcfanning · 2022-12-06T21:35:04Z

src/Sarif.Multitool.Library/RewriteCommand.cs

@@ -64,6 +65,25 @@ public int Run(RewriteOptions options)
                    reformattedLog = new SortingVisitor().VisitSarifLog(reformattedLog);
                }

+                if (options.NormalizeForGitHub)
+                {
+                    if ((reformattedLog.Runs?.Any(r => r.Artifacts?.Any(a => a.Location?.Uri?.IsAbsoluteUri == true) == true) == true) ||


reformattedLog

So I don't like these two lines of code which will traverse the log file twice.

Can't we simply invoke the RebaseUriVisitor in cases when those args are non-null.

And then make the GitHubIngestionVisitor throw new arg exception on encountering the any absolute uri as you detect here.
#Closed

made the suggested change.
Though we can not simply throw in GitHubIngestionVisitor, because it is a same method VisitArtifactLocation to visit both of it in result and also the originalUriBaseIds,
we allow one and not the other to be absolute.
Currently I does not check and throw, let me know your instruction.

lgtm-com · 2022-12-07T19:30:56Z

This pull request fixes 1 alert when merging b17469e into 3fa7f43 - view on LGTM.com

fixed alerts:

1 for Dereferenced variable may be null

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

src/Sarif/Visitors/VisitorHelper.cs

yongyan-gh · 2023-01-06T18:51:40Z

src/Sarif.Multitool.Library/RewriteOptions.cs

@@ -16,5 +16,33 @@ public class RewriteOptions : SingleFileOptionsBase
            Default = false,
            HelpText = "Sort results in the final output file.")]
        public bool SortResults { get; set; }
+
+        [Option(
+            "normalize-for-github",


"normalize-for-github"

Question: the Convert command has the same normalize-for-github parameter, can we use Convert command to achieve the same? #Closed

Convert means 'convert from some other non-SARIF format to SARIF.' This operation is a SARIF->SARIF transformation, not a 'conversion'.

One thing, we could use 'normalize-for-ghas' instead as a more precise designation (though 'ghas' may be more opaque to users.

I'm fine with either choice.

use 'normalize-for-ghas' instead as a more precise designation
---- sounds good, will change it.

use 'normalize-for-ghas' instead as a more precise designation
---- done.

…r `convert` command

yongyan-gh

michaelcfanning · 2023-01-18T17:17:50Z

src/ReleaseHistory.md

@@ -28,6 +28,8 @@
 * FEATURE : Allow initialization of file regions cache in `InsertOptionalDataVisitor` (previously initialized exclusively from `FileRegionsCache.Instance`).
 * FEATURE : Provide 'RuleScanTime` trace and emitted timing data. Provide `ScanExecution` trace with no utilization.
 * FEATURE : Populate associated rule data in `LogToolNotification` as called from `SarifLogger`. [#2604](https://github.com/microsoft/sarif-sdk/pull/2604)
+* FEATURE : Add `--normalize-for-ghas` argument to the `rewrite` command to ensure rewritten SARIF is compatible with GitHub Advanced Security (GHAS) ingestion requirements. [#2581](https://github.com/microsoft/sarif-sdk/pull/2581)
+* BREAKING: Rename `--normalize-for-github` argument to `--normalize-for-ghas` for `convert` command. [#2581](https://github.com/microsoft/sarif-sdk/pull/2581)


BREAKING

move this to top

michaelcfanning · 2023-01-18T17:18:32Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

@@ -161,6 +137,15 @@
            return base.VisitResult(node);
        }

+        public override Message VisitMessage(Message node)
+        {


return

add a blank line.

michaelcfanning · 2023-01-18T17:19:13Z

src/Sarif/Visitors/GitHubIngestionVisitor.cs

@@ -144,6 +118,8 @@

        public override Result VisitResult(Result node)
        {
+            this.ruleIndex = node.RuleIndex;
+
            if (node.Fingerprints != null)
            {
                // GitHub appears to require that fingerprints be emitted to the


VisitMessage

add unit test for result with rule id only, no index

michaelcfanning · 2023-01-18T17:23:24Z

src/Sarif/Visitors/VisitorHelper.cs

+{
+    public static class VisitorHelper
+    {
+        public static void FlattenMessage(Message node, int ruleIndex, Run run)


node

name this message #Resolved

michaelcfanning · 2023-01-18T17:30:18Z

src/Sarif.Multitool.Library/ConvertOptions.cs

@@ -24,8 +24,8 @@ public class ConvertOptions : SingleFileOptionsBase
        public string PluginAssemblyPath { get; set; }

        [Option(
-            "normalize-for-github",
-            HelpText = "Normalize converted output to conform to GitHub Advanced Security code scanning ingestion requirements.")]
-        public bool NormalizeForGitHub { get; set; }


NormalizeForGitHub

leave this and mark it obsolete??

let's go and remove it, per hulon.

added it back as obsolete, per the last decision from Michael:

suvamM · 2023-01-18T17:35:18Z

src/Sarif.Multitool.Library/RewriteOptions.cs

@@ -16,5 +16,33 @@ public class RewriteOptions : SingleFileOptionsBase
            Default = false,
            HelpText = "Sort results in the final output file.")]
        public bool SortResults { get; set; }
+
+        [Option(
+            "normalize-for-ghas",


"normalize-for-ghas"

@shaopeng-gh Sorry if I missed this: this is a required argument?

suvamM · 2023-01-18T17:39:10Z

src/ReleaseHistory.md

@@ -28,6 +28,8 @@
 * FEATURE : Allow initialization of file regions cache in `InsertOptionalDataVisitor` (previously initialized exclusively from `FileRegionsCache.Instance`).
 * FEATURE : Provide 'RuleScanTime` trace and emitted timing data. Provide `ScanExecution` trace with no utilization.
 * FEATURE : Populate associated rule data in `LogToolNotification` as called from `SarifLogger`. [#2604](https://github.com/microsoft/sarif-sdk/pull/2604)
+* FEATURE : Add `--normalize-for-ghas` argument to the `rewrite` command to ensure rewritten SARIF is compatible with GitHub Advanced Security (GHAS) ingestion requirements. [#2581](https://github.com/microsoft/sarif-sdk/pull/2581)


FEATURE : Add --normalize-for-ghas argument to the rewrite command to ensure rewritten SARIF is compatible with GitHub Advanced Security (GHAS) ingestion requirements.

@shaopeng-gh Can you add some more details on what these requirements are? That would make the PR self-contained, otherwise it is a little opaque to me why we are making these changes.

per Yong in the PR meeting, it is from the task
"Binskim action send log file to GHAS".

michaelcfanning · 2023-01-18T17:46:09Z

src/Sarif.Multitool.Library/RewriteOptions.cs

@@ -16,5 +16,33 @@ public class RewriteOptions : SingleFileOptionsBase
            Default = false,
            HelpText = "Sort results in the final output file.")]
        public bool SortResults { get; set; }
+
+        [Option(
+            "normalize-for-ghas",


normalize-for-ghas

don't delete the previous option, mark it as obsolete and make sure it continues to work. update the release notes. #Resolved

michaelcfanning · 2023-01-18T17:46:57Z

src/Sarif.Multitool.Library/RewriteOptions.cs

+            Default = false,
+            HelpText = "All relative uris will be rebased to the base-path-value if true. Default is false."
+            )]
+        public bool RebaseRelativeUris { get; set; }


RebaseRelativeUris

as a next step, you need to make sure rewrite always honors this command in all cases.

…bsolete

michaelcfanning

FEATURE: Add --normalize-for-github argument to the rewrite command

e66e7a1

shaopeng-gh requested review from suvamM and HulonJenkins December 6, 2022 17:44

shaopeng-gh requested review from michaelcfanning, EasyRhinoMSFT, marmegh and cfaucon as code owners December 6, 2022 17:44

shaopeng-gh commented Dec 6, 2022

View reviewed changes

github-advanced-security bot found potential problems Dec 6, 2022

View reviewed changes

shaopeng-gh added 3 commits December 6, 2022 10:01

Test include message flatten

b613f11

fix code scan issue

ac1d133

fix code scan

b6b98c0

shaopeng-gh commented Dec 6, 2022

View reviewed changes

KalleOlaviNiemitalo reviewed Dec 6, 2022

View reviewed changes

michaelcfanning reviewed Dec 6, 2022

View reviewed changes

shaopeng-gh mentioned this pull request Dec 7, 2022

Message creation logic does not properly search tool extensions for format strings. #2582

Open

shaopeng-gh added 3 commits December 7, 2022 00:49

Fix comments

2c7ec70

replace { and }

d1a8b18

fix format

b17469e

github-advanced-security bot found potential problems Dec 7, 2022

View reviewed changes

src/Sarif/Visitors/VisitorHelper.cs Fixed Show fixed Hide fixed

src/Sarif/Visitors/VisitorHelper.cs Fixed Show fixed Hide fixed

shaopeng-gh requested review from KalleOlaviNiemitalo and michaelcfanning and removed request for KalleOlaviNiemitalo December 7, 2022 20:27

yongyan-gh reviewed Jan 6, 2023

View reviewed changes

shaopeng-gh added 3 commits January 6, 2023 12:15

merge from main

a79925e

Rename --normalize-for-github argument to --normalize-for-ghas fo…

cad8330

…r `convert` command

fix code scanning

eae5a55

yongyan-gh approved these changes Jan 9, 2023

View reviewed changes

merge from main

2df868a

michaelcfanning changed the title ~~FEATURE: Add --normalize-for-github argument to the rewrite command~~ FEATURE: Add --normalize-for-ghas argument to the rewrite command Jan 18, 2023

michaelcfanning reviewed Jan 18, 2023

View reviewed changes

suvamM reviewed Jan 18, 2023

View reviewed changes

michaelcfanning reviewed Jan 18, 2023

View reviewed changes

1. move method into Message object 2. add back old option name as o…

fbb97ac

…bsolete

michaelcfanning approved these changes Jan 18, 2023

View reviewed changes

michaelcfanning merged commit afc9681 into main Jan 18, 2023

michaelcfanning deleted the users/shaopeng-gh/ghas branch January 18, 2023 21:12

This was referenced Feb 21, 2023

Releasing v3.2.0 #2622

Closed

Releasing 4.0.0 #2623

Merged

FEATURE: Add --normalize-for-ghas argument to the rewrite command #2581

FEATURE: Add --normalize-for-ghas argument to the rewrite command #2581

Conversation

shaopeng-gh commented Dec 6, 2022

shaopeng-gh Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

shaopeng-gh Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

KalleOlaviNiemitalo Dec 6, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalleOlaviNiemitalo Dec 6, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalleOlaviNiemitalo Dec 6, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 6, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 6, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 6, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Dec 6, 2022 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 7, 2022

yongyan-gh Jan 6, 2023 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongyan-gh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 18, 2023 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 18, 2023 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

FEATURE: Add `--normalize-for-ghas` argument to the `rewrite` command #2581

FEATURE: Add `--normalize-for-ghas` argument to the `rewrite` command #2581

shaopeng-gh Dec 6, 2022 •

edited

Loading

shaopeng-gh Dec 6, 2022 •

edited

Loading

shaopeng-gh Dec 6, 2022 •

edited

Loading

shaopeng-gh Dec 6, 2022 •

edited

Loading

KalleOlaviNiemitalo Dec 6, 2022 •

edited by shaopeng-gh

Loading

KalleOlaviNiemitalo Dec 6, 2022 •

edited by shaopeng-gh

Loading

KalleOlaviNiemitalo Dec 6, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Dec 6, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Dec 6, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Dec 6, 2022 •

edited by shaopeng-gh

Loading

michaelcfanning Dec 6, 2022 •

edited by shaopeng-gh

Loading

yongyan-gh Jan 6, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning Jan 18, 2023 •

edited by shaopeng-gh

Loading