Skip to content

Validate URLs map to unique hashes - #5233

Merged
yao-msft merged 9 commits into
microsoft:masterfrom
Trenly:ValidateUrlHashes
Feb 21, 2025
Merged

Validate URLs map to unique hashes#5233
yao-msft merged 9 commits into
microsoft:masterfrom
Trenly:ValidateUrlHashes

Conversation

@Trenly

@Trenly Kaleb Luedtke (Trenly) commented Feb 20, 2025 •
edited by microsoft-github-policy-service Bot
Loading

Copy link
Copy Markdown
Contributor

Description:

This PR adds validation for URL to SHA256 mappings. It is presumed that a single URL should only return a single SHA256, so if a manifest has different SHA256 for the same URL, this will now throw a validation error. This will also help reduce potential load on the Validation Pipelines at winget-pkgs as the InstallerSha256 mismatch will be caught during manifest validation and the validation will exit before it proceeds to installer validation.

Additionally, as noted in the initial issue, it would be unlikely (but possible) for two different URLS to point to the same SHA256. Validation was added for this case, but will only throw a validation warning. It is also necessary for this to be a warning so that this change does not become a breaking change.

{D367385B-6718-46F4-818D-4198F75D36C9}

Microsoft Reviewers: Open in CodeFlow

@Trenly
Kaleb Luedtke (Trenly) requested a review from a team as a code owner February 20, 2025 16:29
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Feb 20, 2025
@denelon

Demitrius Nelon (denelon) commented Feb 20, 2025

Copy link
Copy Markdown
Collaborator

Nice!

mdanish-kh
Muhammad Danish (mdanish-kh) reviewed Feb 20, 2025
View reviewed changes
Comment thread src/AppInstallerCLITests/YamlManifest.cpp Outdated
JohnMcPMS
JohnMcPMS reviewed Feb 20, 2025
View reviewed changes
Comment thread src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp Outdated
@Trenly @JohnMcPMS
Update src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp
66ed060 
Co-authored-by: JohnMcPMS <johnmcp@microsoft.com>
@JohnMcPMS

JohnMcPMS commented Feb 20, 2025

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

azure-pipelines Bot commented Feb 20, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@Trenly

Kaleb Luedtke (Trenly) commented Feb 20, 2025

Copy link
Copy Markdown
Contributor Author

I hope I wasn’t the one who broke it

Failed ExportTestPackageWithVersion [8 s]
  Error Message:
     Expected: True
  But was:  False
  Stack Trace:
     at AppInstallerCLIE2ETests.ConfigureExportCommand.ExportTestPackageWithVersion() in D:\a\1\s\src\AppInstallerCLIE2ETests\ConfigureExportCommand.cs:line 94
  Standard Output Messages:
 Starting command run. AICLI path: wingetdev.exe Command: configure export Parameters: --package-id AppInstallerTest.TestPackageExport --include-versions -o D:\a\1\Build.x64release\AppInstallerCLIE2ETests\WorkDirectory\23pgi1ni.pnp\exported.yml Timeout: 60000
 Command run completed with exit code: 0
 Command run output. Output:
 Unexpected error while loading settings. Please verify your settings by running the 'settings' command.
 
    - 
                                                                                                                         
 
    - 
                                                                                                                         
 
    - Initializing configuration system

yao-msft
yao-msft reviewed Feb 20, 2025
View reviewed changes
Comment thread src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp Outdated
Comment thread src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp Outdated
Comment thread src/AppInstallerCLITests/AppInstallerCLITests.vcxproj
@yao-msft

yao-msft commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Feb 20, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@Trenly

Kaleb Luedtke (Trenly) commented Feb 20, 2025

Copy link
Copy Markdown
Contributor Author

Aha. . . Looks like I only checked the tests for the bad manifests, and didn't check the tests for the good ones

@yao-msft

yao-msft commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Feb 20, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft
yao-msft merged commit 761af8d into microsoft:master Feb 21, 2025
@Trenly
Kaleb Luedtke (Trenly) deleted the ValidateUrlHashes branch February 21, 2025 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JohnMcPMS JohnMcPMS JohnMcPMS left review comments
@yao-msft yao-msft yao-msft approved these changes
+1 more reviewer
@mdanish-kh Muhammad Danish (mdanish-kh) mdanish-kh left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Issue-Bug It either shouldn't be doing this or needs an investigation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SHA-256 collisions between different installer URLs, for different architectures, are not flagged as a manifest bug?

5 participants

@Trenly @denelon @JohnMcPMS @yao-msft @mdanish-kh