Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add System.Type converter for JsonSerializer #34249

Merged
merged 3 commits into from
Mar 30, 2020

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Mar 29, 2020

Fixes #31567. Per the conversation in this issue & following triage, this PR adds a new converter for System.Type with the following behavior:

  • serialization: writes the AssemblyQualifiedName of the Type instance. This is compatible with Newtonsoft.Json.

  • deserialization: throws NotSupportedException, as deserializing Type instances from arbitrary user input is a security vulnerability. Relevant path and reader position information is added to the exception message, as applicable.

The behavior before this PR was a JsonException (max depth exceeded) on serialization, and JsonException on deserialization (most commonly due to the current token being JsonTokenType.String rather than JsonTokenType.StartObject as the serializer expects when parsing object types).


EDIT: the converter will throw a NotSupportedException for both serialization and deserialization.

@layomia layomia added this to the 5.0 milestone Mar 29, 2020
@layomia layomia self-assigned this Mar 29, 2020
@jkotas
Copy link
Member

jkotas commented Mar 29, 2020

What's the point of writing AssemblyQualifiedName when you cannot deserialize it?

@jkotas
Copy link
Member

jkotas commented Mar 29, 2020

IMHO, we should throw exception for both serialization and deserialization.

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can say A workaround is to use a custom converter and leave it at that. What is the implementation of this theoretical custom converter?

If we believe there's a secure way to implement this, the exception text should point to documentation which shows clearly how to do this safely.

@layomia
Copy link
Contributor Author

layomia commented Mar 30, 2020

IMHO, we should throw exception for both serialization and deserialization.

Makes sense to throw since we won't round-trip otherwise.

@layomia
Copy link
Contributor Author

layomia commented Mar 30, 2020

I don't think we can say A workaround is to use a custom converter and leave it at that. What is the implementation of this theoretical custom converter?

If we believe there's a secure way to implement this, the exception text should point to documentation which shows clearly how to do this safely.

I've removed the note about using a custom converter - users already know they can write one.

Not sure of a way to implement this safely for arbitrary user input. There doesn't seem to be much appetite to (de)serialize Type instances at the moment. If there is in the future, we can add some examples to the JSON docs and extend the exception message to link to them.

@GrabYourPitchforks
Copy link
Member

Not sure of a way to implement this safely for arbitrary user input.

It's not safe for arbitrary input, but it can be made safe for known-good inputs. For example, maybe your custom converter maintains a static Dictionary<string, Type> of known-allowed inputs. When an arbitrary user-provided string comes in, you look up the entry in that static dictionary, either returning the matched Type or throwing due to disallowed input. This mechanism could also be used to perform polymorphic deserialization safely.

It really depends on the application's specific scenario.

@layomia
Copy link
Contributor Author

layomia commented Mar 30, 2020

Test failure unrelated - #28553.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants