diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9f86f2..2181fcf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,6 +25,21 @@ jobs: - run: bundle exec rake test + lint: + name: "Rubocop" + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + + - uses: ruby/setup-ruby@v1 + with: + # Matches TargetRubyVersion in .rubocop.yml. + ruby-version: "3.2" + bundler-cache: true + + - run: bundle exec rubocop + worker: name: "Worker" runs-on: ubuntu-latest diff --git a/.rubocop.yml b/.rubocop.yml index 9b47f07..41477a1 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -12,6 +12,8 @@ Style/StringLiterals: Metrics/ClassLength: Max: 150 + Exclude: + - "test/**/*" Metrics/AbcSize: Max: 30 diff --git a/lib/prebake/async_publisher.rb b/lib/prebake/async_publisher.rb index 472e8d7..902b683 100644 --- a/lib/prebake/async_publisher.rb +++ b/lib/prebake/async_publisher.rb @@ -76,7 +76,8 @@ def self.build_gem(spec, backend) end if !Prebake.libruby_available? && ElfInspector.libruby_needed_for_gem?(gem_path) - Logger.warn "Skipping push of #{cache_key}: binary requires libruby.so (dynamic Ruby) but this is a static Ruby build; binary would crash on this platform" + Logger.warn "Skipping push of #{cache_key}: binary requires libruby.so (dynamic Ruby) " \ + "but this is a static Ruby build; binary would crash on this platform" FileUtils.rm_f(gem_path) return nil end diff --git a/lib/prebake/elf_inspector.rb b/lib/prebake/elf_inspector.rb index 2904d26..10e32cd 100644 --- a/lib/prebake/elf_inspector.rb +++ b/lib/prebake/elf_inspector.rb @@ -10,7 +10,10 @@ module Prebake module ElfInspector def self.required_glibc_for_gem(gem_path) versions = [] - each_gem_binary(gem_path) { |binary| v = required_glibc(binary); versions << v if v } + each_gem_binary(gem_path) do |binary| + v = required_glibc(binary) + versions << v if v + end versions.empty? ? nil : versions.max_by { |v| Gem::Version.new(v) } rescue StandardError => e # Malformed gem, missing objdump, or I/O error — treat as unknown, let @@ -69,7 +72,7 @@ def self.run_objdump(path) Dir.mktmpdir("prebake-gem") do |tmpdir| Gem::Package.new(gem_path).extract_files(tmpdir) Dir.glob(File.join(tmpdir, "**/*.{so,bundle,dll}")).each do |binary| - next if File.symlink?(binary) || File.size(binary).zero? + next if File.symlink?(binary) || File.empty?(binary) yield binary end diff --git a/lib/prebake/ext_builder_patch.rb b/lib/prebake/ext_builder_patch.rb index 0931f93..ebeb7f5 100644 --- a/lib/prebake/ext_builder_patch.rb +++ b/lib/prebake/ext_builder_patch.rb @@ -11,6 +11,14 @@ module Prebake module ExtBuilderPatch + # The metrics cops are off for this method deliberately. It is a cascade of + # cache checks where every failure path falls through to `super`, the real + # source build. Extracting any stage would mean returning a sentinel for the + # caller to turn back into `super`, which reads worse than the cascade and + # risks silently installing a gem with no compiled binaries if a branch is + # mistranslated. + # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity + # rubocop:disable Metrics/MethodLength, Metrics/PerceivedComplexity def build_extensions return super unless @spec.extensions.any? return super unless Prebake.enabled? @@ -59,7 +67,7 @@ def build_extensions unless installed Prebake.backend.delete(cache_key) - return super + super end else super @@ -67,6 +75,8 @@ def build_extensions ensure FileUtils.rm_f(cached_gem) if cached_gem end + # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity + # rubocop:enable Metrics/MethodLength, Metrics/PerceivedComplexity private diff --git a/lib/prebake/extension_validator.rb b/lib/prebake/extension_validator.rb index e4be565..a278553 100644 --- a/lib/prebake/extension_validator.rb +++ b/lib/prebake/extension_validator.rb @@ -24,7 +24,7 @@ def self.validate(spec) nested.each do |binary| next if File.symlink?(binary) - next if File.size(binary).zero? + next if File.empty?(binary) dest = File.join(ext_dir, File.basename(binary)) next if File.exist?(dest) diff --git a/lib/prebake/extractor.rb b/lib/prebake/extractor.rb index 65b7d4e..f72b4ed 100644 --- a/lib/prebake/extractor.rb +++ b/lib/prebake/extractor.rb @@ -17,27 +17,9 @@ def self.install(gem_path, spec) # Copy only binary files (.so, .bundle, .dll) to extension_dir Dir.glob(File.join(tmpdir, "**/*.{so,bundle,dll}")).each do |binary| - # Reject symlinks and empty files - next if File.symlink?(binary) - next if File.size(binary).zero? - - # Verify path is within tmpdir (prevent traversal) - real_binary = File.realpath(binary) - real_tmpdir = File.realpath(tmpdir) - next unless real_binary.start_with?("#{real_tmpdir}/") - - relative = binary.sub("#{tmpdir}/", "") - - # Normalize paths from cached gems where binaries were packaged - # from gem_dir build artifacts or dirty extension_dirs. - # ext//.so → .so (build artifact) - # lib//.so → /.so (gem lib path) - # extension///.so → .so (extension_dir artifact) - relative = relative.sub(%r{\Aext/[^/]+/}, "") if relative.start_with?("ext/") - relative = relative.sub(%r{\Alib/}, "") if relative.start_with?("lib/") - relative = relative.sub(%r{\Aextensions?/[^/]+/[^/]+/}, "") if relative.start_with?("extension/", "extensions/") - - dest = File.join(spec.extension_dir, relative) + next unless safe_binary?(binary, tmpdir) + + dest = File.join(spec.extension_dir, normalized_path(binary, tmpdir)) FileUtils.mkdir_p(File.dirname(dest)) FileUtils.cp(binary, dest) extracted_count += 1 @@ -45,7 +27,7 @@ def self.install(gem_path, spec) end # Mark this extension_dir as prebake-managed for post-install validation - FileUtils.touch(File.join(spec.extension_dir, ".prebake")) if extracted_count > 0 + FileUtils.touch(File.join(spec.extension_dir, ".prebake")) if extracted_count.positive? Logger.info "Installed precompiled #{File.basename(gem_path)} " \ "(#{extracted_count} binary files)" @@ -55,5 +37,28 @@ def self.install(gem_path, spec) Logger.warn "Extraction failed for #{File.basename(gem_path)}: #{e.message}" raise end + + # Reject symlinks, empty files, and anything resolving outside tmpdir + # (path traversal via a crafted gem). + def self.safe_binary?(binary, tmpdir) + return false if File.symlink?(binary) + return false if File.empty?(binary) + + File.realpath(binary).start_with?("#{File.realpath(tmpdir)}/") + end + + # Normalize paths from cached gems where binaries were packaged from + # gem_dir build artifacts or dirty extension_dirs. + # ext//.so → .so (build artifact) + # lib//.so → /.so (gem lib path) + # extension///.so → .so (extension_dir artifact) + def self.normalized_path(binary, tmpdir) + relative = binary.sub("#{tmpdir}/", "") + relative = relative.sub(%r{\Aext/[^/]+/}, "") if relative.start_with?("ext/") + relative = relative.sub(%r{\Alib/}, "") if relative.start_with?("lib/") + return relative unless relative.start_with?("extension/", "extensions/") + + relative.sub(%r{\Aextensions?/[^/]+/[^/]+/}, "") + end end end diff --git a/lib/prebake/platform_gem_builder.rb b/lib/prebake/platform_gem_builder.rb index 92a5ad0..15c453b 100644 --- a/lib/prebake/platform_gem_builder.rb +++ b/lib/prebake/platform_gem_builder.rb @@ -54,34 +54,7 @@ def build_platform_spec(build_dir) Dir.glob(File.join(build_dir, "**/*.{so,bundle,dll}")).each { |f| File.delete(f) } ext_dir = @spec.extension_dir - if ext_dir && File.directory?(ext_dir) - # Collect binaries at root and one level deep (e.g., nokogiri/nokogiri.so). - binaries = Dir.glob(File.join(ext_dir, "*.{so,bundle,dll}")) + - Dir.glob(File.join(ext_dir, "*/*.{so,bundle,dll}")) - - # Ruby 4.0+ places compiled extensions in extension/// - # within extension_dir. Collect these too, normalizing their paths - # to root level so the cached gem is layout-agnostic. - Dir.glob(File.join(ext_dir, "extension/*/*/*.{so,bundle,dll}")).each do |binary| - relative = binary.delete_prefix("#{ext_dir}/") - normalized = relative.sub(%r{\Aextension/[^/]+/[^/]+/}, "") - # Skip if a root-level binary with the same name already exists - next if binaries.any? { |b| b.delete_prefix("#{ext_dir}/") == normalized } - - binaries << binary - end - - binaries.each do |binary| - next if File.symlink?(binary) - next if File.size(binary).zero? - relative = binary.delete_prefix("#{ext_dir}/") - # Normalize extension/// paths to root level - relative = relative.sub(%r{\Aextension/[^/]+/[^/]+/}, "") - dest = File.join(build_dir, relative) - FileUtils.mkdir_p(File.dirname(dest)) - FileUtils.cp(binary, dest) - end - end + copy_installed_binaries(ext_dir, build_dir) if ext_dir && File.directory?(ext_dir) prefix = "#{build_dir}/" compiled = Dir.glob(File.join(build_dir, "**/*.{so,bundle,dll}")) @@ -90,5 +63,38 @@ def build_platform_spec(build_dir) platform_spec end + + def copy_installed_binaries(ext_dir, build_dir) + installed_binaries(ext_dir).each do |binary| + next if File.symlink?(binary) + next if File.empty?(binary) + + dest = File.join(build_dir, root_level_path(binary, ext_dir)) + FileUtils.mkdir_p(File.dirname(dest)) + FileUtils.cp(binary, dest) + end + end + + # Binaries at root and one level deep (e.g., nokogiri/nokogiri.so), plus the + # extension/// layout Ruby 4.0+ installs into. A nested + # binary is skipped when a root-level one already claims its normalized + # path, so the cached gem stays layout-agnostic. + def installed_binaries(ext_dir) + binaries = Dir.glob(File.join(ext_dir, "*.{so,bundle,dll}")) + + Dir.glob(File.join(ext_dir, "*/*.{so,bundle,dll}")) + + Dir.glob(File.join(ext_dir, "extension/*/*/*.{so,bundle,dll}")).each do |binary| + normalized = root_level_path(binary, ext_dir) + next if binaries.any? { |b| b.delete_prefix("#{ext_dir}/") == normalized } + + binaries << binary + end + + binaries + end + + def root_level_path(binary, ext_dir) + binary.delete_prefix("#{ext_dir}/").sub(%r{\Aextension/[^/]+/[^/]+/}, "") + end end end diff --git a/test/extension_validator_test.rb b/test/extension_validator_test.rb index f2e21ce..fa82b95 100644 --- a/test/extension_validator_test.rb +++ b/test/extension_validator_test.rb @@ -27,7 +27,7 @@ def test_skips_without_marker Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "foo.so")), - "Should not copy binaries when .prebake marker is absent" + "Should not copy binaries when .prebake marker is absent" end def test_skips_correct_layout @@ -45,7 +45,7 @@ def test_skips_correct_layout Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "bar.so")), - "Should not copy nested binaries when root-level binaries already exist" + "Should not copy nested binaries when root-level binaries already exist" end def test_fixes_extension_pattern @@ -96,7 +96,7 @@ def test_preserves_nested_lib_subdirectory Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "foo.so")), - "Should not copy binaries from lib// (only direct children of lib/)" + "Should not copy binaries from lib// (only direct children of lib/)" end def test_skips_symlinks @@ -115,7 +115,7 @@ def test_skips_symlinks Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "linked.so")), - "Should not copy symlinked binaries" + "Should not copy symlinked binaries" end def test_skips_empty_files @@ -131,7 +131,7 @@ def test_skips_empty_files Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "empty.so")), - "Should not copy zero-byte binaries" + "Should not copy zero-byte binaries" end def test_idempotent @@ -155,5 +155,4 @@ def test_idempotent root_binaries = Dir.glob(File.join(@extension_dir, "*.so")) assert_equal 1, root_binaries.length, "Should not duplicate files on second run" end - end