From 15013b757946b2b252e4b747c005152f9ee3eb83 Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Fri, 30 Sep 2022 07:38:17 +0000 Subject: [PATCH] Adjust messages, move check, fixed time comparison --- lib/src/command/cache_add.dart | 2 +- lib/src/command/dependency_services.dart | 12 +--- lib/src/entrypoint.dart | 18 ++---- lib/src/global_packages.dart | 6 +- lib/src/null_safety_analysis.dart | 6 +- lib/src/solver/report.dart | 46 +++++++++++++-- lib/src/solver/result.dart | 75 ++++++------------------ lib/src/source/cached.dart | 6 +- lib/src/source/git.dart | 11 ++-- lib/src/source/hosted.dart | 47 ++++++++++----- lib/src/system_cache.dart | 11 ++-- lib/src/utils.dart | 14 ++++- test/content_hash_test.dart | 4 +- 13 files changed, 126 insertions(+), 132 deletions(-) diff --git a/lib/src/command/cache_add.dart b/lib/src/command/cache_add.dart index 3c79d1d5c..ea8a8c162 100644 --- a/lib/src/command/cache_add.dart +++ b/lib/src/command/cache_add.dart @@ -77,7 +77,7 @@ class CacheAddCommand extends PubCommand { } // Download it. - await cache.downloadPackage(id, allowOutdatedHashChecks: true); + await cache.downloadPackage(id); } if (argResults['all']) { diff --git a/lib/src/command/dependency_services.dart b/lib/src/command/dependency_services.dart index d49c3dbce..6a40d37de 100644 --- a/lib/src/command/dependency_services.dart +++ b/lib/src/command/dependency_services.dart @@ -484,17 +484,7 @@ class DependencyServicesApplyCommand extends PubCommand { // This happens when we resolved a package from a legacy server // not providing archive_sha256. As a side-effect of downloading // the package we compute and store the sha256. - await cache.downloadPackage( - package, - allowOutdatedHashChecks: false, - ); - listedId = PackageId( - listedId.name, - listedId.version, - description.withSha256( - cache.hosted.sha256FromCache(listedId, cache), - ), - ); + listedId = await cache.downloadPackage(package); } updatedPackages.add(listedId); } else { diff --git a/lib/src/entrypoint.dart b/lib/src/entrypoint.dart index 9f1e4bf49..b0f007745 100644 --- a/lib/src/entrypoint.dart +++ b/lib/src/entrypoint.dart @@ -298,10 +298,6 @@ class Entrypoint { /// shown --- in case of failure, a reproduction command is shown. /// /// Updates [lockFile] and [packageRoot] accordingly. - /// - /// If [enforceLockfile] is true no changes to the current lockfile are - /// allowed. Instead the existing lockfile is loaded, verified against - /// pubspec.yaml and all dependencies downloaded. Future acquireDependencies( SolveType type, { Iterable? unlock, @@ -360,17 +356,11 @@ class Entrypoint { // We have to download files also with --dry-run to ensure we know the // archive hashes for downloaded files. - final newLockFile = await result.downloadCachedPackages(cache, - allowOutdatedHashChecks: true); + final newLockFile = await result.downloadCachedPackages(cache); final report = SolveReport( - type, - root, - lockFile, - newLockFile, - result.availableVersions, - cache, - ); + type, root, lockFile, newLockFile, result.availableVersions, cache, + dryRun: dryRun); if (!onlyReportSuccessOrFailure) { await report.show(); } @@ -383,7 +373,7 @@ class Entrypoint { if (onlyReportSuccessOrFailure) { log.message('Got dependencies$suffix.'); } else { - await report.summarize(dryRun: dryRun); + await report.summarize(); } if (!dryRun) { diff --git a/lib/src/global_packages.dart b/lib/src/global_packages.dart index fbec3c17d..038fe2337 100644 --- a/lib/src/global_packages.dart +++ b/lib/src/global_packages.dart @@ -224,10 +224,7 @@ class GlobalPackages { // We want the entrypoint to be rooted at 'dep' not the dummy-package. result.packages.removeWhere((id) => id.name == 'pub global activate'); - final lockFile = await result.downloadCachedPackages( - cache, - allowOutdatedHashChecks: true, - ); + final lockFile = await result.downloadCachedPackages(cache); final sameVersions = originalLockFile != null && originalLockFile.samePackageIds(lockFile); @@ -247,6 +244,7 @@ To recompile executables, first run `$topLevelProgram pub global deactivate $nam lockFile, result.availableVersions, cache, + dryRun: false, ).show(); } diff --git a/lib/src/null_safety_analysis.dart b/lib/src/null_safety_analysis.dart index 450098450..fe89b559d 100644 --- a/lib/src/null_safety_analysis.dart +++ b/lib/src/null_safety_analysis.dart @@ -171,11 +171,7 @@ class NullSafetyAnalysis { if (source is CachedSource) { // TODO(sigurdm): Consider using withDependencyType here. - await source.downloadToSystemCache( - dependencyId, - _systemCache, - allowOutdatedHashChecks: true, - ); + await source.downloadToSystemCache(dependencyId, _systemCache); } final libDir = diff --git a/lib/src/solver/report.dart b/lib/src/solver/report.dart index 4307bd0b8..258b3f2df 100644 --- a/lib/src/solver/report.dart +++ b/lib/src/solver/report.dart @@ -10,6 +10,7 @@ import '../lock_file.dart'; import '../log.dart' as log; import '../package.dart'; import '../package_name.dart'; +import '../source/hosted.dart'; import '../source/root.dart'; import '../system_cache.dart'; import '../utils.dart'; @@ -27,6 +28,7 @@ class SolveReport { final LockFile _previousLockFile; final LockFile _newLockFile; final SystemCache _cache; + final bool _dryRun; /// The available versions of all selected packages from their source. /// @@ -44,14 +46,50 @@ class SolveReport { this._previousLockFile, this._newLockFile, this._availableVersions, - this._cache, - ); + this._cache, { + required bool dryRun, + }) : _dryRun = dryRun; /// Displays a report of the results of the version resolution in /// [_newLockFile] relative to the [_previousLockFile] file. Future show() async { await _reportChanges(); await _reportOverrides(); + _checkContentHashesMatchOldLockfile(); + } + + _checkContentHashesMatchOldLockfile() { + for (final newId in _newLockFile.packages.values) { + var newDescription = newId.description; + // Use the cached content-hashes after downloading to ensure that + // content-hashes from legacy servers gets used. + if (newDescription is ResolvedHostedDescription) { + final cachedHash = newDescription.sha256; + assert(cachedHash != null); + final oldId = _previousLockFile.packages[newId.name]; + if (oldId != null && + cachedHash != null && + oldId.version == newId.version) { + final oldDecription = oldId.description; + if (oldDecription is ResolvedHostedDescription && + oldDecription.description == newDescription.description) { + final oldHash = oldDecription.sha256; + if (oldHash != null && !fixedTimeBytesEquals(cachedHash, oldHash)) { + log.warning(''' +The content of ${newId.name}-${newId.version} from ${newDescription.description.url} doesn't match the pubspec.lock. + +This indicates one of: +* The content has changed on the server since you created the pubspec.lock. +* The pubspec.lock has been corrupted. +${_dryRun ? '' : '\nThe pubspec.lock has been updated.'} + +See $contentHashesDocumentationUrl for more information. +'''); + } + } + } + } + } } /// Displays a one-line message summarizing what changes were made (or would @@ -64,7 +102,7 @@ class SolveReport { /// If [type] is `SolveType.UPGRADE` it also shows the number of packages that /// are not at the latest available version and the number of outdated /// packages. - Future summarize({bool dryRun = false}) async { + Future summarize() async { // Count how many dependencies actually changed. var dependencies = _newLockFile.packages.keys.toSet(); dependencies.addAll(_previousLockFile.packages.keys); @@ -90,7 +128,7 @@ class SolveReport { } } - if (dryRun) { + if (_dryRun) { if (numChanged == 0) { log.message('No dependencies would change$suffix.'); } else if (numChanged == 1) { diff --git a/lib/src/solver/result.dart b/lib/src/solver/result.dart index 2ac3939ab..1ba97d093 100644 --- a/lib/src/solver/result.dart +++ b/lib/src/solver/result.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -import 'dart:typed_data'; - import 'package:collection/collection.dart'; import 'package:pub_semver/pub_semver.dart'; @@ -18,7 +16,6 @@ import '../pubspec.dart'; import '../source/cached.dart'; import '../source/hosted.dart'; import '../system_cache.dart'; -import '../utils.dart'; /// The result of a successful version resolution. class SolveResult { @@ -64,20 +61,23 @@ class SolveResult { /// If there is a mismatch between the previous content-hash from pubspec.lock /// and the new one a warning will be printed but the new one will be /// returned. - Future downloadCachedPackages( - SystemCache cache, { - required bool allowOutdatedHashChecks, - }) async { - await Future.wait(packages.map((id) async { - if (id.source is CachedSource) { - await withDependencyType(_root.dependencyType(id.name), () async { - await cache.downloadPackage( - id, - allowOutdatedHashChecks: allowOutdatedHashChecks, - ); - }); - } - })); + Future downloadCachedPackages(SystemCache cache) async { + final resolvedPackageIds = await Future.wait( + packages.map((id) async { + if (id.source is CachedSource) { + return await withDependencyType(_root.dependencyType(id.name), + () async { + return await cache.downloadPackage( + id, + ); + }); + } + return id; + }), + ); + + // Invariant: the content-hashes in PUB_CACHE matches those provided by the + // server. // Don't factor in overridden dependencies' SDK constraints, because we'll // accept those packages even if their constraints don't match. @@ -94,46 +94,7 @@ class SolveResult { }); } return LockFile( - packages.map((id) { - var description = id.description; - // Use the cached content-hashes after downloading to ensure that - // content-hashes from legacy servers gets used. - if (description is ResolvedHostedDescription) { - Uint8List? cachedHash = - description.description.source.sha256FromCache(id, cache); - if (cachedHash == null) { - // This should not happen. - throw StateError( - 'Archive for ${id.name}-${id.version} has no content hash.'); - } - final originalEntry = _previousLockFile.packages[id.name]; - if (originalEntry != null && originalEntry.version == id.version) { - final originalDescription = originalEntry.description; - if (originalDescription is ResolvedHostedDescription) { - final Uint8List? originalHash = originalDescription.sha256; - if (originalHash != null) { - if (!bytesEquals(cachedHash, originalHash)) { - log.warning(''' -The content of ${id.name}-${id.version} on the server doesn't match what was locked in your pubspec.lock. - -This might indicate that: -* The content has changed on the server since you created the pubspec.lock. -* The pubspec.lock has been corrupted. - -Your pubspec.lock has been updated according to what is on the server. - -See $contentHashesDocumentationUrl for more information. -'''); - } - } - } - } - - return PackageId( - id.name, id.version, description.withSha256(cachedHash)); - } - return id; - }).toList(), + resolvedPackageIds, sdkConstraints: sdkConstraints, mainDependencies: MapKeySet(_root.dependencies), devDependencies: MapKeySet(_root.devDependencies), diff --git a/lib/src/source/cached.dart b/lib/src/source/cached.dart index af85b829d..54630c73d 100644 --- a/lib/src/source/cached.dart +++ b/lib/src/source/cached.dart @@ -55,11 +55,7 @@ abstract class CachedSource extends Source { dirExists(getDirectoryInCache(id, cache)); /// Downloads the package identified by [id] to the system cache. - Future downloadToSystemCache( - PackageId id, - SystemCache cache, { - required bool allowOutdatedHashChecks, - }); + Future downloadToSystemCache(PackageId id, SystemCache cache); /// Returns the [Package]s that have been downloaded to the system cache. List getCachedPackages(SystemCache cache); diff --git a/lib/src/source/git.dart b/lib/src/source/git.dart index 791c80a74..24b1294aa 100644 --- a/lib/src/source/git.dart +++ b/lib/src/source/git.dart @@ -298,11 +298,10 @@ class GitSource extends CachedSource { /// itself; each of the commit-specific directories are clones of a directory /// in `cache/`. @override - Future downloadToSystemCache( + Future downloadToSystemCache( PackageId id, - SystemCache cache, { - required bool allowOutdatedHashChecks, - }) async { + SystemCache cache, + ) async { return await _pool.withResource(() async { final ref = id.toRef(); final description = ref.description; @@ -322,8 +321,7 @@ class GitSource extends CachedSource { var revisionCachePath = _revisionCachePath(id, cache); final path = description.path; - return await _revisionCacheClones.putIfAbsent(revisionCachePath, - () async { + await _revisionCacheClones.putIfAbsent(revisionCachePath, () async { if (!entryExists(revisionCachePath)) { await _clone(_repoCachePath(ref, cache), revisionCachePath); await _checkOut(revisionCachePath, resolvedRef); @@ -332,6 +330,7 @@ class GitSource extends CachedSource { _updatePackageList(revisionCachePath, path); } }); + return id; }); } diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index d248ce70e..bdd605b23 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -698,11 +698,8 @@ class HostedSource extends CachedSource { /// `pub get` with a filled cache to be a fast case that doesn't require any /// new version-listings. @override - Future downloadToSystemCache( - PackageId id, - SystemCache cache, { - required bool allowOutdatedHashChecks, - }) async { + Future downloadToSystemCache( + PackageId id, SystemCache cache) async { final packageDir = getDirectoryInCache(id, cache); // Use the content-hash from the version-info to compare with what we @@ -714,13 +711,18 @@ class HostedSource extends CachedSource { // We allow the version-listing to be a few days outdated in order for `pub // get` with an existing working resolution and everything in cache to be // fast. - final versionInfo = await _versionInfo(id.toRef(), id.version, cache, - maxAge: allowOutdatedHashChecks ? Duration(days: 3) : null); + final versionInfo = await _versionInfo( + id.toRef(), + id.version, + cache, + maxAge: Duration(days: 3), + ); final expectedContentHash = versionInfo?.archiveSha256 ?? // Handling of legacy server - we use the hash from the id (typically // from the lockfile) to compare to the existing download. (id.description as ResolvedHostedDescription).sha256; + Uint8List? contentHash; if (!fileExists(hashPath(id, cache))) { if (dirExists(packageDir) && !cache.isOffline) { log.fine( @@ -728,24 +730,36 @@ class HostedSource extends CachedSource { deleteEntry(packageDir); } } else if (expectedContentHash == null) { + // Can happen with a legacy server combined with a legacy lock file. log.fine( 'Content-hash of ${id.name}-${id.version} not known from resolution.'); } else { - if (!bytesEquals(sha256FromCache(id, cache), expectedContentHash)) { + final hashFromCache = sha256FromCache(id, cache); + if (!fixedTimeBytesEquals(hashFromCache, expectedContentHash)) { log.warning( 'Cached version of ${id.name}-${id.version} has wrong hash - redownloading.'); if (cache.isOffline) { - fail('Cannot redownload while offline - try again online.'); + fail('Cannot redownload while offline. Try again without --offline.'); } deleteEntry(packageDir); + } else { + contentHash = hashFromCache; } } - if (!dirExists(packageDir)) { + if (dirExists(packageDir)) { + contentHash ??= sha256FromCache(id, cache); + } else { if (cache.isOffline) { - throw StateError('Cannot download packages when offline.'); + fail( + 'Missing package ${id.name}-${id.version}. Try again without --offline.'); } - await _download(id, packageDir, cache); + contentHash = await _download(id, packageDir, cache); } + return PackageId( + id.name, + id.version, + (id.description as ResolvedHostedDescription).withSha256(contentHash), + ); } /// Determines if the package identified by [id] is already downloaded to the @@ -953,7 +967,9 @@ class HostedSource extends CachedSource { /// If there is no archive_url, try to fetch it from /// `$server/packages/$package/versions/$version.tar.gz` where server comes /// from `id.description`. - Future _download( + /// + /// Returns the content-hash of the downloaded archive. + Future _download( PackageId id, String destPath, SystemCache cache, @@ -976,6 +992,7 @@ class HostedSource extends CachedSource { versions.firstWhereOrNull((i) => i.version == id.version); final packageName = id.name; final version = id.version; + late Uint8List contentHash; if (versionInfo == null) { throw PackageNotFoundException( 'Package $packageName has no version $version'); @@ -986,7 +1003,7 @@ class HostedSource extends CachedSource { log.fine('Downloading ${log.bold(id.name)} ${id.version}...'); // Download and extract the archive to a temp directory. - await withTempDir((tempDirForArchive) async { + return await withTempDir((tempDirForArchive) async { var archivePath = p.join(tempDirForArchive, '$packageName-$version.tar.gz'); var response = await withAuthenticatedClient( @@ -1023,6 +1040,7 @@ See $contentHashesDocumentationUrl. path, hexEncode(actualHash.bytes), ); + contentHash = Uint8List.fromList(actualHash.bytes); } // We download the archive to disk instead of streaming it directly into @@ -1056,6 +1074,7 @@ See $contentHashesDocumentationUrl. // another pub process has installed the same package version while we // downloaded. tryRenameDir(tempDir, destPath); + return contentHash; }); } diff --git a/lib/src/system_cache.dart b/lib/src/system_cache.dart index 4b3ee43da..11a10a174 100644 --- a/lib/src/system_cache.dart +++ b/lib/src/system_cache.dart @@ -221,16 +221,15 @@ class SystemCache { /// response if present instead of probing the server. Not probing allows for /// `pub get` with a filled cache to be a fast case that doesn't require any /// new version-listings. - Future downloadPackage( - PackageId id, { - required bool allowOutdatedHashChecks, - }) async { + /// + /// Returns [id] with an updated [ResolvedDescription], this can be different + /// if the content-hash changed while downloading. + Future downloadPackage(PackageId id) async { final source = id.source; assert(source is CachedSource); - await (source as CachedSource).downloadToSystemCache( + return await (source as CachedSource).downloadToSystemCache( id, this, - allowOutdatedHashChecks: allowOutdatedHashChecks, ); } diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 81ef9cfbf..12309ecce 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -9,7 +9,6 @@ import 'dart:io'; import 'dart:math' as math; import 'dart:typed_data'; -import 'package:collection/collection.dart'; import 'package:convert/convert.dart'; import 'package:crypto/crypto.dart' as crypto; import 'package:pub_semver/pub_semver.dart'; @@ -646,6 +645,15 @@ Map mapMap( }; } -bool bytesEquals(List? a, List? b) { - return const ListEquality().equals(a, b); +/// Compares two lists. If the lists have equal length this comparison will +/// iterate all elements, thus taking a fixed amount of time making timing +/// attacks harder. +bool fixedTimeBytesEquals(List? a, List? b) { + if (a == null || b == null) return a == b; + if (a.length != b.length) return false; + bool e = true; + for (var i = 0; i < a.length; i++) { + e &= a[i] == b[i]; + } + return e; } diff --git a/test/content_hash_test.dart b/test/content_hash_test.dart index 5c2d5f7c0..186d12584 100644 --- a/test/content_hash_test.dart +++ b/test/content_hash_test.dart @@ -79,7 +79,7 @@ Future main() async { warning: allOf( contains('Cached version of foo-1.0.0 has wrong hash - redownloading.'), contains( - 'Content of foo-1.0.0 has changed compared to what was locked your pubspec.lock.'), + 'The content of foo-1.0.0 from ${globalServer.url} doesn\'t match the pubspec.lock.'), ), exitCode: exit_codes.SUCCESS, ); @@ -106,7 +106,7 @@ Future main() async { await pubGet( warning: contains( - 'Content of foo-1.0.0 has changed compared to what was locked your pubspec.lock.', + 'The content of foo-1.0.0 from ${globalServer.url} doesn\'t match the pubspec.lock.', ), exitCode: exit_codes.SUCCESS, );