Merge pull request #895 from grumpydev/OpenRedirect-893

Local url validation and forms auth fix to fix #893

src/Nancy.Authentication.Forms.Tests/FormsAuthenticationFixture.cs

@@ -57,9 +57,11 @@ public FormsAuthenticationFixture()
                 RequiresSSL = true
             };
 
-            this.context = new NancyContext()
+            this.context = new NancyContext
                                {
-                                   Request = new FakeRequest("GET", "/")
+                                    Request = new Request(
+                                                    "GET",
+                                                    new Url { Scheme = "http", BasePath = "/testing", HostName = "test.com", Path = "test" })
                                };
 
             this.userGuid = new Guid("3D97EB33-824A-4173-A2C1-633AC16C1010");
@@ -469,7 +471,7 @@ public void Should_retain_querystring_when_redirecting_to_login_page()
         {
             // Given
             var fakePipelines = new Pipelines();
-            
+
             FormsAuthentication.Enable(fakePipelines, this.config);
 
             var queryContext = new NancyContext()
@@ -490,7 +492,7 @@ public void Should_change_the_forms_authentication_redirect_uri_querystring_key(
         {
             // Given
             var fakePipelines = new Pipelines();
-            
+
             this.config.RedirectQuerystringKey = "next";
             FormsAuthentication.Enable(fakePipelines, this.config);
 
@@ -537,7 +539,7 @@ public void Should_change_the_forms_authentication_redirect_uri_querystring_key_
 
             this.config.RedirectQuerystringKey = string.Empty;
             FormsAuthentication.Enable(fakePipelines, this.config);
-            
+
             var queryContext = new NancyContext()
             {
                 Request = new FakeRequest("GET", "/secure", "?foo=bar"),
@@ -625,5 +627,44 @@ public void Should_set_authentication_cookie_to_secure_when_config_requires_ssl_
             var cookie = result.Cookies.Where(c => c.Name == FormsAuthentication.FormsAuthenticationCookieName).First();
             cookie.Secure.ShouldBeTrue();
         }
+
+        [Fact]
+        public void Should_redirect_to_base_path_if_non_local_url_and_no_fallback()
+        {
+            FormsAuthentication.Enable(A.Fake<IPipelines>(), this.config);
+            context.Request.Query[config.RedirectQuerystringKey] = "http://moo.com/";
+
+            var result = FormsAuthentication.UserLoggedInRedirectResponse(context, userGuid);
+
+            result.ShouldBeOfType(typeof(Response));
+            result.StatusCode.ShouldEqual(HttpStatusCode.SeeOther);
+            result.Headers["Location"].ShouldEqual("/testing");
+        }
+
+        [Fact]
+        public void Should_redirect_to_fallback_if_non_local_url_and_fallback_set()
+        {
+            FormsAuthentication.Enable(A.Fake<IPipelines>(), this.config);
+            context.Request.Query[config.RedirectQuerystringKey] = "http://moo.com/";
+
+            var result = FormsAuthentication.UserLoggedInRedirectResponse(context, userGuid, fallbackRedirectUrl:"/moo");
+
+            result.ShouldBeOfType(typeof(Response));
+            result.StatusCode.ShouldEqual(HttpStatusCode.SeeOther);
+            result.Headers["Location"].ShouldEqual("/moo");
+        }
+
+        [Fact]
+        public void Should_redirect_to_given_url_if_local()
+        {
+            FormsAuthentication.Enable(A.Fake<IPipelines>(), this.config);
+            context.Request.Query[config.RedirectQuerystringKey] = "~/login";
+
+            var result = FormsAuthentication.UserLoggedInRedirectResponse(context, userGuid);
+
+            result.ShouldBeOfType(typeof(Response));
+            result.StatusCode.ShouldEqual(HttpStatusCode.SeeOther);
+            result.Headers["Location"].ShouldEqual("/testing/login");
+        }
     }
 }

src/Nancy.Authentication.Forms/FormsAuthentication.cs

@@ -73,14 +73,30 @@ public static void Enable(IPipelines pipelines, FormsAuthenticationConfiguration
         /// <param name="cookieExpiry">Optional expiry date for the cookie (for 'Remember me')</param>
         /// <param name="fallbackRedirectUrl">Url to redirect to if none in the querystring</param>
         /// <returns>Nancy response with redirect.</returns>
-        public static Response UserLoggedInRedirectResponse(NancyContext context, Guid userIdentifier, DateTime? cookieExpiry = null, string fallbackRedirectUrl = "/")
+        public static Response UserLoggedInRedirectResponse(NancyContext context, Guid userIdentifier, DateTime? cookieExpiry = null, string fallbackRedirectUrl = null)
         {
             var redirectUrl = fallbackRedirectUrl;
+
+            if (string.IsNullOrEmpty(redirectUrl))
+            {
+                redirectUrl = context.Request.Url.BasePath;
+            }
+
+            if (string.IsNullOrEmpty(redirectUrl))
+            {
+                redirectUrl = "/";
+            }
+
             string redirectQuerystringKey = GetRedirectQuerystringKey(currentConfiguration);
 
             if (context.Request.Query[redirectQuerystringKey].HasValue)
             {
-                redirectUrl = context.Request.Query[redirectQuerystringKey];
+                var queryUrl = (string)context.Request.Query[redirectQuerystringKey];
+
+                if (context.IsLocalUrl(queryUrl))
+                {
+                    redirectUrl = queryUrl;
+                }
             }
 
             var response = context.GetRedirect(redirectUrl);

src/Nancy.Tests/Unit/Extensions/ContextExtensionsFixture.cs

@@ -103,5 +103,53 @@ public void Should_replace_tilde_with_nothing_when_parsing_path_if_one_present_a
             result.ShouldEqual("/scripts/test.js");
         }
 
+        [Fact]
+        public void Should_report_simple_relative_path_as_local()
+        {
+            var context = this.CreateContext();
+
+            var result = context.IsLocalUrl("/stuff");
+
+            result.ShouldBeTrue();
+        }
+
+        [Fact]
+        public void Should_report_same_host_absolute_url_as_local()
+        {
+            var context = this.CreateContext();
+
+            var result = context.IsLocalUrl("http://test.com/someotherpath");
+
+            result.ShouldBeTrue();
+        }
+
+        [Fact]
+        public void Should_report_empty_string_as_nonlocal()
+        {
+            var context = this.CreateContext();
+
+            var result = context.IsLocalUrl("");
+
+            result.ShouldBeFalse();
+        }
+
+        [Fact]
+        public void Should_report_different_host_absolute_url_as_nonlocal()
+        {
+            var context = this.CreateContext();
+
+            var result = context.IsLocalUrl("http://anothertest.com/someotherpath");
+
+            result.ShouldBeFalse();
+        }
+
+        private NancyContext CreateContext(Url url = null)
+        {
+            var request = new Request(
+                "GET",
+                url ?? new Url() { Scheme = "http", BasePath = "testing", HostName = "test.com", Path = "test" });
+
+            return new NancyContext() { Request = request };
+        }
     }
 }

src/Nancy/Extensions/ContextExtensions.cs

@@ -85,5 +85,33 @@ public static void WriteTraceLog(this NancyContext context, Action<StringBuilder
         {
             context.Trace.TraceLog.WriteLog(logDelegate);
         }
+
+        /// <summary>
+        /// Returns a boolean indicating whether a given url string is local or not
+        /// </summary>
+        /// <param name="context">Nancy context</param>
+        /// <param name="url">Url string (relative or absolute)</param>
+        /// <returns>True if local, false otherwise</returns>
+        public static bool IsLocalUrl(this NancyContext context, string url)
+        {
+            if (string.IsNullOrEmpty(url))
+            {
+                return false;
+            }
+
+            Uri uri;
+
+            if (Uri.TryCreate(url, UriKind.Relative, out uri))
+            {
+                return true;
+            }
+
+            if (!Uri.TryCreate(url, UriKind.Absolute, out uri))
+            {
+                return false;
+            }
+
+            return string.Equals(uri.Host, context.Request.Url.HostName, StringComparison.OrdinalIgnoreCase);
+        }
     }
 }