From 2977ed10714024d6441d4b33252311917112f9fc Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Wed, 20 May 2026 15:28:07 +0100 Subject: [PATCH 01/12] Support Ruby 4.0 --- .ruby-version | 2 +- Rakefile | 9 --------- ro_crate.gemspec | 2 +- 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/.ruby-version b/.ruby-version index 61a52c9..ea897ac 100644 --- a/.ruby-version +++ b/.ruby-version @@ -1 +1 @@ -ruby-3.4.9 +ruby-4.0.5 diff --git a/Rakefile b/Rakefile index 33a59df..d356330 100644 --- a/Rakefile +++ b/Rakefile @@ -1,6 +1,5 @@ require 'bundler/gem_tasks' require 'rake/testtask' -require 'rdoc/task' desc 'Default: run unit tests.' task default: :test @@ -13,14 +12,6 @@ Rake::TestTask.new(:test) do |t| t.warning = false end -Rake::RDocTask.new(:rdoc) do |rdoc| - rdoc.rdoc_dir = 'rdoc' - rdoc.title = 'Devise' - rdoc.options << '--line-numbers' << '--inline-source' - rdoc.rdoc_files.include('README.md') - rdoc.rdoc_files.include('lib/**/*.rb') -end - task :console do require 'irb' require 'irb/completion' diff --git a/ro_crate.gemspec b/ro_crate.gemspec index ba975ca..f9e98d3 100644 --- a/ro_crate.gemspec +++ b/ro_crate.gemspec @@ -10,7 +10,7 @@ Gem::Specification.new do |s| s.licenses = ['MIT'] s.add_runtime_dependency 'addressable', '>= 2.7', '< 3' s.add_runtime_dependency 'rubyzip', '>= 2.3', '< 4' - s.add_development_dependency 'rake', '~> 13.0.0' + s.add_development_dependency 'rake', '~> 13.4.2' s.add_development_dependency 'test-unit', '~> 3.5.3' s.add_development_dependency 'simplecov', '~> 0.21.2' s.add_development_dependency 'yard', '~> 0.9.25' From f932f89f74959ff1e341a3db0efed233d3d22b1b Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Wed, 20 May 2026 15:29:37 +0100 Subject: [PATCH 02/12] Actually test against Ruby 4.0... --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 50a2192..951af8b 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -5,7 +5,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - ruby: ['2.7', '3.0', '3.1', '3.2', '3.3', '3.4'] + ruby: ['2.7', '3.0', '3.1', '3.2', '3.3', '3.4', '4.0'] fail-fast: false steps: - name: Checkout From 0cb8ae14911e58568a31afca38295c8f84fdf5ab Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Wed, 20 May 2026 15:31:41 +0100 Subject: [PATCH 03/12] Match arg names to documentation --- lib/ro_crate/reader.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/ro_crate/reader.rb b/lib/ro_crate/reader.rb index 368a011..09fa52f 100644 --- a/lib/ro_crate/reader.rb +++ b/lib/ro_crate/reader.rb @@ -42,9 +42,9 @@ def self.unzip_to(source, target) # # @param source [#read] An IO-like object containing a Zip file. # @param target [String, ::File, Pathname] The target directory where the file should be unzipped. - def self.unzip_io_to(io, target) + def self.unzip_io_to(source, target) Dir.chdir(target) do - Zip::InputStream.open(io) do |input| + Zip::InputStream.open(source) do |input| while (entry = input.get_next_entry) unless ::File.exist?(entry.name) || entry.name_is_directory? FileUtils::mkdir_p(::File.dirname(entry.name)) @@ -60,9 +60,9 @@ def self.unzip_io_to(io, target) # # @param source [String, ::File, Pathname] The location of the zip file. # @param target [String, ::File, Pathname] The target directory where the file should be unzipped. - def self.unzip_file_to(file_or_path, target) + def self.unzip_file_to(source, target) Dir.chdir(target) do - Zip::File.open(file_or_path) do |zipfile| + Zip::File.open(source) do |zipfile| zipfile.each do |entry| unless ::File.exist?(entry.name) FileUtils::mkdir_p(::File.dirname(entry.name)) From 261bc34920435b80d4f81e01ef534a6729a5ca42 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Wed, 20 May 2026 15:35:04 +0100 Subject: [PATCH 04/12] Drop dev Ruby version back to 3.4.9 --- .ruby-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ruby-version b/.ruby-version index ea897ac..61a52c9 100644 --- a/.ruby-version +++ b/.ruby-version @@ -1 +1 @@ -ruby-4.0.5 +ruby-3.4.9 From 205802cd1e120bdf86a67b5adbac28d5e594c2ce Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 21 May 2026 09:06:21 +0100 Subject: [PATCH 05/12] Avoid using `Dir.chdir` --- lib/ro_crate/model/directory.rb | 5 ++-- lib/ro_crate/reader.rb | 46 +++++++++++++++++++++------------ 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/lib/ro_crate/model/directory.rb b/lib/ro_crate/model/directory.rb index 91018e7..4585075 100644 --- a/lib/ro_crate/model/directory.rb +++ b/lib/ro_crate/model/directory.rb @@ -74,9 +74,8 @@ def full_entry_path(relative_path) end def list_all_files(source_directory, include_hidden: false) - args = ['**/*'] - args << ::File::FNM_DOTMATCH if include_hidden - Dir.chdir(source_directory) { Dir.glob(*args) }.reject do |path| + flags = include_hidden ? ::File::FNM_DOTMATCH : 0 + Dir.glob('**/*', flags: flags, base: source_directory).reject do |path| path == '.' || path == '..' || path.end_with?('/.') end end diff --git a/lib/ro_crate/reader.rb b/lib/ro_crate/reader.rb index 09fa52f..f465f2a 100644 --- a/lib/ro_crate/reader.rb +++ b/lib/ro_crate/reader.rb @@ -1,7 +1,11 @@ +require 'zip/version' + module ROCrate ## # A class to handle reading of RO-Crates from Zip files or directories. class Reader + LEGACY_EXTRACT = Zip::VERSION.start_with?('2.').freeze + ## # Reads an RO-Crate from a directory or zip file. # @@ -43,14 +47,20 @@ def self.unzip_to(source, target) # @param source [#read] An IO-like object containing a Zip file. # @param target [String, ::File, Pathname] The target directory where the file should be unzipped. def self.unzip_io_to(source, target) - Dir.chdir(target) do - Zip::InputStream.open(source) do |input| - while (entry = input.get_next_entry) - unless ::File.exist?(entry.name) || entry.name_is_directory? - FileUtils::mkdir_p(::File.dirname(entry.name)) - ::File.binwrite(entry.name, input.read) - end - end + target = Pathname(target) + Zip::InputStream.open(source) do |input| + while (entry = input.get_next_entry) + next if entry.name_is_directory? + + dest = target.join(entry.name) + + # Guard against zip slip attacks, even though Rubyzip should block them. + raise "Unsafe path in zip entry: #{entry.name}" unless dest.to_s.start_with?(::File.realpath(target) + ::File::SEPARATOR) + + next if dest.exist? + + FileUtils.mkdir_p(dest.dirname) + ::File.binwrite(dest, input.read) end end end @@ -61,14 +71,18 @@ def self.unzip_io_to(source, target) # @param source [String, ::File, Pathname] The location of the zip file. # @param target [String, ::File, Pathname] The target directory where the file should be unzipped. def self.unzip_file_to(source, target) - Dir.chdir(target) do - Zip::File.open(source) do |zipfile| - zipfile.each do |entry| - unless ::File.exist?(entry.name) - FileUtils::mkdir_p(::File.dirname(entry.name)) - zipfile.extract(entry, entry.name) - end - end + target = Pathname(target) + Zip::File.open(source) do |zipfile| + zipfile.each do |entry| + dest = target.join(entry.name) + + # Guard against zip slip attacks, even though Rubyzip should block them. + raise "Unsafe path in zip entry: #{entry.name}" unless dest.to_s.start_with?(::File.realpath(target) + ::File::SEPARATOR) + + next if dest.exist? + + FileUtils.mkdir_p(dest.dirname) + LEGACY_EXTRACT ? entry.extract(dest) : entry.extract(entry.name, destination_directory: target) end end end From bd1b82d200394bff682a78dc3b10bda94d172d19 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 21 May 2026 09:29:48 +0100 Subject: [PATCH 06/12] Remove zip slip guards. Skip directory in zip --- lib/ro_crate/reader.rb | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/lib/ro_crate/reader.rb b/lib/ro_crate/reader.rb index f465f2a..7505991 100644 --- a/lib/ro_crate/reader.rb +++ b/lib/ro_crate/reader.rb @@ -51,14 +51,8 @@ def self.unzip_io_to(source, target) Zip::InputStream.open(source) do |input| while (entry = input.get_next_entry) next if entry.name_is_directory? - dest = target.join(entry.name) - - # Guard against zip slip attacks, even though Rubyzip should block them. - raise "Unsafe path in zip entry: #{entry.name}" unless dest.to_s.start_with?(::File.realpath(target) + ::File::SEPARATOR) - next if dest.exist? - FileUtils.mkdir_p(dest.dirname) ::File.binwrite(dest, input.read) end @@ -74,13 +68,9 @@ def self.unzip_file_to(source, target) target = Pathname(target) Zip::File.open(source) do |zipfile| zipfile.each do |entry| + next if entry.name_is_directory? dest = target.join(entry.name) - - # Guard against zip slip attacks, even though Rubyzip should block them. - raise "Unsafe path in zip entry: #{entry.name}" unless dest.to_s.start_with?(::File.realpath(target) + ::File::SEPARATOR) - next if dest.exist? - FileUtils.mkdir_p(dest.dirname) LEGACY_EXTRACT ? entry.extract(dest) : entry.extract(entry.name, destination_directory: target) end From 718c9ecd8fd10ba13153f1850400f08110ce4a64 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 21 May 2026 09:30:01 +0100 Subject: [PATCH 07/12] Use positional flags argument --- lib/ro_crate/model/directory.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ro_crate/model/directory.rb b/lib/ro_crate/model/directory.rb index 4585075..3cd7ef5 100644 --- a/lib/ro_crate/model/directory.rb +++ b/lib/ro_crate/model/directory.rb @@ -75,7 +75,7 @@ def full_entry_path(relative_path) def list_all_files(source_directory, include_hidden: false) flags = include_hidden ? ::File::FNM_DOTMATCH : 0 - Dir.glob('**/*', flags: flags, base: source_directory).reject do |path| + Dir.glob('**/*', flags, base: source_directory).reject do |path| path == '.' || path == '..' || path.end_with?('/.') end end From c0d65b239a36acb8751a437ac3effcd0d4f85e43 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 21 May 2026 10:01:21 +0100 Subject: [PATCH 08/12] Re-add zip-slip guards and test --- lib/ro_crate/reader.rb | 28 ++++++++++++++++++------ test/fixtures/unsafe/absolute1.zip | Bin 0 -> 118 bytes test/fixtures/unsafe/relative0.zip | Bin 0 -> 114 bytes test/reader_test.rb | 33 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/unsafe/absolute1.zip create mode 100644 test/fixtures/unsafe/relative0.zip diff --git a/lib/ro_crate/reader.rb b/lib/ro_crate/reader.rb index 7505991..207ddf9 100644 --- a/lib/ro_crate/reader.rb +++ b/lib/ro_crate/reader.rb @@ -47,11 +47,11 @@ def self.unzip_to(source, target) # @param source [#read] An IO-like object containing a Zip file. # @param target [String, ::File, Pathname] The target directory where the file should be unzipped. def self.unzip_io_to(source, target) - target = Pathname(target) + target_path = Pathname(target) Zip::InputStream.open(source) do |input| while (entry = input.get_next_entry) next if entry.name_is_directory? - dest = target.join(entry.name) + dest = safe_join(target_path, entry.name) next if dest.exist? FileUtils.mkdir_p(dest.dirname) ::File.binwrite(dest, input.read) @@ -65,14 +65,13 @@ def self.unzip_io_to(source, target) # @param source [String, ::File, Pathname] The location of the zip file. # @param target [String, ::File, Pathname] The target directory where the file should be unzipped. def self.unzip_file_to(source, target) - target = Pathname(target) + target_path = Pathname(target) Zip::File.open(source) do |zipfile| zipfile.each do |entry| - next if entry.name_is_directory? - dest = target.join(entry.name) + dest = safe_join(target_path, entry.name) next if dest.exist? FileUtils.mkdir_p(dest.dirname) - LEGACY_EXTRACT ? entry.extract(dest) : entry.extract(entry.name, destination_directory: target) + LEGACY_EXTRACT ? entry.extract(dest) : entry.extract(entry.name, destination_directory: target_path) end end end @@ -349,5 +348,22 @@ def self.detect_root_directory(source) nil end + + ## + # Safely joins a desired file path onto a base directory, raising an exception if the path attempts to traverse + # outside it. + # + # @param base [Pathname] The base directory where the file will go. + # @param path [String] The desired file path. + # + # @raise [ROCrate::ReadException] Raised if an unsafe path is given. + # + # @return [Pathname] The safely joined base + path. + def self.safe_join(base, path) + dest = base.join(path) + # Guard against zip-slip attacks. + raise ROCrate::ReadException, "Unsafe path in zip entry: #{path}" if dest.relative_path_from(base).each_filename.first == '..' + dest + end end end diff --git a/test/fixtures/unsafe/absolute1.zip b/test/fixtures/unsafe/absolute1.zip new file mode 100644 index 0000000000000000000000000000000000000000..27c615d98bae9a331eedd1425f5830c40812fd2c GIT binary patch literal 118 zcmWIWW@h1H0D<3gj{B^9WW16E$Od5!Al5I*Ezr-+&j%u|0B=SnIcD5yfyx;efp|$H Why~Lb;LXYg;xhuF8IaZjaTox`+Z8VW literal 0 HcmV?d00001 diff --git a/test/fixtures/unsafe/relative0.zip b/test/fixtures/unsafe/relative0.zip new file mode 100644 index 0000000000000000000000000000000000000000..d27a0d08f03edd0477811b0f3f5c2c2d6a18b9ba GIT binary patch literal 114 zcmWIWW@h1H0D<3gj{B^9WW16E$Od6HAlB2<&&|&VBCY^$MkYCC+$w>J85n_hNh62_ V(HG#&3Ni#J&d6W_q%}Ys1^^b>65jv- literal 0 HcmV?d00001 diff --git a/test/reader_test.rb b/test/reader_test.rb index 268facb..28f73be 100644 --- a/test/reader_test.rb +++ b/test/reader_test.rb @@ -386,6 +386,39 @@ class ReaderTest < Test::Unit::TestCase assert_equal 'a_file', data.first.name end + test 'protect against zip-slip' do + Dir.mktmpdir do |dir| + subdir = ::File.join(dir, 'subdir') + ::Dir.mkdir(subdir) + + # Relative + e = check_exception(ROCrate::ReadException) do + ROCrate::Reader.unzip_file_to(fixture_file('unsafe/relative0.zip').path, subdir) + end + assert_include e.message, 'Unsafe path in zip entry: ../moo' + refute ::File.exist?(::File.join(dir, 'moo')) + + e = check_exception(ROCrate::ReadException) do + ROCrate::Reader.unzip_io_to(fixture_file('unsafe/relative0.zip'), subdir) + end + assert_include e.message, 'Unsafe path in zip entry: ../moo' + refute ::File.exist?(::File.join(dir, 'moo')) + + # Absolute + e = check_exception(ROCrate::ReadException) do + ROCrate::Reader.unzip_file_to(fixture_file('unsafe/absolute1.zip').path, subdir) + end + assert_include e.message, 'Unsafe path in zip entry: /tmp/moo' + refute ::File.exist?(::File.join('/tmp', 'moo')) + + e = check_exception(ROCrate::ReadException) do + ROCrate::Reader.unzip_io_to(fixture_file('unsafe/absolute1.zip'), subdir) + end + assert_include e.message, 'Unsafe path in zip entry: /tmp/moo' + refute ::File.exist?(::File.join('/tmp', 'moo')) + end + end + private def check_exception(exception_class) From ab890ee7687e127c58f8609ebffa12f2d0435d2a Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 21 May 2026 10:17:40 +0100 Subject: [PATCH 09/12] Close open fixture files between tests --- test/test_helper.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/test_helper.rb b/test/test_helper.rb index bcade3c..d9971a1 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -5,8 +5,18 @@ require 'ro_crate' require 'webmock/test_unit' +def teardown + self._opened_files.each(&:close) +end + +def _opened_files + @opened_files ||= [] +end + def fixture_file(name, *args) - ::File.open(::File.join(fixture_dir, name), *args) + f = ::File.open(::File.join(fixture_dir, name), *args) + self._opened_files << f + f end def fixture_dir From d91229d446bee30b9065e2b9877d820f0d9ea266 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 21 May 2026 10:31:42 +0100 Subject: [PATCH 10/12] Handle possible ArgumentError when comparing paths --- lib/ro_crate/reader.rb | 8 +++++++- test/reader_test.rb | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/lib/ro_crate/reader.rb b/lib/ro_crate/reader.rb index 207ddf9..ab6b43a 100644 --- a/lib/ro_crate/reader.rb +++ b/lib/ro_crate/reader.rb @@ -362,7 +362,13 @@ def self.detect_root_directory(source) def self.safe_join(base, path) dest = base.join(path) # Guard against zip-slip attacks. - raise ROCrate::ReadException, "Unsafe path in zip entry: #{path}" if dest.relative_path_from(base).each_filename.first == '..' + begin + unsafe = dest.expand_path.relative_path_from(base.expand_path).each_filename.first == '..' + rescue ArgumentError # Handle unjoinable paths, e.g. on different drives. + unsafe = true + end + raise ROCrate::ReadException, "Unsafe path in zip entry: #{path}" if unsafe + dest end end diff --git a/test/reader_test.rb b/test/reader_test.rb index 28f73be..f3c4c84 100644 --- a/test/reader_test.rb +++ b/test/reader_test.rb @@ -409,13 +409,25 @@ class ReaderTest < Test::Unit::TestCase ROCrate::Reader.unzip_file_to(fixture_file('unsafe/absolute1.zip').path, subdir) end assert_include e.message, 'Unsafe path in zip entry: /tmp/moo' - refute ::File.exist?(::File.join('/tmp', 'moo')) e = check_exception(ROCrate::ReadException) do ROCrate::Reader.unzip_io_to(fixture_file('unsafe/absolute1.zip'), subdir) end assert_include e.message, 'Unsafe path in zip entry: /tmp/moo' - refute ::File.exist?(::File.join('/tmp', 'moo')) + + # Simulate ArgumentError in safe_join + begin + original_expand_path = Pathname.instance_method(:expand_path) + Pathname.define_method(:expand_path) do |*args| + raise ArgumentError, 'Oh no' + end + e = check_exception(ROCrate::ReadException) do + ROCrate::Reader.unzip_file_to(fixture_file('unsafe/absolute1.zip').path, subdir) + end + assert_include e.message, 'Unsafe path in zip entry: /tmp/moo' + ensure + Pathname.define_method(:expand_path, original_expand_path) + end end end From 1e9820a99285037d58875c32af4998d234194b53 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 21 May 2026 10:34:09 +0100 Subject: [PATCH 11/12] Move method into helper --- test/reader_test.rb | 15 --------------- test/test_helper.rb | 13 +++++++++++++ 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/test/reader_test.rb b/test/reader_test.rb index f3c4c84..825e181 100644 --- a/test/reader_test.rb +++ b/test/reader_test.rb @@ -431,21 +431,6 @@ class ReaderTest < Test::Unit::TestCase end end - private - - def check_exception(exception_class) - e = nil - assert_raise(exception_class) do - begin - yield - rescue exception_class => e - raise e - end - end - - e - end - test 'reads spec 1.1 RO-Crate and preserves version' do crate = ROCrate::Reader.read(fixture_file('crate-spec1.1').path) diff --git a/test/test_helper.rb b/test/test_helper.rb index d9971a1..ead03e6 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -22,3 +22,16 @@ def fixture_file(name, *args) def fixture_dir ::File.join(::File.dirname(__FILE__), 'fixtures') end + +def check_exception(exception_class) + e = nil + assert_raise(exception_class) do + begin + yield + rescue exception_class => e + raise e + end + end + + e +end From 0e70cba9ccf5da0f660d40cc1daaa62a062dc0b1 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 21 May 2026 10:44:44 +0100 Subject: [PATCH 12/12] Add teardown method to TestCase --- test/test_helper.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/test_helper.rb b/test/test_helper.rb index ead03e6..674b482 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -5,12 +5,16 @@ require 'ro_crate' require 'webmock/test_unit' -def teardown - self._opened_files.each(&:close) -end +class Test::Unit::TestCase + def teardown + self._opened_files.each do |f| + f.close unless f.closed? + end + end -def _opened_files - @opened_files ||= [] + def _opened_files + @opened_files ||= [] + end end def fixture_file(name, *args)