From 00fa7a7c55721156ea04376d5f497e0935d431b0 Mon Sep 17 00:00:00 2001 From: Patrik Ragnarsson Date: Thu, 28 Jul 2022 09:58:03 +0200 Subject: [PATCH] Only flip group writable bit if permissions differ Otherwise we get lots of false positives. Close #35 --- lib/rubygems/comparator/monitor.rb | 20 ++++++--- .../comparator/test_file_list_comparator.rb | 7 ++-- test/rubygems/comparator/test_monitor.rb | 42 ++++++++++++++++++- test/test_helper.rb | 6 +++ 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/lib/rubygems/comparator/monitor.rb b/lib/rubygems/comparator/monitor.rb index 183c5bc..6927b1d 100644 --- a/lib/rubygems/comparator/monitor.rb +++ b/lib/rubygems/comparator/monitor.rb @@ -47,7 +47,7 @@ def self.files_permissions_changes(prev_file, curr_file, ignore_group_writable=f curr_permissions = File.stat(curr_file).mode diff = prev_permissions ^ curr_permissions - diff ^= 020 if ignore_group_writable + diff ^= 020 if diff != 0 && ignore_group_writable if diff != 0 " (!) New permissions: " + @@ -59,13 +59,21 @@ def self.files_permissions_changes(prev_file, curr_file, ignore_group_writable=f def self.new_file_permissions(file, ignore_group_writable=false) file_permissions = File.stat(file).mode - formatted_file_permissions = format_permissions(file_permissions) - file_permissions ^= 020 if ignore_group_writable + expected_permissions = if DirUtils.gem_bin_file?(file) + 0100755 + else + 0100644 + end + + permissions_to_compare = if ignore_group_writable && file_permissions != expected_permissions + file_permissions ^ 020 + else + file_permissions + end - unless file_permissions == 0100644 || \ - (DirUtils.gem_bin_file?(file) && file_permissions == 0100755) - return " (!) Unexpected permissions: #{formatted_file_permissions}" + unless permissions_to_compare == expected_permissions + return " (!) Unexpected permissions: #{format_permissions(file_permissions)}" end '' end diff --git a/test/rubygems/comparator/test_file_list_comparator.rb b/test/rubygems/comparator/test_file_list_comparator.rb index d28edf2..d4c9ae0 100644 --- a/test/rubygems/comparator/test_file_list_comparator.rb +++ b/test/rubygems/comparator/test_file_list_comparator.rb @@ -9,9 +9,10 @@ def test_files_comparison assert_equal [], @report['files']['0.0.1->0.0.2']['deleted'].messages assert_equal [], @report['files']['0.0.1->0.0.2']['updated'].messages assert_equal "bin/lorem +3/-0", @report['files']['0.0.2->0.0.3']['added'].lines(1) - assert_equal "(!) File is not executable", @report['files']['0.0.2->0.0.3']['added'].lines(2).strip - assert_equal "(!) Shebang found: #!/usr/bin/ruby", @report['files']['0.0.2->0.0.3']['added'].lines(3).strip - assert_equal "lib/lorem/utils.rb +7/-0", @report['files']['0.0.2->0.0.3']['added'].lines(4).strip + assert_equal "(!) Unexpected permissions: 100664", @report['files']['0.0.2->0.0.3']['added'].lines(2).strip + assert_equal "(!) File is not executable", @report['files']['0.0.2->0.0.3']['added'].lines(3).strip + assert_equal "(!) Shebang found: #!/usr/bin/ruby", @report['files']['0.0.2->0.0.3']['added'].lines(4).strip + assert_equal "lib/lorem/utils.rb +7/-0", @report['files']['0.0.2->0.0.3']['added'].lines(5).strip assert_nil @report['files']['0.0.2->0.0.3']['added'].lines(6) assert_equal [], @report['files']['0.0.2->0.0.3']['deleted'].messages assert_equal [], @report['files']['0.0.2->0.0.3']['updated'].messages diff --git a/test/rubygems/comparator/test_monitor.rb b/test/rubygems/comparator/test_monitor.rb index 8b13e62..5928e37 100644 --- a/test/rubygems/comparator/test_monitor.rb +++ b/test/rubygems/comparator/test_monitor.rb @@ -49,6 +49,21 @@ def test_files_permissions_changes file1 = File.join(@v003, 'bin/lorem') file2 = File.join(@v004, 'bin/lorem') assert_equal '(!) New permissions: 100664 -> 100775', Gem::Comparator::Monitor.files_permissions_changes(file1, file2).strip + assert_equal '(!) New permissions: 100664 -> 100775', Gem::Comparator::Monitor.files_permissions_changes(file1, file2, true).strip + end + + def test_files_permissions_changes_no_change + file1 = Tempfile.new + file2 = Tempfile.new + begin + File.chmod(0644, file1) + File.chmod(0644, file2) + assert_equal '', Gem::Comparator::Monitor.files_permissions_changes(file1.path, file2.path) + assert_equal '', Gem::Comparator::Monitor.files_permissions_changes(file1.path, file2.path, true) + ensure + file1.unlink + file2.unlink + end end def test_files_permissions_changes_ignores_group_writable_added @@ -94,16 +109,39 @@ def test_new_file_permissions file1 = File.join(@v004, 'bin/lorem') file2 = File.join(@v004, 'lib/lorem.rb') assert_equal '(!) Unexpected permissions: 100775', Gem::Comparator::Monitor.new_file_permissions(file1).strip - assert_equal '(!) Unexpected permissions: 100664', Gem::Comparator::Monitor.new_file_permissions(file2).strip + assert_equal '(!) Unexpected permissions: 100664', Gem::Comparator::Monitor.new_file_permissions(file2).strip + ignore_group_writable = true + assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file1, ignore_group_writable).strip + assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file2, ignore_group_writable).strip + end + + def test_new_file_permissions_ignore_group_writable_when_not_group_writable + file = Tempfile.new + bin_file = temp_bin_file + begin + File.chmod(0644, file) + assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file.path, true) + + File.chmod(0755, bin_file) + assert_equal '', Gem::Comparator::Monitor.new_file_permissions(bin_file.path, true) + ensure + file.unlink + bin_file.unlink + end end def test_new_file_permissions_ignore_group_writable file = Tempfile.new + bin_file = temp_bin_file begin File.chmod(0664, file) - assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file, true) + assert_equal '', Gem::Comparator::Monitor.new_file_permissions(file.path, true) + + File.chmod(0775, bin_file) + assert_equal '', Gem::Comparator::Monitor.new_file_permissions(bin_file.path, true) ensure file.unlink + bin_file.unlink end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 7cae929..43282a4 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -11,6 +11,12 @@ def setup_file_permissions File.chmod(0775, File.join(gemfiles_path, 'lorem-0.0.4', 'bin', 'lorem')) end +def temp_bin_file + bin_dir = File.join(Dir.mktmpdir, "bin") + Dir.mkdir(bin_dir) + Tempfile.new("", bin_dir) +end + class TestGemComparator < Minitest::Test def setup super