Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regex improvements #1348

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 7 additions & 8 deletions src/Ocelot/Authorization/ClaimsAuthorizer.cs
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
using System.Collections.Generic;
using Ocelot.DownstreamRouteFinder.UrlMatcher;
using Ocelot.Infrastructure.Claims.Parser;
using Ocelot.Responses;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.Claims;
using System.Text.RegularExpressions;

using Ocelot.DownstreamRouteFinder.UrlMatcher;

using Ocelot.Infrastructure.Claims.Parser;

using Ocelot.Responses;

namespace Ocelot.Authorization
{
public class ClaimsAuthorizer : IClaimsAuthorizer
{
private readonly IClaimsParser _claimsParser;
private static readonly Regex _regexAuthorize = new Regex(@"^{(?<variable>.+)}$", RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));

public ClaimsAuthorizer(IClaimsParser claimsParser)
{
Expand All @@ -38,7 +37,7 @@ List<PlaceholderNameAndValue> urlPathPlaceholderNameAndValues
if (values.Data != null)
{
// dynamic claim
var match = Regex.Match(required.Value, @"^{(?<variable>.+)}$");
var match = _regexAuthorize.Match(required.Value);
Copy link
Member

@raman-m raman-m Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No catching and processing of the RegexMatchTimeoutException exception.

Will it result to unhandled exception by app domain?
Will it be caught by upper context?

if (match.Success)
{
var variableName = match.Captures[0].Value;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
using System;
using Ocelot.Responses;
using System;
using System.Text.RegularExpressions;

using Ocelot.Responses;

namespace Ocelot.Configuration.Parser
{
public class ClaimToThingConfigurationParser : IClaimToThingConfigurationParser
{
private readonly Regex _claimRegex = new("Claims\\[.*\\]");
private readonly Regex _indexRegex = new("value\\[.*\\]");
private static readonly Regex _claimRegex = new("Claims\\[.*\\]", RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));
private static readonly Regex _indexRegex = new("value\\[.*\\]", RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));
private const char SplitToken = '>';

public Response<ClaimToThing> Extract(string existingKey, string value)
Expand Down Expand Up @@ -57,4 +56,4 @@ private static string GetIndexValue(string instruction)
return claimKey;
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,20 +1,14 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading.Tasks;

using Ocelot.Errors;

using Ocelot.Configuration.File;

using FluentValidation;

using Microsoft.Extensions.DependencyInjection;

using Ocelot.Responses;

using Ocelot.ServiceDiscovery;
using FluentValidation;
using Microsoft.Extensions.DependencyInjection;
using Ocelot.Configuration.File;
using Ocelot.Errors;
using Ocelot.Responses;
using Ocelot.ServiceDiscovery;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading.Tasks;

namespace Ocelot.Configuration.Validator
{
Expand All @@ -23,6 +17,9 @@ public class FileConfigurationFluentValidator : AbstractValidator<FileConfigurat
private const string Servicefabric = "servicefabric";
private readonly List<ServiceDiscoveryFinderDelegate> _serviceDiscoveryFinderDelegates;

private static readonly Regex _regExPlaceholder =
new Regex("{[^}]+}", RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));

public FileConfigurationFluentValidator(IServiceProvider provider, RouteFluentValidator routeFluentValidator, FileGlobalConfigurationFluentValidator fileGlobalConfigurationFluentValidator)
{
_serviceDiscoveryFinderDelegates = provider
Expand Down Expand Up @@ -107,8 +104,7 @@ private static bool AllRoutesForAggregateExist(FileAggregateRoute fileAggregateR

private static bool IsPlaceholderNotDuplicatedIn(string upstreamPathTemplate)
{
var regExPlaceholder = new Regex("{[^}]+}");
var matches = regExPlaceholder.Matches(upstreamPathTemplate);
var matches = _regExPlaceholder.Matches(upstreamPathTemplate);
Copy link
Member

@raman-m raman-m Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No catching and processing of the RegexMatchTimeoutException exception.

Will it result to unhandled exception by app domain?
Will it be caught by upper context?

var upstreamPathPlaceholders = matches.Select(m => m.Value);
return upstreamPathPlaceholders.Count() == upstreamPathPlaceholders.Distinct().Count();
}
Expand Down
33 changes: 15 additions & 18 deletions src/Ocelot/Configuration/Validator/RouteFluentValidator.cs
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;

using Ocelot.Configuration.File;

using FluentValidation;

using FluentValidation;
using Microsoft.AspNetCore.Authentication;
using Ocelot.Configuration.File;
using System;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;

namespace Ocelot.Configuration.Validator
{
public class RouteFluentValidator : AbstractValidator<FileRoute>
{
private readonly IAuthenticationSchemeProvider _authenticationSchemeProvider;
private static readonly Regex _secondsRegEx = new Regex("^[0-9]+s", RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));
private static readonly Regex _minutesRegEx = new Regex("^[0-9]+m", RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));
private static readonly Regex _hoursRegEx = new Regex("^[0-9]+h", RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));
private static readonly Regex _daysRegEx = new Regex("^[0-9]+d", RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));
Comment on lines +15 to +18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcode has detected!
The 100 milliseconds constant should be public static.


public RouteFluentValidator(IAuthenticationSchemeProvider authenticationSchemeProvider, HostAndPortValidator hostAndPortValidator, FileQoSOptionsFluentValidator fileQoSOptionsFluentValidator)
{
Expand Down Expand Up @@ -116,15 +118,10 @@ private static bool IsValidPeriod(FileRateLimitRule rateLimitOptions)

var period = rateLimitOptions.Period;

var secondsRegEx = new Regex("^[0-9]+s");
var minutesRegEx = new Regex("^[0-9]+m");
var hoursRegEx = new Regex("^[0-9]+h");
var daysRegEx = new Regex("^[0-9]+d");

return secondsRegEx.Match(period).Success
|| minutesRegEx.Match(period).Success
|| hoursRegEx.Match(period).Success
|| daysRegEx.Match(period).Success;
return _secondsRegEx.Match(period).Success
|| _minutesRegEx.Match(period).Success
|| _hoursRegEx.Match(period).Success
|| _daysRegEx.Match(period).Success;
Comment on lines +121 to +124
Copy link
Member

@raman-m raman-m Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No catching and processing of the RegexMatchTimeoutException exception.

Will it result to unhandled exception by app domain?
Will it be caught by upper context?

}
}
}
33 changes: 16 additions & 17 deletions src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,24 +1,25 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text.RegularExpressions;

using Ocelot.Configuration.File;

using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Configuration.Memory;

using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Configuration.Memory;
using Newtonsoft.Json;
using Ocelot.Configuration.File;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text.RegularExpressions;

namespace Ocelot.DependencyInjection
{
public static class ConfigurationBuilderExtensions
{
private static readonly Regex _reg = new Regex(@"^ocelot\.(.*?)\.json$", RegexOptions.IgnoreCase | RegexOptions.Singleline | RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));

[Obsolete("Please set BaseUrl in ocelot.json GlobalConfiguration.BaseUrl")]
public static IConfigurationBuilder AddOcelotBaseUrl(this IConfigurationBuilder builder, string baseUrl)
{
Regex.CacheSize += 100;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the default, current value of the cache in runtime?
So, why did you increase this property value?


var memorySource = new MemoryConfigurationSource
{
InitialData = new List<KeyValuePair<string, string>>
Expand All @@ -39,19 +40,17 @@ public static IConfigurationBuilder AddOcelot(this IConfigurationBuilder builder

public static IConfigurationBuilder AddOcelot(this IConfigurationBuilder builder, string folder, IWebHostEnvironment env)
{
Regex.CacheSize += 100;
Copy link
Member

@raman-m raman-m Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you consider the AddOcelot method as an entry point for entire Ocelot pipeline, and for all regex-objects of the app domain?


const string primaryConfigFile = "ocelot.json";

const string globalConfigFile = "ocelot.global.json";

const string subConfigPattern = @"^ocelot\.(.*?)\.json$";

var excludeConfigName = env?.EnvironmentName != null ? $"ocelot.{env.EnvironmentName}.json" : string.Empty;

var reg = new Regex(subConfigPattern, RegexOptions.IgnoreCase | RegexOptions.Singleline);

var files = new DirectoryInfo(folder)
.EnumerateFiles()
.Where(fi => reg.IsMatch(fi.Name) && (fi.Name != excludeConfigName))
.Where(fi => _reg.IsMatch(fi.Name) && (fi.Name != excludeConfigName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No catching and processing of the RegexMatchTimeoutException exception.

.ToArray();

var fileConfiguration = new FileConfiguration();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,37 +1,31 @@
using System;
using System.Collections.Generic;
using System.Text.RegularExpressions;
using System.Threading.Tasks;

using Ocelot.Configuration;

using Ocelot.DownstreamRouteFinder.UrlMatcher;

using Ocelot.Logging;

using Microsoft.AspNetCore.Http;

using Ocelot.Middleware;
using Ocelot.Request.Middleware;

using Ocelot.Responses;

using Ocelot.DownstreamUrlCreator.UrlTemplateReplacer;

using Microsoft.AspNetCore.Http;
using Ocelot.Configuration;
using Ocelot.DownstreamRouteFinder.UrlMatcher;
using Ocelot.DownstreamUrlCreator.UrlTemplateReplacer;
using Ocelot.Logging;
using Ocelot.Middleware;
using Ocelot.Request.Middleware;
using Ocelot.Responses;
using Ocelot.Values;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Text.RegularExpressions;
using System.Threading.Tasks;

namespace Ocelot.DownstreamUrlCreator.Middleware
{
{
public class DownstreamUrlCreatorMiddleware : OcelotMiddleware
{
private readonly RequestDelegate _next;
private readonly IDownstreamPathPlaceholderReplacer _replacer;
private static ConcurrentDictionary<string, Regex> _regex = new ConcurrentDictionary<string, Regex>();

public DownstreamUrlCreatorMiddleware(RequestDelegate next,
IOcelotLoggerFactory loggerFactory,
IDownstreamPathPlaceholderReplacer replacer
)
: base(loggerFactory.CreateLogger<DownstreamUrlCreatorMiddleware>())
)
: base(loggerFactory.CreateLogger<DownstreamUrlCreatorMiddleware>())
{
_next = next;
_replacer = replacer;
Expand Down Expand Up @@ -66,11 +60,12 @@ public async Task Invoke(HttpContext httpContext)

if (ServiceFabricRequest(internalConfiguration, downstreamRoute))
{
var pathAndQuery = CreateServiceFabricUri(downstreamRequest, downstreamRoute, templatePlaceholderNameAndValues, response);
var pathAndQuery = CreateServiceFabricUri(downstreamRequest, downstreamRoute,
templatePlaceholderNameAndValues, response);

//todo check this works again hope there is a test..
downstreamRequest.AbsolutePath = pathAndQuery.Path;
downstreamRequest.Query = pathAndQuery.Query;
downstreamRequest.AbsolutePath = pathAndQuery.Path;
downstreamRequest.Query = pathAndQuery.Query;
Comment on lines +63 to +68
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero changes!
Revert to previous code!

}
else
{
Expand All @@ -91,7 +86,8 @@ public async Task Invoke(HttpContext httpContext)
}
else
{
RemoveQueryStringParametersThatHaveBeenUsedInTemplate(downstreamRequest, templatePlaceholderNameAndValues);
RemoveQueryStringParametersThatHaveBeenUsedInTemplate(downstreamRequest,
templatePlaceholderNameAndValues);
Comment on lines +89 to +90
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this!


downstreamRequest.AbsolutePath = dsPath.Value;
}
Expand All @@ -102,20 +98,25 @@ public async Task Invoke(HttpContext httpContext)
await _next.Invoke(httpContext);
}

private static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(DownstreamRequest downstreamRequest, List<PlaceholderNameAndValue> templatePlaceholderNameAndValues)
private static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(
DownstreamRequest downstreamRequest, List<PlaceholderNameAndValue> templatePlaceholderNameAndValues)
Comment on lines +101 to +102
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was that changed? 😉
There is no need to change! No new code!

{
foreach (var nAndV in templatePlaceholderNameAndValues)
{
var name = nAndV.Name.Replace("{", string.Empty).Replace("}", string.Empty);
var name = nAndV.Name.Replace("{", string.Empty).Replace("}", string.Empty);

if (downstreamRequest.Query.Contains(name) &&
downstreamRequest.Query.Contains(nAndV.Value))
{
var questionMarkOrAmpersand = downstreamRequest.Query.IndexOf(name, StringComparison.Ordinal);
downstreamRequest.Query = downstreamRequest.Query.Remove(questionMarkOrAmpersand - 1, 1);

var rgx = new Regex($@"\b{name}={nAndV.Value}\b");
downstreamRequest.Query = rgx.Replace(downstreamRequest.Query, string.Empty);
var pattern = $@"\b{name}={nAndV.Value}\b";
var regex = _regex.AddOrUpdate(pattern,
new Regex(pattern, RegexOptions.Compiled, TimeSpan.FromMilliseconds(100)),
(key, oldValue) => oldValue);
Comment on lines +115 to +117
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No catching and processing of the RegexMatchTimeoutException exception.


downstreamRequest.Query = regex.Replace(downstreamRequest.Query, string.Empty);

if (!string.IsNullOrEmpty(downstreamRequest.Query))
{
Expand All @@ -125,22 +126,24 @@ private static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(Downst
}
}

private static string GetPath(DownstreamPath dsPath)
private static string GetPath(DownstreamPath dsPath)
{
return dsPath.Value.Substring(0, dsPath.Value.IndexOf('?', StringComparison.Ordinal));
return dsPath.Value.Substring(0, dsPath.Value.IndexOf('?', StringComparison.Ordinal));
}

private static string GetQueryString(DownstreamPath dsPath)
private static string GetQueryString(DownstreamPath dsPath)
{
return dsPath.Value.Substring(dsPath.Value.IndexOf('?', StringComparison.Ordinal));
return dsPath.Value.Substring(dsPath.Value.IndexOf('?', StringComparison.Ordinal));
}

private static bool ContainsQueryString(DownstreamPath dsPath)
private static bool ContainsQueryString(DownstreamPath dsPath)
{
return dsPath.Value.Contains('?');
return dsPath.Value.Contains('?');
}
Comment on lines +129 to 142
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wonder...
What are the changes? 🤣


private (string Path, string Query) CreateServiceFabricUri(DownstreamRequest downstreamRequest, DownstreamRoute downstreamRoute, List<PlaceholderNameAndValue> templatePlaceholderNameAndValues, Response<DownstreamPath> dsPath)
private (string Path, string Query) CreateServiceFabricUri(DownstreamRequest downstreamRequest,
DownstreamRoute downstreamRoute, List<PlaceholderNameAndValue> templatePlaceholderNameAndValues,
Response<DownstreamPath> dsPath)
Comment on lines +144 to +146
Copy link
Member

@raman-m raman-m Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see zero changes in the method signature!
But, If you split parameters, I suggest to put each parameter onto new line 👇

Suggested change
private (string Path, string Query) CreateServiceFabricUri(DownstreamRequest downstreamRequest,
DownstreamRoute downstreamRoute, List<PlaceholderNameAndValue> templatePlaceholderNameAndValues,
Response<DownstreamPath> dsPath)
private (string Path, string Query) CreateServiceFabricUri(
DownstreamRequest downstreamRequest,
DownstreamRoute downstreamRoute,
List<PlaceholderNameAndValue> templatePlaceholderNameAndValues,
Response<DownstreamPath> dsPath)

{
var query = downstreamRequest.Query;
var serviceName = _replacer.Replace(downstreamRoute.ServiceName, templatePlaceholderNameAndValues);
Expand All @@ -150,7 +153,8 @@ private static bool ContainsQueryString(DownstreamPath dsPath)

private static bool ServiceFabricRequest(IInternalConfiguration config, DownstreamRoute downstreamRoute)
{
return config.ServiceProviderConfiguration.Type?.ToLower() == "servicefabric" && downstreamRoute.UseServiceDiscovery;
return config.ServiceProviderConfiguration.Type?.ToLower() == "servicefabric" &&
downstreamRoute.UseServiceDiscovery;
Comment on lines +156 to +157
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero changes!
Revert this fake change plz!

}
}
}