From 18ee016ae95b19cd0ff0d52fa0f3d989204b9ed2 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Tue, 8 Aug 2023 14:49:18 +0200 Subject: [PATCH 1/4] Implemented modular architecture for filestream security sanitization with an svg-html example --- .../DependencyInjection/UmbracoBuilder.cs | 4 ++ ...eStreamSecuritySanitizationOrchestrator.cs | 46 +++++++++++++++++++ ...eStreamSecuritySanitizationOrchestrator.cs | 13 ++++++ .../Security/IFileStreamSecuritySanitizer.cs | 16 +++++++ .../SvgFileStreamSecuritySanitizer.cs | 37 +++++++++++++++ .../FileUploadPropertyValueEditor.cs | 10 +++- .../ImageCropperPropertyValueEditor.cs | 10 +++- .../Controllers/UsersController.cs | 2 + 8 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs create mode 100644 src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs create mode 100644 src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs create mode 100644 src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs index a850a8f371..5790f44521 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -327,6 +327,10 @@ namespace Umbraco.Cms.Core.DependencyInjection Services.AddUnique(provider => new CultureImpactFactory(provider.GetRequiredService>())); Services.AddUnique(); Services.AddUnique(); + + // Register filestream security sanitizers + Services.AddUnique(); + Services.AddSingleton(); } } } diff --git a/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs b/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs new file mode 100644 index 0000000000..7cf864d9f1 --- /dev/null +++ b/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs @@ -0,0 +1,46 @@ +namespace Umbraco.Cms.Core.Security; + +public class FileStreamSecuritySanitizationOrchestrator : IFileStreamSecuritySanitizationOrchestrator +{ + private readonly IEnumerable _fileSanitizers; + + public FileStreamSecuritySanitizationOrchestrator(IEnumerable fileSanitizers) + { + _fileSanitizers = fileSanitizers; + } + + /// + /// Reads the file content and executes sanitization routines based on the determined file type + /// + /// Needs to be a Read/Write seekable stream + /// Clear fileContent if sanitization fails? + /// If true, this will make the method return true if sanitization fails and file was cleared + /// Whether the file is considered clean after running the necessary sanitizers + public bool Sanitize(FileStream fileStream, bool clearContentIfSanitizationFails) + { + var startBuffer = new byte[_fileSanitizers.Max(fs => fs.MinimumStartBytesRequiredForContentTypeMatching)]; + var endBuffer = new byte[_fileSanitizers.Max(fs => fs.MinimumEndBytesRequiredForContentTypeMatching)]; + fileStream.Read(startBuffer); + fileStream.Seek(endBuffer.Length * -1, SeekOrigin.End); + fileStream.Read(endBuffer); + fileStream.Seek(0, SeekOrigin.Begin); + + foreach (var fileSanitizer in _fileSanitizers) + { + if (!fileSanitizer.FileContentMatchesFileType(startBuffer, endBuffer)) + { + continue; + } + + var sanitationResult = fileSanitizer.RemoveSensitiveContent(fileStream); + if (clearContentIfSanitizationFails == false) + return sanitationResult; + + fileStream.SetLength(0); + return true; + } + + // No sanitizer found, we consider the file to be safe as the implementer has the possibility to add additional sanitizers + return true; + } +} diff --git a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs b/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs new file mode 100644 index 0000000000..7dbfaefd0f --- /dev/null +++ b/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs @@ -0,0 +1,13 @@ +namespace Umbraco.Cms.Core.Security; + +public interface IFileStreamSecuritySanitizationOrchestrator +{ + /// + /// Reads the file content and executes sanitization routines based on the determined file type + /// + /// Needs to be a Read/Write seekable stream + /// Clear fileContent if sanitization fails? + /// If true, this will make the method return true if sanitization fails and file was cleared + /// Whether the file is considered clean after running the necessary sanitizers + bool Sanitize(FileStream fileStream, bool clearContentIfSanitizationFails); +} diff --git a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs b/src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs new file mode 100644 index 0000000000..33e70e7d74 --- /dev/null +++ b/src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs @@ -0,0 +1,16 @@ +namespace Umbraco.Cms.Core.Security; + +public interface IFileStreamSecuritySanitizer +{ + public int MinimumStartBytesRequiredForContentTypeMatching { get; } + public int MinimumEndBytesRequiredForContentTypeMatching{ get; } + + bool FileContentMatchesFileType(byte[] startBytes, byte[] endBytes); + + /// + /// Sanitizes the filestream so it doesn't contain security sensitive content + /// + /// Needs to be a read/Write seekable stream + /// Whether the fileStream is considered clean after the method was run + bool RemoveSensitiveContent(FileStream fileStream); +} diff --git a/src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs b/src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs new file mode 100644 index 0000000000..3c33648d81 --- /dev/null +++ b/src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs @@ -0,0 +1,37 @@ +namespace Umbraco.Cms.Core.Security; + +public class SvgFileStreamSecuritySanitizer : IFileStreamSecuritySanitizer +{ + private readonly IHtmlSanitizer _htmlSanitizer; + public int MinimumStartBytesRequiredForContentTypeMatching => 256; + public int MinimumEndBytesRequiredForContentTypeMatching => 256; + + public SvgFileStreamSecuritySanitizer(IHtmlSanitizer htmlSanitizer) + { + _htmlSanitizer = htmlSanitizer; + } + + public bool FileContentMatchesFileType(byte[] startBytes, byte[] endBytes) + { + var startString = System.Text.Encoding.UTF8.GetString(startBytes, 0, startBytes.Length); + return startString.Contains(" + /// Sanitizes the svg filestream (Remove html and nested Javascript) + /// + /// Needs to be a read/Write seekable stream + /// Whether the fileStream is considered clean after the method was run + public bool RemoveSensitiveContent(FileStream fileStream) + { + //todo optimize streams?, make async? + using var streamReader = new StreamReader(fileStream); + var fileContent = streamReader.ReadToEnd(); + _htmlSanitizer.Sanitize(fileContent); + var outBytes = streamReader.CurrentEncoding.GetBytes(fileContent); + fileStream.Seek(0, SeekOrigin.Begin); + fileStream.SetLength(outBytes.Length); + fileStream.Write(outBytes); + return true; + } +} diff --git a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs index 5a14a1afc1..a5fd27a415 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 IFileStreamSecuritySanitizationOrchestrator _fileStreamSanitizer; private ContentSettings _contentSettings; public FileUploadPropertyValueEditor( @@ -27,10 +29,12 @@ internal class FileUploadPropertyValueEditor : DataValueEditor IShortStringHelper shortStringHelper, IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, - IIOHelper ioHelper) + IIOHelper ioHelper, + IFileStreamSecuritySanitizationOrchestrator fileStreamSanitizer) : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute) { _mediaFileManager = mediaFileManager ?? throw new ArgumentNullException(nameof(mediaFileManager)); + _fileStreamSanitizer = fileStreamSanitizer; _contentSettings = contentSettings.CurrentValue ?? throw new ArgumentNullException(nameof(contentSettings)); contentSettings.OnChange(x => _contentSettings = x); } @@ -147,6 +151,10 @@ internal class FileUploadPropertyValueEditor : DataValueEditor using (FileStream filestream = File.OpenRead(file.TempFilePath)) { + //todo? are we allowed to return null here? + if (_fileStreamSanitizer.Sanitize(filestream, false) == 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..44164f4262 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 IFileStreamSecuritySanitizationOrchestrator _fileStreamSanitizer; 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, + IFileStreamSecuritySanitizationOrchestrator fileStreamSanitizer) : 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; + _fileStreamSanitizer = fileStreamSanitizer; contentSettings.OnChange(x => _contentSettings = x); } @@ -236,6 +240,10 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v using (FileStream filestream = File.OpenRead(file.TempFilePath)) { + //??? are we allowed to return null here? + if (_fileStreamSanitizer.Sanitize(filestream, false) == 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/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index 1a8be10a5c..dd7dedd1b4 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -187,6 +187,8 @@ 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; + //todo implement Filestreamsecurity + using (Stream fs = file.OpenReadStream()) { mediaFileManager.FileSystem.AddFile(user.Avatar, fs, true); From 2b3f048db7cf6cbb9f3d921d541e9fd8a3960f70 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Tue, 8 Aug 2023 16:26:59 +0200 Subject: [PATCH 2/4] Revert "Implemented modular architecture for filestream security sanitization with an svg-html example" This reverts commit 18ee016ae95b19cd0ff0d52fa0f3d989204b9ed2. --- .../DependencyInjection/UmbracoBuilder.cs | 4 -- ...eStreamSecuritySanitizationOrchestrator.cs | 46 ------------------- ...eStreamSecuritySanitizationOrchestrator.cs | 13 ------ .../Security/IFileStreamSecuritySanitizer.cs | 16 ------- .../SvgFileStreamSecuritySanitizer.cs | 37 --------------- .../FileUploadPropertyValueEditor.cs | 10 +--- .../ImageCropperPropertyValueEditor.cs | 10 +--- .../Controllers/UsersController.cs | 2 - 8 files changed, 2 insertions(+), 136 deletions(-) delete mode 100644 src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs delete mode 100644 src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs delete mode 100644 src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs delete mode 100644 src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs index 5790f44521..a850a8f371 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -327,10 +327,6 @@ namespace Umbraco.Cms.Core.DependencyInjection Services.AddUnique(provider => new CultureImpactFactory(provider.GetRequiredService>())); Services.AddUnique(); Services.AddUnique(); - - // Register filestream security sanitizers - Services.AddUnique(); - Services.AddSingleton(); } } } diff --git a/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs b/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs deleted file mode 100644 index 7cf864d9f1..0000000000 --- a/src/Umbraco.Core/Security/FileStreamSecuritySanitizationOrchestrator.cs +++ /dev/null @@ -1,46 +0,0 @@ -namespace Umbraco.Cms.Core.Security; - -public class FileStreamSecuritySanitizationOrchestrator : IFileStreamSecuritySanitizationOrchestrator -{ - private readonly IEnumerable _fileSanitizers; - - public FileStreamSecuritySanitizationOrchestrator(IEnumerable fileSanitizers) - { - _fileSanitizers = fileSanitizers; - } - - /// - /// Reads the file content and executes sanitization routines based on the determined file type - /// - /// Needs to be a Read/Write seekable stream - /// Clear fileContent if sanitization fails? - /// If true, this will make the method return true if sanitization fails and file was cleared - /// Whether the file is considered clean after running the necessary sanitizers - public bool Sanitize(FileStream fileStream, bool clearContentIfSanitizationFails) - { - var startBuffer = new byte[_fileSanitizers.Max(fs => fs.MinimumStartBytesRequiredForContentTypeMatching)]; - var endBuffer = new byte[_fileSanitizers.Max(fs => fs.MinimumEndBytesRequiredForContentTypeMatching)]; - fileStream.Read(startBuffer); - fileStream.Seek(endBuffer.Length * -1, SeekOrigin.End); - fileStream.Read(endBuffer); - fileStream.Seek(0, SeekOrigin.Begin); - - foreach (var fileSanitizer in _fileSanitizers) - { - if (!fileSanitizer.FileContentMatchesFileType(startBuffer, endBuffer)) - { - continue; - } - - var sanitationResult = fileSanitizer.RemoveSensitiveContent(fileStream); - if (clearContentIfSanitizationFails == false) - return sanitationResult; - - fileStream.SetLength(0); - return true; - } - - // No sanitizer found, we consider the file to be safe as the implementer has the possibility to add additional sanitizers - return true; - } -} diff --git a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs b/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs deleted file mode 100644 index 7dbfaefd0f..0000000000 --- a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizationOrchestrator.cs +++ /dev/null @@ -1,13 +0,0 @@ -namespace Umbraco.Cms.Core.Security; - -public interface IFileStreamSecuritySanitizationOrchestrator -{ - /// - /// Reads the file content and executes sanitization routines based on the determined file type - /// - /// Needs to be a Read/Write seekable stream - /// Clear fileContent if sanitization fails? - /// If true, this will make the method return true if sanitization fails and file was cleared - /// Whether the file is considered clean after running the necessary sanitizers - bool Sanitize(FileStream fileStream, bool clearContentIfSanitizationFails); -} diff --git a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs b/src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs deleted file mode 100644 index 33e70e7d74..0000000000 --- a/src/Umbraco.Core/Security/IFileStreamSecuritySanitizer.cs +++ /dev/null @@ -1,16 +0,0 @@ -namespace Umbraco.Cms.Core.Security; - -public interface IFileStreamSecuritySanitizer -{ - public int MinimumStartBytesRequiredForContentTypeMatching { get; } - public int MinimumEndBytesRequiredForContentTypeMatching{ get; } - - bool FileContentMatchesFileType(byte[] startBytes, byte[] endBytes); - - /// - /// Sanitizes the filestream so it doesn't contain security sensitive content - /// - /// Needs to be a read/Write seekable stream - /// Whether the fileStream is considered clean after the method was run - bool RemoveSensitiveContent(FileStream fileStream); -} diff --git a/src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs b/src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs deleted file mode 100644 index 3c33648d81..0000000000 --- a/src/Umbraco.Core/Security/SvgFileStreamSecuritySanitizer.cs +++ /dev/null @@ -1,37 +0,0 @@ -namespace Umbraco.Cms.Core.Security; - -public class SvgFileStreamSecuritySanitizer : IFileStreamSecuritySanitizer -{ - private readonly IHtmlSanitizer _htmlSanitizer; - public int MinimumStartBytesRequiredForContentTypeMatching => 256; - public int MinimumEndBytesRequiredForContentTypeMatching => 256; - - public SvgFileStreamSecuritySanitizer(IHtmlSanitizer htmlSanitizer) - { - _htmlSanitizer = htmlSanitizer; - } - - public bool FileContentMatchesFileType(byte[] startBytes, byte[] endBytes) - { - var startString = System.Text.Encoding.UTF8.GetString(startBytes, 0, startBytes.Length); - return startString.Contains(" - /// Sanitizes the svg filestream (Remove html and nested Javascript) - /// - /// Needs to be a read/Write seekable stream - /// Whether the fileStream is considered clean after the method was run - public bool RemoveSensitiveContent(FileStream fileStream) - { - //todo optimize streams?, make async? - using var streamReader = new StreamReader(fileStream); - var fileContent = streamReader.ReadToEnd(); - _htmlSanitizer.Sanitize(fileContent); - var outBytes = streamReader.CurrentEncoding.GetBytes(fileContent); - fileStream.Seek(0, SeekOrigin.Begin); - fileStream.SetLength(outBytes.Length); - fileStream.Write(outBytes); - return true; - } -} diff --git a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs index a5fd27a415..5a14a1afc1 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs @@ -5,7 +5,6 @@ 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; @@ -19,7 +18,6 @@ namespace Umbraco.Cms.Core.PropertyEditors; internal class FileUploadPropertyValueEditor : DataValueEditor { private readonly MediaFileManager _mediaFileManager; - private readonly IFileStreamSecuritySanitizationOrchestrator _fileStreamSanitizer; private ContentSettings _contentSettings; public FileUploadPropertyValueEditor( @@ -29,12 +27,10 @@ internal class FileUploadPropertyValueEditor : DataValueEditor IShortStringHelper shortStringHelper, IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, - IIOHelper ioHelper, - IFileStreamSecuritySanitizationOrchestrator fileStreamSanitizer) + IIOHelper ioHelper) : base(localizedTextService, shortStringHelper, jsonSerializer, ioHelper, attribute) { _mediaFileManager = mediaFileManager ?? throw new ArgumentNullException(nameof(mediaFileManager)); - _fileStreamSanitizer = fileStreamSanitizer; _contentSettings = contentSettings.CurrentValue ?? throw new ArgumentNullException(nameof(contentSettings)); contentSettings.OnChange(x => _contentSettings = x); } @@ -151,10 +147,6 @@ internal class FileUploadPropertyValueEditor : DataValueEditor using (FileStream filestream = File.OpenRead(file.TempFilePath)) { - //todo? are we allowed to return null here? - if (_fileStreamSanitizer.Sanitize(filestream, false) == 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 44164f4262..223e62d5a3 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs @@ -10,7 +10,6 @@ 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; @@ -25,7 +24,6 @@ namespace Umbraco.Cms.Core.PropertyEditors; internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core vs web? { private readonly IDataTypeService _dataTypeService; - private readonly IFileStreamSecuritySanitizationOrchestrator _fileStreamSanitizer; private readonly ILogger _logger; private readonly MediaFileManager _mediaFileManager; private ContentSettings _contentSettings; @@ -39,15 +37,13 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v IOptionsMonitor contentSettings, IJsonSerializer jsonSerializer, IIOHelper ioHelper, - IDataTypeService dataTypeService, - IFileStreamSecuritySanitizationOrchestrator fileStreamSanitizer) + IDataTypeService dataTypeService) : 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; - _fileStreamSanitizer = fileStreamSanitizer; contentSettings.OnChange(x => _contentSettings = x); } @@ -240,10 +236,6 @@ internal class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core v using (FileStream filestream = File.OpenRead(file.TempFilePath)) { - //??? are we allowed to return null here? - if (_fileStreamSanitizer.Sanitize(filestream, false) == 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/UsersController.cs b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs index dd7dedd1b4..1a8be10a5c 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/UsersController.cs @@ -187,8 +187,6 @@ 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; - //todo implement Filestreamsecurity - using (Stream fs = file.OpenReadStream()) { mediaFileManager.FileSystem.AddFile(user.Avatar, fs, true); From 264e4f8b579c7954ec242fa85caa110ff5725b8d Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Wed, 9 Aug 2023 11:47:45 +0200 Subject: [PATCH 3/4] Add config so it is configurable whether to explicitly index each nested property (#14648) * Added new configuration "Umbraco:CMS:Examine:ExplicitlyIndexEachNestedProperty" that can be used to avoid nested properties like block grid to index each property individually. * Moved the setting "Umbraco:CMS:Examine:ExplicitlyIndexEachNestedProperty" to "Umbraco:CMS:Indexing:ExplicitlyIndexEachNestedProperty" to make it more future proof * Added missing registration * Fixed registration * Small readability improvement --------- Co-authored-by: Sven Geusens --- src/JsonSchema/AppSettings.cs | 1 + .../Configuration/Models/IndexingSettings.cs | 18 +++++++++++ src/Umbraco.Core/Constants-Configuration.cs | 1 + .../UmbracoBuilder.Configuration.cs | 3 +- .../JsonPropertyIndexValueFactoryBase.cs | 32 ++++++++++++++++--- .../TagPropertyIndexValueFactory.cs | 17 +++++++++- .../BlockValuePropertyIndexValueFactory.cs | 16 +++++++++- .../NestedContentPropertyIndexValueFactory.cs | 19 +++++++++-- .../NestedPropertyIndexValueFactoryBase.cs | 19 +++++++++-- 9 files changed, 115 insertions(+), 11 deletions(-) create mode 100644 src/Umbraco.Core/Configuration/Models/IndexingSettings.cs diff --git a/src/JsonSchema/AppSettings.cs b/src/JsonSchema/AppSettings.cs index b491ed2052..4abca7633e 100644 --- a/src/JsonSchema/AppSettings.cs +++ b/src/JsonSchema/AppSettings.cs @@ -51,6 +51,7 @@ namespace JsonSchema public ImagingSettings? Imaging { get; set; } public IndexCreatorSettings? Examine { get; set; } + public IndexingSettings? Indexing { get; set; } public KeepAliveSettings? KeepAlive { get; set; } diff --git a/src/Umbraco.Core/Configuration/Models/IndexingSettings.cs b/src/Umbraco.Core/Configuration/Models/IndexingSettings.cs new file mode 100644 index 0000000000..2a94a406d1 --- /dev/null +++ b/src/Umbraco.Core/Configuration/Models/IndexingSettings.cs @@ -0,0 +1,18 @@ +using System.ComponentModel; + +namespace Umbraco.Cms.Core.Configuration.Models; + +/// +/// Typed configuration options for index creator settings. +/// +[UmbracoOptions(Constants.Configuration.ConfigIndexing)] +public class IndexingSettings +{ + private const bool StaticExplicitlyIndexEachNestedProperty = true; + + /// + /// Gets or sets a value for whether each nested property should have it's own indexed value. Requires a rebuild of indexes when changed. + /// + [DefaultValue(StaticExplicitlyIndexEachNestedProperty)] + public bool ExplicitlyIndexEachNestedProperty { get; set; } = StaticExplicitlyIndexEachNestedProperty; +} diff --git a/src/Umbraco.Core/Constants-Configuration.cs b/src/Umbraco.Core/Constants-Configuration.cs index fea3ceecbf..a7cd0156ac 100644 --- a/src/Umbraco.Core/Constants-Configuration.cs +++ b/src/Umbraco.Core/Constants-Configuration.cs @@ -38,6 +38,7 @@ public static partial class Constants public const string ConfigHosting = ConfigPrefix + "Hosting"; public const string ConfigImaging = ConfigPrefix + "Imaging"; public const string ConfigExamine = ConfigPrefix + "Examine"; + public const string ConfigIndexing = ConfigPrefix + "Indexing"; public const string ConfigKeepAlive = ConfigPrefix + "KeepAlive"; public const string ConfigLogging = ConfigPrefix + "Logging"; public const string ConfigMemberPassword = ConfigPrefix + "Security:MemberPassword"; diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.Configuration.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.Configuration.cs index da49cf68fc..4caf854d60 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.Configuration.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.Configuration.cs @@ -50,6 +50,7 @@ public static partial class UmbracoBuilderExtensions builder .AddUmbracoOptions() .AddUmbracoOptions() + .AddUmbracoOptions() .AddUmbracoOptions() .AddUmbracoOptions() .AddUmbracoOptions() @@ -64,7 +65,7 @@ public static partial class UmbracoBuilderExtensions .AddUmbracoOptions() .AddUmbracoOptions() .AddUmbracoOptions() - .AddUmbracoOptions() + .AddUmbracoOptions() .AddUmbracoOptions() .AddUmbracoOptions() .AddUmbracoOptions() diff --git a/src/Umbraco.Core/PropertyEditors/JsonPropertyIndexValueFactoryBase.cs b/src/Umbraco.Core/PropertyEditors/JsonPropertyIndexValueFactoryBase.cs index a56bf0ed88..bf549e2d2e 100644 --- a/src/Umbraco.Core/PropertyEditors/JsonPropertyIndexValueFactoryBase.cs +++ b/src/Umbraco.Core/PropertyEditors/JsonPropertyIndexValueFactoryBase.cs @@ -1,5 +1,9 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Serialization; +using Umbraco.Cms.Web.Common.DependencyInjection; using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors; @@ -11,13 +15,28 @@ namespace Umbraco.Cms.Core.PropertyEditors; public abstract class JsonPropertyIndexValueFactoryBase : IPropertyIndexValueFactory { private readonly IJsonSerializer _jsonSerializer; + private IndexingSettings _indexingSettings; + + protected bool ForceExplicitlyIndexEachNestedProperty { get; set; } /// /// Constructor for the JsonPropertyIndexValueFactoryBase. /// - protected JsonPropertyIndexValueFactoryBase(IJsonSerializer jsonSerializer) + protected JsonPropertyIndexValueFactoryBase(IJsonSerializer jsonSerializer, IOptionsMonitor indexingSettings) { _jsonSerializer = jsonSerializer; + _indexingSettings = indexingSettings.CurrentValue; + indexingSettings.OnChange(newValue => _indexingSettings = newValue); + } + + + /// + /// Constructor for the JsonPropertyIndexValueFactoryBase. + /// + [Obsolete("Use non-obsolete constructor. This will be removed in Umbraco 14.")] + protected JsonPropertyIndexValueFactoryBase(IJsonSerializer jsonSerializer): this(jsonSerializer, StaticServiceProvider.Instance.GetRequiredService>()) + { + } /// @@ -58,9 +77,14 @@ public abstract class JsonPropertyIndexValueFactoryBase : IProperty } } - result.AddRange(HandleResume(result, property, culture, segment, published)); + IEnumerable>> summary = HandleResume(result, property, culture, segment, published); + if (_indexingSettings.ExplicitlyIndexEachNestedProperty || ForceExplicitlyIndexEachNestedProperty) + { + result.AddRange(summary); + return result; + } - return result; + return summary; } [Obsolete("Use method overload that has availableCultures, scheduled for removal in v14")] @@ -68,7 +92,7 @@ public abstract class JsonPropertyIndexValueFactoryBase : IProperty => GetIndexValues(property, culture, segment, published, Enumerable.Empty()); /// - /// Method to return a list of resume of the content. By default this returns an empty list + /// Method to return a list of summary of the content. By default this returns an empty list /// protected virtual IEnumerable>> HandleResume( List>> result, diff --git a/src/Umbraco.Core/PropertyEditors/TagPropertyIndexValueFactory.cs b/src/Umbraco.Core/PropertyEditors/TagPropertyIndexValueFactory.cs index b3a8e9a2b8..f858b6801a 100644 --- a/src/Umbraco.Core/PropertyEditors/TagPropertyIndexValueFactory.cs +++ b/src/Umbraco.Core/PropertyEditors/TagPropertyIndexValueFactory.cs @@ -1,12 +1,27 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Serialization; +using Umbraco.Cms.Web.Common.DependencyInjection; namespace Umbraco.Cms.Core.PropertyEditors; public class TagPropertyIndexValueFactory : JsonPropertyIndexValueFactoryBase, ITagPropertyIndexValueFactory { - public TagPropertyIndexValueFactory(IJsonSerializer jsonSerializer) : base(jsonSerializer) + public TagPropertyIndexValueFactory( + IJsonSerializer jsonSerializer, + IOptionsMonitor indexingSettings) + : base(jsonSerializer, indexingSettings) { + ForceExplicitlyIndexEachNestedProperty = true; + } + + [Obsolete("Use non-obsolete constructor. This will be removed in Umbraco 14.")] + public TagPropertyIndexValueFactory(IJsonSerializer jsonSerializer) + : this(jsonSerializer, StaticServiceProvider.Instance.GetRequiredService>()) + { + } protected override IEnumerable>> Handle( diff --git a/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyIndexValueFactory.cs b/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyIndexValueFactory.cs index dfedeedc3f..adace6126e 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyIndexValueFactory.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyIndexValueFactory.cs @@ -1,10 +1,14 @@ // Copyright (c) Umbraco. // See LICENSE for more details. +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Blocks; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Web.Common.DependencyInjection; namespace Umbraco.Cms.Core.PropertyEditors; @@ -14,12 +18,22 @@ internal sealed class BlockValuePropertyIndexValueFactory : { private readonly IContentTypeService _contentTypeService; + public BlockValuePropertyIndexValueFactory( + PropertyEditorCollection propertyEditorCollection, + IContentTypeService contentTypeService, + IJsonSerializer jsonSerializer, + IOptionsMonitor indexingSettings) + : base(propertyEditorCollection, jsonSerializer, indexingSettings) + { + _contentTypeService = contentTypeService; + } + [Obsolete("Use non-obsolete constructor. This will be removed in Umbraco 14.")] public BlockValuePropertyIndexValueFactory( PropertyEditorCollection propertyEditorCollection, IContentTypeService contentTypeService, IJsonSerializer jsonSerializer) - : base(propertyEditorCollection, jsonSerializer) + : this(propertyEditorCollection, contentTypeService, jsonSerializer, StaticServiceProvider.Instance.GetRequiredService>()) { _contentTypeService = contentTypeService; } diff --git a/src/Umbraco.Infrastructure/PropertyEditors/NestedContentPropertyIndexValueFactory.cs b/src/Umbraco.Infrastructure/PropertyEditors/NestedContentPropertyIndexValueFactory.cs index 445e1cc361..121a40bec9 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/NestedContentPropertyIndexValueFactory.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/NestedContentPropertyIndexValueFactory.cs @@ -1,9 +1,13 @@ // Copyright (c) Umbraco. // See LICENSE for more details. +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Web.Common.DependencyInjection; namespace Umbraco.Cms.Core.PropertyEditors; @@ -15,11 +19,22 @@ internal sealed class NestedContentPropertyIndexValueFactory { private readonly IContentTypeService _contentTypeService; - public NestedContentPropertyIndexValueFactory( PropertyEditorCollection propertyEditorCollection, IContentTypeService contentTypeService, - IJsonSerializer jsonSerializer) : base(propertyEditorCollection, jsonSerializer) + IJsonSerializer jsonSerializer, + IOptionsMonitor indexingSettings) + : base(propertyEditorCollection, jsonSerializer, indexingSettings) + { + _contentTypeService = contentTypeService; + } + + [Obsolete("Use non-obsolete constructor. This will be removed in Umbraco 14.")] + public NestedContentPropertyIndexValueFactory( + PropertyEditorCollection propertyEditorCollection, + IContentTypeService contentTypeService, + IJsonSerializer jsonSerializer) + : this(propertyEditorCollection, contentTypeService, jsonSerializer, StaticServiceProvider.Instance.GetRequiredService>()) { _contentTypeService = contentTypeService; } diff --git a/src/Umbraco.Infrastructure/PropertyEditors/NestedPropertyIndexValueFactoryBase.cs b/src/Umbraco.Infrastructure/PropertyEditors/NestedPropertyIndexValueFactoryBase.cs index b3799aaa95..94ed0a3e15 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/NestedPropertyIndexValueFactoryBase.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/NestedPropertyIndexValueFactoryBase.cs @@ -1,7 +1,11 @@ using System.Text; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Infrastructure.Examine; +using Umbraco.Cms.Web.Common.DependencyInjection; using Umbraco.Extensions; namespace Umbraco.Cms.Core.PropertyEditors; @@ -10,12 +14,23 @@ internal abstract class NestedPropertyIndexValueFactoryBase { private readonly PropertyEditorCollection _propertyEditorCollection; + + protected NestedPropertyIndexValueFactoryBase( + PropertyEditorCollection propertyEditorCollection, + IJsonSerializer jsonSerializer, + IOptionsMonitor indexingSettings) + : base(jsonSerializer, indexingSettings) + { + _propertyEditorCollection = propertyEditorCollection; + } + + [Obsolete("Use non-obsolete constructor. This will be removed in Umbraco 14.")] protected NestedPropertyIndexValueFactoryBase( PropertyEditorCollection propertyEditorCollection, IJsonSerializer jsonSerializer) - : base(jsonSerializer) + : this(propertyEditorCollection, jsonSerializer, StaticServiceProvider.Instance.GetRequiredService>()) { - _propertyEditorCollection = propertyEditorCollection; + } [Obsolete("Use the overload that specifies availableCultures, scheduled for removal in v14")] From d3243f8700661e2585423b18fed1431f73824d37 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Wed, 9 Aug 2023 13:11:44 +0200 Subject: [PATCH 4/4] Bugfix 31584 - Remove Lazy "caching" that retains keys of deleted ContentTypes which causes unecesary validation errors (#14643) The validation errors should not happen as the model mapping cleans up block data for entity types that do not longer exist (which is checked on the aformentioned key) Co-authored-by: Sven Geusens --- .../PropertyEditors/BlockEditorValues.cs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Infrastructure/PropertyEditors/BlockEditorValues.cs b/src/Umbraco.Infrastructure/PropertyEditors/BlockEditorValues.cs index 8b164557b3..773b9e3a62 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/BlockEditorValues.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/BlockEditorValues.cs @@ -14,14 +14,14 @@ namespace Umbraco.Cms.Core.PropertyEditors; /// internal class BlockEditorValues { - private readonly Lazy> _contentTypes; private readonly BlockEditorDataConverter _dataConverter; + private readonly IContentTypeService _contentTypeService; private readonly ILogger _logger; public BlockEditorValues(BlockEditorDataConverter dataConverter, IContentTypeService contentTypeService, ILogger logger) { - _contentTypes = new Lazy>(() => contentTypeService.GetAll().ToDictionary(c => c.Key)); _dataConverter = dataConverter; + _contentTypeService = contentTypeService; _logger = logger; } @@ -66,11 +66,7 @@ internal class BlockEditorValues return blockEditorData; } - private IContentType? GetElementType(BlockItemData item) - { - _contentTypes.Value.TryGetValue(item.ContentTypeKey, out IContentType? contentType); - return contentType; - } + private IContentType? GetElementType(BlockItemData item) => _contentTypeService.Get(item.ContentTypeKey); private bool ResolveBlockItemData(BlockItemData block, Dictionary> contentTypePropertyTypes) {