Documentation and sample mistakes and inconsistencies

[JaneX8]

When writing a JSONschema rule, I got my hands on all samples and rules I could find. I've downloaded all files I could get my hands on to do some comparison. From:

• https://github.com/Microsoft/DevSkim/tree/main/rules/default
• https://github.com/microsoft/ApplicationInspector/tree/main/AppInspector/rules/default

As the documentation on writing custom rules lacks a lot in my opinion, I consider all the rules itself also documentation for now. Also I'm referencing sometimes to the ApplicationInspector repo as that seems to have some more details needed to understand this repo. Below are some (likely) mistakes I noticed in the documentation:

1. XML or JSON

https://github.com/microsoft/ApplicationInspector/wiki/3.6-Structured-Data-Queries-(XPath,-JSONPath,-YamlPath)#sample-xml-rule contains a JSON rule, not an XML rule as the title suggests. However, if it describes JSON rules for XML validation, (which I suspect) that's what should be the description of the titles and the document.

2. Incorrect rule format - missing array

The formatting of this rule is incorrect. The whole structure should be enclosed in [] as in [{...}]. https://github.com/microsoft/DevSkim/wiki/Sample-Rule#rule-sample. Like the other rules and examples in ApplicationInspector.

3. Rule sample without tags

https://github.com/microsoft/ApplicationInspector/wiki/3.6-Structured-Data-Queries-(XPath,-JSONPath,-YamlPath)#sample-rule this sample does not contains tags: []. From the documentation it's unclear to me when to use or not to use tags. For now I assume it's always required.

4. Wrong encoding?

Not sure why exactly but https://github.com/microsoft/ApplicationInspector/blob/main/AppInspector/rules/default/frameworks/rust.json seems to be the only rule saved with encoding UTF-8 with BOM while all other rules I could find are UTF-8 only. Is encoding of custom rules defined, a requirement and documented?

5. Invalid JSON (due to trailing comma)

• https://github.com/microsoft/ApplicationInspector/blob/main/AppInspector/rules/default/ai/generative_ai_libraries.json:

Error: Parse error on line 133:
...         },        ],        "must-mat
----------------------^
Expecting 'STRING', 'NUMBER', 'NULL', 'TRUE', 'FALSE', '{', '[', got ']'

• https://github.com/microsoft/ApplicationInspector/blob/main/AppInspector/rules/default/cryptography/post_quantum.json

Error: Parse error on line 88:
...058, 830,"    ],  },  {    "name": "
---------------------^
Expecting 'STRING', got '}'

Test for yourself here: https://jsonlint.com/.

6. Inconsistent use of lowercase and CamelCase severity.

Lets decide what it should be and consistently apply it.

"low",
"medium",
"high",
"critical",
"moderate",
"important",
"manualreview",
"bestpractice",
"unspecified",
"Critical",
"ManualReview",
"BestPractice"

• Mostly ManualReview is used while here its lowercase: https://github.com/microsoft/DevSkim/blob/9fa526953c914cfbbb883e106b52441d73f360dc/rules/default/security/frameworks/dotnet_framework.json#L142
• 137 times critical and 9 times Critical. Critical seems to be mainly used in the Wiki Samples (3.6 and 3.7) in both repos.
• While most seem lowercase only, BestPractice only occurs 3x in suggested_api.json in CamelCase.

7. Limited example files

I have access to less than 200 files that contain valid DevSkim rules. Having more working examples would severely help properly defining a JSONschema.

8. patterns.type case

232x RegexWord
16x RegexWord

9. patterns.confidence case

9x High
1004x high

10. depends_on_tags

Is depends_on_tags a thing? It isn't used in any rule I could find. Only in the three wiki (3.7) examples. Maybe it needs more documentation and example rules.

11. "recommendation": "", shouldn't be used if its optional

"recommendation": "", is often used (29x) but it should not exist at all if its optional. If it exists it should contain something.

Other findings:

• ApplicationInspector/AppInspector/rules/default/os/acl.json

  Line 69 in c84796a

  ""

• ApplicationInspector/AppInspector/rules/default/cryptography/post_quantum.json

  Line 238 in c84796a

  "_comment": "",

  
  (multiple times in this file)
• like post_quantum "_comment": "" also appears several times in Wiki samples of DevSkim repo and in many other rules.

I'm planning to use the validation scripts and my JSONschema that lead to all these findings as soon as it is ready.

[gfs]

Thanks again for the detailed analysis. Greatly appreciate the feedback!

First, responses to each:

1. I've updated the hedaing to clarify this is a rule for using XPath (structure query against XML) not a rule specified in XML format itself
2. I updated the wiki to enclose the sample rule in [] so it is a valid standalone rule file not just a single rule object.
3. Tags are required for DevSkim but not for ApplicationInspector, there isn't a particularly compelling reason for this I'm aware of, the historical context is that AppInspector was originally a hard fork of DevSkim, I reworked them to maintain roughly compatible rule object formats but different design choices were also made that I maintained - another example is that ApplicationInspector doesn't have support Fixes because the projects scan files with a similar technique but for different purposes.
4. I don't think there's any intentionality behind only that file having BOM. It would make sense to rewrite the file without BOM to match the others.
5. Concur these should be fixed.
6. I think the deserializer is case insensitive, but it would make sense to standardize in at least the examples and rules themselves.
7. I don't think I have any extra rules 'held back' but I could possibly create a maximally specified sample rule that exercises all the fields.
8. Same as 6.
9. Same as 6.
10. Depends on tags should work, see tests here:

    ApplicationInspector/AppInspector.Tests/Commands/TestAnalyzeCmd.cs

    Line 58 in c84796a

    ""depends_on_tags"": [""Category.A""],

    However, this feature was added as a user requested feature and I don't think we use it any included rules.
11. Agree recommendation should be populated if present.

Based on this feedback I believe these ttems require follow up:
4. Remove BOM Marker from rust.json
5. Remove extraneous trailing commas
6. Standardize casing in rules and examples
7. Create maximally specified sample rule/rules
8. Same as 6
9. Same as 6
11. Populate or remove empty recommendations
12. Remove empty comments