🔒 Fix command injection, path traversal, and file disclosure in deposits

app/controllers/concerns/willow_sword/fetch_headers.rb

@@ -25,16 +25,50 @@ def fetch_content_type
     end
 
     def fetch_filename
-      @headers[:filename] = nil
       cd = request.headers.fetch('Content-Disposition', '')
-      if cd.include? '='
-        @headers[:filename] = cd.split('=')[-1].strip()
+      @headers[:filename] = sanitize_filename(extract_cd_filename(cd))
+    end
+
+    # Prefers RFC 5987/6266 filename* over filename, matching parameter names
+    # exactly (case-insensitive) and respecting quoted-string boundaries so a
+    # filename*= inside a quoted value or a name like xfilename* can't win.
+    # RFC 2231 continuations (filename*0, filename*1) are not reassembled.
+    def extract_cd_filename(cd)
+      return nil if cd.blank?
+      params = {}
+      cd.scan(/(?:\A|;)\s*([^\s=;]+)\s*=\s*("(?:[^"\\]|\\.)*"|[^;]*)/) do |name, val|
+        val = val.start_with?('"') ? val[1..-2].gsub(/\\(.)/, '\1') : val.strip
+        params[name.downcase] ||= val
       end
-      if @headers[:filename].blank?
-        @headers[:filename] = SecureRandom.uuid
+      if params.key?('filename*')
+        decode_ext_value(params['filename*'])
+      elsif params.key?('filename')
+        params['filename']
       end
     end
 
+    # RFC 5987 ext-value: charset'lang'percent-encoded-value
+    def decode_ext_value(val)
+      charset, _lang, encoded = val.split("'", 3)
+      encoded ||= charset # no charset'lang' prefix; treat whole thing as value
+      bytes = encoded.gsub(/%([0-9a-fA-F]{2})/) { $1.hex.chr }
+      src = charset.to_s.casecmp?('iso-8859-1') ? Encoding::ISO_8859_1 : Encoding::UTF_8
+      bytes.force_encoding(src).encode(Encoding::UTF_8, invalid: :replace, undef: :replace)
+    end
+
+    # Blocks path traversal and control/bidi chars. Spaces, unicode, and shell
+    # metacharacters are preserved and safe only because nothing shells out with
+    # this value (see validate_payload, get_content_type) - keep it that way.
+    def sanitize_filename(name)
+      # Header bytes arrive as ASCII-8BIT; tag as UTF-8 so the control/bidi
+      # regex below is encoding-compatible, then scrub any invalid sequences.
+      name = name.to_s.dup.force_encoding(Encoding::UTF_8).scrub('').strip
+      name = name.gsub(/[\u0000-\u001f\u007f\u200e\u200f\u202a-\u202e\u2066-\u2069]/, %q())
+      name = File.basename(name).tr('/\\', '_')
+      return SecureRandom.uuid if name.blank? || name == '.' || name == '..'
+      name
+    end
+
     def fetch_md5hash
       @headers[:md5hash] = request.headers.fetch('Content-MD5', nil)
     end

app/controllers/concerns/willow_sword/process_request.rb

@@ -1,3 +1,5 @@
+require 'digest'
+
 module WillowSword
   module ProcessRequest
     extend ActiveSupport::Concern
@@ -34,7 +36,7 @@ def validate_and_save_request
 
     def validate_payload
       return true if @headers[:md5hash].nil?
-      md5 = `md5sum "#{@file.path}" | awk '{ print $1 }'`.strip
+      md5 = Digest::MD5.file(@file.path).hexdigest
       if md5 == @headers[:md5hash]
         true
       else

app/controllers/concerns/willow_sword/save_data.rb

@@ -1,5 +1,6 @@
 require 'fileutils'
 require 'securerandom'
+require 'marcel'
 module WillowSword
   module SaveData
 
@@ -72,7 +73,7 @@ def fetch_data(data, type, is_metadata)
           if is_metadata
             new_file_name = 'metadata.xml'
           else
-            new_file_name = data.original_filename
+            new_file_name = sanitize_filename(data.original_filename)
           end
           path = File.join(@dir, new_file_name)
           tmp = data.tempfile
@@ -105,6 +106,9 @@ def organize_data(file_path)
       if content_type == 'application/zip'
         zp = WillowSword::ZipPackage.new(file_path, contents_path)
         zp.unzip_file
+        # A symlink in the archive would be dereferenced by the later bagit copy,
+        # ingesting the contents of whatever server file it points at.
+        reject_symlinks!(contents_path)
         validate_bagit(zp.dst) if @headers[:packaging] == 'http://purl.org/net/sword/package/BagIt'
       else
         # Copy file to contents dir
@@ -124,9 +128,33 @@ def verify_data
     end
 
     def get_content_type(file_path)
-      # @extension = Rack::Mime::MIME_TYPES.invert[mime_type]
-      # Not matching content_type and packaging from headers with that computed.
-      return `file --b --mime-type "#{file_path}"`.strip
+      # Detect by content only (open as IO, no name hint) so a misleading
+      # extension can't steer archive handling, matching the old `file` check.
+      File.open(file_path, 'rb') { |io| Marcel::MimeType.for(io) }
+    end
+
+    def reject_symlinks!(dir)
+      link = find_symlink(dir)
+      return unless link
+      message = "Archive contains a symbolic link, which is not allowed"
+      # Set @error (the work controller's local rescue keeps status only when
+      # it's present) and raise a real exception wrapper for the other paths.
+      @error = WillowSword::Error.new(message, :unprocessable_entity)
+      raise WillowSword::SwordError.new(@error)
+    end
+
+    # Recursively finds the first symlink under dir without following symlinked
+    # directories (the symlink? check precedes the directory? descent).
+    def find_symlink(dir)
+      Dir.each_child(dir) do |name|
+        path = File.join(dir, name)
+        return path if File.symlink?(path)
+        if File.directory?(path)
+          found = find_symlink(path)
+          return found if found
+        end
+      end
+      nil
     end
 
     def validate_bagit(file_path)
@@ -135,7 +163,8 @@ def validate_bagit(file_path)
       unless bag.valid?
         error_details = bag.errors.any? ? bag.errors.errors.values.join('; ') : ''
         message = "Invalid BagIt package: #{error_details}"
-        raise @error = WillowSword::Error.new(message, :unprocessable_entity)
+        @error = WillowSword::Error.new(message, :unprocessable_entity)
+        raise WillowSword::SwordError.new(@error)
       end
 
       true

spec/controllers/concerns/willow_sword/fetch_headers_spec.rb

@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+# Exercised through a controller that includes the FetchHeaders concern.
+RSpec.describe WillowSword::V2::WorksController, type: :controller do
+  describe '#fetch_filename (Content-Disposition sanitization)' do
+    def filename_for(cd)
+      controller.request.headers['Content-Disposition'] = cd
+      controller.instance_variable_set(:@headers, {})
+      controller.send(:fetch_filename)
+      controller.instance_variable_get(:@headers)[:filename]
+    end
+
+    it 'strips path separators from an injection-style payload' do
+      name = filename_for('attachment; filename=x"; touch /tmp/pwned; echo "')
+      expect(name).not_to match(%r{[/\\]})
+    end
+
+    it 'prevents path traversal' do
+      expect(filename_for('attachment; filename="../../../../etc/passwd"')).to eq('passwd')
+      expect(filename_for('attachment; filename="..\\..\\windows\\x"')).not_to match(%r{[/\\]})
+    end
+
+    it 'preserves spaces and unicode in the filename' do
+      expect(filename_for('attachment; filename="my report.pdf"')).to eq('my report.pdf')
+      expect(filename_for('attachment; filename="café.pdf"')).to eq('café.pdf')
+    end
+
+    it 'keeps a quoted filename containing "=" intact (old split truncated it)' do
+      expect(filename_for('attachment; filename="a=b.pdf"')).to eq('a=b.pdf')
+    end
+
+    it 'ignores trailing non-filename parameters' do
+      expect(filename_for('attachment; filename="report.pdf"; foo=bar')).to eq('report.pdf')
+    end
+
+    it 'does not treat filename*= inside a quoted value as the extended param' do
+      expect(filename_for('attachment; filename="notes filename*=draft.pdf"')).to eq('notes filename*=draft.pdf')
+    end
+
+    it 'matches parameter names exactly (xfilename* is not filename*)' do
+      expect(filename_for('attachment; xfilename*=draft.pdf')).to match(/\A[0-9a-f-]{36}\z/)
+    end
+
+    it 'prefers and percent-decodes filename* (RFC 5987/6266)' do
+      expect(filename_for("attachment; filename*=UTF-8''%e2%82%ac%20rates.pdf")).to eq('€ rates.pdf')
+    end
+
+    it 'decodes filename* with a non-empty language tag' do
+      expect(filename_for("attachment; filename*=UTF-8'en'%E2%82%AC.pdf")).to eq('€.pdf')
+    end
+
+    it 'does not leave a path separator from a slash-only filename' do
+      expect(filename_for('attachment; filename="/"')).not_to match(%r{[/\\]})
+    end
+
+    it 'handles a raw ASCII-8BIT filename with high bytes without raising' do
+      # Rack delivers header values as ASCII-8BIT; a UTF-8 regex against those
+      # bytes raised Encoding::CompatibilityError before the force_encoding fix.
+      raw = 'café.pdf'.dup.force_encoding('ASCII-8BIT')
+      expect(controller.send(:sanitize_filename, raw)).to eq('café.pdf')
+    end
+
+    it 'matches the parameter name case-insensitively' do
+      expect(filename_for('attachment; FileName="Doc.pdf"')).to eq('Doc.pdf')
+    end
+
+    it 'falls back to a UUID when no filename is present' do
+      expect(filename_for('attachment')).to match(/\A[0-9a-f-]{36}\z/)
+      expect(filename_for('')).to match(/\A[0-9a-f-]{36}\z/)
+    end
+
+    it 'falls back to a UUID when sanitization leaves nothing usable' do
+      expect(filename_for('attachment; filename="../"')).to match(/\A[0-9a-f-]{36}\z/)
+    end
+
+    it 'does not raise on invalid UTF-8 bytes from a decoded filename*' do
+      # regression: strip ran before scrub and raised ArgumentError -> unauthenticated 500
+      expect { filename_for("attachment; filename*=UTF-8''%ff%fe.txt") }.not_to raise_error
+    end
+  end
+end

spec/controllers/concerns/willow_sword/save_data_spec.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+require 'tmpdir'
+
+RSpec.describe WillowSword::V2::WorksController, type: :controller do
+  describe '#reject_symlinks! (zip symlink file-disclosure guard)' do
+    around do |example|
+      Dir.mktmpdir { |d| @dir = d; example.run }
+    end
+
+    it 'rejects the deposit (422) when an extracted archive contains a symlink' do
+      File.write(File.join(@dir, 'ok.txt'), 'fine')
+      File.symlink('/etc/passwd', File.join(@dir, 'exfil.txt'))
+
+      expect { controller.send(:reject_symlinks!, @dir) }
+        .to raise_error(WillowSword::SwordError) { |e| expect(e.sword_error.code).to eq(422) }
+      # @error must be set: the works controller's local rescue only keeps the
+      # 422 status when @error is already present, else it defaults to 400.
+      expect(controller.instance_variable_get(:@error).code).to eq(422)
+    end
+
+    it 'detects a symlink nested in a real subdirectory' do
+      Dir.mkdir(File.join(@dir, 'sub'))
+      File.symlink('/etc/passwd', File.join(@dir, 'sub', 'exfil.txt'))
+
+      expect { controller.send(:reject_symlinks!, @dir) }.to raise_error(StandardError)
+    end
+
+    it 'does not descend into a symlinked directory (detects it, does not follow)' do
+      File.symlink('/etc', File.join(@dir, 'evil'))
+      # returns the symlink itself; must not raise SystemStackError from recursing into /etc
+      expect(controller.send(:find_symlink, @dir)).to eq(File.join(@dir, 'evil'))
+    end
+
+    it 'passes cleanly when no symlinks are present' do
+      File.write(File.join(@dir, 'a.txt'), 'x')
+      Dir.mkdir(File.join(@dir, 'sub'))
+      File.write(File.join(@dir, 'sub', 'b.txt'), 'y')
+
+      expect { controller.send(:reject_symlinks!, @dir) }.not_to raise_error
+      expect(controller.instance_variable_get(:@error)).to be_nil
+    end
+  end
+end