Skip to content

Commit

Permalink
Fixes a CodeQL Finding on a null reference error (#496)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gfs
gfs authored Aug 10, 2022
1 parent dce5493 commit 5e419fe
Show file tree
Hide file tree
Showing 10 changed files with 117 additions and 126 deletions.
1 change: 0 additions & 1 deletion AppInspector.CLI/CLICmdOptions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ namespace Microsoft.ApplicationInspector.CLI
using CommandLine;
using Microsoft.ApplicationInspector.Commands;
using Microsoft.ApplicationInspector.RulesEngine;
using System;
using System.Collections.Generic;

/// <summary>
Expand Down
43 changes: 19 additions & 24 deletions AppInspector.RulesEngine/AbstractRuleSet.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
using System;
using System.Collections.Generic;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
Expand Down Expand Up @@ -73,13 +72,20 @@ public IEnumerable<ConvertedOatRule> GetUniversalRules()
var expression = new StringBuilder("(");
foreach (var pattern in rule.Patterns)
{
clauses.Add(GenerateClause(pattern, clauseNumber));
if (clauseNumber > 0)
if (GenerateClause(pattern, clauseNumber) is { } clause)
{
expression.Append(" OR ");
clauses.Add(clause);
if (clauseNumber > 0)
{
expression.Append(" OR ");
}
expression.Append(clauseNumber);
clauseNumber++;
}
else
{
_logger.LogWarning("Clause could not be generated from pattern {pattern}", pattern.Pattern);
}
expression.Append(clauseNumber);
clauseNumber++;
}

if (clauses.Count > 0)
Expand Down Expand Up @@ -122,12 +128,10 @@ public IEnumerable<ConvertedOatRule> GetUniversalRules()
{
if (condition.SearchIn?.Equals("finding-only", StringComparison.InvariantCultureIgnoreCase) != false)
{
return new WithinClause()
return new WithinClause(subClause)
{
Label = clauseNumber.ToString(CultureInfo.InvariantCulture),
FindingOnly = true,
CustomOperation = "Within",
SubClause = subClause,
Invert = condition.NegateFinding
};
}
Expand All @@ -151,56 +155,49 @@ public IEnumerable<ConvertedOatRule> GetUniversalRules()
}
if (argList.Count == 2)
{
return new WithinClause()
return new WithinClause(subClause)
{
Label = clauseNumber.ToString(CultureInfo.InvariantCulture),
FindingRegion = true,
CustomOperation = "Within",
Before = argList[0],
After = argList[1],
SubClause = subClause,
Invert = condition.NegateFinding
};
}
}
else if (condition.SearchIn.Equals("same-line", StringComparison.InvariantCultureIgnoreCase))
{
return new WithinClause()
return new WithinClause(subClause)
{
Label = clauseNumber.ToString(CultureInfo.InvariantCulture),
SameLineOnly = true,
CustomOperation = "Within",
SubClause = subClause,
Invert = condition.NegateFinding
};
}
else if (condition.SearchIn.Equals("same-file", StringComparison.InvariantCultureIgnoreCase))
{
return new WithinClause()
return new WithinClause(subClause)
{
Label = clauseNumber.ToString(CultureInfo.InvariantCulture),
SameFile = true,
SubClause = subClause,
Invert = condition.NegateFinding
};
}
else if (condition.SearchIn.Equals("only-before", StringComparison.InvariantCultureIgnoreCase))
{
return new WithinClause()
return new WithinClause(subClause)
{
Label = clauseNumber.ToString(CultureInfo.InvariantCulture),
OnlyBefore = true,
SubClause = subClause,
Invert = condition.NegateFinding
};
}
else if (condition.SearchIn.Equals("only-after", StringComparison.InvariantCultureIgnoreCase))
{
return new WithinClause()
return new WithinClause(subClause)
{
Label = clauseNumber.ToString(CultureInfo.InvariantCulture),
OnlyAfter = true,
SubClause = subClause,
Invert = condition.NegateFinding
};
}
Expand Down Expand Up @@ -238,8 +235,7 @@ public IEnumerable<ConvertedOatRule> GetUniversalRules()
.InvariantCulture), //important to pattern index identification
Data = new List<string>() { pattern.Pattern },
Capture = true,
Arguments = modifiers,
CustomOperation = "RegexWithIndex"
Arguments = modifiers
};
}
else if (pattern.PatternType == PatternType.RegexWord)
Expand All @@ -250,7 +246,6 @@ public IEnumerable<ConvertedOatRule> GetUniversalRules()
Data = new List<string>() { $"\\b({pattern.Pattern})\\b" },
Capture = true,
Arguments = pattern.Modifiers?.ToList() ?? new List<string>(),
CustomOperation = "RegexWithIndex"
};
}
}
Expand Down
2 changes: 0 additions & 2 deletions AppInspector.RulesEngine/OatExtensions/OatRegexWithIndexOperation.cs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Text.RegularExpressions;
using Microsoft.CST.OAT;
using Microsoft.CST.OAT.Operations;
Expand Down
5 changes: 3 additions & 2 deletions AppInspector.RulesEngine/OatExtensions/WithinClause.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ namespace Microsoft.ApplicationInspector.RulesEngine.OatExtensions
{
public class WithinClause : Clause
{
public WithinClause(string? field = null) : base(Operation.Custom, field)
public WithinClause(Clause subClause, string? field = null) : base(Operation.Custom, field)
{
SubClause = subClause;
CustomOperation = "Within";
}

Expand All @@ -19,6 +20,6 @@ public WithinClause(string? field = null) : base(Operation.Custom, field)
public bool FindingOnly { get; set; }
public bool SameLineOnly { get; set; }
public bool FindingRegion { get; set; }
public Clause SubClause { get; set; }
public Clause SubClause { get; }
}
}
45 changes: 29 additions & 16 deletions AppInspector.RulesEngine/OatExtensions/WithinOperation.cs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Text.RegularExpressions;
using Microsoft.CST.OAT;
using Microsoft.CST.OAT.Operations;
using Microsoft.Extensions.Logging;
Expand All @@ -15,8 +13,7 @@ public class WithinOperation : OatOperation
{
public WithinOperation(Analyzer analyzer, ILoggerFactory? loggerFactory = null) : base(Operation.Custom, analyzer)
{
_loggerFactory = loggerFactory ?? new NullLoggerFactory();
_regexEngine = new RegexOperation(analyzer);
_loggerFactory = loggerFactory ?? NullLoggerFactory.Instance;
_analyzer = analyzer;
CustomOperation = "Within";
OperationDelegate = WithinOperationDelegate;
Expand All @@ -32,7 +29,7 @@ public OperationResult WithinOperationDelegate(Clause c, object? state1, object?
List<(int, Boundary)> failed =
new List<(int, Boundary)>();

foreach (var capture in captures)
foreach (var capture in captures ?? Array.Empty<ClauseCapture>())
{
if (capture is TypedClauseCapture<List<(int, Boundary)>> tcc)
{
Expand Down Expand Up @@ -80,7 +77,6 @@ public OperationResult WithinOperationDelegate(Clause c, object? state1, object?
Index = startInner,
Length = (endInner - startInner) + 1
};
var theText = tc.GetBoundaryText(bound);
return bound;
}

Expand Down Expand Up @@ -131,7 +127,7 @@ OperationResult ProcessLambda(Boundary target)
return _analyzer.GetClauseCapture(wc.SubClause, tc, target, captures);
}
}
return new OperationResult(false, null);
return new OperationResult(false);
}

public IEnumerable<Violation> WithinValidationDelegate(CST.OAT.Rule rule, Clause clause)
Expand Down Expand Up @@ -166,16 +162,11 @@ public IEnumerable<Violation> WithinValidationDelegate(CST.OAT.Rule rule, Clause
rule, clause);
}
}
if (wc.Data?.Any() ?? false)
if (wc.Data.Any())
{
yield return new Violation($"Don't provide data directly. Instead use SubClause..", rule, clause);
yield return new Violation($"Don't provide data directly. Instead use SubClause.", rule, clause);
}

foreach (var datum in wc.Data ?? new List<string>())
{
yield return new Violation($"Data in WithinClause is ignored. Use SubClause. Data {datum} found in Rule {rule.Name} Clause {clause.Label ?? rule.Clauses.IndexOf(clause).ToString(CultureInfo.InvariantCulture)} is not a valid regex.", rule, clause);
}

var subOp = _analyzer
.GetOperation(wc.SubClause.Key.Operation, wc.SubClause.Key.CustomOperation);

Expand All @@ -189,16 +180,38 @@ public IEnumerable<Violation> WithinValidationDelegate(CST.OAT.Rule rule, Clause
{
yield return violation;
}

if (wc.SubClause is OatRegexWithIndexClause oatRegexWithIndexClause)
{
if ((oatRegexWithIndexClause.JsonPaths?.Any() ?? false) ||
(oatRegexWithIndexClause.XPaths?.Any() ?? false))
{
if (wc.FindingOnly || wc.SameLineOnly || wc.FindingRegion || wc.OnlyAfter || wc.OnlyBefore)
{
yield return new Violation($"When providing JSONPaths or XPaths must use same-file region.", rule, clause);
}
}
}
if (wc.SubClause is OatSubstringIndexClause oatSubstringIndexClause)
{
if ((oatSubstringIndexClause.JsonPaths?.Any() ?? false) ||
(oatSubstringIndexClause.XPaths?.Any() ?? false))
{
if (wc.FindingOnly || wc.SameLineOnly || wc.FindingRegion || wc.OnlyAfter || wc.OnlyBefore)
{
yield return new Violation($"When providing JSONPaths or XPaths must use same-file region.", rule, clause);
}
}
}
}

}
else
{
yield return new Violation($"Rule {rule.Name ?? "Null Rule Name"} clause {clause.Label ?? rule.Clauses.IndexOf(clause).ToString(CultureInfo.InvariantCulture)} is not a WithinClause", rule, clause);
yield return new Violation($"Rule {rule.Name} clause {clause.Label ?? rule.Clauses.IndexOf(clause).ToString(CultureInfo.InvariantCulture)} is not a WithinClause", rule, clause);
}
}

private readonly RegexOperation _regexEngine;
private readonly ILoggerFactory _loggerFactory;
private readonly Analyzer _analyzer;
}
Expand Down
Loading

0 comments on commit 5e419fe

Please sign in to comment.