Merge branch 'rs-pick-security-into-master' into 'master'

GITLAB_PAGES_VERSION

-0.5.0
+0.5.1

app/assets/javascripts/environments/components/environment.vue

@@ -111,11 +111,11 @@ export default {
   },
 
   methods: {
-    toggleFolder(folder, folderUrl) {
+    toggleFolder(folder) {
       this.store.toggleFolder(folder);
 
       if (!folder.isOpen) {
-        this.fetchChildEnvironments(folder, folderUrl, true);
+        this.fetchChildEnvironments(folder, true);
       }
     },
 
@@ -143,10 +143,10 @@ export default {
         .catch(this.errorCallback);
     },
 
-    fetchChildEnvironments(folder, folderUrl, showLoader = false) {
+    fetchChildEnvironments(folder, showLoader = false) {
       this.store.updateEnvironmentProp(folder, 'isLoadingFolderContent', showLoader);
 
-      this.service.getFolderContent(folderUrl)
+      this.service.getFolderContent(folder.folder_path)
         .then(resp => resp.json())
         .then(response => this.store.setfolderContent(folder, response.environments))
         .then(() => this.store.updateEnvironmentProp(folder, 'isLoadingFolderContent', false))
@@ -173,12 +173,7 @@ export default {
       // We need to verify if any folder is open to also update it
       const openFolders = this.store.getOpenFolders();
       if (openFolders.length) {
-        openFolders.forEach((folder) => {
-          // TODO - Move this to the backend
-          const folderUrl = `${window.location.pathname}/folders/${folder.folderName}`;
-
-          return this.fetchChildEnvironments(folder, folderUrl);
-        });
+        openFolders.forEach(folder => this.fetchChildEnvironments(folder));
       }
     },
 

app/assets/javascripts/environments/components/environment_item.vue

@@ -410,20 +410,11 @@ export default {
              this.hasStopAction ||
              this.canRetry;
     },
-
-    /**
-     * Constructs folder URL based on the current location and the folder id.
-     *
-     * @return {String}
-     */
-    folderUrl() {
-      return `${window.location.pathname}/folders/${this.model.folderName}`;
-    },
   },
 
   methods: {
     onClickFolder() {
-      eventHub.$emit('toggleFolder', this.model, this.folderUrl);
+      eventHub.$emit('toggleFolder', this.model);
     },
   },
 };

app/assets/javascripts/notes.js

@@ -1272,16 +1272,16 @@ export default class Notes {
       `<li id="${uniqueId}" class="note being-posted fade-in-half timeline-entry">
          <div class="timeline-entry-inner">
             <div class="timeline-icon">
-               <a href="/${currentUsername}">
+               <a href="/${_.escape(currentUsername)}">
                  <img class="avatar s40" src="${currentUserAvatar}" />
                </a>
             </div>
             <div class="timeline-content ${discussionClass}">
                <div class="note-header">
                   <div class="note-header-info">
-                     <a href="/${currentUsername}">
-                       <span class="hidden-xs">${currentUserFullname}</span>
-                       <span class="note-headline-light">@${currentUsername}</span>
+                     <a href="/${_.escape(currentUsername)}">
+                       <span class="hidden-xs">${_.escape(currentUsername)}</span>
+                       <span class="note-headline-light">${_.escape(currentUsername)}</span>
                      </a>
                   </div>
                </div>
@@ -1295,6 +1295,9 @@ export default class Notes {
       </li>`
     );
 
+    $tempNote.find('.hidden-xs').text(_.escape(currentUserFullname));
+    $tempNote.find('.note-headline-light').text(`@${_.escape(currentUsername)}`);
+
     return $tempNote;
   }
 

app/assets/javascripts/users_select.js

@@ -75,7 +75,7 @@ function UsersSelect(currentUser, els) {
 
         if (currentUserInfo) {
           input.value = currentUserInfo.id;
-          input.dataset.meta = currentUserInfo.name;
+          input.dataset.meta = _.escape(currentUserInfo.name);
         } else if (_this.currentUser) {
           input.value = _this.currentUser.id;
         }
@@ -198,7 +198,7 @@ function UsersSelect(currentUser, els) {
             };
           }
           $value.html(assigneeTemplate(user));
-          $collapsedSidebar.attr('title', user.name).tooltip('fixTitle');
+          $collapsedSidebar.attr('title', _.escape(user.name)).tooltip('fixTitle');
           return $collapsedSidebar.html(collapsedAssigneeTemplate(user));
         });
       };
@@ -506,7 +506,7 @@ function UsersSelect(currentUser, els) {
 
           img = "";
           if (user.beforeDivider != null) {
-            `<li><a href='#' class='${selected === true ? 'is-active' : ''}'>${user.name}</a></li>`;
+            `<li><a href='#' class='${selected === true ? 'is-active' : ''}'>${_.escape(user.name)}</a></li>`;
           } else {
             if (avatar) {
               img = "<img src='" + avatar + "' class='avatar avatar-inline' width='32' />";
@@ -518,7 +518,7 @@ function UsersSelect(currentUser, els) {
               <a href='#' class='dropdown-menu-user-link ${selected === true ? 'is-active' : ''}'>
                 ${img}
                 <strong class='dropdown-menu-user-full-name'>
-                  ${user.name}
+                  ${_.escape(user.name)}
                 </strong>
                 ${username ? `<span class='dropdown-menu-user-username'>${username}</span>` : ''}
               </a>
@@ -643,11 +643,11 @@ UsersSelect.prototype.formatResult = function(user) {
   } else {
     avatar = gon.default_avatar_url;
   }
-  return "<div class='user-result " + (!user.username ? 'no-username' : void 0) + "'> <div class='user-image'><img class='avatar avatar-inline s32' src='" + avatar + "'></div> <div class='user-name dropdown-menu-user-full-name'>" + user.name + "</div> <div class='user-username dropdown-menu-user-username'>" + (!user.invite ? "@" + _.escape(user.username) : "") + "</div> </div>";
+  return "<div class='user-result " + (!user.username ? 'no-username' : void 0) + "'> <div class='user-image'><img class='avatar avatar-inline s32' src='" + avatar + "'></div> <div class='user-name dropdown-menu-user-full-name'>" + _.escape(user.name) + "</div> <div class='user-username dropdown-menu-user-username'>" + (!user.invite ? "@" + _.escape(user.username) : "") + "</div> </div>";
 };
 
 UsersSelect.prototype.formatSelection = function(user) {
-  return user.name;
+  return _.escape(user.name);
 };
 
 UsersSelect.prototype.user = function(user_id, callback) {

app/helpers/commits_helper.rb

@@ -137,7 +137,7 @@ module CommitsHelper
 
     text =
       if options[:avatar]
-        %Q{<span class="commit-#{options[:source]}-name">#{person_name}</span>}
+        content_tag(:span, person_name, class: "commit-#{options[:source]}-name")
       else
         person_name
       end
@@ -148,9 +148,9 @@ module CommitsHelper
     }
 
     if user.nil?
-      mail_to(source_email, text.html_safe, options)
+      mail_to(source_email, text, options)
     else
-      link_to(text.html_safe, user_path(user), options)
+      link_to(text, user_path(user), options)
     end
   end
 

app/models/environment.rb

@@ -82,12 +82,7 @@ class Environment < ActiveRecord::Base
   def set_environment_type
     names = name.split('/')
 
-    self.environment_type =
-      if names.many?
-        names.first
-      else
-        nil
-      end
+    self.environment_type = names.many? ? names.first : nil
   end
 
   def includes_commit?(commit)
@@ -101,7 +96,7 @@ class Environment < ActiveRecord::Base
   end
 
   def update_merge_request_metrics?
-    (environment_type || name) == "production"
+    folder_name == "production"
   end
 
   def first_deployment_for(commit)
@@ -223,6 +218,10 @@ class Environment < ActiveRecord::Base
       format: :json)
   end
 
+  def folder_name
+    self.environment_type || self.name
+  end
+
   private
 
   # Slugifying a name may remove the uniqueness guarantee afforded by it being

app/serializers/environment_entity.rb

@@ -26,5 +26,9 @@ class EnvironmentEntity < Grape::Entity
       terminal_project_environment_path(environment.project, environment)
   end
 
+  expose :folder_path do |environment|
+    folder_project_environments_path(environment.project, environment.folder_name)
+  end
+
   expose :created_at, :updated_at
 end

app/serializers/environment_serializer.rb

@@ -36,9 +36,9 @@ class EnvironmentSerializer < BaseSerializer
   private
 
   def itemize(resource)
-    items = resource.order('folder_name ASC')
+    items = resource.order('folder ASC')
       .group('COALESCE(environment_type, name)')
-      .select('COALESCE(environment_type, name) AS folder_name',
+      .select('COALESCE(environment_type, name) AS folder',
               'COUNT(*) AS size', 'MAX(id) AS last_id')
 
     # It makes a difference when you call `paginate` method, because
@@ -49,7 +49,7 @@ class EnvironmentSerializer < BaseSerializer
     environments = resource.where(id: items.map(&:last_id)).index_by(&:id)
 
     items.map do |item|
-      Item.new(item.folder_name, item.size, environments[item.last_id])
+      Item.new(item.folder, item.size, environments[item.last_id])
     end
   end
 end

app/views/projects/blob/viewers/_route_map.html.haml

@@ -6,4 +6,4 @@
   This Route Map is invalid:
   = viewer.validation_message
 
-= link_to 'Learn more', help_page_path('ci/environments', anchor: 'route-map')
+= link_to 'Learn more', help_page_path('ci/environments', anchor: 'go-directly-from-source-files-to-public-pages-on-the-environment')

app/views/projects/blob/viewers/_route_map_loading.html.haml

 = icon('spinner spin fw')
 Validating Route Map…
 
-= link_to 'Learn more', help_page_path('ci/environments', anchor: 'route-map')
+= link_to 'Learn more', help_page_path('ci/environments', anchor: 'go-directly-from-source-files-to-public-pages-on-the-environment')

doc/ci/environments.md

@@ -446,8 +446,7 @@ and/or `production`) you can see this information in the merge request itself.
 
 ![Environment URLs in merge request](img/environments_link_url_mr.png)
 
-### <a name="route-map"></a>Go directly from source files to public pages on the environment
-
+### Go directly from source files to public pages on the environment
 
 > Introduced in GitLab 8.17.
 

doc/install/database_mysql.md

@@ -75,7 +75,7 @@ log_bin_trust_function_creators=1
 
 ### MySQL utf8mb4 support
 
-After installation or upgrade, remember to [convert any new tables](#convert) to `utf8mb4`/`utf8mb4_general_ci`.
+After installation or upgrade, remember to [convert any new tables](#tables-and-data-conversion-to-utf8mb4) to `utf8mb4`/`utf8mb4_general_ci`.
 
 ---
 
@@ -230,7 +230,6 @@ We need to check, enable and probably convert your existing GitLab DB tables to
 > Now, ensure that [innodb_file_format](https://dev.mysql.com/doc/refman/5.6/en/tablespace-enabling.html) and [innodb_large_prefix](http://dev.mysql.com/doc/refman/5.7/en/innodb-parameters.html#sysvar_innodb_large_prefix) are **persisted** in your `my.cnf` file.
 
 #### Tables and data conversion to utf8mb4
-<a name="convert"></a>
 
 Now that you have a persistent MySQL setup, you can safely upgrade tables after setup or upgrade time:
 

lib/banzai/filter/sanitization_filter.rb

@@ -5,6 +5,7 @@ module Banzai
     # Extends HTML::Pipeline::SanitizationFilter with a custom whitelist.
     class SanitizationFilter < HTML::Pipeline::SanitizationFilter
       UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze
+      TABLE_ALIGNMENT_PATTERN = /text-align: (?<alignment>center|left|right)/
 
       def whitelist
         whitelist = super
@@ -24,7 +25,8 @@ module Banzai
         # Only push these customizations once
         return if customized?(whitelist[:transformers])
 
-        # Allow table alignment
+        # Allow table alignment; we whitelist specific style properties in a
+        # transformer below
         whitelist[:attributes]['th'] = %w(style)
         whitelist[:attributes]['td'] = %w(style)
 
@@ -43,6 +45,9 @@ module Banzai
         whitelist[:elements].push('abbr')
         whitelist[:attributes]['abbr'] = %w(title)
 
+        # Disallow `name` attribute globally
+        whitelist[:attributes][:all].delete('name')
+
         # Allow any protocol in `a` elements...
         whitelist[:protocols].delete('a')
 
@@ -52,6 +57,9 @@ module Banzai
         # Remove `rel` attribute from `a` elements
         whitelist[:transformers].push(self.class.remove_rel)
 
+        # Remove any `style` properties not required for table alignment
+        whitelist[:transformers].push(self.class.remove_unsafe_table_style)
+
         whitelist
       end
 
@@ -81,6 +89,21 @@ module Banzai
             end
           end
         end
+
+        def remove_unsafe_table_style
+          lambda do |env|
+            node = env[:node]
+
+            return unless node.name == 'th' || node.name == 'td'
+            return unless node.has_attribute?('style')
+
+            if node['style'] =~ TABLE_ALIGNMENT_PATTERN
+              node['style'] = "text-align: #{$~[:alignment]}"
+            else
+              node.remove_attribute('style')
+            end
+          end
+        end
       end
     end
   end

lib/gitlab/middleware/go.rb

@@ -3,6 +3,10 @@
 module Gitlab
   module Middleware
     class Go
+      include ActionView::Helpers::TagHelper
+
+      PROJECT_PATH_REGEX = %r{\A(#{Gitlab::PathRegex.full_namespace_route_regex}/#{Gitlab::PathRegex.project_route_regex})/}.freeze
+
       def initialize(app)
         @app = app
       end
@@ -10,17 +14,20 @@ module Gitlab
       def call(env)
         request = Rack::Request.new(env)
 
-        if go_request?(request)
-          render_go_doc(request)
-        else
-          @app.call(env)
-        end
+        render_go_doc(request) || @app.call(env)
       end
 
       private
 
       def render_go_doc(request)
-        body = go_body(request)
+        return unless go_request?(request)
+
+        path = project_path(request)
+        return unless path
+
+        body = go_body(path)
+        return unless body
+
         response = Rack::Response.new(body, 200, { 'Content-Type' => 'text/html' })
         response.finish
       end
@@ -29,11 +36,13 @@ module Gitlab
         request["go-get"].to_i == 1 && request.env["PATH_INFO"].present?
       end
 
-      def go_body(request)
-        project_url = URI.join(Gitlab.config.gitlab.url, project_path(request))
+      def go_body(path)
+        project_url = URI.join(Gitlab.config.gitlab.url, path)
         import_prefix = strip_url(project_url.to_s)
 
-        "<!DOCTYPE html><html><head><meta content='#{import_prefix} git #{project_url}.git' name='go-import'></head></html>\n"
+        meta_tag = tag :meta, name: 'go-import', content: "#{import_prefix} git #{project_url}.git"
+        head_tag = content_tag :head, meta_tag
+        content_tag :html, head_tag
       end
 
       def strip_url(url)
@@ -44,6 +53,10 @@ module Gitlab
         path_info = request.env["PATH_INFO"]
         path_info.sub!(/^\//, '')
 
+        project_path_match = "#{path_info}/".match(PROJECT_PATH_REGEX)
+        return unless project_path_match
+        path = project_path_match[1]
+
         # Go subpackages may be in the form of `namespace/project/path1/path2/../pathN`.
         # In a traditional project with a single namespace, this would denote repo
         # `namespace/project` with subpath `path1/path2/../pathN`, but with nested
@@ -51,7 +64,7 @@ module Gitlab
         # `path2/../pathN`, for example.
 
         # We find all potential project paths out of the path segments
-        path_segments = path_info.split('/')
+        path_segments = path.split('/')
         simple_project_path = path_segments.first(2).join('/')
 
         # If the path is at most 2 segments long, it is a simple `namespace/project` path and we're done

spec/features/copy_as_gfm_spec.rb

@@ -288,8 +288,6 @@ describe 'Copy as GFM', js: true do
         'SanitizationFilter',
 
         <<-GFM.strip_heredoc
-        <a name="named-anchor"></a>
-
         <sub>sub</sub>
 
         <dl>

spec/helpers/commits_helper_spec.rb

@@ -12,6 +12,17 @@ describe CommitsHelper do
       expect(helper.commit_author_link(commit))
         .not_to include('onmouseover="alert(1)"')
     end
+
+    it 'escapes the author name' do
+      user = build_stubbed(:user, name: 'Foo <script>alert("XSS")</script>')
+
+      commit = double(author: user, author_name: '', author_email: '')
+
+      expect(helper.commit_author_link(commit))
+        .to include('Foo &lt;script&gt;')
+      expect(helper.commit_author_link(commit, avatar: true))
+        .to include('commit-author-name', 'Foo &lt;script&gt;')
+    end
   end
 
   describe 'commit_committer_link' do
@@ -25,6 +36,17 @@ describe CommitsHelper do
       expect(helper.commit_committer_link(commit))
         .not_to include('onmouseover="alert(1)"')
     end
+
+    it 'escapes the commiter name' do
+      user = build_stubbed(:user, name: 'Foo <script>alert("XSS")</script>')
+
+      commit = double(committer: user, committer_name: '', committer_email: '')
+
+      expect(helper.commit_committer_link(commit))
+        .to include('Foo &lt;script&gt;')
+      expect(helper.commit_committer_link(commit, avatar: true))
+        .to include('commit-committer-name', 'Foo &lt;script&gt;')
+    end
   end
 
   describe '#view_on_environment_button' do

spec/javascripts/notes_spec.js

@@ -770,6 +770,20 @@ import '~/notes';
         expect($tempNote.prop('nodeName')).toEqual('LI');
         expect($tempNote.find('.timeline-content').hasClass('discussion')).toBeTruthy();
       });
+
+      it('should return a escaped user name', () => {
+        const currentUserFullnameXSS = 'Foo <script>alert("XSS")</script>';
+        const $tempNote = this.notes.createPlaceholderNote({
+          formContent: sampleComment,
+          uniqueId,
+          isDiscussionNote: false,
+          currentUsername,
+          currentUserFullname: currentUserFullnameXSS,
+          currentUserAvatar,
+        });
+        const $tempNoteHeader = $tempNote.find('.note-header');
+        expect($tempNoteHeader.find('.hidden-xs').text().trim()).toEqual('Foo &lt;script&gt;alert(&quot;XSS&quot;)&lt;/script&gt;');
+      });
     });
 
     describe('createPlaceholderSystemNote', () => {

spec/lib/banzai/filter/sanitization_filter_spec.rb

@@ -49,7 +49,7 @@ describe Banzai::Filter::SanitizationFilter do
       instance = described_class.new('Foo')
       3.times { instance.whitelist }
 
-      expect(instance.whitelist[:transformers].size).to eq 4
+      expect(instance.whitelist[:transformers].size).to eq 5
     end
 
     it 'sanitizes `class` attribute from all elements' do
@@ -63,8 +63,8 @@ describe Banzai::Filter::SanitizationFilter do
       expect(filter(act).to_html).to eq %q{<span>def</span>}
     end
 
-    it 'allows `style` attribute on table elements' do
-      html = <<-HTML.strip_heredoc
+    it 'allows `text-align` property in `style` attribute on table elements' do
+      html = <<~HTML
       <table>
         <tr><th style="text-align: center">Head</th></tr>
         <tr><td style="text-align: right">Body</th></tr>
@@ -77,6 +77,20 @@ describe Banzai::Filter::SanitizationFilter do
       expect(doc.at_css('td')['style']).to eq 'text-align: right'
     end
 
+    it 'disallows other properties in `style` attribute on table elements' do
+      html = <<~HTML
+        <table>
+          <tr><th style="text-align: foo">Head</th></tr>
+          <tr><td style="position: fixed; height: 50px; width: 50px; background: red; z-index: 999; font-size: 36px; text-align: center">Body</th></tr>
+        </table>
+      HTML
+
+      doc = filter(html)
+
+      expect(doc.at_css('th')['style']).to be_nil
+      expect(doc.at_css('td')['style']).to eq 'text-align: center'
+    end
+
     it 'allows `span` elements' do
       exp = act = %q{<span>Hello</span>}
       expect(filter(act).to_html).to eq exp
@@ -87,6 +101,18 @@ describe Banzai::Filter::SanitizationFilter do
       expect(filter(act).to_html).to eq exp
     end
 
+    it 'disallows the `name` attribute globally' do
+      html = <<~HTML
+        <img name="getElementById" src="">
+        <span name="foo" class="bar">Hi</span>
+      HTML
+
+      doc = filter(html)
+
+      expect(doc.at_css('img')).not_to have_attribute('name')
+      expect(doc.at_css('span')).not_to have_attribute('name')
+    end
+
     it 'allows `summary` elements' do
       exp = act = '<summary>summary line</summary>'
       expect(filter(act).to_html).to eq exp

spec/lib/gitlab/middleware/go_spec.rb

@@ -79,12 +79,28 @@ describe Gitlab::Middleware::Go do
           it_behaves_like 'a nested project'
         end
 
+        context 'with a subpackage that is not a valid project path' do
+          let(:path) { "#{project.full_path}/---subpackage" }
+
+          it_behaves_like 'a nested project'
+        end
+
         context 'without subpackages' do
           let(:path) { project.full_path }
 
           it_behaves_like 'a nested project'
         end
       end
+
+      context 'with a bogus path' do
+        let(:path) { "http:;url=http://www.example.com'http-equiv='refresh'x='?go-get=1" }
+
+        it 'skips go-import generation' do
+          expect(app).to receive(:call).and_return('no-go')
+
+          go
+        end
+      end
     end
 
     def go
@@ -100,7 +116,7 @@ describe Gitlab::Middleware::Go do
     def expect_response_with_path(response, path)
       expect(response[0]).to eq(200)
       expect(response[1]['Content-Type']).to eq('text/html')
-      expected_body = "<!DOCTYPE html><html><head><meta content='#{Gitlab.config.gitlab.host}/#{path} git http://#{Gitlab.config.gitlab.host}/#{path}.git' name='go-import'></head></html>\n"
+      expected_body = %{<html><head><meta name="go-import" content="#{Gitlab.config.gitlab.host}/#{path} git http://#{Gitlab.config.gitlab.host}/#{path}.git" /></head></html>}
       expect(response[2].body).to eq([expected_body])
     end
   end

spec/models/environment_spec.rb

@@ -54,6 +54,28 @@ describe Environment do
     end
   end
 
+  describe '#folder_name' do
+    context 'when it is inside a folder' do
+      subject(:environment) do
+        create(:environment, name: 'staging/review-1')
+      end
+
+      it 'returns a top-level folder name' do
+        expect(environment.folder_name).to eq 'staging'
+      end
+    end
+
+    context 'when the environment if a top-level item itself' do
+      subject(:environment) do
+        create(:environment, name: 'production')
+      end
+
+      it 'returns an environment name' do
+        expect(environment.folder_name).to eq 'production'
+      end
+    end
+  end
+
   describe '#nullify_external_url' do
     it 'replaces a blank url with nil' do
       env = build(:environment, external_url: "")

spec/serializers/environment_entity_spec.rb

@@ -16,6 +16,10 @@ describe EnvironmentEntity do
     expect(subject).to include(:id, :name, :state, :environment_path)
   end
 
+  it 'exposes folder path' do
+    expect(subject).to include(:folder_path)
+  end
+
   context 'metrics disabled' do
     before do
       allow(environment).to receive(:has_metrics?).and_return(false)