diff --git a/src/Umbraco.Core/Cache/Refreshers/Implement/DataTypeCacheRefresher.cs b/src/Umbraco.Core/Cache/Refreshers/Implement/DataTypeCacheRefresher.cs index ea661c5498..394630fa64 100644 --- a/src/Umbraco.Core/Cache/Refreshers/Implement/DataTypeCacheRefresher.cs +++ b/src/Umbraco.Core/Cache/Refreshers/Implement/DataTypeCacheRefresher.cs @@ -88,10 +88,6 @@ public sealed class DataTypeCacheRefresher : PayloadCacheRefresherBase _publishedSnapshotService.Notify(payloads)); diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs index f35229e4e0..098f770ebc 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -326,6 +326,9 @@ namespace Umbraco.Cms.Core.DependencyInjection Services.AddUnique(provider => new CultureImpactFactory(provider.GetRequiredService>())); Services.AddUnique(); Services.AddUnique(); + + // Register filestream security analyzers + Services.AddUnique(); } } } diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/en.xml b/src/Umbraco.Core/EmbeddedResources/Lang/en.xml index 79d6705b35..da5208e892 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/en.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/en.xml @@ -342,6 +342,7 @@ Failed to create a folder under parent id %0% Failed to rename the folder with id %0% Drag and drop your file(s) into the area + One or more file security validations have failed Create a new member diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml b/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml index de472144a1..57cf89bc2d 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml @@ -353,6 +353,7 @@ Failed to rename the folder with id %0% Drag and drop your file(s) into the area Upload is not allowed in this location. + One or more file security validations have failed Create a new member diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml b/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml index 918cdd5cb6..0452178f92 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/nl.xml @@ -340,6 +340,7 @@ Kan de map met id %0% niet hernoemen Sleep en zet je bestand(en) neer in dit gebied Upload is niet toegelaten in deze locatie. + Een of meerdere veiligheid validaties zijn gefaald voor het bestand Maak nieuw lid aan diff --git a/src/Umbraco.Core/Extensions/TypeExtensions.cs b/src/Umbraco.Core/Extensions/TypeExtensions.cs index e3da8d9ee1..1416888e73 100644 --- a/src/Umbraco.Core/Extensions/TypeExtensions.cs +++ b/src/Umbraco.Core/Extensions/TypeExtensions.cs @@ -219,96 +219,52 @@ public static class TypeExtensions /// public static PropertyInfo[] GetAllProperties(this Type type) { - if (type.IsInterface) - { - var propertyInfos = new List(); - - var considered = new List(); - var queue = new Queue(); - considered.Add(type); - queue.Enqueue(type); - while (queue.Count > 0) - { - Type subType = queue.Dequeue(); - foreach (Type subInterface in subType.GetInterfaces()) - { - if (considered.Contains(subInterface)) - { - continue; - } - - considered.Add(subInterface); - queue.Enqueue(subInterface); - } - - PropertyInfo[] typeProperties = subType.GetProperties( - BindingFlags.FlattenHierarchy - | BindingFlags.Public - | BindingFlags.NonPublic - | BindingFlags.Instance); - - IEnumerable newPropertyInfos = typeProperties - .Where(x => !propertyInfos.Contains(x)); - - propertyInfos.InsertRange(0, newPropertyInfos); - } - - return propertyInfos.ToArray(); - } - - return type.GetProperties(BindingFlags.FlattenHierarchy - | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.NonPublic + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetProperties(bindingFlags)); } /// - /// Returns all public properties including inherited properties even for interfaces + /// Returns public properties including inherited properties even for interfaces /// /// /// - /// - /// taken from - /// http://stackoverflow.com/questions/358835/getproperties-to-return-all-properties-for-an-interface-inheritance-hierarchy - /// public static PropertyInfo[] GetPublicProperties(this Type type) { - if (type.IsInterface) - { - var propertyInfos = new List(); + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetProperties(bindingFlags)); + } - var considered = new List(); - var queue = new Queue(); - considered.Add(type); - queue.Enqueue(type); - while (queue.Count > 0) - { - Type subType = queue.Dequeue(); - foreach (Type subInterface in subType.GetInterfaces()) - { - if (considered.Contains(subInterface)) - { - continue; - } + /// + /// Returns public methods including inherited methods even for interfaces + /// + /// + /// + public static MethodInfo[] GetPublicMethods(this Type type) + { + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetMethods(bindingFlags)); + } - considered.Add(subInterface); - queue.Enqueue(subInterface); - } - - PropertyInfo[] typeProperties = subType.GetProperties( - BindingFlags.FlattenHierarchy - | BindingFlags.Public - | BindingFlags.Instance); - - IEnumerable newPropertyInfos = typeProperties - .Where(x => !propertyInfos.Contains(x)); - - propertyInfos.InsertRange(0, newPropertyInfos); - } - - return propertyInfos.ToArray(); - } - - return type.GetProperties(BindingFlags.FlattenHierarchy - | BindingFlags.Public | BindingFlags.Instance); + /// + /// Returns all methods including inherited methods even for interfaces + /// + /// Includes both Public and Non-Public methods + /// + /// + public static MethodInfo[] GetAllMethods(this Type type) + { + const BindingFlags bindingFlags = BindingFlags.FlattenHierarchy + | BindingFlags.Public + | BindingFlags.NonPublic + | BindingFlags.Instance; + return type.GetAllMemberInfos(t => t.GetMethods(bindingFlags)); } /// @@ -512,4 +468,47 @@ public static class TypeExtensions return attempt; } + + /// + /// taken from + /// http://stackoverflow.com/questions/358835/getproperties-to-return-all-properties-for-an-interface-inheritance-hierarchy + /// + private static T[] GetAllMemberInfos(this Type type, Func getMemberInfos) + where T : MemberInfo + { + if (type.IsInterface is false) + { + return getMemberInfos(type); + } + + var memberInfos = new List(); + + var considered = new List(); + var queue = new Queue(); + considered.Add(type); + queue.Enqueue(type); + while (queue.Count > 0) + { + Type subType = queue.Dequeue(); + foreach (Type subInterface in subType.GetInterfaces()) + { + if (considered.Contains(subInterface)) + { + continue; + } + + considered.Add(subInterface); + queue.Enqueue(subInterface); + } + + T[] typeMethodInfos = getMemberInfos(subType); + + IEnumerable newMethodInfos = typeMethodInfos + .Where(x => !memberInfos.Contains(x)); + + memberInfos.InsertRange(0, newMethodInfos); + } + + return memberInfos.ToArray(); + } } diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs index 76f5b62265..14e80952f4 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/SliderValueConverter.cs @@ -1,4 +1,4 @@ -using System.Collections.Concurrent; +using System.Globalization; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Services; @@ -6,74 +6,123 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters; +/// +/// The slider property value converter. +/// +/// [DefaultPropertyValueConverter] public class SliderValueConverter : PropertyValueConverterBase { - private static readonly ConcurrentDictionary Storages = new(); - private readonly IDataTypeService _dataTypeService; + /// + /// Initializes a new instance of the class. + /// + public SliderValueConverter() + { } - public SliderValueConverter(IDataTypeService dataTypeService) => _dataTypeService = - dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService)); + /// + /// Initializes a new instance of the class. + /// + /// The data type service. + [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")] + public SliderValueConverter(IDataTypeService dataTypeService) + { } - public static void ClearCaches() => Storages.Clear(); + /// + /// Clears the data type configuration caches. + /// + [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")] + public static void ClearCaches() + { } + /// public override bool IsConverter(IPublishedPropertyType propertyType) => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Slider); + /// public override Type GetPropertyValueType(IPublishedPropertyType propertyType) - => IsRangeDataType(propertyType.DataType.Id) ? typeof(Range) : typeof(decimal); + => IsRange(propertyType) ? typeof(Range) : typeof(decimal); + /// public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Element; + /// public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview) { - if (source == null) + bool isRange = IsRange(propertyType); + + var sourceString = source?.ToString(); + + return isRange + ? HandleRange(sourceString) + : HandleDecimal(sourceString); + } + + private static Range HandleRange(string? sourceString) + { + if (sourceString is null) { - return null; + return new Range(); } - if (IsRangeDataType(propertyType.DataType.Id)) - { - var rangeRawValues = source.ToString()!.Split(Constants.CharArrays.Comma); - Attempt minimumAttempt = rangeRawValues[0].TryConvertTo(); - Attempt maximumAttempt = rangeRawValues[1].TryConvertTo(); + string[] rangeRawValues = sourceString.Split(Constants.CharArrays.Comma); - if (minimumAttempt.Success && maximumAttempt.Success) + if (TryParseDecimal(rangeRawValues[0], out var minimum)) + { + if (rangeRawValues.Length == 1) { - return new Range { Maximum = maximumAttempt.Result, Minimum = minimumAttempt.Result }; + // Configuration is probably changed from single to range, return range with same min/max + return new Range + { + Minimum = minimum, + Maximum = minimum + }; + } + + if (rangeRawValues.Length == 2 && TryParseDecimal(rangeRawValues[1], out var maximum)) + { + return new Range + { + Minimum = minimum, + Maximum = maximum + }; } } - Attempt valueAttempt = source.ToString().TryConvertTo(); - if (valueAttempt.Success) + return new Range(); + } + + private static decimal HandleDecimal(string? sourceString) + { + if (string.IsNullOrEmpty(sourceString)) { - return valueAttempt.Result; + return default; } - // Something failed in the conversion of the strings to decimals - return null; + // This used to be a range slider, so we'll assign the minimum value as the new value + if (sourceString.Contains(',')) + { + var minimumValueRepresentation = sourceString.Split(Constants.CharArrays.Comma)[0]; + + if (TryParseDecimal(minimumValueRepresentation, out var minimum)) + { + return minimum; + } + } + else if (TryParseDecimal(sourceString, out var value)) + { + return value; + } + + return default; } /// - /// Discovers if the slider is set to range mode. + /// Helper method for parsing a double consistently /// - /// - /// The data type id. - /// - /// - /// The . - /// - private bool IsRangeDataType(int dataTypeId) => + private static bool TryParseDecimal(string? representation, out decimal value) + => decimal.TryParse(representation, NumberStyles.Number, CultureInfo.InvariantCulture, out value); - // GetPreValuesCollectionByDataTypeId is cached at repository level; - // still, the collection is deep-cloned so this is kinda expensive, - // better to cache here + trigger refresh in DataTypeCacheRefresher - // TODO: this is cheap now, remove the caching - Storages.GetOrAdd(dataTypeId, id => - { - IDataType? dataType = _dataTypeService.GetDataType(id); - SliderConfiguration? configuration = dataType?.ConfigurationAs(); - return configuration?.EnableRange ?? false; - }); + private static bool IsRange(IPublishedPropertyType propertyType) + => propertyType.DataType.ConfigurationAs()?.EnableRange == true; } diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs index 3afc5a6596..2dd1c1d56e 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/TagsValueConverter.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Serialization; @@ -7,69 +6,66 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters; +/// +/// The tags property value converter. +/// +/// [DefaultPropertyValueConverter] public class TagsValueConverter : PropertyValueConverterBase { - private static readonly ConcurrentDictionary Storages = new(); - private readonly IDataTypeService _dataTypeService; private readonly IJsonSerializer _jsonSerializer; + /// + /// Initializes a new instance of the class. + /// + /// The JSON serializer. + /// jsonSerializer + public TagsValueConverter(IJsonSerializer jsonSerializer) + => _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer)); + + /// + /// Initializes a new instance of the class. + /// + /// The data type service. + /// The JSON serializer. + [Obsolete("The IDataTypeService is not used anymore. This constructor will be removed in a future version.")] public TagsValueConverter(IDataTypeService dataTypeService, IJsonSerializer jsonSerializer) - { - _dataTypeService = dataTypeService ?? throw new ArgumentNullException(nameof(dataTypeService)); - _jsonSerializer = jsonSerializer ?? throw new ArgumentNullException(nameof(jsonSerializer)); - } + : this(jsonSerializer) + { } - public static void ClearCaches() => Storages.Clear(); + /// + /// Clears the data type configuration caches. + /// + [Obsolete("Caching of data type configuration is not done anymore. This method will be removed in a future version.")] + public static void ClearCaches() + { } + /// public override bool IsConverter(IPublishedPropertyType propertyType) => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.Tags); + /// public override Type GetPropertyValueType(IPublishedPropertyType propertyType) => typeof(IEnumerable); + /// public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Element; + /// public override object? ConvertSourceToIntermediate(IPublishedElement owner, IPublishedPropertyType propertyType, object? source, bool preview) { - if (source == null) + string? sourceString = source?.ToString(); + if (string.IsNullOrEmpty(sourceString)) { return Array.Empty(); } - // if Json storage type deserialize and return as string array - if (JsonStorageType(propertyType.DataType.Id)) - { - var array = source.ToString() is not null - ? _jsonSerializer.Deserialize(source.ToString()!) - : null; - return array ?? Array.Empty(); - } - - // Otherwise assume CSV storage type and return as string array - return source.ToString()?.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries); + return IsJson(propertyType) + ? _jsonSerializer.Deserialize(sourceString) ?? Array.Empty() + : sourceString.Split(Constants.CharArrays.Comma, StringSplitOptions.RemoveEmptyEntries); } - public override object? ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object? source, bool preview) => (string[]?)source; - - /// - /// Discovers if the tags data type is storing its data in a Json format - /// - /// - /// The data type id. - /// - /// - /// The . - /// - private bool JsonStorageType(int dataTypeId) => - - // GetDataType(id) is cached at repository level; still, there is some - // deep-cloning involved (expensive) - better cache here + trigger - // refresh in DataTypeCacheRefresher - Storages.GetOrAdd(dataTypeId, id => - { - TagConfiguration? configuration = _dataTypeService.GetDataType(id)?.ConfigurationAs(); - return configuration?.StorageType == TagsStorageType.Json; - }); + private static bool IsJson(IPublishedPropertyType propertyType) + => propertyType.DataType.ConfigurationAs()?.StorageType == TagsStorageType.Json; } diff --git a/src/Umbraco.Core/Security/FileStreamSecurityValidator.cs b/src/Umbraco.Core/Security/FileStreamSecurityValidator.cs new file mode 100644 index 0000000000..764ea37d3d --- /dev/null +++ b/src/Umbraco.Core/Security/FileStreamSecurityValidator.cs @@ -0,0 +1,38 @@ +namespace Umbraco.Cms.Core.Security; + +public class FileStreamSecurityValidator : IFileStreamSecurityValidator +{ + private readonly IEnumerable _fileAnalyzers; + + public FileStreamSecurityValidator(IEnumerable fileAnalyzers) + { + _fileAnalyzers = fileAnalyzers; + } + + /// + /// Analyzes whether the file content is considered safe with registered IFileStreamSecurityAnalyzers + /// + /// Needs to be a Read seekable stream + /// Whether the file is considered safe after running the necessary analyzers + public bool IsConsideredSafe(Stream fileStream) + { + foreach (var fileAnalyzer in _fileAnalyzers) + { + fileStream.Seek(0, SeekOrigin.Begin); + if (!fileAnalyzer.ShouldHandle(fileStream)) + { + continue; + } + + fileStream.Seek(0, SeekOrigin.Begin); + if (fileAnalyzer.IsConsideredSafe(fileStream) == false) + { + return false; + } + } + fileStream.Seek(0, SeekOrigin.Begin); + // If no analyzer we consider the file to be safe as the implementer has the possibility to add additional analyzers + // Or all analyzers deem te file to be safe + return true; + } +} diff --git a/src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs b/src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs new file mode 100644 index 0000000000..408f161f21 --- /dev/null +++ b/src/Umbraco.Core/Security/IFileStreamSecurityAnalyzer.cs @@ -0,0 +1,20 @@ +namespace Umbraco.Cms.Core.Security; + +public interface IFileStreamSecurityAnalyzer +{ + + /// + /// Indicates whether the analyzer should process the file + /// The implementation should be considerably faster than IsConsideredSafe + /// + /// + /// + bool ShouldHandle(Stream fileStream); + + /// + /// Analyzes whether the file content is considered safe + /// + /// Needs to be a Read/Write seekable stream + /// Whether the file is considered safe + bool IsConsideredSafe(Stream fileStream); +} diff --git a/src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs b/src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs new file mode 100644 index 0000000000..9dc60507ac --- /dev/null +++ b/src/Umbraco.Core/Security/IFileStreamSecurityValidator.cs @@ -0,0 +1,11 @@ +namespace Umbraco.Cms.Core.Security; + +public interface IFileStreamSecurityValidator +{ + /// + /// Analyzes wether the file content is considered safe with registered IFileStreamSecurityAnalyzers + /// + /// Needs to be a Read seekable stream + /// Whether the file is considered safe after running the necessary analyzers + bool IsConsideredSafe(Stream fileStream); +} diff --git a/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs b/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs index 3c4546da04..44d9cc6790 100644 --- a/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs +++ b/src/Umbraco.Infrastructure/Persistence/Factories/UserGroupFactory.cs @@ -78,7 +78,7 @@ internal static class UserGroupFactory if (entity.HasIdentity) { - dto.Id = short.Parse(entity.Id.ToString()); + dto.Id = entity.Id; } return dto; diff --git a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs index 5a14a1afc1..34a4d33fd9 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs @@ -5,6 +5,7 @@ using Microsoft.Extensions.Options; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.IO; using Umbraco.Cms.Core.Models.Editors; +using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Strings; @@ -18,6 +19,7 @@ namespace Umbraco.Cms.Core.PropertyEditors; internal class FileUploadPropertyValueEditor : DataValueEditor { private readonly MediaFileManager _mediaFileManager; + private readonly IFileStreamSecurityValidator _fileStreamSecurityValidator; private ContentSettings _contentSettings; public FileUploadPropertyValueEditor( @@ -27,10 +29,12 @@ internal class FileUploadPropertyValueEditor : DataValueEditor IShortStringHelper shortStringHelper, IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, - IIOHelper ioHelper) + IIOHelper ioHelper, + IFileStreamSecurityValidator fileStreamSecurityValidator) : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute) { _mediaFileManager = mediaFileManager ?? throw new ArgumentNullException(nameof(mediaFileManager)); + _fileStreamSecurityValidator = fileStreamSecurityValidator; _contentSettings = contentSettings.CurrentValue ?? throw new ArgumentNullException(nameof(contentSettings)); contentSettings.OnChange(x => _contentSettings = x); } @@ -147,6 +151,11 @@ internal class FileUploadPropertyValueEditor : DataValueEditor using (FileStream filestream = File.OpenRead(file.TempFilePath)) { + if (_fileStreamSecurityValidator.IsConsideredSafe(filestream) == false) + { + return null; + } + // TODO: Here it would make sense to do the auto-fill properties stuff but the API doesn't allow us to do that right // since we'd need to be able to return values for other properties from these methods _mediaFileManager.FileSystem.AddFile(filepath, filestream, true); // must overwrite! diff --git a/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs index 223e62d5a3..2c44ef57e3 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs @@ -10,6 +10,7 @@ using Umbraco.Cms.Core.IO; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Editors; using Umbraco.Cms.Core.PropertyEditors.ValueConverters; +using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Strings; @@ -24,6 +25,7 @@ namespace Umbraco.Cms.Core.PropertyEditors; internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core vs web? { private readonly IDataTypeService _dataTypeService; + private readonly IFileStreamSecurityValidator _fileStreamSecurityValidator; private readonly ILogger _logger; private readonly MediaFileManager _mediaFileManager; private ContentSettings _contentSettings; @@ -37,13 +39,15 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, IIOHelper ioHelper, - IDataTypeService dataTypeService) + IDataTypeService dataTypeService, + IFileStreamSecurityValidator fileStreamSecurityValidator) : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute) { _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _mediaFileManager = mediaFileSystem ?? throw new ArgumentNullException(nameof(mediaFileSystem)); _contentSettings = contentSettings.CurrentValue; _dataTypeService = dataTypeService; + _fileStreamSecurityValidator = fileStreamSecurityValidator; contentSettings.OnChange(x => _contentSettings = x); } @@ -236,6 +240,11 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v using (FileStream filestream = File.OpenRead(file.TempFilePath)) { + if (_fileStreamSecurityValidator.IsConsideredSafe(filestream) == false) + { + return null; + } + // TODO: Here it would make sense to do the auto-fill properties stuff but the API doesn't allow us to do that right // since we'd need to be able to return values for other properties from these methods _mediaFileManager.FileSystem.AddFile(filepath, filestream, true); // must overwrite! diff --git a/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs b/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs index f9e53a7c20..1b74953a4f 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/CurrentUserController.cs @@ -48,9 +48,43 @@ public class CurrentUserController : UmbracoAuthorizedJsonController private readonly IShortStringHelper _shortStringHelper; private readonly IUmbracoMapper _umbracoMapper; private readonly IUserDataService _userDataService; + private readonly IFileStreamSecurityValidator? _fileStreamSecurityValidator; // make non nullable in v14 private readonly IUserService _userService; [ActivatorUtilitiesConstructor] + public CurrentUserController( + MediaFileManager mediaFileManager, + IOptionsSnapshot contentSettings, + IHostingEnvironment hostingEnvironment, + IImageUrlGenerator imageUrlGenerator, + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + IUserService userService, + IUmbracoMapper umbracoMapper, + IBackOfficeUserManager backOfficeUserManager, + ILocalizedTextService localizedTextService, + AppCaches appCaches, + IShortStringHelper shortStringHelper, + IPasswordChanger passwordChanger, + IUserDataService userDataService, + IFileStreamSecurityValidator fileStreamSecurityValidator) + { + _mediaFileManager = mediaFileManager; + _contentSettings = contentSettings.Value; + _hostingEnvironment = hostingEnvironment; + _imageUrlGenerator = imageUrlGenerator; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _userService = userService; + _umbracoMapper = umbracoMapper; + _backOfficeUserManager = backOfficeUserManager; + _localizedTextService = localizedTextService; + _appCaches = appCaches; + _shortStringHelper = shortStringHelper; + _passwordChanger = passwordChanger; + _userDataService = userDataService; + _fileStreamSecurityValidator = fileStreamSecurityValidator; + } + + [Obsolete("Use constructor overload that has fileStreamSecurityValidator, scheduled for removal in v14")] public CurrentUserController( MediaFileManager mediaFileManager, IOptionsSnapshot contentSettings, @@ -251,6 +285,7 @@ public class CurrentUserController : UmbracoAuthorizedJsonController _contentSettings, _hostingEnvironment, _imageUrlGenerator, + _fileStreamSecurityValidator, _backofficeSecurityAccessor.BackOfficeSecurity?.GetUserId().Result ?? 0); } diff --git a/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs b/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs index 00b924db5d..1230774e92 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/MediaController.cs @@ -5,6 +5,7 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Infrastructure; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Microsoft.Extensions.Primitives; @@ -51,6 +52,7 @@ public class MediaController : ContentControllerBase { private static readonly Semaphore _postAddFileSemaphore = new(1, 1); private readonly AppCaches _appCaches; + private readonly IFileStreamSecurityValidator? _fileStreamSecurityValidator; // make non nullable in v14 private readonly IAuthorizationService _authorizationService; private readonly IBackOfficeSecurityAccessor _backofficeSecurityAccessor; private readonly ContentSettings _contentSettings; @@ -67,6 +69,58 @@ public class MediaController : ContentControllerBase private readonly ISqlContext _sqlContext; private readonly IUmbracoMapper _umbracoMapper; + [ActivatorUtilitiesConstructor] + public MediaController( + ICultureDictionary cultureDictionary, + ILoggerFactory loggerFactory, + IShortStringHelper shortStringHelper, + IEventMessagesFactory eventMessages, + ILocalizedTextService localizedTextService, + IOptionsSnapshot contentSettings, + IMediaTypeService mediaTypeService, + IMediaService mediaService, + IEntityService entityService, + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + IUmbracoMapper umbracoMapper, + IDataTypeService dataTypeService, + ISqlContext sqlContext, + IContentTypeBaseServiceProvider contentTypeBaseServiceProvider, + IRelationService relationService, + PropertyEditorCollection propertyEditors, + MediaFileManager mediaFileManager, + MediaUrlGeneratorCollection mediaUrlGenerators, + IHostingEnvironment hostingEnvironment, + IImageUrlGenerator imageUrlGenerator, + IJsonSerializer serializer, + IAuthorizationService authorizationService, + AppCaches appCaches, + IFileStreamSecurityValidator streamSecurityValidator) + : base(cultureDictionary, loggerFactory, shortStringHelper, eventMessages, localizedTextService, serializer) + { + _shortStringHelper = shortStringHelper; + _contentSettings = contentSettings.Value; + _mediaTypeService = mediaTypeService; + _mediaService = mediaService; + _entityService = entityService; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _umbracoMapper = umbracoMapper; + _dataTypeService = dataTypeService; + _localizedTextService = localizedTextService; + _sqlContext = sqlContext; + _contentTypeBaseServiceProvider = contentTypeBaseServiceProvider; + _relationService = relationService; + _propertyEditors = propertyEditors; + _mediaFileManager = mediaFileManager; + _mediaUrlGenerators = mediaUrlGenerators; + _hostingEnvironment = hostingEnvironment; + _logger = loggerFactory.CreateLogger(); + _imageUrlGenerator = imageUrlGenerator; + _authorizationService = authorizationService; + _appCaches = appCaches; + _fileStreamSecurityValidator = streamSecurityValidator; + } + + [Obsolete("Use constructor overload that has fileStreamSecurityValidator, scheduled for removal in v14")] public MediaController( ICultureDictionary cultureDictionary, ILoggerFactory loggerFactory, @@ -733,6 +787,17 @@ public class MediaController : ContentControllerBase continue; } + using var stream = new MemoryStream(); + await formFile.CopyToAsync(stream); + if (_fileStreamSecurityValidator != null && _fileStreamSecurityValidator.IsConsideredSafe(stream) == false) + { + tempFiles.Notifications.Add(new BackOfficeNotification( + _localizedTextService.Localize("speechBubbles", "operationFailedHeader"), + _localizedTextService.Localize("media", "fileSecurityValidationFailure"), + NotificationStyle.Warning)); + continue; + } + if (string.IsNullOrEmpty(mediaTypeAlias)) { mediaTypeAlias = Constants.Conventions.MediaTypes.File; @@ -810,11 +875,10 @@ public class MediaController : ContentControllerBase IMedia createdMediaItem = _mediaService.CreateMedia(mediaItemName, parentId.Value, mediaTypeAlias, _backofficeSecurityAccessor.BackOfficeSecurity?.CurrentUser?.Id ?? -1); - await using (Stream stream = formFile.OpenReadStream()) - { + createdMediaItem.SetValue(_mediaFileManager, _mediaUrlGenerators, _shortStringHelper, _contentTypeBaseServiceProvider, Constants.Conventions.Media.File, fileName, stream); - } + Attempt saveResult = _mediaService.Save(createdMediaItem, _backofficeSecurityAccessor.BackOfficeSecurity?.CurrentUser?.Id ?? -1); diff --git a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index 46e06dec6e..5ba735c98b 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -55,6 +55,7 @@ public class UsersController : BackOfficeNotificationsController private readonly GlobalSettings _globalSettings; private readonly IHostingEnvironment _hostingEnvironment; private readonly IHttpContextAccessor _httpContextAccessor; + private readonly IFileStreamSecurityValidator? _fileStreamSecurityValidator; // make non nullable in v14 private readonly IImageUrlGenerator _imageUrlGenerator; private readonly LinkGenerator _linkGenerator; private readonly ILocalizedTextService _localizedTextService; @@ -72,6 +73,58 @@ public class UsersController : BackOfficeNotificationsController private readonly WebRoutingSettings _webRoutingSettings; [ActivatorUtilitiesConstructor] + public UsersController( + MediaFileManager mediaFileManager, + IOptionsSnapshot contentSettings, + IHostingEnvironment hostingEnvironment, + ISqlContext sqlContext, + IImageUrlGenerator imageUrlGenerator, + IOptionsSnapshot securitySettings, + IEmailSender emailSender, + IBackOfficeSecurityAccessor backofficeSecurityAccessor, + AppCaches appCaches, + IShortStringHelper shortStringHelper, + IUserService userService, + ILocalizedTextService localizedTextService, + IUmbracoMapper umbracoMapper, + IOptionsSnapshot globalSettings, + IBackOfficeUserManager backOfficeUserManager, + ILoggerFactory loggerFactory, + LinkGenerator linkGenerator, + IBackOfficeExternalLoginProviders externalLogins, + UserEditorAuthorizationHelper userEditorAuthorizationHelper, + IPasswordChanger passwordChanger, + IHttpContextAccessor httpContextAccessor, + IOptions webRoutingSettings, + IFileStreamSecurityValidator fileStreamSecurityValidator) + { + _mediaFileManager = mediaFileManager; + _contentSettings = contentSettings.Value; + _hostingEnvironment = hostingEnvironment; + _sqlContext = sqlContext; + _imageUrlGenerator = imageUrlGenerator; + _securitySettings = securitySettings.Value; + _emailSender = emailSender; + _backofficeSecurityAccessor = backofficeSecurityAccessor; + _appCaches = appCaches; + _shortStringHelper = shortStringHelper; + _userService = userService; + _localizedTextService = localizedTextService; + _umbracoMapper = umbracoMapper; + _globalSettings = globalSettings.Value; + _userManager = backOfficeUserManager; + _loggerFactory = loggerFactory; + _linkGenerator = linkGenerator; + _externalLogins = externalLogins; + _userEditorAuthorizationHelper = userEditorAuthorizationHelper; + _passwordChanger = passwordChanger; + _logger = _loggerFactory.CreateLogger(); + _httpContextAccessor = httpContextAccessor; + _fileStreamSecurityValidator = fileStreamSecurityValidator; + _webRoutingSettings = webRoutingSettings.Value; + } + + [Obsolete("Use constructor overload that has fileStreamSecurityValidator, scheduled for removal in v14")] public UsersController( MediaFileManager mediaFileManager, IOptionsSnapshot contentSettings, @@ -139,13 +192,15 @@ public class UsersController : BackOfficeNotificationsController [AppendUserModifiedHeader("id")] [Authorize(Policy = AuthorizationPolicies.AdminUserEditsRequireAdmin)] - public IActionResult PostSetAvatar(int id, IList file) => PostSetAvatarInternal(file, _userService, + public IActionResult PostSetAvatar(int id, IList file) + => PostSetAvatarInternal(file, _userService, _appCaches.RuntimeCache, _mediaFileManager, _shortStringHelper, _contentSettings, _hostingEnvironment, - _imageUrlGenerator, id); + _imageUrlGenerator,_fileStreamSecurityValidator, id); internal static IActionResult PostSetAvatarInternal(IList files, IUserService userService, IAppCache cache, MediaFileManager mediaFileManager, IShortStringHelper shortStringHelper, ContentSettings contentSettings, IHostingEnvironment hostingEnvironment, IImageUrlGenerator imageUrlGenerator, + IFileStreamSecurityValidator? fileStreamSecurityValidator, int id) { if (files is null) @@ -187,9 +242,14 @@ public class UsersController : BackOfficeNotificationsController //generate a path of known data, we don't want this path to be guessable user.Avatar = "UserAvatars/" + (user.Id + safeFileName).GenerateHash() + "." + ext; - using (Stream fs = file.OpenReadStream()) + //todo implement Filestreamsecurity + using (var ms = new MemoryStream()) { - mediaFileManager.FileSystem.AddFile(user.Avatar, fs, true); + file.CopyTo(ms); + if(fileStreamSecurityValidator != null && fileStreamSecurityValidator.IsConsideredSafe(ms) == false) + return new ValidationErrorResult("One or more file security analyzers deemed the contents of the file to be unsafe"); + + mediaFileManager.FileSystem.AddFile(user.Avatar, ms, true); } userService.Save(user); diff --git a/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs b/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs index ef59126c1c..47ad9e4e80 100644 --- a/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs +++ b/src/Umbraco.Web.BackOffice/ModelsBuilder/ContentTypeModelValidatorBase.cs @@ -68,10 +68,10 @@ public abstract class ContentTypeModelValidatorBase : EditorV private ValidationResult? ValidateProperty(PropertyTypeBasic property, int groupIndex, int propertyIndex) { - // don't let them match any properties or methods in IPublishedContent + // don't let them match any properties or methods in IPublishedContent (including those defined in any base interfaces like IPublishedElement) // TODO: There are probably more! - var reservedProperties = typeof(IPublishedContent).GetProperties().Select(x => x.Name).ToArray(); - var reservedMethods = typeof(IPublishedContent).GetMethods().Select(x => x.Name).ToArray(); + var reservedProperties = typeof(IPublishedContent).GetPublicProperties().Select(x => x.Name).ToArray(); + var reservedMethods = typeof(IPublishedContent).GetPublicMethods().Select(x => x.Name).ToArray(); var alias = property.Alias; diff --git a/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js b/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js index 20661d5d1c..606d16ad2d 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/blockeditormodelobject.service.js @@ -644,6 +644,12 @@ blockObject.index = 0; if (blockObject.config.label && blockObject.config.label !== "" && blockObject.config.unsupported !== true) { + + // If the label does not contain any AngularJS template, then the MutationObserver wont give us any updates. To ensure labels without angular JS template code, we will just set the label directly for ones without '{{': + if(blockObject.config.label.indexOf("{{") === -1) { + blockObject.label = blockObject.config.label; + } + var labelElement = $('
', { text: blockObject.config.label}); var observer = new MutationObserver(function(mutations) { @@ -673,6 +679,7 @@ this.__labelScope = Object.assign(this.__labelScope, labelVars); $compile(labelElement.contents())(this.__labelScope); + }.bind(blockObject) } else { blockObject.__renderLabel = function() {}; diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs new file mode 100644 index 0000000000..7cdee69daa --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/FileStreamSecurityValidatorTests.cs @@ -0,0 +1,123 @@ +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core.Security; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Security; + +public class FileStreamSecurityValidatorTests +{ + [Test] + public void IsConsideredSafe_True_NoAnalyzersPresent() + { + // Arrange + var sut = new FileStreamSecurityValidator(Enumerable.Empty()); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsTrue(validationResult); + } + + [Test] + public void IsConsideredSafe_True_NoAnalyzerMatchesType() + { + // Arrange + var analyzerOne = new Mock(); + analyzerOne.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + var analyzerTwo = new Mock(); + analyzerTwo.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + + var sut = new FileStreamSecurityValidator(new List{analyzerOne.Object,analyzerTwo.Object}); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsTrue(validationResult); + } + + [Test] + public void IsConsideredSafe_True_AllMatchingAnalyzersReturnTrue() + { + // Arrange + var matchingAnalyzerOne = new Mock(); + matchingAnalyzerOne.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + matchingAnalyzerOne.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(true); + + var matchingAnalyzerTwo = new Mock(); + matchingAnalyzerTwo.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + matchingAnalyzerTwo.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(true); + + var unmatchedAnalyzer = new Mock(); + unmatchedAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + + var sut = new FileStreamSecurityValidator(new List{matchingAnalyzerOne.Object,matchingAnalyzerTwo.Object}); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsTrue(validationResult); + } + + [Test] + public void IsConsideredSafe_False_AnyMatchingAnalyzersReturnFalse() + { + // Arrange + var saveMatchingAnalyzer = new Mock(); + saveMatchingAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + saveMatchingAnalyzer.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(true); + + var unsafeMatchingAnalyzer = new Mock(); + unsafeMatchingAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(true); + unsafeMatchingAnalyzer.Setup(analyzer => analyzer.IsConsideredSafe(It.IsAny())) + .Returns(false); + + var unmatchedAnalyzer = new Mock(); + unmatchedAnalyzer.Setup(analyzer => analyzer.ShouldHandle(It.IsAny())) + .Returns(false); + + var sut = new FileStreamSecurityValidator(new List{saveMatchingAnalyzer.Object,unsafeMatchingAnalyzer.Object}); + + using var memoryStream = new MemoryStream(); + using var streamWriter = new StreamWriter(memoryStream); + streamWriter.Write("TestContent"); + streamWriter.Flush(); + memoryStream.Seek(0, SeekOrigin.Begin); + + // Act + var validationResult = sut.IsConsideredSafe(memoryStream); + + // Assert + Assert.IsFalse(validationResult); + } +}