Add latest changes from gitlab-org/security/gitlab@12-7-stable-ee

app/assets/javascripts/frequent_items/utils.js

@@ -45,8 +45,19 @@ export const updateExistingFrequentItem = (frequentItem, item) => {
   };
 };
 
-export const sanitizeItem = item => ({
-  ...item,
-  name: sanitize(item.name.toString(), { allowedTags: [] }),
-  namespace: sanitize(item.namespace.toString(), { allowedTags: [] }),
-});
+export const sanitizeItem = item => {
+  // Only sanitize if the key exists on the item
+  const maybeSanitize = key => {
+    if (!Object.prototype.hasOwnProperty.call(item, key)) {
+      return {};
+    }
+
+    return { [key]: sanitize(item[key].toString(), { allowedTags: [] }) };
+  };
+
+  return {
+    ...item,
+    ...maybeSanitize('name'),
+    ...maybeSanitize('namespace'),
+  };
+};

app/models/group.rb

@@ -67,6 +67,9 @@ class Group < Namespace
   validates :variables, variable_duplicates: true
 
   validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 }
+  validates :name,
+    format: { with: Gitlab::Regex.group_name_regex,
+              message: Gitlab::Regex.group_name_regex_message }
 
   add_authentication_token_field :runners_token, encrypted: -> { Feature.enabled?(:groups_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
 

changelogs/unreleased/security-59-prevent-create-api-snippet.yml

+---
+title: External user can not create personal snippet through API
+merge_request:
+author:
+type: security

changelogs/unreleased/security-backend-xss-admin-email.yml

+---
+title: Prevent malicious entry for group name
+merge_request:
+author:
+type: security

changelogs/unreleased/security-mask-gh-service-password.yml

+---
+title: Change GitHub service integration token input to password
+merge_request:
+author:
+type: security

changelogs/unreleased/security-xss-vulnerability-in-admin-send-email-notification.yml

+---
+title: Fix XSS vulnerability in `admin/email` "Recipient Group" dropdown
+merge_request:
+author:
+type: security

lib/api/snippets.rb

@@ -74,6 +74,8 @@ module API
                               desc: 'The visibility of the snippet'
       end
       post do
+        authorize! :create_personal_snippet
+
         attrs = declared_params(include_missing: false).merge(request: request, api: true)
         service_response = ::Snippets::CreateService.new(nil, current_user, attrs).execute
         snippet = service_response.payload[:snippet]

lib/gitlab/regex.rb

@@ -13,6 +13,14 @@ module Gitlab
       "It must start with letter, digit, emoji or '_'."
     end
 
+    def group_name_regex
+      project_name_regex
+    end
+
+    def group_name_regex_message
+      project_name_regex_message
+    end
+
     ##
     # Docker Distribution Registry repository / tag name rules
     #

spec/controllers/groups_controller_spec.rb

@@ -258,6 +258,18 @@ describe GroupsController do
         end
       end
     end
+
+    context "malicious group name" do
+      subject { post :create, params: { group: { name: "<script>alert('Mayday!');</script>", path: "invalid_group_url" } } }
+
+      before do
+        sign_in(user)
+      end
+
+      it { expect { subject }.not_to change { Group.count } }
+
+      it { expect(subject).to render_template(:new) }
+    end
   end
 
   describe 'GET #index' do
@@ -829,6 +841,16 @@ describe GroupsController do
             put :update, params: { id: group.to_param, group: { name: 'world' } }
           end.to change { group.reload.name }
         end
+
+        context "malicious group name" do
+          subject { put :update, params: { id: group.to_param, group: { name: "<script>alert('Attack!');</script>" } } }
+
+          it { is_expected.to render_template(:edit) }
+
+          it 'does not update name' do
+            expect { subject }.not_to change { group.reload.name }
+          end
+        end
       end
 
       describe 'DELETE #destroy' do

spec/javascripts/frequent_items/utils_spec.js

@@ -108,5 +108,23 @@ describe('Frequent Items utils spec', () => {
 
       expect(sanitizeItem(input)).toEqual({ name: 'test', namespace: 'test', id: 1 });
     });
+
+    it("skips `name` key if it doesn't exist on the item", () => {
+      const input = {
+        namespace: '<br>test',
+        id: 1,
+      };
+
+      expect(sanitizeItem(input)).toEqual({ namespace: 'test', id: 1 });
+    });
+
+    it("skips `namespace` key if it doesn't exist on the item", () => {
+      const input = {
+        name: '<br><b>test</b>',
+        id: 1,
+      };
+
+      expect(sanitizeItem(input)).toEqual({ name: 'test', id: 1 });
+    });
   });
 });

spec/lib/banzai/filter/label_reference_filter_spec.rb

@@ -523,7 +523,12 @@ describe Banzai::Filter::LabelReferenceFilter do
     end
 
     context 'when group name has HTML entities' do
-      let(:another_group) { create(:group, name: '<img src=x onerror=alert(1)>', path: 'another_group') }
+      let(:another_group) { create(:group, name: 'random', path: 'another_group') }
+
+      before do
+        another_group.name = "<img src=x onerror=alert(1)>"
+        another_group.save!(validate: false)
+      end
 
       it 'escapes the HTML entities' do
         expect(result.text)

spec/lib/gitlab/regex_spec.rb

@@ -3,9 +3,7 @@
 require 'spec_helper'
 
 describe Gitlab::Regex do
-  describe '.project_name_regex' do
-    subject { described_class.project_name_regex }
-
+  shared_examples_for 'project/group name regex' do
     it { is_expected.to match('gitlab-ce') }
     it { is_expected.to match('GitLab CE') }
     it { is_expected.to match('100 lines') }
@@ -15,6 +13,34 @@ describe Gitlab::Regex do
     it { is_expected.not_to match('?gitlab') }
   end
 
+  shared_examples_for 'project/group name error message' do
+    it { is_expected.to eq("can contain only letters, digits, emojis, '_', '.', dash, space. It must start with letter, digit, emoji or '_'.") }
+  end
+
+  describe '.project_name_regex' do
+    subject { described_class.project_name_regex }
+
+    it_behaves_like 'project/group name regex'
+  end
+
+  describe '.group_name_regex' do
+    subject { described_class.group_name_regex }
+
+    it_behaves_like 'project/group name regex'
+  end
+
+  describe '.project_name_regex_message' do
+    subject { described_class.project_name_regex_message }
+
+    it_behaves_like 'project/group name error message'
+  end
+
+  describe '.group_name_regex_message' do
+    subject { described_class.group_name_regex_message }
+
+    it_behaves_like 'project/group name error message'
+  end
+
   describe '.environment_name_regex' do
     subject { described_class.environment_name_regex }
 

spec/models/group_spec.rb

@@ -48,6 +48,9 @@ describe Group do
 
   describe 'validations' do
     it { is_expected.to validate_presence_of :name }
+    it { is_expected.to allow_value('group test_4').for(:name) }
+    it { is_expected.not_to allow_value('test/../foo').for(:name) }
+    it { is_expected.not_to allow_value('<script>alert("Attack!")</script>').for(:name) }
     it { is_expected.to validate_presence_of :path }
     it { is_expected.not_to validate_presence_of :owner }
     it { is_expected.to validate_presence_of :two_factor_grace_period }

spec/requests/api/groups_spec.rb

@@ -568,6 +568,20 @@ describe API::Groups do
         expect(json_response['shared_projects'].length).to eq(0)
       end
 
+      context 'malicious group name' do
+        subject { put api("/groups/#{group1.id}", user1), params: { name: "<SCRIPT>alert('DOUBLE-ATTACK!')</SCRIPT>" } }
+
+        it 'returns bad request' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        it 'does not update group name' do
+          expect { subject }.not_to change { group1.reload.name }
+        end
+      end
+
       it 'returns 404 for a non existing group' do
         put api('/groups/1328', user1), params: { name: new_group_name }
 
@@ -999,6 +1013,20 @@ describe API::Groups do
         expect(json_response["parent_id"]).to eq(parent.id)
       end
 
+      context 'malicious group name' do
+        subject { post api("/groups", user3), params: group_params }
+
+        let(:group_params) { attributes_for_group_api name: "<SCRIPT>alert('ATTACKED!')</SCRIPT>", path: "unique-url" }
+
+        it 'returns bad request' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        it { expect { subject }.not_to change { Group.count } }
+      end
+
       it "does not create group, duplicate" do
         post api("/groups", user3), params: { name: 'Duplicate Test', path: group2.path }
 

spec/requests/api/project_snippets_spec.rb

@@ -98,6 +98,30 @@ describe API::ProjectSnippets do
       }
     end
 
+    context 'with an external user' do
+      let(:user) { create(:user, :external) }
+
+      context 'that belongs to the project' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'creates a new snippet' do
+          post api("/projects/#{project.id}/snippets/", user), params: params
+
+          expect(response).to have_gitlab_http_status(201)
+        end
+      end
+
+      context 'that does not belong to the project' do
+        it 'does not create a new snippet' do
+          post api("/projects/#{project.id}/snippets/", user), params: params
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+    end
+
     context 'with a regular user' do
       let(:user) { create(:user) }
 

spec/requests/api/snippets_spec.rb

@@ -224,6 +224,16 @@ describe API::Snippets do
 
     it_behaves_like 'snippet creation'
 
+    context 'with an external user' do
+      let(:user) { create(:user, :external) }
+
+      it 'does not create a new snippet' do
+        post api("/snippets/", user), params: params
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
     it 'returns 400 for missing parameters' do
       params.delete(:title)