Merge branch 'fix/unauthorized-access-to-build-data' into 'master'

Remove 'unscoped' from project builds selection

This is a fix for this security bug: https://gitlab.com/gitlab-org/gitlab-ce/issues/18188

/cc @kamil @grzegorz @stanhu

See merge request !1968

app/controllers/projects/artifacts_controller.rb

@@ -37,7 +37,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
   private
 
   def build
-    @build ||= project.builds.unscoped.find_by!(id: params[:build_id])
+    @build ||= project.builds.find_by!(id: params[:build_id])
   end
 
   def artifacts_file

app/controllers/projects/builds_controller.rb

@@ -73,7 +73,7 @@ class Projects::BuildsController < Projects::ApplicationController
   private
 
   def build
-    @build ||= project.builds.unscoped.find_by!(id: params[:id])
+    @build ||= project.builds.find_by!(id: params[:id])
   end
 
   def build_path(build)

spec/features/builds_spec.rb

@@ -7,6 +7,7 @@ describe "Builds" do
     login_as(:user)
     @commit = FactoryGirl.create :ci_commit
     @build = FactoryGirl.create :ci_build, commit: @commit
+    @build2 = FactoryGirl.create :ci_build
     @project = @commit.project
     @project.team << [@user, :developer]
   end
@@ -66,13 +67,24 @@ describe "Builds" do
   end
 
   describe "GET /:project/builds/:id" do
-    before do
-      visit namespace_project_build_path(@project.namespace, @project, @build)
+    context "Build from project" do
+      before do
+        visit namespace_project_build_path(@project.namespace, @project, @build)
+      end
+
+      it { expect(page.status_code).to eq(200) }
+      it { expect(page).to have_content @commit.sha[0..7] }
+      it { expect(page).to have_content @commit.git_commit_message }
+      it { expect(page).to have_content @commit.git_author_name }
     end
 
-    it { expect(page).to have_content @commit.sha[0..7] }
-    it { expect(page).to have_content @commit.git_commit_message }
-    it { expect(page).to have_content @commit.git_author_name }
+    context "Build from other project" do
+      before do
+        visit namespace_project_build_path(@project.namespace, @project, @build2)
+      end
+
+      it { expect(page.status_code).to eq(404) }
+    end
 
     context "Download artifacts" do
       before do
@@ -103,51 +115,125 @@ describe "Builds" do
   end
 
   describe "POST /:project/builds/:id/cancel" do
-    before do
-      @build.run!
-      visit namespace_project_build_path(@project.namespace, @project, @build)
-      click_link "Cancel"
+    context "Build from project" do
+      before do
+        @build.run!
+        visit namespace_project_build_path(@project.namespace, @project, @build)
+        click_link "Cancel"
+      end
+
+      it { expect(page.status_code).to eq(200) }
+      it { expect(page).to have_content 'canceled' }
+      it { expect(page).to have_content 'Retry' }
     end
 
-    it { expect(page).to have_content 'canceled' }
-    it { expect(page).to have_content 'Retry' }
+    context "Build from other project" do
+      before do
+        @build.run!
+        visit namespace_project_build_path(@project.namespace, @project, @build)
+        page.driver.post(cancel_namespace_project_build_path(@project.namespace, @project, @build2))
+      end
+
+      it { expect(page.status_code).to eq(404) }
+    end
   end
 
   describe "POST /:project/builds/:id/retry" do
-    before do
-      @build.run!
-      visit namespace_project_build_path(@project.namespace, @project, @build)
-      click_link "Cancel"
-      click_link 'Retry'
+    context "Build from project" do
+      before do
+        @build.run!
+        visit namespace_project_build_path(@project.namespace, @project, @build)
+        click_link 'Cancel'
+        click_link 'Retry'
+      end
+
+      it { expect(page.status_code).to eq(200) }
+      it { expect(page).to have_content 'pending' }
+      it { expect(page).to have_content 'Cancel' }
     end
 
-    it { expect(page).to have_content 'pending' }
-    it { expect(page).to have_content 'Cancel' }
+    context "Build from other project" do
+      before do
+        @build.run!
+        visit namespace_project_build_path(@project.namespace, @project, @build)
+        click_link 'Cancel'
+        page.driver.post(retry_namespace_project_build_path(@project.namespace, @project, @build2))
+      end
+
+      it { expect(page.status_code).to eq(404) }
+    end
   end
 
   describe "GET /:project/builds/:id/download" do
-    before do
-      @build.update_attributes(artifacts_file: artifacts_file)
-      visit namespace_project_build_path(@project.namespace, @project, @build)
-      page.within('.artifacts') { click_link 'Download' }
+    context "Build from project" do
+      before do
+        @build.update_attributes(artifacts_file: artifacts_file)
+        visit namespace_project_build_path(@project.namespace, @project, @build)
+        page.within('.artifacts') { click_link 'Download' }
+      end
+
+      it { expect(page.status_code).to eq(200) }
+      it { expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) }
     end
 
-    it { expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) }
+    context "Build from other project" do
+      before do
+        @build2.update_attributes(artifacts_file: artifacts_file)
+        visit download_namespace_project_build_artifacts_path(@project.namespace, @project, @build2)
+      end
+
+      it { expect(page.status_code).to eq(404) }
+    end
   end
 
   describe "GET /:project/builds/:id/raw" do
-    before do
-      Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
-      @build.run!
-      @build.trace = 'BUILD TRACE'
-      visit namespace_project_build_path(@project.namespace, @project, @build)
+    context "Build from project" do
+      before do
+        Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
+        @build.run!
+        @build.trace = 'BUILD TRACE'
+        visit namespace_project_build_path(@project.namespace, @project, @build)
+        page.within('.build-controls') { click_link 'Raw' }
+      end
+
+      it 'sends the right headers' do
+        expect(page.status_code).to eq(200)
+        expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8')
+        expect(page.response_headers['X-Sendfile']).to eq(@build.path_to_trace)
+      end
     end
 
-    it 'sends the right headers' do
-      page.within('.build-controls') { click_link 'Raw' }
+    context "Build from other project" do
+      before do
+        Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
+        @build2.run!
+        @build2.trace = 'BUILD TRACE'
+        visit raw_namespace_project_build_path(@project.namespace, @project, @build2)
+        puts page.status_code
+        puts current_url
+      end
+
+      it 'sends the right headers' do
+        expect(page.status_code).to eq(404)
+      end
+    end
+  end
+
+  describe "GET /:project/builds/:id/status" do
+    context "Build from project" do
+      before do
+        visit status_namespace_project_build_path(@project.namespace, @project, @build)
+      end
+
+      it { expect(page.status_code).to eq(200) }
+    end
+
+    context "Build from other project" do
+      before do
+        visit status_namespace_project_build_path(@project.namespace, @project, @build2)
+      end
 
-      expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8')
-      expect(page.response_headers['X-Sendfile']).to eq(@build.path_to_trace)
+      it { expect(page.status_code).to eq(404) }
     end
   end
 end