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.

Sign up for GitHub

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

Jump to bottom

x/mod/zip: verify file list without creating zip #36058

Closed
jayconrod opened this issue Dec 9, 2019 · 4 comments
Closed

x/mod/zip: verify file list without creating zip #36058

jayconrod opened this issue Dec 9, 2019 · 4 comments
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@jayconrod
Copy link
Contributor

golang.org/x/mod/zip provides a Create function that takes a list of files ([]File). On success, it creates a zip file with some of those files. It ignores files in submodules and vendor directories and reports errors for files with invalid names or types (symbolic links). On failure, it reports an error on the first file that caused a problem.

This is not the most useful API for clients creating zip files or verifying that a set of files can be used to create a zip file. Some improvements are possible without making incompatible changes to the API:

  • We should provide a Verify function that accepts []File and reports whether each file in that list will be included, ignored, or rejected. Verify would not create a zip file, and it would not read files (just stat to get sizes and types), so it should be faster than Create.
  • When Create encounters an error due to the []File argument, it should return a structured error with information about each file. This should be similar to what Verify returns (perhaps whatever Verify returns can satisfy the error interface).
  • Unzip should also return a structured error so that issues with multiple zip entries can be reported.
  • We may also want a VerifyZip function which checks a zip file for errors without extracting it. Currently, Unzip always needs a directory to write files.
  • We should document more clearly when files are included, ignored, or rejected.

cc @dmitshur
@jayconrod jayconrod added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest Issues asking for a new feature that does not need a proposal. Tools This label describes issues relating to any tools in the x/tools repository. labels Dec 9, 2019
@jayconrod jayconrod added this to the Unreleased milestone Dec 9, 2019
@bcmills
Copy link
Contributor

bcmills commented Dec 9, 2019

Some improvements are possible without making incompatible changes to the API

Incompatible changes would be fine too, I think: the latest tagged version of that repo is still v0.1.0 and the zip package is not included in that tag, so even with our more conservative approach to breaking changes, most users should be unaffected.

dmitshur added a commit to shurcooL/home that referenced this issue Dec 16, 2019
@dmitshur
cmd/githook/pre-receive, internal/code: add pre-receive git hook
854a424 
This pre-receive git hook validates commits pushed to master branches
to catch problems that would cause bad module versions to be produced.

The golang.org/x/mod/zip package is used to verify that the module zip
includes only the expected files and no others, and doesn't violate any
of the restrictions placed on Go module zips.

Improve consistency of parameter order across the codebase. Prefer to
place module.Version first, source of data (repository and commit ID)
second, because module version is higher level information.

Use canonical "Www-Authenticate" header case to save an allocation.

Fixes #35.
Updates golang/go#36058.
@dmitshur
Copy link
Contributor

dmitshur commented Mar 7, 2020

Issue #37397 is related.

It's worth considering that depending on the exact goal of a user, they may want to reproduce that vendor bug in order to ensure consistent behavior with cmd/go, or they want to resolve that bug and avoid stripping some valid packages unneccesarily.

A good high-level way of asking that question is whether the user goal is to stay within the rules of the module proxy protocol, or additionally within the cmd/go implementation restrictions.

@jayconrod
Copy link
Contributor Author

A good high-level way of asking that question is whether the user goal is to stay within the rules of the module proxy protocol, or additionally within the cmd/go implementation restrictions.

That's a good way to phrase it. Perhaps zip.Create could be a lower-level API that zips all files unless they would cause an error in zip.Unzip (e.g., too big, invalid file name). Or maybe we would introduce zip.CreateRaw to avoid an incompatible change. zip.CreateFromDir would be the convenience function that ignores vendor directories.

Verify would still be useful for clients to understand whether a file should be included.

@jayconrod jayconrod mentioned this issue May 18, 2020
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/235597 mentions this issue: zip: add CheckFiles, CheckDir, and CheckZip

@golang golang locked and limited conversation to collaborators Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@jayconrod @dmitshur @bcmills @gopherbot