diff options
5 files changed, 29 insertions, 12 deletions
diff --git a/app/services/terraform/remote_state_handler.rb b/app/services/terraform/remote_state_handler.rb index d2c44d4a265..7e79cb9e007 100644 --- a/app/services/terraform/remote_state_handler.rb +++ b/app/services/terraform/remote_state_handler.rb @@ -23,6 +23,8 @@ module Terraform state.save! unless state.destroyed? end + + nil end def lock! diff --git a/changelogs/unreleased/security-fix-terraform-state-exposed-object-storage-url.yml b/changelogs/unreleased/security-fix-terraform-state-exposed-object-storage-url.yml new file mode 100644 index 00000000000..1e37aed6ca0 --- /dev/null +++ b/changelogs/unreleased/security-fix-terraform-state-exposed-object-storage-url.yml @@ -0,0 +1,5 @@ +--- +title: Do not expose Terraform state record in API +merge_request: +author: +type: security diff --git a/lib/api/terraform/state.rb b/lib/api/terraform/state.rb index 4168cce21ef..3dbde4639ca 100644 --- a/lib/api/terraform/state.rb +++ b/lib/api/terraform/state.rb @@ -39,7 +39,6 @@ module API env['api.format'] = :binary # this bypasses json serialization body state.latest_file.read - status :ok end end @@ -53,8 +52,10 @@ module API remote_state_handler.handle_with_lock do |state| state.update_file!(CarrierWaveStringFile.new(data), version: params[:serial]) - status :ok end + + body false + status :ok end desc 'Delete a terraform state of a certain name' @@ -64,8 +65,10 @@ module API remote_state_handler.handle_with_lock do |state| state.destroy! - status :ok end + + body false + status :ok end desc 'Lock a terraform state of a certain name' diff --git a/spec/requests/api/terraform/state_spec.rb b/spec/requests/api/terraform/state_spec.rb index aff41ff5974..b91f6e1aa88 100644 --- a/spec/requests/api/terraform/state_spec.rb +++ b/spec/requests/api/terraform/state_spec.rb @@ -125,6 +125,7 @@ RSpec.describe API::Terraform::State do expect { request }.to change { Terraform::State.count }.by(0) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty end context 'on Unicorn', :unicorn do @@ -132,6 +133,7 @@ RSpec.describe API::Terraform::State do expect { request }.to change { Terraform::State.count }.by(0) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty end end end @@ -167,6 +169,7 @@ RSpec.describe API::Terraform::State do expect { request }.to change { Terraform::State.count }.by(1) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty end context 'on Unicorn', :unicorn do @@ -174,6 +177,7 @@ RSpec.describe API::Terraform::State do expect { request }.to change { Terraform::State.count }.by(1) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty end end end @@ -206,10 +210,11 @@ RSpec.describe API::Terraform::State do context 'with maintainer permissions' do let(:current_user) { maintainer } - it 'deletes the state' do + it 'deletes the state and returns empty body' do expect { request }.to change { Terraform::State.count }.by(-1) expect(response).to have_gitlab_http_status(:ok) + expect(Gitlab::Json.parse(response.body)).to be_empty end end diff --git a/spec/services/terraform/remote_state_handler_spec.rb b/spec/services/terraform/remote_state_handler_spec.rb index c47367feb14..ca392849d49 100644 --- a/spec/services/terraform/remote_state_handler_spec.rb +++ b/spec/services/terraform/remote_state_handler_spec.rb @@ -42,17 +42,17 @@ RSpec.describe Terraform::RemoteStateHandler do describe '#handle_with_lock' do it 'allows to modify a state using database locking' do - state = subject.handle_with_lock do |state| + record = nil + subject.handle_with_lock do |state| + record = state state.name = 'updated-name' end - expect(state.name).to eq 'updated-name' + expect(record.reload.name).to eq 'updated-name' end - it 'returns the state object itself' do - state = subject.handle_with_lock - - expect(state.name).to eq 'my-state' + it 'returns nil' do + expect(subject.handle_with_lock).to be_nil end end @@ -70,11 +70,13 @@ RSpec.describe Terraform::RemoteStateHandler do it 'handles a locked state using exclusive read lock' do handler.lock! - state = handler.handle_with_lock do |state| + record = nil + handler.handle_with_lock do |state| + record = state state.name = 'new-name' end - expect(state.name).to eq 'new-name' + expect(record.reload.name).to eq 'new-name' end it 'raises exception if lock has not been acquired before' do |