From 1d1363e2bb8a0aee7e2849fd463ea415035710d9 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 7 Jun 2017 20:32:38 -0700 Subject: Bring in security changes from the 9.2.5 release Ran: - git format-patch v9.2.2..v9.2.5 --stdout > patchfile.patch - git checkout -b 9-2-5-security-patch origin/v9.2.2 - git apply patchfile.patch - git commit - [Got the sha ref for the commit] - git checkout -b upstream-9-2-security master - git cherry-pick - [Resolved conflicts] - git cherry-pick --continue --- app/assets/javascripts/notes.js | 4 ++-- app/controllers/autocomplete_controller.rb | 2 +- app/policies/project_snippet_policy.rb | 5 +++++ app/uploaders/file_uploader.rb | 7 +++++++ app/uploaders/gitlab_uploader.rb | 18 ++++++++++++++---- 5 files changed, 29 insertions(+), 7 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 929965de5c1..b0143b12cfe 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -1478,7 +1478,7 @@ const normalizeNewlines = function(str) { const cachedNoteBodyText = $noteBodyText.html(); // Show updated comment content temporarily - $noteBodyText.html(formContent); + $noteBodyText.html(_.escape(formContent)); $editingNote.removeClass('is-editing fade-in-full').addClass('being-posted fade-in-half'); $editingNote.find('.note-headline-meta a').html(''); @@ -1491,7 +1491,7 @@ const normalizeNewlines = function(str) { }) .fail(() => { // Submission failed, revert back to original note - $noteBodyText.html(cachedNoteBodyText); + $noteBodyText.html(_.escape(cachedNoteBodyText)); $editingNote.removeClass('being-posted fade-in'); $editingNote.find('.fa.fa-spinner').remove(); diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 907717dcb96..fe331a883c1 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -21,7 +21,7 @@ class AutocompleteController < ApplicationController @users = [current_user, *@users].uniq end - if params[:author_id].present? + if params[:author_id].present? && current_user author = User.find_by_id(params[:author_id]) @users = [author, *@users].uniq if author end diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb index cf8ff92617f..bc5c4f32f79 100644 --- a/app/policies/project_snippet_policy.rb +++ b/app/policies/project_snippet_policy.rb @@ -1,5 +1,10 @@ class ProjectSnippetPolicy < BasePolicy def rules + # We have to check both project feature visibility and a snippet visibility and take the stricter one + # This will be simplified - check https://gitlab.com/gitlab-org/gitlab-ce/issues/27573 + return unless @subject.project.feature_available?(:snippets, @user) + return unless Ability.allowed?(@user, :read_project, @subject.project) + can! :read_project_snippet if @subject.public? return unless @user diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 7e94218c23d..652277e3b78 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -13,6 +13,13 @@ class FileUploader < GitlabUploader ) end + # Not using `GitlabUploader.base_dir` because all project namespaces are in + # the `public/uploads` dir. + # + def self.base_dir + root_dir + end + # Returns the part of `store_dir` that can change based on the model's current # path # diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 02afddb8c6a..489613030e6 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -3,16 +3,26 @@ class GitlabUploader < CarrierWave::Uploader::Base File.join(CarrierWave.root, upload_record.path) end - def self.base_dir + def self.root_dir 'uploads' end - delegate :base_dir, to: :class + # When object storage is used, keep the `root_dir` as `base_dir`. + # The files aren't really in folders there, they just have a name. + # The files that contain user input in their name, also contain a hash, so + # the names are still unique + # + # This method is overridden in the `FileUploader` + def self.base_dir + return root_dir unless file_storage? + end - def file_storage? - storage.is_a?(CarrierWave::Storage::File) + def self.file_storage? + self.storage.is_a?(CarrierWave::Storage::File) end + delegate :base_dir, :file_storage?, to: :class + def file_cache_storage? cache_storage.is_a?(CarrierWave::Storage::File) end -- cgit v1.2.1 From 565ead610215d32fc6fe57a78f595fad51588e49 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 7 Jun 2017 20:32:38 -0700 Subject: Bring in security changes from the 9.2.5 release Ran: - git format-patch v9.2.2..v9.2.5 --stdout > patchfile.patch - git checkout -b 9-2-5-security-patch origin/v9.2.2 - git apply patchfile.patch - git commit - [Got the sha ref for the commit] - git checkout -b upstream-9-2-security master - git cherry-pick - [Resolved conflicts] - git cherry-pick --continue --- app/assets/javascripts/notes.js | 4 ++-- app/controllers/autocomplete_controller.rb | 2 +- app/policies/project_snippet_policy.rb | 5 +++++ app/uploaders/file_uploader.rb | 7 +++++++ app/uploaders/gitlab_uploader.rb | 18 ++++++++++++++---- 5 files changed, 29 insertions(+), 7 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 929965de5c1..b0143b12cfe 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -1478,7 +1478,7 @@ const normalizeNewlines = function(str) { const cachedNoteBodyText = $noteBodyText.html(); // Show updated comment content temporarily - $noteBodyText.html(formContent); + $noteBodyText.html(_.escape(formContent)); $editingNote.removeClass('is-editing fade-in-full').addClass('being-posted fade-in-half'); $editingNote.find('.note-headline-meta a').html(''); @@ -1491,7 +1491,7 @@ const normalizeNewlines = function(str) { }) .fail(() => { // Submission failed, revert back to original note - $noteBodyText.html(cachedNoteBodyText); + $noteBodyText.html(_.escape(cachedNoteBodyText)); $editingNote.removeClass('being-posted fade-in'); $editingNote.find('.fa.fa-spinner').remove(); diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 907717dcb96..fe331a883c1 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -21,7 +21,7 @@ class AutocompleteController < ApplicationController @users = [current_user, *@users].uniq end - if params[:author_id].present? + if params[:author_id].present? && current_user author = User.find_by_id(params[:author_id]) @users = [author, *@users].uniq if author end diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb index cf8ff92617f..bc5c4f32f79 100644 --- a/app/policies/project_snippet_policy.rb +++ b/app/policies/project_snippet_policy.rb @@ -1,5 +1,10 @@ class ProjectSnippetPolicy < BasePolicy def rules + # We have to check both project feature visibility and a snippet visibility and take the stricter one + # This will be simplified - check https://gitlab.com/gitlab-org/gitlab-ce/issues/27573 + return unless @subject.project.feature_available?(:snippets, @user) + return unless Ability.allowed?(@user, :read_project, @subject.project) + can! :read_project_snippet if @subject.public? return unless @user diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 7e94218c23d..652277e3b78 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -13,6 +13,13 @@ class FileUploader < GitlabUploader ) end + # Not using `GitlabUploader.base_dir` because all project namespaces are in + # the `public/uploads` dir. + # + def self.base_dir + root_dir + end + # Returns the part of `store_dir` that can change based on the model's current # path # diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 02afddb8c6a..489613030e6 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -3,16 +3,26 @@ class GitlabUploader < CarrierWave::Uploader::Base File.join(CarrierWave.root, upload_record.path) end - def self.base_dir + def self.root_dir 'uploads' end - delegate :base_dir, to: :class + # When object storage is used, keep the `root_dir` as `base_dir`. + # The files aren't really in folders there, they just have a name. + # The files that contain user input in their name, also contain a hash, so + # the names are still unique + # + # This method is overridden in the `FileUploader` + def self.base_dir + return root_dir unless file_storage? + end - def file_storage? - storage.is_a?(CarrierWave::Storage::File) + def self.file_storage? + self.storage.is_a?(CarrierWave::Storage::File) end + delegate :base_dir, :file_storage?, to: :class + def file_cache_storage? cache_storage.is_a?(CarrierWave::Storage::File) end -- cgit v1.2.1 From 2e97db887cf5acf28143a04badc3cbf77b54ea44 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 8 Jun 2017 07:23:52 +0200 Subject: Make the uploader use the updated folder --- app/uploaders/gitlab_uploader.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'app') diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 489613030e6..e4e6d6f46b1 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -15,10 +15,12 @@ class GitlabUploader < CarrierWave::Uploader::Base # This method is overridden in the `FileUploader` def self.base_dir return root_dir unless file_storage? + + File.join(root_dir, 'system') end def self.file_storage? - self.storage.is_a?(CarrierWave::Storage::File) + self.storage == CarrierWave::Storage::File end delegate :base_dir, :file_storage?, to: :class -- cgit v1.2.1 From 7113b1a45bd29318c3ec5ea5f61b1d523868ef4d Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Thu, 8 Jun 2017 09:48:10 -0700 Subject: Merge branch 'cherry-pick-dc2ac993' into 'security-9-2' Escapes html content before appending it to the DOM See merge request !2107 --- app/assets/javascripts/notes.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 929965de5c1..b0143b12cfe 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -1478,7 +1478,7 @@ const normalizeNewlines = function(str) { const cachedNoteBodyText = $noteBodyText.html(); // Show updated comment content temporarily - $noteBodyText.html(formContent); + $noteBodyText.html(_.escape(formContent)); $editingNote.removeClass('is-editing fade-in-full').addClass('being-posted fade-in-half'); $editingNote.find('.note-headline-meta a').html(''); @@ -1491,7 +1491,7 @@ const normalizeNewlines = function(str) { }) .fail(() => { // Submission failed, revert back to original note - $noteBodyText.html(cachedNoteBodyText); + $noteBodyText.html(_.escape(cachedNoteBodyText)); $editingNote.removeClass('being-posted fade-in'); $editingNote.find('.fa.fa-spinner').remove(); -- cgit v1.2.1 From 982368dc55bbd22f82bf908f8af220056202a65a Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Thu, 8 Jun 2017 09:52:27 -0700 Subject: Merge branch 'dz-restrict-autocomplete' into 'security-9-1' Allow users autocomplete by author_id only for authenticated users See merge request !2100 --- app/controllers/autocomplete_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 907717dcb96..fe331a883c1 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -21,7 +21,7 @@ class AutocompleteController < ApplicationController @users = [current_user, *@users].uniq end - if params[:author_id].present? + if params[:author_id].present? && current_user author = User.find_by_id(params[:author_id]) @users = [author, *@users].uniq if author end -- cgit v1.2.1 From ae6adf165ce7d9a85d7b8886eefdbe96aac2816b Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Thu, 8 Jun 2017 09:56:39 -0700 Subject: Merge branch '25934-project-snippet-vis' into 'security-9-2' Fix visibility when referencing snippets See merge request !2101 --- app/policies/project_snippet_policy.rb | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'app') diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb index cf8ff92617f..bc5c4f32f79 100644 --- a/app/policies/project_snippet_policy.rb +++ b/app/policies/project_snippet_policy.rb @@ -1,5 +1,10 @@ class ProjectSnippetPolicy < BasePolicy def rules + # We have to check both project feature visibility and a snippet visibility and take the stricter one + # This will be simplified - check https://gitlab.com/gitlab-org/gitlab-ce/issues/27573 + return unless @subject.project.feature_available?(:snippets, @user) + return unless Ability.allowed?(@user, :read_project, @subject.project) + can! :read_project_snippet if @subject.public? return unless @user -- cgit v1.2.1