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

8 files changed, 40 insertions, 13 deletions

diff --git a/app/assets/javascripts/jobs/components/job_app.vue b/app/assets/javascripts/jobs/components/job_app.vue
index e760706c97e..368b9fa45d3 100644
--- a/app/assets/javascripts/jobs/components/job_app.vue
+++ b/app/assets/javascripts/jobs/components/job_app.vue
@@ -1,7 +1,7 @@
 <script>
 import { throttle, isEmpty } from 'lodash';
 import { mapGetters, mapState, mapActions } from 'vuex';
-import { GlLoadingIcon } from '@gitlab/ui';
+import { GlLoadingIcon, GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui';
 import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
 import { isScrolledToBottom } from '~/lib/utils/scroll_utils';
 import { polyfillSticky } from '~/lib/utils/sticky';
@@ -36,6 +36,9 @@ export default {
     GlLoadingIcon,
     SharedRunner: () => import('ee_component/jobs/components/shared_runner_limit_block.vue'),
   },
+  directives: {
+    SafeHtml,
+  },
   mixins: [delayedJobMixin],
   props: {
     runnerSettingsUrl: {
@@ -218,7 +221,7 @@ export default {
           </div>
 
           <callout v-if="shouldRenderHeaderCallout">
-            <div v-html="job.callout_message"></div>
+            <div v-safe-html="job.callout_message"></div>
           </callout>
         </header>
         <!-- EO Header Section -->
diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb
index 523f1a0f8c6..625aa8a017d 100644
--- a/app/serializers/build_details_entity.rb
+++ b/app/serializers/build_details_entity.rb
@@ -136,7 +136,7 @@ class BuildDetailsEntity < JobEntity
     docs_url = "https://docs.gitlab.com/ce/ci/yaml/README.html#dependencies"
 
     [
-      failure_message.html_safe,
+      failure_message,
       help_message(docs_url).html_safe
     ].join("<br />")
   end
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/changelogs/unreleased/security-stored-xss-build-dependencies.yml b/changelogs/unreleased/security-stored-xss-build-dependencies.yml
new file mode 100644
index 00000000000..a5ce2bd0158
--- /dev/null
+++ b/changelogs/unreleased/security-stored-xss-build-dependencies.yml
@@ -0,0 +1,5 @@
+---
+title: Fix XSS vulnerability for job build dependencies
+merge_request:
+author:
+type: security
diff --git a/lib/api/terraform/state.rb b/lib/api/terraform/state.rb
index f6e966defce..c9852ddb03d 100644
--- a/lib/api/terraform/state.rb
+++ b/lib/api/terraform/state.rb
@@ -56,6 +56,9 @@ module API
               state.save!
               status :ok
             end
+
+            body false
+            status :ok
           end
 
           desc 'Delete a terraform state of a certain name'
@@ -65,8 +68,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 c47a12456c3..59dba571445 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