Merge pull request #6456 from alexmalik/fix-bug-2855-cookie-overflow

Fix: #2855: Cookie Overflow when trying to import CasaCase CSV

Author: compwron

app/controllers/imports_controller.rb

@@ -2,6 +2,7 @@ class ImportsController < ApplicationController
   require "csv"
 
   include ActionView::Helpers::UrlHelper
+  before_action :failed_csv_service, only: [:create, :download_failed]
   after_action :verify_authorized
 
   ERR_FAILED_IMPORT_NOTE = "Note: An additional 'error' column has been added to the file. " \
@@ -23,14 +24,17 @@ def index
 
   def create
     authorize :import
+    @failed_csv_service.cleanup
+
     import = import_from_csv(params[:import_type], params[:sms_opt_in], params[:file], current_user.casa_org_id)
     message = import[:message]
 
     # If there were failed imports
     if import[:exported_rows]
-      message << "<p class='mt-4'>" + link_to("Click here to download failed rows.", download_failed_imports_path) +
+      @failed_csv_service.failed_rows = import[:exported_rows]
+      @failed_csv_service.store
+      message << "<p class='mt-4'>" + link_to("Click here to download failed rows.", download_failed_imports_path(import_type: params[:import_type])) +
         "</p>" + "<p>#{ERR_FAILED_IMPORT_NOTE}</p>"
-      session[:exported_rows] = import[:exported_rows]
     end
 
     if import[:type] == :error
@@ -47,13 +51,21 @@ def create
 
   def download_failed
     authorize :import
-    data = session[:exported_rows]
-    session[:exported_rows] = nil
-    send_data data, format: :csv, filename: "failed_rows.csv"
+    csv_contents = @failed_csv_service.read
+
+    send_data csv_contents, format: :csv, filename: "failed_rows.csv"
   end
 
   private
 
+  def failed_csv_service(failed_rows: "")
+    @failed_csv_service = FailedImportCsvService.new(
+      failed_rows: failed_rows,
+      import_type: params[:import_type],
+      user: current_user
+    )
+  end
+
   def header
     {
       "volunteer" => VolunteerImporter::IMPORT_HEADER,

app/services/failed_import_csv_service.rb

@@ -0,0 +1,87 @@
+require "digest"
+
+class FailedImportCsvService
+  attr_accessor :failed_rows
+  attr_reader :import_type, :user, :csv_filepath
+
+  MAX_FILE_SIZE_BYTES = 250.kilobytes
+  EXPIRATION_TIME = 24.hours
+
+  def initialize(import_type:, user:, failed_rows: "", filepath: nil)
+    @failed_rows = failed_rows
+    @import_type = import_type
+    @user = user
+    @csv_filepath = filepath || generate_filepath
+  end
+
+  def store
+    if failed_rows.bytesize > MAX_FILE_SIZE_BYTES
+      Rails.logger.warn("CSV too large to save for user=#{user.id}, size=#{failed_rows.bytesize}")
+      @failed_rows = max_size_warning
+    end
+
+    FileUtils.mkdir_p(File.dirname(csv_filepath))
+    File.write(csv_filepath, failed_rows)
+  end
+
+  def read
+    return exists_warning unless csv_exists?
+
+    if expired?
+      remove_csv
+      return expired_warning
+    end
+
+    File.read(csv_filepath)
+  end
+
+  def cleanup
+    return unless csv_exists?
+
+    log_info("Removing old failed rows CSV")
+    remove_csv
+  end
+
+  private
+
+  def log_info(msg)
+    Rails.logger.info("User=#{user.id}, Type=#{import_type}: #{msg}")
+  end
+
+  def exists_warning
+    "No failed import file found. Please upload a #{humanised_import_type} CSV."
+  end
+
+  def expired_warning
+    "The failed import file has expired. Please upload a new #{humanised_import_type} CSV."
+  end
+
+  def max_size_warning
+    "The file was too large to save. Please make sure your CSV is smaller than 250 KB and try again."
+  end
+
+  def generate_filepath
+    Pathname.new(Rails.root.join("tmp", import_type.to_s, filename)).cleanpath
+  end
+
+  def csv_exists?
+    File.exist?(csv_filepath)
+  end
+
+  def expired?
+    csv_exists? && File.mtime(csv_filepath) < Time.current - EXPIRATION_TIME
+  end
+
+  def remove_csv
+    FileUtils.rm_f(csv_filepath)
+  end
+
+  def filename
+    short_hash = Digest::SHA256.hexdigest(user.id.to_s)[0..15]
+    "failed_rows_userid_#{short_hash}.csv"
+  end
+
+  def humanised_import_type
+    I18n.t("imports.labels.#{import_type}", default: import_type.to_s.humanize.downcase)
+  end
+end

config/locales/en.yml

@@ -79,3 +79,8 @@ en:
       long: "%B %d, %Y"
       short: "%b %d"
       year_first: "%Y-%m-%d"
+  imports:
+    labels:
+      casa_case: "CASA Case"
+      volunteer: "Volunteer"
+      supervisor: "Supervisor"

spec/fixtures/files/supervisors_without_display_names.csv

@@ -0,0 +1,4 @@
+email,display_name,supervisor_volunteers,phone_number
+[email protected],,[email protected],11111111111
+[email protected],,"[email protected], [email protected]",22222222222
+[email protected],,,33333333333

spec/fixtures/files/volunteers_without_display_names.csv

@@ -0,0 +1,4 @@
+display_name,email,phone_number
+,[email protected],111111111
+,[email protected],22222222222
+,[email protected],33333333333

spec/requests/imports_spec.rb

@@ -170,8 +170,101 @@
       }.not_to change(CasaCase, :count)
 
       expect(request.session[:import_error]).to include("Not all rows were imported.")
-      expect(request.session[:exported_rows]).to include("Case CINA-00-0000 already exists, but is inactive. Reactivate the CASA case instead.")
       expect(response).to redirect_to(imports_url(import_type: "casa_case"))
+
+      failed_csv_path = FailedImportCsvService.new(import_type: :casa_case, user: casa_admin).csv_filepath
+      expect(File.exist?(failed_csv_path)).to be true
+
+      file_contents = File.read(failed_csv_path)
+      expect(file_contents).to include("Case CINA-00-0000 already exists, but is inactive. Reactivate the CASA case instead.")
+
+      FileUtils.rm_f(failed_csv_path)
+    end
+
+    it "calls FailedImportCsvService#store when there are failed rows from the import" do
+      sign_in casa_admin
+
+      csv_service_double = instance_double(FailedImportCsvService)
+      allow(csv_service_double).to receive(:failed_rows=)
+      allow(csv_service_double).to receive(:store)
+      allow(csv_service_double).to receive(:cleanup)
+      allow(FailedImportCsvService).to receive(:new).and_return(csv_service_double)
+
+      allow(CaseImporter).to receive(:import_cases).and_return({
+        message: "Some cases were not imported.",
+        exported_rows: "Case CINA-00-0000 already exists, but is inactive. Reactivate the CASA case instead.",
+        type: :error
+      })
+
+      expect(csv_service_double).to receive(:failed_rows=).with("Case CINA-00-0000 already exists, but is inactive. Reactivate the CASA case instead.")
+      expect(csv_service_double).to receive(:store)
+      expect(csv_service_double).to receive(:cleanup)
+
+      post imports_url,
+        params: {
+          import_type: "casa_case",
+          file: fixture_file_upload("existing_casa_case.csv", "text/csv")
+        }
+
+      expect(request.session[:import_error]).to include("Click here to download failed rows.")
+      expect(response).to redirect_to(imports_url(import_type: "casa_case"))
+    end
+
+    it "writes a fallback message when exported rows exceed max size" do
+      sign_in casa_admin
+
+      large_exported_content = "a" * (FailedImportCsvService::MAX_FILE_SIZE_BYTES + 1)
+
+      allow(CaseImporter).to receive(:import_cases).and_return({
+        message: "Some rows were too large.",
+        exported_rows: large_exported_content,
+        type: :error
+      })
+
+      post imports_url,
+        params: {
+          import_type: "casa_case",
+          file: case_file
+        }
+
+      failed_csv_path = FailedImportCsvService.new(import_type: :casa_case, user: casa_admin).csv_filepath
+      expect(File.exist?(failed_csv_path)).to be true
+
+      written_content = File.read(failed_csv_path)
+      expect(written_content).to include("The file was too large to save")
+
+      expect(request.session[:import_error]).to include("Click here to download failed rows.")
+
+      FileUtils.rm_f(failed_csv_path)
+    end
+
+    it "shows a fallback error message when the failed import CSV was never created" do
+      sign_in casa_admin
+
+      get download_failed_imports_path(import_type: "casa_case")
+
+      expect(response.body).to include("Please upload a CASA Case CSV")
+      expect(response.headers["Content-Disposition"]).to include("attachment; filename=\"failed_rows.csv\"")
+    end
+
+    it "shows a fallback error message when the failed import CSV expired" do
+      sign_in casa_admin
+
+      service = FailedImportCsvService.new(
+        import_type: :casa_case,
+        user: casa_admin,
+        failed_rows: "some content",
+        filepath: path
+      )
+      service.store
+
+      File.utime(2.days.ago.to_time, 2.days.ago.to_time, service.csv_filepath)
+
+      get download_failed_imports_path(import_type: "casa_case")
+
+      expect(response.body).to include("Please upload a CASA Case CSV")
+      expect(response.headers["Content-Disposition"]).to include("attachment; filename=\"failed_rows.csv\"")
+      expect(File.exist?(path)).to be_falsey
     end
   end
 end

spec/services/failed_import_csv_service_spec.rb

@@ -0,0 +1,96 @@
+require "rails_helper"
+require "fileutils"
+require "csv"
+
+RSpec.describe FailedImportCsvService do
+  let(:import_type) { "casa_case" }
+  let(:user) { create(:casa_admin) }
+  let(:csv_string) { "case_number,birth_month_year_youth\n12345,2001-04\n" }
+  let(:user_id_hash) { Digest::SHA256.hexdigest(user.id.to_s)[0..15] }
+  let(:csv_path) { Rails.root.join("tmp", import_type, "failed_rows_userid_#{user_id_hash}.csv") }
+
+  subject(:service) { described_class.new(failed_rows: failed_rows, import_type: import_type, user: user) }
+
+  before { FileUtils.rm_f(csv_path) }
+  after { FileUtils.rm_f(csv_path) }
+
+  def create_file(content: csv_string, mtime: Time.current)
+    FileUtils.mkdir_p(File.dirname(csv_path))
+    File.write(csv_path, content)
+    File.utime(mtime.to_time, mtime.to_time, csv_path)
+  end
+
+  describe "#store" do
+    context "when file is within size limit" do
+      let(:failed_rows) { csv_string }
+
+      it "writes the CSV content to the tmp file" do
+        service.store
+
+        expect(File.exist?(csv_path)).to be true
+        expect(File.read(csv_path)).to eq csv_string
+      end
+    end
+
+    context "when file exceeds size limit" do
+      let(:failed_rows) { "a" * (described_class::MAX_FILE_SIZE_BYTES + 1) }
+
+      it "logs a warning and stores a warning" do
+        expect(Rails.logger).to receive(:warn).with(/CSV too large to save for user/)
+        service.store
+
+        expect(File.read(csv_path)).to match(/The file was too large to save/)
+      end
+    end
+  end
+
+  describe "#read" do
+    let(:failed_rows) { "" }
+
+    context "when file exists and has not expired" do
+      before { create_file }
+
+      it "returns the contents" do
+        expect(service.read).to eq csv_string
+      end
+    end
+
+    context "when file is expired" do
+      let(:failed_rows) { "The failed import file has expired. Please upload a new CSV." }
+      before { create_file(mtime: 2.days.ago.to_time) }
+
+      it "deletes the file and returns fallback message" do
+        expect(File.exist?(csv_path)).to be true
+        expect(service.read).to include("The failed import file has expired")
+        expect(File.exist?(csv_path)).to be false
+      end
+    end
+
+    context "when file never existed" do
+      it "returns fallback message" do
+        expect(service.read).to include("No failed import file found")
+      end
+    end
+  end
+
+  describe "#cleanup" do
+    let(:failed_rows) { "" }
+
+    context "when file exists" do
+      before { create_file }
+
+      it "removes the file" do
+        expect(File.exist?(csv_path)).to be true
+        expect(Rails.logger).to receive(:info).with(/Removing old failed rows CSV/)
+        service.cleanup
+        expect(File.exist?(csv_path)).to be false
+      end
+    end
+
+    context "when file does not exist" do
+      it "does nothing" do
+        expect { service.cleanup }.not_to raise_error
+      end
+    end
+  end
+end