Commit 5d6211e0 authored by Stan Hu's avatar Stan Hu

Log all API uploads that exceed max attachment size

Previously if the project were on the `GITLAB_UPLOAD_API_ALLOWLIST`
exemption list we would not log a message. However, it is still useful
to track these exceptions so that we know that they are happening. We
add a `upload_allowed` boolean field to make it possible to filter these
exempted uploads.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/325788
parent 01c56260
---
title: Log all API uploads that exceed max attachment size
merge_request: 59292
author:
type: changed
...@@ -71,10 +71,10 @@ module API ...@@ -71,10 +71,10 @@ module API
# This is to help determine which projects to use in https://gitlab.com/gitlab-org/gitlab/-/issues/325788 # This is to help determine which projects to use in https://gitlab.com/gitlab-org/gitlab/-/issues/325788
def log_if_upload_exceed_max_size(user_project, file) def log_if_upload_exceed_max_size(user_project, file)
return if file.size <= user_project.max_attachment_size return if file.size <= user_project.max_attachment_size
return if exempt_from_global_attachment_size?(user_project)
if file.size > user_project.max_attachment_size if file.size > user_project.max_attachment_size
Gitlab::AppLogger.info({ message: "File exceeds maximum size", file_bytes: file.size, project_id: user_project.id, project_path: user_project.full_path }) allowed = exempt_from_global_attachment_size?(user_project)
Gitlab::AppLogger.info({ message: "File exceeds maximum size", file_bytes: file.size, project_id: user_project.id, project_path: user_project.full_path, upload_allowed: allowed })
end end
end end
end end
......
...@@ -1587,14 +1587,6 @@ RSpec.describe API::Projects do ...@@ -1587,14 +1587,6 @@ RSpec.describe API::Projects do
expect(json_response['full_path']).to start_with("/#{project.namespace.path}/#{project.path}/uploads") expect(json_response['full_path']).to start_with("/#{project.namespace.path}/#{project.path}/uploads")
end end
it "logs a warning if file exceeds attachment size" do
allow(Gitlab::CurrentSettings).to receive(:max_attachment_size).and_return(0)
expect(Gitlab::AppLogger).to receive(:info).with(hash_including(message: 'File exceeds maximum size')).and_call_original
post api("/projects/#{project.id}/uploads", user), params: { file: file }
end
it "does not leave the temporary file in place after uploading, even when the tempfile reaper does not run" do it "does not leave the temporary file in place after uploading, even when the tempfile reaper does not run" do
stub_env('GITLAB_TEMPFILE_IMMEDIATE_UNLINK', '1') stub_env('GITLAB_TEMPFILE_IMMEDIATE_UNLINK', '1')
tempfile = Tempfile.new('foo') tempfile = Tempfile.new('foo')
...@@ -1613,7 +1605,7 @@ RSpec.describe API::Projects do ...@@ -1613,7 +1605,7 @@ RSpec.describe API::Projects do
expect(File.exist?(path)).to be(false) expect(File.exist?(path)).to be(false)
end end
shared_examples 'capped upload attachments' do shared_examples 'capped upload attachments' do |upload_allowed|
it "limits the upload to 1 GB" do it "limits the upload to 1 GB" do
expect_next_instance_of(UploadService) do |instance| expect_next_instance_of(UploadService) do |instance|
expect(instance).to receive(:override_max_attachment_size=).with(1.gigabyte).and_call_original expect(instance).to receive(:override_max_attachment_size=).with(1.gigabyte).and_call_original
...@@ -1623,6 +1615,16 @@ RSpec.describe API::Projects do ...@@ -1623,6 +1615,16 @@ RSpec.describe API::Projects do
expect(response).to have_gitlab_http_status(:created) expect(response).to have_gitlab_http_status(:created)
end end
it "logs a warning if file exceeds attachment size" do
allow(Gitlab::CurrentSettings).to receive(:max_attachment_size).and_return(0)
expect(Gitlab::AppLogger).to receive(:info).with(
hash_including(message: 'File exceeds maximum size', upload_allowed: upload_allowed))
.and_call_original
post api("/projects/#{project.id}/uploads", user), params: { file: file }
end
end end
context 'with exempted project' do context 'with exempted project' do
...@@ -1630,7 +1632,7 @@ RSpec.describe API::Projects do ...@@ -1630,7 +1632,7 @@ RSpec.describe API::Projects do
stub_env('GITLAB_UPLOAD_API_ALLOWLIST', project.id) stub_env('GITLAB_UPLOAD_API_ALLOWLIST', project.id)
end end
it_behaves_like 'capped upload attachments' it_behaves_like 'capped upload attachments', true
end end
context 'with upload size enforcement disabled' do context 'with upload size enforcement disabled' do
...@@ -1638,7 +1640,7 @@ RSpec.describe API::Projects do ...@@ -1638,7 +1640,7 @@ RSpec.describe API::Projects do
stub_feature_flags(enforce_max_attachment_size_upload_api: false) stub_feature_flags(enforce_max_attachment_size_upload_api: false)
end end
it_behaves_like 'capped upload attachments' it_behaves_like 'capped upload attachments', false
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment