Add type package validation - #890
Mingzhe Jiang (jiangmingzhe) wants to merge 14 commits into
Conversation
| /// validation is now implemented (see <c>BCPVT030</c>–<c>BCPVT033</c>); this code is retained for | ||
| /// API stability but is no longer emitted. | ||
| /// </summary> | ||
| public const string ArchiveValidationNotImplemented = "BCPVT001"; |
There was a problem hiding this comment.
The codes seem to be duplicated in this file. Can we make it so they are only in one place (just like in Bicep)?
| } | ||
|
|
||
| // 2. Package-relative path. | ||
| var cmp = string.CompareOrdinal(x.Path ?? string.Empty, y.Path ?? string.Empty); |
There was a problem hiding this comment.
According to https://learn.microsoft.com/en-us/dotnet/api/system.string.compareordinal?view=net-10.0, the string.CompareOrdinal() method accepts nulls already.
| public sealed class TypeValidationDiagnostic | ||
| { | ||
| private static readonly IReadOnlyList<TypeValidationDiagnosticRelatedLocation> NoRelatedLocations = | ||
| new TypeValidationDiagnosticRelatedLocation[0]; |
There was a problem hiding this comment.
nit: We can just do NoRelatedLocations = [];
| return ParseTar(tarBytes); | ||
| } | ||
|
|
||
| private static byte[] Decompress(byte[] archiveBytes) |
There was a problem hiding this comment.
This one was found by AI.
Decompress copies the entire gzip payload into an unbounded MemoryStream, then duplicates it with ToArray(). Archive entries are subsequently copied again.
A small malicious types.tgz could expand to gigabytes and terminate the validating process with OutOfMemoryException. This is especially concerning because package validation is intended to handle third-party packages safely.
Recommended fix: Enforce configurable limits for:
- Total decompressed bytes
- Individual member size
- Member count
- Input stream size
Exceeding a limit should produce the existing fatal container diagnostic rather than attempting further allocation.
| string refValue = refNode.StringValue ?? string.Empty; | ||
|
|
||
| // Validate the $ref string syntax | ||
| if (!ReferencePath.TryParse(refValue, out string packagePath, out int index)) |
There was a problem hiding this comment.
I think references can only appear in index.json, but not type files. Should we validate that?
| TypePackageValidationOptions options) | ||
| { | ||
| if (documents == null) { throw new ArgumentNullException(nameof(documents)); } | ||
| if (options == null) { throw new ArgumentNullException(nameof(options)); } |
There was a problem hiding this comment.
Do we have nullable enabled? If yes, the null checks can be removed. I think AI generated code can sometimes be too defensive.
| { | ||
| if (packageRelativePath == null) { throw new ArgumentNullException(nameof(packageRelativePath)); } | ||
|
|
||
| string key = DirectoryPackageFileSystem.NormalizeSeparators(packageRelativePath); |
There was a problem hiding this comment.
Could we canonicalize package-relative paths before using them as cache keys? NormalizeSeparators leaves forms such as ./types.json distinct from types.json, while the package file systems resolve both to the same physical file. This can load and validate one file multiple times under different keys. It also causes GetReachedFilePaths() to contain ./types.json while EnumerateFiles() returns types.json, resulting in a false BCPVT033 when unreachable-file validation is enabled. Could we use one canonical representation—removing . segments and redundant separators—for cache keys, reached paths, and file enumeration (or reject non-canonical references consistently)?
| public TypeKindDescriptor(string discriminator, TypeFieldDescriptor[] fields) | ||
| { | ||
| Discriminator = discriminator; | ||
| Fields = fields ?? new TypeFieldDescriptor[0]; |
There was a problem hiding this comment.
Can simplify collection initialization: fields ?? [];
| public string? Path { get; } | ||
|
|
||
| /// <summary>JSON pointer into the related file, when available.</summary> | ||
| public string? JsonPointer { get; } |
There was a problem hiding this comment.
Would it be beneficial to use JsonPointer from JsonPointer.Net? If we decide to use it we'd need to pin the version to 6.0 to avoid the EULA cert issue.
|
Just want to make sure I understand the intended behavior when a package is produced by a newer To be clear, I don't think this fires often. The bulk of package churn (new resources, API versions, properties on a resource body) is instance data within the existing kinds and would pass fine. It's specifically the rarer meta-schema evolution (like the So mostly a question: is that newer-package-older-validator case a scenario this library intends to handle, and if so what's the expected behavior? |
Adds a new library for validating serialized Bicep type packages before publishing or consumption.
Functionality
index.json, archive files, and archive streams.Structure
Packaging/: input and archive handlingStructural/: JSON shape validationGraph/: reference and target validationSemantic/: value constraintsPolicy/: compatibility rulesHygiene/: unreachable/unexpected filesDiagnostics/: diagnostic models and orderingAlso adds the projects to the solution and introduces a suite of unit-test and golden-sample samples.