thoth-org / Thoth.Json

Library for working with JSON in a type safe manner, this libs is targeting Fable
https://thoth-org.github.io/Thoth.Json/
MIT License
151 stars 34 forks source link

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

Open RicoSaupe opened 1 month ago

RicoSaupe commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

MangelMaxime commented 1 month ago

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
    )
// 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:

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 commented 1 month ago
// 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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

MangelMaxime commented 1 month ago

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