GHAS validation rule - Message must be flattened

[yongyan-gh]

Description

Now GHAS code scanning will only accept a SARIF log containing results messages expressed by "message.text" and reject if the results messages use "message.id" and "message.arguments".

Without message text GitHub will fail the upload with message: “Error: Code Scanning could not process the submitted SARIF file: expected a result message, expected a result message…”

Creating the GitHub validator rule for this case.

[shaopeng-gh]

I think the _Invalid test has the not flatten message,
the _Valid should have the flatten message.

or, a valid test with results with flatten message,
another valid test that has no result at all like this one.
#Closed

[yongyan-gh]

this is output SARIF file generated by test, do you mean the input test SARIF file?

[shaopeng-gh]

you are right!

[shaopeng-gh]

MessageMustBeFlattened

This rule will tell user the other direction right:
public const string ProvideMessageArguments = "SARIF2002";

will the user get confused

[EasyRhinoMSFT]

It looks like the other names start with the verb when possible. So maybe this on could be "FlattenMessage"

[yongyan-gh]

Updated to "FlattenResultMessage"

[yongyan-gh]

right these 2 rules are opposite to each other.
SARIF validation rules are common for all SARIF files.
GH rules only for SARIF files need to be ingested by GHAS, should be fine if the GH rule conflicts with SARIF rules. What do you think?

[EasyRhinoMSFT]

👍

[michaelcfanning]

I know that 'flatten' is our special jargon but I wonder if something like 'ProvideFullyFormattedMessageStrings' is more descriptive.

[shaopeng-gh]

[michaelcfanning]

Analyze

So now we can't run an analysis without producing errors? Either we recommend using format string args or we object to the absence of flattened messages?

Ideally, the conflicting rules would turn off if the other one is being enforced.

[yongyan-gh]

how about adding a Set property to Skimmer class, e.g. ConflictSkimmerSet. In AnalyzeCommandBase.AnalyzeTarget() method, iterates available skimmer and add the ConflictSkimmerSet value to disabledSkimmers set if has value.

this way the rule author can override existing old rule conflicts with the new rule.

what do you think?

[michaelcfanning]

Interesting! And so the skimmers just need a dependency on rule ids it replaces. Yes, I think this could work.

[michaelcfanning]

OK, so I thought about your idea a bit more.

Rules should be able to declare an 'IncompatibleRuleIds' set or something similar, All the rule does is to publish this data.

Then we update the analysis engine, which, when instantiating skimmers, makes sure that no new enabled skimmers have a rule id that conflicts with the set of loaded skimmers so far. The analysis engine could raise an error-level configuration notification and exit.

This approach would require users to properly configure analysis to account for the configuration problem. I think everything would drop in pretty cleanly by doing this, we need a low-level update to rules metadata and a simple update to the default skimmer loading behavior.

btw - I'm approving this change, we can take on the above or continue to refine the idea in further work items/PRs.

[yongyan-gh]

thanks created the issue #2591

regarding "The analysis engine could raise an error-level configuration notification and exit.", should we raise an configuration notification error and disable the incompatible rules then continue the analysis instead exiting?

am thinking about the web validator scenario, user can run only SARIF validator rules, or all SARIF + GH validator rules, if adding a new GH rule which is not compatible with a SARIF rule, we may raise the configuration error and continue analysis without a config file change. If exits right away, the web validator stops getting results until config file is fixed.

[michaelcfanning]