Repository: rubyzip/rubyzip (ruby)

Issue:
0600 ZIP file permissions with umask 0022
I do:

``` ruby
Zip::File.open(zip_file, Zip::File::CREATE) do |zip|
  begin
    zip.add(name, bin)
  rescue Zip::ZipEntryExistsError
    zip.replace(name, bin)
  end
end
```

Is there a reason, I get `0600` file permissions on my `zip_file` instead of `0644`, I would expect? Diagnostics:

``` sh
$ umask
0022
$ ruby --version 
ruby 2.2.0p0 (2014-12-25 revision 49005) [x86_64-darwin12.0]
```

I googled that and found out an advice to use `File.chmod` implicitly. Am I overlooking something? In particular:
1. Why rubyzip does not default to `umask`-based permissions? Is it documented?
2. Are there any handles to get `umask`-based behaviour, not a hardcoded `0600`? Didn't find them in docs.



Discussion:
@chillum Rubyzip uses Tempfile for create a file of newly created zip archive. So created file will have only 600 due the security reasons. But you can use `File.chmod` for extend permissions.

I'm not sure I follow the logic here.

The bug submitted is that rubyzip doesn't do what is generally expected of programs that create files; in this case take into account default permissions and umask.

I'm inclined to agree that this is a bug. The fact that the way rubyzip creates files produces a side-effect that stops this happening doesn't mean it's not a bug.

I'll look into it and see if we can do something better.


Patch:
diff --git a/README.md b/README.md
--- a/README.md
+++ b/README.md
@@ -141,6 +141,16 @@ fruit/orange
 
 After this, entries in the zip archive will be saved in ordered state.
 
+### Default permissions of zip archives
+
+On Posix file systems the default file permissions applied to a new archive
+are (0666 - umask), which mimics the behavior of standard tools such as `touch`.
+
+On Windows the default file permissions are set to 0644 as suggested by the
+[Ruby File documentation](http://ruby-doc.org/core-2.2.2/File.html).
+
+When modifying a zip archive the file permissions of the archive are preserved.
+
 ### Reading a Zip file
 
 ```ruby
diff --git a/lib/zip/file.rb b/lib/zip/file.rb
--- a/lib/zip/file.rb
+++ b/lib/zip/file.rb
@@ -71,11 +71,12 @@ def initialize(file_name, create = nil, buffer = false, options = {})
       case
       when !buffer && ::File.size?(file_name)
         @create = nil
-        @exist_file_perms = ::File.stat(file_name).mode
+        @file_permissions = ::File.stat(file_name).mode
         ::File.open(name, 'rb') do |f|
           read_from_stream(f)
         end
       when create
+        @file_permissions = create_file_permissions
         @entry_set = EntrySet.new
       else
         raise Error, "File #{file_name} not found"
@@ -405,7 +406,7 @@ def on_success_replace
       tmpfile.close
       if yield tmp_filename
         ::File.rename(tmp_filename, name)
-        ::File.chmod(@exist_file_perms, name) if defined?(@exist_file_perms)
+        ::File.chmod(@file_permissions, name) if defined?(@file_permissions)
       end
     ensure
       tmpfile.unlink if tmpfile
@@ -416,6 +417,10 @@ def get_tempfile
       temp_file.binmode
       temp_file
     end
+
+    def create_file_permissions
+      ::Zip::RUNNING_ON_WINDOWS ? 0644 : 0666 - ::File.umask
+    end
   end
 end
 


Test patch:
diff --git a/test/file_permissions_test.rb b/test/file_permissions_test.rb
new file mode 100644
--- /dev/null
+++ b/test/file_permissions_test.rb
@@ -0,0 +1,69 @@
+require 'test_helper'
+
+class FilePermissionsTest < MiniTest::Test
+
+  FILENAME = File.join(File.dirname(__FILE__), "umask.zip")
+
+  def teardown
+    ::File.unlink(FILENAME)
+  end
+
+  if ::Zip::RUNNING_ON_WINDOWS
+    # Windows tests
+
+    DEFAULT_PERMS = 0644
+
+    def test_windows_perms
+      create_file
+
+      assert_equal DEFAULT_PERMS, ::File.stat(FILENAME).mode
+    end
+
+  else
+    # Unix tests
+
+    DEFAULT_PERMS = 0100666
+
+    def test_current_umask
+      umask = DEFAULT_PERMS - ::File.umask
+      create_file
+
+      assert_equal umask, ::File.stat(FILENAME).mode
+    end
+
+    def test_umask_000
+      set_umask(0000) do
+        create_file
+      end
+
+      assert_equal DEFAULT_PERMS, ::File.stat(FILENAME).mode
+    end
+
+    def test_umask_066
+      umask = 0066
+      set_umask(umask) do
+        create_file
+      end
+
+      assert_equal((DEFAULT_PERMS - umask), ::File.stat(FILENAME).mode)
+    end
+
+  end
+
+  def create_file
+    ::Zip::File.open(FILENAME, ::Zip::File::CREATE) do |zip|
+      zip.comment = "test"
+    end
+  end
+
+  # If anything goes wrong, make sure the umask is restored.
+  def set_umask(umask, &block)
+    begin
+      saved_umask = ::File.umask(umask)
+      yield
+    ensure
+      ::File.umask(saved_umask)
+    end
+  end
+
+end