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

Add configurable maximum depth in ConvertFrom-Json with -Depth #8199

Merged
merged 8 commits into from Feb 20, 2019
Expand Up @@ -36,6 +36,13 @@ public class ConvertFromJsonCommand : Cmdlet
[Parameter()]
public SwitchParameter AsHashtable { get; set; }

/// <summary>
/// Gets or sets the maximum depth the json input is allowed to have. By default, it is 1024. For no maximum, set to 0.
adamgauthier marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
[Parameter]
adamgauthier marked this conversation as resolved.
Show resolved Hide resolved
[ValidateRange(ValidateRangeKind.Positive)]
public int Depth { get; set; } = 1024;

#endregion parameters

#region overrides
Expand Down Expand Up @@ -100,7 +107,7 @@ protected override void EndProcessing()
private bool ConvertFromJsonHelper(string input)
{
ErrorRecord error = null;
object result = JsonObject.ConvertFromJson(input, AsHashtable.IsPresent, out error);
object result = JsonObject.ConvertFromJson(input, AsHashtable.IsPresent, Depth, out error);

if (error != null)
{
Expand Down
Expand Up @@ -19,7 +19,7 @@ namespace Microsoft.PowerShell.Commands
/// <summary>
/// JsonObject class.
/// </summary>
[SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")]
[SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", Justification = "Preferring Json over JSON")]
public static class JsonObject
{
#region HelperTypes
Expand Down Expand Up @@ -112,7 +112,7 @@ private class StoppingException : System.Exception
/// <param name="input">The json text to convert.</param>
/// <param name="error">An error record if the conversion failed.</param>
/// <returns>A PSObject.</returns>
[SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")]
[SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", Justification = "Preferring Json over JSON")]
public static object ConvertFromJson(string input, out ErrorRecord error)
{
return ConvertFromJson(input, returnHashtable: false, out error);
Expand All @@ -128,8 +128,25 @@ public static object ConvertFromJson(string input, out ErrorRecord error)
/// <param name="error">An error record if the conversion failed.</param>
/// <returns>A <see cref="System.Management.Automation.PSObject"/> or a <see cref="System.Collections.Hashtable"/>
/// if the <paramref name="returnHashtable"/> parameter is true.</returns>
[SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")]
[SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", Justification = "Preferring Json over JSON")]
public static object ConvertFromJson(string input, bool returnHashtable, out ErrorRecord error)
{
return ConvertFromJson(input, returnHashtable, maxDepth: 1024, out error);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if a custom type SerializerSettings to encapsulate default 1024 maxDepth and false returnHashtable here would be better than adding overloads.

Copy link
Member

Choose a reason for hiding this comment

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

if there are other settings we would want in the future, then that would be a better approach

Copy link

Choose a reason for hiding this comment

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

I would consider the option to set -Depth less than 0 for no maximum depth.
0 or $Null are often the result of a wrong calculation. Besides, 0 could technically still be a desired "depth".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iRon7

Besides, 0 could technically still be a desired "depth".

How so? Wouldn't that mean you want the deserialization to always fail? Is that really a use case? Also, the underlying Newtonsoft.Json does not allow 0 or less as MaxDepth, it throws at runtime.

Copy link

@iRon7 iRon7 Jan 10, 2019

Choose a reason for hiding this comment

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

For a sample recurring hash table like:

$ht = @{
	Name = "Tree"
	Parent = @{
		Name = "Parent"
		Child = @{
			Name = "Child"
		}
	}
}
$ht.Parent.Child.Parent = $ht.Parent

A $ht | ConvertTo-Json -Depth 1 results in:

{
    "Parent":  {
                   "Child":  "System.Collections.Hashtable",
                   "Name":  "Parent"
               },
    "Name":  "Tree"
}

I was expecting for $ht | ConvertTo-Json -Depth 0 to result in:

{
    "Parent":  "System.Collections.Hashtable",
    "Name":  "Tree"
}

But I indeed get an error:

ConvertTo-Json : Cannot validate argument on parameter 'Depth'. The 0 argument is less than the minimum allowed range
of 1. Supply an argument that is greater than or equal to 1 and then try the command again.
At line:1 char:29

  • $ht | ConvertTo-Json -Depth 0
  •                         ~
    
    • CategoryInfo : InvalidData: (:) [ConvertTo-Json], ParameterBindingValidationException
    • FullyQualifiedErrorId : ParameterArgumentValidationError,Microsoft.PowerShell.Commands.ConvertToJsonCommand

Anyways, wouldn't it be better to get either a completely flat output or an error in case of a (incorrect calculated/unassigned/mistyped variable) Depth of 0 or $Null?
I think a negative Depth would just be more knowingly chosen.
In other words, I do not really expect it to be a desired used case, but in the case of a scripting failure, I think a flat result or an error is more clear then a possible performance issue.

Just a thought, the decision is up to you...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still of the opinion that unlimited is not needed. 1 to int32.MaxValue is sufficient. Any JSON over Int32.MaxValue is either broken or contrived.

Copy link
Member

Choose a reason for hiding this comment

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

In the real world, I expect the default of 1024 to be sufficient, so I would be ok with not needing unlimited. -Depth 0 for only the top level makes sense and consistent with Get-ChildItem -Depth 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iRon7 after thinking it through, I agree a negative value might be better than 0.

@markekraus I understand powershell needs to be secure by default but I think this is an opportunity to provide the option for people to just say "I know what I'm doing, get out of my way". Sure, you could have them type -Depth:([int]::MaxValue), but feels less convenient in my opinion and it would still have the overhead (however small) of the serializer repeatedly checking to make sure max depth isn't exceeded (not the case if you pass null).

What do you guys think of having -Depth -1 for unlimited and maybe throw an error on BeginProcessing() if -Depth is 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not about security.. it's about absurdity. The only JSON objects that are that deep are contrived (made to be deep for fun or to specifically test depth limits) or broken (whatever produced the JSON had some issue). 1024 is already absurdly deep for a JSON object. We all ready catch outliers with the default setting. Between 1024 and Int32.MaxValue is a the realm of intrigue only. We don't need to have special case logic for that. We don't even really need infinite depth. What is the target audience for such a feature? Who really has objects that deep?

Just because we can, doesn't mean we should I still stand by my statement that 1 to int32.MaxValue is sufficient.

}

/// <summary>
/// Convert a Json string back to an object of type <see cref="System.Management.Automation.PSObject"/> or
adamgauthier marked this conversation as resolved.
Show resolved Hide resolved
/// <see cref="System.Collections.Hashtable"/> depending on parameter <paramref name="returnHashtable"/>.
/// </summary>
/// <param name="input">The json text to convert.</param>
adamgauthier marked this conversation as resolved.
Show resolved Hide resolved
/// <param name="returnHashtable">True if the result should be returned as a <see cref="System.Collections.Hashtable"/>
/// instead of a <see cref="System.Management.Automation.PSObject"/>.</param>
/// <param name="maxDepth">The max depth allowed when deserializing the json input. Set to null for no maximum.</param>
/// <param name="error">An error record if the conversion failed.</param>
/// <returns>A <see cref="System.Management.Automation.PSObject"/> or a <see cref="System.Collections.Hashtable"/>
/// if the <paramref name="returnHashtable"/> parameter is true.</returns>
[SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", Justification = "Preferring Json over JSON")]
public static object ConvertFromJson(string input, bool returnHashtable, int? maxDepth, out ErrorRecord error)
{
if (input == null)
{
Expand Down Expand Up @@ -162,7 +179,7 @@ public static object ConvertFromJson(string input, bool returnHashtable, out Err
// This TypeNameHandling setting is required to be secure.
TypeNameHandling = TypeNameHandling.None,
MetadataPropertyHandling = MetadataPropertyHandling.Ignore,
MaxDepth = 1024
MaxDepth = maxDepth
});

switch (obj)
Expand All @@ -176,7 +193,8 @@ public static object ConvertFromJson(string input, bool returnHashtable, out Err
return returnHashtable
? PopulateHashTableFromJArray(list, out error)
: PopulateFromJArray(list, out error);
default: return obj;
default:
return obj;
}
}
catch (JsonException je)
Expand Down
@@ -1,6 +1,36 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.
Describe 'ConvertFrom-Json' -tags "CI" {

function New-NestedJson {
Param(
[ValidateRange(1, 2048)]
[int]
$Depth
)

$nestedJson = "true"

$Depth..1 | ForEach-Object {
$nestedJson = '{"' + $_ + '":' + $nestedJson + '}'
}

return $nestedJson
}

function Count-ObjectDepth {
Param([PSCustomObject] $InputObject)

for ($i=1; $i -le 2048; $i++)
{
$InputObject = Select-Object -InputObject $InputObject -ExpandProperty $i
if ($InputObject -eq $true)
{
return $i
}
}
}

Describe 'ConvertFrom-Json Unit Tests' -tags "CI" {

BeforeAll {
$testCasesWithAndWithoutAsHashtableSwitch = @(
Expand All @@ -9,12 +39,12 @@ Describe 'ConvertFrom-Json' -tags "CI" {
)
}

It 'Can convert a single-line object with AsHashtable switch set to <AsHashtable>' -TestCase $testCasesWithAndWithoutAsHashtableSwitch {
It 'Can convert a single-line object with AsHashtable switch set to <AsHashtable>' -TestCases $testCasesWithAndWithoutAsHashtableSwitch {
Param($AsHashtable)
('{"a" : "1"}' | ConvertFrom-Json -AsHashtable:$AsHashtable).a | Should -Be 1
}

It 'Can convert one string-per-object with AsHashtable switch set to <AsHashtable>' -TestCase $testCasesWithAndWithoutAsHashtableSwitch {
It 'Can convert one string-per-object with AsHashtable switch set to <AsHashtable>' -TestCases $testCasesWithAndWithoutAsHashtableSwitch {
Param($AsHashtable)
$json = @('{"a" : "1"}', '{"a" : "x"}') | ConvertFrom-Json -AsHashtable:$AsHashtable
$json.Count | Should -Be 2
Expand All @@ -25,7 +55,7 @@ Describe 'ConvertFrom-Json' -tags "CI" {
}
}

It 'Can convert multi-line object with AsHashtable switch set to <AsHashtable>' -TestCase $testCasesWithAndWithoutAsHashtableSwitch {
It 'Can convert multi-line object with AsHashtable switch set to <AsHashtable>' -TestCases $testCasesWithAndWithoutAsHashtableSwitch {
Param($AsHashtable)
$json = @('{"a" :', '"x"}') | ConvertFrom-Json -AsHashtable:$AsHashtable
$json.a | Should -Be 'x'
Expand All @@ -35,7 +65,7 @@ Describe 'ConvertFrom-Json' -tags "CI" {
}
}

It 'Can convert an object with Newtonsoft.Json metadata properties with AsHashtable switch set to <AsHashtable>' -TestCase $testCasesWithAndWithoutAsHashtableSwitch {
It 'Can convert an object with Newtonsoft.Json metadata properties with AsHashtable switch set to <AsHashtable>' -TestCases $testCasesWithAndWithoutAsHashtableSwitch {
Param($AsHashtable)
$id = 13
$type = 'Calendar.Months.December'
Expand All @@ -52,4 +82,86 @@ Describe 'ConvertFrom-Json' -tags "CI" {
$json | Should -BeOfType Hashtable
}
}

It 'Can convert an object of depth 1024 by default with AsHashtable switch set to <AsHashtable>' -TestCases $testCasesWithAndWithoutAsHashtableSwitch {
Param($AsHashtable)
$nestedJson = New-NestedJson -Depth 1024

$json = $nestedJson | ConvertFrom-Json -AsHashtable:$AsHashtable
adamgauthier marked this conversation as resolved.
Show resolved Hide resolved

if ($AsHashtable)
{
$json | Should -BeOfType Hashtable
}
else
{
$json | Should -BeOfType PSCustomObject
}
}

It 'Fails to convert an object of depth higher than 1024 by default with AsHashtable switch set to <AsHashtable>' -TestCases $testCasesWithAndWithoutAsHashtableSwitch {
Param($AsHashtable)
$nestedJson = New-NestedJson -Depth 1025

{ $nestedJson | ConvertFrom-Json -AsHashtable:$AsHashtable } |
adamgauthier marked this conversation as resolved.
Show resolved Hide resolved
Should -Throw -ErrorId "System.ArgumentException,Microsoft.PowerShell.Commands.ConvertFromJsonCommand"
}
}

Describe 'ConvertFrom-Json -Depth Tests' -tags "Feature" {

BeforeAll {
$testCasesJsonDepthWithAndWithoutAsHashtableSwitch = @(
@{ Depth = 2; AsHashtable = $true }
@{ Depth = 2; AsHashtable = $false }
@{ Depth = 200; AsHashtable = $true }
@{ Depth = 200; AsHashtable = $false }
@{ Depth = 2000; AsHashtable = $true }
@{ Depth = 2000; AsHashtable = $false }
)
}

It 'Can convert an object with depth less than Depth param set to <Depth> and AsHashtable switch set to <AsHashtable>' -TestCases $testCasesJsonDepthWithAndWithoutAsHashtableSwitch {
Param($AsHashtable, $Depth)
$nestedJson = New-NestedJson -Depth ($Depth - 1)

$json = $nestedJson | ConvertFrom-Json -AsHashtable:$AsHashtable -Depth $Depth

if ($AsHashtable)
{
$json | Should -BeOfType Hashtable
}
else
{
$json | Should -BeOfType PSCustomObject
}

(Count-ObjectDepth -InputObject $json) | Should -Be ($Depth - 1)
}

It 'Can convert an object with depth equal to Depth param set to <Depth> and AsHashtable switch set to <AsHashtable>' -TestCases $testCasesJsonDepthWithAndWithoutAsHashtableSwitch {
Param($AsHashtable, $Depth)
$nestedJson = New-NestedJson -Depth:$Depth

$json = $nestedJson | ConvertFrom-Json -AsHashtable:$AsHashtable -Depth $Depth

if ($AsHashtable)
{
$json | Should -BeOfType Hashtable
}
else
{
$json | Should -BeOfType PSCustomObject
}

(Count-ObjectDepth -InputObject $json) | Should -Be $Depth
}

It 'Fails to convert an object with greater depth than Depth param set to <Depth> and AsHashtable switch set to <AsHashtable>' -TestCases $testCasesJsonDepthWithAndWithoutAsHashtableSwitch {
Param($AsHashtable, $Depth)
$nestedJson = New-NestedJson -Depth ($Depth + 1)

{ $nestedJson | ConvertFrom-Json -AsHashtable:$AsHashtable -Depth $Depth } |
Should -Throw -ErrorId "System.ArgumentException,Microsoft.PowerShell.Commands.ConvertFromJsonCommand"
}
}
Expand Up @@ -5,9 +5,34 @@ Describe 'Unit tests for JsonObject' -tags "CI" {
BeforeAll {
$jsonWithEmptyKey = '{"": "Value"}'
$jsonContainingKeysWithDifferentCasing = '{"key1": "Value1", "Key1": "Value2"}'

$testCasesJsonDepthWithAndWithoutReturnHashTable = @(
@{ Depth = 2; ReturnHashTable = $true }
@{ Depth = 2; ReturnHashTable = $false }
@{ Depth = 200; ReturnHashTable = $true }
@{ Depth = 200; ReturnHashTable = $false }
@{ Depth = 2000; ReturnHashTable = $true }
@{ Depth = 2000; ReturnHashTable = $false }
)

function New-NestedJson {
Param(
[ValidateRange(1, 2048)]
[int]
$Depth
)

$nestedJson = "true"

$Depth..1 | ForEach-Object {
$nestedJson = '{"' + $_ + '":' + $nestedJson + '}'
}

return $nestedJson
}
}

It 'No error for valid string ''<name>'' with -ReturnHashTable:$<ReturnHashTable>' -TestCase @(
It 'No error for valid string ''<name>'' with -ReturnHashTable:<ReturnHashTable>' -TestCases @(
@{ name = "null"; str = $null; ReturnHashTable = $true }
@{ name = "empty"; str = ""; ReturnHashTable = $true }
@{ name = "spaces"; str = " "; ReturnHashTable = $true }
Expand All @@ -23,7 +48,7 @@ Describe 'Unit tests for JsonObject' -tags "CI" {
$errRecord | Should -BeNullOrEmpty
}

It 'Throw ArgumentException for invalid string ''<name>'' with -ReturnHashTable:$<ReturnHashTable>' -TestCase @(
It 'Throw ArgumentException for invalid string ''<name>'' with -ReturnHashTable:<ReturnHashTable>' -TestCases @(
@{ name = "plain text"; str = "plaintext"; ReturnHashTable = $true }
@{ name = "part"; str = '{"a" :'; ReturnHashTable = $true }
@{ name = "plain text"; str = "plaintext"; ReturnHashTable = $false }
Expand All @@ -34,7 +59,31 @@ Describe 'Unit tests for JsonObject' -tags "CI" {
{ [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($str, $ReturnHashTable, [ref]$errRecord) } | Should -Throw -ErrorId "ArgumentException"
}

Context 'Empty key name' {
It 'Can Convert json with depth less than -Depth:<Depth> with -ReturnHashTable:<ReturnHashTable>' -TestCases $testCasesJsonDepthWithAndWithoutReturnHashTable {
param ($Depth, $ReturnHashTable)
$errRecord = $null
$json = New-NestedJson -Depth ($Depth - 1)
[Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($json, $ReturnHashTable, $Depth, [ref]$errRecord)
$errRecord | Should -BeNullOrEmpty
}

It 'Can Convert json with depth equal to -Depth:<Depth> with -ReturnHashTable:<ReturnHashTable>' -TestCases $testCasesJsonDepthWithAndWithoutReturnHashTable {
param ($Depth, $ReturnHashTable)
$errRecord = $null
$json = New-NestedJson -Depth $Depth
[Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($json, $ReturnHashTable, $Depth, [ref]$errRecord)
$errRecord | Should -BeNullOrEmpty
}

It 'Throws ArgumentException for json with greater depth than -Depth:<Depth> with -ReturnHashTable:<ReturnHashTable>' -TestCases $testCasesJsonDepthWithAndWithoutReturnHashTable {
param ($Depth, $ReturnHashTable)
$errRecord = $null
$json = New-NestedJson -Depth ($Depth + 1)
{ [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($json, $ReturnHashTable, $Depth, [ref]$errRecord) } |
Should -Throw -ErrorId "ArgumentException"
}

Context 'Empty key name' {
It 'Throw InvalidOperationException when json contains empty key name' {
$errorRecord = $null
[Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($jsonWithEmptyKey, [ref]$errorRecord)
Expand All @@ -51,7 +100,7 @@ Describe 'Unit tests for JsonObject' -tags "CI" {
}

Context 'Keys with different casing ' {

It 'Throw InvalidOperationException when json contains key with different casing' {
$errorRecord = $null
[Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($jsonContainingKeysWithDifferentCasing, [ref]$errorRecord)
Expand Down