Always set member perms when unzipping

theory · theory · commit 3e6d1c9d3a46 · 2024-02-08T18:33:31.000-05:00
Re-create `t/root/dist/pair/0.1.1/pair-0.1.1.zip` with no permissions for groups or others to any file or directory and add tests to make sure they are readable by all and that directories are executable by all. This causes failures! Change `unzip` to *always* set file permissions, with different values for directories (which must be executable) and files (which need not be executable for serving by the API). Fixes #15. As a result of the re-organization of `pair-0.1.1.zip`, change the `find_docs` tests to sort results before comparing them. Apparently different zip files can index their members in different orders. So just avoid the problem for the future.
diff --git a/Changes b/Changes
@@ -17,6 +17,9 @@ Revision history for Perl extension PGXN::API
         nor testing releases (#2).
       - Updated the SemVer regex when parsing rsync output to the official
         version published in https://regex101.com/r/vkijKf/ (#16).
+      - Fix unzipping of distributions to ensure that all directories are
+        readable and executable but not writeable by all, and that files are
+        only readable by all (#15).
 
 0.16.5  2016-06-22T18:03:05Z
       - Fixed a test failure on systems with a non-English locale, thanks to
diff --git a/lib/PGXN/API/Sync.pm b/lib/PGXN/API/Sync.pm
@@ -195,7 +195,7 @@ sub unzip {
 
     foreach my $member ($zip->members) {
         # Make sure the file is readable by everyone
-        $member->unixFileAttributes($member->unixFileAttributes | 0444);
+        $member->unixFileAttributes( $member->isDirectory ? 0755 : 0444 );
         my $fn = catfile $dist_dir, split m{/} => $member->fileName;
         say "    $fn\n" if $self->verbose > 2;
         if ($member->extractToFileNamed($fn) != AZ_OK) {
diff --git a/t/indexer.t b/t/indexer.t
@@ -1147,57 +1147,57 @@ is_deeply \@called, [qw(update_user_lists _commit)],
 # Test find_docs().
 touch(catfile $indexer->doc_root_file_for(source => $params->{meta}), qw(sql hi.mkdn));
 $params->{meta}{provides}{pair}{docfile} = 'sql/hi.mkdn';
-is_deeply [ $indexer->find_docs($params)], [
-    { filename => 'sql/hi.mkdn', extension => 'pair' },
-    { filename => 'doc/pair.md' },
+is_deeply [ sort { $a->{filename} cmp $b->{filename} } $indexer->find_docs($params)], [
     { filename => 'README.md' },
+    { filename => 'doc/pair.md' },
+    { filename => 'sql/hi.mkdn', extension => 'pair' },
 ], 'find_docs() should find specified and random doc files';
 
 $params->{meta}{no_index} = { file => ['sql/hi.mkdn'] };
-is_deeply [ $indexer->find_docs($params)], [
-    { filename => 'sql/hi.mkdn', extension => 'pair' },
-    { filename => 'doc/pair.md' },
+is_deeply [ sort { $a->{filename} cmp $b->{filename} } $indexer->find_docs($params)], [
     { filename => 'README.md' },
+    { filename => 'doc/pair.md' },
+    { filename => 'sql/hi.mkdn', extension => 'pair' },
 ], 'find_docs() no_index should be ignored for specified doc file';
 
 $params->{meta}{no_index} = { file => ['doc/pair.md'] };
-is_deeply [ $indexer->find_docs($params)], [
-    { filename => 'sql/hi.mkdn', extension => 'pair' },
+is_deeply [ sort { $a->{filename} cmp $b->{filename} } $indexer->find_docs($params)], [
     { filename => 'README.md' },
+    { filename => 'sql/hi.mkdn', extension => 'pair' },
 ], 'find_docs() should respect no_index for found docs';
 
 $params->{meta}{no_index} = { directory => ['sql'] };
-is_deeply [ $indexer->find_docs($params)], [
-    { filename => 'sql/hi.mkdn', extension => 'pair' },
-    { filename => 'doc/pair.md' },
+is_deeply [ sort { $a->{filename} cmp $b->{filename} } $indexer->find_docs($params)], [
     { filename => 'README.md' },
+    { filename => 'doc/pair.md' },
+    { filename => 'sql/hi.mkdn', extension => 'pair' },
 ], 'find_docs() should ignore no_index directory for specified doc';
 
 $params->{meta}{no_index} = { directory => ['doc'] };
-is_deeply [ $indexer->find_docs($params)], [
-    { filename => 'sql/hi.mkdn', extension => 'pair' },
+is_deeply [ sort { $a->{filename} cmp $b->{filename} } $indexer->find_docs($params)], [
     { filename => 'README.md' },
+    { filename => 'sql/hi.mkdn', extension => 'pair' },
 ], 'find_docs() should respect no_index directory for found docs';
 
 delete $params->{meta}{no_index};
 $params->{meta}{provides}{pair}{docfile} = 'foo/bar.txt';
-is_deeply [ $indexer->find_docs($params)], [
-    { filename => 'doc/pair.md' },
+is_deeply [ sort { $a->{filename} cmp $b->{filename} } $indexer->find_docs($params)], [
     { filename => 'README.md' },
+    { filename => 'doc/pair.md' },
 ], 'find_docs() should ignore non-existent specified file';
 
 $params->{meta}{provides}{pair}{docfile} = 'doc/pair.md';
-is_deeply [ $indexer->find_docs($params)], [
-    { filename => 'doc/pair.md', extension => 'pair' },
+is_deeply [ sort { $a->{filename} cmp $b->{filename} } $indexer->find_docs($params)], [
     { filename => 'README.md' },
+    { filename => 'doc/pair.md', extension => 'pair' },
 ], 'find_docs() should not return dupes';
 
 $params->{meta}{provides}{pair}{docfile} = 'doc/pair.pdf';
 touch(catfile $indexer->doc_root_file_for(source => $params->{meta}), qw(doc pair.pdf));
 
-is_deeply [ $indexer->find_docs($params)], [
-    { filename => 'doc/pair.md' },
+is_deeply [ sort { $a->{filename} cmp $b->{filename} } $indexer->find_docs($params)], [
     { filename => 'README.md' },
+    { filename => 'doc/pair.md' },
 ], 'find_docs() should ignore doc files it does not know how to parse';
 
 sub touch {
diff --git a/t/root/dist/pair/0.1.1/META.json b/t/root/dist/pair/0.1.1/META.json
@@ -9,7 +9,7 @@
    "date": "2010-10-29T22:46:45Z",
    "release_status": "testing",
    "user": "theory",
-   "sha1": "703c0c485166636317c3d4bc8a1a36d512761af7",
+   "sha1": "443cbcf678a3c2f479c4c069bcb96054d9b25a32",
    "license": "postgresql",
    "provides": {
       "pair": {
diff --git a/t/root/dist/pair/0.1.1/pair-0.1.1.zip b/t/root/dist/pair/0.1.1/pair-0.1.1.zip
diff --git a/t/sync.t b/t/sync.t
@@ -2,8 +2,8 @@
 
 use strict;
 use warnings;
-#use Test::More tests => 58;
-use Test::More 'no_plan';
+use Test::More tests => 65;
+# use Test::More 'no_plan';
 use File::Spec::Functions qw(catfile catdir tmpdir);
 use Test::MockModule;
 use Test::Output;
@@ -282,7 +282,7 @@ is_deeply \@users, [qw(
 ##############################################################################
 # digest_for()
 my $pgz = catfile qw(dist pair 0.1.1 pair-0.1.1.zip);
-is $sync->digest_for($pgz), '703c0c485166636317c3d4bc8a1a36d512761af7',
+is $sync->digest_for($pgz), '443cbcf678a3c2f479c4c069bcb96054d9b25a32',
     'Should get expected digest from digest_for()';
 
 ##############################################################################
@@ -323,19 +323,35 @@ my @files = (qw(
     catfile(qw(test expected base.out)),
 );
 
+my @dirs = (
+    'test',
+    catfile(qw(test expected)),
+    catfile(qw(test sql)),
+    'doc',
+    'sql',
+);
+
 my $src_dir = PGXN::API->instance->source_dir;
 my $base = catdir $src_dir, 'pair', 'pair-0.1.1';
-
-file_not_exists_ok catfile($base, $_), "$_ should not exist" for @files;
+file_not_exists_ok $base, "pair-0.1.1 directory should not exist";
 
 # Unzip it.
 ok my $zip = $sync->unzip($pgz, {name => 'pair'}), "Unzip $pgz";
 isa_ok $zip, 'Archive::Zip';
 file_exists_ok catfile($base, $_), "$_ should now exist" for @files;
 my $readall = S_IRUSR | S_IRGRP | S_IROTH;
+my $dirall = S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH;
+ok(
+    ((stat $base)[2] & $dirall) == $dirall,
+    'Root directory should be writeable by owner and readable and executable by all'
+);
+ok(
+    ((stat catfile $base, $_)[2] & $dirall) == $dirall,
+    "Directory $_ should be writeable by owner and readable and executable by all"
+) for @dirs;
 ok(
     ((stat catfile $base, $_)[2] & $readall) == $readall,
-    "$_ should be readable by all",
+    "File $_ should be readable by all",
 ) for @files;
 
 # Now try a brokenated zip file.
