Skip to content

Add validation option for portable installer type in archives - #5237

Merged
yao-msft merged 6 commits into
microsoft:masterfrom
Trenly:ValidatePortableEXE
Mar 12, 2025
Merged

Add validation option for portable installer type in archives#5237
yao-msft merged 6 commits into
microsoft:masterfrom
Trenly:ValidatePortableEXE

Conversation

@Trenly

@Trenly Kaleb Luedtke (Trenly) commented Feb 21, 2025

Copy link
Copy Markdown
Contributor

Description

For the portable installer type, the winget-pkgs repository has a policy of not allowing scripts as installers. This includes .bat, .ps1, and .cmd files.

In addition to the policy at winget-pkgs, the client itself is not designed to handle portable files that are not .exe types. Issues have been filed asking for WinGet to support these in the future, but it currently does not -

This PR adds a validation option which validates the RelativeFilePath within a zip file is always a .exe, but can be extended to include additional formats as the requests above are addressed. This was added as a validation option (similar to the Schema Header Validation) as private repositories may want to not have the file type restrictions that winget-pkgs does - as described in these issues -

This should allow for the validation pipeline at winget-pkgs to automatically reject manifests that contain NestedInstallerFiles that are not supported, while allowing maintainers of private repositories to suppress the restriction by turning the validation option to false.


Microsoft Reviewers: Open in CodeFlow

@Trenly
Kaleb Luedtke (Trenly) requested a review from a team as a code owner February 21, 2025 19:05
@yao-msft

Copy link
Copy Markdown
Contributor

Basically same comment as the other pr, I think fullValidation should just work instead of a new ValidateOption. (Just to be clear, any new ValidateOption require a winget service side code change to enable the option).
Regarding warning vs error, if there's no existing manifests that violate this rule, error seems ok. Otherwise, maybe use warning to prevent unexpected failures in winget service.

@Trenly

Kaleb Luedtke (Trenly) commented Feb 21, 2025

Copy link
Copy Markdown
Contributor Author

Basically same comment as the other pr, I think fullValidation should just work instead of a new ValidateOption. (Just to be clear, any new ValidateOption require a winget service side code change to enable the option). Regarding warning vs error, if there's no existing manifests that violate this rule, error seems ok. Otherwise, maybe use warning to prevent unexpected failures in winget service.

Wouldn't ValidateOption be required in order for private repositories to be able to disable the validation of this, similar to how the schema header validation is?

I'll do a run through of winget-pkgs locally using the dev build and verify that there are no manifests that would be impacted by this being an error

@yao-msft

Copy link
Copy Markdown
Contributor

Basically same comment as the other pr, I think fullValidation should just work instead of a new ValidateOption. (Just to be clear, any new ValidateOption require a winget service side code change to enable the option). Regarding warning vs error, if there's no existing manifests that violate this rule, error seems ok. Otherwise, maybe use warning to prevent unexpected failures in winget service.

Wouldn't ValidateOption be required in order for private repositories to be able to disable the validation of this, similar to how the schema header validation is?

I'll do a run through of winget-pkgs locally using the dev build and verify that there are no manifests that would be impacted by this being an error

For all manifest read operations, fullValidation is false. So private repositories should be fine. ValidateOptions is for granular control of winget service to choose which validation are performed during different steps. Currently I don't see a need to have a special switch just for this specific validation.

@Trenly

Kaleb Luedtke (Trenly) commented Feb 22, 2025

Copy link
Copy Markdown
Contributor Author

I'll submit corrections to these 9 manifests - Interestingly, not all of the errors are due to the changes in this PR

image

Comment thread src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters Outdated
Comment thread src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp Outdated
Comment thread src/AppInstallerCommonCore/Public/winget/ManifestValidation.h Outdated
Comment thread src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp Outdated
@yao-msft

yao-msft commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

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

@yao-msft

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

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

Comment thread src/AppInstallerCommonCore/Public/winget/ManifestValidation.h Outdated
Comment thread src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp Outdated
@yao-msft

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants