Skip to content

Commit a327d7f

Browse files
gavinbarrongavinbarron
committed
fix tests and nesting behavior on streams with SharpYaml update
1 parent 79ed241 commit a327d7f

5 files changed

Lines changed: 149 additions & 24 deletions

File tree

‎src/Microsoft.OpenApi.YamlReader/YamlConversionBudget.cs‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -43,33 +43,39 @@ public YamlConversionBudget(uint maxDepth, uint maxNodeCount, uint maxAliasExpan
4343
/// <summary>
4444
/// Charges one node at the supplied depth.
4545
/// </summary>
46-
/// <param name="depth">Nesting depth of the node being materialized.</param>
46+
/// <param name="depth">Zero-based nesting depth of the node being materialized.</param>
4747
/// <exception cref="OpenApiReaderException">The depth or total node limit would be exceeded.</exception>
4848
public void EnterNode(uint depth)
4949
{
50-
if (depth > _maxDepth)
51-
{
52-
throw new OpenApiReaderException($"The YAML document exceeds the maximum supported nesting depth of {_maxDepth}.");
53-
}
54-
50+
ValidateDepth(depth);
5551
AddNodes(1);
5652
}
5753

5854
/// <summary>
5955
/// Charges the full cost of expanding an alias, against both the alias budget and the total budget.
6056
/// </summary>
61-
/// <param name="depth">Nesting depth at which the alias appears.</param>
57+
/// <param name="depth">Zero-based nesting depth at which the alias appears.</param>
6258
/// <param name="expandedNodeCount">Number of nodes the alias will materialize when cloned.</param>
59+
/// <param name="expandedHeight">
60+
/// Height of the subtree the alias will materialize, where a scalar has height 1.
61+
/// </param>
6362
/// <exception cref="OpenApiReaderException">The depth, alias, or total node limit would be exceeded.</exception>
6463
/// <remarks>
6564
/// Must be called before the clone is taken. Charging afterwards would allow the very allocation
6665
/// this limit exists to prevent.
6766
/// </remarks>
68-
public void EnterAlias(uint depth, uint expandedNodeCount)
67+
public void EnterAlias(uint depth, uint expandedNodeCount, uint expandedHeight)
6968
{
70-
if (depth > _maxDepth)
69+
ValidateDepth(depth);
70+
71+
// The alias site clears the depth check on its own, but expanding it grafts an entire
72+
// subtree at this position. Without charging the grafted height, an anchor defined at a
73+
// legal depth can be replayed from another legal depth to produce a tree deeper than the
74+
// limit. The underlying YAML parser cannot catch this either, because it sees an alias as
75+
// a single event and never re-walks the anchored content.
76+
if (expandedHeight > _maxDepth - depth)
7177
{
72-
throw new OpenApiReaderException($"The YAML document exceeds the maximum supported nesting depth of {_maxDepth}.");
78+
throw new OpenApiReaderException($"The YAML document expands an alias to more than the maximum supported nesting depth of {_maxDepth}.");
7379
}
7480

7581
if (expandedNodeCount > _maxAliasExpansionNodeCount - _aliasExpansionNodeCount)
@@ -81,6 +87,23 @@ public void EnterAlias(uint depth, uint expandedNodeCount)
8187
AddNodes(expandedNodeCount);
8288
}
8389

90+
/// <summary>
91+
/// Validates that a node at <paramref name="depth"/> is within the depth limit.
92+
/// </summary>
93+
/// <remarks>
94+
/// <paramref name="depth"/> is zero-based, so a node at that depth occupies level
95+
/// <c>depth + 1</c>. Rejecting <c>depth &gt;= _maxDepth</c> therefore admits exactly
96+
/// <c>_maxDepth</c> levels, matching the limit enforced by the underlying YAML parser.
97+
/// The comparison avoids arithmetic so it cannot overflow.
98+
/// </remarks>
99+
private void ValidateDepth(uint depth)
100+
{
101+
if (depth >= _maxDepth)
102+
{
103+
throw new OpenApiReaderException($"The YAML document exceeds the maximum supported nesting depth of {_maxDepth}.");
104+
}
105+
}
106+
84107
/// <summary>
85108
/// Charges <paramref name="count"/> nodes against the total node budget.
86109
/// </summary>

‎src/Microsoft.OpenApi.YamlReader/YamlConverter.cs‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,8 @@ public MaterializedNode Convert(YamlNode yaml, uint depth)
266266

267267
if (_completed.TryGetValue(yaml, out var completed))
268268
{
269-
_budget.EnterAlias(depth, completed.NodeCount);
270-
return new(completed.Node.DeepClone(), completed.NodeCount);
269+
_budget.EnterAlias(depth, completed.NodeCount, completed.Height);
270+
return new(completed.Node.DeepClone(), completed.NodeCount, completed.Height);
271271
}
272272

273273
_budget.EnterNode(depth);
@@ -278,7 +278,7 @@ public MaterializedNode Convert(YamlNode yaml, uint depth)
278278
{
279279
YamlMappingNode map => ConvertMapping(map, depth),
280280
YamlSequenceNode sequence => ConvertSequence(sequence, depth),
281-
YamlScalarNode scalar => new MaterializedNode(ToJsonValue(scalar.Value, scalar.Style), 1),
281+
YamlScalarNode scalar => new MaterializedNode(ToJsonValue(scalar.Value, scalar.Style), 1, 1),
282282
_ => throw new NotSupportedException("This yaml isn't convertible to JSON")
283283
};
284284
_completed.Add(yaml, materialized);
@@ -294,6 +294,7 @@ private MaterializedNode ConvertMapping(YamlMappingNode yaml, uint depth)
294294
{
295295
var node = new JsonObject();
296296
uint nodeCount = 1;
297+
uint maxChildHeight = 0;
297298
foreach (var keyValuePair in yaml)
298299
{
299300
if (keyValuePair.Key is not YamlScalarNode scalarKey || scalarKey.Value is null)
@@ -309,36 +310,43 @@ private MaterializedNode ConvertMapping(YamlMappingNode yaml, uint depth)
309310
var child = Convert(keyValuePair.Value, depth + 1);
310311
node.Add(scalarKey.Value, child.Node);
311312
nodeCount = checked(nodeCount + child.NodeCount);
313+
maxChildHeight = Math.Max(maxChildHeight, child.Height);
312314
}
313315

314-
return new(node, nodeCount);
316+
return new(node, nodeCount, maxChildHeight + 1);
315317
}
316318

317319
private MaterializedNode ConvertSequence(YamlSequenceNode yaml, uint depth)
318320
{
319321
var node = new JsonArray();
320322
uint nodeCount = 1;
323+
uint maxChildHeight = 0;
321324
foreach (var value in yaml)
322325
{
323326
var child = Convert(value, depth + 1);
324327
node.Add(child.Node);
325328
nodeCount = checked(nodeCount + child.NodeCount);
329+
maxChildHeight = Math.Max(maxChildHeight, child.Height);
326330
}
327331

328-
return new(node, nodeCount);
332+
return new(node, nodeCount, maxChildHeight + 1);
329333
}
330334
}
331335

332336
private sealed class MaterializedNode
333337
{
334-
public MaterializedNode(JsonNode node, uint nodeCount)
338+
public MaterializedNode(JsonNode node, uint nodeCount, uint height)
335339
{
336340
Node = node;
337341
NodeCount = nodeCount;
342+
Height = height;
338343
}
339344

340345
public JsonNode Node { get; }
341346
public uint NodeCount { get; }
347+
348+
/// <summary>Number of levels in this subtree, where a scalar has height 1.</summary>
349+
public uint Height { get; }
342350
}
343351

344352
private sealed class ReferenceEqualityComparer<T> : IEqualityComparer<T> where T : class

‎src/Microsoft.OpenApi.YamlReader/YamlJsonParser.cs‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,19 +20,27 @@ internal sealed class YamlJsonParser
2020
private readonly HashSet<string> _activeAnchors = new(StringComparer.Ordinal);
2121
private readonly Stack<ContainerFrame> _containers = new();
2222
private readonly uint _maxScalarLength;
23+
private readonly uint _maxDepth;
2324
private JsonNode? _root;
2425

2526
public YamlJsonParser(OpenApiYamlReaderSettings settings)
2627
{
2728
_budget = new(settings.MaxDepth, settings.MaxNodeCount, settings.MaxAliasExpansionNodeCount);
2829
_maxScalarLength = settings.MaxScalarLength;
30+
_maxDepth = settings.MaxDepth;
2931
}
3032

3133
public JsonNode Parse(TextReader input, CancellationToken cancellationToken)
3234
{
3335
cancellationToken.ThrowIfCancellationRequested();
3436
var cancellationReader = new CancellationTokenTextReader(input, cancellationToken);
35-
var parser = new Parser<LookAheadBuffer>(new LookAheadBuffer(cancellationReader, LookAheadBufferCapacity));
37+
38+
// SharpYaml applies its own nesting limit, defaulting to 64. Passing the configured limit
39+
// keeps the two enforcement points in agreement; leaving it unset would silently cap every
40+
// reader at 64 regardless of MaxDepth, making values above the default a no-op.
41+
var parser = new Parser<LookAheadBuffer>(
42+
new LookAheadBuffer(cancellationReader, LookAheadBufferCapacity),
43+
(int)_maxDepth);
3644
var documentStarted = false;
3745

3846
while (true)
@@ -104,6 +112,7 @@ private void AddScalar(Scalar scalar, CancellationToken cancellationToken)
104112
var materialized = new MaterializedNode(
105113
YamlConverter.ToJsonValue(scalar.Value, scalar.Style),
106114
1,
115+
1,
107116
scalar.Value);
108117

109118
RegisterCompletedAnchor(scalar.Anchor, materialized);
@@ -122,9 +131,9 @@ private void AddAlias(AnchorAlias alias, CancellationToken cancellationToken)
122131
throw new OpenApiReaderException($"The YAML alias '*{alias.Value}' refers to an unknown anchor.");
123132
}
124133

125-
_budget.EnterAlias((uint)_containers.Count, anchor.NodeCount);
134+
_budget.EnterAlias((uint)_containers.Count, anchor.NodeCount, anchor.Height);
126135
cancellationToken.ThrowIfCancellationRequested();
127-
AddNode(new(anchor.Node.DeepClone(), anchor.NodeCount, anchor.MappingKey));
136+
AddNode(new(anchor.Node.DeepClone(), anchor.NodeCount, anchor.Height, anchor.MappingKey));
128137
}
129138

130139
private void EndContainer()
@@ -140,7 +149,7 @@ private void EndContainer()
140149
throw new OpenApiReaderException("The YAML mapping contains a key without a value.");
141150
}
142151

143-
var materialized = new MaterializedNode(frame.Container, frame.NodeCount, null);
152+
var materialized = new MaterializedNode(frame.Container, frame.NodeCount, frame.MaxChildHeight + 1, null);
144153
if (frame.Anchor is not null)
145154
{
146155
_activeAnchors.Remove(frame.Anchor);
@@ -169,6 +178,7 @@ private void AddNode(MaterializedNode materialized)
169178
case JsonArray array:
170179
array.Add(materialized.Node);
171180
frame.NodeCount = checked(frame.NodeCount + materialized.NodeCount);
181+
frame.MaxChildHeight = Math.Max(frame.MaxChildHeight, materialized.Height);
172182
break;
173183
case JsonObject map when frame.PendingKey is null:
174184
frame.PendingKey = materialized.MappingKey
@@ -183,6 +193,7 @@ private void AddNode(MaterializedNode materialized)
183193
map.Add(frame.PendingKey, materialized.Node);
184194
frame.PendingKey = null;
185195
frame.NodeCount = checked(frame.NodeCount + materialized.NodeCount);
196+
frame.MaxChildHeight = Math.Max(frame.MaxChildHeight, materialized.Height);
186197
break;
187198
}
188199
}
@@ -221,19 +232,27 @@ private sealed class ContainerFrame(JsonNode container, string? anchor)
221232
public string? Anchor { get; } = anchor;
222233
public string? PendingKey { get; set; }
223234
public uint NodeCount { get; set; } = 1;
235+
236+
/// <summary>Height of the tallest child added so far; 0 while the container is empty.</summary>
237+
public uint MaxChildHeight { get; set; }
224238
}
225239

226240
private sealed class MaterializedNode
227241
{
228-
public MaterializedNode(JsonNode node, uint nodeCount, string? mappingKey)
242+
public MaterializedNode(JsonNode node, uint nodeCount, uint height, string? mappingKey)
229243
{
230244
Node = node;
231245
NodeCount = nodeCount;
246+
Height = height;
232247
MappingKey = mappingKey;
233248
}
234249

235250
public JsonNode Node { get; }
236251
public uint NodeCount { get; }
252+
253+
/// <summary>Number of levels in this subtree, where a scalar has height 1.</summary>
254+
public uint Height { get; }
255+
237256
public string? MappingKey { get; }
238257
}
239258

‎test/Microsoft.OpenApi.Readers.Tests/OpenApiYamlReaderTests.cs‎

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
using System;
1+
using System;
22
using System.IO;
33
using System.Linq;
44
using System.Text;
@@ -201,7 +201,55 @@ public void ReadReturnsDiagnosticForDeepNestingBeforeYamlDomComposition(bool flo
201201
var result = reader.Read(stream, DocumentLocation, SettingsFixture.ReaderSettings);
202202

203203
Assert.Null(result.Document);
204-
Assert.Contains(result.Diagnostic.Errors, error => error.Message.Contains("maximum supported nesting depth", StringComparison.Ordinal));
204+
Assert.Contains(result.Diagnostic.Errors, error => error.Message.Contains("maximum nesting depth", StringComparison.Ordinal));
205+
}
206+
207+
[Fact]
208+
public void ReadHonoursMaxDepthAboveTheUnderlyingParserDefault()
209+
{
210+
// SharpYaml applies its own nesting limit, defaulting to 64. Unless the reader forwards
211+
// MaxDepth to it, every configured value above that default silently has no effect.
212+
const int depth = 100;
213+
var yaml = new string('[', depth) + new string(']', depth);
214+
var reader = new OpenApiYamlReader(new OpenApiYamlReaderSettings { MaxDepth = 200 });
215+
using var stream = CreateStream(yaml);
216+
217+
var result = reader.Read(stream, DocumentLocation, SettingsFixture.ReaderSettings);
218+
219+
Assert.DoesNotContain(result.Diagnostic.Errors, error => error.Message.Contains("nesting depth", StringComparison.Ordinal));
220+
}
221+
222+
[Fact]
223+
public void ReadHonoursMaxDepthBelowTheUnderlyingParserDefault()
224+
{
225+
const int depth = 40;
226+
var yaml = new string('[', depth) + new string(']', depth);
227+
var reader = new OpenApiYamlReader(new OpenApiYamlReaderSettings { MaxDepth = 32 });
228+
using var stream = CreateStream(yaml);
229+
230+
var result = reader.Read(stream, DocumentLocation, SettingsFixture.ReaderSettings);
231+
232+
Assert.Null(result.Document);
233+
Assert.Contains(result.Diagnostic.Errors, error => error.Message.Contains("nesting depth of 32", StringComparison.Ordinal));
234+
}
235+
236+
[Fact]
237+
public void ReadRejectsAliasExpansionThatExceedsMaxDepth()
238+
{
239+
// The anchor and the alias each sit within the depth limit, but expanding the alias grafts
240+
// the anchored subtree onto an equally deep position, producing a tree twice as deep. The
241+
// YAML parser cannot catch this because it sees the alias as a single event.
242+
const int half = 50;
243+
var yaml =
244+
$"a: &d {new string('[', half)}{new string(']', half)}\n" +
245+
$"b: {new string('[', half)}*d{new string(']', half)}\n";
246+
var reader = new OpenApiYamlReader();
247+
using var stream = CreateStream(yaml);
248+
249+
var result = reader.Read(stream, DocumentLocation, SettingsFixture.ReaderSettings);
250+
251+
Assert.Null(result.Document);
252+
Assert.Contains(result.Diagnostic.Errors, error => error.Message.Contains("expands an alias", StringComparison.Ordinal));
205253
}
206254

207255
[Fact]

‎test/Microsoft.OpenApi.Readers.Tests/YamlConverterTests.cs‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ public void ExcessiveNestingDepthIsRejected()
363363
const int depth = 70;
364364
var deeplyNested = new string('[', depth) + new string(']', depth);
365365

366-
Assert.Throws<OpenApiReaderException>(() => ConvertYamlStringToJsonNode(deeplyNested));
366+
Assert.Throws<YamlException>(() => ConvertYamlStringToJsonNode(deeplyNested));
367367
}
368368

369369
[Fact]
@@ -401,6 +401,33 @@ public void ComplexMappingKeyIsRejectedAsReaderException()
401401
Assert.Throws<OpenApiReaderException>(() => mapping.ToJsonNode());
402402
}
403403

404+
[Fact]
405+
public void SharedSubtreeGraftedTooDeepIsRejected()
406+
{
407+
// The converter memoizes each YamlNode it has already materialized, then deep-clones the
408+
// result when the same instance reappears. The shared subtree clears the depth check where
409+
// it is first seen, so without charging its height at the reuse site it can be replayed
410+
// from a deeper position to build a tree past the limit.
411+
YamlNode shared = new YamlScalarNode("value");
412+
for (var index = 0; index < 40; index++)
413+
{
414+
shared = new YamlSequenceNode(shared);
415+
}
416+
417+
var grafted = shared;
418+
for (var index = 0; index < 30; index++)
419+
{
420+
grafted = new YamlSequenceNode(grafted);
421+
}
422+
423+
// The shallow element is converted first and memoizes the shared subtree; the deep element
424+
// then reuses it 31 levels down, which would materialize 72 levels against a limit of 64.
425+
var root = new YamlSequenceNode(shared, grafted);
426+
427+
var exception = Assert.Throws<OpenApiReaderException>(() => root.ToJsonNode());
428+
Assert.Contains("expands an alias", exception.Message, StringComparison.Ordinal);
429+
}
430+
404431
private static JsonNode ConvertYamlStringToJsonNode(string yamlInput)
405432
{
406433
var yamlDocument = new YamlStream();

0 commit comments

Comments
 (0)