From 82bb1f00fc4a714ab5d8cecfb5ad906889d8411a Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 6 Dec 2019 09:49:10 +0100 Subject: [PATCH] AB4079 - Review fixes --- .../Configuration/IGlobalSettings.cs | 9 ------ .../Configuration/IHostingSettings.cs | 11 +++++++ src/Umbraco.Abstractions/IO/IIOHelper.cs | 1 - src/Umbraco.Abstractions/IO/IOHelper.cs | 14 ++++----- src/Umbraco.Abstractions/StringExtensions.cs | 15 ---------- .../Strings/PathUtility.cs | 22 ++++++++++++++ src/Umbraco.Configuration/GlobalSettings.cs | 24 +++++++++------ .../Enrichers/HttpRequestIdEnricher.cs | 14 ++++----- .../Enrichers/HttpRequestNumberEnricher.cs | 11 ++++--- .../Logging/Serilog/LoggerConfigExtensions.cs | 6 ++-- .../Logging/Serilog/SerilogLogger.cs | 4 +-- src/Umbraco.Tests/IO/IoHelperTests.cs | 29 +++---------------- .../AuthenticationControllerTests.cs | 2 +- .../Mvc/UmbracoViewPageOfTModel.cs | 8 ++--- src/Umbraco.Web/UmbracoApplicationBase.cs | 3 +- 15 files changed, 78 insertions(+), 95 deletions(-) create mode 100644 src/Umbraco.Abstractions/Configuration/IHostingSettings.cs create mode 100644 src/Umbraco.Abstractions/Strings/PathUtility.cs diff --git a/src/Umbraco.Abstractions/Configuration/IGlobalSettings.cs b/src/Umbraco.Abstractions/Configuration/IGlobalSettings.cs index 25466d6cf7..212e6f1843 100644 --- a/src/Umbraco.Abstractions/Configuration/IGlobalSettings.cs +++ b/src/Umbraco.Abstractions/Configuration/IGlobalSettings.cs @@ -1,14 +1,5 @@ namespace Umbraco.Core.Configuration { - public interface IHostingSettings - { - bool DebugMode { get; } - /// - /// Gets the configuration for the location of temporary files. - /// - LocalTempStorage LocalTempStorageLocation { get; } - } - /// /// Contains general settings information for the entire Umbraco instance based on information from web.config appsettings /// diff --git a/src/Umbraco.Abstractions/Configuration/IHostingSettings.cs b/src/Umbraco.Abstractions/Configuration/IHostingSettings.cs new file mode 100644 index 0000000000..ef399177a4 --- /dev/null +++ b/src/Umbraco.Abstractions/Configuration/IHostingSettings.cs @@ -0,0 +1,11 @@ +namespace Umbraco.Core.Configuration +{ + public interface IHostingSettings + { + bool DebugMode { get; } + /// + /// Gets the configuration for the location of temporary files. + /// + LocalTempStorage LocalTempStorageLocation { get; } + } +} diff --git a/src/Umbraco.Abstractions/IO/IIOHelper.cs b/src/Umbraco.Abstractions/IO/IIOHelper.cs index 2d535a8d1e..e8ef6d2973 100644 --- a/src/Umbraco.Abstractions/IO/IIOHelper.cs +++ b/src/Umbraco.Abstractions/IO/IIOHelper.cs @@ -11,7 +11,6 @@ namespace Umbraco.Core.IO string ResolveVirtualUrl(string path); string ResolveUrl(string virtualPath); Attempt TryResolveUrl(string virtualPath); - string MapPath(string path, bool useHttpContext); string MapPath(string path); diff --git a/src/Umbraco.Abstractions/IO/IOHelper.cs b/src/Umbraco.Abstractions/IO/IOHelper.cs index 672228e3ca..d5e1335f35 100644 --- a/src/Umbraco.Abstractions/IO/IOHelper.cs +++ b/src/Umbraco.Abstractions/IO/IOHelper.cs @@ -5,6 +5,7 @@ using System.Reflection; using System.IO; using System.Linq; using Umbraco.Core.Hosting; +using Umbraco.Core.Strings; namespace Umbraco.Core.IO { @@ -78,10 +79,9 @@ namespace Umbraco.Core.IO } } - public string MapPath(string path, bool useHttpContext) + public string MapPath(string path) { - if (path == null) throw new ArgumentNullException("path"); - useHttpContext = useHttpContext && _hostingEnvironment.IsHosted; + if (path == null) throw new ArgumentNullException(nameof(path)); // Check if the path is already mapped if ((path.Length >= 2 && path[1] == Path.VolumeSeparatorChar) @@ -93,7 +93,7 @@ namespace Umbraco.Core.IO // http://umbraco.codeplex.com/workitem/30946 - if (useHttpContext && _hostingEnvironment.IsHosted) + if (_hostingEnvironment.IsHosted) { var result = (String.IsNullOrEmpty(path) == false && (path.StartsWith("~") || path.StartsWith(Root))) ? _hostingEnvironment.MapPath(path) @@ -113,10 +113,6 @@ namespace Umbraco.Core.IO return retval; } - public string MapPath(string path) - { - return MapPath(path, true); - } /// /// Verifies that the current filepath matches a directory where the user is allowed to edit a file. @@ -276,7 +272,7 @@ namespace Umbraco.Core.IO path = relativePath; } - return path.EnsurePathIsApplicationRootPrefixed(); + return PathUtility.EnsurePathIsApplicationRootPrefixed(path); } private string _root; diff --git a/src/Umbraco.Abstractions/StringExtensions.cs b/src/Umbraco.Abstractions/StringExtensions.cs index 37a6ae4258..c838e3ec24 100644 --- a/src/Umbraco.Abstractions/StringExtensions.cs +++ b/src/Umbraco.Abstractions/StringExtensions.cs @@ -1215,21 +1215,6 @@ namespace Umbraco.Core public static string NullOrWhiteSpaceAsNull(this string text) => string.IsNullOrWhiteSpace(text) ? null : text; - /// - /// Ensures that a path has `~/` as prefix - /// - /// - /// - public static string EnsurePathIsApplicationRootPrefixed(this string path) - { - if (path.StartsWith("~/")) - return path; - if (path.StartsWith("/") == false && path.StartsWith("\\") == false) - path = string.Format("/{0}", path); - if (path.StartsWith("~") == false) - path = string.Format("~{0}", path); - return path; - } /// /// Checks if a given path is a full path including drive letter diff --git a/src/Umbraco.Abstractions/Strings/PathUtility.cs b/src/Umbraco.Abstractions/Strings/PathUtility.cs new file mode 100644 index 0000000000..973f1dccf2 --- /dev/null +++ b/src/Umbraco.Abstractions/Strings/PathUtility.cs @@ -0,0 +1,22 @@ +namespace Umbraco.Core.Strings +{ + public static class PathUtility + { + + /// + /// Ensures that a path has `~/` as prefix + /// + /// + /// + public static string EnsurePathIsApplicationRootPrefixed(string path) + { + if (path.StartsWith("~/")) + return path; + if (path.StartsWith("/") == false && path.StartsWith("\\") == false) + path = string.Format("/{0}", path); + if (path.StartsWith("~") == false) + path = string.Format("~{0}", path); + return path; + } + } +} diff --git a/src/Umbraco.Configuration/GlobalSettings.cs b/src/Umbraco.Configuration/GlobalSettings.cs index fea5de726f..ed7fdc2a91 100644 --- a/src/Umbraco.Configuration/GlobalSettings.cs +++ b/src/Umbraco.Configuration/GlobalSettings.cs @@ -8,6 +8,8 @@ namespace Umbraco.Core.Configuration { public class HostingSettings : IHostingSettings { + private bool? _debugMode; + /// public LocalTempStorage LocalTempStorageLocation { @@ -29,21 +31,25 @@ namespace Umbraco.Core.Configuration { get { - try + if (!_debugMode.HasValue) { - if (ConfigurationManager.GetSection("system.web/compilation") is ConfigurationSection compilation) + try { - var debugElement = compilation.ElementInformation.Properties["debug"]; + if (ConfigurationManager.GetSection("system.web/compilation") is ConfigurationSection compilation) + { + var debugElement = compilation.ElementInformation.Properties["debug"]; - return debugElement != null && (debugElement.Value is bool debug && debug); + _debugMode = debugElement != null && (debugElement.Value is bool debug && debug); + + } + } + catch + { + _debugMode = false; } } - catch - { - // ignored - } - return false; + return _debugMode.GetValueOrDefault(); } } } diff --git a/src/Umbraco.Core/Logging/Serilog/Enrichers/HttpRequestIdEnricher.cs b/src/Umbraco.Core/Logging/Serilog/Enrichers/HttpRequestIdEnricher.cs index f1dd04bf76..45468ace9f 100644 --- a/src/Umbraco.Core/Logging/Serilog/Enrichers/HttpRequestIdEnricher.cs +++ b/src/Umbraco.Core/Logging/Serilog/Enrichers/HttpRequestIdEnricher.cs @@ -13,11 +13,11 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers /// internal class HttpRequestIdEnricher : ILogEventEnricher { - private readonly Func _factoryGetter; + private readonly Func _requestCacheGetter; - public HttpRequestIdEnricher(Func factoryGetter) + public HttpRequestIdEnricher(Func requestCacheGetter) { - _factoryGetter = factoryGetter; + _requestCacheGetter = requestCacheGetter; } /// @@ -32,12 +32,10 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers /// Factory for creating new properties to add to the event. public void Enrich(LogEvent logEvent, ILogEventPropertyFactory propertyFactory) { - if (logEvent == null) throw new ArgumentNullException("logEvent"); + if (logEvent == null) throw new ArgumentNullException(nameof(logEvent)); - var factory = _factoryGetter(); - if(factory is null) return; - - var requestCache = factory.GetInstance(); + var requestCache = _requestCacheGetter(); + if(requestCache is null) return; Guid requestId; if (!LogHttpRequest.TryGetCurrentHttpRequestId(out requestId, requestCache)) diff --git a/src/Umbraco.Core/Logging/Serilog/Enrichers/HttpRequestNumberEnricher.cs b/src/Umbraco.Core/Logging/Serilog/Enrichers/HttpRequestNumberEnricher.cs index 5142f3d30f..08eb6b93f0 100644 --- a/src/Umbraco.Core/Logging/Serilog/Enrichers/HttpRequestNumberEnricher.cs +++ b/src/Umbraco.Core/Logging/Serilog/Enrichers/HttpRequestNumberEnricher.cs @@ -15,7 +15,7 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers /// internal class HttpRequestNumberEnricher : ILogEventEnricher { - private readonly Func _factoryFunc; + private readonly Func _requestCacheGetter; private static int _lastRequestNumber; private static readonly string _requestNumberItemName = typeof(HttpRequestNumberEnricher).Name + "+RequestNumber"; @@ -25,9 +25,9 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers private const string _httpRequestNumberPropertyName = "HttpRequestNumber"; - public HttpRequestNumberEnricher(Func factoryFunc) + public HttpRequestNumberEnricher(Func requestCacheGetter) { - _factoryFunc = factoryFunc; + _requestCacheGetter = requestCacheGetter; } /// @@ -39,10 +39,9 @@ namespace Umbraco.Core.Logging.Serilog.Enrichers { if (logEvent == null) throw new ArgumentNullException(nameof(logEvent)); - var factory = _factoryFunc(); - if (factory is null) return; + var requestCache = _requestCacheGetter(); + if (requestCache is null) return; - var requestCache = factory.GetInstance(); var requestNumber = requestCache.Get(_requestNumberItemName, () => Interlocked.Increment(ref _lastRequestNumber)); diff --git a/src/Umbraco.Core/Logging/Serilog/LoggerConfigExtensions.cs b/src/Umbraco.Core/Logging/Serilog/LoggerConfigExtensions.cs index 39b02d4684..f4e8f85281 100644 --- a/src/Umbraco.Core/Logging/Serilog/LoggerConfigExtensions.cs +++ b/src/Umbraco.Core/Logging/Serilog/LoggerConfigExtensions.cs @@ -25,7 +25,7 @@ namespace Umbraco.Core.Logging.Serilog /// /// A Serilog LoggerConfiguration /// - public static LoggerConfiguration MinimalConfiguration(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment, ISessionIdResolver sessionIdResolver, Func factoryFunc) + public static LoggerConfiguration MinimalConfiguration(this LoggerConfiguration logConfig, IHostingEnvironment hostingEnvironment, ISessionIdResolver sessionIdResolver, Func requestCacheGetter) { global::Serilog.Debugging.SelfLog.Enable(msg => System.Diagnostics.Debug.WriteLine(msg)); @@ -43,8 +43,8 @@ namespace Umbraco.Core.Logging.Serilog .Enrich.WithProperty("MachineName", Environment.MachineName) .Enrich.With() .Enrich.With(new HttpSessionIdEnricher(sessionIdResolver)) - .Enrich.With(new HttpRequestNumberEnricher(factoryFunc)) - .Enrich.With(new HttpRequestIdEnricher(factoryFunc)); + .Enrich.With(new HttpRequestNumberEnricher(requestCacheGetter)) + .Enrich.With(new HttpRequestIdEnricher(requestCacheGetter)); return logConfig; } diff --git a/src/Umbraco.Core/Logging/Serilog/SerilogLogger.cs b/src/Umbraco.Core/Logging/Serilog/SerilogLogger.cs index 47623145ef..af5f712489 100644 --- a/src/Umbraco.Core/Logging/Serilog/SerilogLogger.cs +++ b/src/Umbraco.Core/Logging/Serilog/SerilogLogger.cs @@ -38,11 +38,11 @@ namespace Umbraco.Core.Logging.Serilog /// Creates a logger with some pre-defined configuration and remainder from config file /// /// Used by UmbracoApplicationBase to get its logger. - public static SerilogLogger CreateWithDefaultConfiguration(IHostingEnvironment hostingEnvironment, ISessionIdResolver sessionIdResolver, Func factoryFunc) + public static SerilogLogger CreateWithDefaultConfiguration(IHostingEnvironment hostingEnvironment, ISessionIdResolver sessionIdResolver, Func requestCacheGetter) { var loggerConfig = new LoggerConfiguration(); loggerConfig - .MinimalConfiguration(hostingEnvironment, sessionIdResolver, factoryFunc) + .MinimalConfiguration(hostingEnvironment, sessionIdResolver, requestCacheGetter) .ReadFromConfigFile() .ReadFromUserConfigFile(); diff --git a/src/Umbraco.Tests/IO/IoHelperTests.cs b/src/Umbraco.Tests/IO/IoHelperTests.cs index 910feb47be..9458f384ce 100644 --- a/src/Umbraco.Tests/IO/IoHelperTests.cs +++ b/src/Umbraco.Tests/IO/IoHelperTests.cs @@ -4,6 +4,7 @@ using Umbraco.Core.IO; using Umbraco.Core; using Umbraco.Core.Composing; using Umbraco.Core.Configuration; +using Umbraco.Core.Strings; using Umbraco.Tests.TestHelpers; namespace Umbraco.Tests.IO @@ -30,35 +31,13 @@ namespace Umbraco.Tests.IO } } - /// - ///A test for MapPath verifying that HttpContext method (which includes vdirs) matches non-HttpContext method - /// - [Test] - public void IOHelper_MapPathTestVDirTraversal() - { - //System.Diagnostics.Debugger.Break(); - var globalSettings = SettingsForTests.GenerateMockGlobalSettings(); - - Assert.AreEqual(_ioHelper.MapPath(Constants.SystemDirectories.Bin, true), _ioHelper.MapPath(Constants.SystemDirectories.Bin, false)); - Assert.AreEqual(_ioHelper.MapPath(Constants.SystemDirectories.Config, true), _ioHelper.MapPath(Constants.SystemDirectories.Config, false)); - Assert.AreEqual(_ioHelper.MapPath(globalSettings.UmbracoCssPath, true), _ioHelper.MapPath(globalSettings.UmbracoCssPath, false)); - Assert.AreEqual(_ioHelper.MapPath(Constants.SystemDirectories.Data, true), _ioHelper.MapPath(Constants.SystemDirectories.Data, false)); - Assert.AreEqual(_ioHelper.MapPath(Constants.SystemDirectories.Install, true), _ioHelper.MapPath(Constants.SystemDirectories.Install, false)); - Assert.AreEqual(_ioHelper.MapPath(globalSettings.UmbracoMediaPath, true), _ioHelper.MapPath(globalSettings.UmbracoMediaPath, false)); - Assert.AreEqual(_ioHelper.MapPath(Constants.SystemDirectories.Packages, true), _ioHelper.MapPath(Constants.SystemDirectories.Packages, false)); - Assert.AreEqual(_ioHelper.MapPath(Constants.SystemDirectories.Preview, true), _ioHelper.MapPath(Constants.SystemDirectories.Preview, false)); - Assert.AreEqual(_ioHelper.MapPath(_ioHelper.Root, true), _ioHelper.MapPath(_ioHelper.Root, false)); - Assert.AreEqual(_ioHelper.MapPath(globalSettings.UmbracoScriptsPath, true), _ioHelper.MapPath(globalSettings.UmbracoScriptsPath, false)); - Assert.AreEqual(_ioHelper.MapPath(globalSettings.UmbracoPath, true), _ioHelper.MapPath(globalSettings.UmbracoPath, false)); - } - [Test] public void EnsurePathIsApplicationRootPrefixed() { //Assert - Assert.AreEqual("~/Views/Template.cshtml", "Views/Template.cshtml".EnsurePathIsApplicationRootPrefixed()); - Assert.AreEqual("~/Views/Template.cshtml", "/Views/Template.cshtml".EnsurePathIsApplicationRootPrefixed()); - Assert.AreEqual("~/Views/Template.cshtml", "~/Views/Template.cshtml".EnsurePathIsApplicationRootPrefixed()); + Assert.AreEqual("~/Views/Template.cshtml", PathUtility.EnsurePathIsApplicationRootPrefixed("Views/Template.cshtml")); + Assert.AreEqual("~/Views/Template.cshtml", PathUtility.EnsurePathIsApplicationRootPrefixed("/Views/Template.cshtml")); + Assert.AreEqual("~/Views/Template.cshtml", PathUtility.EnsurePathIsApplicationRootPrefixed("~/Views/Template.cshtml")); } } } diff --git a/src/Umbraco.Tests/Web/Controllers/AuthenticationControllerTests.cs b/src/Umbraco.Tests/Web/Controllers/AuthenticationControllerTests.cs index 4d7815b991..e99d3eb165 100644 --- a/src/Umbraco.Tests/Web/Controllers/AuthenticationControllerTests.cs +++ b/src/Umbraco.Tests/Web/Controllers/AuthenticationControllerTests.cs @@ -71,7 +71,7 @@ namespace Umbraco.Tests.Web.Controllers } else { - var baseDir = IOHelper.MapPath("", false).TrimEnd(IOHelper.DirSepChar); + var baseDir = IOHelper.MapPath("").TrimEnd(IOHelper.DirSepChar); HttpContext.Current = new HttpContext(new SimpleWorkerRequest("/", baseDir, "", "", new StringWriter())); } IOHelper.ForceNotHosted = true; diff --git a/src/Umbraco.Web/Mvc/UmbracoViewPageOfTModel.cs b/src/Umbraco.Web/Mvc/UmbracoViewPageOfTModel.cs index 8e231de38d..c25a42195e 100644 --- a/src/Umbraco.Web/Mvc/UmbracoViewPageOfTModel.cs +++ b/src/Umbraco.Web/Mvc/UmbracoViewPageOfTModel.cs @@ -5,10 +5,6 @@ using System.Web.Mvc; using System.Web.WebPages; using Umbraco.Core; using Umbraco.Core.Cache; -using Umbraco.Core.Composing; -using Umbraco.Core.Configuration; -using Umbraco.Core.Dictionary; -using Umbraco.Core.IO; using Umbraco.Core.Models.PublishedContent; using Umbraco.Core.Services; using Umbraco.Core.Strings; @@ -238,9 +234,9 @@ namespace Umbraco.Web.Mvc public override void Write(object value) { - if (value is IHtmlEncodedString) + if (value is IHtmlEncodedString htmlEncodedString) { - base.WriteLiteral(value); + base.WriteLiteral(htmlEncodedString.ToHtmlString()); } else { diff --git a/src/Umbraco.Web/UmbracoApplicationBase.cs b/src/Umbraco.Web/UmbracoApplicationBase.cs index 313df4b619..b93e52088f 100644 --- a/src/Umbraco.Web/UmbracoApplicationBase.cs +++ b/src/Umbraco.Web/UmbracoApplicationBase.cs @@ -4,6 +4,7 @@ using System.Threading; using System.Web; using System.Web.Hosting; using Umbraco.Core; +using Umbraco.Core.Cache; using Umbraco.Core.Composing; using Umbraco.Core.Configuration; using Umbraco.Core.Hosting; @@ -45,7 +46,7 @@ namespace Umbraco.Web _profiler = new LogProfiler(_logger); - _logger = SerilogLogger.CreateWithDefaultConfiguration(_hostingEnvironment, new AspNetSessionIdResolver(), () => _factory); + _logger = SerilogLogger.CreateWithDefaultConfiguration(_hostingEnvironment, new AspNetSessionIdResolver(), () => _factory?.GetInstance()); _backOfficeInfo = new AspNetBackOfficeInfo(_configs.Global(), _ioHelper, _configs.Settings(), _logger); }