Skip to content

Conversation

@NikiforovAll
Copy link
Contributor

@NikiforovAll NikiforovAll commented May 7, 2020

Fixes #34626

I decided to take over because of no activity of the current assignee. Let me know if I'm moving in the right direction.

@NikiforovAll NikiforovAll force-pushed the jsonSerializerdefaults-for-json-serializer-options branch from 5e3b9d7 to 3d042df Compare May 8, 2020 09:36
@NikiforovAll
Copy link
Contributor Author

skipped docs/coding-guidelines/updating-ref-source.md: ref-source was updated manually

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

@layomia layomia added this to the 5.0 milestone May 8, 2020
@layomia layomia requested review from pranavkm, rynowak and terrajobst May 8, 2020 16:41
@layomia
Copy link
Contributor

layomia commented May 8, 2020

Pinging ASP.NET folks and @terrajobst as there was some discussion about whether options.Encoder should be set to JavaScriptEncoder.UnsafeRelaxedJsonEscaping when using Web defaults - https://github.com/dotnet/aspnetcore/pull/21016/files#r411571846.

@pranavkm previously mentioned that UnsafeRelaxedJsonEscaping is only safe if the JSON isn't interspersed with markup, and that Web defaults might not convey that this is meant for REST/API scenarios. I just want to be sure about the conclusion here before this PR is merged.

if (defaults == JsonSerializerDefaults.Web)
{
_propertyNameCaseInsensitive = true;
_jsonPropertyNamingPolicy = JsonNamingPolicy.CamelCase;
Copy link
Member

Choose a reason for hiding this comment

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

@layomia - asked me to drop by and confirm whether this matches ASP.NET's expectations. The relevant code is here for anyone who wants to compare.

Some items worth of discussion:

  • MaxDepth
  • relaxed encoder

For MaxDepth - we did not scientifically compute the number for maxdepth. We have just always had a value of 32 - and yours is 64. I don't foresee a problem with us using 64. So I think we can agree to use your default of 64.

For the encoder - (this is the fun one) we do some pretty subtle stuff. Our main options class doesn't specify an encoder, and we special case it in two places:

If we want to be technical - JSON as a document/request/response with the content type application/json is always unicode. The JSON specs all agree, that JSON is all unicode all of the time. We use the relaxed encoder when sending JSON over the wire as its own self-contained payload because all consumers should expect it to the unicode.

We don't use the relaxed encoder when JSON is being embedded inside an HTML response, because that's browser-land, and it's much more of a wierd place.

I think regardless of which encoder is used as the default we'll keep our behavior the same. I won't try to cosplay as a security expert. My assumption is that the common task using the "web" settings is to read and write JSON documents for communication with a REST API, so my intuition is that using the relaxed encoding is right.

/cc @GrabYourPitchforks @pranavkm

Copy link
Contributor Author

@NikiforovAll NikiforovAll May 8, 2020

Choose a reason for hiding this comment

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

My assumption is that the common task using the "web" settings is to read and write JSON documents for communication with a REST API, so my intuition is that using the relaxed encoding is right.

I agree with you on that. Although, just Web option could mislead code clients to potential vulnerability issues. We could provide a more verbose name that communicates usage and intentions.

What do you think about adding separate enum? I this case, developer hits dot in IDE and sees that there is another thingy for HTML

  • General
  • Web (relaxed encoding)
  • WebHtmlClient (default encoding)

Waiting for the final decision on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I discussed with @terrajobst - since specifying the unsafe relaxed encoder isn't recommended in some scenarios, we shouldn't specify it in a generic "web" default. Developers should specify it atop the default, depending on the scenario, as is done in an example @rynowak provided above. So, the current implementation works fine.

We don't want to have various "web" default variants (WebRest, WebHtml etc) as this would likely introduce the overhead of figuring out what the differences are, particularly if the set of defaults grows.

We'll leave max depth (effective) as 64, since we don't foresee issues with ASP.NET switching to this number.

@NikiforovAll NikiforovAll force-pushed the jsonSerializerdefaults-for-json-serializer-options branch from 7329138 to 65c0c05 Compare May 8, 2020 22:31
@NikiforovAll
Copy link
Contributor Author

@layomia. Could you please clarify the final decision regarding the Encoder?

Copy link
Contributor

@layomia layomia left a comment

Choose a reason for hiding this comment

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

Just a couple comments, otherwise LGTM. We can merge when addressed & conflicts are fixed.

@NikiforovAll
Copy link
Contributor Author

@layomia I assume you want me to add/update .NET API Reference Docs. Also, I would like to add a little section to docs/standard/serialization/system-text-json-how-to.md, something like "Use predefined JsonSerializerOptions", but I'm not sure if it is valuable enough.

@NikiforovAll
Copy link
Contributor Author

NikiforovAll commented May 27, 2020

@layomia
Unfortunately, I've not found guidance on how to update API Reference Docs. Could you please assist? 🙂

@layomia
Copy link
Contributor

layomia commented May 27, 2020

@NikiforovAll, I didn't intend for you to update the docs. We'll handle that e.g. with dotnet/dotnet-api-docs#4284. I was assigning authors to PRs as part of triage. Thanks for reaching out, and sorry for the noise.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't forget about JsonSerializerOptions.CreateForWeb()

5 participants