#17238: A module's default route URL should be a valid URL without the need for further escaping.

- adding a Path property on ExtensionDescriptor that's used in place of the area name for default route URLs
- the Path property if not set will be the Name if it's a valid URL segment, otherwise it will be the Id
- if an *invalid* Path is given an error will be logged and the extension will not be loaded

--HG--
branch : dev

src/Orchard.Tests/Environment/Extensions/ExtensionFoldersTests.cs

@@ -48,12 +48,15 @@ namespace Orchard.Tests.Environment.Extensions {
         }
 
         [Test]
-        public void NamesFromFoldersWithModuleTxtShouldBeListed() {
+        public void IdsFromFoldersWithModuleTxtShouldBeListed() {
             IExtensionFolders folders = new ModuleFolders(new[] { _tempFolderName }, new StubCacheManager(), new StubWebSiteFolder());
-            var names = folders.AvailableExtensions().Select(d => d.Id);
+            var ids = folders.AvailableExtensions().Select(d => d.Id);
-            Assert.That(names.Count(), Is.EqualTo(2));
+            Assert.That(ids.Count(), Is.EqualTo(5));
-            Assert.That(names, Has.Some.EqualTo("Sample1"));
+            Assert.That(ids, Has.Some.EqualTo("Sample1")); // Sample1 - obviously
-            Assert.That(names, Has.Some.EqualTo("Sample3"));
+            Assert.That(ids, Has.Some.EqualTo("Sample3")); // Sample3
+            Assert.That(ids, Has.Some.EqualTo("Sample4")); // Sample4
+            Assert.That(ids, Has.Some.EqualTo("Sample6")); // Sample6
+            Assert.That(ids, Has.Some.EqualTo("Sample7")); // Sample7
         }
 
         [Test]
@@ -61,7 +64,31 @@ namespace Orchard.Tests.Environment.Extensions {
             IExtensionFolders folders = new ModuleFolders(new[] { _tempFolderName }, new StubCacheManager(), new StubWebSiteFolder());
             var sample1 = folders.AvailableExtensions().Single(d => d.Id == "Sample1");
             Assert.That(sample1.Id, Is.Not.Empty);
-            Assert.That(sample1.Author, Is.EqualTo("Bertrand Le Roy"));
+            Assert.That(sample1.Author, Is.EqualTo("Bertrand Le Roy")); // Sample1
         }
-   }
+
+        [Test]
+        public void NamesFromFoldersWithModuleTxtShouldFallBackToIdIfNotGiven() {
+            IExtensionFolders folders = new ModuleFolders(new[] { _tempFolderName }, new StubCacheManager(), new StubWebSiteFolder());
+            var names = folders.AvailableExtensions().Select(d => d.Name);
+            Assert.That(names.Count(), Is.EqualTo(5));
+            Assert.That(names, Has.Some.EqualTo("Le plug-in français")); // Sample1
+            Assert.That(names, Has.Some.EqualTo("This is another test.txt")); // Sample3
+            Assert.That(names, Has.Some.EqualTo("Sample4")); // Sample4
+            Assert.That(names, Has.Some.EqualTo("SampleSix")); // Sample6
+            Assert.That(names, Has.Some.EqualTo("Sample7")); // Sample7
+        }
+
+        [Test]
+        public void PathsFromFoldersWithModuleTxtShouldFallBackAppropriatelyIfNotGiven() {
+            IExtensionFolders folders = new ModuleFolders(new[] { _tempFolderName }, new StubCacheManager(), new StubWebSiteFolder());
+            var paths = folders.AvailableExtensions().Select(d => d.Path);
+            Assert.That(paths.Count(), Is.EqualTo(5));
+            Assert.That(paths, Has.Some.EqualTo("Sample1")); // Sample1 - Id, Name invalid URL segment
+            Assert.That(paths, Has.Some.EqualTo("Sample3")); // Sample3 - Id, Name invalid URL segment
+            Assert.That(paths, Has.Some.EqualTo("ThisIs.Sample4")); // Sample4 - Path
+            Assert.That(paths, Has.Some.EqualTo("SampleSix")); // Sample6 - Name, no Path
+            Assert.That(paths, Has.Some.EqualTo("Sample7")); // Sample7 - Id, no Name or Path
+        }
+    }
 }

src/Orchard.Tests/Environment/Extensions/FoldersData/Sample3/Module.txt

@@ -1 +1,2 @@
 Name: This is another test.txt
+Description: But not really.

src/Orchard.Tests/Environment/Extensions/FoldersData/Sample4/Module.txt

@@ -0,0 +1,2 @@
+Path: ThisIs.Sample4
+Author: Bertrand Le Roy

src/Orchard.Tests/Environment/Extensions/FoldersData/Sample5/Module.txt

@@ -0,0 +1,3 @@
+Name: AnotherSample
+Path: Sample 5
+Author: Some One

src/Orchard.Tests/Environment/Extensions/FoldersData/Sample6/Module.txt

@@ -0,0 +1,2 @@
+Name: SampleSix
+Author: Some One

src/Orchard.Tests/Mvc/Routes/StandardExtensionRouteProviderTests.cs

@@ -18,7 +18,9 @@ namespace Orchard.Tests.Mvc.Routes {
                         Feature =new Feature {
                             Descriptor=new FeatureDescriptor {
                                 Extension=new ExtensionDescriptor {
-                                    Name="Foo"
+                                    Id="Foo",
+                                    Name="A Foo Module",
+                                    Path="Foo"
                                 }
                             }
                         }
@@ -28,7 +30,9 @@ namespace Orchard.Tests.Mvc.Routes {
                         Feature =new Feature {
                             Descriptor=new FeatureDescriptor {
                                 Extension=new ExtensionDescriptor {
-                                    Name="Bar"
+                                    Id="Bar",
+                                    Name="Bar",
+                                    Path="BarBar"
                                 }
                             }
                         }
@@ -46,9 +50,9 @@ namespace Orchard.Tests.Mvc.Routes {
             var fooRoute = routes.Select(x => x.Route).OfType<Route>()
                 .Single(x => x.Url == "Foo/{controller}/{action}/{id}");
             var barAdmin = routes.Select(x => x.Route).OfType<Route>()
-                .Single(x => x.Url == "Admin/Bar/{action}/{id}");
+                .Single(x => x.Url == "Admin/BarBar/{action}/{id}");
             var barRoute = routes.Select(x => x.Route).OfType<Route>()
-                .Single(x => x.Url == "Bar/{controller}/{action}/{id}");
+                .Single(x => x.Url == "BarBar/{controller}/{action}/{id}");
 
             Assert.That(fooAdmin.DataTokens["area"], Is.EqualTo("Long.Name.Foo"));
             Assert.That(fooRoute.DataTokens["area"], Is.EqualTo("Long.Name.Foo"));

src/Orchard.Tests/Orchard.Framework.Tests.csproj

@@ -335,7 +335,18 @@
       <Install>true</Install>
     </BootstrapperPackage>
   </ItemGroup>
-  <ItemGroup />
+  <ItemGroup>
+    <EmbeddedResource Include="Environment\Extensions\FoldersData\Sample4\Module.txt" />
+  </ItemGroup>
+  <ItemGroup>
+    <EmbeddedResource Include="Environment\Extensions\FoldersData\Sample5\Module.txt" />
+  </ItemGroup>
+  <ItemGroup>
+    <EmbeddedResource Include="Environment\Extensions\FoldersData\Sample6\Module.txt" />
+  </ItemGroup>
+  <ItemGroup>
+    <EmbeddedResource Include="Environment\Extensions\FoldersData\Sample7\Module.txt" />
+  </ItemGroup>
   <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
   <!-- To modify your build process, add your task inside one of the targets below and uncomment it. 
        Other similar extension points exist, see Microsoft.Common.targets.

src/Orchard/Environment/Extensions/Folders/ExtensionFolders.cs

@@ -2,11 +2,13 @@ using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using System.Text.RegularExpressions;
 using Orchard.Caching;
 using Orchard.Environment.Extensions.Models;
 using Orchard.FileSystems.WebSite;
 using Orchard.Localization;
 using Orchard.Logging;
+using Orchard.Utility.Extensions;
 
 namespace Orchard.Environment.Extensions.Folders {
     public class ExtensionFolders : IExtensionFolders {
@@ -49,8 +51,24 @@ namespace Orchard.Environment.Extensions.Folders {
                         var manifestPath = Path.Combine(subfolderPath, _manifestName);
                         try {
                             var descriptor = GetExtensionDescriptor(path, extensionId, manifestPath);
-                            if (descriptor != null)
+
-                                localList.Add(descriptor);
+                            if (descriptor == null)
+                                continue;
+
+                            if (descriptor.Path != null && !descriptor.Path.IsValidUrlSegment()) {
+                                Logger.Error("The module '{0}' could not be loaded because it has an invalid Path ({1}). It was ignored. The Path if specified must be a valid URL segment. The best bet is to stick with letters and numbers with no spaces.",
+                                    extensionId,
+                                    descriptor.Path);
+                                continue;
+                            }
+
+                            if (descriptor.Path == null) {
+                                descriptor.Path = descriptor.Name.IsValidUrlSegment()
+                                    ? descriptor.Name
+                                    : descriptor.Id;
+                            }
+
+                            localList.Add(descriptor);
                         }
                         catch (Exception ex) {
                             // Ignore invalid module manifests
@@ -72,6 +90,7 @@ namespace Orchard.Environment.Extensions.Folders {
                 Id = extensionId,
                 ExtensionType = extensionType,
                 Name = GetValue(manifest, "Name") ?? extensionId,
+                Path = GetValue(manifest, "Path"),
                 Description = GetValue(manifest, "Description"),
                 Version = GetValue(manifest, "Version"),
                 OrchardVersion = GetValue(manifest, "OrchardVersion"),
@@ -125,6 +144,9 @@ namespace Orchard.Environment.Extensions.Folders {
                         case "Name":
                             manifest.Add("Name", field[1]);
                             break;
+                        case "Path":
+                            manifest.Add("Path", field[1]);
+                            break;
                         case "Description":
                             manifest.Add("Description", field[1]);
                             break;

src/Orchard/Environment/Extensions/Models/ExtensionDescriptor.cs

@@ -19,6 +19,7 @@ namespace Orchard.Environment.Extensions.Models {
         
         // extension metadata
         public string Name { get; set; }
+        public string Path { get; set; }
         public string Description { get; set; }
         public string Version { get; set; }
         public string OrchardVersion { get; set; }

src/Orchard/Mvc/Routes/StandardExtensionRouteProvider.cs

@@ -13,18 +13,18 @@ namespace Orchard.Mvc.Routes {
         }
 
         public IEnumerable<RouteDescriptor> GetRoutes() {
-            var displayNamesPerArea = _blueprint.Controllers.GroupBy(
+            var displayPathsPerArea = _blueprint.Controllers.GroupBy(
                 x => x.AreaName,
-                x => x.Feature.Descriptor.Extension.Name);
+                x => x.Feature.Descriptor.Extension.Path);
 
-            foreach (var item in displayNamesPerArea) {
+            foreach (var item in displayPathsPerArea) {
                 var areaName = item.Key;
-                var displayName = item.Distinct().Single();
+                var displayPath = item.Distinct().Single();
 
                 yield return new RouteDescriptor {
                     Priority = -10,
                     Route = new Route(
-                        "Admin/" + displayName + "/{action}/{id}",
+                        "Admin/" + displayPath + "/{action}/{id}",
                         new RouteValueDictionary {
                             {"area", areaName},
                             {"controller", "admin"},
@@ -40,7 +40,7 @@ namespace Orchard.Mvc.Routes {
                 yield return new RouteDescriptor {
                     Priority = -10,
                     Route = new Route(
-                        displayName + "/{controller}/{action}/{id}",
+                        displayPath + "/{controller}/{action}/{id}",
                         new RouteValueDictionary {
                             {"area", areaName},
                             {"controller", "home"},

src/Orchard/Utility/Extensions/StringExtensions.cs

@@ -67,5 +67,20 @@ namespace Orchard.Utility.Extensions {
                 Select(x => Convert.ToByte(hex.Substring(x, 2), 16)).
                 ToArray();
         }
+
+        public static bool IsValidUrlSegment(this string segment) {
+            // valid isegment from rfc3987 - http://tools.ietf.org/html/rfc3987#page-8
+            // the relevant bits:
+            // isegment    = *ipchar
+            // ipchar      = iunreserved / pct-encoded / sub-delims / ":" / "@"
+            // iunreserved = ALPHA / DIGIT / "-" / "." / "_" / "~" / ucschar
+            // pct-encoded = "%" HEXDIG HEXDIG
+            // sub-delims  = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "="
+            // ucschar     = %xA0-D7FF / %xF900-FDCF / %xFDF0-FFEF / %x10000-1FFFD / %x20000-2FFFD / %x30000-3FFFD / %x40000-4FFFD / %x50000-5FFFD / %x60000-6FFFD / %x70000-7FFFD / %x80000-8FFFD / %x90000-9FFFD / %xA0000-AFFFD / %xB0000-BFFFD / %xC0000-CFFFD / %xD0000-DFFFD / %xE1000-EFFFD
+            // 
+            // rough blacklist regex == m/^[^/?#[]@"^{}|\s`<>]+$/ (leaving off % to keep the regex simple)
+
+            return Regex.IsMatch(segment, @"^[^/?#[\]@""^{}|`<>\s]+$");
+        }
     }
 }