Yaml manifest schemaheader validation for V.1.10.0 and above #5126

Madhusudhan-MSFT · 2025-01-12T04:02:42Z

Enable schema header validation for YAML manifests from version 1.10.0 onwards.

YAML Manifest Validation Changes:

Introduce functions to search, parse, and validate schema headers in YAML manifests.
Add/Update structs and enums to support schema header validation.
Add new error messages and validation options, including treating schema header validation errors as warnings.

Schema Header Validation Includes:

Validate presence of Manifest Schema Header.
Ensure Schema Header format correctness.
Verify Schema Header URL Pattern accuracy.
Confirm Schema Header Manifest Type matches ManifestType Property.
Check Schema Header Manifest Version matches ManifestVersion Property.

WinGet CLI Validation Command Changes:

Treat schema header validation errors as warnings for 'winget validate --manifest <>' command
- Introduced a new option SchemaHeaderValidationAsWarning to the validateOption object and set it to true.
- This change treats schema header validation issues as warnings instead of errors, making the validation process more lenient.

[NOTE:]

SchemaHeaderValidation errors should be treated as warnings for the winget CLI validate command.
SchemaHeaderValidation errors should be treated as errors for wingetsvc community manifests.

[Test Coverage:]

Added new test data files to validate various schema header errors in YAML manifests, including
- invalid headers,
- type mismatches,
- version mismatches,
- missing headers, and
- URL pattern mismatches.
Added test coverage for WinGet Utils Interop binary and WinGet CLI Validated command

[How Validated:]

Compiled the AppInstaller end-to-end solution incorporating the changes.
Performed the YAML manifest validation tests, WinGet Utils tests, and WinGet CLI Validate command tests, ensuring all tests passed.
Locally validated the winget CLI validation command with various schema header errors, ensuring they are treated as warnings and that no warnings are reported for a valid manifest.

[Test Results:]

YAML Manifest Validation Test Results:

WinGet Utils Interop Test Results:

Validate Command Test Results:

I have signed the Contributor License Agreement.
This pull request is related to an issue.

Microsoft Reviewers: Open in CodeFlow

1. Introduce functions to search, parse, and validate schema headers in YAML manifests. 2. Add/Update structs and enums to support schema header validation. 3. Add new error messages and validation options, including treating schema header validation errors as warnings. Schema Header Validation Includes: 1. Validate presence of Manifest Schema Header. 2. Ensure Schema Header format correctness. 3. Verify Schema Header URL Pattern accuracy. 4. Confirm Schema Header Manifest Type matches ManifestType Property. 5. Check Schema Header Manifest Version matches ManifestVersion Property.

…e --manifest <<path>>' command 1. Introduced a new option `SchemaHeaderValidationAsWarning` to the `validateOption` object and set it to `true`. 2. This change treats schema header validation issues as warnings instead of errors, making the validation process more lenient. [NOTE:] 1. SchemaHeaderValidation errors should be treated as warnings for the winget CLI validate command. 2. SchemaHeaderValidation errors should be treated as errors for wingetsvc community manifests.

1. Added new test data files to validate various schema header errors in YAML manifests, including a. invalid headers, b. type mismatches, c. version mismatches, d. missing headers, and e. URL pattern mismatches. 2. Updated project files to include these test data files. 3. Added a new test case in `YamlManifest.cpp` to validate the schema headers of the new test data files and existing v.1.10 schema test files.

…ommand and WinGetUtils Interop binary 1. Added new test manifests with schema headers to validate various scenarios. 2. Updated `ValidateCommand.cs` to include new test cases for extended characters and schema headers. 3. Enhanced `WinGetUtilManifest.cs` with new test cases for schema headers and added a helper method for validation. Also, made minor formatting corrections.

The error message in the test case `TEST_CASE("ManifestV1_10_SchemaHeaderValidations", "[ManifestValidation]")` has been updated to provide a clearer and more specific description. The new message clarifies the mismatch issue between the manifest version in the schema header and the `ManifestVersion` property value in the manifest.

src/AppInstallerCommonCore/Manifest/ManifestSchemaValidation.cpp

The changes correct a typo in the file path for a test manifest file in two different test files. Specifically, the file name `TestWarningManifestV1_10-SchemaHeadeVersionMismatch.yaml` is corrected to `TestWarningManifestV1_10-SchemaHeaderVersionMismatch.yaml` in both `ValidateCommand.cs` and `WinGetUtilManifest.cs`.

- Updated regex pattern for schema URL matching, - Renamed and revised validation functions, and introduced new helper functions for improved error handling and validation logic. - These changes ensure accurate validation of schema header URLs against schema definition files.

Updated the URL in the comment for the YAML language server schema to correct the casing of "defaultLocale" to "defaultlocale". This ensures the schema URL is correctly referenced.

Madhusudhan-MSFT · 2025-01-12T09:21:28Z

src/AppInstallerCommonCore/Manifest/ManifestSchemaValidation.cpp

+                if (std::equal(schemaId.begin(), schemaId.end(), schemaHeaderUrlString.begin(), schemaHeaderUrlString.end(),
+                    [](char a, char b) { return tolower(a) == tolower(b); }))
+                {
+                    return true;
+                }


Do we need to perform a case-insensitive comparison, or should we enforce case sensitivity (all lowercase)?

I think it is fine to be case-sensitive here as I don't think anyone will be typing the value in by hand.

src/AppInstallerCommonCore/Manifest/YamlParser.cpp

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

…0GoodManifestAndVerifyContents test failure Corrected the URL in the comment for the YAML language server schema by changing "defaultlocale" to "defaultLocale" to ensure proper casing.

JohnMcPMS · 2025-01-13T19:25:17Z

src/AppInstallerCLICore/Commands/ValidateCommand.cpp

@@ -46,6 +46,7 @@ namespace AppInstaller::CLI
            {
                ManifestValidateOption validateOption;
                validateOption.FullValidation = true;
+                validateOption.SchemaHeaderValidationAsWarning = true;


Why do we need a new option here? This seems like it should be under the same umbrella as every other warning.

This option allows the YAML parser to treat SchemaHeader violations as either errors or warnings. For the winget CLI validate command, these violations should be warnings, but for API calls through WinGetUtils, they should be treated as errors ( for WinGetSVC use case)

Additionally, Warnings do not block validation pipeline in WinGetSVC, but errors do. Therefore, for WinGetSVC calls, violations need to be treated as errors to be marked as failures in the validation pipeline for schema header violations.

JohnMcPMS · 2025-01-13T19:26:15Z

src/AppInstallerCLIE2ETests/ValidateCommand.cs

@@ -31,7 +31,7 @@ public void ValidateManifest()
        [Test]
        public void ValidateManifestWithExtendedCharacter()
        {
-            var result = TestCommon.RunAICLICommand("validate", TestCommon.GetTestDataFile("Manifests\\T�stExeInstaller.yaml"));
+            var result = TestCommon.RunAICLICommand("validate", TestCommon.GetTestDataFile("Manifests\\TëstExeInstaller.yaml"));


This seems like a mistake that changed the encoding of the file.

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

src/AppInstallerCommonCore/Manifest/ManifestSchemaValidation.cpp

JohnMcPMS · 2025-01-13T21:14:49Z

src/AppInstallerCommonCore/Manifest/ManifestSchemaValidation.cpp

+            schemaHeader.SchemaHeaderString.clear();
+
+            // Search for the schema header string in the comments before the root node.
+            while (currentLine < rootNodeBeginsAtLine && std::getline(*yamlInputStream, line))


This code assumes that the input stream actually contains 8-bit characters. The YAML parser (wrapper) supports detecting the encoding on the input stream and handling it properly. The Parser type is the one that ends up containing the full string text of the stream, while Document contains probably the level that a schema should apply to (we don't really attempt to do anything past the first document in a stream).

Since the YAML parser ignores comments, I would suggest moving the code that detects the schema into the wrapper (probably Document Parser::Load()) and making it a property of a Document.

JohnMcPMS · 2025-01-13T21:17:24Z

src/AppInstallerCommonCore/Manifest/ManifestSchemaValidation.cpp

+                if (std::equal(schemaId.begin(), schemaId.end(), schemaHeaderUrlString.begin(), schemaHeaderUrlString.end(),
+                    [](char a, char b) { return tolower(a) == tolower(b); }))
+                {
+                    return true;
+                }


I think it is fine to be case-sensitive here as I don't think anyone will be typing the value in by hand.

…r messages 1. Updated error messages in multiple files to simplify instructions for verifying schema headers. 2. Removed detailed YAML node verification instructions. 3. Modified function signatures in ManifestSchemaValidation.cpp to remove const qualifiers.

Madhusudhan-MSFT added 5 commits January 11, 2025 18:14

This comment has been minimized.

Sign in to view

Trenly reviewed Jan 12, 2025

View reviewed changes

src/AppInstallerCommonCore/Manifest/ManifestSchemaValidation.cpp Outdated Show resolved Hide resolved

Madhusudhan-MSFT added 3 commits January 11, 2025 23:05

Fix casing in YAML language server schema URL comment

bd4fd24

Updated the URL in the comment for the YAML language server schema to correct the casing of "defaultLocale" to "defaultlocale". This ensures the schema URL is correctly referenced.

Madhusudhan-MSFT commented Jan 12, 2025

View reviewed changes

src/AppInstallerCommonCore/Manifest/YamlParser.cpp Show resolved Hide resolved

Madhusudhan-MSFT commented Jan 12, 2025

View reviewed changes

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp Outdated Show resolved Hide resolved

Fix casing in YAML language server schema URL to address ValidateV1_1…

4b18e9e

…0GoodManifestAndVerifyContents test failure Corrected the URL in the comment for the YAML language server schema by changing "defaultlocale" to "defaultLocale" to ensure proper casing.

Madhusudhan-MSFT marked this pull request as ready for review January 12, 2025 19:31

Madhusudhan-MSFT requested a review from a team as a code owner January 12, 2025 19:31

Madhusudhan-MSFT marked this pull request as draft January 12, 2025 19:31

Madhusudhan-MSFT marked this pull request as ready for review January 13, 2025 01:20

Madhusudhan-MSFT requested review from JohnMcPMS and msftrubengu January 13, 2025 17:46

JohnMcPMS reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yaml manifest schemaheader validation for V.1.10.0 and above #5126

Yaml manifest schemaheader validation for V.1.10.0 and above #5126

Madhusudhan-MSFT commented Jan 12, 2025 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

Madhusudhan-MSFT Jan 12, 2025

JohnMcPMS Jan 13, 2025

JohnMcPMS Jan 13, 2025

Madhusudhan-MSFT Jan 13, 2025 •

edited

Loading

JohnMcPMS Jan 13, 2025

JohnMcPMS Jan 13, 2025

JohnMcPMS Jan 13, 2025

Yaml manifest schemaheader validation for V.1.10.0 and above #5126

Are you sure you want to change the base?

Yaml manifest schemaheader validation for V.1.10.0 and above #5126

Conversation

Madhusudhan-MSFT commented Jan 12, 2025 • edited Loading

Enable schema header validation for YAML manifests from version 1.10.0 onwards.

Microsoft Reviewers: Open in CodeFlow

This comment has been minimized.

This comment has been minimized.

Madhusudhan-MSFT Jan 12, 2025

Choose a reason for hiding this comment

JohnMcPMS Jan 13, 2025

Choose a reason for hiding this comment

JohnMcPMS Jan 13, 2025

Choose a reason for hiding this comment

Madhusudhan-MSFT Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jan 13, 2025

Choose a reason for hiding this comment

JohnMcPMS Jan 13, 2025

Choose a reason for hiding this comment

JohnMcPMS Jan 13, 2025

Choose a reason for hiding this comment

Madhusudhan-MSFT commented Jan 12, 2025 •

edited

Loading

Madhusudhan-MSFT Jan 13, 2025 •

edited

Loading