From 21aa39539c8d68f7f28a0badc2f4193078d8c0d9 Mon Sep 17 00:00:00 2001 From: greymag Date: Tue, 8 Sep 2026 16:18:11 +0200 Subject: [PATCH] fix(api): rewrite the pubspec inside the archive too, not only the metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3.6.0 rewrote the repository url in the metadata and stopped there, which fixed exactly one `pub get` per cleared cache. `dart pub` reads a hosted package's dependencies from the version listing only while that package is not yet in the local cache; once it has been extracted into `$PUB_CACHE/hosted//-/`, every later solve reads that copy instead. So the first resolution succeeded on the rewritten listing and the next one failed with the same source conflict, off the `pubspec.yaml` pub had kept. Proved rather than reasoned about: with one PUB_CACHE, run 1 exits 0 and run 2 exits 1 with the conflict; patching only the extracted pubspec makes run 3 exit 0. The acceptance test that shipped with 3.6.0 ran `pub get` once, and asserted the extracted archive still named the old address — reading the cause as evidence of harmlessness. It now runs twice, which is the run that could have caught this. So the archive is rewritten on its way out as well. The stored file is never touched; the bytes are transformed per request, and only the one url differs — comments, quoting, key order, file modes and timestamps are all carried through, because the pubspec text is edited at the source spans `package:yaml` reports rather than re-serialised from a parsed document. Nothing is unpacked unless the stored metadata says a rewrite applies, so a repository where nothing was published under an old address pays nothing. The gzip output is made deterministic by hand. `GZipEncoder` stamps `DateTime.now()` into the header's MTIME field, so the same archive encoded two seconds apart hashes differently — and pub records that hash in `pubspec.lock` and checks the cached copy against it. Every clean-cache resolve would have reported the lockfile hash as stale and rewritten it, and `--enforce-lockfile` would simply have failed. The field is zeroed, which is what RFC 1952 reserves for "no timestamp" and what `gzip -n` writes. The first determinism test passed only because both encodes landed in the same second; there is now one asserting the field itself. What consumers pay, once: the archive they receive is no longer byte-identical to the one they received before, so a cache already holding the old copy is not repaired — pub does not re-download what it already has. One `dart pub cache clean` each, after which `pub get` reports the hash in `pubspec.lock` is out of date, updates it, and succeeds. `pubspec.lock` does not need deleting and it does not have to be done twice. A package store that redirects to its own download urls cannot be rewritten this way, the bytes never passing through here. That would silently reproduce the bug this fixes, so the server says so once per package instead. Co-Authored-By: Claude Opus 5 (1M context) --- unpub/CHANGELOG.md | 5 + unpub/README.md | 45 ++- unpub/lib/src/app.dart | 68 ++++- unpub/lib/src/hosted_url_compat.dart | 213 +++++++++++++- unpub/pubspec.yaml | 2 +- .../test/hosted_url_compat_pub_get_test.dart | 105 +++++-- unpub/test/hosted_url_compat_test.dart | 268 ++++++++++++++++++ 7 files changed, 675 insertions(+), 31 deletions(-) diff --git a/unpub/CHANGELOG.md b/unpub/CHANGELOG.md index f9a2528..8ff45b5 100644 --- a/unpub/CHANGELOG.md +++ b/unpub/CHANGELOG.md @@ -1,3 +1,8 @@ +## 3.6.1 + +### Fixed +- `--legacy-hosted-url-rewrite` now rewrites the `pubspec.yaml` inside the package archive as well as the metadata, because the metadata alone fixed exactly one `pub get` per cleared cache. `dart pub` reads a hosted package's dependencies from the version listing only while that package is not yet in the local cache; once it has been extracted into `$PUB_CACHE/hosted//-/`, every later solve reads that copy instead — so the first resolution succeeded and the next one failed with the source conflict again. The stored archive is still never touched: the bytes are transformed on their way out, only that one url differs, and comments, quoting, key order, file modes and timestamps are carried through. The output is a deterministic function of the input, so the content hash does not move between requests or restarts, and an archive naming no old address is streamed straight through without being unpacked. What does change is that the archive a client receives is no longer byte-identical to the one it received before, so its content hash differs: each consumer needs one `dart pub cache clean`, after which `pub get` reports the hash in `pubspec.lock` is out of date, updates it, and succeeds. A cache still holding the old copy is not repaired, since pub does not re-download what it already has. A package store that redirects to its own download urls cannot be rewritten this way; the server warns once per package when that happens. + ## 3.6.0 ### Added diff --git a/unpub/README.md b/unpub/README.md index e7435be..c28438a 100644 --- a/unpub/README.md +++ b/unpub/README.md @@ -389,7 +389,15 @@ consumer, or serve the old metadata under the new address. The server can do the third, when asked. Switched on, it looks at every repository API answer it sends over https: a dependency naming the *same* -address over plain http is served as https instead. +address over plain http is served as https instead — and it does the same to +the `pubspec.yaml` inside the archive on its way out. + +Both halves are needed, and the second is not obvious. `dart pub` reads a +hosted package's dependencies from the version listing **only while that +package is not yet in the local cache**; once it has been extracted into +`$PUB_CACHE/hosted//-/`, every later solve reads that +copy instead. Rewriting the metadata alone therefore fixes exactly one +`pub get` per cleared cache and then the conflict comes back. ```sh dart pub global run in_pub --proxy-origin https://pub.example.org \ @@ -410,11 +418,30 @@ Pub compatibility rewrite: package=innim_iap_google_apple version=1.0.0 dependency=innim_lib from=http://pub.example.org to=https://pub.example.org ``` -Nothing stored changes. Archives keep the `pubspec.yaml` they were published -with, their content hashes stay valid, no version number moves, and switching -the layer off again restores the previous answers exactly — there is nothing -to migrate back. It also does not excuse a bad publish: new versions should -name the https address, and one that already does is left untouched. +Nothing stored changes: the archive on disk keeps the `pubspec.yaml` it was +published with, no version number moves, and switching the layer off restores +the previous answers exactly — there is nothing to migrate back. Inside the +archive only that one url differs; comments, quoting, key order, file modes +and timestamps are all carried through, and the output is a deterministic +function of the input, so the bytes a client receives do not move between +requests or restarts. It also does not excuse a bad publish: new versions +should name the https address, and one that already does is served untouched, +without being unpacked at all. + +### What consumers have to do once + +Because the archive a client downloads now differs from the one it downloaded +before, its content hash differs too, and a cache already holding the old copy +is not repaired — pub does not re-download what it already has. Each consumer +needs one: + +```sh +dart pub cache clean +``` + +The next `pub get` reports that the hash in `pubspec.lock` is out of date, +updates it, and succeeds. `pubspec.lock` does not need deleting, and it does +not have to be done again. Only this repository's own address is rewritten. A dependency on another hosted repository, on `git`, `path` or an sdk, is served as published, and the @@ -431,6 +458,12 @@ behind a TLS-terminating proxy means `--proxy-origin` has to be set — the same setting the archive urls already depend on. Without it the server sees its own plain-http address, finds no https counterpart, and rewrites nothing. +A package store that hands out its own download urls (`supportsDownloadUrl`, +as an object store would) is a redirect: the bytes never pass through this +server, so the pubspec inside cannot be rewritten and the layer can only fix +the first resolution. The server says so, once per package, when it happens. +The built-in `FileStore` streams through and is unaffected. + Rewriting happens on the way out, and this server holds no metadata cache, so there is nothing to invalidate. The pub client does keep one, at `$PUB_CACHE/hosted//.cache/-versions.json`; a consumer that diff --git a/unpub/lib/src/app.dart b/unpub/lib/src/app.dart index 0bd2336..ad0c01c 100644 --- a/unpub/lib/src/app.dart +++ b/unpub/lib/src/app.dart @@ -563,14 +563,76 @@ class App { } if (packageStore.supportsDownloadUrl) { + // Nothing passes through this server on that path, so the pubspec + // inside cannot be rewritten. Said once, because the resulting failure + // — a `pub get` that works on a clean cache and not afterwards — gives + // no hint of its cause. + _warnArchiveRewriteUnavailable(name, version, req); return shelf.Response.found( await packageStore.downloadUrl(name, version)); - } else { + } + + var rewritten = await _rewrittenArchive(name, version, req); + if (rewritten != null) { return shelf.Response.ok( - packageStore.download(name, version), - headers: {HttpHeaders.contentTypeHeader: ContentType.binary.mimeType}, + rewritten, + headers: { + HttpHeaders.contentTypeHeader: ContentType.binary.mimeType, + // Known in full, unlike the streamed answer below. + HttpHeaders.contentLengthHeader: '${rewritten.length}', + }, ); } + + return shelf.Response.ok( + packageStore.download(name, version), + headers: {HttpHeaders.contentTypeHeader: ContentType.binary.mimeType}, + ); + } + + /// The archive with the `pubspec.yaml` inside it named at this server's + /// current address, or null when it needs no change and should be streamed + /// straight through. + /// + /// Rewriting the metadata is not enough on its own: pub reads a hosted + /// package's dependencies from the version listing only until it has the + /// package extracted in its cache, and from that copy afterwards. See + /// [HostedUrlCompat.rewriteArchive]. + /// + /// The stored pubspec decides whether to bother. It is the same content as + /// the one in the archive, and reading it costs a lookup this request has + /// already done — so a repository where nothing was published under an old + /// address never unpacks a single tarball. + Future?> _rewrittenArchive( + String name, String version, shelf.Request req) async { + if (!hostedUrlCompat.enabled) return null; + var package = await metaStore.queryPackage(name); + var stored = + package?.versions.firstWhereOrNull((v) => v.version == version); + if (stored == null) return null; + var canonical = _selfUri(req); + if (identical(hostedUrlCompat.rewrite(stored.pubspec, canonical: canonical), + stored.pubspec)) { + return null; + } + return hostedUrlCompat.rewriteArchive(await _readTarball(name, version), + canonical: canonical, package: name, version: version); + } + + /// Packages already reported as unrewritable, so a repeated `pub get` does + /// not repeat the warning. + final Set _redirectWarned = {}; + + void _warnArchiveRewriteUnavailable( + String name, String version, shelf.Request req) { + if (!hostedUrlCompat.enabled) return; + if (!_redirectWarned.add('$name $version')) return; + if (_redirectWarned.length > 1000) _redirectWarned.clear(); + print('Warning: $name $version is served by a redirect to the package ' + 'store, so the pubspec inside its archive cannot be rewritten. ' + 'A consumer will resolve it once on a clean cache and fail on every ' + 'run after that. Use a package store this server streams through, or ' + 'republish the affected versions.'); } Future> _readTarball(String name, String version) async { diff --git a/unpub/lib/src/hosted_url_compat.dart b/unpub/lib/src/hosted_url_compat.dart index 6568c1d..cf5432a 100644 --- a/unpub/lib/src/hosted_url_compat.dart +++ b/unpub/lib/src/hosted_url_compat.dart @@ -1,4 +1,9 @@ +import 'dart:convert'; + +import 'package:archive/archive.dart'; +import 'package:collection/collection.dart' show IterableExtension; import 'package:logging/logging.dart'; +import 'package:yaml/yaml.dart'; /// Rewrites the repository urls that a *published* pubspec names, so that /// packages published under an earlier address of this repository resolve @@ -107,10 +112,7 @@ class HostedUrlCompat { }) { if (!enabled) return pubspec; - final targets = _identitiesFor == canonical - ? _identities! - : (_identities = _legacyIdentities(canonical)); - _identitiesFor = canonical; + final targets = _targetsFor(canonical); if (targets.isEmpty) return pubspec; final replacement = _identity(canonical); @@ -139,6 +141,198 @@ class HostedUrlCompat { return result ?? pubspec; } + /// [archive] — a published `.tar.gz` — with the `pubspec.yaml` inside it + /// rewritten the same way [rewrite] rewrites the metadata. Null when + /// nothing in it names an old address, which is the answer for everything + /// published since the move. + /// + /// This exists because rewriting the metadata alone fixes exactly one + /// resolution. `dart pub` reads a hosted package's dependencies from the + /// version listing only while that package is not yet in the local cache; + /// once it has been extracted into + /// `$PUB_CACHE/hosted//-/`, every later solve reads + /// *that* directory's `pubspec.yaml` instead. So a `pub get` on a clean + /// cache succeeded and the very next one failed with the conflict again. + /// The copy the client keeps has to name the current address too. + /// + /// The stored archive is never touched — this transforms the bytes on their + /// way out — but it does change what the client receives, and therefore the + /// content hash it records in `pubspec.lock`. That is the price, and it is + /// paid once: pub reports the hash it had is out of date, updates it, and + /// is quiet from then on. The output is a deterministic function of the + /// input, so the hash does not move between requests or restarts. + List? rewriteArchive( + List archive, { + required Uri canonical, + String? package, + String? version, + }) { + if (!enabled || _targetsFor(canonical).isEmpty) return null; + + final Archive decoded; + try { + decoded = TarDecoder().decodeBytes(GZipDecoder().decodeBytes(archive)); + } catch (error) { + // Not this layer's business to reject an archive: whatever is stored is + // what was published, and a client that can read it should keep getting + // it. Served untouched, with a line saying why. + _log.warning('Could not read the archive of $package $version to ' + 'rewrite its pubspec; serving it unchanged. $error'); + return null; + } + + // `dart pub publish` puts the pubspec at the root. `./pubspec.yaml` is + // accepted too, since not every archive in a repository this old was + // necessarily written by the same tool. + final pubspecFile = decoded.files.firstWhereOrNull( + (f) => f.name == 'pubspec.yaml' || f.name == './pubspec.yaml'); + if (pubspecFile == null) return null; + + final String source; + try { + source = utf8.decode(pubspecFile.content as List); + } on FormatException catch (error) { + _log.warning('The pubspec of $package $version is not valid UTF-8; ' + 'serving the archive unchanged. $error'); + return null; + } + + final rewritten = rewritePubspecYaml(source, + canonical: canonical, package: package, version: version); + if (rewritten == null) return null; + + final bytes = utf8.encode(rewritten); + final result = Archive(); + for (final file in decoded.files) { + if (!identical(file, pubspecFile)) { + result.addFile(file); + continue; + } + // Mode and timestamp carried over, so the only difference between the + // archive that was published and the one served is the url. + result.addFile(ArchiveFile(file.name, bytes.length, bytes) + ..mode = file.mode + ..lastModTime = file.lastModTime); + } + return _deterministicGzip(TarEncoder().encode(result)); + } + + /// [tar] gzipped with the timestamp left out of the header. + /// + /// `GZipEncoder` stamps `DateTime.now()` into the gzip MTIME field, so the + /// same archive encoded two seconds apart comes out with a different + /// SHA-256 — and pub records that hash in `pubspec.lock` and checks the + /// cached copy against it. Every clean-cache resolve would report the hash + /// as out of date and rewrite the lockfile, and `--enforce-lockfile` would + /// simply fail. Zero is what RFC 1952 reserves for "no timestamp", which is + /// what `gzip -n` writes and what every reader ignores. + static List? _deterministicGzip(List tar) { + final bytes = GZipEncoder().encode(tar); + if (bytes == null || bytes.length < 8) return bytes; + // Only on something that is actually gzip, so a future encoder change + // cannot have four unrelated bytes overwritten. + if (bytes[0] != 0x1f || bytes[1] != 0x8b) return bytes; + for (var i = 4; i < 8; i++) { + bytes[i] = 0; + } + return bytes; + } + + /// [yaml] with the hosted urls that name an old address of this repository + /// replaced, or null when there are none. + /// + /// Edits the text in place rather than re-serialising a parsed document: + /// what goes back into the archive is the pubspec its author wrote, with + /// comments, quoting and key order intact and one url different. Round + /// tripping it through a YAML writer would hand the client a file that + /// differs from what was published in ways nobody asked for. + String? rewritePubspecYaml( + String yaml, { + required Uri canonical, + String? package, + String? version, + }) { + if (!enabled) return null; + final targets = _targetsFor(canonical); + if (targets.isEmpty) return null; + final replacement = _identity(canonical); + + final YamlNode document; + try { + document = loadYamlNode(yaml); + } on YamlException catch (error) { + _log.warning('Could not parse the pubspec of $package $version; ' + 'serving it unchanged. $error'); + return null; + } + if (document is! YamlMap) return null; + + // Collected first and applied last-to-first, so replacing one url cannot + // move the offsets of the ones still to come. + final edits = <_UrlEdit>[]; + for (final section in _sections) { + final deps = document.nodes[section]; + if (deps is! YamlMap) continue; + for (final entry in deps.nodes.entries) { + final name = entry.key; + final dependency = name is YamlScalar ? name.value : name; + if (dependency is! String) continue; + final spec = entry.value; + if (spec is! YamlMap) continue; + if (spec.containsKey('path') || + spec.containsKey('git') || + spec.containsKey('sdk')) { + continue; + } + final hosted = spec.nodes['hosted']; + YamlNode? urlNode; + if (hosted is YamlScalar) { + urlNode = hosted; + } else if (hosted is YamlMap) { + final url = hosted.nodes['url']; + if (url is YamlScalar) urlNode = url; + } + final from = urlNode?.value; + if (urlNode == null || from is! String) continue; + final to = _rewriteUrl(from, targets, replacement); + if (to == null) continue; + edits.add(_UrlEdit(urlNode.span.start.offset, urlNode.span.end.offset, + from, to, dependency)); + } + } + if (edits.isEmpty) return null; + + edits.sort((a, b) => b.start.compareTo(a.start)); + var result = yaml; + for (final edit in edits) { + final slice = result.substring(edit.start, edit.end); + // The span is the scalar and nothing else, so the url is in it — unless + // it was written with escapes, in which case leaving the file alone + // beats guessing at its spelling. + if (!slice.contains(edit.from)) continue; + result = result.replaceRange( + edit.start, edit.end, slice.replaceFirst(edit.from, edit.to)); + _report( + package: package, + version: version, + dependency: edit.dependency, + from: edit.from, + to: edit.to); + } + return result == yaml ? null : result; + } + + /// The addresses to look for when answering on [canonical], memoised: the + /// answer depends on nothing else, and both entry points ask for it per + /// published version. + List _targetsFor(Uri canonical) { + if (_identitiesFor != canonical) { + _identities = _legacyIdentities(canonical); + _identitiesFor = canonical; + } + return _identities!; + } + /// The rewritten form of one dependency entry, or null to leave it alone. Map? _rewriteDependency( dynamic spec, @@ -307,3 +501,14 @@ class HostedUrlCompat { 'dependency=$dependency from=$from to=$to'); } } + +/// One hosted url found in a pubspec's text, and what it should say. +class _UrlEdit { + final int start; + final int end; + final String from; + final String to; + final String dependency; + + _UrlEdit(this.start, this.end, this.from, this.to, this.dependency); +} diff --git a/unpub/pubspec.yaml b/unpub/pubspec.yaml index 0f6e6dd..120c85b 100644 --- a/unpub/pubspec.yaml +++ b/unpub/pubspec.yaml @@ -1,6 +1,6 @@ name: in_pub description: Self-hosted private Dart Pub server for Enterprise, with a simple web interface to search and view packages information. -version: 3.6.0 +version: 3.6.1 homepage: https://github.com/Innim/in_pub environment: sdk: ">=3.0.0 <4.0.0" diff --git a/unpub/test/hosted_url_compat_pub_get_test.dart b/unpub/test/hosted_url_compat_pub_get_test.dart index 06131c7..4acec6e 100644 --- a/unpub/test/hosted_url_compat_pub_get_test.dart +++ b/unpub/test/hosted_url_compat_pub_get_test.dart @@ -2,6 +2,8 @@ import 'dart:convert'; import 'dart:io'; import 'package:archive/archive.dart'; +import 'package:crypto/crypto.dart'; +import 'package:http/http.dart' as http; import 'package:in_pub/in_pub.dart'; import 'package:path/path.dart' as path; import 'package:test/test.dart'; @@ -79,7 +81,7 @@ void main() { /// Starts the repository with [compat] and returns the address it answers /// on. Both stores are rebuilt each time so a test cannot see another's /// leftovers. - Future serve(HostedUrlCompat compat) async { + Future serveOn(int port, HostedUrlCompat compat) async { var meta = _MemoryMetaStore(); // Published today, against the current address. await publish(meta, 'package_b', pubspecYaml('package_b', '1.0.0')); @@ -88,7 +90,7 @@ void main() { metaStore: meta, packageStore: FileStore(packages.path), hostedUrlCompat: compat, - ).serve('127.0.0.1', 0); + ).serve('127.0.0.1', port); var url = 'http://localhost:${server.port}'; // Published long ago, against the address this repository has since left. @@ -101,10 +103,12 @@ void main() { return url; } + Future serve(HostedUrlCompat compat) => serveOn(0, compat); + /// Runs `dart pub get` in [work] against the server, with a cache of its /// own so no test can be answered from another's. - Future pubGet() async { - var cache = Directory(path.join(work.path, 'pub-cache')) + Future pubGet({String cacheName = 'pub-cache'}) async { + var cache = Directory(path.join(work.path, cacheName)) ..createSync(recursive: true); return Process.run( Platform.resolvedExecutable, @@ -160,33 +164,100 @@ dependencies: reason: 'the old address is what the solver cannot reconcile'); }, timeout: const Timeout(Duration(minutes: 2))); - test('with the rewrite, the same graph resolves', () async { + test('with the rewrite, the same graph resolves — and keeps resolving', + () async { hostedUrl = await serve(HostedUrlCompat(legacyUrls: [legacy])); writeApp(); - var result = await pubGet(); + var first = await pubGet(); + expect(first.exitCode, 0, + reason: 'stdout: ${first.stdout}\nstderr: ${first.stderr}'); - expect(result.exitCode, 0, - reason: 'stdout: ${result.stdout}\nstderr: ${result.stderr}'); + // The run that matters, and the one this test used to be missing. Pub + // reads a hosted package's dependencies from the version listing only + // while the package is not yet in its cache; from the second run on it + // reads the copy it extracted. A rewrite that stops at the metadata + // passes the first run and fails here, which is exactly how it reached + // production looking correct. + var second = await pubGet(); + expect(second.exitCode, 0, + reason: 'a second `pub get` on a warm cache must resolve too.\n' + 'stdout: ${second.stdout}\nstderr: ${second.stderr}'); var lock = File(path.join(work.path, 'pubspec.lock')).readAsStringSync(); expect(lock, contains('package_a')); expect(lock, contains('package_b')); expect(lock, isNot(contains(legacy.host)), reason: 'nothing may still be resolved against the old address'); - // The archive still holds the pubspec as it was published: the rewrite - // never touched it, and pub extracted it unchanged. - var cached = Directory(path.join(work.path, 'pub-cache', 'hosted')); - var extracted = cached + + // Which is only true because the copy pub kept names this server. + expect(_cachedPubspec(work, 'package_a'), isNot(contains(legacy.host))); + expect(_cachedPubspec(work, 'package_a'), contains(hostedUrl)); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('the archive is byte-identical on every request', () async { + // A moving content hash would be worse than the problem it fixes: pub + // records one in `pubspec.lock` and checks the cached copy against it. + hostedUrl = await serve(HostedUrlCompat(legacyUrls: [legacy])); + var url = Uri.parse('$hostedUrl/packages/package_a/versions/1.0.0.tar.gz'); + + var first = await http.readBytes(url); + var second = await http.readBytes(url); + + expect(sha256.convert(first), sha256.convert(second)); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('a package published since the move is served untouched', () async { + hostedUrl = await serve(HostedUrlCompat(legacyUrls: [legacy])); + + var served = await http.readBytes( + Uri.parse('$hostedUrl/packages/package_b/versions/1.0.0.tar.gz')); + var stored = File(path.join(packages.path, 'package_b-1.0.0.tar.gz')) + .readAsBytesSync(); + + expect(served, stored, + reason: 'nothing in it names an old address, so nothing is unpacked'); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('a cache holding the old copy is not repaired without clearing it', + () async { + // The state every consumer is in today: the package was extracted while + // the repository still served the archive as published, so the copy pub + // keeps names the old address. Constructed by putting that copy back, + // which is exactly what is on their disk. + hostedUrl = await serve(HostedUrlCompat(legacyUrls: [legacy])); + writeApp(); + expect((await pubGet()).exitCode, 0); + + var cached = _cachedPubspecFile(work, 'package_a'); + cached.writeAsStringSync( + cached.readAsStringSync().replaceAll(hostedUrl, legacy.toString())); + + var stale = await pubGet(); + expect(stale.exitCode, isNot(0), + reason: 'pub does not re-download what it already has, so the old ' + 'extracted pubspec goes on naming the old address'); + + // Clearing the cache is the one thing a consumer has to do by hand, and + // then it holds — rather than working for exactly one run, which is what + // rewriting only the metadata bought. + expect((await pubGet(cacheName: 'pub-cache-2')).exitCode, 0); + expect((await pubGet(cacheName: 'pub-cache-2')).exitCode, 0); + }, timeout: const Timeout(Duration(minutes: 3))); +} + +/// The `pubspec.yaml` pub extracted for [name], which is what every solve +/// after the first one reads. +String _cachedPubspec(Directory work, String name) => + _cachedPubspecFile(work, name).readAsStringSync(); + +File _cachedPubspecFile(Directory work, String name) => + Directory(path.join(work.path, 'pub-cache', 'hosted')) .listSync(recursive: true) .whereType() .firstWhere((f) => path.basename(f.path) == 'pubspec.yaml' && - path.basename(f.parent.path).startsWith('package_a-')); - expect(extracted.readAsStringSync(), contains(legacy.toString()), - reason: 'the published archive is left exactly as it was'); - }, timeout: const Timeout(Duration(minutes: 2))); -} + path.basename(f.parent.path).startsWith('$name-')); Map _loadYaml(String yaml) { // Only what these fixtures write, so the test does not depend on a yaml diff --git a/unpub/test/hosted_url_compat_test.dart b/unpub/test/hosted_url_compat_test.dart index 70a71ca..fed0567 100644 --- a/unpub/test/hosted_url_compat_test.dart +++ b/unpub/test/hosted_url_compat_test.dart @@ -1,3 +1,6 @@ +import 'dart:convert'; + +import 'package:archive/archive.dart'; import 'package:in_pub/in_pub.dart'; import 'package:test/test.dart'; @@ -344,6 +347,9 @@ void main() { }); }); + _yamlGroup(); + _archiveGroup(); + group('the layer as a whole', () { test('applying it twice changes nothing the second time', () { var pubspec = one('innim_lib', { @@ -400,3 +406,265 @@ void main() { }); }); } + +/// The text-level rewrite, which is what goes back into the archive. +/// +/// Separate from the map rewrite above because it has a different obligation: +/// the map only has to be correct, this also has to hand back the file its +/// author wrote — comments, quoting and key order intact, one url different. +void _yamlGroup() { + final canonical = Uri.parse('https://pub.example.org'); + final compat = HostedUrlCompat(); + + String? rewrite(String yaml, {HostedUrlCompat? using}) => + (using ?? compat).rewritePubspecYaml(yaml, canonical: canonical); + + group('the pubspec text', () { + test('an old url is replaced where it stands', () { + var result = rewrite(''' +name: package_a +dependencies: + innim_lib: + hosted: http://pub.example.org + version: ^1.0.0 +'''); + + expect(result, ''' +name: package_a +dependencies: + innim_lib: + hosted: https://pub.example.org + version: ^1.0.0 +'''); + }); + + test('comments, blank lines and key order survive', () { + var result = rewrite(''' +name: package_a +# The library everything here is built on. +dependencies: + innim_lib: + version: ^1.0.0 # pinned deliberately + hosted: http://pub.example.org + +dev_dependencies: + test: ^1.0.0 +'''); + + expect(result, ''' +name: package_a +# The library everything here is built on. +dependencies: + innim_lib: + version: ^1.0.0 # pinned deliberately + hosted: https://pub.example.org + +dev_dependencies: + test: ^1.0.0 +'''); + }); + + test('a quoted url keeps its quotes', () { + var result = rewrite(''' +dependencies: + innim_lib: + hosted: "http://pub.example.org" +'''); + + expect(result, contains('hosted: "https://pub.example.org"')); + }); + + test('the structured form is rewritten in place', () { + var result = rewrite(''' +dependencies: + innim_lib: + hosted: + name: innim_lib + url: http://pub.example.org + version: ^1.0.0 +'''); + + expect(result, contains(' url: https://pub.example.org')); + expect(result, contains(' name: innim_lib')); + }); + + test('several dependencies in one file are all rewritten', () { + var result = rewrite(''' +dependencies: + a: + hosted: http://pub.example.org + b: + hosted: http://pub.example.org + c: + hosted: https://elsewhere.example.com +dev_dependencies: + d: + hosted: http://pub.example.org +'''); + + expect('https://pub.example.org'.allMatches(result!).length, 3); + expect(result, contains('hosted: https://elsewhere.example.com')); + }); + + test('nothing to change returns null rather than a copy', () { + expect(rewrite(''' +dependencies: + innim_lib: + hosted: https://pub.example.org +'''), isNull); + }); + + test('git, path and sdk dependencies are untouched', () { + expect(rewrite(''' +dependencies: + a: + git: + url: http://pub.example.org/a.git + b: + path: ../b + c: + sdk: flutter +'''), isNull); + }); + + test('a lookalike hostname is untouched', () { + expect(rewrite(''' +dependencies: + a: + hosted: http://pub.example.org.attacker.test +'''), isNull); + }); + + test('a file that is not valid yaml is left alone', () { + expect(rewrite('name: [unclosed\n'), isNull); + }); + + test('switched off, it changes nothing', () { + expect(rewrite(''' +dependencies: + a: + hosted: http://pub.example.org +''', using: HostedUrlCompat.disabled()), isNull); + }); + }); +} + +/// The archive path: what a client actually downloads. +void _archiveGroup() { + final canonical = Uri.parse('https://pub.example.org'); + + List archiveWith(String pubspec, {String library = 'const a = 1;\n'}) { + var archive = Archive(); + void add(String name, String content, {int mode = 420}) { + var bytes = utf8.encode(content); + archive.addFile(ArchiveFile(name, bytes.length, bytes) + ..mode = mode + ..lastModTime = 1600000000); + } + + add('pubspec.yaml', pubspec); + add('lib/a.dart', library); + add('bin/run.sh', '#!/bin/sh\n', mode: 493); + return GZipEncoder().encode(TarEncoder().encode(archive))!; + } + + ArchiveFile fileIn(List bytes, String name) => TarDecoder() + .decodeBytes(GZipDecoder().decodeBytes(bytes)) + .files + .firstWhere((f) => f.name == name); + + const legacyPubspec = ''' +name: package_a +version: 1.0.0 +dependencies: + innim_lib: + hosted: http://pub.example.org + version: ^1.0.0 +'''; + + group('the archive', () { + test('comes back with the pubspec rewritten', () { + var result = HostedUrlCompat() + .rewriteArchive(archiveWith(legacyPubspec), canonical: canonical); + + expect(result, isNotNull); + expect(utf8.decode(fileIn(result!, 'pubspec.yaml').content as List), + contains('hosted: https://pub.example.org')); + }); + + test('every other file is carried through, mode and timestamp included', + () { + var result = HostedUrlCompat() + .rewriteArchive(archiveWith(legacyPubspec), canonical: canonical)!; + + var script = fileIn(result, 'bin/run.sh'); + expect(script.mode, 493, reason: 'an executable bit must survive'); + expect(script.lastModTime, 1600000000); + expect(utf8.decode(fileIn(result, 'lib/a.dart').content as List), + 'const a = 1;\n'); + // The rewritten pubspec keeps its own metadata too. + expect(fileIn(result, 'pubspec.yaml').lastModTime, 1600000000); + }); + + test('the gzip header carries no timestamp', () { + // Without this the bytes differ every second, and the equality check + // below would pass only because both calls landed in the same one. + var result = HostedUrlCompat() + .rewriteArchive(archiveWith(legacyPubspec), canonical: canonical)!; + + expect(result.sublist(0, 2), [0x1f, 0x8b], reason: 'gzip magic'); + expect(result.sublist(4, 8), [0, 0, 0, 0], + reason: 'the MTIME field, which GZipEncoder fills with the clock'); + }); + + test('the same input always produces the same bytes', () { + var compat = HostedUrlCompat(); + var input = archiveWith(legacyPubspec); + + var first = compat.rewriteArchive(input, canonical: canonical)!; + var second = compat.rewriteArchive(input, canonical: canonical)!; + + expect(first, second, + reason: 'pub records a content hash and checks the cached copy ' + 'against it, so a moving one would be worse than the problem ' + 'this fixes'); + }); + + test('an archive naming no old address is left for the caller to stream', + () { + var current = legacyPubspec.replaceAll('http://', 'https://'); + + expect( + HostedUrlCompat() + .rewriteArchive(archiveWith(current), canonical: canonical), + isNull); + }); + + test('an archive with no pubspec is left alone', () { + var archive = Archive(); + var bytes = utf8.encode('nothing here'); + archive.addFile(ArchiveFile('README.md', bytes.length, bytes)); + + expect( + HostedUrlCompat().rewriteArchive( + GZipEncoder().encode(TarEncoder().encode(archive))!, + canonical: canonical), + isNull); + }); + + test('bytes that are not an archive are left alone rather than refused', + () { + expect( + HostedUrlCompat().rewriteArchive(utf8.encode('not a tarball'), + canonical: canonical), + isNull); + }); + + test('switched off, it does nothing', () { + expect( + HostedUrlCompat.disabled() + .rewriteArchive(archiveWith(legacyPubspec), canonical: canonical), + isNull); + }); + }); +}