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 Newtonsoft 13.0.1 #2504

Merged
merged 16 commits into from
Jul 21, 2022
Merged

Update to Newtonsoft 13.0.1 #2504

merged 16 commits into from
Jul 21, 2022

Conversation

marmegh
Copy link
Contributor

@marmegh marmegh commented Jul 15, 2022

Bumps Newtonsoft.Json from 10.0.3 and 12.0.3 to 13.0.1. Additionally, this updates JSchema to v1.1.5 and Microsoft.NET.Test.Sdk to 17.4.0-preview-20220707-01 (both of which are implementing updates to Newtonsoft.Json).

Address Advisory: Improper Handling of Exceptional Conditions in Newtonsoft.Json for direct Newtonsoft.Json dependencies.

Supercedes: #2489. #2490, #2491, #2492, #2493, #2494, #2495, #2496, #2497, and #2499.

Release notes

Sourced from Newtonsoft.Json's releases.

13.0.1

  • New feature - Add JsonSelectSettings with configuration for a regex timeout
  • Change - Remove portable assemblies from NuGet package
  • Change - JsonReader and JsonSerializer MaxDepth defaults to 64
  • Fix - Fixed throwing missing member error on ignored fields
  • Fix - Fixed various nullable annotations
  • Fix - Fixed annotations not being copied when tokens are cloned
  • Fix - Fixed naming strategy not being used when deserializing dictionary enum keys
  • Fix - Fixed serializing nullable struct dictionaries
  • Fix - Fixed JsonWriter.WriteToken to allow null with string token
  • Fix - Fixed missing error when deserializing JToken with a contract type mismatch
  • Fix - Fixed JTokenWriter when writing comment to an object

12.0.3

  • New feature - Added support for nullable reference types
  • New feature - Added KebabCaseNamingStrategy
  • Change - Package now uses embedded package icon
  • Fix - Fixed bug when merging JToken with itself
  • Fix - Fixed performance of calling ICustomTypeDescriptor.GetProperties
  • Fix - Fixed serializing Enumerable.Empty and empty arrays on .NET Core 3.0
  • Fix - Fixed deserializing some collection types with constructor
  • Fix - Fixed deserializing IImmutableSet to ImmutableHashSet instead of ImmutableSortedSet
  • Fix - Fixed deserializing IImmutableDictionary to ImmutableDictionary instead of ImmutableSortedDictionary
  • Fix - Fixed deserializing into constructors with more than 256 parameters
  • Fix - Fixed hang when deserializing JTokenReader with preceding comment
  • Fix - Fixed JSONPath scanning with nested indexer
  • Fix - Fixed deserializing incomplete JSON object to JObject
  • Fix - Fixed using StringEnumConverter with naming strategy and specified values

12.0.2

  • New feature - Added MissingMemberHandling to JsonObjectAttribute and JsonObjectContract
  • New feature - Added constructor to JTokenReader to specify initial path
  • New feature - Added JsonProperty.IsRequiredSpecified
  • New feature - Added JsonContract.InternalConverter
  • Change - Moved embedded debug symbols in NuGet package to a symbol package on NuGet.org
  • Fix - Fixed deserializing nullable struct collections
  • Fix - Fixed memory link when serializing enums to named values
  • Fix - Fixed error when setting JsonLoadSettings.DuplicatePropertyNameHandling to Replace

12.0.1

  • New feature - Added NuGet package signing
  • New feature - Added Authenticode assembly signing
  • New feature - Added SourceLink support
  • New feature - Added constructors to StringEnumConverter for setting AllowIntegerValue
  • New feature - Added JsonNameTable and JsonTextReader.PropertyNameTable
  • New feature - Added line information to JsonSerializationException
  • New feature - Added JObject.Property overload with a StringComparison
  • New feature - Added JsonMergeSettings.PropertyNameComparison
  • New feature - Added support for multiple Date constructors with JavaScriptDateTimeConverter
  • New feature - Added support for strict equals and strict not equals in JSON Path queries

... (truncated)

<PackageReference Include="Microsoft.TeamFoundationServer.Client" Version="16.153.0" />
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All" />
<PackageReference Include="Newtonsoft.Json" Version="13.0.1" />
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Jul 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line looks new in this .csproj, is it needed #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is needed. Otherwise, TFS will default to an older version of Newtonsoft.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFS --- do you mean this dll? I see it defaults to 10.0.0

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, I think this line is not need, the reason is
Microsoft.Json.Schema 1.1.4 only needs Newtonsoft.Json (>= 9.0.1);
Microsoft.TeamFoundationServer.Client 16.153.0 needs
Newtonsoft.Json (>= 10.0.3);
so the result is 10.0.3;
now this project already using Microsoft.Json.Schema 1.1.5 needs Newtonsoft.Json (>= 13.0.1)
so we will get 13.0.1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eddy experimented on this one both ways and determined that it is needed at this time.

@@ -47,7 +47,7 @@

<ItemGroup>
<PackageReference Include="FluentAssertions" Version="5.10.2" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.0.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.4.0-preview-20220707-01" />
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Jul 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"17.4.0-preview-20220707-01"

I see we are using preview version, looks like we need a certain fix in that version.
can you add a comment in the PR for information purpose. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This preview version includes the update to Newtonsoft 13.0.1

@@ -4,6 +4,7 @@

* FEATURE: Add `max-file-size-in-kb` argument that allows filtering scan targets by file size. [#2494](https://github.com/microsoft/sarif-sdk/pull/2494)
* BUGFIX: Fix false positive for `SARIF1002.UrisMustBeValid` for file URIs that omit the `authority`. [#2501](https://github.com/microsoft/sarif-sdk/pull/2501)
* DEPENDENCY: Updating [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json/13.0.1) to v13.0.1, [Microsoft.Json.Schema](https://www.nuget.org/packages/Microsoft.Json.Schema) to v1.1.5, and [Microsoft.Json.Pointer](https://www.nuget.org/packages/Microsoft.Json.Pointer) to v1.1.5 in response to [Advisory: Improper Handling of Exceptional Conditions in Newtonsoft.Json](https://github.com/advisories/GHSA-5crp-9r3c-p9vr). [#2504](https://github.com/microsoft/sarif-sdk/pull/2504)
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Jul 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEPENDENCY:

previously we have like this, maybe add before the current string?
DEPENDENCY BREAKING: SARIF now requires Newtonsoft.Json v13.0.1. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't follow this convention when we updated to v12.0.1. In fact, I don't see a release history note for that specific update. In this case, the release history serves to tell anyone using the SARIF sdk that this version resolves the Newtonsoft vulnerability. I did consider this approach. If during testing, we see any indicators that this is indeed a breaking change, then we can re-evaluate the note.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to correct my previous statement. I was able to find the latest release history note and confirmed that this pattern has been repeated and took the suggestion. Thank you.

@@ -28,10 +28,11 @@
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="3.1.2" />
<PackageReference Include="Microsoft.Extensions.Logging.ApplicationInsights" Version="2.13.1" />
<PackageReference Include="Microsoft.Extensions.Logging.Console" Version="3.1.2" />
<PackageReference Include="Microsoft.Json.Schema" Version="1.1.4" />
<PackageReference Include="Microsoft.Json.Schema.Validation" Version="1.1.4" />
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Jul 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I also see this in solution, not sure if we want to update it as well. It is used in the generation of c# files.

<package id="Microsoft.Json.Schema.ToDotNet" version="1.1.4" #Resolved

@yongyan-gh
Copy link
Collaborator

yongyan-gh commented Jul 18, 2022

I created a local enlistment of this branch, and run BuildAndTest.cmd and ran BuildAndTest.cmd.
After build was completed, I still saw newtonsoft,json 10.0.3 in the packages folder in source root path.
Looks like some assemblies still reference the old 10.0.3 newtonsoft?


In reply to: 1188373442

@yongyan-gh
Copy link
Collaborator

don't see where the reference to 10.0.3 comes from, but i got this build error after block the access to 10.0.3

         Acquiring lock for the installation of Newtonsoft.Json 10.0.3
         Acquired lock for the installation of Newtonsoft.Json 10.0.3
         Reading project file C:\repo\github.com\sarifsdkNewton\src\Test.FunctionalTests.Sarif\Test.FunctionalTests.Sarif.csproj.
         Restoring packages for C:\repo\github.com\sarifsdkNewton\src\Test.FunctionalTests.Sarif\Test.FunctionalTests.Sarif.csproj...
         Restoring packages for .NETCoreApp,Version=v3.1...
     1>C:\Program Files\dotnet\sdk\6.0.302\NuGet.targets(130,5): error : Cannot create 'C:\repo\github.com\sarifsdkNewton\packages\newtonsoft.json\10.0.3' because a file or directory with the same name alread
       y exists. [C:\repo\github.com\sarifsdkNewton\src\Sarif.Sdk.sln]
         Resolving conflicts for .NETCoreApp,Version=v3.1...
         System.IO.IOException: Cannot create 'C:\repo\github.com\sarifsdkNewton\packages\newtonsoft.json\10.0.3' because a file or directory with the same name already exists.
            at System.IO.FileSystem.CreateDirectory(String fullPath, Byte[] securityDescriptor)
            at System.IO.Directory.CreateDirectory(String path)
            at NuGet.Packaging.PackageExtractor.<>c__DisplayClass5_0.<<InstallFromSourceAsync>b__0>d.MoveNext()
         --- End of stack trace from previous location ---

In reply to: 1188373442

EasyRhinoMSFT
EasyRhinoMSFT previously approved these changes Jul 19, 2022
Copy link
Collaborator

@EasyRhinoMSFT EasyRhinoMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@EasyRhinoMSFT EasyRhinoMSFT dismissed their stale review July 19, 2022 17:11

revoking review

@EasyRhinoMSFT
Copy link
Collaborator

I see the 10.0.3 dependency in these packages:
microsoft.applicationinsights.windowsserver.telemetrychannel
microsoft.azure.kusto.data
microsoft.visualstudio.services.client
microsoft.teamfoundationserver.client

We use Microsoft.Extensions.Logging.ApplicationInsights, where all versions are marked deprecated except the latest. This is where the ref to microsoft.applicationinsights.windowsserver.telemetrychannel comes from I believe.


In reply to: 1188488582

@marmegh marmegh marked this pull request as ready for review July 20, 2022 05:50
@marmegh
Copy link
Contributor Author

marmegh commented Jul 20, 2022

Just wanted to note that as of the updates to the above packages, the scan from the build pipeline is passing.


In reply to: 1189381557

Copy link
Collaborator

@shaopeng-gh shaopeng-gh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

src/build.props Outdated
@@ -10,8 +10,8 @@
<Company Condition=" '$(Company)' == '' ">Microsoft</Company>
<Product Condition=" '$(Product)' == '' ">Microsoft SARIF SDK</Product>
<Copyright Condition=" '$(Copyright)' == '' ">© Microsoft Corporation. All rights reserved.</Copyright>
<VersionPrefix>2.4.15</VersionPrefix>
<PreviousVersionPrefix>2.4.14</PreviousVersionPrefix>
<VersionPrefix>2.4.16</VersionPrefix>
Copy link
Collaborator

@eddynaka eddynaka Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16

If we are changing this, we should also update the release history header #Closed

@@ -4,6 +4,7 @@

* FEATURE: Add `max-file-size-in-kb` argument that allows filtering scan targets by file size. [#2494](https://github.com/microsoft/sarif-sdk/pull/2494)
* BUGFIX: Fix false positive for `SARIF1002.UrisMustBeValid` for file URIs that omit the `authority`. [#2501](https://github.com/microsoft/sarif-sdk/pull/2501)
* DEPENDENCY BREAKING: SARIF now requires Newtonsoft.JSON 13.0.1. Updating [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json/13.0.1) to v13.0.1, [Microsoft.Json.Schema](https://www.nuget.org/packages/Microsoft.Json.Schema) to v1.1.5, and [Microsoft.Json.Pointer](https://www.nuget.org/packages/Microsoft.Json.Pointer) to v1.1.5 in response to [Advisory: Improper Handling of Exceptional Conditions in Newtonsoft.Json](https://github.com/advisories/GHSA-5crp-9r3c-p9vr). [#2504](https://github.com/microsoft/sarif-sdk/pull/2504)
Copy link
Collaborator

@eddynaka eddynaka Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newtonsoft.JSON 13.0.1

we updated a few more dependencies. don't we have to include those as well? #Resolved

Copy link
Collaborator

@eddynaka eddynaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link
Collaborator

@EasyRhinoMSFT EasyRhinoMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link
Collaborator

@yongyan-gh yongyan-gh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@marmegh marmegh merged commit 41c12ef into main Jul 21, 2022
@marmegh marmegh deleted the newtonsoftUpgrade branch July 21, 2022 15:46
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

Successfully merging this pull request may close these issues.

None yet

5 participants