Make sure package has the expected hash when browsing and installing #3817
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3817 +/- ##
==========================================
+ Coverage 35.50% 35.51% +0.01%
==========================================
Files 711 711
Lines 31236 31244 +8
Branches 4670 4670
==========================================
+ Hits 11091 11097 +6
- Misses 18991 18993 +2
Partials 1154 1154
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -619,6 +619,10 @@ def download_manifest(dst): | |||
|
|
|||
| pkg = cls._from_path(local_pkg_manifest) | |||
| pkg._origin = PackageRevInfo(str(registry.base), name, top_hash) | |||
|
|
|||
| if pkg.top_hash != top_hash: | |||
There was a problem hiding this comment.
OK so doing this check in _browse also gets install (in PR title)? And works for subpackage install as well? Since the user can install any subpackage and we have the manifest at that point I think we only need to check the installed object hashes and not the tophash.
There was a problem hiding this comment.
Yeah, browse and install both run this code.
This only checks the tophash for now, not the object hashes. Subpackages are not special - they just download a subset of objects. But we need to check the tophash regardless, before we do anything else.
| @@ -619,6 +619,10 @@ def download_manifest(dst): | |||
|
|
|||
| pkg = cls._from_path(local_pkg_manifest) | |||
| pkg._origin = PackageRevInfo(str(registry.base), name, top_hash) | |||
|
|
|||
| if pkg.top_hash != top_hash: | |||
There was a problem hiding this comment.
that might be OK, but are you sure we should put manifest to the cache if this check fails?
There was a problem hiding this comment.
It's safe to do that right now, since it checks the hash regardless of where the package is being loaded from - local, S3, or cache.
I could change it to only check hash when loading from S3, and before putting it into the cache. Then we'll trust any local package (not sure we should?), and anything in the cache (probably fine, except for packages that were already cached before this PR).
|
@dimaryaz I think you can close this one per discussion to focus on ensuring that client and S3 hash the same bits. |
No description provided.