diff --git a/lib/api/helpers/attachment_renderer.rb b/lib/api/helpers/attachment_renderer.rb index 440b0935aa3..25ae274267b 100644 --- a/lib/api/helpers/attachment_renderer.rb +++ b/lib/api/helpers/attachment_renderer.rb @@ -94,6 +94,8 @@ module API def attachment_content_type(attachment) if attachment.is_text? + # Even if the text mime type might differ, always output plain text + # so this doesn't get interpreted as e.g., a script or html file "text/plain" elsif attachment.inlineable? attachment.content_type diff --git a/spec/features/security/plain_text_content_type.rb b/spec/features/security/plain_text_content_type.rb index 1179304f25b..465aa49cdbb 100644 --- a/spec/features/security/plain_text_content_type.rb +++ b/spec/features/security/plain_text_content_type.rb @@ -28,47 +28,47 @@ require "spec_helper" -RSpec.describe "Session TTL", - with_settings: { session_ttl_enabled?: true, session_ttl: "10" } do +RSpec.describe "Plain text content type XSS prevention", :js, :with_cuprite do shared_let(:admin) { create(:admin) } - let(:admin_password) { "adminADMIN!" } + shared_let(:work_package) { create(:work_package) } - let!(:work_package) { create(:work_package) } + let(:wp_page) { Pages::FullWorkPackage.new(work_package) } + let(:attachments) { Components::Attachments.new } + let(:attachments_list) { Components::AttachmentsList.new } + let(:image_fixture) { UploadedFile.load_from("spec/fixtures/files/test.js") } before do - login_with(admin.login, admin_password) + login_as admin end - def expire! - page.set_rack_session(updated_at: Time.now - 1.hour) - end + it "allows accessing text/javascript files as inlinable plain text" do + wp_page.visit_tab!(:files) + attachments_list.wait_until_visible - describe "outdated TTL on Rails request" do - it "expires on the next Rails request" do - visit "/my/account" - expect(page).to have_css(".form--field-container", text: admin.login) + ## + # Attach file manually + attachments_list.expect_empty + attachments.attach_file_on_input(image_fixture.path) + attachments_list.expect_attached("test.js") - # Expire the session - expire! + expect(work_package.attachments.count).to eq 1 + attachment = work_package.attachments.first - visit "/" - expect(page).to have_css(".action-login") - end - end + expect(attachment.content_type).to eq "text/x-javascript" + expect(attachment).to be_inlineable - describe "outdated TTL on API request" do - it "expires on the next APIv3 request" do - page.driver.header("X-Requested-With", "XMLHttpRequest") - visit "/api/v3/work_packages/#{work_package.id}" + visit home_path - body = JSON.parse(page.body) - expect(body["id"]).to eq(work_package.id) - - # Expire the session - expire! - visit "/api/v3/work_packages/#{work_package.id}" - - expect(page.body).to eq("unauthorized") - end + # Assume we have a HTML sanitation issue + expect do + accept_alert(wait: 1) do + page.execute_script <<-JS + const element = document.createElement('script') + element.id = "testscript" + element.src = '/api/v3/attachments/#{attachment.id}/content' + document.body.appendChild(element); + JS + end + end.to raise_error(Capybara::ModalNotFound) end end diff --git a/spec/fixtures/files/test.js b/spec/fixtures/files/test.js new file mode 100644 index 00000000000..3254c929467 --- /dev/null +++ b/spec/fixtures/files/test.js @@ -0,0 +1 @@ +alert("hello"); diff --git a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb index 10d9a529335..cc32297a87d 100644 --- a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb +++ b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb @@ -219,6 +219,8 @@ RSpec.shared_examples "an APIv3 attachment resource", content_type: :json, type: let(:missing_permissions_user) { user_with_permissions } + let(:expected_content_type) { "application/octet-stream" } + before do allow(User).to receive(:current).and_return current_user end @@ -452,7 +454,7 @@ RSpec.shared_examples "an APIv3 attachment resource", content_type: :json, type: .to eql content_disposition expect(subject.headers["Content-Type"]) - .to eql mock_file.content_type + .to eql expected_content_type max_age = OpenProject::Configuration.fog_download_url_expires_in.to_i - 10 @@ -473,11 +475,20 @@ RSpec.shared_examples "an APIv3 attachment resource", content_type: :json, type: context "for a local text file" do it_behaves_like "for a local file" do + let(:expected_content_type) { "text/plain" } let(:mock_file) { FileHelpers.mock_uploaded_file name: "foobar.txt" } let(:content_disposition) { "inline; filename=foobar.txt" } end end + context "for a local JS file" do + it_behaves_like "for a local file" do + let(:expected_content_type) { "text/plain" } + let(:mock_file) { FileHelpers.mock_uploaded_file name: "foobar.js", content_type: "text/x-javascript" } + let(:content_disposition) { "inline; filename=foobar.js" } + end + end + context "for a local binary file" do it_behaves_like "for a local file" do let(:mock_file) { FileHelpers.mock_uploaded_file name: "foobar.dat", content_type: "application/octet-stream" }