diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-06-06 11:59:06 +0300 |
---|---|---|
committer | Tomasz Maczukin <tomasz@maczukin.pl> | 2016-06-14 20:45:11 +0300 |
commit | 18968821ddd0307fcd36050ff27cc17dc739d0bc (patch) | |
tree | 047e51f673ee612cc9e1b2529aa1de27957e74d0 | |
parent | aba6bae46d5307258172d67f8ee1c1bb6cc3af0c (diff) |
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
-rw-r--r-- | app/controllers/projects/artifacts_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/builds_controller.rb | 2 | ||||
-rw-r--r-- | spec/features/builds_spec.rb | 115 |
3 files changed, 94 insertions, 25 deletions
diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index cfea1266516..832d7deb57d 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/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 diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb index f159e169f6d..95a2f3a59bc 100644 --- a/app/controllers/projects/builds_controller.rb +++ b/app/controllers/projects/builds_controller.rb @@ -65,7 +65,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) diff --git a/spec/features/builds_spec.rb b/spec/features/builds_spec.rb index 6da3a857b3f..e098904c420 100644 --- a/spec/features/builds_spec.rb +++ b/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 @@ -89,35 +101,92 @@ 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 + + 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/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 - it { expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) } + context "Build from other project" do + before do + visit status_namespace_project_build_path(@project.namespace, @project, @build2) + end + + it { expect(page.status_code).to eq(404) } + end end end |