Path could also have slashes! Feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_14347729

4 files changed, 36 insertions, 10 deletions

diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index 60e432d68d8..17c6d56c8b9 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -1,4 +1,6 @@
 class Projects::ArtifactsController < Projects::ApplicationController
+  include ExtractsPath
+
   layout 'project'
   before_action :authorize_read_build!
   before_action :authorize_update_build!, only: [:keep]
@@ -35,7 +37,8 @@ class Projects::ArtifactsController < Projects::ApplicationController
   end
 
   def latest_succeeded
-    target_path = artifacts_action_path(params[:path], project, build)
+    path = ref_name_and_path.last
+    target_path = artifacts_action_path(path, project, build)
 
     if target_path
       redirect_to(target_path)
@@ -59,13 +62,18 @@ class Projects::ArtifactsController < Projects::ApplicationController
   end
 
   def build_from_ref
-    if params[:ref_name]
-      builds = project.latest_successful_builds_for(params[:ref_name])
+    if params[:ref_name_and_path]
+      ref_name = ref_name_and_path.first
+      builds = project.latest_successful_builds_for(ref_name)
 
       builds.find_by(name: params[:job])
     end
   end
 
+  def ref_name_and_path
+    @ref_name_and_path ||= extract_ref(params[:ref_name_and_path])
+  end
+
   def artifacts_file
     @artifacts_file ||= build.artifacts_file
   end
diff --git a/app/helpers/gitlab_routing_helper.rb b/app/helpers/gitlab_routing_helper.rb
index cd526f17b99..a322a90cc4e 100644
--- a/app/helpers/gitlab_routing_helper.rb
+++ b/app/helpers/gitlab_routing_helper.rb
@@ -153,9 +153,10 @@ module GitlabRoutingHelper
   # Artifacts
 
   def artifacts_action_path(path, project, build)
-    args = [project.namespace, project, build]
+    action, path_params = path.split('/', 2)
+    args = [project.namespace, project, build, path_params]
 
-    case path
+    case action
     when 'download'
       download_namespace_project_build_artifacts_path(*args)
     when 'browse'
diff --git a/config/routes.rb b/config/routes.rb
index 606181ff837..879cd61a02f 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -787,9 +787,8 @@ Rails.application.routes.draw do
             resources :artifacts, only: [] do
               collection do
                 get :latest_succeeded,
-                  path: ':ref_name/*path',
-                  format: false,
-                  constraints: { ref_name: /.+/ } # could have /
+                  path: '*ref_name_and_path',
+                  format: false
               end
             end
           end
diff --git a/spec/requests/projects/artifacts_controller_spec.rb b/spec/requests/projects/artifacts_controller_spec.rb
index 3ba6725efc3..e02f0eacc93 100644
--- a/spec/requests/projects/artifacts_controller_spec.rb
+++ b/spec/requests/projects/artifacts_controller_spec.rb
@@ -26,8 +26,7 @@ describe Projects::ArtifactsController do
       latest_succeeded_namespace_project_artifacts_path(
         project.namespace,
         project,
-        ref,
-        path,
+        [ref, path].join('/'),
         job: job)
     end
 
@@ -94,6 +93,25 @@ describe Projects::ArtifactsController do
 
         it_behaves_like 'redirect to the build'
       end
+
+      context 'with branch name and path containing slashes' do
+        before do
+          pipeline.update(ref: 'improve/awesome',
+                          sha: project.commit('improve/awesome').sha)
+
+          get path_from_ref('improve/awesome', build.name, 'file/README.md')
+        end
+
+        it 'redirects' do
+          path = file_namespace_project_build_artifacts_path(
+            project.namespace,
+            project,
+            build,
+            'README.md')
+
+          expect(response).to redirect_to(path)
+        end
+      end
     end
   end
 end