Add latest changes from gitlab-org/security/gitlab@14-0-stable-ee

4 files changed, 99 insertions, 77 deletions

diff --git a/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js b/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js
index 9a8af79210e..19ebab36481 100644
--- a/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js
+++ b/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js
@@ -1,4 +1,5 @@
 import $ from 'jquery';
+import { sanitize } from '~/lib/dompurify';
 import { getSelectedFragment, insertText } from '~/lib/utils/common_utils';
 
 export class CopyAsGFM {
@@ -69,7 +70,7 @@ export class CopyAsGFM {
     } else {
       // Due to the async copy call we are not able to produce gfm so we transform the cached HTML
       const div = document.createElement('div');
-      div.innerHTML = gfmHtml;
+      div.innerHTML = sanitize(gfmHtml);
       CopyAsGFM.nodeToGFM(div)
         .then((transformedGfm) => {
           CopyAsGFM.insertPastedText(e.target, text, transformedGfm);
diff --git a/app/controllers/ide_controller.rb b/app/controllers/ide_controller.rb
index 4c7a91ee602..44beceb4f48 100644
--- a/app/controllers/ide_controller.rb
+++ b/app/controllers/ide_controller.rb
@@ -7,6 +7,8 @@ class IdeController < ApplicationController
   include StaticObjectExternalStorageCSP
   include Gitlab::Utils::StrongMemoize
 
+  before_action :authorize_read_project!
+
   before_action do
     push_frontend_feature_flag(:build_service_proxy)
     push_frontend_feature_flag(:schema_linting)
@@ -22,6 +24,10 @@ class IdeController < ApplicationController
 
   private
 
+  def authorize_read_project!
+    render_404 unless can?(current_user, :read_project, project)
+  end
+
   def define_index_vars
     return unless project
 
diff --git a/spec/frontend/behaviors/copy_as_gfm_spec.js b/spec/frontend/behaviors/copy_as_gfm_spec.js
index acff990e84a..557b609f5f9 100644
--- a/spec/frontend/behaviors/copy_as_gfm_spec.js
+++ b/spec/frontend/behaviors/copy_as_gfm_spec.js
@@ -1,50 +1,54 @@
 import initCopyAsGFM, { CopyAsGFM } from '~/behaviors/markdown/copy_as_gfm';
-import * as commonUtils from '~/lib/utils/common_utils';
 
 describe('CopyAsGFM', () => {
   describe('CopyAsGFM.pasteGFM', () => {
-    function callPasteGFM() {
+    let target;
+
+    beforeEach(() => {
+      target = document.createElement('input');
+      target.value = 'This is code: ';
+    });
+
+    // When GFM code is copied, we put the regular plain text
+    // on the clipboard as `text/plain`, and the GFM as `text/x-gfm`.
+    // This emulates the behavior of `getData` with that data.
+    function callPasteGFM(data = { 'text/plain': 'code', 'text/x-gfm': '`code`' }) {
       const e = {
         originalEvent: {
           clipboardData: {
             getData(mimeType) {
-              // When GFM code is copied, we put the regular plain text
-              // on the clipboard as `text/plain`, and the GFM as `text/x-gfm`.
-              // This emulates the behavior of `getData` with that data.
-              if (mimeType === 'text/plain') {
-                return 'code';
-              }
-              if (mimeType === 'text/x-gfm') {
-                return '`code`';
-              }
-              return null;
+              return data[mimeType] || null;
             },
           },
         },
         preventDefault() {},
+        target,
       };
 
       CopyAsGFM.pasteGFM(e);
     }
 
     it('wraps pasted code when not already in code tags', () => {
-      jest.spyOn(commonUtils, 'insertText').mockImplementation((el, textFunc) => {
-        const insertedText = textFunc('This is code: ', '');
+      callPasteGFM();
 
-        expect(insertedText).toEqual('`code`');
-      });
+      expect(target.value).toBe('This is code: `code`');
+    });
+
+    it('does not wrap pasted code when already in code tags', () => {
+      target.value = 'This is code: `';
 
       callPasteGFM();
+
+      expect(target.value).toBe('This is code: `code');
     });
 
-    it('does not wrap pasted code when already in code tags', () => {
-      jest.spyOn(commonUtils, 'insertText').mockImplementation((el, textFunc) => {
-        const insertedText = textFunc('This is code: `', '`');
+    it('does not allow xss in x-gfm-html', () => {
+      const testEl = document.createElement('div');
+      jest.spyOn(document, 'createElement').mockReturnValueOnce(testEl);
 
-        expect(insertedText).toEqual('code');
-      });
+      callPasteGFM({ 'text/plain': 'code', 'text/x-gfm-html': 'code<img/src/onerror=alert(1)>' });
 
-      callPasteGFM();
+      expect(testEl.innerHTML).toBe('code<img src="">');
     });
   });
 
diff --git a/spec/requests/ide_controller_spec.rb b/spec/requests/ide_controller_spec.rb
index 9b0d8dcd828..4bf1e43ba40 100644
--- a/spec/requests/ide_controller_spec.rb
+++ b/spec/requests/ide_controller_spec.rb
@@ -3,7 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe IdeController do
-  let_it_be(:project) { create(:project, :public) }
+  let_it_be(:reporter) { create(:user) }
+
+  let_it_be(:project) do
+    create(:project, :private).tap do |p|
+      p.add_reporter(reporter)
+    end
+  end
+
   let_it_be(:creator) { project.creator }
   let_it_be(:other_user) { create(:user) }
 
@@ -14,48 +21,62 @@ RSpec.describe IdeController do
     sign_in(user)
   end
 
-  it 'increases the views counter' do
-    expect(Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_views_count)
-
-    get ide_url
-  end
-
   describe '#index', :aggregate_failures do
     subject { get route }
 
-    shared_examples 'user cannot push code' do
-      include ProjectForksHelper
-
-      let(:user) { other_user }
+    shared_examples 'user access rights check' do
+      context 'user can read project' do
+        it 'increases the views counter' do
+          expect(Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_views_count)
 
-      context 'when user does not have fork' do
-        it 'instantiates fork_info instance var with fork_path and return 200' do
           subject
-
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(assigns(:project)).to eq project
-          expect(assigns(:fork_info)).to eq({ fork_path: controller.helpers.ide_fork_and_edit_path(project, branch, '', with_notice: false) })
         end
 
-        it 'has nil fork_info if user cannot fork' do
-          project.project_feature.update!(forking_access_level: ProjectFeature::DISABLED)
+        context 'user can read project but cannot push code' do
+          include ProjectForksHelper
 
-          subject
+          let(:user) { reporter }
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(assigns(:fork_info)).to be_nil
+          context 'when user does not have fork' do
+            it 'instantiates fork_info instance var with fork_path and returns 200' do
+              subject
+
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(assigns(:project)).to eq project
+              expect(assigns(:fork_info)).to eq({ fork_path: controller.helpers.ide_fork_and_edit_path(project, branch, '', with_notice: false) })
+            end
+
+            it 'has nil fork_info if user cannot fork' do
+              project.project_feature.update!(forking_access_level: ProjectFeature::DISABLED)
+
+              subject
+
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(assigns(:fork_info)).to be_nil
+            end
+          end
+
+          context 'when user has fork' do
+            let!(:fork) { fork_project(project, user, repository: true, namespace: user.namespace) }
+
+            it 'instantiates fork_info instance var with ide_path and returns 200' do
+              subject
+
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(assigns(:project)).to eq project
+              expect(assigns(:fork_info)).to eq({ ide_path: controller.helpers.ide_edit_path(fork, branch, '') })
+            end
+          end
         end
       end
 
-      context 'when user has fork' do
-        let!(:fork) { fork_project(project, user, repository: true, namespace: user.namespace) }
+      context 'user cannot read project' do
+        let(:user) { other_user }
 
-        it 'instantiates fork_info instance var with ide_path and return 200' do
+        it 'returns 404' do
           subject
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(assigns(:project)).to eq project
-          expect(assigns(:fork_info)).to eq({ ide_path: controller.helpers.ide_edit_path(fork, branch, '') })
+          expect(response).to have_gitlab_http_status(:not_found)
         end
       end
     end
@@ -63,37 +84,27 @@ RSpec.describe IdeController do
     context '/-/ide' do
       let(:route) { '/-/ide' }
 
-      it 'does not instantiate any instance var and return 200' do
+      it 'returns 404' do
         subject
 
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(assigns(:project)).to be_nil
-        expect(assigns(:branch)).to be_nil
-        expect(assigns(:path)).to be_nil
-        expect(assigns(:merge_request)).to be_nil
-        expect(assigns(:fork_info)).to be_nil
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
     context '/-/ide/project' do
       let(:route) { '/-/ide/project' }
 
-      it 'does not instantiate any instance var and return 200' do
+      it 'returns 404' do
         subject
 
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(assigns(:project)).to be_nil
-        expect(assigns(:branch)).to be_nil
-        expect(assigns(:path)).to be_nil
-        expect(assigns(:merge_request)).to be_nil
-        expect(assigns(:fork_info)).to be_nil
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
     context '/-/ide/project/:project' do
       let(:route) { "/-/ide/project/#{project.full_path}" }
 
-      it 'instantiates project instance var and return 200' do
+      it 'instantiates project instance var and returns 200' do
         subject
 
         expect(response).to have_gitlab_http_status(:ok)
@@ -104,13 +115,13 @@ RSpec.describe IdeController do
         expect(assigns(:fork_info)).to be_nil
       end
 
-      it_behaves_like 'user cannot push code'
+      it_behaves_like 'user access rights check'
 
       %w(edit blob tree).each do |action|
         context "/-/ide/project/:project/#{action}" do
           let(:route) { "/-/ide/project/#{project.full_path}/#{action}" }
 
-          it 'instantiates project instance var and return 200' do
+          it 'instantiates project instance var and returns 200' do
             subject
 
             expect(response).to have_gitlab_http_status(:ok)
@@ -121,13 +132,13 @@ RSpec.describe IdeController do
             expect(assigns(:fork_info)).to be_nil
           end
 
-          it_behaves_like 'user cannot push code'
+          it_behaves_like 'user access rights check'
 
           context "/-/ide/project/:project/#{action}/:branch" do
             let(:branch) { 'master' }
             let(:route) { "/-/ide/project/#{project.full_path}/#{action}/#{branch}" }
 
-            it 'instantiates project and branch instance vars and return 200' do
+            it 'instantiates project and branch instance vars and returns 200' do
               subject
 
               expect(response).to have_gitlab_http_status(:ok)
@@ -138,13 +149,13 @@ RSpec.describe IdeController do
               expect(assigns(:fork_info)).to be_nil
             end
 
-            it_behaves_like 'user cannot push code'
+            it_behaves_like 'user access rights check'
 
             context "/-/ide/project/:project/#{action}/:branch/-" do
               let(:branch) { 'branch/slash' }
               let(:route) { "/-/ide/project/#{project.full_path}/#{action}/#{branch}/-" }
 
-              it 'instantiates project and branch instance vars and return 200' do
+              it 'instantiates project and branch instance vars and returns 200' do
                 subject
 
                 expect(response).to have_gitlab_http_status(:ok)
@@ -155,13 +166,13 @@ RSpec.describe IdeController do
                 expect(assigns(:fork_info)).to be_nil
               end
 
-              it_behaves_like 'user cannot push code'
+              it_behaves_like 'user access rights check'
 
               context "/-/ide/project/:project/#{action}/:branch/-/:path" do
                 let(:branch) { 'master' }
                 let(:route) { "/-/ide/project/#{project.full_path}/#{action}/#{branch}/-/foo/.bar" }
 
-                it 'instantiates project, branch, and path instance vars and return 200' do
+                it 'instantiates project, branch, and path instance vars and returns 200' do
                   subject
 
                   expect(response).to have_gitlab_http_status(:ok)
@@ -172,7 +183,7 @@ RSpec.describe IdeController do
                   expect(assigns(:fork_info)).to be_nil
                 end
 
-                it_behaves_like 'user cannot push code'
+                it_behaves_like 'user access rights check'
               end
             end
           end
@@ -184,7 +195,7 @@ RSpec.describe IdeController do
 
         let(:route) { "/-/ide/project/#{project.full_path}/merge_requests/#{merge_request.id}" }
 
-        it 'instantiates project and merge_request instance vars and return 200' do
+        it 'instantiates project and merge_request instance vars and returns 200' do
           subject
 
           expect(response).to have_gitlab_http_status(:ok)
@@ -195,7 +206,7 @@ RSpec.describe IdeController do
           expect(assigns(:fork_info)).to be_nil
         end
 
-        it_behaves_like 'user cannot push code'
+        it_behaves_like 'user access rights check'
       end
     end
   end