From 5cc9b1eb6c1808a90e25c5847f1fe93f630e557e Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Mon, 27 Jul 2026 22:45:54 +0800 Subject: [PATCH 01/12] Implemented asynchronous S3-backed downloads for: - Work downloads - Challenge signup CSVs - Tag wrangler reports - Admin bulk-user-search CSVs The new flow creates a tokenized download record, queues `GeneratedDownloadJob`, redirects to a status page, uploads the completed file through Active Storage, and redirects the browser directly to the storage URL when ready. Downloads expire after 24 hours and an hourly cleanup removes expired files and records. CSV output is written incrementally to a tempfile in the worker, preserving the existing UTF-16LE Excel-compatible format. --- .../admin/admin_users_controller.rb | 11 +- .../challenge_signups_controller.rb | 7 +- app/controllers/downloads_controller.rb | 33 ++---- .../generated_downloads_controller.rb | 21 ++++ app/controllers/tag_wranglers_controller.rb | 17 +-- app/helpers/exports_helper.rb | 25 +++- app/jobs/generated_download_job.rb | 108 ++++++++++++++++++ app/models/generated_download.rb | 32 ++++++ app/views/generated_downloads/show.html.erb | 16 +++ config/resque_schedule.yml | 7 ++ config/routes.rb | 1 + ...260727000000_create_generated_downloads.rb | 17 +++ db/schema.rb | 16 ++- .../challenge_signups_controller_spec.rb | 6 +- .../generated_downloads_controller_spec.rb | 28 +++++ .../tag_wranglers_controller_spec.rb | 32 +++--- spec/models/generated_download_spec.rb | 27 +++++ 17 files changed, 337 insertions(+), 67 deletions(-) create mode 100644 app/controllers/generated_downloads_controller.rb create mode 100644 app/jobs/generated_download_job.rb create mode 100644 app/models/generated_download.rb create mode 100644 app/views/generated_downloads/show.html.erb create mode 100644 db/migrate/20260727000000_create_generated_downloads.rb create mode 100644 spec/controllers/generated_downloads_controller_spec.rb create mode 100644 spec/models/generated_download_spec.rb diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 0df19517da9..97fe3ddc21b 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -46,11 +46,12 @@ def bulk_search @users = found_users.paginate(page: params[:page] || 1) if params[:download_button] - header = [%w(Email Username)] - found = found_users.map { |u| [u.email, u.login] } - not_found = not_found_emails.map { |email| [email, ""] } - send_csv_data(header + found + not_found, "bulk_user_search_#{Time.now.strftime("%Y-%m-%d-%H%M")}.csv") - flash.now[:notice] = ts("Downloaded CSV") + queue_csv_download( + kind: "bulk_user_search", + arguments: { emails: @emails }, + filename: "bulk_user_search_#{Time.now.strftime("%Y-%m-%d-%H%M")}.csv" + ) + return end @results = { total: @emails.size, diff --git a/app/controllers/challenge_signups_controller.rb b/app/controllers/challenge_signups_controller.rb index e456e3ecdab..3c72b228c6e 100644 --- a/app/controllers/challenge_signups_controller.rb +++ b/app/controllers/challenge_signups_controller.rb @@ -119,9 +119,12 @@ def index if privileged_collection_admin? || (@collection.gift_exchange? && @challenge.user_allowed_to_see_signups?(current_user)) || (@collection.prompt_meme? && @collection.user_is_maintainer?(current_user)) - csv_data = self.send("#{@challenge.class.name.underscore}_to_csv") filename = "#{@collection.name}_signups_#{Time.now.strftime('%Y-%m-%d-%H%M')}.csv" - send_csv_data(csv_data, filename) + queue_csv_download( + kind: "challenge_signups", + arguments: { collection_id: @collection.id }, + filename: filename + ) else flash[:error] = ts("You aren't allowed to see the CSV summary.") redirect_to collection_path(@collection) rescue redirect_to '/' and return diff --git a/app/controllers/downloads_controller.rb b/app/controllers/downloads_controller.rb index 810b9f861c5..97d75a3a5a6 100644 --- a/app/controllers/downloads_controller.rb +++ b/app/controllers/downloads_controller.rb @@ -3,25 +3,16 @@ class DownloadsController < ApplicationController before_action :load_work, only: :show before_action :check_download_posted_status, only: :show before_action :check_download_visibility, only: :show - around_action :remove_downloads, only: :show - def show respond_to :html, :pdf, :mobi, :epub, :azw3 - @download = Download.new(@work, mime_type: request.format) - @download.generate - - # Make sure we were able to generate the download. - unless @download.exists? - flash[:error] = ts("We were not able to render this work. Please try again in a little while or try another format.") - redirect_to work_path(@work) - return - end - - # Send file synchronously so we don't delete it before we have finished - # sending it - File.open(@download.file_path, 'r') do |f| - send_data f.read, filename: "#{@download.file_name}.#{@download.file_type}", type: @download.mime_type - end + download = Download.new(@work, mime_type: request.format) + generated_download = GeneratedDownload.create!( + kind: "work", + arguments: { work_id: @work.id, format: download.file_type }, + filename: "#{download.file_name}.#{download.file_type}" + ) + GeneratedDownloadJob.perform_later(generated_download) + redirect_to generated_download_path(token: generated_download.token), status: :see_other end protected @@ -42,14 +33,6 @@ def load_work @work = Work.find(params[:id]) end - # We're currently just writing everything to tmp and feeding them through - # nginx so we don't want to keep the files around. - def remove_downloads - yield - ensure - @download.remove - end - # We can't use check_visibility because this controller doesn't have access to # cookies on production or staging. def check_download_visibility diff --git a/app/controllers/generated_downloads_controller.rb b/app/controllers/generated_downloads_controller.rb new file mode 100644 index 00000000000..0a486bc1b29 --- /dev/null +++ b/app/controllers/generated_downloads_controller.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class GeneratedDownloadsController < ApplicationController + def show + @generated_download = GeneratedDownload.find_by!(token: params[:token]) + + if @generated_download.expired? + @generated_download.file.purge_later if @generated_download.file.attached? + head :gone + elsif @generated_download.status == "ready" && @generated_download.file.attached? + redirect_to @generated_download.file.blob.url( + disposition: :attachment, + filename: @generated_download.filename + ), allow_other_host: true + elsif @generated_download.status == "failed" + render :show, status: :unprocessable_content + else + render :show, status: :accepted + end + end +end diff --git a/app/controllers/tag_wranglers_controller.rb b/app/controllers/tag_wranglers_controller.rb index f3359580a32..b923da1ae1d 100644 --- a/app/controllers/tag_wranglers_controller.rb +++ b/app/controllers/tag_wranglers_controller.rb @@ -45,19 +45,12 @@ def report_csv authorize :wrangling wrangler = User.find_by!(login: params[:id]) - wrangled_tags = Tag - .where(last_wrangler: wrangler) - .limit(ArchiveConfig.WRANGLING_REPORT_LIMIT) - .includes(:merger, :parents) - results = [%w[Name Last\ Updated Type Merger Fandoms Unwrangleable]] - wrangled_tags.find_each(order: :desc) do |tag| - merger = tag.merger&.name || "" - fandoms = tag.parents.filter_map { |parent| parent.name if parent.is_a?(Fandom) } - .join(", ") - results << [tag.name, tag.updated_at, tag.type, merger, fandoms, tag.unwrangleable] - end filename = "wrangled_tags_#{wrangler.login}_#{Time.now.utc.strftime('%Y-%m-%d-%H%M')}.csv" - send_csv_data(results, filename) + queue_csv_download( + kind: "tag_wrangler", + arguments: { user_id: wrangler.id }, + filename: filename + ) end def create diff --git a/app/helpers/exports_helper.rb b/app/helpers/exports_helper.rb index 08131f1410c..774be622b31 100644 --- a/app/helpers/exports_helper.rb +++ b/app/helpers/exports_helper.rb @@ -1,8 +1,14 @@ # frozen_string_literal: true module ExportsHelper - def send_csv_data(content_array, filename) - send_data(export_csv(content_array), filename: filename, type: :csv) + def queue_csv_download(kind:, arguments:, filename:) + generated_download = GeneratedDownload.create!( + kind: kind, + arguments: arguments, + filename: filename + ) + GeneratedDownloadJob.perform_later(generated_download) + redirect_to generated_download_path(token: generated_download.token), status: :see_other end # Tab-separated CSV with utf-16le encoding (unicode) and byte order @@ -10,8 +16,17 @@ def send_csv_data(content_array, filename) # automatically into proper table format. OpenOffice handles it # well, too. def export_csv(content_array) - csv_data = content_array.map { |x| x.to_csv(col_sep: "\t", encoding: "utf-8") }.join - byte_order_mark = "\uFEFF" - (byte_order_mark + csv_data).encode("utf-16le", "utf-8", invalid: :replace, undef: :replace, replace: "") + io = StringIO.new("".b) + ExportsHelper.write_csv(io, content_array) + io.string + end + + def self.write_csv(io, rows) + io.write("\uFEFF".encode("utf-16le")) + rows.each do |row| + io.write(row.to_csv(col_sep: "\t", encoding: "utf-8").encode( + "utf-16le", "utf-8", invalid: :replace, undef: :replace, replace: "" + )) + end end end diff --git a/app/jobs/generated_download_job.rb b/app/jobs/generated_download_job.rb new file mode 100644 index 00000000000..71378b3bcb7 --- /dev/null +++ b/app/jobs/generated_download_job.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +class GeneratedDownloadJob < ApplicationJob + queue_as :utilities + + def perform(generated_download) + return if generated_download.expired? + + generated_download.update!(status: "processing", error: nil) + + case generated_download.kind + when "work" + attach_work(generated_download) + when "challenge_signups", "tag_wrangler", "bulk_user_search" + attach_csv(generated_download) + else + raise ArgumentError, "Unknown generated download kind: #{generated_download.kind}" + end + + generated_download.update!(status: "ready") + rescue StandardError => error + generated_download.update_columns(status: "failed", error: error.message, updated_at: Time.current) + raise + end + + private + + def attach_work(generated_download) + arguments = generated_download.arguments + download = Download.new( + Work.find(arguments.fetch("work_id")), + format: arguments.fetch("format") + ).generate + raise "Download generation failed" unless download.exists? + + File.open(download.file_path, "rb") do |file| + generated_download.file.attach( + io: file, + filename: generated_download.filename, + content_type: download.mime_type + ) + end + ensure + download&.remove + end + + def attach_csv(generated_download) + tempfile = Tempfile.new(["generated-download", ".csv"]) + tempfile.binmode + ExportsHelper.write_csv(tempfile, csv_rows(generated_download)) + tempfile.rewind + generated_download.file.attach( + io: tempfile, + filename: generated_download.filename, + content_type: "text/csv" + ) + ensure + tempfile&.close! + end + + def csv_rows(generated_download) + arguments = generated_download.arguments + case generated_download.kind + when "tag_wrangler" + tag_wrangler_rows(arguments.fetch("user_id")) + when "bulk_user_search" + bulk_user_search_rows(arguments.fetch("emails")) + when "challenge_signups" + challenge_signup_rows(arguments.fetch("collection_id")) + end + end + + def tag_wrangler_rows(user_id) + wrangler = User.find(user_id) + rows = [%w[Name Last\ Updated Type Merger Fandoms Unwrangleable]] + Tag.where(last_wrangler: wrangler) + .limit(ArchiveConfig.WRANGLING_REPORT_LIMIT) + .includes(:merger, :parents) + .find_each(order: :desc) do |tag| + fandoms = tag.parents.filter_map { |parent| parent.name if parent.is_a?(Fandom) }.join(", ") + rows << [tag.name, tag.updated_at, tag.type, tag.merger&.name || "", fandoms, tag.unwrangleable] + end + rows + end + + def bulk_user_search_rows(emails) + found_users, not_found_emails = User.search_multiple_by_email(emails) + [%w[Email Username]] + + found_users.map { |user| [user.email, user.login] } + + not_found_emails.map { |email| [email, ""] } + end + + def challenge_signup_rows(collection_id) + collection = Collection.find(collection_id) + controller = ChallengeSignupsController.new + controller.instance_variable_set(:@collection, collection) + controller.instance_variable_set(:@challenge, collection.challenge) + controller.define_singleton_method(:collection_signup_url) do |requested_collection, signup| + Rails.application.routes.url_helpers.collection_signup_url( + requested_collection, + signup, + host: ArchiveConfig.APP_HOST, + protocol: "https" + ) + end + controller.send("#{collection.challenge.class.name.underscore}_to_csv") + end +end diff --git a/app/models/generated_download.rb b/app/models/generated_download.rb new file mode 100644 index 00000000000..f4e533f25c2 --- /dev/null +++ b/app/models/generated_download.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class GeneratedDownload < ApplicationRecord + EXPIRATION = 1.day + STATUSES = %w[pending processing ready failed].freeze + + has_one_attached :file + + validates :token, :kind, :filename, :expires_at, presence: true + validates :token, uniqueness: true + validates :status, inclusion: { in: STATUSES } + + before_validation :set_defaults, on: :create + + def self.cleanup + where(expires_at: ...Time.current).find_each do |download| + download.file.purge if download.file.attached? + download.destroy! + end + end + + def expired? + expires_at.past? + end + + private + + def set_defaults + self.token ||= SecureRandom.urlsafe_base64(32) + self.expires_at ||= EXPIRATION.from_now + end +end diff --git a/app/views/generated_downloads/show.html.erb b/app/views/generated_downloads/show.html.erb new file mode 100644 index 00000000000..efa5ad9e954 --- /dev/null +++ b/app/views/generated_downloads/show.html.erb @@ -0,0 +1,16 @@ +<% content_for :head do %> + <% unless @generated_download.status == "failed" %> + + <% end %> +<% end %> + +

<%= ts("Preparing download") %>

+ +<% if @generated_download.status == "failed" %> +

<%= ts("We were not able to prepare this download. Please try again.") %>

+<% else %> +

+ <%= ts("Your file is being prepared. This page will download it automatically when it is ready.") %> +

+

<%= link_to ts("Check download status"), generated_download_path(token: @generated_download.token) %>

+<% end %> diff --git a/config/resque_schedule.yml b/config/resque_schedule.yml index 988be768cfb..c1a6974cb70 100644 --- a/config/resque_schedule.yml +++ b/config/resque_schedule.yml @@ -5,6 +5,13 @@ run_main_reindex_queues: args: main description: "Kick off a reindex of all main content indexing" +cleanup_generated_downloads: + every: 1h + class: "GeneratedDownload" + queue: utilities + args: cleanup + description: "Remove expired generated downloads from storage and the database." + run_background_reindex_queue: every: 11m class: "ScheduledReindexJob" diff --git a/config/routes.rb b/config/routes.rb index 0d145b1826c..79de9f64339 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -59,6 +59,7 @@ #### DOWNLOADS #### get 'downloads/:id/:download_title.:format' => 'downloads#show', as: 'download' + get 'generated_downloads/:token' => 'generated_downloads#show', as: 'generated_download' #### OPEN DOORS #### namespace :opendoors do diff --git a/db/migrate/20260727000000_create_generated_downloads.rb b/db/migrate/20260727000000_create_generated_downloads.rb new file mode 100644 index 00000000000..3b28b9eed71 --- /dev/null +++ b/db/migrate/20260727000000_create_generated_downloads.rb @@ -0,0 +1,17 @@ +class CreateGeneratedDownloads < ActiveRecord::Migration[8.0] + def change + create_table :generated_downloads do |t| + t.string :token, null: false + t.string :kind, null: false + t.json :arguments, null: false + t.string :filename, null: false + t.string :status, null: false, default: "pending" + t.text :error + t.datetime :expires_at, null: false + t.timestamps + end + + add_index :generated_downloads, :token, unique: true + add_index :generated_downloads, :expires_at + end +end diff --git a/db/schema.rb b/db/schema.rb index 0cf5dc2ec51..5a1a10515f5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_02_07_100830) do +ActiveRecord::Schema[8.0].define(version: 2026_07_27_000000) do create_table "abuse_reports", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", options: "ENGINE=InnoDB ROW_FORMAT=DYNAMIC", force: :cascade do |t| t.string "email" t.string "url", limit: 2080, null: false @@ -576,6 +576,20 @@ t.boolean "requests_summary_visible", default: false, null: false end + create_table "generated_downloads", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t| + t.string "token", null: false + t.string "kind", null: false + t.json "arguments", null: false + t.string "filename", null: false + t.string "status", default: "pending", null: false + t.text "error" + t.datetime "expires_at", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["expires_at"], name: "index_generated_downloads_on_expires_at" + t.index ["token"], name: "index_generated_downloads_on_token", unique: true + end + create_table "gifts", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", options: "ENGINE=InnoDB ROW_FORMAT=DYNAMIC", force: :cascade do |t| t.integer "work_id" t.string "recipient_name" diff --git a/spec/controllers/challenge_signups_controller_spec.rb b/spec/controllers/challenge_signups_controller_spec.rb index 4f269dde817..9535ca570af 100644 --- a/spec/controllers/challenge_signups_controller_spec.rb +++ b/spec/controllers/challenge_signups_controller_spec.rb @@ -308,11 +308,13 @@ it "allows support admins to download CSV" do fake_login_admin(create(:support_admin)) + allow(GeneratedDownloadJob).to receive(:perform_later) get :index, params: { collection_id: closed_collection.name, format: :csv } - expect(response).to have_http_status(:success) - expect(response.content_type).to include("text/csv") + download = GeneratedDownload.order(:created_at).last + expect(response).to redirect_to(generated_download_path(token: download.token)) + expect(GeneratedDownloadJob).to have_received(:perform_later).with(download) end end end diff --git a/spec/controllers/generated_downloads_controller_spec.rb b/spec/controllers/generated_downloads_controller_spec.rb new file mode 100644 index 00000000000..21bdd396eca --- /dev/null +++ b/spec/controllers/generated_downloads_controller_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe GeneratedDownloadsController do + describe "GET #show" do + it "returns accepted while the file is being generated" do + download = GeneratedDownload.create!(kind: "test", arguments: {}, filename: "test.csv") + + get :show, params: { token: download.token } + + expect(response).to have_http_status(:accepted) + end + + it "returns gone after the download expires" do + download = GeneratedDownload.create!( + kind: "test", + arguments: {}, + filename: "test.csv", + expires_at: 1.minute.ago + ) + + get :show, params: { token: download.token } + + expect(response).to have_http_status(:gone) + end + end +end diff --git a/spec/controllers/tag_wranglers_controller_spec.rb b/spec/controllers/tag_wranglers_controller_spec.rb index 4b4a58594ef..d440c0604a2 100644 --- a/spec/controllers/tag_wranglers_controller_spec.rb +++ b/spec/controllers/tag_wranglers_controller_spec.rb @@ -129,13 +129,22 @@ context "when logged in as an admin with proper authorization" do before { fake_login_admin(admin) } + def download_report(user) + get :report_csv, params: { id: user.login } + csv = GeneratedDownload.order(:created_at).last.file.download + .force_encoding("utf-16le").encode("utf-8") + CSV.parse(csv[1..], col_sep: "\t") + end + wrangling_roles.each do |admin_role| context "with role #{admin_role}" do let(:admin) { create(:admin, roles: [admin_role]) } it "allows access to the report" do get :report_csv, params: { id: user.login } - expect(response).to have_http_status(:success) + expect(response).to redirect_to( + generated_download_path(token: GeneratedDownload.order(:created_at).last.token) + ) end it "only includes wrangling activity for the specified user" do @@ -143,8 +152,7 @@ tag1 = create(:tag, last_wrangler: user) create(:tag, last_wrangler: other_user) - get :report_csv, params: { id: user.login } - result = CSV.parse(response.body.encode("utf-8")[1..], col_sep: "\t") + result = download_report(user) expect(result) .to eq([["Name", "Last Updated", "Type", "Merger", "Fandoms", "Unwrangleable"], @@ -157,8 +165,7 @@ create(:tag, last_wrangler: user) tag2 = create(:tag, last_wrangler: user) - get :report_csv, params: { id: user.login } - result = CSV.parse(response.body.encode("utf-8")[1..], col_sep: "\t") + result = download_report(user) expect(result.length).to eq(2) expect(result[1][0]).to eq(tag2.name) @@ -168,8 +175,7 @@ tag1 = create(:tag, last_wrangler: user) tag2 = create(:tag, last_wrangler: user, merger: tag1) - get :report_csv, params: { id: user.login } - result = CSV.parse(response.body.encode("utf-8")[1..], col_sep: "\t") + result = download_report(user) expect(result) .to eq([["Name", "Last Updated", "Type", "Merger", "Fandoms", "Unwrangleable"], @@ -182,8 +188,7 @@ tag = create(:freeform, last_wrangler: user) expect(fandom.add_association(tag)).to be_truthy - get :report_csv, params: { id: user.login } - result = CSV.parse(response.body.encode("utf-8")[1..], col_sep: "\t") + result = download_report(user) expect(result) .to eq([["Name", "Last Updated", "Type", "Merger", "Fandoms", "Unwrangleable"], @@ -197,8 +202,7 @@ expect(fandom1.add_association(tag)).to be_truthy expect(fandom2.add_association(tag)).to be_truthy - get :report_csv, params: { id: user.login } - result = CSV.parse(response.body.encode("utf-8")[1..], col_sep: "\t") + result = download_report(user) expect(result) .to eq([["Name", "Last Updated", "Type", "Merger", "Fandoms", "Unwrangleable"], @@ -210,8 +214,7 @@ media = create(:media, last_wrangler: user) expect(media.add_association(fandom)).to be_truthy - get :report_csv, params: { id: user.login } - result = CSV.parse(response.body.encode("utf-8")[1..], col_sep: "\t") + result = download_report(user) expect(result[1][4]).to be_empty end @@ -219,8 +222,7 @@ it "correctly reports a tag marked unwrangleable" do tag = create(:tag, last_wrangler: user, unwrangleable: true) - get :report_csv, params: { id: user.login } - result = CSV.parse(response.body.encode("utf-8")[1..], col_sep: "\t") + result = download_report(user) expect(result) .to eq([["Name", "Last Updated", "Type", "Merger", "Fandoms", "Unwrangleable"], diff --git a/spec/models/generated_download_spec.rb b/spec/models/generated_download_spec.rb new file mode 100644 index 00000000000..a56cce6f37c --- /dev/null +++ b/spec/models/generated_download_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe GeneratedDownload do + it "sets a token and expiration" do + download = GeneratedDownload.create!(kind: "test", arguments: {}, filename: "test.csv") + + expect(download.token).to be_present + expect(download.expires_at).to be_within(1.second).of(GeneratedDownload::EXPIRATION.from_now) + end + + describe ".cleanup" do + it "removes expired records" do + expired = GeneratedDownload.create!( + kind: "test", + arguments: {}, + filename: "test.csv", + expires_at: 1.minute.ago + ) + + GeneratedDownload.cleanup + + expect(GeneratedDownload.exists?(expired.id)).to be(false) + end + end +end From edd50752c33e5ec94fcd712693b593c78a2a0762 Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 00:04:58 +0800 Subject: [PATCH 02/12] fix: download arguments use a text column with explicit JSON serialization; added regression test for non-empty arguments --- app/models/generated_download.rb | 2 ++ ...0000_change_generated_download_arguments_to_text.rb | 9 +++++++++ db/schema.rb | 4 ++-- spec/models/generated_download_spec.rb | 10 ++++++++++ 4 files changed, 23 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20260728000000_change_generated_download_arguments_to_text.rb diff --git a/app/models/generated_download.rb b/app/models/generated_download.rb index f4e533f25c2..c94f821d7fb 100644 --- a/app/models/generated_download.rb +++ b/app/models/generated_download.rb @@ -6,6 +6,8 @@ class GeneratedDownload < ApplicationRecord has_one_attached :file + serialize :arguments, coder: JSON + validates :token, :kind, :filename, :expires_at, presence: true validates :token, uniqueness: true validates :status, inclusion: { in: STATUSES } diff --git a/db/migrate/20260728000000_change_generated_download_arguments_to_text.rb b/db/migrate/20260728000000_change_generated_download_arguments_to_text.rb new file mode 100644 index 00000000000..f9dc4489086 --- /dev/null +++ b/db/migrate/20260728000000_change_generated_download_arguments_to_text.rb @@ -0,0 +1,9 @@ +class ChangeGeneratedDownloadArgumentsToText < ActiveRecord::Migration[8.0] + def up + change_column :generated_downloads, :arguments, :text, null: false + end + + def down + change_column :generated_downloads, :arguments, :json, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 5a1a10515f5..debb0c792d1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_07_27_000000) do +ActiveRecord::Schema[8.0].define(version: 2026_07_28_000000) do create_table "abuse_reports", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", options: "ENGINE=InnoDB ROW_FORMAT=DYNAMIC", force: :cascade do |t| t.string "email" t.string "url", limit: 2080, null: false @@ -579,7 +579,7 @@ create_table "generated_downloads", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t| t.string "token", null: false t.string "kind", null: false - t.json "arguments", null: false + t.text "arguments", null: false t.string "filename", null: false t.string "status", default: "pending", null: false t.text "error" diff --git a/spec/models/generated_download_spec.rb b/spec/models/generated_download_spec.rb index a56cce6f37c..106e1ba0dc2 100644 --- a/spec/models/generated_download_spec.rb +++ b/spec/models/generated_download_spec.rb @@ -10,6 +10,16 @@ expect(download.expires_at).to be_within(1.second).of(GeneratedDownload::EXPIRATION.from_now) end + it "round trips non-empty job arguments" do + download = GeneratedDownload.create!( + kind: "test", + arguments: { user_id: 42 }, + filename: "test.csv" + ) + + expect(download.reload.arguments).to eq("user_id" => 42) + end + describe ".cleanup" do it "removes expired records" do expired = GeneratedDownload.create!( From d4ffd42bd4f6c3837de124b439c9e2a8c8878203 Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 00:09:28 +0800 Subject: [PATCH 03/12] AO3-7461 Clean up generated download implementation --- app/controllers/admin/admin_users_controller.rb | 2 +- app/controllers/challenge_signups_controller.rb | 2 +- app/helpers/exports_helper.rb | 7 ++++--- app/jobs/generated_download_job.rb | 10 ++++++---- .../20260727000000_create_generated_downloads.rb | 2 +- ...0000_change_generated_download_arguments_to_text.rb | 9 --------- db/schema.rb | 2 +- 7 files changed, 14 insertions(+), 20 deletions(-) delete mode 100644 db/migrate/20260728000000_change_generated_download_arguments_to_text.rb diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 97fe3ddc21b..e026d75491b 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -49,7 +49,7 @@ def bulk_search queue_csv_download( kind: "bulk_user_search", arguments: { emails: @emails }, - filename: "bulk_user_search_#{Time.now.strftime("%Y-%m-%d-%H%M")}.csv" + filename: "bulk_user_search_#{Time.current.strftime('%Y-%m-%d-%H%M')}.csv" ) return end diff --git a/app/controllers/challenge_signups_controller.rb b/app/controllers/challenge_signups_controller.rb index 3c72b228c6e..7f1921cfd00 100644 --- a/app/controllers/challenge_signups_controller.rb +++ b/app/controllers/challenge_signups_controller.rb @@ -119,7 +119,7 @@ def index if privileged_collection_admin? || (@collection.gift_exchange? && @challenge.user_allowed_to_see_signups?(current_user)) || (@collection.prompt_meme? && @collection.user_is_maintainer?(current_user)) - filename = "#{@collection.name}_signups_#{Time.now.strftime('%Y-%m-%d-%H%M')}.csv" + filename = "#{@collection.name}_signups_#{Time.current.strftime('%Y-%m-%d-%H%M')}.csv" queue_csv_download( kind: "challenge_signups", arguments: { collection_id: @collection.id }, diff --git a/app/helpers/exports_helper.rb b/app/helpers/exports_helper.rb index 774be622b31..c937a5a7cd5 100644 --- a/app/helpers/exports_helper.rb +++ b/app/helpers/exports_helper.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -module ExportsHelper +module ExportsHelper def queue_csv_download(kind:, arguments:, filename:) generated_download = GeneratedDownload.create!( kind: kind, @@ -24,9 +24,10 @@ def export_csv(content_array) def self.write_csv(io, rows) io.write("\uFEFF".encode("utf-16le")) rows.each do |row| - io.write(row.to_csv(col_sep: "\t", encoding: "utf-8").encode( + encoded_row = row.to_csv(col_sep: "\t", encoding: "utf-8").encode( "utf-16le", "utf-8", invalid: :replace, undef: :replace, replace: "" - )) + ) + io.write(encoded_row) end end end diff --git a/app/jobs/generated_download_job.rb b/app/jobs/generated_download_job.rb index 71378b3bcb7..6e2f04a6448 100644 --- a/app/jobs/generated_download_job.rb +++ b/app/jobs/generated_download_job.rb @@ -18,8 +18,8 @@ def perform(generated_download) end generated_download.update!(status: "ready") - rescue StandardError => error - generated_download.update_columns(status: "failed", error: error.message, updated_at: Time.current) + rescue StandardError => e + generated_download.update_columns(status: "failed", error: e.message, updated_at: Time.current) raise end @@ -72,12 +72,14 @@ def csv_rows(generated_download) def tag_wrangler_rows(user_id) wrangler = User.find(user_id) - rows = [%w[Name Last\ Updated Type Merger Fandoms Unwrangleable]] + rows = [["Name", "Last Updated", "Type", "Merger", "Fandoms", "Unwrangleable"]] Tag.where(last_wrangler: wrangler) .limit(ArchiveConfig.WRANGLING_REPORT_LIMIT) .includes(:merger, :parents) .find_each(order: :desc) do |tag| - fandoms = tag.parents.filter_map { |parent| parent.name if parent.is_a?(Fandom) }.join(", ") + fandoms = tag.parents + .filter_map { |parent| parent.name if parent.is_a?(Fandom) } + .join(", ") rows << [tag.name, tag.updated_at, tag.type, tag.merger&.name || "", fandoms, tag.unwrangleable] end rows diff --git a/db/migrate/20260727000000_create_generated_downloads.rb b/db/migrate/20260727000000_create_generated_downloads.rb index 3b28b9eed71..0d7d98b94e2 100644 --- a/db/migrate/20260727000000_create_generated_downloads.rb +++ b/db/migrate/20260727000000_create_generated_downloads.rb @@ -3,7 +3,7 @@ def change create_table :generated_downloads do |t| t.string :token, null: false t.string :kind, null: false - t.json :arguments, null: false + t.text :arguments, null: false t.string :filename, null: false t.string :status, null: false, default: "pending" t.text :error diff --git a/db/migrate/20260728000000_change_generated_download_arguments_to_text.rb b/db/migrate/20260728000000_change_generated_download_arguments_to_text.rb deleted file mode 100644 index f9dc4489086..00000000000 --- a/db/migrate/20260728000000_change_generated_download_arguments_to_text.rb +++ /dev/null @@ -1,9 +0,0 @@ -class ChangeGeneratedDownloadArgumentsToText < ActiveRecord::Migration[8.0] - def up - change_column :generated_downloads, :arguments, :text, null: false - end - - def down - change_column :generated_downloads, :arguments, :json, null: false - end -end diff --git a/db/schema.rb b/db/schema.rb index debb0c792d1..0d36a390ce7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_07_28_000000) do +ActiveRecord::Schema[8.0].define(version: 2026_07_27_000000) do create_table "abuse_reports", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", options: "ENGINE=InnoDB ROW_FORMAT=DYNAMIC", force: :cascade do |t| t.string "email" t.string "url", limit: 2080, null: false From 6cc60afdf4e7b5f312cbb8fe1e509ec4555c93de Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 00:33:55 +0800 Subject: [PATCH 04/12] AO3-7461 Fix CI checks for generated downloads --- .../generated_downloads_controller.rb | 10 +++++---- app/jobs/generated_download_job.rb | 3 ++- app/views/generated_downloads/show.html.erb | 10 ++++----- config/locales/views/en.yml | 6 ++++++ config/routes.rb | 2 +- db/schema.rb | 16 +------------- .../generated_downloads_controller_spec.rb | 21 +++++++++++++++++++ 7 files changed, 42 insertions(+), 26 deletions(-) diff --git a/app/controllers/generated_downloads_controller.rb b/app/controllers/generated_downloads_controller.rb index 0a486bc1b29..4abbdd762e0 100644 --- a/app/controllers/generated_downloads_controller.rb +++ b/app/controllers/generated_downloads_controller.rb @@ -8,10 +8,12 @@ def show @generated_download.file.purge_later if @generated_download.file.attached? head :gone elsif @generated_download.status == "ready" && @generated_download.file.attached? - redirect_to @generated_download.file.blob.url( - disposition: :attachment, - filename: @generated_download.filename - ), allow_other_host: true + blob = @generated_download.file.blob + redirect_to rails_storage_redirect_path( + blob.signed_id, + blob.filename, + disposition: :attachment + ) elsif @generated_download.status == "failed" render :show, status: :unprocessable_content else diff --git a/app/jobs/generated_download_job.rb b/app/jobs/generated_download_job.rb index 6e2f04a6448..043db6654e3 100644 --- a/app/jobs/generated_download_job.rb +++ b/app/jobs/generated_download_job.rb @@ -33,7 +33,8 @@ def attach_work(generated_download) ).generate raise "Download generation failed" unless download.exists? - File.open(download.file_path, "rb") do |file| + file_path = File.join(download.dir, File.basename(download.file_path)) + File.open(file_path, "rb") do |file| generated_download.file.attach( io: file, filename: generated_download.filename, diff --git a/app/views/generated_downloads/show.html.erb b/app/views/generated_downloads/show.html.erb index efa5ad9e954..2ff988c39e9 100644 --- a/app/views/generated_downloads/show.html.erb +++ b/app/views/generated_downloads/show.html.erb @@ -1,16 +1,16 @@ <% content_for :head do %> <% unless @generated_download.status == "failed" %> - + <% end %> <% end %> -

<%= ts("Preparing download") %>

+

<%= t(".heading") %>

<% if @generated_download.status == "failed" %> -

<%= ts("We were not able to prepare this download. Please try again.") %>

+

<%= t(".failed") %>

<% else %>

- <%= ts("Your file is being prepared. This page will download it automatically when it is ready.") %> + <%= t(".processing") %>

-

<%= link_to ts("Check download status"), generated_download_path(token: @generated_download.token) %>

+

<%= link_to t(".check_status"), generated_download_path(token: @generated_download.token) %>

<% end %> diff --git a/config/locales/views/en.yml b/config/locales/views/en.yml index 03b5b0cf4cb..673d90b34c0 100644 --- a/config/locales/views/en.yml +++ b/config/locales/views/en.yml @@ -1207,6 +1207,12 @@ en: current_html: For current updates on AO3 performance or downtime, please check our %{status_page_link} and follow %{bluesky_link} on Bluesky or %{tumblr_link} on Tumblr. status_page: status page tumblr: ao3org + generated_downloads: + show: + check_status: Check download status + failed: We were not able to prepare this download. Please try again. + heading: Preparing download + processing: Your file is being prepared. This page will download it automatically when it is ready. help: collectibles_add_to_collection: collection_names: Note that you need to use the collection's name, which gets used in the collection's URL, and not the collection's spiffy title (because different collections can have the same title). A collection's name is the equivalent of your user login. Names will be auto-completed for you if you have JavaScript turned on. diff --git a/config/routes.rb b/config/routes.rb index 79de9f64339..0f41f5fcc69 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -59,7 +59,7 @@ #### DOWNLOADS #### get 'downloads/:id/:download_title.:format' => 'downloads#show', as: 'download' - get 'generated_downloads/:token' => 'generated_downloads#show', as: 'generated_download' + get "generated_downloads/:token" => "generated_downloads#show", as: "generated_download" #### OPEN DOORS #### namespace :opendoors do diff --git a/db/schema.rb b/db/schema.rb index 0d36a390ce7..0cf5dc2ec51 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_07_27_000000) do +ActiveRecord::Schema[8.0].define(version: 2026_02_07_100830) do create_table "abuse_reports", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", options: "ENGINE=InnoDB ROW_FORMAT=DYNAMIC", force: :cascade do |t| t.string "email" t.string "url", limit: 2080, null: false @@ -576,20 +576,6 @@ t.boolean "requests_summary_visible", default: false, null: false end - create_table "generated_downloads", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t| - t.string "token", null: false - t.string "kind", null: false - t.text "arguments", null: false - t.string "filename", null: false - t.string "status", default: "pending", null: false - t.text "error" - t.datetime "expires_at", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["expires_at"], name: "index_generated_downloads_on_expires_at" - t.index ["token"], name: "index_generated_downloads_on_token", unique: true - end - create_table "gifts", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", options: "ENGINE=InnoDB ROW_FORMAT=DYNAMIC", force: :cascade do |t| t.integer "work_id" t.string "recipient_name" diff --git a/spec/controllers/generated_downloads_controller_spec.rb b/spec/controllers/generated_downloads_controller_spec.rb index 21bdd396eca..743984715e0 100644 --- a/spec/controllers/generated_downloads_controller_spec.rb +++ b/spec/controllers/generated_downloads_controller_spec.rb @@ -24,5 +24,26 @@ expect(response).to have_http_status(:gone) end + + it "redirects ready downloads through the Active Storage redirect endpoint" do + download = GeneratedDownload.create!( + kind: "test", + arguments: {}, + filename: "test.csv", + status: "ready" + ) + download.file.attach( + io: StringIO.new("contents"), + filename: download.filename, + content_type: "text/csv" + ) + blob = download.file.blob + + get :show, params: { token: download.token } + + expect(response).to redirect_to( + rails_storage_redirect_path(blob.signed_id, blob.filename, disposition: :attachment) + ) + end end end From 1d8ee6ebce47b770787d5e5f2885f81045629d6b Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 00:42:27 +0800 Subject: [PATCH 05/12] AO3-7461 Convert Active Storage filename for redirect --- app/controllers/generated_downloads_controller.rb | 2 +- spec/controllers/generated_downloads_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/generated_downloads_controller.rb b/app/controllers/generated_downloads_controller.rb index 4abbdd762e0..89f30b5a52d 100644 --- a/app/controllers/generated_downloads_controller.rb +++ b/app/controllers/generated_downloads_controller.rb @@ -11,7 +11,7 @@ def show blob = @generated_download.file.blob redirect_to rails_storage_redirect_path( blob.signed_id, - blob.filename, + blob.filename.to_s, disposition: :attachment ) elsif @generated_download.status == "failed" diff --git a/spec/controllers/generated_downloads_controller_spec.rb b/spec/controllers/generated_downloads_controller_spec.rb index 743984715e0..cb2998be090 100644 --- a/spec/controllers/generated_downloads_controller_spec.rb +++ b/spec/controllers/generated_downloads_controller_spec.rb @@ -42,7 +42,7 @@ get :show, params: { token: download.token } expect(response).to redirect_to( - rails_storage_redirect_path(blob.signed_id, blob.filename, disposition: :attachment) + rails_storage_redirect_path(blob.signed_id, blob.filename.to_s, disposition: :attachment) ) end end From 8ae0859f5e628157b026b4ed06b1255a77824c56 Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 00:58:20 +0800 Subject: [PATCH 06/12] AO3-7461 Use string disposition for storage redirect --- app/controllers/generated_downloads_controller.rb | 2 +- spec/controllers/generated_downloads_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/generated_downloads_controller.rb b/app/controllers/generated_downloads_controller.rb index 89f30b5a52d..cf302647b74 100644 --- a/app/controllers/generated_downloads_controller.rb +++ b/app/controllers/generated_downloads_controller.rb @@ -12,7 +12,7 @@ def show redirect_to rails_storage_redirect_path( blob.signed_id, blob.filename.to_s, - disposition: :attachment + disposition: "attachment" ) elsif @generated_download.status == "failed" render :show, status: :unprocessable_content diff --git a/spec/controllers/generated_downloads_controller_spec.rb b/spec/controllers/generated_downloads_controller_spec.rb index cb2998be090..d0b13aebebf 100644 --- a/spec/controllers/generated_downloads_controller_spec.rb +++ b/spec/controllers/generated_downloads_controller_spec.rb @@ -42,7 +42,7 @@ get :show, params: { token: download.token } expect(response).to redirect_to( - rails_storage_redirect_path(blob.signed_id, blob.filename.to_s, disposition: :attachment) + rails_storage_redirect_path(blob.signed_id, blob.filename.to_s, disposition: "attachment") ) end end From 5b92d084a81c8578125cc7aafc1836a146cba2fe Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 00:59:26 +0800 Subject: [PATCH 07/12] AO3-7461 Preserve exported CSV encoding --- app/helpers/exports_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/exports_helper.rb b/app/helpers/exports_helper.rb index c937a5a7cd5..87df6672ce3 100644 --- a/app/helpers/exports_helper.rb +++ b/app/helpers/exports_helper.rb @@ -18,7 +18,7 @@ def queue_csv_download(kind:, arguments:, filename:) def export_csv(content_array) io = StringIO.new("".b) ExportsHelper.write_csv(io, content_array) - io.string + io.string.force_encoding("utf-16le") end def self.write_csv(io, rows) From cd6305633d7c487b7ca4929cae5d9c8cf025e0ef Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 01:07:13 +0800 Subject: [PATCH 08/12] AO3-7461 Use Active Storage redirect helper correctly --- app/controllers/generated_downloads_controller.rb | 3 +-- spec/controllers/generated_downloads_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/controllers/generated_downloads_controller.rb b/app/controllers/generated_downloads_controller.rb index cf302647b74..d29eb66014f 100644 --- a/app/controllers/generated_downloads_controller.rb +++ b/app/controllers/generated_downloads_controller.rb @@ -10,8 +10,7 @@ def show elsif @generated_download.status == "ready" && @generated_download.file.attached? blob = @generated_download.file.blob redirect_to rails_storage_redirect_path( - blob.signed_id, - blob.filename.to_s, + blob, disposition: "attachment" ) elsif @generated_download.status == "failed" diff --git a/spec/controllers/generated_downloads_controller_spec.rb b/spec/controllers/generated_downloads_controller_spec.rb index d0b13aebebf..32a292c0960 100644 --- a/spec/controllers/generated_downloads_controller_spec.rb +++ b/spec/controllers/generated_downloads_controller_spec.rb @@ -42,7 +42,7 @@ get :show, params: { token: download.token } expect(response).to redirect_to( - rails_storage_redirect_path(blob.signed_id, blob.filename.to_s, disposition: "attachment") + rails_storage_redirect_path(blob, disposition: "attachment") ) end end From 7cdf6984c4ce68fe826dc707b5fda701959d4317 Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 01:15:11 +0800 Subject: [PATCH 09/12] AO3-7461 Decode stored CSV downloads as UTF-16LE --- app/jobs/generated_download_job.rb | 2 +- features/step_definitions/web_steps.rb | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/jobs/generated_download_job.rb b/app/jobs/generated_download_job.rb index 043db6654e3..5ac29a904a2 100644 --- a/app/jobs/generated_download_job.rb +++ b/app/jobs/generated_download_job.rb @@ -53,7 +53,7 @@ def attach_csv(generated_download) generated_download.file.attach( io: tempfile, filename: generated_download.filename, - content_type: "text/csv" + content_type: "text/csv; charset=utf-16le" ) ensure tempfile&.close! diff --git a/features/step_definitions/web_steps.rb b/features/step_definitions/web_steps.rb index fd02752f857..d6542b42356 100644 --- a/features/step_definitions/web_steps.rb +++ b/features/step_definitions/web_steps.rb @@ -282,7 +282,10 @@ def with_scope(locator) Then /^I should download a ([^"]*) file with(?: (\d+) rows and)? the header row "(.*?)"$/ do |type, rows, header| page.response_headers['Content-Disposition'].should =~ /attachment; filename=.*?\.#{type}/i page.response_headers['Content-Type'].should =~ /\/#{type}/i - body_without_bom = page.body.encode("UTF-8").delete!("\xEF\xBB\xBF") + body_without_bom = page.body.dup + .force_encoding("UTF-16LE") + .encode("UTF-8") + .delete_prefix("\uFEFF") csv = CSV.parse(body_without_bom, col_sep: "\t") # array of arrays expect(csv.first.join(" ")).to eq(header) expect(csv.size).to eq(rows) unless rows.blank? || rows.zero? From 7ca6efa245eaab859a4c4c7c3af4d47e8aee1037 Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 01:39:55 +0800 Subject: [PATCH 10/12] AO3-7461 Preserve generated download MIME types --- app/jobs/generated_download_job.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/jobs/generated_download_job.rb b/app/jobs/generated_download_job.rb index 5ac29a904a2..03adb1790fd 100644 --- a/app/jobs/generated_download_job.rb +++ b/app/jobs/generated_download_job.rb @@ -38,7 +38,8 @@ def attach_work(generated_download) generated_download.file.attach( io: file, filename: generated_download.filename, - content_type: download.mime_type + content_type: download.mime_type, + identify: false ) end ensure @@ -53,7 +54,8 @@ def attach_csv(generated_download) generated_download.file.attach( io: tempfile, filename: generated_download.filename, - content_type: "text/csv; charset=utf-16le" + content_type: "text/csv; charset=utf-16le", + identify: false ) ensure tempfile&.close! From 2f83b39fd3b9f5354e51a60d725b3837c4e4ee37 Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 09:12:15 +0800 Subject: [PATCH 11/12] AO3-7461 Expect safe content type for HTML downloads --- features/step_definitions/work_download_steps.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/features/step_definitions/work_download_steps.rb b/features/step_definitions/work_download_steps.rb index e558b6b2533..ac1fc99642b 100644 --- a/features/step_definitions/work_download_steps.rb +++ b/features/step_definitions/work_download_steps.rb @@ -14,6 +14,8 @@ Then /^I should receive a file of type "(.*?)"$/ do |filetype| mime_type = Marcel::MimeType.for(name: "foo.#{filetype}").to_s + # Active Storage deliberately serves HTML as binary to prevent stored XSS. + mime_type = "application/octet-stream" if filetype == "html" expect(page.response_headers['Content-Disposition']).to match(/filename=.+\.#{filetype}/) expect(page.response_headers['Content-Length'].to_i).to be_positive expect(page.response_headers['Content-Type']).to eq(mime_type) From 1701b832f988812477b56f583d9d5ddc3bcb28d6 Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Tue, 28 Jul 2026 09:20:28 +0800 Subject: [PATCH 12/12] AO3-7461 Preserve download response content types --- .../generated_downloads_controller.rb | 11 +++--- app/jobs/generated_download_job.rb | 34 +++++++++++++------ .../step_definitions/work_download_steps.rb | 2 -- .../generated_downloads_controller_spec.rb | 13 +++++-- 4 files changed, 40 insertions(+), 20 deletions(-) diff --git a/app/controllers/generated_downloads_controller.rb b/app/controllers/generated_downloads_controller.rb index d29eb66014f..00e4241fb84 100644 --- a/app/controllers/generated_downloads_controller.rb +++ b/app/controllers/generated_downloads_controller.rb @@ -9,10 +9,13 @@ def show head :gone elsif @generated_download.status == "ready" && @generated_download.file.attached? blob = @generated_download.file.blob - redirect_to rails_storage_redirect_path( - blob, - disposition: "attachment" - ) + redirect_to blob.service.url( + blob.key, + expires_in: ActiveStorage.service_urls_expire_in, + filename: blob.filename, + content_type: blob.content_type, + disposition: :attachment + ), allow_other_host: true elsif @generated_download.status == "failed" render :show, status: :unprocessable_content else diff --git a/app/jobs/generated_download_job.rb b/app/jobs/generated_download_job.rb index 03adb1790fd..973a0e26859 100644 --- a/app/jobs/generated_download_job.rb +++ b/app/jobs/generated_download_job.rb @@ -35,12 +35,7 @@ def attach_work(generated_download) file_path = File.join(download.dir, File.basename(download.file_path)) File.open(file_path, "rb") do |file| - generated_download.file.attach( - io: file, - filename: generated_download.filename, - content_type: download.mime_type, - identify: false - ) + attach_file(generated_download, file, download.mime_type) end ensure download&.remove @@ -51,14 +46,31 @@ def attach_csv(generated_download) tempfile.binmode ExportsHelper.write_csv(tempfile, csv_rows(generated_download)) tempfile.rewind - generated_download.file.attach( - io: tempfile, + attach_file(generated_download, tempfile, "text/csv; charset=utf-16le") + ensure + tempfile&.close! + end + + def attach_file(generated_download, io, content_type) + blob = ActiveStorage::Blob.create_after_unfurling!( + io: io, filename: generated_download.filename, - content_type: "text/csv; charset=utf-16le", + content_type: content_type, identify: false ) - ensure - tempfile&.close! + io.rewind + blob.service.upload( + blob.key, + io, + checksum: blob.checksum, + filename: blob.filename, + content_type: content_type, + disposition: :attachment + ) + generated_download.file.attach(blob) + rescue StandardError + blob&.purge + raise end def csv_rows(generated_download) diff --git a/features/step_definitions/work_download_steps.rb b/features/step_definitions/work_download_steps.rb index ac1fc99642b..e558b6b2533 100644 --- a/features/step_definitions/work_download_steps.rb +++ b/features/step_definitions/work_download_steps.rb @@ -14,8 +14,6 @@ Then /^I should receive a file of type "(.*?)"$/ do |filetype| mime_type = Marcel::MimeType.for(name: "foo.#{filetype}").to_s - # Active Storage deliberately serves HTML as binary to prevent stored XSS. - mime_type = "application/octet-stream" if filetype == "html" expect(page.response_headers['Content-Disposition']).to match(/filename=.+\.#{filetype}/) expect(page.response_headers['Content-Length'].to_i).to be_positive expect(page.response_headers['Content-Type']).to eq(mime_type) diff --git a/spec/controllers/generated_downloads_controller_spec.rb b/spec/controllers/generated_downloads_controller_spec.rb index 32a292c0960..eebf0660adc 100644 --- a/spec/controllers/generated_downloads_controller_spec.rb +++ b/spec/controllers/generated_downloads_controller_spec.rb @@ -25,7 +25,7 @@ expect(response).to have_http_status(:gone) end - it "redirects ready downloads through the Active Storage redirect endpoint" do + it "redirects ready downloads to the storage service" do download = GeneratedDownload.create!( kind: "test", arguments: {}, @@ -38,11 +38,18 @@ content_type: "text/csv" ) blob = download.file.blob + service_url = "https://downloads.example.test/test.csv" + allow(blob.service).to receive(:url).and_return(service_url) get :show, params: { token: download.token } - expect(response).to redirect_to( - rails_storage_redirect_path(blob, disposition: "attachment") + expect(response).to redirect_to(service_url) + expect(blob.service).to have_received(:url).with( + blob.key, + expires_in: ActiveStorage.service_urls_expire_in, + filename: blob.filename, + content_type: blob.content_type, + disposition: :attachment ) end end