diff --git a/Core/src/Umbrella.Utilities.Integration.NewtonsoftJson/Converters/HtmlEncodeStringPropertiesConverter.cs b/Core/src/Umbrella.Utilities.Integration.NewtonsoftJson/Converters/HtmlEncodeStringPropertiesConverter.cs index bdca732f2..0efd9620d 100644 --- a/Core/src/Umbrella.Utilities.Integration.NewtonsoftJson/Converters/HtmlEncodeStringPropertiesConverter.cs +++ b/Core/src/Umbrella.Utilities.Integration.NewtonsoftJson/Converters/HtmlEncodeStringPropertiesConverter.cs @@ -5,6 +5,10 @@ namespace Umbrella.Utilities.Integration.NewtonsoftJson.Converters { + // TODO V3: Rework this so that nothing is encoded on the way in as per Microsoft's advice: https://docs.microsoft.com/en-us/aspnet/core/security/cross-site-scripting + // The general accepted practice is that encoding takes place at the point of output and encoded values should never be stored in a database. + // Encoding at the point of output allows you to change the use of data, for example, from HTML to a query string value. + // It also enables you to easily search your data without having to encode values before searching and allows you to take advantage of any changes or bug fixes made to encoders. /// /// A custom to ensure string values are properly encoded and decoded to mitigate XSS attacks. /// @@ -40,6 +44,7 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist { // Clean the incoming string before running through the sanitizer and then again afterwards // to prevent double encoding of things we don't want encoding. + // TODO: Remove this completely. value = WebUtility.HtmlEncode(value.Clean()).Clean(); } @@ -60,11 +65,16 @@ public override void WriteJson(JsonWriter writer, object value, JsonSerializer s { // Cleaning again on the way out to ensure that anything that is encoded // that shouldn't be is decoded properly. + // TODO: This is wrong. Always send out encoded content and allow the client to decode it if deemed necessary. valueToWrite = valueToWrite.Clean(); } writer.WriteValue(valueToWrite ?? string.Empty); } #endregion + + // TODO: What about providing hooks to allow behaviour to be overridden, for example, for a specific URL? + // What about creating a few different converters, e.g. one that strips out HTML on the way in when we know the user shouldn't + // be posting HTML to the server? } } \ No newline at end of file diff --git a/Core/src/Umbrella.Utilities/Extensions/StringExtensions.cs b/Core/src/Umbrella.Utilities/Extensions/StringExtensions.cs index 981e8f06a..91b324296 100644 --- a/Core/src/Umbrella.Utilities/Extensions/StringExtensions.cs +++ b/Core/src/Umbrella.Utilities/Extensions/StringExtensions.cs @@ -139,18 +139,19 @@ public static string ConvertHtmlBrTagsToLineBreaks(this string value) if (string.IsNullOrEmpty(value)) return value; - StringBuilder sb = new StringBuilder(value); + var sb = new StringBuilder(value); sb.ConvertHtmlBrTagsToReplacement("\n"); return sb.ToString(); } + [Obsolete("This will be removed in the next major version. It's become a bit of a mess over time and has defeated it's original purpose!")] public static string Clean(this string value, bool convertBrTagsToNl = false, bool trim = true, bool trimNewLines = true, bool stripHtml = true, bool stripNbsp = true, bool decodeHtmlEncodedLineBreaks = true) { if (string.IsNullOrWhiteSpace(value)) return null; - StringBuilder sb = new StringBuilder(value); + var sb = new StringBuilder(value); if (convertBrTagsToNl) sb.ConvertHtmlBrTagsToReplacement("\n"); @@ -168,12 +169,13 @@ public static string Clean(this string value, bool convertBrTagsToNl = false, bo .Replace(" ", "\r"); } - //Replace the following in strings + // Replace the following in strings sb.Replace("&", "&"); sb.Replace("'", "'"); sb.Replace(""", "\""); - sb.Replace("‘", "'"); //Left quote - sb.Replace("’", "'"); //Right quote + sb.Replace("‘", "'"); // Left quote + sb.Replace("’", "'"); // Right quote + sb.Replace("£", "£"); if (trimNewLines) {