From f7703d4b2ad90d067fed6fd2a9f9aa0ce381c251 Mon Sep 17 00:00:00 2001 From: MAEDA Go Date: Sun, 8 Nov 2026 17:20:06 +0900 Subject: [PATCH] Stream ZIP downloads of attachments and wiki pages using zip_kit Bulk attachment downloads and wiki ZIP exports built the whole archive in memory before sending it. Generate them with zip_kit and stream the response instead, since rubyzip's OutputStream needs a seekable output. Redmine::Export::ZipArchive renames duplicate entry names and stores modification times in the user's time zone, and ApplicationController#send_zip_stream sends the archive. The response allows reverse proxies to buffer it so that the application process is not held by slow clients. Entries in bulk attachment downloads now have the attachment's creation time instead of the download time. rubyzip is no longer used at runtime, so it is moved to the test group where it is used to read the generated archives. Co-Authored-By: Claude Opus 5.5 --- Gemfile | 4 +- app/controllers/application_controller.rb | 12 +++ app/controllers/attachments_controller.rb | 19 ++--- app/controllers/wiki_controller.rb | 64 ++++----------- app/models/attachment.rb | 28 ------- lib/redmine/export/zip_archive.rb | 56 +++++++++++++ .../functional/attachments_controller_test.rb | 8 +- test/functional/wiki_controller_test.rb | 8 +- test/unit/attachment_test.rb | 30 ------- .../lib/redmine/export/zip_archive_test.rb | 79 +++++++++++++++++++ 10 files changed, 188 insertions(+), 120 deletions(-) create mode 100644 lib/redmine/export/zip_archive.rb create mode 100644 test/unit/lib/redmine/export/zip_archive_test.rb diff --git a/Gemfile b/Gemfile index acb42a470..b9f0edb46 100644 --- a/Gemfile +++ b/Gemfile @@ -15,7 +15,7 @@ gem 'i18n', '~> 1.15.2' # Loaded on demand by lib/redmine/export/pdf/itcpdf.rb, keep `require: false` gem 'rbpdf', '~> 1.21.4', require: false gem 'addressable' -gem 'rubyzip', '~> 3.7.0' +gem 'zip_kit', '~> 6.4.1' gem 'propshaft', '~> 1.3.0' gem 'rack', '>= 3.1.3' gem "stimulus-rails", "~> 1.3" @@ -124,6 +124,8 @@ group :test do # for testing oauth provider capabilities gem 'oauth2' gem 'webrick' + # for reading ZIP files generated by zip_kit + gem 'rubyzip' end local_gemfile = File.join(File.dirname(__FILE__), "Gemfile.local") diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2118193ce..cf6d4d626 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -773,6 +773,18 @@ class ApplicationController < ActionController::Base name end + # Streams a ZIP file built in the given block, which receives a + # Redmine::Export::ZipArchive + def send_zip_stream(file_name) + user = User.current + # Let reverse proxies such as nginx buffer the response so that + # the application process is released without waiting for slow clients + response.headers['x-accel-buffering'] = 'yes' + zip_kit_stream(:filename => filename_for_content_disposition(file_name)) do |streamer| + yield Redmine::Export::ZipArchive.new(streamer, user) + end + end + def api_request? %w(xml json).include? params[:format] end diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 1aad88a3c..3d77a96be 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -144,16 +144,17 @@ class AttachmentsController < ApplicationController end def download_all - zip_data = Attachment.archive_attachments(@attachments) - if zip_data - file_name = "#{@container.class.to_s.downcase}-#{@container.id}-attachments.zip" - send_data( - zip_data, - :type => Redmine::MimeType.of(file_name), - :filename => file_name - ) - else + if @attachments.blank? render_404 + return + end + + send_zip_stream("#{@container.class.to_s.downcase}-#{@container.id}-attachments.zip") do |zip| + @attachments.each do |attachment| + zip.add_file(attachment.filename, attachment.created_on) do |sink| + File.open(attachment.diskfile, 'rb') {|file| IO.copy_stream(file, sink)} + end + end end end diff --git a/app/controllers/wiki_controller.rb b/app/controllers/wiki_controller.rb index 3bc196064..e43d87058 100644 --- a/app/controllers/wiki_controller.rb +++ b/app/controllers/wiki_controller.rb @@ -17,8 +17,6 @@ # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -require 'zip' - # The WikiController follows the Rails REST controller pattern but with # a few differences # @@ -311,24 +309,25 @@ class WikiController < ApplicationController # Export wiki to a single pdf or html file def export - @pages = @wiki.pages. - includes([:content, {:attachments => :author}]). - to_a respond_to do |format| format.html do + load_pages_for_export export = render_to_string :action => 'export_multiple', :layout => false send_data(export, :type => 'text/html', :filename => "wiki.html") end format.pdf do + load_pages_for_export send_file_headers! :type => 'application/pdf', :filename => "#{@project.identifier}.pdf" end format.zip do - file_name = "#{@project.identifier}-wiki.zip" - send_data( - wiki_pages_to_zip(@pages), - :type => Redmine::MimeType.of(file_name), - :filename => filename_for_content_disposition(file_name) - ) + send_zip_stream("#{@project.identifier}-wiki.zip") do |zip| + @wiki.pages.with_updated_on.each do |page| + text = WikiContent.where(:page_id => page.id).pick(:text).to_s + zip.add_file("#{archived_wiki_page_title(page)}.txt", page.updated_on) do |sink| + IO.copy_stream(StringIO.new(text), sink) + end + end + end end end end @@ -411,46 +410,15 @@ class WikiController < ApplicationController to_a end - def wiki_pages_to_zip(pages) - Zip.unicode_names = true - archived_file_names = [] - buffer = Zip::OutputStream.write_buffer do |zos| - pages.each do |page| - filename = archived_wiki_page_filename(page, archived_file_names) - entry = Zip::Entry.new('', filename) - if page.updated_on.present? - local_time = User.current.convert_time_to_user_timezone(page.updated_on) - # DOS timestamp stores user's displayed local time - entry.time = Zip::DOSTime.new( - local_time.year, local_time.month, local_time.day, - local_time.hour, local_time.min, local_time.sec - ) - # UT extra field stores time in UTC - entry.extra[:universaltime].mtime = local_time.utc - end - zos.put_next_entry(entry) - zos << page.content.text.to_s - end - end - buffer.string - ensure - buffer&.close + def load_pages_for_export + @pages = @wiki.pages. + includes([:content, {:attachments => :author}]). + to_a end - def archived_wiki_page_filename(page, archived_file_names) - extension = '.txt' + def archived_wiki_page_title(page) # Keep this character set aligned with Attachment#sanitize_filename. # Unlike attachments, do not drop path-like components from wiki titles. - sanitized_title = page.title.tr('\\', '_').gsub(/[\/?%*:|"'<>\n\r\x00]+/, '_') - filename = "#{sanitized_title}#{extension}" - dup_count = 0 - - while archived_file_names.include?(filename) - dup_count += 1 - filename = "#{sanitized_title}(#{dup_count})#{extension}" - end - - archived_file_names << filename - filename + page.title.tr('\\', '_').gsub(/[\/?%*:|"'<>\n\r\x00]+/, '_') end end diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 6b00ab3b2..dabb68db3 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -19,7 +19,6 @@ require "digest" require "fileutils" -require "zip" class Attachment < ApplicationRecord include Redmine::SafeAttributes @@ -452,33 +451,6 @@ class Attachment < ApplicationRecord Attachment.where("created_on < ? AND (container_type IS NULL OR container_type = '')", Time.now - age).destroy_all end - def self.archive_attachments(attachments) - attachments = attachments.select(&:readable?) - return nil if attachments.blank? - - Zip.unicode_names = true - archived_file_names = [] - buffer = Zip::OutputStream.write_buffer do |zos| - attachments.each do |attachment| - filename = attachment.filename - # rename the file if a file with the same name already exists - dup_count = 0 - while archived_file_names.include?(filename) - dup_count += 1 - extname = File.extname(attachment.filename) - basename = File.basename(attachment.filename, extname) - filename = "#{basename}(#{dup_count})#{extname}" - end - zos.put_next_entry(filename) - zos << File.binread(attachment.diskfile) - archived_file_names << filename - end - end - buffer.string - ensure - buffer&.close - end - # Moves an existing attachment to its target directory def move_to_target_directory! return unless !new_record? && readable? diff --git a/lib/redmine/export/zip_archive.rb b/lib/redmine/export/zip_archive.rb new file mode 100644 index 000000000..633c31a8d --- /dev/null +++ b/lib/redmine/export/zip_archive.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +# Redmine - project management software +# Copyright (C) 2006- Jean-Philippe Lang +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +module Redmine + module Export + class ZipArchive + def initialize(streamer, user) + @streamer = streamer + @user = user + @entry_names = Set.new + end + + def add_file(name, modification_time, &) + entry_name = unique_entry_name(name) + @entry_names << entry_name + @streamer.write_file(entry_name, :modification_time => zip_time(modification_time), &) + end + + private + + def unique_entry_name(name) + entry_name = name + extname = File.extname(name) + basename = File.basename(name, extname) + dup_count = 0 + while @entry_names.include?(entry_name) + dup_count += 1 + entry_name = "#{basename}(#{dup_count})#{extname}" + end + entry_name + end + + # DateTime is used because zip_kit calls #utc on the given time, + # which converts a Time object in place. + def zip_time(time) + @user.convert_time_to_user_timezone(time).to_datetime + end + end + end +end diff --git a/test/functional/attachments_controller_test.rb b/test/functional/attachments_controller_test.rb index 2a5856cfa..cb61a3f7c 100644 --- a/test/functional/attachments_controller_test.rb +++ b/test/functional/attachments_controller_test.rb @@ -793,7 +793,13 @@ class AttachmentsControllerTest < Redmine::ControllerTest assert_response :ok assert_equal response.headers['Content-Type'], 'application/zip' assert_match /issue-2-attachments.zip/, response.headers['Content-Disposition'] - assert_not_includes Dir.entries(Rails.root.join('tmp')), /attachments_zip/ + assert_equal 'yes', response.headers['X-Accel-Buffering'] + zip_data = +"" + response.body.each {|chunk| zip_data << chunk} + Zip::File.open_buffer(zip_data) do |zip| + assert_equal ['source.rb'], zip.entries.map(&:name) + assert_equal File.binread(Attachment.find(4).diskfile), zip.read('source.rb') + end end def test_download_all_with_invalid_container diff --git a/test/functional/wiki_controller_test.rb b/test/functional/wiki_controller_test.rb index 9f4ee657b..2e9b2542d 100644 --- a/test/functional/wiki_controller_test.rb +++ b/test/functional/wiki_controller_test.rb @@ -1389,11 +1389,13 @@ class WikiControllerTest < Redmine::ControllerTest def zip_entries_from_response entries = {} + zip_data = +'' + @response.body.each {|chunk| zip_data << chunk} - Zip::InputStream.open(StringIO.new(@response.body)) do |io| - while (entry = io.get_next_entry) + Zip::File.open_buffer(zip_data) do |zip| + zip.each do |entry| entries[entry.name] = { - :content => io.read, + :content => entry.get_input_stream.read, :dos_time => entry.instance_variable_get(:@time), :utc_time => entry.extra[:universaltime]&.mtime } diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb index 8ad4b5973..a34cb1b73 100644 --- a/test/unit/attachment_test.rb +++ b/test/unit/attachment_test.rb @@ -397,36 +397,6 @@ class AttachmentTest < ActiveSupport::TestCase end end - def test_archive_attachments - attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", ""), :author_id => 1) - zip_data = Attachment.archive_attachments([attachment]) - file_names = [] - Zip::InputStream.open(StringIO.new(zip_data)) do |io| - while (entry = io.get_next_entry) - file_names << entry.name - end - end - assert_equal ['testfile.txt'], file_names - end - - def test_archive_attachments_without_attachments - zip_data = Attachment.archive_attachments([]) - assert_nil zip_data - end - - def test_archive_attachments_should_rename_duplicate_file_names - attachment1 = Attachment.create!(:file => uploaded_test_file("testfile.txt", ""), :author_id => 1) - attachment2 = Attachment.create!(:file => uploaded_test_file("testfile.txt", ""), :author_id => 1) - zip_data = Attachment.archive_attachments([attachment1, attachment2]) - file_names = [] - Zip::InputStream.open(StringIO.new(zip_data)) do |io| - while (entry = io.get_next_entry) - file_names << entry.name - end - end - assert_equal ['testfile.txt', 'testfile(1).txt'], file_names - end - def test_move_from_root_to_target_directory_should_move_root_files a = Attachment.find(20) assert a.disk_directory.blank? diff --git a/test/unit/lib/redmine/export/zip_archive_test.rb b/test/unit/lib/redmine/export/zip_archive_test.rb new file mode 100644 index 000000000..e948581d2 --- /dev/null +++ b/test/unit/lib/redmine/export/zip_archive_test.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +# Redmine - project management software +# Copyright (C) 2006- Jean-Philippe Lang +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +require_relative '../../../../test_helper' + +class ZipArchiveTest < ActiveSupport::TestCase + # Uses a server time zone other than UTC so that a local time converted + # to UTC can be detected + def setup + @original_tz = ENV['TZ'] + ENV['TZ'] = 'HST10' + end + + def teardown + ENV['TZ'] = @original_tz + end + + def test_add_file_should_rename_duplicate_names + entries = write_zip(User.find(2)) do |zip| + 3.times { zip.add_file('foo.txt', Time.now) {|sink| sink << 'foo'} } + end + + assert_equal ['foo.txt', 'foo(1).txt', 'foo(2).txt'], entries.map(&:name) + end + + def test_add_file_should_store_modification_time_in_user_time_zone + user = User.find(2) + user.pref.update!(:time_zone => 'Tokyo') + entries = write_zip(user) do |zip| + zip.add_file('foo.txt', Time.utc(2026, 1, 1, 18, 4, 6)) {|sink| sink << 'foo'} + end + + assert_equal [2026, 1, 2, 3, 4, 6], dos_time_components(entries.first) + assert_equal Time.utc(2026, 1, 1, 18, 4, 6), entries.first.extra[:universaltime].mtime.utc + end + + def test_add_file_should_store_modification_time_in_server_local_time_without_user_time_zone + user = User.find(2) + user.pref.update!(:time_zone => '') + entries = write_zip(user) do |zip| + zip.add_file('foo.txt', Time.utc(2026, 1, 1, 18, 4, 6)) {|sink| sink << 'foo'} + end + + # 18:04:06 UTC in the server time zone (HST, UTC-10) + assert_equal [2026, 1, 1, 8, 4, 6], dos_time_components(entries.first) + assert_equal Time.utc(2026, 1, 1, 18, 4, 6), entries.first.extra[:universaltime].mtime.utc + end + + private + + def write_zip(user) + buffer = StringIO.new(+''.b) + ZipKit::Streamer.open(buffer) do |streamer| + yield Redmine::Export::ZipArchive.new(streamer, user) + end + Zip::File.open_buffer(buffer.string).entries + end + + def dos_time_components(entry) + dos_time = entry.instance_variable_get(:@time) + [dos_time.year, dos_time.month, dos_time.day, dos_time.hour, dos_time.min, dos_time.sec] + end +end -- 2.55.0