Do not expose Terraform state record in API

In particular, do not expose the object storage URL, which could be used
to write unwanted artifacts, bypassing backend validation.

See https://gitlab.com/gitlab-org/gitlab/-/issues/250266

app/services/terraform/remote_state_handler.rb

@@ -23,6 +23,8 @@ module Terraform
 
         state.save! unless state.destroyed?
       end
+
+      nil
     end
 
     def lock!

changelogs/unreleased/security-fix-terraform-state-exposed-object-storage-url.yml

+---
+title: Do not expose Terraform state record in API
+merge_request:
+author:
+type: security

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], build: current_authenticated_job)
-              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'

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
@@ -218,10 +222,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
 

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