[Fixes #359] Routing is matching empty segments to parameters and defaults are wrong

src/Microsoft.AspNetCore.Routing/Template/TemplateMatcher.cs

@@ -82,12 +82,17 @@ namespace Microsoft.AspNetCore.Routing.Template
             //
             // On the second pass, we're almost certainly going to match the URL, so go ahead and allocate the 'values'
             // and start capturing strings. 
-            foreach (var requestSegment in pathTokenizer)
+            foreach (var pathSegment in pathTokenizer)
             {
-                var routeSegment = Template.GetSegment(i++);
+                if (pathSegment.Length == 0)
-                if (routeSegment == null && requestSegment.Length > 0)
                 {
-                    // If pathSegment is null, then we're out of route segments. All we can match is the empty
+                    return false;
+                }
+
+                var routeSegment = Template.GetSegment(i++);
+                if (routeSegment == null && pathSegment.Length > 0)
+                {
+                    // If routeSegment is null, then we're out of route segments. All we can match is the empty
                     // string.
                     return false;
                 }
@@ -95,7 +100,7 @@ namespace Microsoft.AspNetCore.Routing.Template
                 {
                     // This is a literal segment, so we need to match the text, or the route isn't a match.
                     var part = routeSegment.Parts[0];
-                    if (!requestSegment.Equals(part.Text, StringComparison.OrdinalIgnoreCase))
+                    if (!pathSegment.Equals(part.Text, StringComparison.OrdinalIgnoreCase))
                     {
                         return false;
                     }
@@ -111,7 +116,7 @@ namespace Microsoft.AspNetCore.Routing.Template
                 {
                     // For a parameter, validate that it's a has some length, or we have a default, or it's optional.
                     var part = routeSegment.Parts[0];
-                    if (requestSegment.Length == 0 &&
+                    if (pathSegment.Length == 0 &&
                         !_hasDefaultValue[i] &&
                         !part.IsOptional)
                     {
@@ -186,7 +191,7 @@ namespace Microsoft.AspNetCore.Routing.Template
                     else
                     {
                         // It's ok for a catch-all to produce a null value, so we don't check _hasDefaultValue.
-                        values[part.Name] =_defaultValues[i];
+                        values[part.Name] = _defaultValues[i];
                     }
 
                     // A catch-all has to be the last part, so we're done.
@@ -228,7 +233,7 @@ namespace Microsoft.AspNetCore.Routing.Template
 
                 var part = routeSegment.Parts[0];
                 Debug.Assert(part.IsParameter);
-   
+
                 // It's ok for a catch-all to produce a null value
                 if (_hasDefaultValue[i] || part.IsCatchAll)
                 {

test/Microsoft.AspNetCore.Routing.Tests/RouteTest.cs

@@ -1575,6 +1575,34 @@ namespace Microsoft.AspNetCore.Routing
             Assert.Equal("RouteName", name);
         }
 
+        [Theory]
+        [InlineData("///")]
+        [InlineData("/a//")]
+        [InlineData("/a/b//")]
+        [InlineData("//b//")]
+        [InlineData("///c")]
+        [InlineData("///c/")]
+        public async Task RouteAsync_MultipleOptionalParameters_WithEmptyIntermediateSegmentsDoesNotMatch(string url)
+        {
+            // Arrange
+            var builder = CreateRouteBuilder();
+
+            builder.MapRoute(name: null,
+                    template: "{controller?}/{action?}/{id?}",
+                    defaults: null,
+                    constraints: null);
+
+            var route = builder.Build();
+
+            var context = CreateRouteContext(url);
+
+            // Act
+            await route.RouteAsync(context);
+
+            // Assert
+            Assert.Null(context.Handler);
+        }
+
         // DataTokens test data for TemplateRoute.GetVirtualPath
         public static IEnumerable<object[]> DataTokensTestData
         {

test/Microsoft.AspNetCore.Routing.Tests/Template/TemplateMatcherTests.cs

@@ -991,6 +991,107 @@ namespace Microsoft.AspNetCore.Routing.Template.Tests
             Assert.False(values.ContainsKey("id"));
         }
 
+        [Theory]
+        [InlineData("///")]
+        [InlineData("/a//")]
+        [InlineData("/a/b//")]
+        [InlineData("//b//")]
+        [InlineData("///c")]
+        [InlineData("///c/")]
+        public void TryMatch_MultipleOptionalParameters_WithEmptyIntermediateSegmentsDoesNotMatch(string url)
+        {
+            // Arrange
+            var route = CreateMatcher("{controller?}/{action?}/{id?}");
+
+            var values = new RouteValueDictionary();
+
+            // Act
+            var match = route.TryMatch(url, values);
+
+            // Assert
+            Assert.False(match);
+        }
+
+        [Theory]
+        [InlineData("")]
+        [InlineData("/")]
+        [InlineData("/a")]
+        [InlineData("/a/")]
+        [InlineData("/a/b")]
+        [InlineData("/a/b/")]
+        [InlineData("/a/b/c")]
+        [InlineData("/a/b/c/")]
+        public void TryMatch_MultipleOptionalParameters_WithIncrementalOptionalValues(string url)
+        {
+            // Arrange
+            var route = CreateMatcher("{controller?}/{action?}/{id?}");
+
+            var values = new RouteValueDictionary();
+
+            // Act
+            var match = route.TryMatch(url, values);
+
+            // Assert
+            Assert.True(match);
+        }
+
+        [Theory]
+        [InlineData("///")]
+        [InlineData("////")]
+        [InlineData("/a//")]
+        [InlineData("/a///")]
+        [InlineData("//b/")]
+        [InlineData("//b//")]
+        [InlineData("///c")]
+        [InlineData("///c/")]
+        public void TryMatch_MultipleParameters_WithEmptyValues(string url)
+        {
+            // Arrange
+            var route = CreateMatcher("{controller}/{action}/{id}");
+
+            var values = new RouteValueDictionary();
+
+            // Act
+            var match = route.TryMatch(url, values);
+
+            // Assert
+            Assert.False(match);
+        }
+
+        [Theory]
+        [InlineData("/a/b/c//")]
+        [InlineData("/a/b/c/////")]
+        public void TryMatch_CatchAllParameters_WithEmptyValuesAtTheEnd(string url)
+        {
+            // Arrange
+            var route = CreateMatcher("{controller}/{action}/{*id}");
+
+            var values = new RouteValueDictionary();
+
+            // Act
+            var match = route.TryMatch(url, values);
+
+            // Assert
+            Assert.True(match);
+        }
+
+        [Theory]
+        [InlineData("/a/b//")]
+        [InlineData("/a/b///c")]
+        public void TryMatch_CatchAllParameters_WithEmptyValues(string url)
+        {
+            // Arrange
+            var route = CreateMatcher("{controller}/{action}/{*id}");
+
+            var values = new RouteValueDictionary();
+
+            // Act
+            var match = route.TryMatch(url, values);
+
+            // Assert
+            Assert.False(match);
+        }
+
         private TemplateMatcher CreateMatcher(string template, object defaults = null)
         {
             return new TemplateMatcher(
@@ -1006,7 +1107,7 @@ namespace Microsoft.AspNetCore.Routing.Template.Tests
         {
             // Arrange
             var matcher = new TemplateMatcher(
-                TemplateParser.Parse(template), 
+                TemplateParser.Parse(template),
                 defaults ?? new RouteValueDictionary());
 
             var values = new RouteValueDictionary();

test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs

@@ -339,6 +339,127 @@ namespace Microsoft.AspNetCore.Routing.Tree
             Assert.Equal(expectedRouteGroup, context.RouteData.Values["test_route_group"]);
         }
 
+        [Theory]
+        [InlineData("///")]
+        [InlineData("/a//")]
+        [InlineData("/a/b//")]
+        [InlineData("//b//")]
+        [InlineData("///c")]
+        [InlineData("///c/")]
+        public async Task TryMatch_MultipleOptionalParameters_WithEmptyIntermediateSegmentsDoesNotMatch(string url)
+        {
+            // Arrange
+            var builder = CreateBuilder();
+
+            MapInboundEntry(builder, "{controller?}/{action?}/{id?}");
+
+            var route = builder.Build();
+
+            var context = CreateRouteContext(url);
+
+            // Act
+            await route.RouteAsync(context);
+
+            // Assert
+            Assert.Null(context.Handler);
+        }
+
+        [Theory]
+        [InlineData("")]
+        [InlineData("/")]
+        [InlineData("/a")]
+        [InlineData("/a/")]
+        [InlineData("/a/b")]
+        [InlineData("/a/b/")]
+        [InlineData("/a/b/c")]
+        [InlineData("/a/b/c/")]
+        public async Task TryMatch_MultipleOptionalParameters_WithIncrementalOptionalValues(string url)
+        {
+            // Arrange
+            var builder = CreateBuilder();
+
+            MapInboundEntry(builder, "{controller?}/{action?}/{id?}");
+
+            var route = builder.Build();
+
+            var context = CreateRouteContext(url);
+
+            // Act
+            await route.RouteAsync(context);
+
+            // Assert
+            Assert.NotNull(context.Handler);
+        }
+
+        [Theory]
+        [InlineData("///")]
+        [InlineData("////")]
+        [InlineData("/a//")]
+        [InlineData("/a///")]
+        [InlineData("//b/")]
+        [InlineData("//b//")]
+        [InlineData("///c")]
+        [InlineData("///c/")]
+        public async Task TryMatch_MultipleParameters_WithEmptyValues(string url)
+        {
+            // Arrange
+            var builder = CreateBuilder();
+
+            MapInboundEntry(builder, "{controller}/{action}/{id}");
+
+            var route = builder.Build();
+
+            var context = CreateRouteContext(url);
+
+            // Act
+            await route.RouteAsync(context);
+
+            // Assert
+            Assert.Null(context.Handler);
+        }
+
+        [Theory]
+        [InlineData("/a/b/c//")]
+        [InlineData("/a/b/c/////")]
+        public async Task TryMatch_CatchAllParameters_WithEmptyValuesAtTheEnd(string url)
+        {
+            // Arrange
+            var builder = CreateBuilder();
+
+            MapInboundEntry(builder, "{controller}/{action}/{*id}");
+
+            var route = builder.Build();
+
+            var context = CreateRouteContext(url);
+
+            // Act
+            await route.RouteAsync(context);
+
+            // Assert
+            Assert.NotNull(context.Handler);
+        }
+
+        [Theory]
+        [InlineData("/a/b//")]
+        [InlineData("/a/b///c")]
+        public async Task TryMatch_CatchAllParameters_WithEmptyValues(string url)
+        {
+            // Arrange
+            var builder = CreateBuilder();
+
+            MapInboundEntry(builder, "{controller}/{action}/{*id}");
+
+            var route = builder.Build();
+
+            var context = CreateRouteContext(url);
+
+            // Act
+            await route.RouteAsync(context);
+
+            // Assert
+            Assert.Null(context.Handler);
+        }
+
         [Theory]
         [InlineData("{*path}", "/a", "a")]
         [InlineData("{*path}", "/a/b/c", "a/b/c")]