Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validation option for portable installer type in archives #5237

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Feb 21, 2025

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 Trenly requested a review from a team as a code owner February 21, 2025 19:05
@yao-msft
Copy link
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
Copy link
Contributor Author

Trenly commented Feb 21, 2025

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
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
Copy link
Contributor Author

Trenly commented Feb 22, 2025

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

image

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