Extract GitLab Pages using RubyZip

RubyZip allows us to perform strong validation of
expanded paths where we do extract file.

We introduce the following additional checks
to extract routines:

1. None of path components can be symlinked,
2. We drop privileges support for directories,
3. Symlink source needs to point within the target directory,
   like `public/`,
4. The symlink source needs to exist ahead of time.

10 files changed, 356 insertions, 9 deletions

diff --git a/spec/fixtures/pages_non_writeable.zip b/spec/fixtures/pages_non_writeable.zip
new file mode 100644
index 00000000000..69f175d8504
--- /dev/null
+++ b/spec/fixtures/pages_non_writeable.zip
diff --git a/spec/fixtures/safe_zip/invalid-symlink-does-not-exist.zip b/spec/fixtures/safe_zip/invalid-symlink-does-not-exist.zip
new file mode 100644
index 00000000000..b9ae1548713
--- /dev/null
+++ b/spec/fixtures/safe_zip/invalid-symlink-does-not-exist.zip
diff --git a/spec/fixtures/safe_zip/invalid-symlinks-outside.zip b/spec/fixtures/safe_zip/invalid-symlinks-outside.zip
new file mode 100644
index 00000000000..c184a1dafe2
--- /dev/null
+++ b/spec/fixtures/safe_zip/invalid-symlinks-outside.zip
diff --git a/spec/fixtures/safe_zip/valid-non-writeable.zip b/spec/fixtures/safe_zip/valid-non-writeable.zip
new file mode 100644
index 00000000000..69f175d8504
--- /dev/null
+++ b/spec/fixtures/safe_zip/valid-non-writeable.zip
diff --git a/spec/fixtures/safe_zip/valid-simple.zip b/spec/fixtures/safe_zip/valid-simple.zip
new file mode 100644
index 00000000000..a56b8b41dcc
--- /dev/null
+++ b/spec/fixtures/safe_zip/valid-simple.zip
diff --git a/spec/fixtures/safe_zip/valid-symlinks-first.zip b/spec/fixtures/safe_zip/valid-symlinks-first.zip
new file mode 100644
index 00000000000..f5952ef71c9
--- /dev/null
+++ b/spec/fixtures/safe_zip/valid-symlinks-first.zip
diff --git a/spec/lib/safe_zip/entry_spec.rb b/spec/lib/safe_zip/entry_spec.rb
new file mode 100644
index 00000000000..115e28c5994
--- /dev/null
+++ b/spec/lib/safe_zip/entry_spec.rb
@@ -0,0 +1,196 @@
+require 'spec_helper'
+
+describe SafeZip::Entry do
+  let(:target_path) { Dir.mktmpdir('safe-zip') }
+  let(:directories) { %w(public folder/with/subfolder) }
+  let(:params) { SafeZip::ExtractParams.new(directories: directories, to: target_path) }
+
+  let(:entry) { described_class.new(zip_archive, zip_entry, params) }
+  let(:entry_name) { 'public/folder/index.html' }
+  let(:entry_path_dir) { File.join(target_path, File.dirname(entry_name)) }
+  let(:entry_path) { File.join(target_path, entry_name) }
+  let(:zip_archive) { double }
+
+  let(:zip_entry) do
+    double(
+      name: entry_name,
+      file?: false,
+      directory?: false,
+      symlink?: false)
+  end
+
+  after do
+    FileUtils.remove_entry_secure(target_path)
+  end
+
+  context '#path_dir' do
+    subject { entry.path_dir }
+
+    it { is_expected.to eq(target_path + '/public/folder') }
+  end
+
+  context '#exist?' do
+    subject { entry.exist? }
+
+    context 'when entry does not exist' do
+      it { is_expected.not_to be_truthy }
+    end
+
+    context 'when entry does exist' do
+      before do
+        create_entry
+      end
+
+      it { is_expected.to be_truthy }
+    end
+  end
+
+  describe '#extract' do
+    subject { entry.extract }
+
+    context 'when entry does not match the filtered directories' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:entry_name) do
+        [
+          'assets/folder/index.html',
+          'public/../folder/index.html',
+          'public/../../../../../index.html',
+          '../../../../../public/index.html',
+          '/etc/passwd'
+        ]
+      end
+
+      with_them do
+        it 'does not extract file' do
+          is_expected.to be_falsey
+        end
+      end
+    end
+
+    context 'when entry does exist' do
+      before do
+        create_entry
+      end
+
+      it 'raises an exception' do
+        expect { subject }.to raise_error(SafeZip::Extract::AlreadyExistsError)
+      end
+    end
+
+    context 'when entry type is unknown' do
+      it 'raises an exception' do
+        expect { subject }.to raise_error(SafeZip::Extract::UnsupportedEntryError)
+      end
+    end
+
+    context 'when entry is valid' do
+      shared_examples 'secured symlinks' do
+        context 'when we try to extract entry into symlinked folder' do
+          before do
+            FileUtils.mkdir_p(File.join(target_path, "source"))
+            File.symlink("source", File.join(target_path, "public"))
+          end
+
+          it 'raises an exception' do
+            expect { subject }.to raise_error(SafeZip::Extract::PermissionDeniedError)
+          end
+        end
+      end
+
+      context 'and is file' do
+        before do
+          allow(zip_entry).to receive(:file?) { true }
+        end
+
+        it 'does extract file' do
+          expect(zip_archive).to receive(:extract)
+            .with(zip_entry, entry_path)
+            .and_return(true)
+
+          is_expected.to be_truthy
+        end
+
+        it_behaves_like 'secured symlinks'
+      end
+
+      context 'and is directory' do
+        let(:entry_name) { 'public/folder/assets' }
+
+        before do
+          allow(zip_entry).to receive(:directory?) { true }
+        end
+
+        it 'does create directory' do
+          is_expected.to be_truthy
+
+          expect(File.exist?(entry_path)).to eq(true)
+        end
+
+        it_behaves_like 'secured symlinks'
+      end
+
+      context 'and is symlink' do
+        let(:entry_name) { 'public/folder/assets' }
+
+        before do
+          allow(zip_entry).to receive(:symlink?) { true }
+          allow(zip_archive).to receive(:read).with(zip_entry) { entry_symlink }
+        end
+
+        shared_examples 'a valid symlink' do
+          it 'does create symlink' do
+            is_expected.to be_truthy
+
+            expect(File.exist?(entry_path)).to eq(true)
+          end
+        end
+
+        context 'when source is within target' do
+          let(:entry_symlink) { '../images' }
+
+          context 'but does not exist' do
+            it 'raises an exception' do
+              expect { subject }.to raise_error(SafeZip::Extract::SymlinkSourceDoesNotExistError)
+            end
+          end
+
+          context 'and does exist' do
+            before do
+              FileUtils.mkdir_p(File.join(target_path, 'public', 'images'))
+            end
+
+            it_behaves_like 'a valid symlink'
+          end
+        end
+
+        context 'when source points outside of target' do
+          let(:entry_symlink) { '../../images' }
+
+          before do
+            FileUtils.mkdir(File.join(target_path, 'images'))
+          end
+
+          it 'raises an exception' do
+            expect { subject }.to raise_error(SafeZip::Extract::PermissionDeniedError)
+          end
+        end
+
+        context 'when source points to /etc/passwd' do
+          let(:entry_symlink) { '/etc/passwd' }
+
+          it 'raises an exception' do
+            expect { subject }.to raise_error(SafeZip::Extract::PermissionDeniedError)
+          end
+        end
+      end
+    end
+  end
+
+  private
+
+  def create_entry
+    FileUtils.mkdir_p(entry_path_dir)
+    FileUtils.touch(entry_path)
+  end
+end
diff --git a/spec/lib/safe_zip/extract_params_spec.rb b/spec/lib/safe_zip/extract_params_spec.rb
new file mode 100644
index 00000000000..85e22cfa495
--- /dev/null
+++ b/spec/lib/safe_zip/extract_params_spec.rb
@@ -0,0 +1,54 @@
+require 'spec_helper'
+
+describe SafeZip::ExtractParams do
+  let(:target_path) { Dir.mktmpdir("safe-zip") }
+  let(:params) { described_class.new(directories: directories, to: target_path) }
+  let(:directories) { %w(public folder/with/subfolder) }
+
+  after do
+    FileUtils.remove_entry_secure(target_path)
+  end
+
+  describe '#extract_path' do
+    subject { params.extract_path }
+
+    it { is_expected.to eq(target_path) }
+  end
+
+  describe '#matching_target_directory' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { params.matching_target_directory(target_path + path) }
+
+    where(:path, :result) do
+      '/public/index.html' | '/public/'
+      '/non/existing/path' | nil
+      '/public' | nil
+      '/folder/with/index.html' | nil
+    end
+
+    with_them do
+      it { is_expected.to eq(result ? target_path + result : nil) }
+    end
+  end
+
+  describe '#target_directories' do
+    subject { params.target_directories }
+
+    it 'starts with target_path' do
+      is_expected.to all(start_with(target_path + '/'))
+    end
+
+    it 'ends with / for all paths' do
+      is_expected.to all(end_with('/'))
+    end
+  end
+
+  describe '#directories_wildcard' do
+    subject { params.directories_wildcard }
+
+    it 'adds * for all paths' do
+      is_expected.to all(end_with('/*'))
+    end
+  end
+end
diff --git a/spec/lib/safe_zip/extract_spec.rb b/spec/lib/safe_zip/extract_spec.rb
new file mode 100644
index 00000000000..dc7e25c0cf6
--- /dev/null
+++ b/spec/lib/safe_zip/extract_spec.rb
@@ -0,0 +1,80 @@
+require 'spec_helper'
+
+describe SafeZip::Extract do
+  let(:target_path) { Dir.mktmpdir('safe-zip') }
+  let(:directories) { %w(public) }
+  let(:object) { described_class.new(archive) }
+  let(:archive) { Rails.root.join('spec', 'fixtures', 'safe_zip', archive_name) }
+
+  after do
+    FileUtils.remove_entry_secure(target_path)
+  end
+
+  context '#extract' do
+    subject { object.extract(directories: directories, to: target_path) }
+
+    shared_examples 'extracts archive' do |param|
+      before do
+        stub_feature_flags(safezip_use_rubyzip: param)
+      end
+
+      it 'does extract archive' do
+        subject
+
+        expect(File.exist?(File.join(target_path, 'public', 'index.html'))).to eq(true)
+        expect(File.exist?(File.join(target_path, 'source'))).to eq(false)
+      end
+    end
+
+    shared_examples 'fails to extract archive' do |param|
+      before do
+        stub_feature_flags(safezip_use_rubyzip: param)
+      end
+
+      it 'does not extract archive' do
+        expect { subject }.to raise_error(SafeZip::Extract::Error)
+      end
+    end
+
+    %w(valid-simple.zip valid-symlinks-first.zip valid-non-writeable.zip).each  do |name|
+      context "when using #{name} archive" do
+        let(:archive_name) { name }
+
+        context 'for RubyZip' do
+          it_behaves_like 'extracts archive', true
+        end
+
+        context 'for UnZip' do
+          it_behaves_like 'extracts archive', false
+        end
+      end
+    end
+
+    %w(invalid-symlink-does-not-exist.zip invalid-symlinks-outside.zip).each do |name|
+      context "when using #{name} archive" do
+        let(:archive_name) { name }
+
+        context 'for RubyZip' do
+          it_behaves_like 'fails to extract archive', true
+        end
+
+        context 'for UnZip (UNSAFE)' do
+          it_behaves_like 'extracts archive', false
+        end
+      end
+    end
+
+    context 'when no matching directories are found' do
+      let(:archive_name) { 'valid-simple.zip' }
+      let(:directories) { %w(non/existing) }
+
+      context 'for RubyZip' do
+        it_behaves_like 'fails to extract archive', true
+      end
+
+      context 'for UnZip' do
+        it_behaves_like 'fails to extract archive', false
+      end
+    end
+  end
+end
diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb
index 36b619ba9be..8b70845befe 100644
--- a/spec/services/projects/update_pages_service_spec.rb
+++ b/spec/services/projects/update_pages_service_spec.rb
@@ -5,24 +5,27 @@ describe Projects::UpdatePagesService do
   set(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) }
   set(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') }
   let(:invalid_file) { fixture_file_upload('spec/fixtures/dk.png') }
-  let(:extension) { 'zip' }
 
-  let(:file) { fixture_file_upload("spec/fixtures/pages.#{extension}") }
-  let(:empty_file) { fixture_file_upload("spec/fixtures/pages_empty.#{extension}") }
-  let(:metadata) do
-    filename = "spec/fixtures/pages.#{extension}.meta"
-    fixture_file_upload(filename) if File.exist?(filename)
-  end
+  let(:file) { fixture_file_upload("spec/fixtures/pages.zip") }
+  let(:empty_file) { fixture_file_upload("spec/fixtures/pages_empty.zip") }
+  let(:metadata_filename) { "spec/fixtures/pages.zip.meta" }
+  let(:metadata) { fixture_file_upload(metadata_filename) if File.exist?(metadata_filename) }
 
   subject { described_class.new(project, build) }
 
   before do
+    stub_feature_flags(safezip_use_rubyzip: true)
+
     project.remove_pages
   end
 
-  context 'legacy artifacts' do
-    let(:extension) { 'zip' }
+  context '::TMP_EXTRACT_PATH' do
+    subject { described_class::TMP_EXTRACT_PATH }
 
+    it { is_expected.not_to match(Gitlab::PathRegex.namespace_format_regex) }
+  end
+
+  context 'legacy artifacts' do
     before do
       build.update(legacy_artifacts_file: file)
       build.update(legacy_artifacts_metadata: metadata)
@@ -132,6 +135,20 @@ describe Projects::UpdatePagesService do
         end
       end
 
+      context 'when using pages with non-writeable public' do
+        let(:file) { fixture_file_upload("spec/fixtures/pages_non_writeable.zip") }
+
+        context 'when using RubyZip' do
+          before do
+            stub_feature_flags(safezip_use_rubyzip: true)
+          end
+
+          it 'succeeds to extract' do
+            expect(execute).to eq(:success)
+          end
+        end
+      end
+
       context 'when timeout happens by DNS error' do
         before do
           allow_any_instance_of(described_class)