From c14adba064aa86114dc43cae657212a4b19d6189 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 1 May 2023 12:11:08 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-9-stable-ee --- lib/gitlab/checks/branch_check.rb | 2 +- spec/lib/gitlab/checks/branch_check_spec.rb | 8 +++- ..._max_access_level_in_projects_preloader_spec.rb | 3 +- workhorse/internal/headers/content_headers.go | 32 +++++++++++-- workhorse/internal/headers/content_headers_test.go | 56 ++++++++++++++++++++++ .../contentprocessor/contentprocessor_test.go | 4 +- workhorse/testdata/index.xhtml | 9 ++++ workhorse/testdata/test.xml | 6 +++ workhorse/testdata/xml.svg | 7 +++ 9 files changed, 116 insertions(+), 11 deletions(-) create mode 100644 workhorse/internal/headers/content_headers_test.go create mode 100644 workhorse/testdata/index.xhtml create mode 100644 workhorse/testdata/test.xml create mode 100644 workhorse/testdata/xml.svg diff --git a/lib/gitlab/checks/branch_check.rb b/lib/gitlab/checks/branch_check.rb index e8f13a92ee7..fa7c4972c91 100644 --- a/lib/gitlab/checks/branch_check.rb +++ b/lib/gitlab/checks/branch_check.rb @@ -42,7 +42,7 @@ module Gitlab def prohibited_branch_checks return if deletion? - if branch_name =~ /\A\h{40}\z/ + if branch_name =~ %r{\A\h{40}(/|\z)} raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_hex_branch_name] end end diff --git a/spec/lib/gitlab/checks/branch_check_spec.rb b/spec/lib/gitlab/checks/branch_check_spec.rb index d6280d3c28c..7f535e86d69 100644 --- a/spec/lib/gitlab/checks/branch_check_spec.rb +++ b/spec/lib/gitlab/checks/branch_check_spec.rb @@ -26,8 +26,14 @@ RSpec.describe Gitlab::Checks::BranchCheck do expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a 40-character hexadecimal branch name.") end + it "prohibits 40-character hexadecimal branch names as the start of a path" do + allow(subject).to receive(:branch_name).and_return("267208abfe40e546f5e847444276f7d43a39503e/test") + + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a branch with a 40-character hexadecimal branch name.") + end + it "doesn't prohibit a nested hexadecimal in a branch name" do - allow(subject).to receive(:branch_name).and_return("fix-267208abfe40e546f5e847444276f7d43a39503e") + allow(subject).to receive(:branch_name).and_return("267208abfe40e546f5e847444276f7d43a39503e-fix") expect { subject.validate! }.not_to raise_error end diff --git a/spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb b/spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb index de10653d87e..a2ab59f56ab 100644 --- a/spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb +++ b/spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb @@ -23,8 +23,7 @@ RSpec.describe Preloaders::UserMaxAccessLevelInProjectsPreloader do # we have an existing N+1, one for each project for which user is not a member # in this spec, project_3, project_4, project_5 # https://gitlab.com/gitlab-org/gitlab/-/issues/362890 - ee_only_policy_check_queries = Gitlab.ee? ? 1 : 0 - expect { query }.to make_queries(projects.size + 3 + ee_only_policy_check_queries) + expect { query }.to make_queries(projects.size + 3) end end diff --git a/workhorse/internal/headers/content_headers.go b/workhorse/internal/headers/content_headers.go index 854cc8abddd..54c7c1bdd95 100644 --- a/workhorse/internal/headers/content_headers.go +++ b/workhorse/internal/headers/content_headers.go @@ -1,6 +1,7 @@ package headers import ( + "mime" "net/http" "regexp" @@ -13,8 +14,9 @@ var ( imageTypeRegex = regexp.MustCompile(`^image/*`) svgMimeTypeRegex = regexp.MustCompile(`^image/svg\+xml$`) - textTypeRegex = regexp.MustCompile(`^text/*`) - + textTypeRegex = regexp.MustCompile(`^text/*`) + xmlTypeRegex = regexp.MustCompile(`^text/xml`) + xhtmlTypeRegex = regexp.MustCompile(`^text/html`) videoTypeRegex = regexp.MustCompile(`^video/*`) pdfTypeRegex = regexp.MustCompile(`application\/pdf`) @@ -26,6 +28,8 @@ var ( // Mime types that can't be inlined. Usually subtypes of main types var forbiddenInlineTypes = []*regexp.Regexp{svgMimeTypeRegex} +var htmlRenderingTypes = []*regexp.Regexp{xmlTypeRegex, xhtmlTypeRegex} + // Mime types that can be inlined. We can add global types like "image/" or // specific types like "text/plain". If there is a specific type inside a global // allowed type that can't be inlined we must add it to the forbiddenInlineTypes var. @@ -38,12 +42,28 @@ const ( textPlainContentType = "text/plain; charset=utf-8" attachmentDispositionText = "attachment" inlineDispositionText = "inline" + dummyFilename = "blob" ) func SafeContentHeaders(data []byte, contentDisposition string) (string, string) { - contentType := safeContentType(data) + detectedContentType := detectContentType(data) + + contentType := safeContentType(detectedContentType) contentDisposition = safeContentDisposition(contentType, contentDisposition) + // Some browsers will render XML inline unless a filename directive is provided with a non-xml file extension + // This overrides the filename directive in the case of XML data + for _, element := range htmlRenderingTypes { + if isType(detectedContentType, element) { + disposition, directives, err := mime.ParseMediaType(contentDisposition) + if err == nil { + directives["filename"] = dummyFilename + contentDisposition = mime.FormatMediaType(disposition, directives) + break + } + } + } + // Set attachments to application/octet-stream since browsers can do // a better job distinguishing certain types (for example: ZIP files // vs. Microsoft .docx files). However, browsers may safely render SVGs even @@ -56,15 +76,17 @@ func SafeContentHeaders(data []byte, contentDisposition string) (string, string) return contentType, contentDisposition } -func safeContentType(data []byte) string { +func detectContentType(data []byte) string { // Special case for svg because DetectContentType detects it as text if svg.Is(data) { return svgContentType } // Override any existing Content-Type header from other ResponseWriters - contentType := http.DetectContentType(data) + return http.DetectContentType(data) +} +func safeContentType(contentType string) string { // http.DetectContentType does not support JavaScript and would only // return text/plain. But for cautionary measures, just in case they start supporting // it down the road and start returning application/javascript, we want to handle it now diff --git a/workhorse/internal/headers/content_headers_test.go b/workhorse/internal/headers/content_headers_test.go new file mode 100644 index 00000000000..7cfce335d88 --- /dev/null +++ b/workhorse/internal/headers/content_headers_test.go @@ -0,0 +1,56 @@ +package headers + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +func fileContents(fileName string) []byte { + fileContents, _ := os.ReadFile(fileName) + return fileContents +} + +func TestHeaders(t *testing.T) { + tests := []struct { + desc string + fileContents []byte + expectedContentType string + expectedContentDisposition string + }{ + { + desc: "XML file", + fileContents: fileContents("../../testdata/test.xml"), + expectedContentType: "text/plain; charset=utf-8", + expectedContentDisposition: "inline; filename=blob", + }, + { + desc: "XHTML file", + fileContents: fileContents("../../testdata/index.xhtml"), + expectedContentType: "text/plain; charset=utf-8", + expectedContentDisposition: "inline; filename=blob", + }, + { + desc: "svg+xml file", + fileContents: fileContents("../../testdata/xml.svg"), + expectedContentType: "image/svg+xml", + expectedContentDisposition: "attachment", + }, + { + desc: "text file", + fileContents: []byte(`a text file`), + expectedContentType: "text/plain; charset=utf-8", + expectedContentDisposition: "inline", + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + contentType, newContentDisposition := SafeContentHeaders(test.fileContents, "") + + require.Equal(t, test.expectedContentType, contentType) + require.Equal(t, test.expectedContentDisposition, newContentDisposition) + }) + } +} diff --git a/workhorse/internal/senddata/contentprocessor/contentprocessor_test.go b/workhorse/internal/senddata/contentprocessor/contentprocessor_test.go index b04263de6b9..e863935be6f 100644 --- a/workhorse/internal/senddata/contentprocessor/contentprocessor_test.go +++ b/workhorse/internal/senddata/contentprocessor/contentprocessor_test.go @@ -51,13 +51,13 @@ func TestSetProperContentTypeAndDisposition(t *testing.T) { { desc: "HTML type", contentType: "text/plain; charset=utf-8", - contentDisposition: "inline", + contentDisposition: "inline; filename=blob", body: "Hello world!", }, { desc: "Javascript within HTML type", contentType: "text/plain; charset=utf-8", - contentDisposition: "inline", + contentDisposition: "inline; filename=blob", body: "", }, { diff --git a/workhorse/testdata/index.xhtml b/workhorse/testdata/index.xhtml new file mode 100644 index 00000000000..1dd50a70e69 --- /dev/null +++ b/workhorse/testdata/index.xhtml @@ -0,0 +1,9 @@ + + + + Title of document + + + + diff --git a/workhorse/testdata/test.xml b/workhorse/testdata/test.xml new file mode 100644 index 00000000000..54b94e62355 --- /dev/null +++ b/workhorse/testdata/test.xml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/workhorse/testdata/xml.svg b/workhorse/testdata/xml.svg new file mode 100644 index 00000000000..c41c4c44b49 --- /dev/null +++ b/workhorse/testdata/xml.svg @@ -0,0 +1,7 @@ + + + + + +
hello this is html
+ -- cgit v1.2.1