Commit 184aa521 authored by Douwe Maan's avatar Douwe Maan

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
parents fc809d68 1f608ac4
...@@ -37,7 +37,7 @@ class Projects::ArtifactsController < Projects::ApplicationController ...@@ -37,7 +37,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
private private
def build def build
@build ||= project.builds.unscoped.find_by!(id: params[:build_id]) @build ||= project.builds.find_by!(id: params[:build_id])
end end
def artifacts_file def artifacts_file
......
...@@ -81,7 +81,7 @@ class Projects::BuildsController < Projects::ApplicationController ...@@ -81,7 +81,7 @@ class Projects::BuildsController < Projects::ApplicationController
private private
def build def build
@build ||= project.builds.unscoped.find_by!(id: params[:id]) @build ||= project.builds.find_by!(id: params[:id])
end end
def build_path(build) def build_path(build)
......
...@@ -7,6 +7,7 @@ describe "Builds" do ...@@ -7,6 +7,7 @@ describe "Builds" do
login_as(:user) login_as(:user)
@commit = FactoryGirl.create :ci_commit @commit = FactoryGirl.create :ci_commit
@build = FactoryGirl.create :ci_build, commit: @commit @build = FactoryGirl.create :ci_build, commit: @commit
@build2 = FactoryGirl.create :ci_build
@project = @commit.project @project = @commit.project
@project.team << [@user, :developer] @project.team << [@user, :developer]
end end
...@@ -66,13 +67,24 @@ describe "Builds" do ...@@ -66,13 +67,24 @@ describe "Builds" do
end end
describe "GET /:project/builds/:id" do describe "GET /:project/builds/:id" do
context "Build from project" do
before do before do
visit namespace_project_build_path(@project.namespace, @project, @build) visit namespace_project_build_path(@project.namespace, @project, @build)
end 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.sha[0..7] }
it { expect(page).to have_content @commit.git_commit_message } it { expect(page).to have_content @commit.git_commit_message }
it { expect(page).to have_content @commit.git_author_name } it { expect(page).to have_content @commit.git_author_name }
end
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 context "Download artifacts" do
before do before do
...@@ -103,51 +115,143 @@ describe "Builds" do ...@@ -103,51 +115,143 @@ describe "Builds" do
end end
describe "POST /:project/builds/:id/cancel" do describe "POST /:project/builds/:id/cancel" do
context "Build from project" do
before do before do
@build.run! @build.run!
visit namespace_project_build_path(@project.namespace, @project, @build) visit namespace_project_build_path(@project.namespace, @project, @build)
click_link "Cancel" click_link "Cancel"
end end
it { expect(page.status_code).to eq(200) }
it { expect(page).to have_content 'canceled' } it { expect(page).to have_content 'canceled' }
it { expect(page).to have_content 'Retry' } it { expect(page).to have_content 'Retry' }
end end
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 describe "POST /:project/builds/:id/retry" do
context "Build from project" do
before do before do
@build.run! @build.run!
visit namespace_project_build_path(@project.namespace, @project, @build) visit namespace_project_build_path(@project.namespace, @project, @build)
click_link "Cancel" click_link 'Cancel'
click_link 'Retry' click_link 'Retry'
end end
it { expect(page.status_code).to eq(200) }
it { expect(page).to have_content 'pending' } it { expect(page).to have_content 'pending' }
it { expect(page).to have_content 'Cancel' } it { expect(page).to have_content 'Cancel' }
end end
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 describe "GET /:project/builds/:id/download" do
context "Build from project" do
before do before do
@build.update_attributes(artifacts_file: artifacts_file) @build.update_attributes(artifacts_file: artifacts_file)
visit namespace_project_build_path(@project.namespace, @project, @build) visit namespace_project_build_path(@project.namespace, @project, @build)
page.within('.artifacts') { click_link 'Download' } page.within('.artifacts') { click_link 'Download' }
end end
it { expect(page.status_code).to eq(200) }
it { expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) } it { expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) }
end 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/raw" do describe "GET /:project/builds/:id/raw" do
context "Build from project" do
before do before do
Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile') Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
@build.run! @build.run!
@build.trace = 'BUILD TRACE' @build.trace = 'BUILD TRACE'
visit namespace_project_build_path(@project.namespace, @project, @build) visit namespace_project_build_path(@project.namespace, @project, @build)
page.within('.build-controls') { click_link 'Raw' }
end end
it 'sends the right headers' do it 'sends the right headers' do
page.within('.build-controls') { click_link 'Raw' } expect(page.status_code).to eq(200)
expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8') 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) expect(page.response_headers['X-Sendfile']).to eq(@build.path_to_trace)
end end
end end
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/trace.json" do
context "Build from project" do
before do
visit trace_namespace_project_build_path(@project.namespace, @project, @build, format: :json)
end
it { expect(page.status_code).to eq(200) }
end
context "Build from other project" do
before do
visit trace_namespace_project_build_path(@project.namespace, @project, @build2, format: :json)
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
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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment