Add latest changes from gitlab-org/security/gitlab@13-5-stable-ee

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