Skip to content

Commit

Permalink
Improve speed of hard-coded url check for linter and fix span highlig…
Browse files Browse the repository at this point in the history 
…hting (#3662)

* Benchmark!

* Default diagnostics not overridden by config are created once and not expected to change.

* Optimize to reduce regex calls and fix to only call base visit once

* Fix span mapping for hardcoded url's

* Finalize hardcoded url test and optimization

* remove override config call  - not needed

reformat is incidental

* Simplify config override logic in linter analyzer

* reset global config ignoring diag

* Update regex to handle braces when encountered in string tokens

* eliminate CI warning for use of non-secure Random()

* fix index for crypto random number max value

Co-authored-by: Anthony Martin <[email protected]>
Co-authored-by: jofleish <[email protected]>
  • Loading branch information
@jfleisher @anthony-c-martin
3 people authored Jul 20, 2021
1 parent 4c7d000 commit 6e7cd63
Show file tree
Hide file tree
Showing 6 changed files with 159 additions and 88 deletions.
39 changes: 39 additions & 0 deletions src/Bicep.Core.IntegrationTests/ScenarioTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.
using System;
using System.Linq;
using System.Security.Cryptography;
using System.Xml.Linq;
using Bicep.Core.Analyzers.Linter.Rules;
using Bicep.Core.Diagnostics;
Expand All @@ -20,6 +21,44 @@ namespace Bicep.Core.IntegrationTests
[TestClass]
public class ScenarioTests
{
[TestMethod]
// https://github.com/azure/bicep/issues/3636
public void Test_Issue3636()
{
var lineCount = 100; // increase this number to 10,000 for more intense test

// use this crypto random number gen to avoid CI warning
int generateRandomInt(int minVal = 0, int maxVal = 50)
{
var rnd = new byte[4];
using var rng = new RNGCryptoServiceProvider();
rng.GetBytes(rnd);
var i = Math.Abs(BitConverter.ToInt32(rnd, 0));
return Convert.ToInt32(i % (maxVal - minVal + 1) + minVal);
}
Random random = new Random();

string randomString()
{
const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
return new string(Enumerable.Repeat(chars, generateRandomInt())
.Select(s => s[generateRandomInt(0, s.Length-1)]).ToArray());
}

var file = "param adminuser string\nvar adminstring = 'xyx ${adminuser} 123'\n";
for (var i = 0; i < lineCount; i++)
{
file += $"output testa{i} string = '{randomString()} ${{adminuser}} {randomString()}'\n";
file += $"output testb{i} string = '{randomString()} ${{adminstring}} {randomString()}'\n";
}

// not a true test for existing diagnostics
// this is a trigger to allow timing within the
// linter rules - timing must be readded to
// initially added to time NoHardcodedEnvironmentUrlsRule
CompilationHelper.Compile(file).Should().NotHaveAnyDiagnostics();
}

[TestMethod]
// https://github.com/azure/bicep/issues/746
public void Test_Issue746()
Expand Down
7 changes: 6 additions & 1 deletion src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/NoHardcodedEnvironmentUrlsRuleTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,12 @@ param p2 string
[DataRow(1, @"
param p1 string
param p2 string
var a = '${p1}azuredatalakestore.net${p2}'
var a = '${p1} azuredatalakestore.net${p2}'
")]
[DataRow(1, @"
param p1 string
param p2 string
var a = '${p1} azuredatalakestore.net$ {p2}'
")]
[DataTestMethod]
public void InsideStringInterpolation(int diagnosticCount, string text)
Expand Down
34 changes: 14 additions & 20 deletions src/Bicep.Core/Analyzers/Linter/LinterAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ public class LinterAnalyzer : IBicepAnalyzer
public static string LinterEnabledSetting => $"{SettingsRoot}:{AnalyzerName}:enabled";
public static string LinterVerboseSetting => $"{SettingsRoot}:{AnalyzerName}:verbose";

private ConfigHelper configHelper = new ConfigHelper();
private readonly ConfigHelper defaultConfigHelper = new ConfigHelper();
private ConfigHelper activeConfigHelper;
private ImmutableArray<IBicepAnalyzerRule> RuleSet;
private ImmutableArray<IDiagnostic> RuleCreationErrors;

Expand All @@ -32,11 +33,12 @@ public class LinterAnalyzer : IBicepAnalyzer

public LinterAnalyzer()
{
this.activeConfigHelper = this.defaultConfigHelper;
(RuleSet, RuleCreationErrors) = CreateLinterRules();
}

private bool LinterEnabled => this.configHelper.GetValue(LinterEnabledSetting, true);
private bool LinterVerbose => this.configHelper.GetValue(LinterVerboseSetting, true);
private bool LinterEnabled => this.activeConfigHelper.GetValue(LinterEnabledSetting, true);
private bool LinterVerbose => this.activeConfigHelper.GetValue(LinterVerboseSetting, true);

private (ImmutableArray<IBicepAnalyzerRule> rules, ImmutableArray<IDiagnostic> errors) CreateLinterRules()
{
Expand Down Expand Up @@ -77,21 +79,23 @@ internal IEnumerable<IDiagnostic> Analyze(SemanticModel semanticModel, ConfigHel
{
// check for configuration overrides
/// typically only used in unit tests
var configHelp = overrideConfig ?? this.configHelper;
this.activeConfigHelper = overrideConfig ?? this.defaultConfigHelper;

var diagnostics = new List<IDiagnostic>();

try
{
configHelp.LoadConfigurationForSourceFile(semanticModel.SourceFile.FileUri);
this.activeConfigHelper.LoadConfigurationForSourceFile(semanticModel.SourceFile.FileUri);
}
catch (Exception ex)
{
diagnostics.Add(CreateInternalErrorDiagnostic(AnalyzerName, ex.InnerException?.Message ?? ex.Message));
// Build a default config to continue with. This should not fail.
configHelp.LoadDefaultConfiguration();
this.activeConfigHelper.LoadDefaultConfiguration();
}
this.RuleSet.ForEach(r => r.Configure(configHelp.Config));

this.RuleSet.ForEach(r => r.Configure(this.activeConfigHelper.Config));

if (this.LinterEnabled)
{
// Add diaagnostics for rules that failed to load
Expand All @@ -115,7 +119,7 @@ internal IEnumerable<IDiagnostic> Analyze(SemanticModel semanticModel, ConfigHel
new TextSpan(0, 0),
DiagnosticLevel.Info,
"Linter Disabled",
string.Format(CoreResources.LinterDisabledFormatMessage, this.configHelper.CustomSettingsFileName),
string.Format(CoreResources.LinterDisabledFormatMessage, this.activeConfigHelper.CustomSettingsFileName),
null, null));
}
}
Expand All @@ -124,9 +128,9 @@ internal IEnumerable<IDiagnostic> Analyze(SemanticModel semanticModel, ConfigHel

private IDiagnostic GetConfigurationDiagnostic()
{
var configMessage = this.configHelper.CustomSettingsFileName == default ?
var configMessage = this.activeConfigHelper.CustomSettingsFileName == default ?
CoreResources.BicepConfigNoCustomSettingsMessage
: string.Format(CoreResources.BicepConfigCustomSettingsFoundFormatMessage, this.configHelper.CustomSettingsFileName);
: string.Format(CoreResources.BicepConfigCustomSettingsFoundFormatMessage, this.activeConfigHelper.CustomSettingsFileName);

return new AnalyzerDiagnostic(AnalyzerName,
new TextSpan(0, 0),
Expand All @@ -136,16 +140,6 @@ private IDiagnostic GetConfigurationDiagnostic()
null, null);
}

/// <summary>
/// Internal method intended to allow easy configuration
/// override in Unit Testing
/// </summary>
/// <param name="overrideConfig"></param>
internal void OverrideConfig(ConfigHelper overrideConfig)
{
this.configHelper = overrideConfig;
}

internal IDiagnostic CreateInternalErrorDiagnostic(string analyzerName, string message)
{
return new AnalyzerDiagnostic(
Expand Down
69 changes: 45 additions & 24 deletions src/Bicep.Core/Analyzers/Linter/Rules/NoHardcodedEnvironmentUrlsRule.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using Bicep.Core.Parsing;
using Bicep.Core.Semantics;
using Bicep.Core.Syntax;
using Bicep.Core.Text;
using Microsoft.Extensions.Configuration;
using System;
using System.Collections.Generic;
Expand All @@ -20,7 +19,10 @@ public sealed class NoHardcodedEnvironmentUrlsRule : LinterRuleBase
public new const string Code = "no-hardcoded-env-urls";

public ImmutableArray<string>? DisallowedHosts;

private ImmutableArray<string>? ExcludedHosts;
private int MinimumHostLength;
private bool HasHosts;

private readonly Lazy<Regex> hostRegexLazy;
private Regex hostRegex => hostRegexLazy.Value;
Expand All @@ -40,10 +42,14 @@ public NoHardcodedEnvironmentUrlsRule() : base(
public override void Configure(IConfigurationRoot config)
{
base.Configure(config);
this.DisallowedHosts = GetArray(nameof(DisallowedHosts).ToLower(), Array.Empty<string>())

this.DisallowedHosts = GetArray(nameof(DisallowedHosts), Array.Empty<string>())
.ToImmutableArray();
this.ExcludedHosts = GetArray(nameof(ExcludedHosts).ToLower(), Array.Empty<string>())
this.MinimumHostLength = this.DisallowedHosts.Value.Min(h => h.Length);
this.ExcludedHosts = GetArray(nameof(ExcludedHosts), Array.Empty<string>())
.ToImmutableArray();

this.HasHosts = this.DisallowedHosts?.Any() ?? false;
}

public override string FormatMessage(params object[] values)
Expand All @@ -57,60 +63,75 @@ public override string FormatMessage(params object[] values)
/// </summary>
/// <returns></returns>
public Regex CreateDisallowedHostRegex() =>
new Regex(string.Join('|', this.DisallowedHosts!.Value.Select(h => $@"(?<=\.|\s|^|/){Regex.Escape(h)}")),
new Regex(string.Join('|', this.DisallowedHosts!.Value.Select(h => $@"(?<=\.|'|{{|}}|\s|^|/){Regex.Escape(h)}")),
RegexOptions.Compiled | RegexOptions.IgnoreCase);

public Regex CreateExcludedHostsRegex() =>
new Regex(string.Join('|', this.ExcludedHosts!.Value.Select(h => $@"(?<=\.|\s|^|/){Regex.Escape(h)}")),
new Regex(string.Join('|', this.ExcludedHosts!.Value.Select(h => $@"(?<=\.|'|{{|}}|\s|^|/){Regex.Escape(h)}")),
RegexOptions.Compiled | RegexOptions.IgnoreCase);

public override IEnumerable<IDiagnostic> AnalyzeInternal(SemanticModel model)
{
if (this.DisallowedHosts.HasValue && this.DisallowedHosts.Value.Any())
if (HasHosts)
{
var visitor = new Visitor(this.hostRegex, this.excludedRegex);
var visitor = new Visitor(this.hostRegex, this.MinimumHostLength, this.excludedRegex);
visitor.Visit(model.SourceFile.ProgramSyntax);
return visitor.DisallowedHostSpans.Select(entry => CreateDiagnosticForSpan(entry.Key, entry.Value));
}

return Enumerable.Empty<IDiagnostic>();
}

private class Visitor : SyntaxVisitor
{
public readonly Dictionary<TextSpan, string> DisallowedHostSpans = new Dictionary<TextSpan, string>();
private readonly Regex hostRegex;
private readonly int minHostLen;
private readonly Regex exclusionRegex;

public Visitor(Regex disallowedRegex, Regex exclusionRegex)
public Visitor(Regex disallowedRegex, int minHostLen, Regex exclusionRegex)
{
this.hostRegex = disallowedRegex;
this.minHostLen = minHostLen;
this.exclusionRegex = exclusionRegex;
}

public override void VisitStringSyntax(StringSyntax syntax)
{
foreach (var segment in syntax.SegmentValues)
// shortcut check by testing length of full span
if (syntax.Span.Length > minHostLen)
{
var exclusionMatches = exclusionRegex.Matches(segment);

// does this segment have a host match
foreach (Match match in this.hostRegex.Matches(segment))
foreach (var token in syntax.StringTokens)
{
// exclusion is found containing the host match
var isExcluded = exclusionMatches.Any(exclusionMatch =>
match.Index > exclusionMatch.Index
&& match.Index + match.Length <= exclusionMatch.Index + exclusionMatch.Length);

if (!isExcluded)
// shortcut check by testing length of token
if (token.Text.Length >= minHostLen)
{
// create a span for the specific identified instance
// to allow for multiple instances in a single syntax
this.DisallowedHostSpans[new TextSpan(syntax.Span.Position+match.Index, match.Length)] = match.Value;
var disallowedMatches = this.hostRegex.Matches(token.Text);

if (disallowedMatches.Any())
{
var exclusionMatches = exclusionRegex.Matches(token.Text);

// does this segment have a host match
foreach (Match match in disallowedMatches)
{

// exclusion is found containing the host match
var isExcluded = exclusionMatches.Any(exclusionMatch =>
match.Index > exclusionMatch.Index
&& match.Index + match.Length <= exclusionMatch.Index + exclusionMatch.Length);

if (!isExcluded)
{
// create a span for the specific identified instance
// to allow for multiple instances in a single syntax
this.DisallowedHostSpans[new TextSpan(token.Span.Position + match.Index, match.Length)] = match.Value;
}
}
}
}
}
base.VisitStringSyntax(syntax);
}
base.VisitStringSyntax(syntax);
}
}
}
Expand Down
Loading

0 comments on commit 6e7cd63

Please sign in to comment.