umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
Other
4.49k stars 2.69k forks source link

Remove upper constraint of 12.99 for Newtonsoft.Json dependency for UmbracoCms.Core #12235

Closed nul800sebastiaan closed 2 years ago

nul800sebastiaan commented 2 years ago

Discussed in https://github.com/umbraco/Umbraco-CMS/discussions/12231

Originally posted by **DanDiplo** April 6, 2022 Currently [UmbracoCms.Core](https://www.nuget.org/packages/UmbracoCms.Core/) NuGet package (in Umbraco 8) has a dependency on `Newtonsoft.Json` expressed as: [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json/) (>= 12.0.1 && < 12.999999.0) This prevents upgrading to the latest version of `Newtonsoft.Json` (currently `13.0.1`) and, more crucially, prevents installing any other dependencies that require a later version than Umbraco allows. For instance, I was trying to install the latest version of some Google APIs, but these all required 13.0.1 and Umbraco's dependency on this prevented installation. See https://www.nuget.org/packages/Google.Apis.Core/ for an example. This means you can never use any of Google's recent APIs with Umbraco 8. It's also possible that there may be improvements or security fixes in `Newtonsoft.Json` that Umbraco will prevent being installed. Is there a good reason why this constraint can't be raised to 13.x ? If not, could it be removed for reasons outlined above. Thank you!
nul800sebastiaan commented 2 years ago

Fixed in https://github.com/umbraco/Umbraco-CMS/commit/2c0a380d8b63b94edf479b0b9976230629222953

nul800sebastiaan commented 2 years ago

To be clear, there's a workaround in the Discussion but this should work too for everyone else:

Install-Package Newtonsoft.Json -v 13.0.1 -IgnoreDependencies

Additionally, we're aware of a small security issue: https://github.com/JamesNK/Newtonsoft.Json/issues/2457

If you're not processing input from your anonymous site visitors there's nothing to worry about. But I guess it would make sense to update the default to v13, though it contains 2 breaking changes, so we'll have to weigh our options.

In order to work around the security issue now (for v12), you could set the MaxDepth at startup: GlobalConfiguration.Configuration.Formatters.JsonFormatter.SerializerSettings.MaxDepth = 128;

nul800sebastiaan commented 2 years ago

FYI: we discussed updating the dependency but since it contains breaking changes we can't do that. People can easily updat the dependency themselves when 8.14.4 comes out or right now by using the workaround I posted above.

ronaldbarendse commented 2 years ago

FYI: we discussed updating the dependency but since it contains breaking changes we can't do that.

If we don't bump the minimal dependency version, shouldn't we make sure MaxDepth is set to 128 and release that as a patch version? That would ensure this security issue is fixed using OOTB dependency versions and allow a manual upgrade to use the latest Newtonsoft.Json version if you want 👍🏻

nul800sebastiaan commented 2 years ago

MaxDepth is set to 128

This IS the breaking change. 😅 (and the new default is 64, not 128).

ronaldbarendse commented 2 years ago

As a drop-in workaround/fix for v8, this composer sets the MaxDepth to 64 for both the JsonConvert and ASP.NET formatters (if it's not already set to a value):

using System.Web.Http;
using Newtonsoft.Json;
using Umbraco.Core.Composing;

public class NewtonsoftJsonComposer : IUserComposer
{
    private const int DefaultMaxDepth = 64;

    public void Compose(Composition composition)
    {
        // Update default settings
        var defaultSettings = JsonConvert.DefaultSettings;
        JsonConvert.DefaultSettings = () =>
        {
            var settings = defaultSettings?.Invoke() ?? new JsonSerializerSettings();
            if (settings.MaxDepth.HasValue == false)
            {
                settings.MaxDepth = DefaultMaxDepth;
            }

            return settings;
        };

        // Update ASP.NET JSON formatter settings
        if (GlobalConfiguration.Configuration.Formatters.JsonFormatter.SerializerSettings.MaxDepth.HasValue == false)
        {
            GlobalConfiguration.Configuration.Formatters.JsonFormatter.SerializerSettings.MaxDepth = DefaultMaxDepth;
        }
    }
}
iqb-dawn commented 2 years ago

We updated only JSON.NET package using Install-Package Newtonsoft.Json -v 13.0.1 -IgnoreDependencies. This worked for us without updating any Umbraco package.

snerpton commented 2 years ago

@nul800sebastiaan It is great that this issue has been identified and fixed in version 7.15.8. However, this was back on 7 April, nearly 4 months ago, and we're still waiting for this version to be released.

I'm having difficulty explaining to some enterprise users of Umbraco why this security fix hasn't been released when v7 is still in the security phase and pre-end of life (https://umbraco.com/products/knowledge-center/long-term-support-and-end-of-life/).

Do you / HQ have an update on when 7.15.8 will be released?

Thanks, C

nul800sebastiaan commented 2 years ago

@snerpton There's a workaround a few comments above and we'll release 7.15.8 in the next few weeks.

snerpton commented 2 years ago

Thanks @nul800sebastiaan we're currently deploying the workaround.

Is there a reason why it is taking so long to release 7.15.8 that I can relay to the clients? They don't understand why it is taking ~4 months to release a security fix for a platform that is still in the security phase and pre-EOL for a vulnerability that was identified and fixed 4 months ago.

nul800sebastiaan commented 2 years ago

In our estimation it's not a low priority security update, there has been workarounds for a readily available (even just ignoring the version restrictions would have worked as well) and we needed to schedule some time to work on a proper release.

snerpton commented 2 years ago

OK, thank you for the response.