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 Newtonsoft.Json NuGet package of Rules project from 9.0.1 to 10.0.3 #937

Merged
merged 3 commits into from
Mar 20, 2018

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Mar 15, 2018

PR Summary

Update Newtonsoft.Json NuGet package of Rules project from 9.0.1 to 10.0.3 to ensure we have all the latest fixes and match the version of PsCore.
Version 11.0.1 got recently released but we want to bump the version only together with PowerShell when the next version gets released.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • User facing documentation needed
  • Change is not breaking
  • NA Make sure you've added a new test if existing tests do not effectively test the code changed
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Copy link
Member

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

PSCore seems to be on 10.0.3, do you think we'll have problems if we get out ahead of that?

@bergmeister
Copy link
Collaborator Author

bergmeister commented Mar 16, 2018

Good point.
Let me start with some analysis that I did first: PSScriptAnalyzer requires Newtonsoft.Json only for usage in the UseCompatibleCmdlets rule here for calling JObject.Parse for the json files being present in the installation folder. A brief, unrelated remark about security: The usage of json deserialisation seems to be safe to me (due to the method not deserializing to arbitrary .net objects and due to the deserialized files being in a protected directory) and is therefore not subject to the attacks presented last year at Black Hat.

PSScriptAnalyzer should in an ideal world not behave any different if different versions of PowerShell use different versions of Newtonsoft.Json. However, it is worthwhile noting that PowerShell still has not fixed issue PowerShell/PowerShell#2083, which states that if another 3rd party module has already imported a different version of Newtonsoft.Json then the first version being loaded will be used. This comment suggest that it might be safer to upgrade only to 10.0.3 to be more on the safe side. Do you know what the state about Windows PowerShell is? I could not find Newtonsoft.Json in either the GaC or the Windows PowerShell folder. I ran the test suite on pwsh using version 11 and 10 and on version 11 there were 4 dll loading failures on PowerShell Core. Therfore I will change it to upgrade to 10.0.3 only for the moment. Note that PowerShell has this recently created issue to update to the latest version of Newtonsoft.Json. Therefore maybe it is better/safer to stick to 10.0.3 for the next PSSA release but bump the version together with PowerShell when the time comes? The idea behind the upgrade was a preventative measure to get the latest fixes in case a future exploit in json deserialisation gets found since there are always various improvements to JSON deserialisation.

…erShell due to the current problem of loading multiple assembly version in pscore
@bergmeister bergmeister changed the title Update Newtonsoft.Json NuGet package of Rules project from 9.0.1 to 11.0.1 Update Newtonsoft.Json NuGet package of Rules project from 9.0.1 to 10.0.3 Mar 16, 2018
Copy link
Member

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

LGTM

@JamesWTruher JamesWTruher merged commit 3cd2910 into PowerShell:development Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants