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

Update to newer Newtonsoft or switch to System.Text.json #2202

Open
allenkepler-finastra opened this issue May 4, 2023 · 10 comments
Open

Update to newer Newtonsoft or switch to System.Text.json #2202

allenkepler-finastra opened this issue May 4, 2023 · 10 comments

Comments

@allenkepler-finastra
Copy link

The version of Newtonsoft referenced has known vulnerabilities. Anyone referencing this has to also reference a newer version of Newtonsoft to clear security scans. My solution is Newtonsoft free and I'd rather not add packages for my dev teams to accidentally use. If possible I think it would be better to switch to System.Text.Json or extract serialization in a similar fashion to how database was done.

Thanks

Vulnerability info:
GHSA-5crp-9r3c-p9vr

@odinserj
Copy link
Member

odinserj commented May 5, 2023

I'm afraid that issue belongs more to NuGet than to Hangfire and is present in other packages as well, for example, see the following question on Stack Overflow:

https://stackoverflow.com/questions/56727314/nuget-package-manager-does-not-install-package-with-highest-depencency-version

There was the DependencyVersion switch in the early days of NuGet, but I don't know how to use it with PackageReference tags. The problem is also described here – https://weblog.west-wind.com/posts/2014/Jun/19/Nuget-Dependencies-and-latest-Versions and there's an issue on GitHub in the NuGet repository that describes this issue in great detail, but unfortunately, I can't find it.

The thing is, Hangfire is an abstraction itself (since it's a framework). It does specify the minimum supported version for its dependencies, and actual versions can be specified (Hangfire will work with any of them) in the target application by specifying those dependent packages explicitly:

<ItemGroup>
    <PackageReference Include="Newtonsoft.Json" Version="*" />
</ItemGroup>

Unfortunately, if we try to implement this NuGet-based feature in the project itself, we'll be forced to specify only the latest versions almost all of the time, forcing developers into dependency hell problem, where one package requires one version, and another package requires another version of Hangfire.Core or Newtonsoft.Json.

UPD. This is a general issue with NuGet, and is tracked in NuGet/Home#5553.

@burningice2866
Copy link
Contributor

@allenkepler-finastra Mybe instruct your dev-team to always make sure they run on newest version of all packages their project references.

@allenkepler-finastra
Copy link
Author

I did find that this has been talked about a number of times here. I didn't see it at first and can't find the close issue button. Hopefully you'll consider updated to the minimal "safe" version in 2.0 or moving to System.Text.Json. As far as dependency hell goes, I've been their and done that. I've never had to do much more for newtonsoft than add binding redirects in my older net framework projects.

Thanks!

@odinserj
Copy link
Member

odinserj commented May 8, 2023

It can be very challenging to migrate to another serialisation framework, even if we talk about JSON – there can be different ways of expressing the same things (like types using the $type directive), and I'm afraid there will be breaking changes for some projects. So this step should be made with advanced care, and perhaps moved at least to Hangfire 2.0.

@sabiland
Copy link

sabiland commented May 8, 2023

I've tried to migrate my project from Newtonsoft to System.Text.json. I quickly abandoned that mission ;).

@odinserj
Copy link
Member

odinserj commented May 9, 2023

Yes, and there also can be some advanced configuration of the serialisation process that will be very difficult to match with System.Text.Json. Also if we even migrated to the other package, we will be required to set some early version to prevent breaking changes as well, yielding the same problems.

@jttommy
Copy link

jttommy commented Sep 28, 2023

At the very least I would like to be on Newtonsoft 13.

@odinserj
Copy link
Member

@jttommy, you can use any Newtonsoft.Json version, starting from 5.0.1 by explicitly referencing this package from your project, please see #2202 (comment).

@JamesTerwilliger
Copy link

@jttommy, you can use any Newtonsoft.Json version, starting from 5.0.1 by explicitly referencing this package from your project, please see #2202 (comment).

@odinserj I hear that, but it's quite frustrating to need to add a reference to an assembly that we ourselves don't need to use just to patch over a security vulnerability introduced by the library. At this point, all users of Newtonsoft JSON from NetStandard2.0 or higher should be using version 13.0.1 or higher, and allowing a minimum version that is well-known to be vulnerable feels a little odd, possibly irresponsible.

Just my $0.02 as someone who is now having to go in and add a ton of unnecessary references to Newtonsoft.Json in a bunch of projects that have nothing to do with Json.

@klemmchr
Copy link

klemmchr commented Oct 24, 2023

It can be very challenging to migrate to another serialisation framework, even if we talk about JSON – there can be different ways of expressing the same things (like types using the $type directive), and I'm afraid there will be breaking changes for some projects. So this step should be made with advanced care, and perhaps moved at least to Hangfire 2.0.

@odinserj instead of making a breaking change it would be way better to provide an extensibility point to allow different serializers (and therefore also custom ones provided by the user). For some projects it might be better to use Newtonsoft.Json (especially existing ones when they use custom json converters), for others System.Text.Json would be better, maybe some want to use XML. Looking at SerializationHelper it has a very simple public surface which essentially just consists out of string Serialize(object) and object Deserialize(string) (and some overloads here and there). From my understanding Hangfire really doesn't care about the serialized format that is written to storage.

Just as a thought, IGlobalConfiguration could expose something like UseSerializer(IJobSerializer) which could then be provided by the user. Serializers could then be moved to dedicated packages like Hangfire.Serialization.SystemTextJson and Hangfire.Serialization.JsonNet with some extension methods for IGlobalConfiguration. The Hangfire metapackage would reference Hangfire.Serialization.JsonNet and configure the Newtonsoft serializer as a default. This would provide great backwards compatibility. This would only break stuff for people not using the metapackage. However, migration is fairly simple and even easier than a schema migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

7 participants