summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>2023-05-02 11:11:07 +0000
committerGitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>2023-05-02 11:11:07 +0000
commit8554eaaf8c461767bae4be077d925aec055dde4b (patch)
tree4d3748f99871b44b64fa4074ff9ecf1eba1ade44
parentc6cc9bc94e23e01a01ed191aba993ccf2b443680 (diff)
parent44e981b3fb85a561c9d93f6d823d562b27789df4 (diff)
downloadgitlab-ce-8554eaaf8c461767bae4be077d925aec055dde4b.tar.gz
Merge remote-tracking branch 'dev/15-9-stable' into 15-9-stable
-rw-r--r--CHANGELOG.md13
-rw-r--r--GITALY_SERVER_VERSION2
-rw-r--r--GITLAB_PAGES_VERSION2
-rw-r--r--VERSION2
-rw-r--r--app/helpers/avatars_helper.rb2
-rw-r--r--doc/integration/saml.md2
-rw-r--r--lib/banzai/filter/asset_proxy_filter.rb44
-rw-r--r--lib/gitlab/checks/branch_check.rb2
-rw-r--r--lib/gitlab/git/repository.rb26
-rw-r--r--spec/helpers/avatars_helper_spec.rb16
-rw-r--r--spec/lib/banzai/filter/asset_proxy_filter_spec.rb9
-rw-r--r--spec/lib/banzai/filter/commit_trailers_filter_spec.rb2
-rw-r--r--spec/lib/gitlab/checks/branch_check_spec.rb8
-rw-r--r--spec/lib/gitlab/git/repository_spec.rb70
-rw-r--r--spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb3
-rw-r--r--workhorse/internal/headers/content_headers.go32
-rw-r--r--workhorse/internal/headers/content_headers_test.go56
-rw-r--r--workhorse/internal/senddata/contentprocessor/contentprocessor_test.go4
-rw-r--r--workhorse/testdata/index.xhtml9
-rw-r--r--workhorse/testdata/test.xml6
-rw-r--r--workhorse/testdata/xml.svg7
21 files changed, 293 insertions, 24 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f8582f91b45..5330484d047 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,19 @@
documentation](doc/development/changelog.md) for instructions on adding your own
entry.
+## 15.9.6 (2023-05-01)
+
+### Security (8 changes)
+
+- [Resolve ambiguous references for archive metadata](gitlab-org/security/gitlab@233b0f78baf8eb9adcfd77e4d1aa606d54472d34) ([merge request](gitlab-org/security/gitlab!3203))
+- [Commit trailers now only match public user email addresses](gitlab-org/security/gitlab@e360774721bb9b5f6a2da9908ef08d92ad5a79cd) ([merge request](gitlab-org/security/gitlab!3209))
+- [Handle invalid URLs in asset proxy](gitlab-org/security/gitlab@ee6df7196b14014b5416f090a684e3b6ba600b5a) ([merge request](gitlab-org/security/gitlab!3213))
+- [Relay state to check for only allowing sub paths](gitlab-org/security/gitlab@c690eec0a2f8aa506b8ff3ffadf306aa91501648) ([merge request](gitlab-org/security/gitlab!3221))
+- [Prohibit 40 character hex sets at beginning of path-based branch name](gitlab-org/security/gitlab@889683b6b1884bfc36208dfae899d0fb9437246c) ([merge request](gitlab-org/security/gitlab!3195))
+- [Update policy to prevent banned members from accessing public projects](gitlab-org/security/gitlab@1abcbdc23881dab5f675e858afa31be87d5d47ce) ([merge request](gitlab-org/security/gitlab!3187))
+- [Use dummy filename as filename when viewing raw xml files](gitlab-org/security/gitlab@33563159bcc7d46c95f013bf089ed94128f10379) ([merge request](gitlab-org/security/gitlab!3193))
+- [Authorize access to vulnerabilitiesCountByDay resolver](gitlab-org/security/gitlab@4b0825f79b0a27eeddabaee0b3a7f627b2487706) ([merge request](gitlab-org/security/gitlab!3181))
+
## 15.9.5 (2023-04-21)
### Fixed (1 change)
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 252fd0533de..c7c1b69191f 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-15.9.5 \ No newline at end of file
+15.9.6 \ No newline at end of file
diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION
index 252fd0533de..c7c1b69191f 100644
--- a/GITLAB_PAGES_VERSION
+++ b/GITLAB_PAGES_VERSION
@@ -1 +1 @@
-15.9.5 \ No newline at end of file
+15.9.6 \ No newline at end of file
diff --git a/VERSION b/VERSION
index 252fd0533de..c7c1b69191f 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-15.9.5 \ No newline at end of file
+15.9.6 \ No newline at end of file
diff --git a/app/helpers/avatars_helper.rb b/app/helpers/avatars_helper.rb
index 0fac2cb5fc5..57075a44d0f 100644
--- a/app/helpers/avatars_helper.rb
+++ b/app/helpers/avatars_helper.rb
@@ -116,7 +116,7 @@ module AvatarsHelper
private
def avatar_icon_by_user_email_or_gravatar(email, size, scale, only_path:)
- user = User.find_by_any_email(email)
+ user = User.with_public_email(email).first
if user
avatar_icon_for_user(user, size, scale, only_path: only_path)
diff --git a/doc/integration/saml.md b/doc/integration/saml.md
index 24b5e6152a5..231709df7f4 100644
--- a/doc/integration/saml.md
+++ b/doc/integration/saml.md
@@ -3115,7 +3115,7 @@ such as the following:
| Encrypt SAML assertion | Optional | Uses TLS between your identity provider, the user's browser, and GitLab. |
| Sign SAML assertion | Optional | Validates the integrity of a SAML assertion. When active, signs the whole response. |
| Check SAML request signature | Optional | Checks the signature on the SAML response. |
-| Default RelayState | Optional | Specifies the URL users should end up on after successfully signing in through SAML at your IdP. |
+| Default RelayState | Optional | Specifies the sub-paths of the base URL that users should end up on after successfully signing in through SAML at your IdP. |
| NameID format | Persistent | See [NameID format details](../user/group/saml_sso/index.md#nameid-format). |
| Additional URLs | Optional | May include the issuer, identifier, or assertion consumer service URL in other fields on some providers. |
diff --git a/lib/banzai/filter/asset_proxy_filter.rb b/lib/banzai/filter/asset_proxy_filter.rb
index 4c14ee7299b..6371a8f23af 100644
--- a/lib/banzai/filter/asset_proxy_filter.rb
+++ b/lib/banzai/filter/asset_proxy_filter.rb
@@ -6,11 +6,35 @@ module Banzai
# as well as hiding the customer's IP address when requesting images.
# Copies the original img `src` to `data-canonical-src` then replaces the
# `src` with a new url to the proxy server.
- class AssetProxyFilter < HTML::Pipeline::CamoFilter
+ #
+ # Based on https://github.com/gjtorikian/html-pipeline/blob/v2.14.3/lib/html/pipeline/camo_filter.rb
+ class AssetProxyFilter < HTML::Pipeline::Filter
def initialize(text, context = nil, result = nil)
super
end
+ def call
+ return doc unless asset_proxy_enabled?
+
+ doc.search('img').each do |element|
+ original_src = element['src']
+ next unless original_src
+
+ begin
+ uri = URI.parse(original_src)
+ rescue StandardError
+ next
+ end
+
+ next if uri.host.nil? && !original_src.start_with?('///')
+ next if asset_host_allowed?(uri.host)
+
+ element['src'] = asset_proxy_url(original_src)
+ element['data-canonical-src'] = original_src
+ end
+ doc
+ end
+
def validate
needs(:asset_proxy, :asset_proxy_secret_key) if asset_proxy_enabled?
end
@@ -63,6 +87,24 @@ module Banzai
application_settings.try(:asset_proxy_whitelist).presence ||
[Gitlab.config.gitlab.host]
end
+
+ private
+
+ def asset_proxy_enabled?
+ !context[:disable_asset_proxy]
+ end
+
+ def asset_proxy_url(url)
+ "#{context[:asset_proxy]}/#{asset_url_hash(url)}/#{hexencode(url)}"
+ end
+
+ def asset_url_hash(url)
+ OpenSSL::HMAC.hexdigest('sha1', context[:asset_proxy_secret_key], url)
+ end
+
+ def hexencode(str)
+ str.unpack1('H*')
+ end
end
end
end
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/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index e054b6df98f..e1399b6642b 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -262,7 +262,11 @@ module Gitlab
def archive_metadata(ref, storage_path, project_path, format = "tar.gz", append_sha:, path: nil)
ref ||= root_ref
- commit = Gitlab::Git::Commit.find(self, ref)
+
+ commit_id = extract_commit_id_from_ref(ref)
+ return {} if commit_id.nil?
+
+ commit = Gitlab::Git::Commit.find(self, commit_id)
return {} if commit.nil?
prefix = archive_prefix(ref, commit.id, project_path, append_sha: append_sha, path: path)
@@ -1233,6 +1237,26 @@ module Gitlab
def gitaly_delete_refs(*ref_names)
gitaly_ref_client.delete_refs(refs: ref_names) if ref_names.any?
end
+
+ # The order is based on git priority to resolve ambiguous references
+ #
+ # `git show <ref>`
+ #
+ # In case of name clashes, it uses this order:
+ # 1. Commit
+ # 2. Tag
+ # 3. Branch
+ def extract_commit_id_from_ref(ref)
+ return ref if Gitlab::Git.commit_id?(ref)
+
+ tag = find_tag(ref)
+ return tag.dereferenced_target.sha if tag
+
+ branch = find_branch(ref)
+ return branch.dereferenced_target.sha if branch
+
+ ref
+ end
end
end
end
diff --git a/spec/helpers/avatars_helper_spec.rb b/spec/helpers/avatars_helper_spec.rb
index cef72d24c43..bf23c74c0f0 100644
--- a/spec/helpers/avatars_helper_spec.rb
+++ b/spec/helpers/avatars_helper_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe AvatarsHelper do
+RSpec.describe AvatarsHelper, feature_category: :source_code_management do
include UploadHelpers
let_it_be(:user) { create(:user) }
@@ -88,7 +88,7 @@ RSpec.describe AvatarsHelper do
describe '#avatar_icon_for' do
let!(:user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: 'bar@example.com') }
let(:email) { 'foo@example.com' }
- let!(:another_user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: email) }
+ let!(:another_user) { create(:user, :public_email, avatar: File.open(uploaded_image_temp_path), email: email) }
it 'prefers the user to retrieve the avatar_url' do
expect(helper.avatar_icon_for(user, email).to_s)
@@ -102,7 +102,7 @@ RSpec.describe AvatarsHelper do
end
describe '#avatar_icon_for_email', :clean_gitlab_redis_cache do
- let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
+ let(:user) { create(:user, :public_email, avatar: File.open(uploaded_image_temp_path)) }
subject { helper.avatar_icon_for_email(user.email).to_s }
@@ -114,6 +114,14 @@ RSpec.describe AvatarsHelper do
end
end
+ context 'when a private email is used' do
+ it 'calls gravatar_icon' do
+ expect(helper).to receive(:gravatar_icon).with(user.commit_email, 20, 2)
+
+ helper.avatar_icon_for_email(user.commit_email, 20, 2)
+ end
+ end
+
context 'when no user exists for the email' do
it 'calls gravatar_icon' do
expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
@@ -136,7 +144,7 @@ RSpec.describe AvatarsHelper do
it_behaves_like "returns avatar for email"
it "caches the request" do
- expect(User).to receive(:find_by_any_email).once.and_call_original
+ expect(User).to receive(:with_public_email).once.and_call_original
expect(helper.avatar_icon_for_email(user.email).to_s).to eq(user.avatar.url)
expect(helper.avatar_icon_for_email(user.email).to_s).to eq(user.avatar.url)
diff --git a/spec/lib/banzai/filter/asset_proxy_filter_spec.rb b/spec/lib/banzai/filter/asset_proxy_filter_spec.rb
index 004c70c28f1..dc6ac52a8c2 100644
--- a/spec/lib/banzai/filter/asset_proxy_filter_spec.rb
+++ b/spec/lib/banzai/filter/asset_proxy_filter_spec.rb
@@ -80,6 +80,15 @@ RSpec.describe Banzai::Filter::AssetProxyFilter, feature_category: :team_plannin
expect(doc.at_css('img')['data-canonical-src']).to eq src
end
+ it 'replaces invalid URLs' do
+ src = '///example.com/test.png'
+ new_src = 'https://assets.example.com/3368d2c7b9bed775bdd1e811f36a4b80a0dcd8ab/2f2f2f6578616d706c652e636f6d2f746573742e706e67'
+ doc = filter(image(src), @context)
+
+ expect(doc.at_css('img')['src']).to eq new_src
+ expect(doc.at_css('img')['data-canonical-src']).to eq src
+ end
+
it 'skips internal images' do
src = "#{Gitlab.config.gitlab.url}/test.png"
doc = filter(image(src), @context)
diff --git a/spec/lib/banzai/filter/commit_trailers_filter_spec.rb b/spec/lib/banzai/filter/commit_trailers_filter_spec.rb
index 3ebe0798972..896f3beb7c2 100644
--- a/spec/lib/banzai/filter/commit_trailers_filter_spec.rb
+++ b/spec/lib/banzai/filter/commit_trailers_filter_spec.rb
@@ -218,7 +218,7 @@ RSpec.describe Banzai::Filter::CommitTrailersFilter, feature_category: :source_c
# any path-only link will automatically be prefixed
# with the path of its repository.
# See: "build_relative_path" in "lib/banzai/filter/relative_link_filter.rb"
- let(:user_with_avatar) { create(:user, :with_avatar, username: 'foobar') }
+ let(:user_with_avatar) { create(:user, :public_email, :with_avatar, username: 'foobar') }
it 'returns a full path for avatar urls' do
_, message_html = build_commit_message(
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/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 72043ba2a21..a8423703716 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -116,7 +116,8 @@ RSpec.describe Gitlab::Git::Repository, feature_category: :source_code_managemen
let(:expected_extension) { 'tar.gz' }
let(:expected_filename) { "#{expected_prefix}.#{expected_extension}" }
let(:expected_path) { File.join(storage_path, cache_key, "@v2", expected_filename) }
- let(:expected_prefix) { "gitlab-git-test-#{ref}-#{TestEnv::BRANCH_SHA['master']}" }
+ let(:expected_prefix) { "gitlab-git-test-#{ref.tr('/', '-')}-#{expected_prefix_sha}" }
+ let(:expected_prefix_sha) { TestEnv::BRANCH_SHA['master'] }
subject(:metadata) { repository.archive_metadata(ref, storage_path, 'gitlab-git-test', format, append_sha: append_sha, path: path) }
@@ -173,6 +174,73 @@ RSpec.describe Gitlab::Git::Repository, feature_category: :source_code_managemen
it { expect(metadata['ArchivePath']).to eq(expected_path) }
end
end
+
+ context 'when references are ambiguous' do
+ let_it_be(:ambiguous_project) { create(:project, :repository) }
+ let_it_be(:repository) { ambiguous_project.repository.raw }
+ let_it_be(:branch_merged_commit_id) { ambiguous_project.repository.find_branch('branch-merged').dereferenced_target.id }
+ let_it_be(:branch_master_commit_id) { ambiguous_project.repository.find_branch('master').dereferenced_target.id }
+ let_it_be(:tag_1_0_0_commit_id) { ambiguous_project.repository.find_tag('v1.0.0').dereferenced_target.id }
+
+ context 'when tag is ambiguous' do
+ before do
+ ambiguous_project.repository.add_tag(user, ref, 'master', 'foo')
+ end
+
+ after do
+ ambiguous_project.repository.rm_tag(user, ref)
+ end
+
+ where(:ref, :expected_commit_id, :desc) do
+ 'refs/heads/branch-merged' | ref(:branch_master_commit_id) | 'when tag looks like a branch'
+ 'branch-merged' | ref(:branch_master_commit_id) | 'when tag has the same name as a branch'
+ ref(:branch_merged_commit_id) | ref(:branch_merged_commit_id) | 'when tag looks like a commit id'
+ 'v0.0.0' | ref(:branch_master_commit_id) | 'when tag looks like a normal tag'
+ end
+
+ with_them do
+ it 'selects the correct commit' do
+ expect(metadata['CommitId']).to eq(expected_commit_id)
+ end
+ end
+ end
+
+ context 'when branch is ambiguous' do
+ before do
+ ambiguous_project.repository.add_branch(user, ref, 'master')
+ end
+
+ where(:ref, :expected_commit_id, :desc) do
+ 'refs/tags/v1.0.0' | ref(:branch_master_commit_id) | 'when branch looks like a tag'
+ 'v1.0.0' | ref(:tag_1_0_0_commit_id) | 'when branch has the same name as a tag'
+ ref(:branch_merged_commit_id) | ref(:branch_merged_commit_id) | 'when branch looks like a commit id'
+ 'just-a-normal-branch' | ref(:branch_master_commit_id) | 'when branch looks like a normal branch'
+ end
+
+ with_them do
+ it 'selects the correct commit' do
+ expect(metadata['CommitId']).to eq(expected_commit_id)
+ end
+ end
+ end
+
+ context 'when ref is HEAD' do
+ let(:ref) { 'HEAD' }
+
+ it 'selects commit id from HEAD ref' do
+ expect(metadata['CommitId']).to eq(branch_master_commit_id)
+ expect(metadata['ArchivePrefix']).to eq(expected_prefix)
+ end
+ end
+
+ context 'when ref is not found' do
+ let(:ref) { 'unknown-ref-cannot-be-found' }
+
+ it 'returns empty metadata' do
+ expect(metadata).to eq({})
+ end
+ end
+ end
end
describe '#size' do
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: "<html><body>Hello world!</body></html>",
},
{
desc: "Javascript within HTML type",
contentType: "text/plain; charset=utf-8",
- contentDisposition: "inline",
+ contentDisposition: "inline; filename=blob",
body: "<script>alert(\"foo\")</script>",
},
{
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 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
+"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head>
+ <title>Title of document</title>
+</head>
+<body>
+</body>
+</html>
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 @@
+<?xml version="1.0" encoding="utf-8"?>
+<html>
+ <head></head>
+ <body>
+ </body>
+</html> \ 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 @@
+<?xml version="1.0" standalone="no"?>
+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
+
+<svg version="1.1" baseProfile="full" xmlns="http://www.w3.org/2000/svg">
+ <polygon id="triangle" points="0,0 0,50 50,0" fill="#009900" stroke="#004400"/>
+ <div>hello this is html</div>
+</svg>