From 253457545ea6bff299beb4ceec992013c41d8d19 Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Mon, 16 Sep 2019 16:25:14 +0100 Subject: [PATCH 1/5] Correctly set/default options in the File class. Fixes #395. Set the options to false for now for consistency. --- lib/zip/file.rb | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/zip/file.rb b/lib/zip/file.rb index 7c051bcd..e494a653 100644 --- a/lib/zip/file.rb +++ b/lib/zip/file.rb @@ -51,6 +51,12 @@ class File < CentralDirectory DATA_BUFFER_SIZE = 8192 IO_METHODS = [:tell, :seek, :read, :close] + DEFAULT_OPTIONS = { + restore_ownership: false, + restore_permissions: false, + restore_times: false + }.freeze + attr_reader :name # default -> false @@ -66,6 +72,7 @@ class File < CentralDirectory # a new archive if it doesn't exist already. def initialize(path_or_io, create = false, buffer = false, options = {}) super() + options = DEFAULT_OPTIONS.merge(options) @name = path_or_io.respond_to?(:path) ? path_or_io.path : path_or_io @comment = '' @create = create ? true : false # allow any truthy value to mean true @@ -98,9 +105,9 @@ def initialize(path_or_io, create = false, buffer = false, options = {}) @stored_entries = @entry_set.dup @stored_comment = @comment - @restore_ownership = options[:restore_ownership] || false - @restore_permissions = options[:restore_permissions] || true - @restore_times = options[:restore_times] || true + @restore_ownership = options[:restore_ownership] + @restore_permissions = options[:restore_permissions] + @restore_times = options[:restore_times] end class << self From 378293539d84916f36f7a8793ab3c1d801d9c33d Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Mon, 16 Sep 2019 16:26:40 +0100 Subject: [PATCH 2/5] Make the attr_accessors in File more readable. Note what the default is and that a couple of them will change at some point soon. --- lib/zip/file.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/zip/file.rb b/lib/zip/file.rb index e494a653..df0594ce 100644 --- a/lib/zip/file.rb +++ b/lib/zip/file.rb @@ -59,12 +59,15 @@ class File < CentralDirectory attr_reader :name - # default -> false + # default -> false. attr_accessor :restore_ownership - # default -> false + + # default -> false, but will be set to true in a future version. attr_accessor :restore_permissions - # default -> true + + # default -> false, but will be set to true in a future version. attr_accessor :restore_times + # Returns the zip files comment, if it has one attr_accessor :comment From 8c694d38ee683b1c2fda3430a928e2308942e434 Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Sun, 6 Oct 2019 19:37:03 +0100 Subject: [PATCH 3/5] Add functionality to restore file timestamps. There has been an option in `Zip::File` (`:restore_times`) for a long time, but it seems it has never worked. Firstly the actual timestamp of an added file wasn't being saved, and secondly an extracted file wasn't having its timestamp set correctly. This commit fixes both of those issues, and adds tests to make sure. --- lib/zip/entry.rb | 18 ++++++++---- test/file_options_test.rb | 58 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 6 deletions(-) create mode 100644 test/file_options_test.rb diff --git a/lib/zip/entry.rb b/lib/zip/entry.rb index 142308a1..88b50494 100644 --- a/lib/zip/entry.rb +++ b/lib/zip/entry.rb @@ -406,16 +406,22 @@ def get_extra_attributes_from_path(path) # :nodoc: @unix_uid = stat.uid @unix_gid = stat.gid @unix_perms = stat.mode & 0o7777 + + mtime = stat.mtime + @time = ::Zip::DOSTime.local(mtime.year, mtime.month, mtime.day, mtime.hour, mtime.min, mtime.sec) end - def set_unix_permissions_on_path(dest_path) - # BUG: does not update timestamps into account + def set_unix_attributes_on_path(dest_path) # ignore setuid/setgid bits by default. honor if @restore_ownership unix_perms_mask = 0o1777 unix_perms_mask = 0o7777 if @restore_ownership ::FileUtils.chmod(@unix_perms & unix_perms_mask, dest_path) if @restore_permissions && @unix_perms ::FileUtils.chown(@unix_uid, @unix_gid, dest_path) if @restore_ownership && @unix_uid && @unix_gid && ::Process.egid == 0 - # File::utimes() + + # Restore the timestamp on a file. This will either have come from the + # original source file that was copied into the archive, or from the + # creation date of the archive if there was no original source file. + ::FileUtils.touch(dest_path, mtime: time) if @restore_times end def set_extra_attributes_on_path(dest_path) # :nodoc: @@ -423,7 +429,7 @@ def set_extra_attributes_on_path(dest_path) # :nodoc: case @fstype when ::Zip::FSTYPE_UNIX - set_unix_permissions_on_path(dest_path) + set_unix_attributes_on_path(dest_path) end end @@ -601,8 +607,6 @@ def create_file(dest_path, _continue_on_exists_proc = proc { Zip.continue_on_exi end ::File.open(dest_path, 'wb') do |os| get_input_stream do |is| - set_extra_attributes_on_path(dest_path) - bytes_written = 0 warned = false buf = ''.dup @@ -621,6 +625,8 @@ def create_file(dest_path, _continue_on_exists_proc = proc { Zip.continue_on_exi end end end + + set_extra_attributes_on_path(dest_path) end def create_directory(dest_path) diff --git a/test/file_options_test.rb b/test/file_options_test.rb new file mode 100644 index 00000000..4f8e5717 --- /dev/null +++ b/test/file_options_test.rb @@ -0,0 +1,58 @@ +require 'test_helper' + +class FileOptionsTest < MiniTest::Test + ZIPPATH = ::File.join(Dir.tmpdir, 'options.zip').freeze + TXTPATH = ::File.expand_path(::File.join('data', 'file1.txt'), __dir__).freeze + EXTPATH_1 = ::File.join(Dir.tmpdir, 'extracted_1.txt').freeze + EXTPATH_2 = ::File.join(Dir.tmpdir, 'extracted_2.txt').freeze + ENTRY_1 = 'entry_1.txt'.freeze + ENTRY_2 = 'entry_2.txt'.freeze + + def teardown + ::File.unlink(ZIPPATH) if ::File.exist?(ZIPPATH) + ::File.unlink(EXTPATH_1) if ::File.exist?(EXTPATH_1) + ::File.unlink(EXTPATH_2) if ::File.exist?(EXTPATH_2) + end + + def test_restore_times_true + ::Zip::File.open(ZIPPATH, true) do |zip| + zip.add(ENTRY_1, TXTPATH) + zip.add_stored(ENTRY_2, TXTPATH) + end + + ::Zip::File.open(ZIPPATH, false, restore_times: true) do |zip| + zip.extract(ENTRY_1, EXTPATH_1) + zip.extract(ENTRY_2, EXTPATH_2) + end + + assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_1)) + assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_2)) + end + + def test_restore_times_false + ::Zip::File.open(ZIPPATH, true) do |zip| + zip.add(ENTRY_1, TXTPATH) + zip.add_stored(ENTRY_2, TXTPATH) + end + + ::Zip::File.open(ZIPPATH, false, restore_times: false) do |zip| + zip.extract(ENTRY_1, EXTPATH_1) + zip.extract(ENTRY_2, EXTPATH_2) + end + + assert_time_equal(::Time.now, ::File.mtime(EXTPATH_1)) + assert_time_equal(::Time.now, ::File.mtime(EXTPATH_2)) + end + + private + + # Method to compare file times. DOS times only have 2 second accuracy. + def assert_time_equal(expected, actual) + assert_equal(expected.year, actual.year) + assert_equal(expected.month, actual.month) + assert_equal(expected.day, actual.day) + assert_equal(expected.hour, actual.hour) + assert_equal(expected.min, actual.min) + assert_in_delta(expected.sec, actual.sec, 1) + end +end From 2bdd37d8949aadb8b07a598f720e0d96b571d5a2 Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Sun, 6 Oct 2019 20:01:14 +0100 Subject: [PATCH 4/5] Add a convenience method for creating DOSTime instances. DOSTime::from_time creates a DOSTime instance from a vanilla Time instance. --- lib/zip/dos_time.rb | 5 +++++ lib/zip/entry.rb | 4 +--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/zip/dos_time.rb b/lib/zip/dos_time.rb index bf0cb7e0..c912b773 100644 --- a/lib/zip/dos_time.rb +++ b/lib/zip/dos_time.rb @@ -29,6 +29,11 @@ def dos_equals(other) to_i / 2 == other.to_i / 2 end + # Create a DOSTime instance from a vanilla Time instance. + def self.from_time(time) + local(time.year, time.month, time.day, time.hour, time.min, time.sec) + end + def self.parse_binary_dos_format(binaryDosDate, binaryDosTime) second = 2 * (0b11111 & binaryDosTime) minute = (0b11111100000 & binaryDosTime) >> 5 diff --git a/lib/zip/entry.rb b/lib/zip/entry.rb index 88b50494..37b1690b 100644 --- a/lib/zip/entry.rb +++ b/lib/zip/entry.rb @@ -406,9 +406,7 @@ def get_extra_attributes_from_path(path) # :nodoc: @unix_uid = stat.uid @unix_gid = stat.gid @unix_perms = stat.mode & 0o7777 - - mtime = stat.mtime - @time = ::Zip::DOSTime.local(mtime.year, mtime.month, mtime.day, mtime.hour, mtime.min, mtime.sec) + @time = ::Zip::DOSTime.from_time(stat.mtime) end def set_unix_attributes_on_path(dest_path) From 1a21f39e82323e023df882f94b408da55d54c4c6 Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Sun, 6 Oct 2019 23:08:52 +0100 Subject: [PATCH 5/5] Add a test for restoring file permissions on extract. --- test/file_options_test.rb | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/test/file_options_test.rb b/test/file_options_test.rb index 4f8e5717..48b83601 100644 --- a/test/file_options_test.rb +++ b/test/file_options_test.rb @@ -3,15 +3,46 @@ class FileOptionsTest < MiniTest::Test ZIPPATH = ::File.join(Dir.tmpdir, 'options.zip').freeze TXTPATH = ::File.expand_path(::File.join('data', 'file1.txt'), __dir__).freeze + TXTPATH_600 = ::File.join(Dir.tmpdir, 'file1.600.txt').freeze + TXTPATH_755 = ::File.join(Dir.tmpdir, 'file1.755.txt').freeze EXTPATH_1 = ::File.join(Dir.tmpdir, 'extracted_1.txt').freeze EXTPATH_2 = ::File.join(Dir.tmpdir, 'extracted_2.txt').freeze + EXTPATH_3 = ::File.join(Dir.tmpdir, 'extracted_3.txt').freeze ENTRY_1 = 'entry_1.txt'.freeze ENTRY_2 = 'entry_2.txt'.freeze + ENTRY_3 = 'entry_3.txt'.freeze def teardown ::File.unlink(ZIPPATH) if ::File.exist?(ZIPPATH) ::File.unlink(EXTPATH_1) if ::File.exist?(EXTPATH_1) ::File.unlink(EXTPATH_2) if ::File.exist?(EXTPATH_2) + ::File.unlink(EXTPATH_3) if ::File.exist?(EXTPATH_3) + ::File.unlink(TXTPATH_600) if ::File.exist?(TXTPATH_600) + ::File.unlink(TXTPATH_755) if ::File.exist?(TXTPATH_755) + end + + def test_restore_permissions + # Copy and set up files with different permissions. + ::FileUtils.cp(TXTPATH, TXTPATH_600) + ::File.chmod(0600, TXTPATH_600) + ::FileUtils.cp(TXTPATH, TXTPATH_755) + ::File.chmod(0755, TXTPATH_755) + + ::Zip::File.open(ZIPPATH, true) do |zip| + zip.add(ENTRY_1, TXTPATH) + zip.add(ENTRY_2, TXTPATH_600) + zip.add(ENTRY_3, TXTPATH_755) + end + + ::Zip::File.open(ZIPPATH, false, restore_permissions: true) do |zip| + zip.extract(ENTRY_1, EXTPATH_1) + zip.extract(ENTRY_2, EXTPATH_2) + zip.extract(ENTRY_3, EXTPATH_3) + end + + assert_equal(::File.stat(TXTPATH).mode, ::File.stat(EXTPATH_1).mode) + assert_equal(::File.stat(TXTPATH_600).mode, ::File.stat(EXTPATH_2).mode) + assert_equal(::File.stat(TXTPATH_755).mode, ::File.stat(EXTPATH_3).mode) end def test_restore_times_true