From 65e7f7f44b5bf86d11e86e3cee70b85c56f23e76 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Wed, 31 Aug 2016 14:37:33 -0700 Subject: [PATCH] Removes startsWith in favor of array index, guarantee that string is non-empty in someplaces --- .../Internal/CodeRules/RedirectRule.cs | 24 ++++++-- .../Internal/CodeRules/RewriteRule.cs | 34 +++++++++--- .../Internal/ModRewrite/FlagParser.cs | 3 +- .../Internal/ModRewrite/RuleRegexParser.cs | 2 +- .../Internal/UrlActions/RedirectAction.cs | 9 ++- .../Internal/UrlActions/RewriteAction.cs | 17 ++++-- .../UrlRewrite/UrlRewriteFileParser.cs | 13 ++++- .../UrlRewrite/UrlRewriteRuleBuilder.cs | 2 +- .../CodeRules/MiddlewareTests.cs | 35 ++++++++++++ .../ModRewrite/ModRewriteMiddlewareTest.cs | 38 +++++++++++++ .../FormatExceptionHandlingTests.cs | 10 ++++ .../UrlRewrite/MiddleWareTests.cs | 55 +++++++++++++++++++ 12 files changed, 218 insertions(+), 24 deletions(-) diff --git a/src/Microsoft.AspNetCore.Rewrite/Internal/CodeRules/RedirectRule.cs b/src/Microsoft.AspNetCore.Rewrite/Internal/CodeRules/RedirectRule.cs index 7c33bd426f..90ddf74273 100644 --- a/src/Microsoft.AspNetCore.Rewrite/Internal/CodeRules/RedirectRule.cs +++ b/src/Microsoft.AspNetCore.Rewrite/Internal/CodeRules/RedirectRule.cs @@ -16,6 +16,16 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.CodeRules public int StatusCode { get; } public RedirectRule(string regex, string replacement, int statusCode) { + if (string.IsNullOrEmpty(regex)) + { + throw new ArgumentNullException(nameof(regex)); + } + + if (string.IsNullOrEmpty(replacement)) + { + throw new ArgumentNullException(nameof(replacement)); + } + InitialMatch = new Regex(regex, RegexOptions.Compiled | RegexOptions.CultureInvariant, _regexTimeout); Replacement = replacement; StatusCode = statusCode; @@ -38,9 +48,17 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.CodeRules { var newPath = initMatchResults.Result(Replacement); var response = context.HttpContext.Response; - response.StatusCode = StatusCode; - if (newPath.IndexOf("://", StringComparison.Ordinal) == -1 && !newPath.StartsWith("/")) + response.StatusCode = StatusCode; + context.Result = RuleTermination.ResponseComplete; + + if (string.IsNullOrEmpty(newPath)) + { + response.Headers[HeaderNames.Location] = "/"; + return; + } + + if (newPath.IndexOf("://", StringComparison.Ordinal) == -1 && newPath[0] != '/') { newPath = '/' + newPath; } @@ -58,8 +76,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.CodeRules { response.Headers[HeaderNames.Location] = newPath; } - - context.Result = RuleTermination.ResponseComplete; } } } diff --git a/src/Microsoft.AspNetCore.Rewrite/Internal/CodeRules/RewriteRule.cs b/src/Microsoft.AspNetCore.Rewrite/Internal/CodeRules/RewriteRule.cs index dd82c6fcde..d930a2b279 100644 --- a/src/Microsoft.AspNetCore.Rewrite/Internal/CodeRules/RewriteRule.cs +++ b/src/Microsoft.AspNetCore.Rewrite/Internal/CodeRules/RewriteRule.cs @@ -10,13 +10,22 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.CodeRules { public class RewriteRule : Rule { - private readonly string ForwardSlash = "/"; private readonly TimeSpan _regexTimeout = TimeSpan.FromSeconds(1); public Regex InitialMatch { get; } public string Replacement { get; } public bool StopProcessing { get; } public RewriteRule(string regex, string replacement, bool stopProcessing) { + if (string.IsNullOrEmpty(regex)) + { + throw new ArgumentNullException(nameof(regex)); + } + + if (string.IsNullOrEmpty(replacement)) + { + throw new ArgumentNullException(nameof(replacement)); + } + InitialMatch = new Regex(regex, RegexOptions.Compiled | RegexOptions.CultureInvariant, _regexTimeout); Replacement = replacement; StopProcessing = stopProcessing; @@ -39,6 +48,17 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.CodeRules { var result = initMatchResults.Result(Replacement); var request = context.HttpContext.Request; + + if (StopProcessing) + { + context.Result = RuleTermination.StopRules; + } + + if (string.IsNullOrEmpty(result)) + { + result = "/"; + } + if (result.IndexOf("://", StringComparison.Ordinal) >= 0) { string scheme; @@ -59,13 +79,13 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.CodeRules if (split >= 0) { var newPath = result.Substring(0, split); - if (newPath.StartsWith(ForwardSlash)) + if (newPath[0] == '/') { request.Path = PathString.FromUriComponent(newPath); } else { - request.Path = PathString.FromUriComponent(ForwardSlash + newPath); + request.Path = PathString.FromUriComponent('/' + newPath); } request.QueryString = request.QueryString.Add( QueryString.FromUriComponent( @@ -73,20 +93,16 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.CodeRules } else { - if (result.StartsWith(ForwardSlash)) + if (result[0] == '/') { request.Path = PathString.FromUriComponent(result); } else { - request.Path = PathString.FromUriComponent(ForwardSlash + result); + request.Path = PathString.FromUriComponent('/' + result); } } } - if (StopProcessing) - { - context.Result = RuleTermination.StopRules; - } } } } diff --git a/src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/FlagParser.cs b/src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/FlagParser.cs index 265c4acf22..0888f614ab 100644 --- a/src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/FlagParser.cs +++ b/src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/FlagParser.cs @@ -63,7 +63,8 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite } // Check that flags are contained within [] - if (!(flagString.StartsWith("[") && flagString.EndsWith("]"))) + // Guaranteed to have a length of at least 1 here, so this will never throw for indexing. + if (!(flagString[0] == '[' && flagString[flagString.Length - 1] == ']')) { throw new FormatException("Flags should start and end with square brackets: [flags]"); } diff --git a/src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/RuleRegexParser.cs b/src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/RuleRegexParser.cs index bdc6273d02..e9cbb8a36c 100644 --- a/src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/RuleRegexParser.cs +++ b/src/Microsoft.AspNetCore.Rewrite/Internal/ModRewrite/RuleRegexParser.cs @@ -13,7 +13,7 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.ModRewrite { throw new FormatException("Regex expression is null"); } - if (regex.StartsWith("!")) + if (regex[0] == '!') { return new ParsedModRewriteInput { Invert = true, Operand = regex.Substring(1) }; } diff --git a/src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RedirectAction.cs b/src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RedirectAction.cs index 9e6b636205..dfac28fc80 100644 --- a/src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RedirectAction.cs +++ b/src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RedirectAction.cs @@ -53,7 +53,14 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions pattern = Uri.EscapeDataString(pattern); } - if (pattern.IndexOf("://", StringComparison.Ordinal) == -1 && !pattern.StartsWith("/")) + if (string.IsNullOrEmpty(pattern)) + { + response.Headers[HeaderNames.Location] = "/"; + return; + } + + + if (pattern.IndexOf("://", StringComparison.Ordinal) == -1 && pattern[0] != '/') { pattern = '/' + pattern; } diff --git a/src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RewriteAction.cs b/src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RewriteAction.cs index efb8468ed3..92a4b9c070 100644 --- a/src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RewriteAction.cs +++ b/src/Microsoft.AspNetCore.Rewrite/Internal/UrlActions/RewriteAction.cs @@ -9,7 +9,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions { public class RewriteAction : UrlAction { - private readonly string ForwardSlash = "/"; public RuleTermination Result { get; } public bool QueryStringAppend { get; } public bool QueryStringDelete { get; } @@ -22,6 +21,8 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions bool queryStringDelete, bool escapeBackReferences) { + // For the replacement, we must have at least + // one segment (cannot have an empty replacement) Result = result; Url = pattern; QueryStringAppend = queryStringAppend; @@ -47,12 +48,18 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions var pattern = Url.Evaluate(context, ruleMatch, condMatch); var request = context.HttpContext.Request; + if (string.IsNullOrEmpty(pattern)) + { + pattern = "/"; + } + if (EscapeBackReferences) { // because escapebackreferences will be encapsulated by the pattern, just escape the pattern pattern = Uri.EscapeDataString(pattern); } + // TODO PERF, substrings, object creation, etc. if (pattern.IndexOf("://", StringComparison.Ordinal) >= 0) { @@ -89,13 +96,13 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions if (split >= 0) { var path = pattern.Substring(0, split); - if (path.StartsWith(ForwardSlash)) + if (path[0] == '/') { request.Path = PathString.FromUriComponent(path); } else { - request.Path = PathString.FromUriComponent(ForwardSlash + path); + request.Path = PathString.FromUriComponent('/' + path); } if (QueryStringAppend) @@ -112,13 +119,13 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlActions } else { - if (pattern.StartsWith(ForwardSlash)) + if (pattern[0] == '/') { request.Path = PathString.FromUriComponent(pattern); } else { - request.Path = PathString.FromUriComponent(ForwardSlash + pattern); + request.Path = PathString.FromUriComponent('/' + pattern); } if (QueryStringDelete) diff --git a/src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/UrlRewriteFileParser.cs b/src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/UrlRewriteFileParser.cs index 9dcd7f4a4a..d3d071585c 100644 --- a/src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/UrlRewriteFileParser.cs +++ b/src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/UrlRewriteFileParser.cs @@ -165,7 +165,6 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite } var parsedPatternString = condition.Attribute(RewriteTags.Pattern)?.Value; - try { var input = _inputParser.ParseInputString(parsedInputString); @@ -197,9 +196,19 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite redirectType = RedirectType.Permanent; } + string url = string.Empty; + if (urlAction.Attribute(RewriteTags.Url) != null) + { + url = urlAction.Attribute(RewriteTags.Url).Value; + if (string.IsNullOrEmpty(url)) + { + ThrowUrlFormatException(urlAction, "Url attribute cannot contain an empty string"); + } + } + try { - var input = _inputParser.ParseInputString(urlAction.Attribute(RewriteTags.Url)?.Value); + var input = _inputParser.ParseInputString(url); builder.AddUrlAction(input, actionType, appendQuery, stopProcessing, (int)redirectType); } catch (FormatException formatException) diff --git a/src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/UrlRewriteRuleBuilder.cs b/src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/UrlRewriteRuleBuilder.cs index 38d942912c..104fb0e91b 100644 --- a/src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/UrlRewriteRuleBuilder.cs +++ b/src/Microsoft.AspNetCore.Rewrite/Internal/UrlRewrite/UrlRewriteRuleBuilder.cs @@ -102,7 +102,7 @@ namespace Microsoft.AspNetCore.Rewrite.Internal.UrlRewrite { case MatchType.Pattern: { - if (pattern == null) + if (string.IsNullOrEmpty(pattern)) { throw new FormatException("Match does not have an associated pattern attribute in condition"); } diff --git a/test/Microsoft.AspNetCore.Rewrite.Tests/CodeRules/MiddlewareTests.cs b/test/Microsoft.AspNetCore.Rewrite.Tests/CodeRules/MiddlewareTests.cs index 1e4e10beb3..51456357c6 100644 --- a/test/Microsoft.AspNetCore.Rewrite.Tests/CodeRules/MiddlewareTests.cs +++ b/test/Microsoft.AspNetCore.Rewrite.Tests/CodeRules/MiddlewareTests.cs @@ -67,5 +67,40 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.CodeRules Assert.Equal(response.Headers.Location.OriginalString, "https://example.com/"); } + + + [Fact] + public async Task CheckIfEmptyStringRedirectCorrectly() + { + var options = new RewriteOptions().AddRedirect("(.*)", "$1", statusCode: 301); + var builder = new WebHostBuilder() + .Configure(app => + { + app.UseRewriter(options); + }); + var server = new TestServer(builder); + + var response = await server.CreateClient().GetAsync(""); + Assert.Equal(response.Headers.Location.OriginalString, "/"); + } + + [Fact] + public async Task CheckIfEmptyStringRewriteCorrectly() + { + var options = new RewriteOptions().AddRewrite("(.*)", "$1"); + var builder = new WebHostBuilder() + .Configure(app => + { + app.UseRewriter(options); + app.Run(context => context.Response.WriteAsync( + context.Request.Path + + context.Request.QueryString)); + }); + var server = new TestServer(builder); + + var response = await server.CreateClient().GetStringAsync(""); + + Assert.Equal(response, "/"); + } } } diff --git a/test/Microsoft.AspNetCore.Rewrite.Tests/ModRewrite/ModRewriteMiddlewareTest.cs b/test/Microsoft.AspNetCore.Rewrite.Tests/ModRewrite/ModRewriteMiddlewareTest.cs index 2fd055a7cb..57daef1bc3 100644 --- a/test/Microsoft.AspNetCore.Rewrite.Tests/ModRewrite/ModRewriteMiddlewareTest.cs +++ b/test/Microsoft.AspNetCore.Rewrite.Tests/ModRewrite/ModRewriteMiddlewareTest.cs @@ -248,5 +248,43 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.ModRewrite Assert.Equal(response.StatusCode, (HttpStatusCode)301); Assert.Equal(response.Headers.Location.AbsoluteUri, @"https://www.example.com/foo/"); } + + [Theory] + [InlineData("http://www.example.com/")] + public async Task Invoke_CaptureEmptyStringInRegexAssertRedirectLocationHasForwardSlash(string input) + { + var options = new RewriteOptions() + .AddApacheModRewrite(new StringReader("RewriteRule ^(.*)$ $1 [R=301,L]")); + var builder = new WebHostBuilder() + .Configure(app => + { + app.UseRewriter(options); + app.Run(context => context.Response.WriteAsync(context.Request.Scheme + "://" + context.Request.Host.Host + context.Request.Path + context.Request.QueryString)); + }); + var server = new TestServer(builder); + + var response = await server.CreateClient().GetAsync(input); + + Assert.Equal(response.StatusCode, (HttpStatusCode)301); + Assert.Equal(response.Headers.Location.OriginalString, "/"); + } + + [Theory] + [InlineData("http://www.example.com/")] + public async Task Invoke_CaptureEmptyStringInRegexAssertRewriteHasForwardSlash(string input) + { + var options = new RewriteOptions() + .AddApacheModRewrite(new StringReader("RewriteRule ^(.*)$ $1 [L]")); + var builder = new WebHostBuilder() + .Configure(app => + { + app.UseRewriter(options); + app.Run(context => context.Response.WriteAsync(context.Request.Path + context.Request.QueryString)); + }); + var server = new TestServer(builder); + + var response = await server.CreateClient().GetStringAsync(input); + Assert.Equal(response, "/"); + } } } diff --git a/test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/FormatExceptionHandlingTests.cs b/test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/FormatExceptionHandlingTests.cs index db70d69e4a..41adf2fbbc 100644 --- a/test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/FormatExceptionHandlingTests.cs +++ b/test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/FormatExceptionHandlingTests.cs @@ -91,6 +91,16 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.UrlRewrite ", "Could not parse the UrlRewrite file. Message: 'Match does not have an associated pattern attribute in condition'. Line number '6': '18'.")] + [InlineData( +@" + + + + + + +", + "Could not parse the UrlRewrite file. Message: 'Url attribute cannot contain an empty string'. Line number '5': '14'.")] public void ThrowFormatExceptionWithCorrectMessage(string input, string expected) { // Arrange, Act, Assert diff --git a/test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/MiddleWareTests.cs b/test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/MiddleWareTests.cs index af460e9f13..e20c5b9419 100644 --- a/test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/MiddleWareTests.cs +++ b/test/Microsoft.AspNetCore.Rewrite.Tests/UrlRewrite/MiddleWareTests.cs @@ -255,5 +255,60 @@ namespace Microsoft.AspNetCore.Rewrite.Tests.UrlRewrite Assert.Equal(response, "http://internalserver/"); } + + [Fact] + public async Task Invoke_CaptureEmptyStringInRegexAssertRedirectLocationHasForwardSlash() + { + var options = new RewriteOptions().AddIISUrlRewrite(new StringReader(@" + + + + + + + ")); + var builder = new WebHostBuilder() + .Configure(app => + { + app.UseRewriter(options); + app.Run(context => context.Response.WriteAsync( + context.Request.Scheme + + "://" + + context.Request.Host + + context.Request.Path + + context.Request.QueryString)); + }); + var server = new TestServer(builder); + + var response = await server.CreateClient().GetAsync(new Uri("http://example.com/")); + + Assert.Equal(response.Headers.Location.OriginalString, "/"); + } + + [Fact] + public async Task Invoke_CaptureEmptyStringInRegexAssertRewriteLocationHasForwardSlash() + { + var options = new RewriteOptions().AddIISUrlRewrite(new StringReader(@" + + + + + + + ")); + var builder = new WebHostBuilder() + .Configure(app => + { + app.UseRewriter(options); + app.Run(context => context.Response.WriteAsync( + context.Request.Path + + context.Request.QueryString)); + }); + var server = new TestServer(builder); + + var response = await server.CreateClient().GetStringAsync(new Uri("http://example.com/")); + + Assert.Equal(response, "/"); + } } }