Allow PStore to reject symbolic links

lib/pstore.rb

@@ -329,10 +329,11 @@ class PStore
   # :stopdoc:
   VERSION = "0.2.1"
 
-  RDWR_ACCESS = {mode: IO::RDWR | IO::CREAT | IO::BINARY, encoding: Encoding::ASCII_8BIT}.freeze
-  RD_ACCESS = {mode: IO::RDONLY | IO::BINARY, encoding: Encoding::ASCII_8BIT}.freeze
-  WR_ACCESS = {mode: IO::WRONLY | IO::CREAT | IO::TRUNC | IO::BINARY, encoding: Encoding::ASCII_8BIT}.freeze
-  private_constant :RDWR_ACCESS, :RD_ACCESS, :WR_ACCESS
+  RDWR_ACCESS = IO::RDWR | IO::CREAT | IO::BINARY
+  RD_ACCESS = IO::RDONLY | IO::BINARY
+  WR_ACCESS = IO::WRONLY | IO::CREAT | IO::TRUNC | IO::BINARY
+  NOFOLLOW = IO.const_defined?(:NOFOLLOW) ? IO::NOFOLLOW : 0
+  private_constant :RDWR_ACCESS, :RD_ACCESS, :WR_ACCESS, :NOFOLLOW
   # :startdoc:
 
   # The error type thrown by all PStore methods.
@@ -367,12 +368,26 @@ class Error < StandardError
   #
   # A \PStore object is
   # {reentrant}[https://en.wikipedia.org/wiki/Reentrancy_(computing)].
-  # If argument +thread_safe+ is given as +true+,
+  # If argument or keyword argument +thread_safe+ is given as +true+,
   # the object is also thread-safe (at the cost of a small performance penalty):
   #
   #   store = PStore.new(path, true)
+  #   store = PStore.new(path, thread_safe: true)
   #
-  def initialize(file, thread_safe = false)
+  # If keyword argument +ultra_safe+ is given as +true+, the object is
+  # set to +ultra_safe+ mode.
+  #
+  #   store = PStore.new(path, ultra_safe: true)
+  #
+  # If keyword argument +follow_symlink+ is given as +false+, the store file
+  # itself must not be a symbolic link.  This option has no effect on platforms
+  # that do not support IO::NOFOLLOW.  Symbolic links in parent directories are
+  # not affected.
+  #
+  #   store = PStore.new(path, follow_symlink: false)
+  #
+  def initialize(file, _thread_safe = false, thread_safe: _thread_safe,
+                 ultra_safe: false, follow_symlink: true)
     dir = File::dirname(file)
     unless File::directory? dir
       raise PStore::Error, format("directory %s does not exist", dir)
@@ -382,8 +397,9 @@ def initialize(file, thread_safe = false)
     end
     @filename = File.path(file)
     @abort = false
-    @ultra_safe = false
+    @ultra_safe = ultra_safe
     @thread_safe = thread_safe
+    @nofollow = follow_symlink ? 0 : NOFOLLOW
     @lock = Thread::Mutex.new
   end
 
@@ -625,12 +641,12 @@ def open_and_lock_file(filename, read_only)
     loop do
       if read_only
         begin
-          file = File.new(filename, **RD_ACCESS)
+          file = File.new(filename, mode: RD_ACCESS | @nofollow, encoding: Encoding::ASCII_8BIT)
         rescue Errno::ENOENT
           return nil
         end
       else
-        file = File.new(filename, **RDWR_ACCESS)
+        file = File.new(filename, mode: RDWR_ACCESS | @nofollow, encoding: Encoding::ASCII_8BIT)
       end
       current = false
       begin
@@ -698,7 +714,7 @@ def save_data(original_checksum, original_file_size, file)
 
   def save_data_with_atomic_file_rename_strategy(data, file)
     temp_filename = "#{@filename}.tmp.#{Process.pid}.#{rand 1000000}"
-    temp_file = File.new(temp_filename, **WR_ACCESS, perm: 0o000)
+    temp_file = File.new(temp_filename, mode: WR_ACCESS | @nofollow, encoding: Encoding::ASCII_8BIT, perm: 0o000)
     begin
       temp_file.flock(File::LOCK_EX)
       temp_file.write(data)

pstore.gemspec

@@ -17,6 +17,7 @@ Gem::Specification.new do |spec|
   spec.description   = spec.summary
   spec.homepage      = "https://github.com/ruby/pstore"
   spec.licenses      = ["Ruby", "BSD-2-Clause"]
+  spec.required_ruby_version = ">= 2.4"
 
   spec.metadata["homepage_uri"] = spec.homepage
   spec.metadata["source_code_uri"] = "https://github.com/ruby/pstore"

test/test_pstore.rb

@@ -143,6 +143,46 @@ def test_thread_safe
     File.unlink(second_file) rescue nil
   end
 
+  def test_thread_safe_argument
+    assert_equal false, PStore.new(@pstore_file).instance_variable_get(:@thread_safe)
+    assert_equal true, PStore.new(@pstore_file, true).instance_variable_get(:@thread_safe)
+    assert_equal true, PStore.new(@pstore_file, thread_safe: true).instance_variable_get(:@thread_safe)
+    assert_equal false, PStore.new(@pstore_file, true, thread_safe: false).instance_variable_get(:@thread_safe)
+  end
+
+  def test_ultra_safe_argument
+    assert_equal false, PStore.new(@pstore_file).ultra_safe
+    assert_equal true, PStore.new(@pstore_file, ultra_safe: true).ultra_safe
+    assert_equal false, PStore.new(@pstore_file, ultra_safe: false).ultra_safe
+  end
+
+  def test_follows_symlink_by_default
+    Dir.mktmpdir do |dir|
+      target = File.join(dir, "target")
+      link = File.join(dir, "link")
+      PStore.new(target).transaction { |store| store[:key] = "value" }
+      File.symlink(target, link)
+
+      assert_equal "value", PStore.new(link).transaction(true) { |store| store[:key] }
+    end
+  end
+
+  def test_does_not_follow_symlink_when_disabled
+    omit("O_NOFOLLOW is not supported") unless nofollow_supported?
+
+    Dir.mktmpdir do |dir|
+      target = File.join(dir, "target")
+      link = File.join(dir, "link")
+      File.write(target, "")
+      File.symlink(target, link)
+
+      assert_raise(Errno::ELOOP) do
+        PStore.new(link, follow_symlink: false).transaction {}
+      end
+      assert_empty File.read(target)
+    end
+  end
+
   def test_nested_transaction_raises_error
     assert_raise(PStore::Error) do
       @pstore.transaction { @pstore.transaction { } }
@@ -203,6 +243,23 @@ def clear_store
     end
   end
 
+  def nofollow_supported?
+    return false unless IO.const_defined?(:NOFOLLOW)
+
+    Dir.mktmpdir do |dir|
+      target = File.join(dir, "target")
+      link = File.join(dir, "link")
+      File.write(target, "")
+      File.symlink(target, link)
+      File.open(link, IO::RDONLY|IO::NOFOLLOW).close
+    end
+    false
+  rescue Errno::ELOOP
+    true
+  rescue NotImplementedError, SystemCallError
+    false
+  end
+
   def second_file
     File.join(Dir.tmpdir, "pstore.tmp2.#{Process.pid}")
   end