Symlink and duplicate check in pkg/pub_package_reader (#4763)

Author: isoos

app/lib/package/backend.dart

@@ -13,8 +13,6 @@ import 'package:gcloud/service_scope.dart' as ss;
 import 'package:gcloud/storage.dart';
 import 'package:logging/logging.dart';
 import 'package:meta/meta.dart';
-import 'package:pana/pana.dart' show runProc;
-import 'package:path/path.dart' as p;
 import 'package:pool/pool.dart';
 import 'package:pub_package_reader/pub_package_reader.dart';
 import 'package:pub_semver/pub_semver.dart';
@@ -677,8 +675,6 @@ class PackageBackend {
       if (archive.hasIssues) {
         throw PackageRejectedException(archive.issues.first.message);
       }
-      // TODO: move this to pkg/pub_package_reader
-      await verifyTarGzSymlinks(filename);
 
       final pubspec = Pubspec.fromYaml(archive.pubspecContent);
       PackageRejectedException.check(await nameTracker.accept(pubspec.name),
@@ -1133,43 +1129,6 @@ Future _saveTarballToFS(Stream<List<int>> data, String filename) async {
   _logger.info('Finished streaming tarball to FS.');
 }
 
-// Throws [PackageRejectedException] if the archive is not readable or if there
-// is any symlink in the archive.
-@visibleForTesting
-Future<void> verifyTarGzSymlinks(String filename) async {
-  Future<List<String>> listFiles(bool verbose) async {
-    final pr = await runProc(
-      ['tar', verbose ? '-tvf' : '-tf', filename],
-    );
-    if (pr.exitCode != 0) {
-      _logger.info('Rejecting package: tar returned with ${pr.exitCode}\n'
-          '${pr.stdout}\n${pr.stderr}');
-      throw PackageRejectedException.invalidTarGz();
-    }
-    return pr.stdout.toString().split('\n');
-  }
-
-  final fileNames = (await listFiles(false)).toSet();
-  final verboseLines = await listFiles(true);
-  // Check if the file has any symlink.
-  for (final line in verboseLines) {
-    if (line.startsWith('l')) {
-      // report only the source path
-      // if output is non-standard for any reason, this reports the full line
-      final parts = line.split(' -> ');
-      final source = parts.first.split(' ').last;
-      final target = parts.last;
-      if (p.isAbsolute(target)) {
-        throw PackageRejectedException.brokenSymlink(source, target);
-      }
-      final resolvedPath = p.normalize(Uri(path: source).resolve(target).path);
-      if (!fileNames.contains(resolvedPath)) {
-        throw PackageRejectedException.brokenSymlink(source, target);
-      }
-    }
-  }
-}
-
 class _UploadEntities {
   final PackageVersion packageVersion;
   final PackageVersionInfo packageVersionInfo;

app/test/package/upload_test.dart

@@ -3,11 +3,8 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'dart:async';
-import 'dart:io';
 
 import 'package:gcloud/db.dart';
-import 'package:pana/pana.dart';
-import 'package:path/path.dart' as p;
 import 'package:test/test.dart';
 import 'package:yaml/yaml.dart';
 
@@ -507,91 +504,4 @@ void main() {
       timeout: Timeout.factor(1.5),
     );
   });
-
-  group('.tar.gz verification', () {
-    test('invalid file content', () async {
-      await withTempDirectory((tempDir) async {
-        final file = File(p.join(tempDir, 'bad.tar.gz'));
-        await file.writeAsBytes([0, 1, 2]);
-        final rs = verifyTarGzSymlinks(file.path);
-        await expectLater(
-            rs,
-            throwsA(isA<PackageRejectedException>().having(
-              (e) => '$e',
-              'text',
-              contains('is not a valid'),
-            )));
-      });
-    });
-
-    Future<void> runArchiveVerification({
-      Future<void> Function(String pkgDirPath) setupDir,
-      String expectedErrorMessage,
-    }) async {
-      return await withTempDirectory((tempDir) async {
-        final archiveFile = File(p.join(tempDir, 'with-symlink.tar.gz'));
-        final pkgDir = Directory(p.join(tempDir, 'pkg'));
-        await pkgDir.create(recursive: true);
-        await setupDir(pkgDir.path);
-        final pr = await runProc(['tar', '-czvf', archiveFile.path, 'pkg'],
-            workingDirectory: tempDir);
-        expect(pr.exitCode, 0);
-        final rs = verifyTarGzSymlinks(archiveFile.path);
-        if (expectedErrorMessage != null) {
-          await expectLater(
-              rs,
-              throwsA(isA<PackageRejectedException>().having(
-                (e) => '$e',
-                'text',
-                contains(expectedErrorMessage),
-              )));
-        } else {
-          await rs;
-        }
-      });
-    }
-
-    test('has a relative symlink outside of archive', () async {
-      await runArchiveVerification(
-        setupDir: (pkgDir) async {
-          await Link(p.join(pkgDir, 'README.md')).create('../../README.md');
-        },
-        expectedErrorMessage:
-            'Package archive contains a broken symlink: `pkg/README.md` -> `../../README.md`.',
-      );
-    });
-
-    test('has an absolute symlink', () async {
-      await runArchiveVerification(
-        setupDir: (pkgDir) async {
-          await Link(p.join(pkgDir, 'README.md')).create('/README.md');
-        },
-        expectedErrorMessage:
-            'Package archive contains a broken symlink: `pkg/README.md` -> `/README.md`.',
-      );
-    });
-
-    // This test-case should fail, but it for simplicity it is accepted by pub.
-    // TODO: fix verification to detect circular links
-    test('has symlink(s) with circular links', () async {
-      // TODO: this use case should fail instead
-      await runArchiveVerification(
-        setupDir: (pkgDir) async {
-          await Link(p.join(pkgDir, 'README.md')).create('./README.md');
-        },
-      );
-    });
-
-    test('valid symlink to another file', () async {
-      await runArchiveVerification(
-        setupDir: (pkgDir) async {
-          await Directory(p.join(pkgDir, 'a')).create();
-          await Directory(p.join(pkgDir, 'b')).create();
-          await File(p.join(pkgDir, 'a', 'existing.txt')).writeAsString('');
-          await Link(p.join(pkgDir, 'b', 'link.txt'))
-              .create('../a/existing.txt');
-        },
-      );
-    });
-  });
 }

pkg/pub_package_reader/lib/pub_package_reader.dart

@@ -93,6 +93,15 @@ Future<PackageSummary> summarizePackageArchive(
         ArchiveIssue('Failed to scan tar archive. ($e)'));
   }
 
+  // symlinks check
+  final brokenSymlinks = tar.brokenSymlinks();
+  if (brokenSymlinks.isNotEmpty) {
+    final from = brokenSymlinks.keys.first;
+    final to = brokenSymlinks[from];
+    return PackageSummary.fail(ArchiveIssue(
+        'Package archive contains a broken symlink: `$from` -> `$to`.'));
+  }
+
   // processing pubspec.yaml
   final pubspecPath = tar.searchForFile(['pubspec.yaml']);
   if (pubspecPath == null) {

pkg/pub_package_reader/lib/src/tar_utils.dart

@@ -19,20 +19,32 @@ abstract class TarArchive {
   /// Maps the normalized names to their original value;
   final Map<String, String> _normalizedNames;
 
-  TarArchive._(this._normalizedNames)
-      : fileNames = _normalizedNames.keys.toList()..sort();
-
-  TarArchive(List<String> names) : this._(_normalizeNames(names));
+  /// The map of files that are symlinks, using normalized names
+  /// in both parts of the map entries.
+  final Map<String, String> _symlinks;
+
+  TarArchive._(
+    this._normalizedNames,
+    this._symlinks,
+  ) : fileNames = _normalizedNames.keys.toList()..sort();
+
+  TarArchive(List<String> names, Map<String, String> symlinks)
+      : this._(
+          _normalizeNames(names),
+          symlinks.map((key, value) =>
+              MapEntry<String, String>(_normalize(key), _normalize(value))),
+        );
 
   static Map<String, String> _normalizeNames(List<String> names) {
     final files = <String, String>{};
     for (final name in names) {
-      final normalized = p.normalize(name).trim();
-      files[normalized] = name;
+      files[_normalize(name)] = name;
     }
     return files;
   }
 
+  static String _normalize(String path) => p.normalize(path).trim();
+
   /// Reads file content as String.
   Future<String> readContentAsString(String name, {int maxLength = 0});
 
@@ -52,6 +64,24 @@ abstract class TarArchive {
     return null;
   }
 
+  /// Returns the brokens links (that point outside, or to a non-existent file).
+  Map<String, String> brokenSymlinks() {
+    final broken = <String, String>{};
+    for (final from in _symlinks.keys) {
+      final to = _symlinks[from];
+      final toAsUri = Uri.tryParse(to);
+      if (toAsUri == null || toAsUri.isAbsolute) {
+        broken[from] = to;
+        continue;
+      }
+      final resolvedPath = p.normalize(Uri(path: from).resolve(to).path);
+      if (!fileNames.contains(resolvedPath)) {
+        broken[from] = to;
+      }
+    }
+    return broken;
+  }
+
   /// Creates a new instance by scanning the archive at [path].
   static Future<TarArchive> scan(String path, {bool useNative = false}) async {
     return useNative
@@ -64,22 +94,49 @@ abstract class TarArchive {
 class _ProcessTarArchive extends TarArchive {
   final String _path;
 
-  _ProcessTarArchive._(this._path, List<String> names) : super(names);
+  _ProcessTarArchive._(
+    this._path,
+    List<String> names,
+    Map<String, String> symlinks,
+  ) : super(names, symlinks);
 
   /// Creates a new instance by scanning the archive at [path].
   static Future<_ProcessTarArchive> _scan(String path) async {
-    final args = ['-tzf', path];
-    final result = await Process.run('tar', args);
-    if (result.exitCode != 0) {
+    // normal file list
+    final rs1 = await _runTar(['-tzf', path]);
+    final names = <String>{};
+    for (final name in (rs1.stdout as String).split('\n')) {
+      if (!names.add(name)) {
+        throw Exception('Duplicate tar entry: `$name`.');
+      }
+    }
+
+    // symlink file list
+    final symlinks = <String, String>{};
+    final rs2 = await _runTar(['-tvzf', path]);
+    for (final line in (rs2.stdout as String).split('\n')) {
+      if (line.startsWith('l')) {
+        final parts = line.split(' -> ');
+        if (parts.length != 2) {
+          throw Exception('Unable to parse symlinks: $line');
+        }
+        symlinks[parts.first.trim().split(' ').last] = parts.last.trim();
+      }
+    }
+
+    return _ProcessTarArchive._(path, names.toList(), symlinks);
+  }
+
+  static Future<ProcessResult> _runTar(List<String> args) async {
+    final rs = await Process.run('tar', args);
+    if (rs.exitCode != 0) {
       _logger.warning('The "tar $args" command failed:\n'
-          'with exit code: ${result.exitCode}\n'
-          'stdout: ${result.stdout}\n'
-          'stderr: ${result.stderr}');
+          'with exit code: ${rs.exitCode}\n'
+          'stdout: ${rs.stdout}\n'
+          'stderr: ${rs.stderr}');
       throw Exception('Failed to list tarball contents.');
     }
-
-    final names = (result.stdout as String).split('\n').toList();
-    return _ProcessTarArchive._(path, names);
+    return rs;
   }
 
   /// Reads a text content of [name] from the tar.gz file identified by [_path].
@@ -107,21 +164,31 @@ class _ProcessTarArchive extends TarArchive {
 
 class _PkgTarArchive extends TarArchive {
   final String _path;
-  _PkgTarArchive._(this._path, List<String> names) : super(names);
+  _PkgTarArchive._(
+    this._path,
+    List<String> names,
+    Map<String, String> symlinks,
+  ) : super(names, symlinks);
 
   /// Creates a new instance by scanning the archive at [path].
   static Future<_PkgTarArchive> _scan(String path) async {
-    final names = <String>[];
+    final names = <String>{};
+    final symlinks = <String, String>{};
     final reader = TarReader(
       File(path).openRead().transform(gzip.decoder),
       disallowTrailingData: true,
     );
     while (await reader.moveNext()) {
       final entry = reader.current;
-      names.add(entry.name);
+      if (!names.add(entry.name)) {
+        throw Exception('Duplicate tar entry: `${entry.name}`.');
+      }
+      if (entry.header.linkName != null) {
+        symlinks[entry.name] = entry.header.linkName;
+      }
     }
     await reader.cancel();
-    return _PkgTarArchive._(path, names);
+    return _PkgTarArchive._(path, names.toList(), symlinks);
   }
 
   @override

pkg/pub_package_reader/pubspec.lock

@@ -49,7 +49,7 @@ packages:
       name: checked_yaml
       url: "https://pub.dartlang.org"
     source: hosted
-    version: "1.0.2"
+    version: "1.0.4"
   chunked_stream:
     dependency: transitive
     description:
@@ -224,7 +224,7 @@ packages:
       name: pub_semver
       url: "https://pub.dartlang.org"
     source: hosted
-    version: "1.4.4"
+    version: "2.0.0"
   pubspec_parse:
     dependency: "direct main"
     description:
@@ -378,6 +378,6 @@ packages:
       name: yaml
       url: "https://pub.dartlang.org"
     source: hosted
-    version: "2.2.1"
+    version: "3.1.0"
 sdks:
   dart: ">=2.12.0 <3.0.0"