From 1fefabec149185c0c86fcd69cb68916b54a3c5eb Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 1 Sep 2022 16:41:22 -0700 Subject: [PATCH 01/28] Add CRC32C checksum methods from sigurdm's branch --- lib/src/crc32c.dart | 91 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 lib/src/crc32c.dart diff --git a/lib/src/crc32c.dart b/lib/src/crc32c.dart new file mode 100644 index 000000000..3bce94824 --- /dev/null +++ b/lib/src/crc32c.dart @@ -0,0 +1,91 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// 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. + +/// Computes a crc32c checksum. +class Crc32c { + int _current = mask; + static const mask = 0xFFFFFFFF; + + // Algorithm based on https://en.wikipedia.org/wiki/Cyclic_redundancy_check + void update(List data) { + for (var i = 0; i < data.length; i++) { + final lookupIndex = (_current ^ data[i]) & 0xff; + _current = (_current >> 8) ^ _crcTable[lookupIndex]; + } + } + + int finalize() { + // Finalize the CRC-32 value by inverting all the bits + return _current ^ mask & mask; + } +} + +// Generated by ./pycrc.py --algorithm=table-driven --model=crc-32c --generate=c +// See: https://pycrc.org/ +const _crcTable = [ + 0x00000000, 0xf26b8303, 0xe13b70f7, 0x1350f3f4, // + 0xc79a971f, 0x35f1141c, 0x26a1e7e8, 0xd4ca64eb, + 0x8ad958cf, 0x78b2dbcc, 0x6be22838, 0x9989ab3b, + 0x4d43cfd0, 0xbf284cd3, 0xac78bf27, 0x5e133c24, + 0x105ec76f, 0xe235446c, 0xf165b798, 0x030e349b, + 0xd7c45070, 0x25afd373, 0x36ff2087, 0xc494a384, + 0x9a879fa0, 0x68ec1ca3, 0x7bbcef57, 0x89d76c54, + 0x5d1d08bf, 0xaf768bbc, 0xbc267848, 0x4e4dfb4b, + 0x20bd8ede, 0xd2d60ddd, 0xc186fe29, 0x33ed7d2a, + 0xe72719c1, 0x154c9ac2, 0x061c6936, 0xf477ea35, + 0xaa64d611, 0x580f5512, 0x4b5fa6e6, 0xb93425e5, + 0x6dfe410e, 0x9f95c20d, 0x8cc531f9, 0x7eaeb2fa, + 0x30e349b1, 0xc288cab2, 0xd1d83946, 0x23b3ba45, + 0xf779deae, 0x05125dad, 0x1642ae59, 0xe4292d5a, + 0xba3a117e, 0x4851927d, 0x5b016189, 0xa96ae28a, + 0x7da08661, 0x8fcb0562, 0x9c9bf696, 0x6ef07595, + 0x417b1dbc, 0xb3109ebf, 0xa0406d4b, 0x522bee48, + 0x86e18aa3, 0x748a09a0, 0x67dafa54, 0x95b17957, + 0xcba24573, 0x39c9c670, 0x2a993584, 0xd8f2b687, + 0x0c38d26c, 0xfe53516f, 0xed03a29b, 0x1f682198, + 0x5125dad3, 0xa34e59d0, 0xb01eaa24, 0x42752927, + 0x96bf4dcc, 0x64d4cecf, 0x77843d3b, 0x85efbe38, + 0xdbfc821c, 0x2997011f, 0x3ac7f2eb, 0xc8ac71e8, + 0x1c661503, 0xee0d9600, 0xfd5d65f4, 0x0f36e6f7, + 0x61c69362, 0x93ad1061, 0x80fde395, 0x72966096, + 0xa65c047d, 0x5437877e, 0x4767748a, 0xb50cf789, + 0xeb1fcbad, 0x197448ae, 0x0a24bb5a, 0xf84f3859, + 0x2c855cb2, 0xdeeedfb1, 0xcdbe2c45, 0x3fd5af46, + 0x7198540d, 0x83f3d70e, 0x90a324fa, 0x62c8a7f9, + 0xb602c312, 0x44694011, 0x5739b3e5, 0xa55230e6, + 0xfb410cc2, 0x092a8fc1, 0x1a7a7c35, 0xe811ff36, + 0x3cdb9bdd, 0xceb018de, 0xdde0eb2a, 0x2f8b6829, + 0x82f63b78, 0x709db87b, 0x63cd4b8f, 0x91a6c88c, + 0x456cac67, 0xb7072f64, 0xa457dc90, 0x563c5f93, + 0x082f63b7, 0xfa44e0b4, 0xe9141340, 0x1b7f9043, + 0xcfb5f4a8, 0x3dde77ab, 0x2e8e845f, 0xdce5075c, + 0x92a8fc17, 0x60c37f14, 0x73938ce0, 0x81f80fe3, + 0x55326b08, 0xa759e80b, 0xb4091bff, 0x466298fc, + 0x1871a4d8, 0xea1a27db, 0xf94ad42f, 0x0b21572c, + 0xdfeb33c7, 0x2d80b0c4, 0x3ed04330, 0xccbbc033, + 0xa24bb5a6, 0x502036a5, 0x4370c551, 0xb11b4652, + 0x65d122b9, 0x97baa1ba, 0x84ea524e, 0x7681d14d, + 0x2892ed69, 0xdaf96e6a, 0xc9a99d9e, 0x3bc21e9d, + 0xef087a76, 0x1d63f975, 0x0e330a81, 0xfc588982, + 0xb21572c9, 0x407ef1ca, 0x532e023e, 0xa145813d, + 0x758fe5d6, 0x87e466d5, 0x94b49521, 0x66df1622, + 0x38cc2a06, 0xcaa7a905, 0xd9f75af1, 0x2b9cd9f2, + 0xff56bd19, 0x0d3d3e1a, 0x1e6dcdee, 0xec064eed, + 0xc38d26c4, 0x31e6a5c7, 0x22b65633, 0xd0ddd530, + 0x0417b1db, 0xf67c32d8, 0xe52cc12c, 0x1747422f, + 0x49547e0b, 0xbb3ffd08, 0xa86f0efc, 0x5a048dff, + 0x8ecee914, 0x7ca56a17, 0x6ff599e3, 0x9d9e1ae0, + 0xd3d3e1ab, 0x21b862a8, 0x32e8915c, 0xc083125f, + 0x144976b4, 0xe622f5b7, 0xf5720643, 0x07198540, + 0x590ab964, 0xab613a67, 0xb831c993, 0x4a5a4a90, + 0x9e902e7b, 0x6cfbad78, 0x7fab5e8c, 0x8dc0dd8f, + 0xe330a81a, 0x115b2b19, 0x020bd8ed, 0xf0605bee, + 0x24aa3f05, 0xd6c1bc06, 0xc5914ff2, 0x37faccf1, + 0x69e9f0d5, 0x9b8273d6, 0x88d28022, 0x7ab90321, + 0xae7367ca, 0x5c18e4c9, 0x4f48173d, 0xbd23943e, + 0xf36e6f75, 0x0105ec76, 0x12551f82, 0xe03e9c81, + 0x34f4f86a, 0xc69f7b69, 0xd5cf889d, 0x27a40b9e, + 0x79b737ba, 0x8bdcb4b9, 0x988c474d, 0x6ae7c44e, + 0xbe2da0a5, 0x4c4623a6, 0x5f16d052, 0xad7d5351 +]; From 8c3bffc1911175fd64ba76f384d7129fc815555a Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 1 Sep 2022 16:42:19 -0700 Subject: [PATCH 02/28] Add CRC32C checksum validation for packages served from GCS --- lib/src/source/hosted.dart | 114 +++++++++++++++++++++++++++++++++++-- pubspec.yaml | 1 + 2 files changed, 111 insertions(+), 4 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index e7dc87063..fae2166e4 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -5,6 +5,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io' as io; +import 'dart:typed_data'; import 'package:collection/collection.dart' show maxBy, IterableNullableExtension; @@ -12,8 +13,10 @@ import 'package:http/http.dart' as http; import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; import 'package:stack_trace/stack_trace.dart'; +import 'package:stream_transform/stream_transform.dart'; import '../authentication/client.dart'; +import '../crc32c.dart'; import '../exceptions.dart'; import '../http.dart'; import '../io.dart'; @@ -860,16 +863,27 @@ class HostedSource extends CachedSource { var archivePath = p.join(tempDirForArchive, '$packageName-$version.tar.gz'); var response = await withAuthenticatedClient( - cache, - Uri.parse(description.url), - (client) => client.send(http.Request('GET', url))); + cache, Uri.parse(description.url), (client) { + var req = http.Request('GET', url); + // TODO: How can we properly check if a request is GCS bound, so only + // those requests get this header? (for checksum validation) + req.headers['Cache-Control'] = 'no-transform'; + return client.send(req); + }); + + Stream> responseStream = response.stream; + + if (response.isFromGCS) { + // Taps into response stream without starting consumption. + responseStream = response.streamWithGCSChecksumValidationTap(); + } // We download the archive to disk instead of streaming it directly into // the tar unpacking. This simplifies stream handling. // Package:tar cancels the stream when it reaches end-of-archive, and // cancelling a http stream makes it not reusable. // There are ways around this, and we might revisit this later. - await createFileFromStream(response.stream, archivePath); + await createFileFromStream(responseStream, archivePath); var tempDir = cache.createTempDir(); await extractTarGz(readBinaryFileAsSream(archivePath), tempDir); @@ -1100,3 +1114,95 @@ class _RefAndCache { @override bool operator ==(Object other) => other is _RefAndCache && other.ref == ref; } + +extension GCSChecksumValidation on http.StreamedResponse { + static const checksumsHeaderName = 'x-goog-hash'; + + bool get isFromGCS { + // TODO: Does this suffice as a heuristic? + return headers.containsKey(checksumsHeaderName); + } + + /// Adds a checksum validation "tap" to the response stream and returns a + /// wrapped Stream object, which should be used to consume the incoming data. + /// + /// As chunks are received, a CRC32C checksum is updated. + /// Once the download is completed, the final checksum is compared with + /// the one present in the `x-goog-hash` response header from GCS. + /// + /// Throws [PackageIntegrityException] if anything is wrong with the checksum + /// or if there is a checksum mismatch. + Stream> streamWithGCSChecksumValidationTap() { + if (gcsCrc32c == null) { + throw PackageIntegrityException( + 'Package served from GCS is missing CRC32C checksum', request?.url); + } + + final checksumComputer = Crc32c(); + + return stream.tap(checksumComputer.update, onDone: () { + var computedCrc32c = checksumComputer.finalize(); + + log.io( + 'Computed checksum ($computedCrc32c) for package with GCS checksum of ($gcsCrc32c).'); + + if (gcsCrc32c != computedCrc32c) { + throw PackageIntegrityException( + 'Package served from GCS has a CRC32C checksum mismatch; Computed checksum ($computedCrc32c) != GCS checksum ($gcsCrc32c)', + request?.url); + } + }); + } + + /// Parses GCS response headers and returns the package's CRC32C checksum. + int? get gcsCrc32c { + String? gcsChecksums = headers[checksumsHeaderName]; + if (gcsChecksums == null) return null; + + // In most cases, GCS provides both MD5 and CRC32C checksums in its response + // headers. It uses the header name "x-goog-hash" for these values. It has + // been documented and observed that GCS will send multiple response headers + // with the same "x-goog-hash" token as the key. + // https://cloud.google.com/storage/docs/xml-api/reference-headers#xgooghash + + // Additionally, when the Dart http package encounters multiple response + // headers with the same key, it concatenates their values with a comma + // before inserting a single item with that key and concatenated value into + // its response "headers" Map. There is a TODO to refactor the + // response class to use a HttpHeaders object instead of a Map, but the + // person it was assigned to, nweiz@, has long since left the Dart team. + // https://github.com/dart-lang/http/issues/24 + // https://github.com/dart-lang/http/blob/06649afbb5847dbb0293816ba8348766b116e419/pkgs/http/lib/src/base_response.dart#L29 + + // Therefore, we need to use this roundabout method for parsing the + // CRC32C checksum. + + final parts = gcsChecksums.split(','); + for (final part in parts) { + if (part.startsWith('crc32c=')) { + final undecoded = part.substring(7); + final rawBytes = base64.decode(undecoded); + return ByteData.view(rawBytes.buffer).getUint32(0); + } + } + + return null; + } +} + +/// Package checksum related exception. +class PackageIntegrityException implements Exception { + const PackageIntegrityException(this.message, this.url); + + final String message; + final Uri? url; + + @override + String toString() { + var temp = message; + if (url != null) { + temp += ': $url'; + } + return temp; + } +} diff --git a/pubspec.yaml b/pubspec.yaml index 4f52414d1..547875f59 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -24,6 +24,7 @@ dependencies: shelf: ^1.1.1 source_span: ^1.8.1 stack_trace: ^1.10.0 + stream_transform: ^2.0.0 usage: ^4.0.2 yaml: ^3.1.0 yaml_edit: ^2.0.0 From 7755d07bbf4c736aa12567c298e2fb8f6d42149c Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 2 Sep 2022 14:41:13 -0700 Subject: [PATCH 03/28] Replace stream_transform dependency with compact implementation --- lib/src/source/hosted.dart | 7 ++++--- lib/src/utils.dart | 11 +++++++++++ pubspec.yaml | 1 - 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index fae2166e4..db262ae90 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -13,7 +13,6 @@ import 'package:http/http.dart' as http; import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; import 'package:stack_trace/stack_trace.dart'; -import 'package:stream_transform/stream_transform.dart'; import '../authentication/client.dart'; import '../crc32c.dart'; @@ -1140,7 +1139,9 @@ extension GCSChecksumValidation on http.StreamedResponse { final checksumComputer = Crc32c(); - return stream.tap(checksumComputer.update, onDone: () { + return stream + .transform(onDataTransformer(checksumComputer.update)) + .transform(onDoneTransformer(() { var computedCrc32c = checksumComputer.finalize(); log.io( @@ -1151,7 +1152,7 @@ extension GCSChecksumValidation on http.StreamedResponse { 'Package served from GCS has a CRC32C checksum mismatch; Computed checksum ($computedCrc32c) != GCS checksum ($gcsCrc32c)', request?.url); } - }); + })); } /// Parses GCS response headers and returns the package's CRC32C checksum. diff --git a/lib/src/utils.dart b/lib/src/utils.dart index e4451dbdf..185290494 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -148,6 +148,17 @@ Future> waitAndPrintErrors(Iterable> futures) { }); } +/// Returns a [StreamTransformer] that will call [onData] for each data +/// event of the stream. +/// +/// The stream will be passed through unchanged. +StreamTransformer onDataTransformer(void Function(T data) onData) { + return StreamTransformer.fromHandlers(handleData: (data, sink) { + onData(data); + sink.add(data); + }); +} + /// Returns a [StreamTransformer] that will call [onDone] when the stream /// completes. /// diff --git a/pubspec.yaml b/pubspec.yaml index 547875f59..4f52414d1 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -24,7 +24,6 @@ dependencies: shelf: ^1.1.1 source_span: ^1.8.1 stack_trace: ^1.10.0 - stream_transform: ^2.0.0 usage: ^4.0.2 yaml: ^3.1.0 yaml_edit: ^2.0.0 From 7d84f89e332f47f957ca60f9c67d587fa528db55 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 6 Sep 2022 09:24:42 -0700 Subject: [PATCH 04/28] Update lib/src/source/hosted.dart Co-authored-by: Sigurd Meldgaard --- lib/src/source/hosted.dart | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index db262ae90..e4c00e684 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -1166,13 +1166,11 @@ extension GCSChecksumValidation on http.StreamedResponse { // with the same "x-goog-hash" token as the key. // https://cloud.google.com/storage/docs/xml-api/reference-headers#xgooghash - // Additionally, when the Dart http package encounters multiple response + // Additionally, when the Dart http client encounters multiple response // headers with the same key, it concatenates their values with a comma // before inserting a single item with that key and concatenated value into - // its response "headers" Map. There is a TODO to refactor the - // response class to use a HttpHeaders object instead of a Map, but the - // person it was assigned to, nweiz@, has long since left the Dart team. - // https://github.com/dart-lang/http/issues/24 + // its response "headers" Map. + // See https://github.com/dart-lang/http/issues/24 // https://github.com/dart-lang/http/blob/06649afbb5847dbb0293816ba8348766b116e419/pkgs/http/lib/src/base_response.dart#L29 // Therefore, we need to use this roundabout method for parsing the From 999d8042fa58747b4240270cabcedc2105168d43 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 6 Sep 2022 09:25:25 -0700 Subject: [PATCH 05/28] Update lib/src/source/hosted.dart Co-authored-by: Sigurd Meldgaard --- lib/src/source/hosted.dart | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index e4c00e684..5b773ec3d 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -1173,8 +1173,6 @@ extension GCSChecksumValidation on http.StreamedResponse { // See https://github.com/dart-lang/http/issues/24 // https://github.com/dart-lang/http/blob/06649afbb5847dbb0293816ba8348766b116e419/pkgs/http/lib/src/base_response.dart#L29 - // Therefore, we need to use this roundabout method for parsing the - // CRC32C checksum. final parts = gcsChecksums.split(','); for (final part in parts) { From 3c6eed11f7a5f45ba20df16989b6832a6d3b5f1b Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 6 Sep 2022 09:25:57 -0700 Subject: [PATCH 06/28] Update lib/src/source/hosted.dart Co-authored-by: Sigurd Meldgaard --- lib/src/source/hosted.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 5b773ec3d..fbb255cf7 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -1177,7 +1177,7 @@ extension GCSChecksumValidation on http.StreamedResponse { final parts = gcsChecksums.split(','); for (final part in parts) { if (part.startsWith('crc32c=')) { - final undecoded = part.substring(7); + final undecoded = part.substring('crc32c='.length); final rawBytes = base64.decode(undecoded); return ByteData.view(rawBytes.buffer).getUint32(0); } From 25fa20e4d14a2d26a7b34dad607c77d521d8fc78 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 12 Sep 2022 14:06:01 -0700 Subject: [PATCH 07/28] Recompose functionality without extension --- lib/src/source/hosted.dart | 148 ++++++++++++++++++------------------- 1 file changed, 72 insertions(+), 76 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index fbb255cf7..2d443fc60 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -861,21 +861,12 @@ class HostedSource extends CachedSource { await withTempDir((tempDirForArchive) async { var archivePath = p.join(tempDirForArchive, '$packageName-$version.tar.gz'); + var request = _createArchiveRequest(url); var response = await withAuthenticatedClient( - cache, Uri.parse(description.url), (client) { - var req = http.Request('GET', url); - // TODO: How can we properly check if a request is GCS bound, so only - // those requests get this header? (for checksum validation) - req.headers['Cache-Control'] = 'no-transform'; - return client.send(req); - }); - - Stream> responseStream = response.stream; - - if (response.isFromGCS) { - // Taps into response stream without starting consumption. - responseStream = response.streamWithGCSChecksumValidationTap(); - } + cache, Uri.parse(description.url), (client) => client.send(request)); + var responseStream = _hasChecksumHeader(response.headers) + ? _responseStreamWithChecksumValidationTap(response) + : response.stream; // We download the archive to disk instead of streaming it directly into // the tar unpacking. This simplifies stream handling. @@ -896,6 +887,12 @@ class HostedSource extends CachedSource { }); } + http.Request _createArchiveRequest(Uri url) { + var request = http.Request('GET', url); + request.headers[io.HttpHeaders.cacheControlHeader] = 'no-transform'; + return request; + } + /// When an error occurs trying to read something about [package] from [hostedUrl], /// this tries to translate into a more user friendly error message. /// @@ -1114,77 +1111,76 @@ class _RefAndCache { bool operator ==(Object other) => other is _RefAndCache && other.ref == ref; } -extension GCSChecksumValidation on http.StreamedResponse { - static const checksumsHeaderName = 'x-goog-hash'; +const checksumHeaderName = 'x-goog-hash'; - bool get isFromGCS { - // TODO: Does this suffice as a heuristic? - return headers.containsKey(checksumsHeaderName); - } - - /// Adds a checksum validation "tap" to the response stream and returns a - /// wrapped Stream object, which should be used to consume the incoming data. - /// - /// As chunks are received, a CRC32C checksum is updated. - /// Once the download is completed, the final checksum is compared with - /// the one present in the `x-goog-hash` response header from GCS. - /// - /// Throws [PackageIntegrityException] if anything is wrong with the checksum - /// or if there is a checksum mismatch. - Stream> streamWithGCSChecksumValidationTap() { - if (gcsCrc32c == null) { - throw PackageIntegrityException( - 'Package served from GCS is missing CRC32C checksum', request?.url); - } +bool _hasChecksumHeader(Map headers) { + return headers.containsKey(checksumHeaderName); +} - final checksumComputer = Crc32c(); +/// Adds a checksum validation "tap" to the response stream and returns a +/// wrapped Stream object, which should be used to consume the incoming data. +/// +/// As chunks are received, a CRC32C checksum is updated. +/// Once the download is completed, the final checksum is compared with +/// the one present in the `x-goog-hash` response header. +/// +/// Throws [PackageIntegrityException] if anything is wrong with the checksum +/// or if there is a checksum mismatch. +Stream> _responseStreamWithChecksumValidationTap( + http.StreamedResponse response) { + final hostedChecksum = _parseChecksum(response.headers); + if (hostedChecksum == null) { + throw PackageIntegrityException( + 'Package response headers are missing CRC32C checksum', + response.request?.url); + } - return stream - .transform(onDataTransformer(checksumComputer.update)) - .transform(onDoneTransformer(() { - var computedCrc32c = checksumComputer.finalize(); + final checksumComputer = Crc32c(); - log.io( - 'Computed checksum ($computedCrc32c) for package with GCS checksum of ($gcsCrc32c).'); + return response.stream + .transform(onDataTransformer(checksumComputer.update)) + .transform(onDoneTransformer(() { + var computedCrc32c = checksumComputer.finalize(); - if (gcsCrc32c != computedCrc32c) { - throw PackageIntegrityException( - 'Package served from GCS has a CRC32C checksum mismatch; Computed checksum ($computedCrc32c) != GCS checksum ($gcsCrc32c)', - request?.url); - } - })); - } + log.fine( + 'Computed checksum ($computedCrc32c) for package with hosted checksum of ($hostedChecksum).'); - /// Parses GCS response headers and returns the package's CRC32C checksum. - int? get gcsCrc32c { - String? gcsChecksums = headers[checksumsHeaderName]; - if (gcsChecksums == null) return null; - - // In most cases, GCS provides both MD5 and CRC32C checksums in its response - // headers. It uses the header name "x-goog-hash" for these values. It has - // been documented and observed that GCS will send multiple response headers - // with the same "x-goog-hash" token as the key. - // https://cloud.google.com/storage/docs/xml-api/reference-headers#xgooghash - - // Additionally, when the Dart http client encounters multiple response - // headers with the same key, it concatenates their values with a comma - // before inserting a single item with that key and concatenated value into - // its response "headers" Map. - // See https://github.com/dart-lang/http/issues/24 - // https://github.com/dart-lang/http/blob/06649afbb5847dbb0293816ba8348766b116e419/pkgs/http/lib/src/base_response.dart#L29 - - - final parts = gcsChecksums.split(','); - for (final part in parts) { - if (part.startsWith('crc32c=')) { - final undecoded = part.substring('crc32c='.length); - final rawBytes = base64.decode(undecoded); - return ByteData.view(rawBytes.buffer).getUint32(0); - } + if (hostedChecksum != computedCrc32c) { + throw PackageIntegrityException( + 'Package fetched from host has a CRC32C checksum mismatch; Computed checksum ($computedCrc32c) != Hosted checksum ($hostedChecksum)', + response.request?.url); } + })); +} - return null; +/// Parses response headers and returns the package's CRC32C checksum. +/// +/// In most cases, GCS provides both MD5 and CRC32C checksums in its response +/// headers. It uses the header name "x-goog-hash" for these values. It has +/// been documented and observed that GCS will send multiple response headers +/// with the same "x-goog-hash" token as the key. +/// https://cloud.google.com/storage/docs/xml-api/reference-headers#xgooghash +/// +/// Additionally, when the Dart http client encounters multiple response +/// headers with the same key, it concatenates their values with a comma +/// before inserting a single item with that key and concatenated value into +/// its response "headers" Map. +/// See https://github.com/dart-lang/http/issues/24 +/// https://github.com/dart-lang/http/blob/06649afbb5847dbb0293816ba8348766b116e419/pkgs/http/lib/src/base_response.dart#L29 +int? _parseChecksum(Map responseHeaders) { + String? checksums = responseHeaders[checksumHeaderName]; + if (checksums == null) return null; + + final parts = checksums.split(','); + for (final part in parts) { + if (part.startsWith('crc32c=')) { + final undecoded = part.substring('crc32c='.length); + final rawBytes = base64.decode(undecoded); + return ByteData.view(rawBytes.buffer).getUint32(0); + } } + + return null; } /// Package checksum related exception. From 415481bf861c479c2ab549f27ac6336089a479d3 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 12 Sep 2022 14:38:27 -0700 Subject: [PATCH 08/28] Add cache-control request header to golden test log --- ...e is written with --verbose and on unexpected exceptions.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt index 14ea76f59..3741c4072 100644 --- a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt +++ b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt @@ -40,6 +40,7 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] IO : Get package from http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz. [E] IO : Created temp directory $DIR [E] IO : HTTP GET http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz +[E] | cache-control: no-transform [E] | X-Pub-OS: $OS [E] | X-Pub-Command: get [E] | X-Pub-Session-ID: $ID @@ -178,6 +179,7 @@ IO : Get package from http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz MSG : Downloading foo 1.0.0... IO : Created temp directory $DIR IO : HTTP GET http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz + | cache-control: no-transform | X-Pub-OS: $OS | X-Pub-Command: get | X-Pub-Session-ID: $ID From dfabdac094aa09115d30b65a63a4bd8d7fc9a6e7 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 15 Sep 2022 17:02:43 -0700 Subject: [PATCH 09/28] Add basic checksum validation tests and ability to pass arbitrary response headers to archives in test server --- lib/src/crc32c.dart | 27 +++++++++++++++++ lib/src/source/hosted.dart | 29 +++++++----------- lib/src/utils.dart | 13 +++++++++ test/get/hosted/get_test.dart | 36 +++++++++++++++++++++++ test/package_server.dart | 55 ++++++++++++++++++++++++++++++++--- 5 files changed, 138 insertions(+), 22 deletions(-) diff --git a/lib/src/crc32c.dart b/lib/src/crc32c.dart index 3bce94824..1539c0296 100644 --- a/lib/src/crc32c.dart +++ b/lib/src/crc32c.dart @@ -2,6 +2,8 @@ // 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 'utils.dart'; + /// Computes a crc32c checksum. class Crc32c { int _current = mask; @@ -19,6 +21,31 @@ class Crc32c { // Finalize the CRC-32 value by inverting all the bits return _current ^ mask & mask; } + + /// Transforms `stream` with a tap that calculates a CRC32C checksum, which + /// is accessible through the `handleDone` method once the stream is finished. + static Stream> computeByTappingStream(Stream> stream, + {required void Function(int crc32c) handleDone}) { + final checksumComputer = Crc32c(); + + return stream + .transform(onDataTransformer(checksumComputer.update)) + .transform(onDoneTransformer(() { + handleDone(checksumComputer.finalize()); + })); + } + + /// Consumes the entirety of `stream` and returns the CRC32C checksum of its + /// data once the stream is finished. + static Future computeByConsumingStream(Stream> stream) async { + final checksumComputer = Crc32c(); + + await for (final data in stream) { + checksumComputer.update(data); + } + + return checksumComputer.finalize(); + } } // Generated by ./pycrc.py --algorithm=table-driven --model=crc-32c --generate=c diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 2d443fc60..fb987aec4 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -5,7 +5,6 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io' as io; -import 'dart:typed_data'; import 'package:collection/collection.dart' show maxBy, IterableNullableExtension; @@ -1122,35 +1121,29 @@ bool _hasChecksumHeader(Map headers) { /// /// As chunks are received, a CRC32C checksum is updated. /// Once the download is completed, the final checksum is compared with -/// the one present in the `x-goog-hash` response header. +/// the one present in the checksum response header. /// /// Throws [PackageIntegrityException] if anything is wrong with the checksum /// or if there is a checksum mismatch. Stream> _responseStreamWithChecksumValidationTap( http.StreamedResponse response) { - final hostedChecksum = _parseChecksum(response.headers); - if (hostedChecksum == null) { + final hostedCrc32c = _parseCrc32c(response.headers); + if (hostedCrc32c == null) { throw PackageIntegrityException( 'Package response headers are missing CRC32C checksum', response.request?.url); } - final checksumComputer = Crc32c(); - - return response.stream - .transform(onDataTransformer(checksumComputer.update)) - .transform(onDoneTransformer(() { - var computedCrc32c = checksumComputer.finalize(); - + return Crc32c.computeByTappingStream(response.stream, handleDone: (crc32c) { log.fine( - 'Computed checksum ($computedCrc32c) for package with hosted checksum of ($hostedChecksum).'); + 'Computed CRC32C ($crc32c) for package with hosted CRC32C of ($hostedCrc32c).'); - if (hostedChecksum != computedCrc32c) { + if (hostedCrc32c != crc32c) { throw PackageIntegrityException( - 'Package fetched from host has a CRC32C checksum mismatch; Computed checksum ($computedCrc32c) != Hosted checksum ($hostedChecksum)', + 'Package fetched from host has a CRC32C checksum mismatch; Computed checksum ($crc32c) != Hosted checksum ($hostedCrc32c)', response.request?.url); } - })); + }); } /// Parses response headers and returns the package's CRC32C checksum. @@ -1167,7 +1160,7 @@ Stream> _responseStreamWithChecksumValidationTap( /// its response "headers" Map. /// See https://github.com/dart-lang/http/issues/24 /// https://github.com/dart-lang/http/blob/06649afbb5847dbb0293816ba8348766b116e419/pkgs/http/lib/src/base_response.dart#L29 -int? _parseChecksum(Map responseHeaders) { +int? _parseCrc32c(Map responseHeaders) { String? checksums = responseHeaders[checksumHeaderName]; if (checksums == null) return null; @@ -1175,8 +1168,8 @@ int? _parseChecksum(Map responseHeaders) { for (final part in parts) { if (part.startsWith('crc32c=')) { final undecoded = part.substring('crc32c='.length); - final rawBytes = base64.decode(undecoded); - return ByteData.view(rawBytes.buffer).getUint32(0); + final bytes = base64.decode(undecoded); + return bytesToUint32(bytes); } } diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 185290494..0aaa09f2e 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -7,6 +7,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; import 'dart:math' as math; +import 'dart:typed_data'; import 'package:crypto/crypto.dart' as crypto; import 'package:pub_semver/pub_semver.dart'; @@ -649,3 +650,15 @@ Map mapMap( key(entry.key, entry.value): value(entry.key, entry.value), }; } + +/// Converts `bytes` into an unsigned 32 bit integer. +int bytesToUint32(Uint8List bytes, + [int byteOffset = 0, Endian endian = Endian.big]) { + return ByteData.view(bytes.buffer).getUint32(byteOffset, endian); +} + +/// Converts `value` as an unsigned 32 bit integer to bytes. +Uint8List uint32ToBytes(int value, + [int byteOffset = 0, Endian endian = Endian.big]) { + return Uint8List(4)..buffer.asByteData().setInt32(byteOffset, value, endian); +} diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index f0270a3c7..fa92ab54f 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -64,6 +64,42 @@ void main() { ]).validate(); }); + test('response with CRC32C checksum is validated', () async { + var server = await startPackageServer(); + + server.serve('foo', '1.2.3'); + + expect(await server.peekArchiveChecksumHeader('foo', '1.2.3'), isNotNull); + + await d.appDir({ + 'foo': { + 'version': '1.2.3', + 'hosted': {'name': 'foo', 'url': 'http://localhost:${server.port}'} + } + }).create(); + + await pubGet(); + }); + + test('response with CRC32C checksum mismatch is caught', () async { + var server = await startPackageServer(); + + server.serve('foo', '1.2.3', headers: { + 'x-goog-hash': PackageServer.composeChecksumHeader(crc32c: 3381945770) + }); + + await d.appDir({ + 'foo': { + 'version': '1.2.3', + 'hosted': {'name': 'foo', 'url': 'http://localhost:${server.port}'} + } + }).create(); + + await pubGet( + error: contains( + 'Package fetched from host has a CRC32C checksum mismatch')); + }); + group('categorizes dependency types in the lockfile', () { setUp(() async { await servePackages() diff --git a/test/package_server.dart b/test/package_server.dart index 6aaea6893..fffd1efbc 100644 --- a/test/package_server.dart +++ b/test/package_server.dart @@ -7,7 +7,10 @@ import 'dart:convert'; import 'dart:io'; import 'package:path/path.dart' as p; +import 'package:pub/src/crc32c.dart'; +import 'package:pub/src/source/hosted.dart'; import 'package:pub/src/third_party/tar/tar.dart'; +import 'package:pub/src/utils.dart' hide fail; import 'package:pub_semver/pub_semver.dart'; import 'package:shelf/shelf.dart' as shelf; import 'package:shelf/shelf_io.dart' as shelf_io; @@ -27,6 +30,9 @@ class PackageServer { // A list of all the requests recieved up till now. final List requestedPaths = []; + // Setting this to false will disable automatic calculation of checksums. + bool serveChecksums = true; + PackageServer._(this._inner) { _inner.mount((request) { final path = request.url.path; @@ -82,7 +88,7 @@ class PackageServer { server.handle( _downloadPattern, - (shelf.Request request) { + (shelf.Request request) async { final parts = request.url.pathSegments; assert(parts[0] == 'packages'); final name = parts[1]; @@ -98,7 +104,18 @@ class PackageServer { for (final packageVersion in package.versions.values) { if (packageVersion.version == version) { - return shelf.Response.ok(packageVersion.contents()); + final headers = packageVersion.headers ?? {}; + + // This gate enables tests to validate the CRC32C parser by + // passing in arbitrary values for the checksum header. + if (server.serveChecksums && + !headers.containsKey(checksumHeaderName)) { + headers[checksumHeaderName] = composeChecksumHeader( + crc32c: await packageVersion.computeArchiveCrc32c()); + } + + return shelf.Response.ok(packageVersion.contents(), + headers: headers); } } return shelf.Response.notFound('No version $version of $name'); @@ -178,7 +195,8 @@ class PackageServer { void serve(String name, String version, {Map? deps, Map? pubspec, - List? contents}) { + List? contents, + Map>? headers}) { var pubspecFields = {'name': name, 'version': version}; if (pubspec != null) pubspecFields.addAll(pubspec); if (deps != null) pubspecFields['dependencies'] = deps; @@ -189,6 +207,7 @@ class PackageServer { var package = _packages.putIfAbsent(name, _ServedPackage.new); package.versions[version] = _ServedPackageVersion( pubspecFields, + headers: headers, contents: () { final entries = []; @@ -243,6 +262,29 @@ class PackageServer { void retractPackageVersion(String name, String version) { _packages[name]!.versions[version]!.isRetracted = true; } + + Future peekArchiveChecksumHeader(String name, String version) async { + final v = _packages[name]!.versions[version]!; + + // If the test configured an overriding header value. + var checksumHeader = v.headers?[checksumHeaderName]; + + // Otherwise, compute from package contents. + if (serveChecksums) { + checksumHeader ??= + composeChecksumHeader(crc32c: await v.computeArchiveCrc32c()); + } + + return checksumHeader?.join(','); + } + + static List composeChecksumHeader( + {int? crc32c, String? md5 = '5f4dcc3b5aa765d61d8327deb882cf99'}) { + return [ + if (crc32c != null) 'crc32c=${base64.encode(uint32ToBytes(crc32c))}', + if (md5 != null) 'md5=${base64.encode(utf8.encode(md5))}' + ]; + } } class _ServedPackage { @@ -255,11 +297,16 @@ class _ServedPackage { class _ServedPackageVersion { final Map pubspec; final Stream> Function() contents; + final Map>? headers; bool isRetracted = false; Version get version => Version.parse(pubspec['version']); - _ServedPackageVersion(this.pubspec, {required this.contents}); + _ServedPackageVersion(this.pubspec, {required this.contents, this.headers}); + + Future computeArchiveCrc32c() async { + return await Crc32c.computeByConsumingStream(contents()); + } } class _PatternAndHandler { From f49da42040f2f5b5b78e674e6864bd9db615b912 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 16 Sep 2022 13:03:14 -0700 Subject: [PATCH 10/28] Add checksum header and log line to golden test log --- lib/src/source/hosted.dart | 2 +- test/embedding/embedding_test.dart | 10 ++++++++++ ...ten with --verbose and on unexpected exceptions.txt | 8 ++++++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index fb987aec4..e788848cd 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -1136,7 +1136,7 @@ Stream> _responseStreamWithChecksumValidationTap( return Crc32c.computeByTappingStream(response.stream, handleDone: (crc32c) { log.fine( - 'Computed CRC32C ($crc32c) for package with hosted CRC32C of ($hostedCrc32c).'); + 'Computed CRC32C ($crc32c) for package with hosted CRC32C of ($hostedCrc32c)'); if (hostedCrc32c != crc32c) { throw PackageIntegrityException( diff --git a/test/embedding/embedding_test.dart b/test/embedding/embedding_test.dart index 73370d7f6..5eee34d47 100644 --- a/test/embedding/embedding_test.dart +++ b/test/embedding/embedding_test.dart @@ -357,6 +357,16 @@ String _filter(String input) { RegExp(r'Writing \d+ characters', multiLine: true), r'Writing $N characters', ) + .replaceAll( + RegExp(r'x-goog-hash(.*)$', multiLine: true), + r'x-goog-hash: $CHECKSUM_HEADER', + ) + .replaceAll( + RegExp( + r'Computed CRC32C \(\d+\) for package with hosted CRC32C of \(\d+\)', + multiLine: true), + r'Computed CRC32C ($CRC32C) for package with hosted CRC32C of ($CRC32C)', + ) /// TODO(sigurdm): This hack suppresses differences in stack-traces /// between dart 2.17 and 2.18. Remove when 2.18 is stable. diff --git a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt index 3741c4072..fd5019d1c 100644 --- a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt +++ b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt @@ -50,13 +50,15 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] IO : HTTP response 200 OK for GET http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz [E] | took: $TIME [E] | x-powered-by: Dart with package:shelf -[E] | transfer-encoding: chunked [E] | date: $TIME +[E] | transfer-encoding: chunked +[E] | x-goog-hash: $CHECKSUM_HEADER [E] | x-frame-options: SAMEORIGIN [E] | content-type: text/plain; charset=utf-8 [E] | x-xss-protection: 1; mode=block [E] | x-content-type-options: nosniff [E] IO : Creating $FILE from stream +[E] FINE: Computed CRC32C ($CRC32C) for package with hosted CRC32C of ($CRC32C) [E] FINE: Created $FILE from stream [E] IO : Created temp directory $DIR [E] IO : Reading binary file $FILE. @@ -189,13 +191,15 @@ IO : HTTP GET http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz IO : HTTP response 200 OK for GET http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz | took: $TIME | x-powered-by: Dart with package:shelf - | transfer-encoding: chunked | date: $TIME + | transfer-encoding: chunked + | x-goog-hash: $CHECKSUM_HEADER | x-frame-options: SAMEORIGIN | content-type: text/plain; charset=utf-8 | x-xss-protection: 1; mode=block | x-content-type-options: nosniff IO : Creating $FILE from stream +FINE: Computed CRC32C ($CRC32C) for package with hosted CRC32C of ($CRC32C) FINE: Created $FILE from stream IO : Created temp directory $DIR IO : Reading binary file $FILE. From e5240d4b8d1eaaac89a564d0d6096e3a1b385c22 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 16 Sep 2022 13:55:53 -0700 Subject: [PATCH 11/28] Add tests for invalid checksum headers --- lib/src/source/hosted.dart | 17 +++++++---- lib/src/utils.dart | 23 ++++++++++++--- test/get/hosted/get_test.dart | 54 +++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index e788848cd..42a252e21 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -1130,8 +1130,8 @@ Stream> _responseStreamWithChecksumValidationTap( final hostedCrc32c = _parseCrc32c(response.headers); if (hostedCrc32c == null) { throw PackageIntegrityException( - 'Package response headers are missing CRC32C checksum', - response.request?.url); + 'Package response headers have an invalid or missing CRC32C checksum', + url: response.request?.url); } return Crc32c.computeByTappingStream(response.stream, handleDone: (crc32c) { @@ -1141,7 +1141,7 @@ Stream> _responseStreamWithChecksumValidationTap( if (hostedCrc32c != crc32c) { throw PackageIntegrityException( 'Package fetched from host has a CRC32C checksum mismatch; Computed checksum ($crc32c) != Hosted checksum ($hostedCrc32c)', - response.request?.url); + url: response.request?.url); } }); } @@ -1168,8 +1168,13 @@ int? _parseCrc32c(Map responseHeaders) { for (final part in parts) { if (part.startsWith('crc32c=')) { final undecoded = part.substring('crc32c='.length); - final bytes = base64.decode(undecoded); - return bytesToUint32(bytes); + + try { + final bytes = base64.decode(undecoded); + return bytesToUint32(bytes); + } catch (e) { + return null; + } } } @@ -1178,7 +1183,7 @@ int? _parseCrc32c(Map responseHeaders) { /// Package checksum related exception. class PackageIntegrityException implements Exception { - const PackageIntegrityException(this.message, this.url); + const PackageIntegrityException(this.message, {this.url}); final String message; final Uri? url; diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 0aaa09f2e..bbb3f6996 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -651,14 +651,29 @@ Map mapMap( }; } -/// Converts `bytes` into an unsigned 32 bit integer. +/// Converts [bytes] into an unsigned 32 bit integer. +/// +/// Throws `RangeError` if [byteOffset] is negative, or `byteOffset + 4` is +/// greater than the length of [bytes]. int bytesToUint32(Uint8List bytes, [int byteOffset = 0, Endian endian = Endian.big]) { - return ByteData.view(bytes.buffer).getUint32(byteOffset, endian); + try { + return ByteData.view(bytes.buffer).getUint32(byteOffset, endian); + } catch (e) { + rethrow; + } } -/// Converts `value` as an unsigned 32 bit integer to bytes. +/// Converts [value] as an unsigned 32 bit integer to bytes. +/// +/// Throws `RangeError` if [byteOffset] is negative, or `byteOffset + 4` is +/// greater than the length of [value]. Uint8List uint32ToBytes(int value, [int byteOffset = 0, Endian endian = Endian.big]) { - return Uint8List(4)..buffer.asByteData().setInt32(byteOffset, value, endian); + try { + return Uint8List(4) + ..buffer.asByteData().setUint32(byteOffset, value, endian); + } catch (e) { + rethrow; + } } diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index fa92ab54f..b8f748e22 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -100,6 +100,60 @@ void main() { 'Package fetched from host has a CRC32C checksum mismatch')); }); + group('recognizes bad checksum header', () { + late PackageServer server; + + setUp(() async { + server = await servePackages() + ..serve('foo', '1.2.3', headers: { + 'x-goog-hash': [''] + }) + ..serve('bar', '1.2.3', headers: { + 'x-goog-hash': ['crc32c=,md5='] + }) + ..serve('baz', '1.2.3', headers: { + 'x-goog-hash': ['crc32c=loremipsum,md5=loremipsum'] + }); + }); + + test('when it is empty', () async { + await d.appDir({ + 'foo': { + 'version': '1.2.3', + 'hosted': {'name': 'foo', 'url': 'http://localhost:${server.port}'} + } + }).create(); + + await pubGet( + error: contains( + 'Package response headers have an invalid or missing CRC32C checksum')); + }); + + test('when it is invalid', () async { + await d.appDir({ + 'bar': { + 'version': '1.2.3', + 'hosted': {'name': 'foo', 'url': 'http://localhost:${server.port}'} + } + }).create(); + + await pubGet( + error: contains( + 'Package response headers have an invalid or missing CRC32C checksum')); + + await d.appDir({ + 'baz': { + 'version': '1.2.3', + 'hosted': {'name': 'foo', 'url': 'http://localhost:${server.port}'} + } + }).create(); + + await pubGet( + error: contains( + 'Package response headers have an invalid or missing CRC32C checksum')); + }); + }); + group('categorizes dependency types in the lockfile', () { setUp(() async { await servePackages() From 6b451bb583c248d10f903750fff290cccf1da2c1 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 16 Sep 2022 16:46:36 -0700 Subject: [PATCH 12/28] Add file name to checksum IO log --- lib/src/source/hosted.dart | 10 +++++----- test/embedding/embedding_test.dart | 4 ++-- ...ten with --verbose and on unexpected exceptions.txt | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 42a252e21..d7d0f1858 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -858,13 +858,13 @@ class HostedSource extends CachedSource { // Download and extract the archive to a temp directory. await withTempDir((tempDirForArchive) async { - var archivePath = - p.join(tempDirForArchive, '$packageName-$version.tar.gz'); + var fileName = '$packageName-$version.tar.gz'; + var archivePath = p.join(tempDirForArchive, fileName); var request = _createArchiveRequest(url); var response = await withAuthenticatedClient( cache, Uri.parse(description.url), (client) => client.send(request)); var responseStream = _hasChecksumHeader(response.headers) - ? _responseStreamWithChecksumValidationTap(response) + ? _responseStreamWithChecksumValidationTap(response, fileName) : response.stream; // We download the archive to disk instead of streaming it directly into @@ -1126,7 +1126,7 @@ bool _hasChecksumHeader(Map headers) { /// Throws [PackageIntegrityException] if anything is wrong with the checksum /// or if there is a checksum mismatch. Stream> _responseStreamWithChecksumValidationTap( - http.StreamedResponse response) { + http.StreamedResponse response, String fileName) { final hostedCrc32c = _parseCrc32c(response.headers); if (hostedCrc32c == null) { throw PackageIntegrityException( @@ -1136,7 +1136,7 @@ Stream> _responseStreamWithChecksumValidationTap( return Crc32c.computeByTappingStream(response.stream, handleDone: (crc32c) { log.fine( - 'Computed CRC32C ($crc32c) for package with hosted CRC32C of ($hostedCrc32c)'); + 'Computed CRC32C ($crc32c) for $fileName with hosted CRC32C of ($hostedCrc32c)'); if (hostedCrc32c != crc32c) { throw PackageIntegrityException( diff --git a/test/embedding/embedding_test.dart b/test/embedding/embedding_test.dart index 5eee34d47..7f97745e4 100644 --- a/test/embedding/embedding_test.dart +++ b/test/embedding/embedding_test.dart @@ -363,9 +363,9 @@ String _filter(String input) { ) .replaceAll( RegExp( - r'Computed CRC32C \(\d+\) for package with hosted CRC32C of \(\d+\)', + r'Computed CRC32C \(\d+\) for (.+) with hosted CRC32C of \(\d+\)', multiLine: true), - r'Computed CRC32C ($CRC32C) for package with hosted CRC32C of ($CRC32C)', + r'Computed CRC32C ($CRC32C) for $FILENAME with hosted CRC32C of ($CRC32C)', ) /// TODO(sigurdm): This hack suppresses differences in stack-traces diff --git a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt index fd5019d1c..bee9344c3 100644 --- a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt +++ b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt @@ -58,7 +58,7 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] | x-xss-protection: 1; mode=block [E] | x-content-type-options: nosniff [E] IO : Creating $FILE from stream -[E] FINE: Computed CRC32C ($CRC32C) for package with hosted CRC32C of ($CRC32C) +[E] FINE: Computed CRC32C ($CRC32C) for $FILENAME with hosted CRC32C of ($CRC32C) [E] FINE: Created $FILE from stream [E] IO : Created temp directory $DIR [E] IO : Reading binary file $FILE. @@ -199,7 +199,7 @@ IO : HTTP response 200 OK for GET http://localhost:$PORT/packages/foo/versions/ | x-xss-protection: 1; mode=block | x-content-type-options: nosniff IO : Creating $FILE from stream -FINE: Computed CRC32C ($CRC32C) for package with hosted CRC32C of ($CRC32C) +FINE: Computed CRC32C ($CRC32C) for $FILENAME with hosted CRC32C of ($CRC32C) FINE: Created $FILE from stream IO : Created temp directory $DIR IO : Reading binary file $FILE. From b38352f49522ae6af109af9a18d799f1f3d6dcd2 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 19 Sep 2022 15:21:07 -0700 Subject: [PATCH 13/28] Document header in archive request --- lib/src/source/hosted.dart | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index d7d0f1858..52ce49bae 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -886,9 +886,16 @@ class HostedSource extends CachedSource { }); } + /// Creates a GET `Request` for [url] that will ask the host to prevent + /// decompressive transcoding. + /// + /// GCS docmentation indicates that you should do this when you want to + /// validate the object's checksum. + /// + /// See https://cloud.google.com/storage/docs/transcoding#decompressive_transcoding http.Request _createArchiveRequest(Uri url) { var request = http.Request('GET', url); - request.headers[io.HttpHeaders.cacheControlHeader] = 'no-transform'; + request.headers[io.HttpHeaders.acceptEncodingHeader] = 'gzip'; return request; } From aba4b795f82ac33f8d072000b3df086b5b1387e1 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 19 Sep 2022 15:53:02 -0700 Subject: [PATCH 14/28] Improve error handling, logging, and tests --- lib/src/source/hosted.dart | 76 +++++++++---------- test/embedding/embedding_test.dart | 4 +- test/get/hosted/get_test.dart | 56 +++++++------- ...--verbose and on unexpected exceptions.txt | 4 +- 4 files changed, 69 insertions(+), 71 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 52ce49bae..23dc10bda 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -863,8 +863,10 @@ class HostedSource extends CachedSource { var request = _createArchiveRequest(url); var response = await withAuthenticatedClient( cache, Uri.parse(description.url), (client) => client.send(request)); - var responseStream = _hasChecksumHeader(response.headers) - ? _responseStreamWithChecksumValidationTap(response, fileName) + + final expectedChecksum = _parseCrc32c(response.headers, fileName); + final responseStream = expectedChecksum != null + ? _validateStream(response.stream, expectedChecksum, fileName) : response.stream; // We download the archive to disk instead of streaming it directly into @@ -1119,41 +1121,29 @@ class _RefAndCache { const checksumHeaderName = 'x-goog-hash'; -bool _hasChecksumHeader(Map headers) { - return headers.containsKey(checksumHeaderName); -} - /// Adds a checksum validation "tap" to the response stream and returns a -/// wrapped Stream object, which should be used to consume the incoming data. +/// wrapped `Stream` object, which should be used to consume the incoming data. /// /// As chunks are received, a CRC32C checksum is updated. /// Once the download is completed, the final checksum is compared with /// the one present in the checksum response header. /// -/// Throws [PackageIntegrityException] if anything is wrong with the checksum -/// or if there is a checksum mismatch. -Stream> _responseStreamWithChecksumValidationTap( - http.StreamedResponse response, String fileName) { - final hostedCrc32c = _parseCrc32c(response.headers); - if (hostedCrc32c == null) { - throw PackageIntegrityException( - 'Package response headers have an invalid or missing CRC32C checksum', - url: response.request?.url); - } - - return Crc32c.computeByTappingStream(response.stream, handleDone: (crc32c) { +/// Throws [PackageIntegrityException] if there is a checksum mismatch. +Stream> _validateStream( + Stream> stream, int expectedChecksum, String fileName) { + return Crc32c.computeByTappingStream(stream, handleDone: (actualChecksum) { log.fine( - 'Computed CRC32C ($crc32c) for $fileName with hosted CRC32C of ($hostedCrc32c)'); + 'Computed checksum $actualChecksum for "$fileName" with expected CRC32C of $expectedChecksum'); - if (hostedCrc32c != crc32c) { + if (actualChecksum != expectedChecksum) { throw PackageIntegrityException( - 'Package fetched from host has a CRC32C checksum mismatch; Computed checksum ($crc32c) != Hosted checksum ($hostedCrc32c)', - url: response.request?.url); + 'Package archive "$fileName" has a CRC32C checksum mismatch', + ); } }); } -/// Parses response headers and returns the package's CRC32C checksum. +/// Parses response [headers] and returns the archive's CRC32C checksum. /// /// In most cases, GCS provides both MD5 and CRC32C checksums in its response /// headers. It uses the header name "x-goog-hash" for these values. It has @@ -1167,11 +1157,13 @@ Stream> _responseStreamWithChecksumValidationTap( /// its response "headers" Map. /// See https://github.com/dart-lang/http/issues/24 /// https://github.com/dart-lang/http/blob/06649afbb5847dbb0293816ba8348766b116e419/pkgs/http/lib/src/base_response.dart#L29 -int? _parseCrc32c(Map responseHeaders) { - String? checksums = responseHeaders[checksumHeaderName]; - if (checksums == null) return null; +/// +/// Throws [PackageIntegrityException] if the CRC32C checksum cannot be parsed. +int? _parseCrc32c(Map headers, String fileName) { + final checksumHeader = headers[checksumHeaderName]; + if (checksumHeader == null) return null; - final parts = checksums.split(','); + final parts = checksumHeader.split(','); for (final part in parts) { if (part.startsWith('crc32c=')) { final undecoded = part.substring('crc32c='.length); @@ -1179,28 +1171,30 @@ int? _parseCrc32c(Map responseHeaders) { try { final bytes = base64.decode(undecoded); return bytesToUint32(bytes); - } catch (e) { - return null; + + // Suppress lint because 1) we are dealing with arbitrary runtime values + // 2) we do not want to duplicate the validation logic that is already + // throwing the `RangeError`. + // ignore: avoid_catching_errors + } on RangeError { + continue; + } on FormatException { + continue; } } } - return null; + throw PackageIntegrityException( + 'Package archive "$fileName" has a malformed CRC32C checksum in its response headers', + ); } /// Package checksum related exception. class PackageIntegrityException implements Exception { - const PackageIntegrityException(this.message, {this.url}); - final String message; - final Uri? url; + + PackageIntegrityException(this.message); @override - String toString() { - var temp = message; - if (url != null) { - temp += ': $url'; - } - return temp; - } + String toString() => message; } diff --git a/test/embedding/embedding_test.dart b/test/embedding/embedding_test.dart index 7f97745e4..bd7718bce 100644 --- a/test/embedding/embedding_test.dart +++ b/test/embedding/embedding_test.dart @@ -363,9 +363,9 @@ String _filter(String input) { ) .replaceAll( RegExp( - r'Computed CRC32C \(\d+\) for (.+) with hosted CRC32C of \(\d+\)', + r'Computed checksum \(\d+\) for "(.+)" with expected CRC32C of \(\d+\)', multiLine: true), - r'Computed CRC32C ($CRC32C) for $FILENAME with hosted CRC32C of ($CRC32C)', + r'Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C', ) /// TODO(sigurdm): This hack suppresses differences in stack-traces diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index b8f748e22..f00bfb6ab 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -12,10 +12,29 @@ import '../../descriptor.dart' as d; import '../../test_pub.dart'; void main() { - test('gets a package from a pub server', () async { + test('gets a package from a pub server serving CRC32C checksums', () async { final server = await servePackages(); server.serve('foo', '1.2.3'); + expect(await server.peekArchiveChecksumHeader('foo', '1.2.3'), isNotNull); + + await d.appDir({'foo': '1.2.3'}).create(); + + await pubGet(); + + await d.cacheDir({'foo': '1.2.3'}).validate(); + await d.appPackageConfigFile([ + d.packageConfigEntry(name: 'foo', version: '1.2.3'), + ]).validate(); + }); + + test('gets a package from a pub server not serving checksums', () async { + final server = await servePackages(); + server.serveChecksums = false; + server.serve('foo', '1.2.3'); + + expect(await server.peekArchiveChecksumHeader('foo', '1.2.3'), isNull); + await d.appDir({'foo': '1.2.3'}).create(); await pubGet(); @@ -64,24 +83,9 @@ void main() { ]).validate(); }); - test('response with CRC32C checksum is validated', () async { - var server = await startPackageServer(); - - server.serve('foo', '1.2.3'); - - expect(await server.peekArchiveChecksumHeader('foo', '1.2.3'), isNotNull); - - await d.appDir({ - 'foo': { - 'version': '1.2.3', - 'hosted': {'name': 'foo', 'url': 'http://localhost:${server.port}'} - } - }).create(); - - await pubGet(); - }); - - test('response with CRC32C checksum mismatch is caught', () async { + test( + 'recognizes a package\'s actual checksum did not match the expected checksum', + () async { var server = await startPackageServer(); server.serve('foo', '1.2.3', headers: { @@ -97,7 +101,7 @@ void main() { await pubGet( error: contains( - 'Package fetched from host has a CRC32C checksum mismatch')); + 'Package archive "foo-1.2.3.tar.gz" has a CRC32C checksum mismatch')); }); group('recognizes bad checksum header', () { @@ -126,31 +130,31 @@ void main() { await pubGet( error: contains( - 'Package response headers have an invalid or missing CRC32C checksum')); + 'Package archive "foo-1.2.3.tar.gz" has a malformed CRC32C checksum in its response headers')); }); - test('when it is invalid', () async { + test('when it is malformed', () async { await d.appDir({ 'bar': { 'version': '1.2.3', - 'hosted': {'name': 'foo', 'url': 'http://localhost:${server.port}'} + 'hosted': {'name': 'bar', 'url': 'http://localhost:${server.port}'} } }).create(); await pubGet( error: contains( - 'Package response headers have an invalid or missing CRC32C checksum')); + 'Package archive "bar-1.2.3.tar.gz" has a malformed CRC32C checksum in its response headers')); await d.appDir({ 'baz': { 'version': '1.2.3', - 'hosted': {'name': 'foo', 'url': 'http://localhost:${server.port}'} + 'hosted': {'name': 'baz', 'url': 'http://localhost:${server.port}'} } }).create(); await pubGet( error: contains( - 'Package response headers have an invalid or missing CRC32C checksum')); + 'Package archive "baz-1.2.3.tar.gz" has a malformed CRC32C checksum in its response headers')); }); }); diff --git a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt index bee9344c3..080d95c62 100644 --- a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt +++ b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt @@ -58,7 +58,7 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] | x-xss-protection: 1; mode=block [E] | x-content-type-options: nosniff [E] IO : Creating $FILE from stream -[E] FINE: Computed CRC32C ($CRC32C) for $FILENAME with hosted CRC32C of ($CRC32C) +[E] FINE: Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C [E] FINE: Created $FILE from stream [E] IO : Created temp directory $DIR [E] IO : Reading binary file $FILE. @@ -199,7 +199,7 @@ IO : HTTP response 200 OK for GET http://localhost:$PORT/packages/foo/versions/ | x-xss-protection: 1; mode=block | x-content-type-options: nosniff IO : Creating $FILE from stream -FINE: Computed CRC32C ($CRC32C) for $FILENAME with hosted CRC32C of ($CRC32C) +FINE: Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C FINE: Created $FILE from stream IO : Created temp directory $DIR IO : Reading binary file $FILE. From 499e4efacf967c48b939e99c4d73ab9d62013165 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 20 Sep 2022 11:48:16 -0700 Subject: [PATCH 15/28] Retry downloads after checksum validation errors --- lib/src/source/hosted.dart | 50 ++++++++++++++++++++--------- lib/src/utils.dart | 60 +++++++++++++++++++++++++++++++++++ test/get/hosted/get_test.dart | 4 ++- 3 files changed, 98 insertions(+), 16 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 23dc10bda..b7c5d874b 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -860,21 +860,41 @@ class HostedSource extends CachedSource { await withTempDir((tempDirForArchive) async { var fileName = '$packageName-$version.tar.gz'; var archivePath = p.join(tempDirForArchive, fileName); - var request = _createArchiveRequest(url); - var response = await withAuthenticatedClient( - cache, Uri.parse(description.url), (client) => client.send(request)); - - final expectedChecksum = _parseCrc32c(response.headers, fileName); - final responseStream = expectedChecksum != null - ? _validateStream(response.stream, expectedChecksum, fileName) - : response.stream; - - // We download the archive to disk instead of streaming it directly into - // the tar unpacking. This simplifies stream handling. - // Package:tar cancels the stream when it reaches end-of-archive, and - // cancelling a http stream makes it not reusable. - // There are ways around this, and we might revisit this later. - await createFileFromStream(responseStream, archivePath); + + // The client from `withAuthenticatedClient` will retry HTTP requests. + // This wrapper is one layer up and will retry checksum validation errors. + await retry( + // Attempt to download archive and validate its checksum. + () async { + final request = _createArchiveRequest(url); + final response = await withAuthenticatedClient(cache, + Uri.parse(description.url), (client) => client.send(request)); + final expectedChecksum = _parseCrc32c(response.headers, fileName); + + // We download the archive to disk instead of streaming it directly + // into the tar unpacking. This simplifies stream handling. + // Package:tar cancels the stream when it reaches end-of-archive, and + // cancelling a http stream makes it not reusable. + // There are ways around this, and we might revisit this later. + + if (expectedChecksum == null) { + // Skip validation if the checksum response header is not present. + await createFileFromStream(response.stream, archivePath); + } else { + // The checksum will be validated once this finishes consuming the + // stream. [PackageIntegrityException] will be thrown on failure. + await createFileFromStream( + _validateStream(response.stream, expectedChecksum, fileName), + archivePath); + } + }, + // Retry if the checksum response header was malformed or the actual + // checksum did not match the expected checksum. + retryIf: (e) => e is PackageIntegrityException, + onRetry: (e, retryCount) => log.io( + 'Retry #${retryCount + 1} for checksum error with "$fileName"...'), + ); + var tempDir = cache.createTempDir(); await extractTarGz(readBinaryFileAsSream(archivePath), tempDir); diff --git a/lib/src/utils.dart b/lib/src/utils.dart index bbb3f6996..625766f7d 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -677,3 +677,63 @@ Uint8List uint32ToBytes(int value, rethrow; } } + +/// Call [fn] retrying so long as [retryIf] return `true` for the exception +/// thrown, up-to [maxAttempts] times. +/// +/// Defaults to 8 attempts, sleeping as following after 1st, 2nd, 3rd, ..., +/// 7th attempt: +/// 1. 400 ms +/- 25% +/// 2. 800 ms +/- 25% +/// 3. 1600 ms +/- 25% +/// 4. 3200 ms +/- 25% +/// 5. 6400 ms +/- 25% +/// 6. 12800 ms +/- 25% +/// 7. 25600 ms +/- 25% +/// +/// ```dart +/// final response = await retry( +/// // Make a GET request +/// () => http.get('https://google.com').timeout(Duration(seconds: 5)), +/// // Retry on SocketException or TimeoutException +/// retryIf: (e) => e is SocketException || e is TimeoutException, +/// ); +/// print(response.body); +/// ``` +/// +/// If no [retryIf] function is given this will retry any for any [Exception] +/// thrown. To retry on an [Error], the error must be caught and _rethrown_ +/// as an [Exception]. +/// +/// See https://github.com/google/dart-neats/blob/master/retry/lib/retry.dart +Future retry( + FutureOr Function() fn, { + Duration delayFactor = const Duration(milliseconds: 200), + double randomizationFactor = 0.25, + Duration maxDelay = const Duration(seconds: 30), + int maxAttempts = 8, + FutureOr Function(Exception)? retryIf, + FutureOr Function(Exception, int retryCount)? onRetry, +}) async { + var attempt = 0; + // ignore: literal_only_boolean_expressions + while (true) { + attempt++; // first invocation is the first attempt + try { + return await fn(); + } on Exception catch (e) { + if (attempt >= maxAttempts || (retryIf != null && !(await retryIf(e)))) { + rethrow; + } + if (onRetry != null) { + await onRetry(e, attempt); + } + } + + // Sleep for a delay + final rf = randomizationFactor * (random.nextDouble() * 2 - 1) + 1; + final exp = math.min(attempt, 31); // prevent overflows. + final delay = delayFactor * math.pow(2.0, exp) * rf; + await Future.delayed(delay < maxDelay ? delay : maxDelay); + } +} diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index f00bfb6ab..8970c8b33 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -84,7 +84,7 @@ void main() { }); test( - 'recognizes a package\'s actual checksum did not match the expected checksum', + "recognizes a package's actual checksum did not match the expected checksum", () async { var server = await startPackageServer(); @@ -107,6 +107,8 @@ void main() { group('recognizes bad checksum header', () { late PackageServer server; + // TODO: test where server doesn't respond with md5 at all? + setUp(() async { server = await servePackages() ..serve('foo', '1.2.3', headers: { From d505b29b95873c0e385cf7363f280073af955ede Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 20 Sep 2022 16:14:59 -0700 Subject: [PATCH 16/28] Simplify checksum logic and improve error handling --- lib/src/crc32c.dart | 21 ++------- lib/src/exceptions.dart | 9 ++++ lib/src/source/hosted.dart | 90 ++++++++++++++++++-------------------- lib/src/utils.dart | 28 ------------ test/package_server.dart | 18 +++++--- 5 files changed, 68 insertions(+), 98 deletions(-) diff --git a/lib/src/crc32c.dart b/lib/src/crc32c.dart index 1539c0296..e346a5943 100644 --- a/lib/src/crc32c.dart +++ b/lib/src/crc32c.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 'utils.dart'; - /// Computes a crc32c checksum. class Crc32c { int _current = mask; @@ -22,26 +20,13 @@ class Crc32c { return _current ^ mask & mask; } - /// Transforms `stream` with a tap that calculates a CRC32C checksum, which - /// is accessible through the `handleDone` method once the stream is finished. - static Stream> computeByTappingStream(Stream> stream, - {required void Function(int crc32c) handleDone}) { - final checksumComputer = Crc32c(); - - return stream - .transform(onDataTransformer(checksumComputer.update)) - .transform(onDoneTransformer(() { - handleDone(checksumComputer.finalize()); - })); - } - - /// Consumes the entirety of `stream` and returns the CRC32C checksum of its + /// Consumes the entirety of "stream" and returns the CRC32C checksum of its /// data once the stream is finished. static Future computeByConsumingStream(Stream> stream) async { final checksumComputer = Crc32c(); - await for (final data in stream) { - checksumComputer.update(data); + await for (final chunk in stream) { + checksumComputer.update(chunk); } return checksumComputer.finalize(); diff --git a/lib/src/exceptions.dart b/lib/src/exceptions.dart index fcde747d8..ec9f87659 100644 --- a/lib/src/exceptions.dart +++ b/lib/src/exceptions.dart @@ -104,6 +104,15 @@ class PackageNotFoundException extends WrappedException { String toString() => 'Package not available ($message).'; } +/// A class for exceptions where a package's checksum could not be validated. +class PackageIntegrityException extends WrappedException { + PackageIntegrityException( + String message, { + Object? innerError, + StackTrace? innerTrace, + }) : super(message, innerError, innerTrace); +} + /// Returns whether [error] is a user-facing error object. /// /// This includes both [ApplicationException] and any dart:io errors. diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index b7c5d874b..e48b98a87 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -5,6 +5,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io' as io; +import 'dart:typed_data'; import 'package:collection/collection.dart' show maxBy, IterableNullableExtension; @@ -871,28 +872,24 @@ class HostedSource extends CachedSource { Uri.parse(description.url), (client) => client.send(request)); final expectedChecksum = _parseCrc32c(response.headers, fileName); + Stream> stream = response.stream; + if (expectedChecksum != null) { + stream = + _validateStream(response.stream, expectedChecksum, fileName); + } + // We download the archive to disk instead of streaming it directly // into the tar unpacking. This simplifies stream handling. // Package:tar cancels the stream when it reaches end-of-archive, and // cancelling a http stream makes it not reusable. // There are ways around this, and we might revisit this later. - - if (expectedChecksum == null) { - // Skip validation if the checksum response header is not present. - await createFileFromStream(response.stream, archivePath); - } else { - // The checksum will be validated once this finishes consuming the - // stream. [PackageIntegrityException] will be thrown on failure. - await createFileFromStream( - _validateStream(response.stream, expectedChecksum, fileName), - archivePath); - } + await createFileFromStream(stream, archivePath); }, // Retry if the checksum response header was malformed or the actual // checksum did not match the expected checksum. retryIf: (e) => e is PackageIntegrityException, onRetry: (e, retryCount) => log.io( - 'Retry #${retryCount + 1} for checksum error with "$fileName"...'), + 'Retry #${retryCount + 1} because of checksum error with GET $url...'), ); var tempDir = cache.createTempDir(); @@ -1150,17 +1147,25 @@ const checksumHeaderName = 'x-goog-hash'; /// /// Throws [PackageIntegrityException] if there is a checksum mismatch. Stream> _validateStream( - Stream> stream, int expectedChecksum, String fileName) { - return Crc32c.computeByTappingStream(stream, handleDone: (actualChecksum) { - log.fine( - 'Computed checksum $actualChecksum for "$fileName" with expected CRC32C of $expectedChecksum'); - - if (actualChecksum != expectedChecksum) { - throw PackageIntegrityException( - 'Package archive "$fileName" has a CRC32C checksum mismatch', - ); - } - }); + Stream> stream, int expectedChecksum, String fileName) async* { + final crc32c = Crc32c(); + + await for (final chunk in stream) { + crc32c.update(chunk); + yield chunk; + } + + final actualChecksum = crc32c.finalize(); + + log.fine( + 'Computed checksum $actualChecksum for "$fileName" with expected CRC32C ' + 'of $expectedChecksum'); + + if (actualChecksum != expectedChecksum) { + throw PackageIntegrityException( + 'Package archive "$fileName" has a CRC32C checksum mismatch', + ); + } } /// Parses response [headers] and returns the archive's CRC32C checksum. @@ -1189,32 +1194,23 @@ int? _parseCrc32c(Map headers, String fileName) { final undecoded = part.substring('crc32c='.length); try { - final bytes = base64.decode(undecoded); - return bytesToUint32(bytes); - - // Suppress lint because 1) we are dealing with arbitrary runtime values - // 2) we do not want to duplicate the validation logic that is already - // throwing the `RangeError`. - // ignore: avoid_catching_errors - } on RangeError { - continue; - } on FormatException { - continue; + final bytes = base64Decode(undecoded); + + // CRC32C must be 32 bits, or 4 bytes. + if (bytes.length != 4) { + throw FormatException('CRC32C checksum has invalid length', bytes); + } + + return ByteData.view(bytes.buffer).getUint32(0); + } on FormatException catch (e, s) { + throw PackageIntegrityException( + 'Package archive "$fileName" has a malformed CRC32C checksum in ' + 'its response headers', + innerError: e, + innerTrace: s); } } } - throw PackageIntegrityException( - 'Package archive "$fileName" has a malformed CRC32C checksum in its response headers', - ); -} - -/// Package checksum related exception. -class PackageIntegrityException implements Exception { - final String message; - - PackageIntegrityException(this.message); - - @override - String toString() => message; + return null; } diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 625766f7d..3e72d79c9 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -7,7 +7,6 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; import 'dart:math' as math; -import 'dart:typed_data'; import 'package:crypto/crypto.dart' as crypto; import 'package:pub_semver/pub_semver.dart'; @@ -651,33 +650,6 @@ Map mapMap( }; } -/// Converts [bytes] into an unsigned 32 bit integer. -/// -/// Throws `RangeError` if [byteOffset] is negative, or `byteOffset + 4` is -/// greater than the length of [bytes]. -int bytesToUint32(Uint8List bytes, - [int byteOffset = 0, Endian endian = Endian.big]) { - try { - return ByteData.view(bytes.buffer).getUint32(byteOffset, endian); - } catch (e) { - rethrow; - } -} - -/// Converts [value] as an unsigned 32 bit integer to bytes. -/// -/// Throws `RangeError` if [byteOffset] is negative, or `byteOffset + 4` is -/// greater than the length of [value]. -Uint8List uint32ToBytes(int value, - [int byteOffset = 0, Endian endian = Endian.big]) { - try { - return Uint8List(4) - ..buffer.asByteData().setUint32(byteOffset, value, endian); - } catch (e) { - rethrow; - } -} - /// Call [fn] retrying so long as [retryIf] return `true` for the exception /// thrown, up-to [maxAttempts] times. /// diff --git a/test/package_server.dart b/test/package_server.dart index fffd1efbc..011282052 100644 --- a/test/package_server.dart +++ b/test/package_server.dart @@ -5,12 +5,12 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; +import 'dart:typed_data'; import 'package:path/path.dart' as p; import 'package:pub/src/crc32c.dart'; import 'package:pub/src/source/hosted.dart'; import 'package:pub/src/third_party/tar/tar.dart'; -import 'package:pub/src/utils.dart' hide fail; import 'package:pub_semver/pub_semver.dart'; import 'package:shelf/shelf.dart' as shelf; import 'package:shelf/shelf_io.dart' as shelf_io; @@ -280,10 +280,18 @@ class PackageServer { static List composeChecksumHeader( {int? crc32c, String? md5 = '5f4dcc3b5aa765d61d8327deb882cf99'}) { - return [ - if (crc32c != null) 'crc32c=${base64.encode(uint32ToBytes(crc32c))}', - if (md5 != null) 'md5=${base64.encode(utf8.encode(md5))}' - ]; + List header = []; + + if (crc32c != null) { + final bytes = Uint8List(4)..buffer.asByteData().setUint32(0, crc32c); + header.add('crc32c=${base64.encode(bytes)}'); + } + + if (md5 != null) { + header.add('md5=${base64.encode(utf8.encode(md5))}'); + } + + return header; } } From 0d1b87fa6eeb92adb553d18cc7abd9f7a811eab1 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 21 Sep 2022 16:59:27 -0700 Subject: [PATCH 17/28] Use max retries environment variable --- lib/src/source/hosted.dart | 6 ++++++ lib/src/utils.dart | 11 ----------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index e48b98a87..130b30889 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -5,6 +5,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io' as io; +import 'dart:math' as math; import 'dart:typed_data'; import 'package:collection/collection.dart' @@ -890,6 +891,11 @@ class HostedSource extends CachedSource { retryIf: (e) => e is PackageIntegrityException, onRetry: (e, retryCount) => log.io( 'Retry #${retryCount + 1} because of checksum error with GET $url...'), + maxAttempts: math.max( + 1, // Having less than 1 retry is **always** wrong. + int.tryParse(io.Platform.environment['PUB_MAX_HTTP_RETRIES'] ?? '') ?? + 7, + ), ); var tempDir = cache.createTempDir(); diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 3e72d79c9..dbc762255 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -148,17 +148,6 @@ Future> waitAndPrintErrors(Iterable> futures) { }); } -/// Returns a [StreamTransformer] that will call [onData] for each data -/// event of the stream. -/// -/// The stream will be passed through unchanged. -StreamTransformer onDataTransformer(void Function(T data) onData) { - return StreamTransformer.fromHandlers(handleData: (data, sink) { - onData(data); - sink.add(data); - }); -} - /// Returns a [StreamTransformer] that will call [onDone] when the stream /// completes. /// From f1d40d597e9b6ff158e1b35724d40a9d46f39686 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 21 Sep 2022 17:28:57 -0700 Subject: [PATCH 18/28] Improve tests --- test/get/hosted/get_test.dart | 120 +++++++++++++++++++++++++--------- 1 file changed, 90 insertions(+), 30 deletions(-) diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index 8970c8b33..1b59c182d 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -12,7 +12,8 @@ import '../../descriptor.dart' as d; import '../../test_pub.dart'; void main() { - test('gets a package from a pub server serving CRC32C checksums', () async { + test('gets a package from a pub server and validates CRC32C checksum', + () async { final server = await servePackages(); server.serve('foo', '1.2.3'); @@ -28,21 +29,60 @@ void main() { ]).validate(); }); - test('gets a package from a pub server not serving checksums', () async { - final server = await servePackages(); - server.serveChecksums = false; - server.serve('foo', '1.2.3'); + group('gets a package from a pub server without validating checksum', () { + late PackageServer server; - expect(await server.peekArchiveChecksumHeader('foo', '1.2.3'), isNull); + setUp(() async { + server = await servePackages() + ..serveChecksums = false + ..serve('foo', '1.2.3') + ..serve('bar', '1.2.3', headers: { + 'x-goog-hash': [''] + }) + ..serve('baz', '1.2.3', headers: { + 'x-goog-hash': ['md5=loremipsum'] + }); + }); - await d.appDir({'foo': '1.2.3'}).create(); + test('because of missing checksum header', () async { + expect(await server.peekArchiveChecksumHeader('foo', '1.2.3'), isNull); - await pubGet(); + await d.appDir({'foo': '1.2.3'}).create(); - await d.cacheDir({'foo': '1.2.3'}).validate(); - await d.appPackageConfigFile([ - d.packageConfigEntry(name: 'foo', version: '1.2.3'), - ]).validate(); + await pubGet(); + + await d.cacheDir({'foo': '1.2.3'}).validate(); + await d.appPackageConfigFile([ + d.packageConfigEntry(name: 'foo', version: '1.2.3'), + ]).validate(); + }); + + test('because of empty checksum header', () async { + expect(await server.peekArchiveChecksumHeader('bar', '1.2.3'), ''); + + await d.appDir({'bar': '1.2.3'}).create(); + + await pubGet(); + + await d.cacheDir({'bar': '1.2.3'}).validate(); + await d.appPackageConfigFile([ + d.packageConfigEntry(name: 'bar', version: '1.2.3'), + ]).validate(); + }); + + test('because of missing CRC32C in checksum header', () async { + expect(await server.peekArchiveChecksumHeader('baz', '1.2.3'), + 'md5=loremipsum'); + + await d.appDir({'baz': '1.2.3'}).create(); + + await pubGet(); + + await d.cacheDir({'baz': '1.2.3'}).validate(); + await d.appPackageConfigFile([ + d.packageConfigEntry(name: 'baz', version: '1.2.3'), + ]).validate(); + }); }); test('URL encodes the package name', () async { @@ -83,9 +123,7 @@ void main() { ]).validate(); }); - test( - "recognizes a package's actual checksum did not match the expected checksum", - () async { + test('recognizes a package with a CRC32C checksum mismatch', () async { var server = await startPackageServer(); server.serve('foo', '1.2.3', headers: { @@ -100,29 +138,31 @@ void main() { }).create(); await pubGet( - error: contains( - 'Package archive "foo-1.2.3.tar.gz" has a CRC32C checksum mismatch')); + error: contains( + 'Package archive "foo-1.2.3.tar.gz" has a CRC32C checksum mismatch'), + environment: { + 'PUB_MAX_HTTP_RETRIES': '1', + }, + ); }); group('recognizes bad checksum header', () { late PackageServer server; - // TODO: test where server doesn't respond with md5 at all? - setUp(() async { server = await servePackages() ..serve('foo', '1.2.3', headers: { - 'x-goog-hash': [''] + 'x-goog-hash': ['crc32c=,md5='] }) ..serve('bar', '1.2.3', headers: { - 'x-goog-hash': ['crc32c=,md5='] + 'x-goog-hash': ['crc32c=loremipsum,md5=loremipsum'] }) ..serve('baz', '1.2.3', headers: { - 'x-goog-hash': ['crc32c=loremipsum,md5=loremipsum'] + 'x-goog-hash': ['crc32c=MTIzNDU=,md5=NTQzMjE='] }); }); - test('when it is empty', () async { + test('when CRC32C checksum is empty', () async { await d.appDir({ 'foo': { 'version': '1.2.3', @@ -131,11 +171,17 @@ void main() { }).create(); await pubGet( - error: contains( - 'Package archive "foo-1.2.3.tar.gz" has a malformed CRC32C checksum in its response headers')); + exitCode: exit_codes.DATA, + error: contains( + 'Package archive "foo-1.2.3.tar.gz" has a malformed CRC32C ' + 'checksum in its response headers'), + environment: { + 'PUB_MAX_HTTP_RETRIES': '1', + }, + ); }); - test('when it is malformed', () async { + test('when CRC32C checksum has bad encoding', () async { await d.appDir({ 'bar': { 'version': '1.2.3', @@ -144,9 +190,17 @@ void main() { }).create(); await pubGet( - error: contains( - 'Package archive "bar-1.2.3.tar.gz" has a malformed CRC32C checksum in its response headers')); + exitCode: exit_codes.DATA, + error: contains( + 'Package archive "bar-1.2.3.tar.gz" has a malformed CRC32C ' + 'checksum in its response headers'), + environment: { + 'PUB_MAX_HTTP_RETRIES': '1', + }, + ); + }); + test('when CRC32C checksum is malformed', () async { await d.appDir({ 'baz': { 'version': '1.2.3', @@ -155,8 +209,14 @@ void main() { }).create(); await pubGet( - error: contains( - 'Package archive "baz-1.2.3.tar.gz" has a malformed CRC32C checksum in its response headers')); + exitCode: exit_codes.DATA, + error: contains( + 'Package archive "baz-1.2.3.tar.gz" has a malformed CRC32C ' + 'checksum in its response headers'), + environment: { + 'PUB_MAX_HTTP_RETRIES': '1', + }, + ); }); }); From 02658975d738261530a7498655fcccc51465f4b7 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 21 Sep 2022 17:45:15 -0700 Subject: [PATCH 19/28] Test CRC32C checksum related retries --- test/get/hosted/get_test.dart | 17 +++++++++++------ test/test_pub.dart | 2 ++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index 1b59c182d..59fa0cf68 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -123,7 +123,8 @@ void main() { ]).validate(); }); - test('recognizes a package with a CRC32C checksum mismatch', () async { + test('recognizes and retries a package with a CRC32C checksum mismatch', + () async { var server = await startPackageServer(); server.serve('foo', '1.2.3', headers: { @@ -140,13 +141,14 @@ void main() { await pubGet( error: contains( 'Package archive "foo-1.2.3.tar.gz" has a CRC32C checksum mismatch'), + silent: contains('Retry #2 because of checksum error'), environment: { - 'PUB_MAX_HTTP_RETRIES': '1', + 'PUB_MAX_HTTP_RETRIES': '2', }, ); }); - group('recognizes bad checksum header', () { + group('recognizes bad checksum header and retries', () { late PackageServer server; setUp(() async { @@ -175,8 +177,9 @@ void main() { error: contains( 'Package archive "foo-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), + silent: contains('Retry #2 because of checksum error'), environment: { - 'PUB_MAX_HTTP_RETRIES': '1', + 'PUB_MAX_HTTP_RETRIES': '2', }, ); }); @@ -194,8 +197,9 @@ void main() { error: contains( 'Package archive "bar-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), + silent: contains('Retry #2 because of checksum error'), environment: { - 'PUB_MAX_HTTP_RETRIES': '1', + 'PUB_MAX_HTTP_RETRIES': '2', }, ); }); @@ -213,8 +217,9 @@ void main() { error: contains( 'Package archive "baz-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), + silent: contains('Retry #2 because of checksum error'), environment: { - 'PUB_MAX_HTTP_RETRIES': '1', + 'PUB_MAX_HTTP_RETRIES': '2', }, ); }); diff --git a/test/test_pub.dart b/test/test_pub.dart index 99f56554f..acf222ac7 100644 --- a/test/test_pub.dart +++ b/test/test_pub.dart @@ -182,6 +182,7 @@ Future pubGet({ Iterable? args, Object? output, Object? error, + Object? silent, Object? warning, int? exitCode, Map? environment, @@ -193,6 +194,7 @@ Future pubGet({ args: args, output: output, error: error, + silent: silent, warning: warning, exitCode: exitCode, environment: environment, From 2656135d1ab37eb42a8527c03415f24c98187837 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 22 Sep 2022 11:13:10 -0700 Subject: [PATCH 20/28] Swap request header line to "accept-encoding" in golden log --- ...is written with --verbose and on unexpected exceptions.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt index 080d95c62..7092c17fa 100644 --- a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt +++ b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt @@ -40,7 +40,7 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] IO : Get package from http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz. [E] IO : Created temp directory $DIR [E] IO : HTTP GET http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz -[E] | cache-control: no-transform +[E] | accept-encoding: gzip [E] | X-Pub-OS: $OS [E] | X-Pub-Command: get [E] | X-Pub-Session-ID: $ID @@ -181,7 +181,7 @@ IO : Get package from http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz MSG : Downloading foo 1.0.0... IO : Created temp directory $DIR IO : HTTP GET http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz - | cache-control: no-transform + | accept-encoding: gzip | X-Pub-OS: $OS | X-Pub-Command: get | X-Pub-Session-ID: $ID From cb5d6eff96e521ce532f56d52d619cca6303b05c Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 22 Sep 2022 11:29:50 -0700 Subject: [PATCH 21/28] Fix CRC32C fine log RegExp in golden log test --- test/embedding/embedding_test.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/embedding/embedding_test.dart b/test/embedding/embedding_test.dart index bd7718bce..a58760904 100644 --- a/test/embedding/embedding_test.dart +++ b/test/embedding/embedding_test.dart @@ -362,8 +362,7 @@ String _filter(String input) { r'x-goog-hash: $CHECKSUM_HEADER', ) .replaceAll( - RegExp( - r'Computed checksum \(\d+\) for "(.+)" with expected CRC32C of \(\d+\)', + RegExp(r'Computed checksum \d+ for "(.+)" with expected CRC32C of \d+', multiLine: true), r'Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C', ) From 6a12e4263acd36f49c6815ad4e03cabce84c67ac Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 22 Sep 2022 16:21:35 -0700 Subject: [PATCH 22/28] Remove explicit Accept-Encoding request header because it is included by default --- lib/src/io.dart | 2 +- lib/src/source/hosted.dart | 19 +++---------------- test/embedding/embedding_test.dart | 5 +++-- ...--verbose and on unexpected exceptions.txt | 6 ++---- 4 files changed, 9 insertions(+), 23 deletions(-) diff --git a/lib/src/io.dart b/lib/src/io.dart index 50f7eb9d3..2b5d0cdc5 100644 --- a/lib/src/io.dart +++ b/lib/src/io.dart @@ -172,7 +172,7 @@ List readBinaryFile(String file) { } /// Reads the contents of the binary file [file] as a [Stream]. -Stream> readBinaryFileAsSream(String file) { +Stream> readBinaryFileAsStream(String file) { log.io('Reading binary file $file.'); var contents = File(file).openRead(); return contents; diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 130b30889..b4a661820 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -868,7 +868,7 @@ class HostedSource extends CachedSource { await retry( // Attempt to download archive and validate its checksum. () async { - final request = _createArchiveRequest(url); + final request = http.Request('GET', url); final response = await withAuthenticatedClient(cache, Uri.parse(description.url), (client) => client.send(request)); final expectedChecksum = _parseCrc32c(response.headers, fileName); @@ -899,7 +899,7 @@ class HostedSource extends CachedSource { ); var tempDir = cache.createTempDir(); - await extractTarGz(readBinaryFileAsSream(archivePath), tempDir); + await extractTarGz(readBinaryFileAsStream(archivePath), tempDir); // Now that the get has succeeded, move it to the real location in the // cache. @@ -911,19 +911,6 @@ class HostedSource extends CachedSource { }); } - /// Creates a GET `Request` for [url] that will ask the host to prevent - /// decompressive transcoding. - /// - /// GCS docmentation indicates that you should do this when you want to - /// validate the object's checksum. - /// - /// See https://cloud.google.com/storage/docs/transcoding#decompressive_transcoding - http.Request _createArchiveRequest(Uri url) { - var request = http.Request('GET', url); - request.headers[io.HttpHeaders.acceptEncodingHeader] = 'gzip'; - return request; - } - /// When an error occurs trying to read something about [package] from [hostedUrl], /// this tries to translate into a more user friendly error message. /// @@ -1165,7 +1152,7 @@ Stream> _validateStream( log.fine( 'Computed checksum $actualChecksum for "$fileName" with expected CRC32C ' - 'of $expectedChecksum'); + 'of $expectedChecksum.'); if (actualChecksum != expectedChecksum) { throw PackageIntegrityException( diff --git a/test/embedding/embedding_test.dart b/test/embedding/embedding_test.dart index a58760904..9fe3e2a4f 100644 --- a/test/embedding/embedding_test.dart +++ b/test/embedding/embedding_test.dart @@ -362,9 +362,10 @@ String _filter(String input) { r'x-goog-hash: $CHECKSUM_HEADER', ) .replaceAll( - RegExp(r'Computed checksum \d+ for "(.+)" with expected CRC32C of \d+', + RegExp( + r'Computed checksum \d+ for "(.+)" with expected CRC32C of \d+\.', multiLine: true), - r'Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C', + r'Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C.', ) /// TODO(sigurdm): This hack suppresses differences in stack-traces diff --git a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt index 7092c17fa..2eee09e16 100644 --- a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt +++ b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt @@ -40,7 +40,6 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] IO : Get package from http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz. [E] IO : Created temp directory $DIR [E] IO : HTTP GET http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz -[E] | accept-encoding: gzip [E] | X-Pub-OS: $OS [E] | X-Pub-Command: get [E] | X-Pub-Session-ID: $ID @@ -58,7 +57,7 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] | x-xss-protection: 1; mode=block [E] | x-content-type-options: nosniff [E] IO : Creating $FILE from stream -[E] FINE: Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C +[E] FINE: Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C. [E] FINE: Created $FILE from stream [E] IO : Created temp directory $DIR [E] IO : Reading binary file $FILE. @@ -181,7 +180,6 @@ IO : Get package from http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz MSG : Downloading foo 1.0.0... IO : Created temp directory $DIR IO : HTTP GET http://localhost:$PORT/packages/foo/versions/1.0.0.tar.gz - | accept-encoding: gzip | X-Pub-OS: $OS | X-Pub-Command: get | X-Pub-Session-ID: $ID @@ -199,7 +197,7 @@ IO : HTTP response 200 OK for GET http://localhost:$PORT/packages/foo/versions/ | x-xss-protection: 1; mode=block | x-content-type-options: nosniff IO : Creating $FILE from stream -FINE: Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C +FINE: Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C. FINE: Created $FILE from stream IO : Created temp directory $DIR IO : Reading binary file $FILE. From 79f5a9be1b8db5bc27a9da75f3ca8399e32e881e Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 26 Sep 2022 19:05:27 -0700 Subject: [PATCH 23/28] Add test for redundant gzip compression --- test/get/hosted/get_test.dart | 50 ++++++++++++++++++++++++++++++----- test/package_server.dart | 13 +++++++-- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index 59fa0cf68..bda64f57e 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -12,7 +12,7 @@ import '../../descriptor.dart' as d; import '../../test_pub.dart'; void main() { - test('gets a package from a pub server and validates CRC32C checksum', + test('gets a package from a pub server and validates its CRC32C checksum', () async { final server = await servePackages(); server.serve('foo', '1.2.3'); @@ -29,7 +29,7 @@ void main() { ]).validate(); }); - group('gets a package from a pub server without validating checksum', () { + group('gets a package from a pub server without validating its checksum', () { late PackageServer server; setUp(() async { @@ -44,7 +44,7 @@ void main() { }); }); - test('because of missing checksum header', () async { + test('because of omitted checksum header', () async { expect(await server.peekArchiveChecksumHeader('foo', '1.2.3'), isNull); await d.appDir({'foo': '1.2.3'}).create(); @@ -164,7 +164,7 @@ void main() { }); }); - test('when CRC32C checksum is empty', () async { + test('when the CRC32C checksum is empty', () async { await d.appDir({ 'foo': { 'version': '1.2.3', @@ -184,7 +184,7 @@ void main() { ); }); - test('when CRC32C checksum has bad encoding', () async { + test('when the CRC32C checksum has bad encoding', () async { await d.appDir({ 'bar': { 'version': '1.2.3', @@ -204,7 +204,7 @@ void main() { ); }); - test('when CRC32C checksum is malformed', () async { + test('when the CRC32C checksum is malformed', () async { await d.appDir({ 'baz': { 'version': '1.2.3', @@ -225,6 +225,44 @@ void main() { }); }); + test('gets a package from a pub server that uses gzip response compression', + () async { + final server = await servePackages(); + server.autoCompress = true; + server.serveChecksums = false; + server.serve('foo', '1.2.3'); + + expect(await server.peekArchiveChecksumHeader('foo', '1.2.3'), isNull); + + await d.appDir({'foo': '1.2.3'}).create(); + + await pubGet(); + + await d.cacheDir({'foo': '1.2.3'}).validate(); + await d.appPackageConfigFile([ + d.packageConfigEntry(name: 'foo', version: '1.2.3'), + ]).validate(); + }); + + test( + 'gets a package from a pub server that uses gzip response compression ' + 'and validates its CRC32C checksum', () async { + final server = await servePackages(); + server.autoCompress = true; + server.serve('foo', '1.2.3'); + + expect(await server.peekArchiveChecksumHeader('foo', '1.2.3'), isNotNull); + + await d.appDir({'foo': '1.2.3'}).create(); + + await pubGet(); + + await d.cacheDir({'foo': '1.2.3'}).validate(); + await d.appPackageConfigFile([ + d.packageConfigEntry(name: 'foo', version: '1.2.3'), + ]).validate(); + }); + group('categorizes dependency types in the lockfile', () { setUp(() async { await servePackages() diff --git a/test/package_server.dart b/test/package_server.dart index 011282052..f7570bbeb 100644 --- a/test/package_server.dart +++ b/test/package_server.dart @@ -21,8 +21,8 @@ import 'descriptor.dart' as d; import 'test_pub.dart'; class PackageServer { - /// The inner [DescriptorServer] that this uses to serve its descriptors. - final shelf.Server _inner; + /// The inner [IOServer] that this uses to serve its descriptors. + final shelf_io.IOServer _inner; /// Handlers of requests. Last matching handler will be used. final List<_PatternAndHandler> _handlers = []; @@ -30,6 +30,12 @@ class PackageServer { // A list of all the requests recieved up till now. final List requestedPaths = []; + // Whether the [IOServer] should compress the content, if possible. + // See [HttpServer.autoCompress] for details. + bool get autoCompress => _inner.server.autoCompress; + set autoCompress(bool shouldAutoCompress) => + _inner.server.autoCompress = shouldAutoCompress; + // Setting this to false will disable automatic calculation of checksums. bool serveChecksums = true; @@ -105,6 +111,9 @@ class PackageServer { for (final packageVersion in package.versions.values) { if (packageVersion.version == version) { final headers = packageVersion.headers ?? {}; + headers[HttpHeaders.contentTypeHeader] ??= [ + 'application/octet-stream' + ]; // This gate enables tests to validate the CRC32C parser by // passing in arbitrary values for the checksum header. From 9b0e5ec4209c8de96ddca0a378a210925d9e0ced Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 26 Sep 2022 19:30:03 -0700 Subject: [PATCH 24/28] Update Content-Type response headers to match Hosted Pub Repository spec --- test/package_server.dart | 32 +++++++++++-------- ...--verbose and on unexpected exceptions.txt | 8 ++--- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/test/package_server.dart b/test/package_server.dart index f7570bbeb..1b053585e 100644 --- a/test/package_server.dart +++ b/test/package_server.dart @@ -75,20 +75,24 @@ class PackageServer { if (package == null) { return shelf.Response.notFound('No package named $name'); } - return shelf.Response.ok(jsonEncode({ - 'name': name, - 'uploaders': ['nweiz@google.com'], - 'versions': package.versions.values - .map((version) => packageVersionApiMap( - server._inner.url.toString(), - version.pubspec, - retracted: version.isRetracted, - )) - .toList(), - if (package.isDiscontinued) 'isDiscontinued': true, - if (package.discontinuedReplacementText != null) - 'replacedBy': package.discontinuedReplacementText, - })); + return shelf.Response.ok( + jsonEncode({ + 'name': name, + 'uploaders': ['nweiz@google.com'], + 'versions': package.versions.values + .map((version) => packageVersionApiMap( + server._inner.url.toString(), + version.pubspec, + retracted: version.isRetracted, + )) + .toList(), + if (package.isDiscontinued) 'isDiscontinued': true, + if (package.discontinuedReplacementText != null) + 'replacedBy': package.discontinuedReplacementText, + }), + headers: { + HttpHeaders.contentTypeHeader: 'application/vnd.pub.v2+json' + }); }, ); diff --git a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt index 2eee09e16..74e05726e 100644 --- a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt +++ b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt @@ -27,7 +27,7 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] | date: $TIME [E] | content-length: 197 [E] | x-frame-options: SAMEORIGIN -[E] | content-type: text/plain; charset=utf-8 +[E] | content-type: application/vnd.pub.v2+json [E] | x-xss-protection: 1; mode=block [E] | x-content-type-options: nosniff [E] IO : Writing $N characters to text file $SANDBOX/cache/hosted/localhost%58$PORT/.cache/foo-versions.json. @@ -53,7 +53,7 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] | transfer-encoding: chunked [E] | x-goog-hash: $CHECKSUM_HEADER [E] | x-frame-options: SAMEORIGIN -[E] | content-type: text/plain; charset=utf-8 +[E] | content-type: application/octet-stream [E] | x-xss-protection: 1; mode=block [E] | x-content-type-options: nosniff [E] IO : Creating $FILE from stream @@ -165,7 +165,7 @@ IO : HTTP response 200 OK for GET http://localhost:$PORT/api/packages/foo | date: $TIME | content-length: 197 | x-frame-options: SAMEORIGIN - | content-type: text/plain; charset=utf-8 + | content-type: application/vnd.pub.v2+json | x-xss-protection: 1; mode=block | x-content-type-options: nosniff IO : Writing $N characters to text file $SANDBOX/cache/hosted/localhost%58$PORT/.cache/foo-versions.json. @@ -193,7 +193,7 @@ IO : HTTP response 200 OK for GET http://localhost:$PORT/packages/foo/versions/ | transfer-encoding: chunked | x-goog-hash: $CHECKSUM_HEADER | x-frame-options: SAMEORIGIN - | content-type: text/plain; charset=utf-8 + | content-type: application/octet-stream | x-xss-protection: 1; mode=block | x-content-type-options: nosniff IO : Creating $FILE from stream From ce3a0ec5fd8598d609b6c4498b20ceb37c6798ae Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 27 Sep 2022 10:54:11 -0700 Subject: [PATCH 25/28] Update lib/src/source/hosted.dart Co-authored-by: Sigurd Meldgaard --- lib/src/source/hosted.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index b4a661820..722f19a03 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -892,7 +892,7 @@ class HostedSource extends CachedSource { onRetry: (e, retryCount) => log.io( 'Retry #${retryCount + 1} because of checksum error with GET $url...'), maxAttempts: math.max( - 1, // Having less than 1 retry is **always** wrong. + 1, // Having less than 1 attempt doesn't make sense. int.tryParse(io.Platform.environment['PUB_MAX_HTTP_RETRIES'] ?? '') ?? 7, ), From 8e44bbbd117814cdd994dd3eb0079ff8e240d1de Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 29 Sep 2022 11:55:01 -0700 Subject: [PATCH 26/28] Update documentation for autoCompress --- test/package_server.dart | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/package_server.dart b/test/package_server.dart index 1b053585e..b7587e73d 100644 --- a/test/package_server.dart +++ b/test/package_server.dart @@ -30,8 +30,9 @@ class PackageServer { // A list of all the requests recieved up till now. final List requestedPaths = []; - // Whether the [IOServer] should compress the content, if possible. - // See [HttpServer.autoCompress] for details. + /// Whether the [IOServer] should compress the content, if possible. + /// The default value is `false` (compression disabled). + /// See [HttpServer.autoCompress] for details. bool get autoCompress => _inner.server.autoCompress; set autoCompress(bool shouldAutoCompress) => _inner.server.autoCompress = shouldAutoCompress; From 0ed432d885dae79a928b1d887748c3798d4318cc Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 29 Sep 2022 11:55:13 -0700 Subject: [PATCH 27/28] Update error message for checksum mismatch --- lib/src/source/hosted.dart | 28 ++++++++++--------- test/embedding/embedding_test.dart | 6 ++-- test/get/hosted/get_test.dart | 6 ++-- ...--verbose and on unexpected exceptions.txt | 4 +-- 4 files changed, 25 insertions(+), 19 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 722f19a03..e296d3d10 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -854,8 +854,8 @@ class HostedSource extends CachedSource { 'Package $packageName has no version $version'); } - var url = versionInfo.archiveUrl; - log.io('Get package from $url.'); + final archiveUrl = versionInfo.archiveUrl; + log.io('Get package from $archiveUrl.'); log.message('Downloading ${log.bold(id.name)} ${id.version}...'); // Download and extract the archive to a temp directory. @@ -868,15 +868,15 @@ class HostedSource extends CachedSource { await retry( // Attempt to download archive and validate its checksum. () async { - final request = http.Request('GET', url); + final request = http.Request('GET', archiveUrl); final response = await withAuthenticatedClient(cache, Uri.parse(description.url), (client) => client.send(request)); final expectedChecksum = _parseCrc32c(response.headers, fileName); Stream> stream = response.stream; if (expectedChecksum != null) { - stream = - _validateStream(response.stream, expectedChecksum, fileName); + stream = _validateStream( + response.stream, expectedChecksum, id, archiveUrl); } // We download the archive to disk instead of streaming it directly @@ -889,8 +889,9 @@ class HostedSource extends CachedSource { // Retry if the checksum response header was malformed or the actual // checksum did not match the expected checksum. retryIf: (e) => e is PackageIntegrityException, - onRetry: (e, retryCount) => log.io( - 'Retry #${retryCount + 1} because of checksum error with GET $url...'), + onRetry: (e, retryCount) => log + .io('Retry #${retryCount + 1} because of checksum error with GET ' + '$archiveUrl...'), maxAttempts: math.max( 1, // Having less than 1 attempt doesn't make sense. int.tryParse(io.Platform.environment['PUB_MAX_HTTP_RETRIES'] ?? '') ?? @@ -1139,8 +1140,8 @@ const checksumHeaderName = 'x-goog-hash'; /// the one present in the checksum response header. /// /// Throws [PackageIntegrityException] if there is a checksum mismatch. -Stream> _validateStream( - Stream> stream, int expectedChecksum, String fileName) async* { +Stream> _validateStream(Stream> stream, + int expectedChecksum, PackageId id, Uri archiveUrl) async* { final crc32c = Crc32c(); await for (final chunk in stream) { @@ -1151,13 +1152,14 @@ Stream> _validateStream( final actualChecksum = crc32c.finalize(); log.fine( - 'Computed checksum $actualChecksum for "$fileName" with expected CRC32C ' - 'of $expectedChecksum.'); + 'Computed checksum $actualChecksum for ${id.name} ${id.version} with ' + 'expected CRC32C of $expectedChecksum.'); if (actualChecksum != expectedChecksum) { throw PackageIntegrityException( - 'Package archive "$fileName" has a CRC32C checksum mismatch', - ); + 'Package archive for ${id.name} ${id.version} downloaded from ' + '"$archiveUrl" has "x-goog-hash: crc32c=$expectedChecksum", which ' + 'doesn\'t match the checksum of the archive downloaded.'); } } diff --git a/test/embedding/embedding_test.dart b/test/embedding/embedding_test.dart index 9fe3e2a4f..9f74405d4 100644 --- a/test/embedding/embedding_test.dart +++ b/test/embedding/embedding_test.dart @@ -363,9 +363,11 @@ String _filter(String input) { ) .replaceAll( RegExp( - r'Computed checksum \d+ for "(.+)" with expected CRC32C of \d+\.', + r'Computed checksum \d+ for foo 1.0.0 with expected CRC32C of ' + r'\d+\.', multiLine: true), - r'Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C.', + r'Computed checksum $CRC32C for foo 1.0.0 with expected CRC32C of ' + r'$CRC32C.', ) /// TODO(sigurdm): This hack suppresses differences in stack-traces diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index bda64f57e..81d484558 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -139,8 +139,10 @@ void main() { }).create(); await pubGet( - error: contains( - 'Package archive "foo-1.2.3.tar.gz" has a CRC32C checksum mismatch'), + error: RegExp( + r'''Package archive for foo 1.2.3 downloaded from "(.+)" has ''' + r'''"x-goog-hash: crc32c=(\d+)", which doesn't match the checksum ''' + r'''of the archive downloaded\.'''), silent: contains('Retry #2 because of checksum error'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', diff --git a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt index 74e05726e..dc09988f5 100644 --- a/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt +++ b/test/testdata/goldens/embedding/embedding_test/logfile is written with --verbose and on unexpected exceptions.txt @@ -57,7 +57,7 @@ MSG : Logs written to $SANDBOX/cache/log/pub_log.txt. [E] | x-xss-protection: 1; mode=block [E] | x-content-type-options: nosniff [E] IO : Creating $FILE from stream -[E] FINE: Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C. +[E] FINE: Computed checksum $CRC32C for foo 1.0.0 with expected CRC32C of $CRC32C. [E] FINE: Created $FILE from stream [E] IO : Created temp directory $DIR [E] IO : Reading binary file $FILE. @@ -197,7 +197,7 @@ IO : HTTP response 200 OK for GET http://localhost:$PORT/packages/foo/versions/ | x-xss-protection: 1; mode=block | x-content-type-options: nosniff IO : Creating $FILE from stream -FINE: Computed checksum $CRC32C for $FILENAME with expected CRC32C of $CRC32C. +FINE: Computed checksum $CRC32C for foo 1.0.0 with expected CRC32C of $CRC32C. FINE: Created $FILE from stream IO : Created temp directory $DIR IO : Reading binary file $FILE. From 63e5f29bf38ee28b349ca954a7271f7e8aed796d Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 30 Sep 2022 09:21:04 -0700 Subject: [PATCH 28/28] Add visibleForTesting annotation to checksumHeaderName --- lib/src/source/hosted.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index e296d3d10..de90a64dd 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -11,6 +11,7 @@ import 'dart:typed_data'; import 'package:collection/collection.dart' show maxBy, IterableNullableExtension; import 'package:http/http.dart' as http; +import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; import 'package:stack_trace/stack_trace.dart'; @@ -1130,6 +1131,7 @@ class _RefAndCache { bool operator ==(Object other) => other is _RefAndCache && other.ref == ref; } +@visibleForTesting const checksumHeaderName = 'x-goog-hash'; /// Adds a checksum validation "tap" to the response stream and returns a