From f499f78573bf5e4a6fe6c058ed60cdb4d404b22a Mon Sep 17 00:00:00 2001
From: Richard Edwards <richard.edwards@zinofi.co.uk>
Date: Thu, 1 Aug 2019 22:57:38 +0100
Subject: [PATCH] Added TODO comments. Fix to the Clean method.

---
 .../HtmlEncodeStringPropertiesConverter.cs           | 10 ++++++++++
 .../Extensions/StringExtensions.cs                   | 12 +++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

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.
 	/// <summary>
 	/// A custom <see cref="JsonConverter"/> to ensure string values are properly encoded and decoded to mitigate XSS attacks.
 	/// </summary>
@@ -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("&#13;", "\r");
 			}
 
-			//Replace the following in strings
+			// Replace the following in strings
 			sb.Replace("&amp;", "&");
 			sb.Replace("&#39;", "'");
 			sb.Replace("&quot;", "\"");
-			sb.Replace("&#8216;", "'"); //Left quote
-			sb.Replace("&#8217;", "'"); //Right quote
+			sb.Replace("&#8216;", "'"); // Left quote
+			sb.Replace("&#8217;", "'"); // Right quote
+			sb.Replace("&#163;", "£");
 
 			if (trimNewLines)
 			{