Serialize only simple types to session in TempData #2317
Conversation
ghost
added
the
|
cc @Eilon |
| @@ -76,6 +76,44 @@ public void Save_NullSession_NonEmptyDictionary_Throws() | |||
| }); | |||
| } | |||
|
|
|||
| [Theory] | |||
| [MemberData(nameof(InvalidTypes))] | |||
| public void Save_InvalidType_Throws(string key, object value, Type type) | |||
There was a problem hiding this comment.
Can you add test to verify that all the types mentioned in TypeHelper.IsSimpleType can be deserialized...I am specifically interested about struct and also not sure about Uri
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Common/TypeHelper.cs#L68
|
We need functional tests that verify these values round trip correctly - if I throw in a |
|
Updated. |
| testProvider.EnsureObjectCanBeSerialized(new Dictionary<string, object> { { key, value } }); | ||
| } | ||
|
|
||
| public static TheoryData<string, object, Type> InvalidTypes |
There was a problem hiding this comment.
Formatting nit: member data needs to preceed the actual test.
|
|
it from me. Get sign off from 
| private readonly JsonSerializer _jsonSerializer = JsonSerializer.Create( | ||
| new JsonSerializerSettings() | ||
| { | ||
| TypeNameHandling = TypeNameHandling.Auto |
There was a problem hiding this comment.
Need to consult with @blowdart to use this because it is generally not allowed to use any TypeNameHandling value other than None. I don't know if we can guarantee that the data is coming from a trusted source. For example, suppose that the session state provider is a custom one that stores all session data in a cookie - that is no longer trusted data because it can be tampered. Our default session state providers can't be tampered because they are stored in trusted environments (local machine memory, Azure Cache, etc.).
There was a problem hiding this comment.
Indeed. You can't ship this. Use of Auto is banned because it enables RCE. Unless you can ensure every input is signed, and validated you need to find another way to do this.
| private readonly JsonSerializer _jsonSerializer = JsonSerializer.Create( | ||
| new JsonSerializerSettings() | ||
| { | ||
| TypeNameHandling = TypeNameHandling.None |
There was a problem hiding this comment.
When am I ever HAPPY? I am content.
|
What about dictionaries? Is it easy to support those? |
| tempDataDictionary = jsonSerializer.Deserialize<Dictionary<string, object>>(writer); | ||
| tempDataDictionary = _jsonSerializer.Deserialize<Dictionary<string, object>>(writer); | ||
| } | ||
| foreach (var item in tempDataDictionary.ToList()) |
There was a problem hiding this comment.
Could you create a new dictionary rather than writing to this one (preferably one with StringComparer.OrdinalIgnoreCase semantics? Seems cleaner than doing a ToList here. An additional advantage is we could change the dictionary we wrap in TempDataDictionary to be a copy-on-write one.
|
|
||
| private static readonly ConcurrentDictionary<Type, Func<JArray, object>> _arrayConverters = | ||
| new ConcurrentDictionary<Type, Func<JArray, object>>(); | ||
| private static readonly ConcurrentDictionary<Type, Func<JObject, object>> _dictConverters = |
|
It can only be string because |
|
Good :) |
| actualTypes = actualTypes ?? new Type[] { itemType }; | ||
|
|
||
| // Throw if the key type of the dictionary is not string. | ||
| if (actualTypes.Length > 1 && actualTypes[0] != typeof(string)) |
There was a problem hiding this comment.
Do this early on as part of detecting the IDictionary<,>. You can avoid having Type[] actualTypes and just have one.
|
⌚ |
|
Updated. |
| <value>Cannot deserialize JToken of type {0}.</value> | ||
| </data> | ||
| <data name="TempData_CannotSerializeDictionary" xml:space="preserve"> | ||
| <value>The dictionary with TKey {0} cannot be serialized to Session by '{1}'.</value> |
There was a problem hiding this comment.
The '{0}' cannot serialize a dictionary with a key of type '{1}' to session state. (Swapped {0} and {1} too.)
|
Just some small string comments, then |
| } | ||
| else if (itemType.GetTypeInfo().IsGenericType) | ||
| { | ||
| if (itemType.ExtractGenericInterface(typeof(IList<>)) != null || |
There was a problem hiding this comment.
Just split this out:
if (itemType.ExtractGenericInterface(typeof(IList<>)))
{
Debug.Asser(itemType.GetGenericArguments().Length == 1, "IList<T> has one argument");
itemType = itemType.GetGenericArguments()[0];
}
if (itemType.ExtractGenericInterface(typeof(IDictionary<>)))
{
var genericTypeArguments = itemType.GetGenericArguments();
Debug.Assert(genericTypeArguments.Length == 0);
...
}
| <value>The '{0}' cannot serialize an object of type '{1}' to session state.</value> | ||
| </data> | ||
| <data name="TempData_CannotDeserializeToken" xml:space="preserve"> | ||
| <value>Cannot deserialize JToken of type {0}.</value> |
There was a problem hiding this comment.
Let's make JToken a parameter.
|
|
Issue - #2276
@pranavkm