Render only valid paths in artifacts metadata

In this version we will support only relative paths in artifacts
metadata. Support for absolute paths will be introduced later.

6 files changed, 28 insertions, 19 deletions

diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index 9f9861dec79..f88d866febc 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -16,7 +16,10 @@ class Projects::ArtifactsController < Projects::ApplicationController
 
   def browse
     return render_404 unless build.artifacts?
-    @path = build.artifacts_metadata_path(params[:path].to_s)
+
+    directory = params[:path] ? "#{params[:path]}/" : ''
+    @path = build.artifacts_metadata_path(directory)
+
     return render_404 unless @path.exists?
   end
 
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 2d2206cc4d7..7593ceda488 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -347,10 +347,8 @@ module Ci
       artifacts? && artifacts_file.path.end_with?('zip') && artifacts_metadata.exists?
     end
 
-
     def artifacts_metadata_path(path)
-      metadata_file = artifacts_metadata.path
-      Gitlab::Ci::Build::Artifacts::Metadata.new(metadata_file, path).to_path
+      Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path).to_path
     end
 
     private
diff --git a/lib/gitlab/ci/build/artifacts/metadata.rb b/lib/gitlab/ci/build/artifacts/metadata.rb
index 996b5d91ff2..91017f633a0 100644
--- a/lib/gitlab/ci/build/artifacts/metadata.rb
+++ b/lib/gitlab/ci/build/artifacts/metadata.rb
@@ -12,7 +12,6 @@ module Gitlab
           def initialize(file, path)
             @file, @path = file, path
             @full_version = read_version
-            @path << '/' unless path.end_with?('/') || path.empty?
           end
 
           def version
@@ -43,14 +42,15 @@ module Gitlab
 
           def match_entries(gz)
             paths, metadata = [], []
-            child_pattern = %r{^#{Regexp.escape(@path)}[^/\s]*/?$}
+            match_pattern = %r{^#{Regexp.escape(@path)}[^/\s]*/?$}
 
             until gz.eof? do
               begin
                 path = read_string(gz)
                 meta = read_string(gz)
                
-                next unless path =~ child_pattern
+                next unless path =~ match_pattern
+                next unless path_valid?(path)
 
                 paths.push(path)
                 metadata.push(JSON.parse(meta.chomp, symbolize_names: true))
@@ -62,6 +62,10 @@ module Gitlab
             [paths, metadata]
           end
 
+          def path_valid?(path)
+            !(path.start_with?('/') || path =~ %r{\.?\./})
+          end
+
           def read_version
             gzip do|gz|
               version_string = read_string(gz)
diff --git a/lib/gitlab/ci/build/artifacts/metadata/path.rb b/lib/gitlab/ci/build/artifacts/metadata/path.rb
index 222903b348e..80ead335d57 100644
--- a/lib/gitlab/ci/build/artifacts/metadata/path.rb
+++ b/lib/gitlab/ci/build/artifacts/metadata/path.rb
@@ -23,7 +23,7 @@ module Gitlab
         end
 
         def directory?
-          @path.end_with?('/') || @path.blank?
+          blank_node? || @path.end_with?('/')
         end
 
         def file?
@@ -40,11 +40,11 @@ module Gitlab
         end
 
         def basename
-          directory? ? name + ::File::SEPARATOR : name
+          (directory? && !blank_node?) ? name + ::File::SEPARATOR : name
         end
 
         def name
-          @name || @path.split(::File::SEPARATOR).last
+          @name || @path.split(::File::SEPARATOR).last.to_s
         end
 
         def children
@@ -83,7 +83,11 @@ module Gitlab
         end
 
         def exists?
-          @path.blank? || @universe.include?(@path)
+          blank_node? || @universe.include?(@path)
+        end
+
+        def blank_node?
+          @path.empty? # "" is considered to be './'
         end
 
         def to_s
diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb
index 148d05b5902..3b254c3ce2f 100644
--- a/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb
+++ b/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb
@@ -108,14 +108,14 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Path do
     end
   end
 
-  describe '#nodes', path: './test' do
+  describe '#nodes', path: 'test' do
     subject { |example| path(example).nodes }
-    it { is_expected.to eq 2 }
+    it { is_expected.to eq 1 }
   end
 
-  describe '#nodes', path: './test/' do
+  describe '#nodes', path: 'test/' do
     subject { |example| path(example).nodes }
-    it { is_expected.to eq 2 }
+    it { is_expected.to eq 1 }
   end
 
   describe '#metadata' do
diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb
index 36c4851126c..456314768be 100644
--- a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb
+++ b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb
@@ -28,8 +28,8 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do
       end
     end
 
-    describe '#match! other_artifacts_0.1.2' do
-      subject { metadata('other_artifacts_0.1.2').match! }
+    describe '#match! other_artifacts_0.1.2/' do
+      subject { metadata('other_artifacts_0.1.2/').match! }
 
       it 'matches correct paths' do
         expect(subject.first).
@@ -39,7 +39,7 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do
       end
     end
 
-    describe '#match! other_artifacts_0.1.2/another-subdirectory' do
+    describe '#match! other_artifacts_0.1.2/another-subdirectory/' do
       subject { metadata('other_artifacts_0.1.2/another-subdirectory/').match! }
 
       it 'matches correct paths' do
@@ -52,7 +52,7 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do
 
     describe '#to_path' do
       subject { metadata('').to_path }
-      it { is_expected.to be_an_instance_of(Gitlab::Ci::Build::Artifacts::Metdata::Path) }
+      it { is_expected.to be_an_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Path) }
     end
 
     describe '#full_version' do