High Severity Vulnerability - Newtonsoft.Json < 13.0.1

[nuttytree]

There is a high severity vulnerability in Newtonsoft.Json prior to version 13.0.1. Because of the continued use of NJsonSchema 8.33.6323.36213 which was brought up as an issue previously in issue #284 and closed with a "we might fix this someday" response using this package forces the use of Newtonsoft.Json 12.0.3.

[mikechu-optimizely]

Thanks for the callout and research. Let me put this on our agenda for stand up and get it on the board.

[mikechu-optimizely]

I've opened internal ticket FSSDK-8938 to be reviewed.

[mikechu-optimizely]

Related PR: https://github.com/optimizely/csharp-sdk/pull/330

[tfabraham]

We've seen an unexpected behavior change around NJsonSchema and Newtonsoft.Json in the 3.11.2 release. That release claims to update nothing but docs and comments, but the full Git diff between the 3.11.1 and 3.11.2 releases reveals binding redirect and other changes related to those NuGet packages. Were those other changes included unintentionally?

[mikechu-optimizely]

Apologies for the inaccuracy. The primary reason for the release was to handle a documentation change. For C# we also had a critical fix for Newtonsoft and NJsonSchema.

[mikechu-optimizely]

I'm running a comparison again to review the full diff between the releases.

[nuttytree]

When I pulled in the 3.11.2 version of Optimizely it did not update the version of Newtonsoft.Json or NJsonSchema however I started getting errors validating the config file. When I also added Newtonsoft.Json 13.0.3 and NJsonSchema 10.8.0 then everything started working again. So it seems like something with the update of those NuGet's was not completely updated.

[mikechu-optimizely]

Thanks for providing the update experience. That's surprising that NuGet wouldn't have also updated the dependency versions at the same time. 🤔 .

Let me install 3.11.1 into a Hello World and then update it.

[mikechu-optimizely]

...but so I'm clear: Is the root of this Issue good to go with the vulnerability being closed?

[tfabraham]

I wouldn't consider it resolved because the NuGet package still declares the old (vulnerable) versions as minimums.

[mikechu-optimizely]

Grrr. You're right. That should have been updated by the release branch's nuspec.

I do see some misses in this file though, like version and releaseNotes.

Thanks for collaborating. I'm digging deeper.

[tfabraham]

Another thought - with the major version update to Newtonsoft.Json, it seems like your NuGet version should be bumped by at least a minor version vs. a patch.

[mikechu-optimizely]

Thanks for the notes. I agree with you. This would give more "severity" to the update. I think we'll need to amend our documentation/SOP for similar future situations/work.

For the moment, I'm focusing on getting a version 3.11.2.1 out to NuGet which requires some increment to republish. Before nuget push I'm going to manually review the nuspec in the package to confirm correct dep versions.

Thanks for hanging in there.

[mikechu-optimizely]

I've published version 3.11.2.1 to NuGet and would value the feedback and your consumer-side experience.

I'll hold this Issue and our corresponding internal Jira ticket open.

[tfabraham]

Thanks Mike. The .1 release's nuspec correctly reflects the required package versions.

It's pretty rare to see that fourth digit used. Our automated dependency management tool (Renovate) didn't see it as an available patch, but it's possible that's because it's so newly published. Since you did change the package (the nuspec), there's a good argument for 3.11.3. (3.12.0 would have been ideal given the major version changes.)

I think we're in a good state where things won't break with the dependencies now correctly declared. Thanks!

[mikechu-optimizely]

Oh, that's an excellent point/experience RE: the 4th lesser-used digit. During my code reviews, I had some DMs with a colleague where he head-scratched on my choice to use the 4th digit. 😬

Your 3.12.0 is well taken for this situation.

Note: We will be releasing a major version in the near-ish term to support a new CDP feature that's going into beta. I'll ensure we go back to a 3-digit semver format.

[mikechu-optimizely]

@nuttytree Whenever you get a chance, I'd like to check that you're solid with the root of this Issue. A 👍 or 👎 is good too.

[nuttytree]

I agree with @tfabraham on his comments but the major issue of the vulnerability is solved.

[mikechu-optimizely]

Thanks to you both.