diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Routing/UrlHelper.cs b/src/Microsoft.AspNetCore.Mvc.Core/Routing/UrlHelper.cs index 418eea5360..2fbd47a17c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Routing/UrlHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Routing/UrlHelper.cs @@ -16,6 +16,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing /// public class UrlHelper : IUrlHelper { + internal const string UseRelaxedLocalRedirectValidationSwitch = "Switch.Microsoft.AspNetCore.Mvc.UseRelaxedLocalRedirectValidation"; // Perf: Share the StringBuilder object across multiple calls of GenerateURL for this UrlHelper private StringBuilder _stringBuilder; @@ -102,14 +103,53 @@ public virtual string Action(UrlActionContext actionContext) /// public virtual bool IsLocalUrl(string url) { - return - !string.IsNullOrEmpty(url) && + if (string.IsNullOrEmpty(url)) + { + return false; + } + + // Allows "/" or "/foo" but not "//" or "/\". + if (url[0] == '/') + { + // url is exactly "/" + if (url.Length == 1) + { + return true; + } + + // url doesn't start with "//" or "/\" + if (url[1] != '/' && url[1] != '\\') + { + return true; + } + + return false; + } + + // Allows "~/" or "~/foo" but not "~//" or "~/\". + if (url[0] == '~' && url.Length > 1 && url[1] == '/') + { + // url is exactly "~/" + if (url.Length == 2) + { + return true; + } + + // url doesn't start with "~//" or "~/\" + if (url[2] != '/' && url[2] != '\\') + { + return true; + } - // Allows "/" or "/foo" but not "//" or "/\". - ((url[0] == '/' && (url.Length == 1 || (url[1] != '/' && url[1] != '\\'))) || + if (AppContext.TryGetSwitch(UseRelaxedLocalRedirectValidationSwitch, out var relaxedLocalRedirectValidation) && relaxedLocalRedirectValidation) + { + return true; + } - // Allows "~/" or "~/foo". - (url.Length > 1 && url[0] == '~' && url[1] == '/')); + return false; + } + + return false; } /// diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/LocalRedirectResultTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/LocalRedirectResultTest.cs index 0409d15052..3c7876d94c 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/LocalRedirectResultTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/LocalRedirectResultTest.cs @@ -85,6 +85,10 @@ public void Execute_ReturnsExpectedValues() } [Theory] + [InlineData("", "//", "/test")] + [InlineData("", "/\\", "/test")] + [InlineData("", "//foo", "/test")] + [InlineData("", "/\\foo", "/test")] [InlineData("", "Home/About", "/Home/About")] [InlineData("/myapproot", "http://www.example.com", "/test")] public void Execute_Throws_ForNonLocalUrl( @@ -109,6 +113,44 @@ public void Execute_ReturnsExpectedValues() exception.Message); } + [Theory] + [InlineData("", "~//", "//")] + [InlineData("", "~/\\", "/\\")] + [InlineData("", "~//foo", "//foo")] + [InlineData("", "~/\\foo", "/\\foo")] + public void Execute_Throws_ForNonLocalUrlTilde( + string appRoot, + string contentPath, + string expectedPath) + { + // Arrange + var httpResponse = new Mock(); + httpResponse.Setup(o => o.Redirect(expectedPath, false)) + .Verifiable(); + + var httpContext = GetHttpContext(appRoot, contentPath, expectedPath, httpResponse.Object); + var actionContext = GetActionContext(httpContext); + var result = new LocalRedirectResult(contentPath); + + var relaxedLocalRedirectValidation = false; + var success = AppContext.TryGetSwitch(UrlHelper.UseRelaxedLocalRedirectValidationSwitch, out relaxedLocalRedirectValidation); + + // Act & Assert + if (relaxedLocalRedirectValidation) + { + result.ExecuteResult(actionContext); + httpResponse.Verify(); + } + else + { + var exception = Assert.Throws(() => result.ExecuteResult(actionContext)); + Assert.Equal( + "The supplied URL is not local. A URL with an absolute path is considered local if it does not " + + "have a host/authority part. URLs using virtual paths ('~/') are also local.", + exception.Message); + } + } + private static ActionContext GetActionContext(HttpContext httpContext) { var routeData = new RouteData(); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/UrlHelperTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/UrlHelperTest.cs index 563cfd2410..7a98bdd427 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/UrlHelperTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/UrlHelperTest.cs @@ -261,6 +261,60 @@ public void IsLocalUrl_RejectsInvalidUrls(string url) Assert.False(result); } + [Theory] + [InlineData("~//www.example.com")] + [InlineData("~//www.example.com?")] + [InlineData("~//www.example.com:80")] + [InlineData("~//www.example.com/foobar.html")] + [InlineData("~///www.example.com")] + [InlineData("~//////www.example.com")] + public void IsLocalUrl_RejectsTokenUrlsWithMissingSchemeName(string url) + { + // Arrange + var helper = CreateUrlHelper("www.mysite.com"); + + // Act + var result = helper.IsLocalUrl(url); + + // Assert + var relaxedLocalRedirectValidation = false; + var success = AppContext.TryGetSwitch(UrlHelper.UseRelaxedLocalRedirectValidationSwitch, out relaxedLocalRedirectValidation); + + if (relaxedLocalRedirectValidation) + { + Assert.True(result); + } + else + { + Assert.False(result); + } + } + + [Theory] + [InlineData("~/\\")] + [InlineData("~/\\foo")] + public void IsLocalUrl_RejectsInvalidTokenUrls(string url) + { + // Arrange + var helper = CreateUrlHelper("www.mysite.com"); + + // Act + var result = helper.IsLocalUrl(url); + + // Assert + var relaxedLocalRedirectValidation = false; + var success = AppContext.TryGetSwitch(UrlHelper.UseRelaxedLocalRedirectValidationSwitch, out relaxedLocalRedirectValidation); + + if (relaxedLocalRedirectValidation) + { + Assert.True(result); + } + else + { + Assert.False(result); + } + } + [Fact] public void RouteUrlWithDictionary() {