U4-7083 - filesystems, polish, fix, and back-compat

src/Umbraco.Core/IO/PhysicalFileSystem.cs

@@ -8,7 +8,12 @@ namespace Umbraco.Core.IO
 {
     public class PhysicalFileSystem : IFileSystem
     {
+        // the rooted, filesystem path, using directory separator chars, NOT ending with a separator
+        // eg "c:" or "c:\path\to\site" or "\\server\path"
         private readonly string _rootPath;
+
+        // the ??? url, using url separator chars, NOT ending with a separator
+        // eg "" (?) or "/Scripts" or ???
         private readonly string _rootUrl;
 
         public PhysicalFileSystem(string virtualRoot)
@@ -22,6 +27,7 @@ namespace Umbraco.Core.IO
             _rootPath = _rootPath.TrimEnd(Path.DirectorySeparatorChar);
 
             _rootUrl = IOHelper.ResolveUrl(virtualRoot);
+            _rootUrl = EnsureUrlSeparatorChar(_rootUrl);
             _rootUrl = _rootUrl.TrimEnd('/');
         }
 
@@ -46,6 +52,9 @@ namespace Umbraco.Core.IO
                 rootPath = Path.Combine(localRoot, rootPath);
             }
 
+            rootPath = EnsureDirectorySeparatorChar(rootPath);
+            rootUrl = EnsureUrlSeparatorChar(rootUrl);
+
             _rootPath = rootPath.TrimEnd(Path.DirectorySeparatorChar);
             _rootUrl = rootUrl.TrimEnd('/');
         }
@@ -173,6 +182,8 @@ namespace Umbraco.Core.IO
             return File.Exists(fullpath);
         }
 
+        // beware, many things depend on how the GetRelative/AbsolutePath methods work!
+
         /// <summary>
         /// Gets the relative path.
         /// </summary>
@@ -180,33 +191,27 @@ namespace Umbraco.Core.IO
         /// <returns>The path, relative to this filesystem's root.</returns>
         /// <remarks>
         /// <para>The relative path is relative to this filesystem's root, not starting with any
-        /// directory separator. All separators are converted to Path.DirectorySeparatorChar.</para>
+        /// directory separator. If input was recognized as a url (path), then output uses url (path) separator
+        /// chars.</para>
         /// </remarks>
         public string GetRelativePath(string fullPathOrUrl)
         {
             // test url
-            var path = EnsureUrlSeparatorChar(fullPathOrUrl);
-            if (IOHelper.PathStartsWith(path, _rootUrl, '/'))
-                return path.Substring(_rootUrl.Length)  
-                    .Replace('/', Path.DirectorySeparatorChar)
-                    .TrimStart(Path.DirectorySeparatorChar);
+            var path = fullPathOrUrl.Replace('\\', '/'); // ensure url separator char
+
+            if (IOHelper.PathStartsWith(path, _rootUrl, '/')) // if it starts with the root url...
+                return path.Substring(_rootUrl.Length) // strip it
+                            .TrimStart('/'); // it's relative
 
             // test path
             path = EnsureDirectorySeparatorChar(fullPathOrUrl);
-            if (IOHelper.PathStartsWith(path, _rootPath, Path.DirectorySeparatorChar))
-                return path.Substring(_rootPath.Length)
-                    .TrimStart(Path.DirectorySeparatorChar);
 
+            if (IOHelper.PathStartsWith(path, _rootPath, Path.DirectorySeparatorChar)) // if it starts with the root path
+                return path.Substring(_rootPath.Length) // strip it
+                            .TrimStart(Path.DirectorySeparatorChar); // it's relative
+
+            // unchanged - including separators
             return fullPathOrUrl;
-
-            // previous code kept for reference
-
-            //var relativePath = fullPathOrUrl
-            //    .TrimStart(_rootUrl)
-            //    .Replace('/', Path.DirectorySeparatorChar)
-            //    .TrimStart(RootPath)
-            //    .TrimStart(Path.DirectorySeparatorChar);
-            //return relativePath;
         }
 
         /// <summary>

src/Umbraco.Core/Models/File.cs

@@ -27,7 +27,7 @@ namespace Umbraco.Core.Models
 
         protected File(string path, Func<File, string> getFileContent = null)
         {
-            _path = path;
+            _path = SanitizePath(path);
             _originalPath = _path;
             GetFileContent = getFileContent;
             _content = getFileContent != null ? null : string.Empty;
@@ -38,6 +38,14 @@ namespace Umbraco.Core.Models
         private string _alias;
         private string _name;
 
+        private static string SanitizePath(string path)
+        {
+            return path
+                .Replace('\\', System.IO.Path.DirectorySeparatorChar)
+                .Replace('/', System.IO.Path.DirectorySeparatorChar);
+                //.TrimStart(System.IO.Path.DirectorySeparatorChar);
+        }
+
         /// <summary>
         /// Gets or sets the Name of the File including extension
         /// </summary>
@@ -81,7 +89,7 @@ namespace Umbraco.Core.Models
 
                 SetPropertyValueAndDetectChanges(o =>
                 {
-                    _path = value;
+                    _path = SanitizePath(value);
                     return _path;
                 }, _path, PathSelector);
             }

src/Umbraco.Tests/Persistence/Repositories/PartialViewRepositoryTests.cs

@@ -0,0 +1,119 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using NUnit.Framework;
+using Umbraco.Core.IO;
+using Umbraco.Core.Models;
+using Umbraco.Core.Persistence.Repositories;
+using Umbraco.Core.Persistence.UnitOfWork;
+using Umbraco.Tests.TestHelpers;
+
+namespace Umbraco.Tests.Persistence.Repositories
+{
+    [TestFixture]
+    public class PartialViewRepositoryTests : BaseUmbracoApplicationTest
+    {
+        private IFileSystem _fileSystem;
+
+        [SetUp]
+        public override void Initialize()
+        {
+            base.Initialize();
+
+            _fileSystem = new PhysicalFileSystem(SystemDirectories.MvcViews + "/Partials/");
+        }
+
+        [Test]
+        public void PathTests()
+        {
+            // unless noted otherwise, no changes / 7.2.8
+
+            var provider = new FileUnitOfWorkProvider();
+            var unitOfWork = provider.GetUnitOfWork();
+            var repository = new PartialViewRepository(unitOfWork, _fileSystem);
+
+            var partialView = new PartialView("test-path-1.cshtml") { Content = "// partialView" };
+            repository.AddOrUpdate(partialView);
+            unitOfWork.Commit();
+            Assert.IsTrue(_fileSystem.FileExists("test-path-1.cshtml"));
+            Assert.AreEqual("test-path-1.cshtml", partialView.Path);
+            Assert.AreEqual("/Views/Partials/test-path-1.cshtml", partialView.VirtualPath);
+
+            partialView = new PartialView("path-2/test-path-2.cshtml") { Content = "// partialView" };
+            repository.AddOrUpdate(partialView);
+            unitOfWork.Commit();
+            Assert.IsTrue(_fileSystem.FileExists("path-2/test-path-2.cshtml"));
+            Assert.AreEqual("path-2\\test-path-2.cshtml", partialView.Path); // fixed in 7.3 - 7.2.8 does not update the path
+            Assert.AreEqual("/Views/Partials/path-2/test-path-2.cshtml", partialView.VirtualPath);
+
+            partialView = (PartialView) repository.Get("path-2/test-path-2.cshtml");
+            Assert.IsNotNull(partialView);
+            Assert.AreEqual("path-2\\test-path-2.cshtml", partialView.Path);
+            Assert.AreEqual("/Views/Partials/path-2/test-path-2.cshtml", partialView.VirtualPath);
+
+            partialView = new PartialView("path-2\\test-path-3.cshtml") { Content = "// partialView" };
+            repository.AddOrUpdate(partialView);
+            unitOfWork.Commit();
+            Assert.IsTrue(_fileSystem.FileExists("path-2/test-path-3.cshtml"));
+            Assert.AreEqual("path-2\\test-path-3.cshtml", partialView.Path);
+            Assert.AreEqual("/Views/Partials/path-2/test-path-3.cshtml", partialView.VirtualPath);
+
+            partialView = (PartialView) repository.Get("path-2/test-path-3.cshtml");
+            Assert.IsNotNull(partialView);
+            Assert.AreEqual("path-2\\test-path-3.cshtml", partialView.Path);
+            Assert.AreEqual("/Views/Partials/path-2/test-path-3.cshtml", partialView.VirtualPath);
+
+            partialView = (PartialView) repository.Get("path-2\\test-path-3.cshtml");
+            Assert.IsNotNull(partialView);
+            Assert.AreEqual("path-2\\test-path-3.cshtml", partialView.Path);
+            Assert.AreEqual("/Views/Partials/path-2/test-path-3.cshtml", partialView.VirtualPath);
+
+            partialView = new PartialView("\\test-path-4.cshtml") { Content = "// partialView" };
+            Assert.Throws<FileSecurityException>(() => // fixed in 7.3 - 7.2.8 used to strip the \
+            {
+                repository.AddOrUpdate(partialView);
+            });
+
+            partialView = (PartialView) repository.Get("missing.cshtml");
+            Assert.IsNull(partialView);
+
+            // fixed in 7.3 - 7.2.8 used to...
+            Assert.Throws<FileSecurityException>(() =>
+            {
+                partialView = (PartialView) repository.Get("\\test-path-4.cshtml"); // outside the filesystem, does not exist
+            });
+            Assert.Throws<FileSecurityException>(() =>
+            {
+                partialView = (PartialView) repository.Get("../../packages.config"); // outside the filesystem, exists
+            });
+        }
+
+        [TearDown]
+        public override void TearDown()
+        {
+            base.TearDown();
+
+            //Delete all files
+            Purge((PhysicalFileSystem)_fileSystem, "");
+            _fileSystem = null;
+        }
+
+        private void Purge(PhysicalFileSystem fs, string path)
+        {
+            var files = fs.GetFiles(path, "*.cshtml");
+            foreach (var file in files)
+            {
+                fs.DeleteFile(file);
+            }
+            var dirs = fs.GetDirectories(path);
+            foreach (var dir in dirs)
+            {
+                Purge(fs, dir);
+                fs.DeleteDirectory(dir);
+            }
+        }
+    }
+}

src/Umbraco.Tests/Persistence/Repositories/ScriptRepositoryTest.cs

@@ -220,15 +220,79 @@ namespace Umbraco.Tests.Persistence.Repositories
             Assert.AreEqual(content, script.Content);
         }
 
+        [Test]
+        public void PathTests()
+        {
+            // unless noted otherwise, no changes / 7.2.8
+
+            var provider = new FileUnitOfWorkProvider();
+            var unitOfWork = provider.GetUnitOfWork();
+            var repository = new ScriptRepository(unitOfWork, _fileSystem, Mock.Of<IContentSection>());
+
+            var script = new Script("test-path-1.js") { Content = "// script" };
+            repository.AddOrUpdate(script);
+            unitOfWork.Commit();
+            Assert.IsTrue(_fileSystem.FileExists("test-path-1.js"));
+            Assert.AreEqual("test-path-1.js", script.Path);
+            Assert.AreEqual("/scripts/test-path-1.js", script.VirtualPath);
+
+            script = new Script("path-2/test-path-2.js") { Content = "// script" };
+            repository.AddOrUpdate(script);
+            unitOfWork.Commit();
+            Assert.IsTrue(_fileSystem.FileExists("path-2/test-path-2.js"));
+            Assert.AreEqual("path-2\\test-path-2.js", script.Path); // fixed in 7.3 - 7.2.8 does not update the path
+            Assert.AreEqual("/scripts/path-2/test-path-2.js", script.VirtualPath);
+
+            script = repository.Get("path-2/test-path-2.js");
+            Assert.IsNotNull(script);
+            Assert.AreEqual("path-2\\test-path-2.js", script.Path);
+            Assert.AreEqual("/scripts/path-2/test-path-2.js", script.VirtualPath);
+
+            script = new Script("path-2\\test-path-3.js") { Content = "// script" };
+            repository.AddOrUpdate(script);
+            unitOfWork.Commit();
+            Assert.IsTrue(_fileSystem.FileExists("path-2/test-path-3.js"));
+            Assert.AreEqual("path-2\\test-path-3.js", script.Path);
+            Assert.AreEqual("/scripts/path-2/test-path-3.js", script.VirtualPath);
+
+            script = repository.Get("path-2/test-path-3.js");
+            Assert.IsNotNull(script);
+            Assert.AreEqual("path-2\\test-path-3.js", script.Path);
+            Assert.AreEqual("/scripts/path-2/test-path-3.js", script.VirtualPath);
+
+            script = repository.Get("path-2\\test-path-3.js");
+            Assert.IsNotNull(script);
+            Assert.AreEqual("path-2\\test-path-3.js", script.Path);
+            Assert.AreEqual("/scripts/path-2/test-path-3.js", script.VirtualPath);
+
+            script = new Script("\\test-path-4.js") { Content = "// script" };
+            Assert.Throws<FileSecurityException>(() => // fixed in 7.3 - 7.2.8 used to strip the \
+            {
+                repository.AddOrUpdate(script);
+            });
+
+            script = repository.Get("missing.js");
+            Assert.IsNull(script);
+
+            // fixed in 7.3 - 7.2.8 used to...
+            Assert.Throws<FileSecurityException>(() =>
+            {
+                script = repository.Get("\\test-path-4.js"); // outside the filesystem, does not exist
+            });
+            Assert.Throws<FileSecurityException>(() =>
+            {
+                script = repository.Get("../packages.config"); // outside the filesystem, exists
+            });
+        }
+
         [TearDown]
         public override void TearDown()
         {
             base.TearDown();
 
-            _fileSystem = null;
             //Delete all files
-	        var fs = new PhysicalFileSystem(SystemDirectories.Scripts);
-            Purge(fs, "");
+            Purge((PhysicalFileSystem) _fileSystem, "");
+            _fileSystem = null;
         }
 
         private void Purge(PhysicalFileSystem fs, string path)

src/Umbraco.Tests/Persistence/Repositories/StylesheetRepositoryTest.cs

@@ -1,4 +1,5 @@
-using System.Data;
+using System;
+using System.Data;
 using System.IO;
 using System.Linq;
 using System.Text;
@@ -241,18 +242,95 @@ p{font-size:2em;}"));
             Assert.That(exists, Is.True);
         }
 
+        [Test]
+        public void PathTests()
+        {
+            // unless noted otherwise, no changes / 7.2.8
+
+            var provider = new FileUnitOfWorkProvider();
+            var unitOfWork = provider.GetUnitOfWork();
+            var repository = new StylesheetRepository(unitOfWork, _fileSystem);
+
+            var stylesheet = new Stylesheet("test-path-1.css") { Content = "body { color:#000; } .bold {font-weight:bold;}" };
+            repository.AddOrUpdate(stylesheet);
+            unitOfWork.Commit();
+            Assert.IsTrue(_fileSystem.FileExists("test-path-1.css"));
+            Assert.AreEqual("test-path-1.css", stylesheet.Path);
+            Assert.AreEqual("/css/test-path-1.css", stylesheet.VirtualPath);
+
+            stylesheet = new Stylesheet("path-2/test-path-2.css") { Content = "body { color:#000; } .bold {font-weight:bold;}" };
+            repository.AddOrUpdate(stylesheet);
+            unitOfWork.Commit();
+            Assert.IsTrue(_fileSystem.FileExists("path-2/test-path-2.css"));
+            Assert.AreEqual("path-2\\test-path-2.css", stylesheet.Path); // fixed in 7.3 - 7.2.8 does not update the path
+            Assert.AreEqual("/css/path-2/test-path-2.css", stylesheet.VirtualPath);
+
+            stylesheet = repository.Get("path-2/test-path-2.css");
+            Assert.IsNotNull(stylesheet);
+            Assert.AreEqual("path-2\\test-path-2.css", stylesheet.Path);
+            Assert.AreEqual("/css/path-2/test-path-2.css", stylesheet.VirtualPath);
+
+            stylesheet = new Stylesheet("path-2\\test-path-3.css") { Content = "body { color:#000; } .bold {font-weight:bold;}" };
+            repository.AddOrUpdate(stylesheet);
+            unitOfWork.Commit();
+            Assert.IsTrue(_fileSystem.FileExists("path-2/test-path-3.css"));
+            Assert.AreEqual("path-2\\test-path-3.css", stylesheet.Path);
+            Assert.AreEqual("/css/path-2/test-path-3.css", stylesheet.VirtualPath);
+
+            stylesheet = repository.Get("path-2/test-path-3.css");
+            Assert.IsNotNull(stylesheet);
+            Assert.AreEqual("path-2\\test-path-3.css", stylesheet.Path);
+            Assert.AreEqual("/css/path-2/test-path-3.css", stylesheet.VirtualPath);
+
+            stylesheet = repository.Get("path-2\\test-path-3.css");
+            Assert.IsNotNull(stylesheet);
+            Assert.AreEqual("path-2\\test-path-3.css", stylesheet.Path);
+            Assert.AreEqual("/css/path-2/test-path-3.css", stylesheet.VirtualPath);
+
+            stylesheet = new Stylesheet("\\test-path-4.css") { Content = "body { color:#000; } .bold {font-weight:bold;}" };
+            Assert.Throws<FileSecurityException>(() => // fixed in 7.3 - 7.2.8 used to strip the \
+            {
+                repository.AddOrUpdate(stylesheet);
+            });
+
+            // fixed in 7.3 - 7.2.8 used to throw
+            stylesheet = repository.Get("missing.css");
+            Assert.IsNull(stylesheet);
+
+            // fixed in 7.3 - 7.2.8 used to...
+            Assert.Throws<FileSecurityException>(() =>
+            {
+                stylesheet = repository.Get("\\test-path-4.css"); // outside the filesystem, does not exist
+            });
+            Assert.Throws<FileSecurityException>(() =>
+            {
+                stylesheet = repository.Get("../packages.config"); // outside the filesystem, exists
+            });
+        }
+
         [TearDown]
-        public void TearDown()
+        public override void TearDown()
         {
             base.TearDown();
 
             //Delete all files
-            var files = _fileSystem.GetFiles("", "*.css");
+            Purge((PhysicalFileSystem) _fileSystem, "");
+            _fileSystem = null;
+        }
+
+        private void Purge(PhysicalFileSystem fs, string path)
+        {
+            var files = fs.GetFiles(path, "*.css");
             foreach (var file in files)
             {
-                _fileSystem.DeleteFile(file);
+                fs.DeleteFile(file);
+            }
+            var dirs = fs.GetDirectories(path);
+            foreach (var dir in dirs)
+            {
+                Purge(fs, dir);
+                fs.DeleteDirectory(dir);
             }
-            _fileSystem = null;
         }
 
         protected Stream CreateStream(string contents = null)

src/Umbraco.Tests/Umbraco.Tests.csproj

@@ -182,6 +182,7 @@
     <Compile Include="Logging\ParallelForwarderTest.cs" />
     <Compile Include="Persistence\Repositories\AuditRepositoryTest.cs" />
     <Compile Include="Persistence\Repositories\DomainRepositoryTest.cs" />
+    <Compile Include="Persistence\Repositories\PartialViewRepositoryTests.cs" />
     <Compile Include="Persistence\Repositories\PublicAccessRepositoryTest.cs" />
     <Compile Include="Persistence\Repositories\TaskRepositoryTest.cs" />
     <Compile Include="Persistence\Repositories\TaskTypeRepositoryTest.cs" />

src/Umbraco.Web/WebServices/SaveFileController.cs

@@ -75,6 +75,11 @@ namespace Umbraco.Web.WebServices
             Func<IFileService, IPartialView, bool> validate,
             Func<IFileService, IPartialView, Attempt<IPartialView>> save)
         {
+            // sanitize input - partial view names have an extension
+            filename = filename
+                .Replace('\\', '/')
+                .TrimStart('/');
+
             // sharing the editor with partial views & partial view macros,
             // using path prefix to differenciate,
             // but the file service manages different filesystems,
@@ -85,8 +90,12 @@ namespace Umbraco.Web.WebServices
             if (filename.InvariantStartsWith(pathPrefix))
                 filename = filename.TrimStart(pathPrefix);
 
-            if (oldname != null && oldname.InvariantStartsWith(pathPrefix))
-                oldname = oldname.TrimStart(pathPrefix);
+            if (oldname != null)
+            {
+                oldname = oldname.TrimStart('/', '\\');
+                if (oldname.InvariantStartsWith(pathPrefix))
+                    oldname = oldname.TrimStart(pathPrefix);
+            }
 
             var view = get(svce, oldname);
             if (view == null)
@@ -187,7 +196,10 @@ namespace Umbraco.Web.WebServices
         [HttpPost]
         public JsonResult SaveScript(string filename, string oldName, string contents)
         {
-            filename = filename.TrimStart('/', '\\');
+            // sanitize input - script names have an extension
+            filename = filename
+                .Replace('\\', '/')
+                .TrimStart('/');
 
             var svce = (FileService) Services.FileService;
             var script = svce.GetScriptByName(oldName);
@@ -223,7 +235,11 @@ namespace Umbraco.Web.WebServices
         [HttpPost]
         public JsonResult SaveStylesheet(string filename, string oldName, string contents)
         {
-            filename = filename.TrimStart('/', '\\').EnsureEndsWith(".css");
+            // sanitize input - stylesheet names have no extension
+            filename = filename
+                .Replace('\\', '/')
+                .TrimStart('/')
+                .EnsureEndsWith(".css");
 
             var svce = (FileService) Services.FileService;
             var stylesheet = svce.GetStylesheetByName(oldName);