fix(api): rewrite the pubspec inside the archive too, not only the metadata

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/<host>/<package>-<version>/`, 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

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/<host>/<package>-<version>/`, 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/<host>/.cache/<package>-versions.json`; a consumer that

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<List<int>?> _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<String> _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<List<int>> _readTarball(String name, String version) async {

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/<host>/<package>-<version>/`, 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<int>? rewriteArchive(
+    List<int> 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<int>);
+    } 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<int>? _deterministicGzip(List<int> 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<String> _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<String, dynamic>? _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);
+}

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"