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

Offers APIs allowing to customise the underlying JSON library (with the risk of breaking something by the user) #194

Open
RicoSaupe opened this issue May 22, 2024 · 10 comments

Comments

@RicoSaupe
Copy link

Hi. I am using a json with more than 64 level of hierarchy. How can I set the maxdepth in an easy way without overriding the fromstring function.

error i am getting:

Given an invalid JSON: The reader's MaxDepth of 64 has been exceeded. Path 'documentLibs[0].folders[0].folders[0].folders[1].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0].folders[0]', line 1418, position 129.

currently i am overwriting the function but this doesnt struck me as a good solution.

image

@MangelMaxime
Copy link
Contributor

Hello,

This is caused by NewtonsSoft which introduce the max depth 64 for working around a security vulnerability.

I see 2 solutions, either I set the MapDepth to null in Thoth.Json.Net so it never happens again but then there is a security vulnerability according to the SO answer.

Or, the user shadow the fromString function with a custom function like you did. And then this is the responsibility of the user to decide to take the risk or not.

Note

This is a first time that I see such feature as maxDepth for JSON and I suppose not all libraries / runtime with behave the same way in such a situation.

TBH I am not sure what is the best decision to take here. If people with more experience in security domain etc. can voice in I am interested in reading what you think?

@njlr
Copy link
Contributor

njlr commented May 22, 2024

Hello,

This is caused by NewtonsSoft which introduce the max depth 64 for working around a security vulnerability.

I see 2 solutions, either I set the MapDepth to null in Thoth.Json.Net so it never happens again but then there is a security vulnerability according to the SO answer.

Or, the user shadow the fromString function with a custom function like you did. And then this is the responsibility of the user to decide to take the risk or not.

Note

This is a first time that I see such feature as maxDepth for JSON and I suppose not all libraries / runtime with behave the same way in such a situation.

TBH I am not sure what is the best decision to take here. If people with more experience in security domain etc. can voice in I am interested in reading what you think?

With the upcoming changes to Thoth.Json, I think there is an opportunity to make this user configurable.

Each JSON backend can provide additional functions, where options specific to that backend are supplied.

For example, Thoth.Json.Newtonsoft could provide these:

let fromString (decoder: Decoder<'T>) : Result<'T, string> =
  // ...

let unsafeFromString (decoder: Decoder<'T>) : 'T =
  // ...

let fromStringWithOptions (options : JsonSerializerSettings) (decoder: Decoder<'T>) : Result<'T, string> =
  // ...

let unsafeFromStringWithOptions (options : JsonSerializerSettings) (decoder: Decoder<'T>) : 'T =
  // ...

This allows each backend to be configured in arbitrary ways, whilst also keeping a simple default.

@RicoSaupe
Copy link
Author

that sounds good. As I understand its not yet in the package?

@MangelMaxime
Copy link
Contributor

With the upcoming changes to Thoth.Json, I think there is an opportunity to make this user configurable.

Indeed, this is an opportunity.

We could decide on a set of common public API they all should have like:

Decode.fromString
Decode.unsafeFromString
// etc.

and also expose more specialised handlers to expose customisation for people that need it. These handlers can be dangerous because if not setup correctly then user could generate a JSON which is not supported by other backend.

For exemple, in the case of NewtonSoft the following settings are mandartory:

let serializationSettings =
    JsonSerializerSettings(
        DateParseHandling = DateParseHandling.None,
        CheckAdditionalContent = true
    )
  • Should we try to check that things are setup correctly and fails early?
  • Should we expose a custom record with the option we allows to customise?
  • Should we override the settings?
// Pseudo code, I don't know if this is allowed by NewtonSoft
let fromStringWithOptions (options : JsonSerializerSettings) (decoder: Decoder<'T>) : Result<'T, string> =
	options.DateParseHandling <- DateParseHandling.None
	options.CheckAdditionalContent <- true

As I understand its not yet in the package?

The new packages are already published in beta:

  • Thoth.Json.Core
  • Thoth.Json.NewtonSoft
  • Thoth.Json.JavaScript
  • Thoth.Json.Python

But they are not yet officially announced. I am finishing the documentation so people know how to consume then, even if there is not much changes in reality. The main changes is that to share APIs you don't need to use compiler directives anymore.

Also these packages for now only support the manual API, auto API will be supported soon too thanks to @njlr work.

@njlr
Copy link
Contributor

njlr commented May 22, 2024

// Pseudo code, I don't know if this is allowed by NewtonSoft
let fromStringWithOptions (options : JsonSerializerSettings) (decoder: Decoder<'T>) : Result<'T, string> =
	options.DateParseHandling <- DateParseHandling.None
	options.CheckAdditionalContent <- true

I don't think this is appropriate since it prevents users from having full control - maybe they have a good reason for CheckAdditionalContent to be disabled?

One approach in Elm is to have an Advanced namespace to indicate that the operations must be used carefully.

So perhaps this?

module Decode = 
  
  let fromString (decoder: Decoder<'T>) : Result<'T, string> =
    // ...

  let unsafeFromString (decoder: Decoder<'T>) : 'T =
    // ...



module Advanced = 

  module Decode = 

    let fromStringWithOptions (options : JsonSerializerSettings) (decoder: Decoder<'T>) : Result<'T, string> =
      // ...

    let unsafeFromStringWithOptions (options : JsonSerializerSettings) (decoder: Decoder<'T>) : 'T =
      // ...

@MangelMaxime
Copy link
Contributor

I don't think this is appropriate since it prevents users from having full control - maybe they have a good reason for CheckAdditionalContent to be disabled?

That's what I tough too.

One approach in Elm is to have an Advanced namespace to indicate that the operations must be used carefully.

This is indeed something we are already using in Thoth.Json.Net

https://github.com/thoth-org/Thoth.Json.Net/blob/dde35518f9eda3c5d35b39dd97945892e4435018/src/Decode.fs#L1439-L1443

@RicoSaupe
Copy link
Author

RicoSaupe commented May 22, 2024

I dont know about enforcing some values, maybe we need to be flexible here. I implemented it similar in my solution but this function was in a shared project used by fable and .net. And JsonSerializerSettings cannot be used with fable. So it only works for .net so far. i had to put the directives around that.

#if !FABLE_COMPILER
    let fromStringWithOptions (options: JsonSerializerSettings) (decoder: Decoder<'T>) =
        fun value ->
            try
                let serializer = JsonSerializer.Create(options)
...
                
                
#endif

@MangelMaxime
Copy link
Contributor

I dont know about enforcing some values, maybe we need to be flexible here. I implemented it similar in my solution but this function was in a shared project used by fable and .net. And JsonSerializerSettings cannot be used with fable. So it only works for .net so far. i had to put the directives around that.

#if !FABLE_COMPILER
    let fromStringWithOptions (options: JsonSerializerSettings) (decoder: Decoder<'T>) =
        fun value ->
            try
                let serializer = JsonSerializer.Create(options)
...
                
                
#endif

This situation should not happen anymore with the new API and packages. The decoder/encoder API is runtime agnostics meaning that you can share them across packages without the need for compiler directives.

Shared/Types.fs - can be shared across projects

module Shared.Types

open Thoh.Json.Core

type User =
	{
		Name : string
	}

module User =

	let decoder : Decoder<User> =
		Decode.object (fun get ->
			{
				Name = get.Required.Field "name" Decode.string
			}
		)

JavaScript/Main.fs

open Shared.Typed
open Thoth.Json.JavaScript

Decode.fromString "json" User.decoder

DotNet/Main.fs

open Shared.Typed
open Thoth.Json.Newtonsoft

Decode.fromString "json" User.decoder

@RicoSaupe
Copy link
Author

Thanks for all the answers. I got that working now. Still have to make the move to the new core packages

@MangelMaxime
Copy link
Contributor

I am re-opening because we want to provide ability to the user to customise the options

@MangelMaxime MangelMaxime reopened this May 23, 2024
@MangelMaxime MangelMaxime changed the title The reader's MaxDepth of 64 has been exceeded. Offers APIs allowing to customise the underlying JSON library (with the risk of breaking something by the user) May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants