Skip to content

Commit

Permalink
Added TODO comments.
Browse files Browse the repository at this point in the history
Fix to the Clean method.
  • Loading branch information
ricardoeduardo committed Aug 1, 2019
1 parent 24113ab commit f499f78
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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();
}

Expand All @@ -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?
}
}
12 changes: 7 additions & 5 deletions Core/src/Umbrella.Utilities/Extensions/StringExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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)
{
Expand Down

0 comments on commit f499f78

Please sign in to comment.