Make Ocelot not add forward slash to downstream url (#158)

* removed code where we add a trailing slash as this means if we request /1.txt/ instead of /1.txt then some servers will not return the resource at /1.txt. After reading up it seems if you dont have a trailing slash then its a file, if you do then its a resource * test for 145 * removed unused code * fix broken build..my bad
ThreeMammals · Nov 19, 2017 · 9ba57f8 · 9ba57f8
1 parent 6824210
commit 9ba57f8
Show file tree

Hide file tree

Showing 12 changed files with 86 additions and 39 deletions.
diff --git a/src/Ocelot/Configuration/Creator/FileOcelotConfigurationCreator.cs b/src/Ocelot/Configuration/Creator/FileOcelotConfigurationCreator.cs
@@ -14,7 +14,6 @@
 using Ocelot.Logging;
 using Ocelot.Requester.QoS;
 using Ocelot.Responses;
-using Ocelot.Utilities;
 
 namespace Ocelot.Configuration.Creator
 {

diff --git a/src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs b/src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs
@@ -1,12 +1,11 @@
 using System.Collections.Generic;
 using Ocelot.Configuration.File;
-using Ocelot.Utilities;
 
 namespace Ocelot.Configuration.Creator
 {
     public class UpstreamTemplatePatternCreator : IUpstreamTemplatePatternCreator
     {
-        private const string RegExMatchEverything = ".*";
+        private const string RegExMatchEverything = "[0-9a-zA-Z].*";
         private const string RegExMatchEndString = "$";
         private const string RegExIgnoreCase = "(?i)";
         private const string RegExForwardSlashOnly = "^/$";
@@ -15,8 +14,6 @@ public string Create(FileReRoute reRoute)
         {
             var upstreamTemplate = reRoute.UpstreamPathTemplate;
 
-            upstreamTemplate = upstreamTemplate.SetLastCharacterAs('/');
-
             var placeholders = new List<string>();
 
             for (var i = 0; i < upstreamTemplate.Length; i++)
@@ -40,6 +37,11 @@ public string Create(FileReRoute reRoute)
                 return RegExForwardSlashOnly;
             }
 
+            if(upstreamTemplate.EndsWith("/"))
+            {
+                upstreamTemplate = upstreamTemplate.Remove(upstreamTemplate.Length -1, 1) + "(/|)";
+            }
+
             var route = reRoute.ReRouteIsCaseSensitive 
                 ? $"^{upstreamTemplate}{RegExMatchEndString}" 
                 : $"^{RegExIgnoreCase}{upstreamTemplate}{RegExMatchEndString}";

diff --git a/src/Ocelot/DownstreamRouteFinder/Finder/DownstreamRouteFinder.cs b/src/Ocelot/DownstreamRouteFinder/Finder/DownstreamRouteFinder.cs
@@ -7,7 +7,6 @@
 using Ocelot.DownstreamRouteFinder.UrlMatcher;
 using Ocelot.Errors;
 using Ocelot.Responses;
-using Ocelot.Utilities;
 
 namespace Ocelot.DownstreamRouteFinder.Finder
 {
@@ -24,8 +23,6 @@ public DownstreamRouteFinder(IUrlPathToUrlTemplateMatcher urlMatcher, IUrlPathPl
 
         public Response<DownstreamRoute> FindDownstreamRoute(string upstreamUrlPath, string upstreamHttpMethod, IOcelotConfiguration configuration)
         {
-            upstreamUrlPath = upstreamUrlPath.SetLastCharacterAs('/');
-
             var applicableReRoutes = configuration.ReRoutes.Where(r => r.UpstreamHttpMethod.Count == 0 || r.UpstreamHttpMethod.Select(x => x.Method.ToLower()).Contains(upstreamHttpMethod.ToLower()));
 
             foreach (var reRoute in applicableReRoutes)

diff --git a/src/Ocelot/DownstreamRouteFinder/Middleware/DownstreamRouteFinderMiddleware.cs b/src/Ocelot/DownstreamRouteFinder/Middleware/DownstreamRouteFinderMiddleware.cs
@@ -8,7 +8,6 @@
 using Ocelot.Infrastructure.RequestData;
 using Ocelot.Logging;
 using Ocelot.Middleware;
-using Ocelot.Utilities;
 
 namespace Ocelot.DownstreamRouteFinder.Middleware
 {

diff --git a/src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs b/src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs
@@ -13,7 +13,7 @@ public Response<List<UrlPathPlaceholderNameAndValue>> Find(string upstreamUrlPat
 
             for (int counterForTemplate = 0; counterForTemplate < upstreamUrlPathTemplate.Length; counterForTemplate++)
             {
-                if (CharactersDontMatch(upstreamUrlPathTemplate[counterForTemplate], upstreamUrlPath[counterForUrl]) && ContinueScanningUrl(counterForUrl,upstreamUrlPath.Length))
+                if ((upstreamUrlPath.Length > counterForUrl) && CharactersDontMatch(upstreamUrlPathTemplate[counterForTemplate], upstreamUrlPath[counterForUrl]) && ContinueScanningUrl(counterForUrl,upstreamUrlPath.Length))
                 {
                     if (IsPlaceholder(upstreamUrlPathTemplate[counterForTemplate]))
                     {

diff --git a/src/Ocelot/Utilities/StringExtensions.cs b/src/Ocelot/Utilities/StringExtensions.cs
diff --git a/test/Ocelot.AcceptanceTests/RoutingTests.cs b/test/Ocelot.AcceptanceTests/RoutingTests.cs
@@ -159,7 +159,7 @@ public void should_return_response_200_when_host_has_trailing_slash()
         }
 
         [Fact]
-        public void should_not_care_about_no_trailing()
+        public void should_return_ok_when_upstream_url_ends_with_forward_slash_but_template_does_not()
         {
             var configuration = new FileConfiguration
             {
@@ -187,7 +187,7 @@ public void should_not_care_about_no_trailing()
         }
 
         [Fact]
-        public void should_not_care_about_trailing()
+        public void should_return_not_found_when_upstream_url_ends_with_forward_slash_but_template_does_not()
         {
             var configuration = new FileConfiguration
             {
@@ -209,8 +209,7 @@ public void should_not_care_about_trailing()
                 .And(x => _steps.GivenThereIsAConfiguration(configuration))
                 .And(x => _steps.GivenOcelotIsRunning())
                 .When(x => _steps.WhenIGetUrlOnTheApiGateway("/products/"))
-                .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
-                .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
+                .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.NotFound))
                 .BDDfy();
         }
 
@@ -483,6 +482,41 @@ public void should_return_404_when_calling_upstream_route_with_no_matching_downs
                 .BDDfy();
         }
 
+
+        [Fact]
+        public void should_fix_145()
+        {
+            var configuration = new FileConfiguration
+            {
+                ReRoutes = new List<FileReRoute>
+                    {
+                        new FileReRoute
+                        {
+                            DownstreamPathTemplate = "/api/{url}",
+                            DownstreamScheme = "http",
+                            DownstreamHost = "localhost",
+                            DownstreamPort = 51899,
+                            UpstreamPathTemplate = "/platform/{url}",
+                            UpstreamHttpMethod = new List<string> { "Get" },
+                            QoSOptions = new FileQoSOptions {
+                                ExceptionsAllowedBeforeBreaking = 3,
+                                DurationOfBreak = 10,
+                                TimeoutValue = 5000
+                            }
+                        }
+                    }
+            };
+
+            this.Given(x => x.GivenThereIsAServiceRunningOn("http://localhost:51899", "", 200, "Hello from Laura"))
+                .And(x => _steps.GivenThereIsAConfiguration(configuration))
+                .And(x => _steps.GivenOcelotIsRunning())
+                .When(x => _steps.WhenIGetUrlOnTheApiGateway("/platform/swagger/lib/backbone-min.js"))
+                .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
+                .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
+                .And(x => ThenTheDownstreamUrlPathShouldBe("/api/swagger/lib/backbone-min.js"))
+                .BDDfy();
+        }
+
         private void GivenThereIsAServiceRunningOn(string baseUrl, string basePath, int statusCode, string responseBody)
         {
             _builder = new WebHostBuilder()
@@ -495,7 +529,7 @@ private void GivenThereIsAServiceRunningOn(string baseUrl, string basePath, int
                     app.UsePathBase(basePath);
                     app.Run(async context =>
                     {   
-                        _downstreamPath = context.Request.PathBase.Value;
+                        _downstreamPath = !string.IsNullOrEmpty(context.Request.PathBase.Value) ? context.Request.PathBase.Value : context.Request.Path.Value;
                         context.Response.StatusCode = statusCode;
                         await context.Response.WriteAsync(responseBody);
                     });

diff --git a/test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs b/test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs
@@ -28,7 +28,23 @@ public void should_set_upstream_template_pattern_to_ignore_case_sensitivity()
 
             this.Given(x => x.GivenTheFollowingFileReRoute(fileReRoute))
                 .When(x => x.WhenICreateTheTemplatePattern())
-                .Then(x => x.ThenTheFollowingIsReturned("^(?i)/PRODUCTS/.*/$"))
+                .Then(x => x.ThenTheFollowingIsReturned("^(?i)/PRODUCTS/[0-9a-zA-Z].*$"))
+                .BDDfy();
+        }
+
+
+        [Fact]
+        public void should_match_forward_slash_or_no_forward_slash_if_template_end_with_forward_slash()
+        {
+            var fileReRoute = new FileReRoute
+            {
+                UpstreamPathTemplate = "/PRODUCTS/",
+                ReRouteIsCaseSensitive = false
+            };
+
+            this.Given(x => x.GivenTheFollowingFileReRoute(fileReRoute))
+                .When(x => x.WhenICreateTheTemplatePattern())
+                .Then(x => x.ThenTheFollowingIsReturned("^(?i)/PRODUCTS(/|)$"))
                 .BDDfy();
         }
 
@@ -42,7 +58,7 @@ public void should_set_upstream_template_pattern_to_respect_case_sensitivity()
                 };
             this.Given(x => x.GivenTheFollowingFileReRoute(fileReRoute))
                 .When(x => x.WhenICreateTheTemplatePattern())
-                .Then(x => x.ThenTheFollowingIsReturned("^/PRODUCTS/.*/$"))
+                .Then(x => x.ThenTheFollowingIsReturned("^/PRODUCTS/[0-9a-zA-Z].*$"))
                 .BDDfy();
         }
 
@@ -57,7 +73,7 @@ public void should_create_template_pattern_that_matches_anything_to_end_of_strin
 
             this.Given(x => x.GivenTheFollowingFileReRoute(fileReRoute))
                 .When(x => x.WhenICreateTheTemplatePattern())
-                .Then(x => x.ThenTheFollowingIsReturned("^/api/products/.*/$"))
+                .Then(x => x.ThenTheFollowingIsReturned("^/api/products/[0-9a-zA-Z].*$"))
                 .BDDfy();
         }
 
@@ -72,9 +88,10 @@ public void should_create_template_pattern_that_matches_more_than_one_placeholde
 
             this.Given(x => x.GivenTheFollowingFileReRoute(fileReRoute))
                 .When(x => x.WhenICreateTheTemplatePattern())
-                .Then(x => x.ThenTheFollowingIsReturned("^/api/products/.*/variants/.*/$"))
+                .Then(x => x.ThenTheFollowingIsReturned("^/api/products/[0-9a-zA-Z].*/variants/[0-9a-zA-Z].*$"))
                 .BDDfy();
         }
+
         [Fact]
         public void should_create_template_pattern_that_matches_more_than_one_placeholder_with_trailing_slash()
         {
@@ -86,7 +103,7 @@ public void should_create_template_pattern_that_matches_more_than_one_placeholde
 
             this.Given(x => x.GivenTheFollowingFileReRoute(fileReRoute))
                 .When(x => x.WhenICreateTheTemplatePattern())
-                .Then(x => x.ThenTheFollowingIsReturned("^/api/products/.*/variants/.*/$"))
+                .Then(x => x.ThenTheFollowingIsReturned("^/api/products/[0-9a-zA-Z].*/variants/[0-9a-zA-Z].*(/|)$"))
                 .BDDfy();
         }
 

diff --git a/test/Ocelot.UnitTests/DownstreamRouteFinder/DownstreamRouteFinderTests.cs b/test/Ocelot.UnitTests/DownstreamRouteFinder/DownstreamRouteFinderTests.cs
@@ -68,7 +68,7 @@ public void should_return_route()
 
 
         [Fact]
-        public void should_append_slash_to_upstream_url_path()
+        public void should_not_append_slash_to_upstream_url_path()
         {
             var serviceProviderConfig = new ServiceProviderConfigurationBuilder().Build();
 
@@ -97,7 +97,7 @@ public void should_append_slash_to_upstream_url_path()
                                 .WithUpstreamHttpMethod(new List<string> { "Get" })
                                 .Build()
                 )))
-                .And(x => x.ThenTheUrlMatcherIsCalledCorrectly("matchInUrlMatcher/"))
+                .And(x => x.ThenTheUrlMatcherIsCalledCorrectly("matchInUrlMatcher"))
                 .BDDfy();
         }
 

diff --git a/...lot.UnitTests/DownstreamRouteFinder/UrlMatcher/TemplateVariableNameAndValueFinderTests.cs b/...lot.UnitTests/DownstreamRouteFinder/UrlMatcher/TemplateVariableNameAndValueFinderTests.cs
@@ -30,6 +30,16 @@ public void can_match_down_stream_url()
                 .BDDfy();
         }
 
+        [Fact]
+        public void should_not_find_anything()
+        {
+            this.Given(x => x.GivenIHaveAUpstreamPath("/products"))
+                .And(x => x.GivenIHaveAnUpstreamUrlTemplate("/products/"))
+                .When(x => x.WhenIFindTheUrlVariableNamesAndValues())
+                .And(x => x.ThenTheTemplatesVariablesAre(new List<UrlPathPlaceholderNameAndValue>()))
+                .BDDfy();
+        }
+
         [Fact]
         public void can_match_down_stream_url_with_no_slash()
         {

diff --git a/test/Ocelot.UnitTests/Ocelot.UnitTests.csproj b/test/Ocelot.UnitTests/Ocelot.UnitTests.csproj
@@ -31,6 +31,12 @@
   <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />
   </ItemGroup>
 
+   <ItemGroup>
+    <None Update="idsrv3test.pfx">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>
+  </ItemGroup>
+
   <ItemGroup>
     <PackageReference Include="Microsoft.AspNetCore.All" Version="2.0.0" />
     <PackageReference Include="Microsoft.AspNetCore.TestHost" Version="2.0.0" />

diff --git a/test/Ocelot.UnitTests/idsrv3test.pfx b/test/Ocelot.UnitTests/idsrv3test.pfx