fix: properly transcode CSV content to UTF-8 on import (Sentry DROPLET-SHIPPING-OPTIONS-1) - DRO-237

app/controllers/rates_controller.rb

@@ -110,11 +110,12 @@ def process_import
         csv_content = file_to_use.read
         file_to_use.rewind
 
-        # Handle encoding: uploaded files may arrive as ASCII-8BIT with a
-        # UTF-8 BOM (common when exported from Excel). Force to UTF-8 and
-        # strip the BOM so downstream CSV parsing works cleanly.
-        csv_content = csv_content.force_encoding("UTF-8")
-        csv_content.delete_prefix!("\xEF\xBB\xBF")
+        # Handle encoding before the bytes are parked in a temp file, so the
+        # auto-correction round trip re-reads content that is already valid
+        # UTF-8. RateCsvImportService normalizes again on read (the same call,
+        # which is a no-op on already-valid content) so that callers reaching
+        # the service directly get the identical treatment.
+        csv_content = CsvEncoding.to_utf8(csv_content)
 
         # Create a temporary file to store the CSV content
         temp_file = Tempfile.new([ "csv_import_#{session.id}_", ".csv" ], Rails.root.join("tmp"))

app/services/csv_encoding.rb

@@ -0,0 +1,81 @@
+module CsvEncoding
+  # Internal: Byte-order mark that Excel and friends prepend to UTF-8 exports.
+  BOM = "\xEF\xBB\xBF".b.freeze
+
+  # Internal: Stands in for bytes that no reading can make sense of. Windows-1252
+  # leaves five bytes undefined (0x81 0x8D 0x8F 0x90 0x9D), so even the
+  # Windows-1252 reading can produce these.
+  REPLACEMENT_CHARACTER = "�".freeze
+
+  # Public: Normalizes the raw bytes of an uploaded CSV into valid UTF-8.
+  #
+  # Uploaded CSVs arrive as raw bytes that are usually UTF-8 (often with a BOM,
+  # which is what Excel writes) but are sometimes Windows-1252/Latin-1 instead,
+  # and are occasionally UTF-8 that picked up a handful of corrupt bytes along
+  # the way (a truncated write, a byte-sliced concatenation, a bad export).
+  #
+  # Telling "Windows-1252 file" apart from "UTF-8 file with a few bad bytes"
+  # matters, because the two need opposite treatment and guessing wrong destroys
+  # data either way:
+  #
+  #   * Transcoding a corrupt UTF-8 file from Windows-1252 mojibakes every
+  #     legitimate multi-byte character in it ("Café" becomes "Café").
+  #   * Scrubbing a genuine Windows-1252 file silently deletes every accented
+  #     character in it ("Señor" becomes "Seor").
+  #
+  # The signal used here is that valid multi-byte UTF-8 sequences essentially
+  # never occur by accident in Windows-1252 text: an accented Windows-1252
+  # character is almost always followed by an ASCII letter or punctuation mark,
+  # and ASCII bytes are never UTF-8 continuation bytes. So content that decodes
+  # to intact multi-byte characters is UTF-8, and the bytes that do not decode
+  # are damage to be scrubbed.
+  #
+  # Rather than treating a single intact sequence as proof, the two readings are
+  # weighed against each other and the one that loses fewer characters wins.
+  # That keeps one accidental byte pair in a Windows-1252 file from flipping the
+  # whole file to the wrong interpretation.
+  #
+  # bytes - The String read from the upload. May carry any encoding tag; only the
+  #         bytes are used. A leading UTF-8 BOM is removed before anything else
+  #         looks at the content.
+  #
+  # Examples
+  #
+  #   CsvEncoding.to_utf8("Se\xF1or Freight".b)
+  #   # => "Señor Freight"
+  #
+  #   CsvEncoding.to_utf8("Caf\xC3\xA9 Express".b + "\xC3".b)
+  #   # => "Café Express�"
+  #
+  # Returns a String tagged UTF-8 that is guaranteed to satisfy #valid_encoding?.
+  #   Content that is already valid UTF-8 comes back byte for byte unchanged,
+  #   apart from the stripped BOM.
+  def self.to_utf8(bytes)
+    return bytes if bytes.nil?
+
+    content = bytes.to_s.dup.force_encoding(Encoding::BINARY).delete_prefix(BOM)
+    content.force_encoding(Encoding::UTF_8)
+    return content if content.valid_encoding?
+
+    damaged_sequences = 0
+    scrubbed = content.scrub { damaged_sequences += 1; REPLACEMENT_CHARACTER }
+    intact_multibyte = multibyte_character_count(scrubbed) - damaged_sequences
+
+    if intact_multibyte.positive? && intact_multibyte >= damaged_sequences
+      scrubbed
+    else
+      content.encode(Encoding::UTF_8, Encoding::WINDOWS_1252, invalid: :replace, undef: :replace)
+    end
+  end
+
+  # Internal: Counts the characters in a valid UTF-8 String that occupy more than
+  # one byte.
+  #
+  # utf8_content - A String tagged UTF-8 with no invalid byte sequences.
+  #
+  # Returns an Integer.
+  def self.multibyte_character_count(utf8_content)
+    utf8_content.each_char.count { |character| character.bytesize > 1 }
+  end
+  private_class_method :multibyte_character_count
+end

app/services/rate_csv_import_service.rb

@@ -79,9 +79,11 @@ def valid_file_type?
 
   def read_csv_file
     content = file.respond_to?(:read) ? file.read : File.read(file.path)
-    # Handle encoding: force to UTF-8 and strip BOM if present
-    content = content.force_encoding("UTF-8")
-    content.delete_prefix!("\xEF\xBB\xBF")
+    # Handle encoding: strip the BOM and turn whatever bytes arrived into valid
+    # UTF-8. Shared with RatesController#process_import so an upload and a direct
+    # service call are transcoded identically; a no-op on valid UTF-8, so content
+    # the controller already normalized passes straight through.
+    content = CsvEncoding.to_utf8(content)
     # Store original content for re-reading when applying corrections
     @csv_content = content
     CSV.parse(content, headers: true, header_converters: :symbol)

test/controllers/rates_controller_test.rb

@@ -3,6 +3,8 @@
 class RatesControllerTest < ActionDispatch::IntegrationTest
   fixtures :companies
 
+  CSV_IMPORT_HEADER = "shipping_method,country,region,min_range_lbs,max_range_lbs,flat_rate,min_charge\n".freeze
+
   test "gets index with dri parameter" do
     get rate_tables_url, params: { dri: "test-dri" }
     assert_response :success
@@ -37,4 +39,113 @@ class RatesControllerTest < ActionDispatch::IntegrationTest
     # Puede ser success o redirect, pero no debe ser 400 Bad Request
     assert_not_equal 400, response.status
   end
+
+  test "process_import successfully imports a CSV with non-UTF-8 bytes (Sentry DROPLET-SHIPPING-OPTIONS-1)" do
+    # Reproduces the Sentry crash scenario: a CSV exported from Excel/another tool can
+    # contain Windows-1252/raw binary bytes (e.g. a lone 0xC2) that are not valid UTF-8.
+    #
+    # `String#force_encoding("UTF-8")` only relabels the bytes as UTF-8 without converting
+    # them, so the string stays invalid UTF-8 all the way through import. That silently
+    # sends the row through `CSV.parse`, which raises `CSV::InvalidEncodingError` when it
+    # hits the bad byte, and `RateCsvImportService#read_csv_file` rescues that as a plain
+    # "Unable to read CSV file" failure (HTTP 422) instead of importing the row.
+    #
+    # Transcoding the bytes from Windows-1252 makes the string genuinely valid UTF-8,
+    # so the row survives CSV parsing and the import succeeds.
+    csv_bytes = CSV_IMPORT_HEADER.b + "Expr\xC2ess Shipping,US,CA,0,5,9.99,5.00\n".b
+    uploaded = uploaded_csv(csv_bytes, "non_utf8.csv")
+
+    assert_difference -> { Rate.count }, 1 do
+      post process_import_rate_tables_url, params: { dri: "test-dri", csv_file: uploaded }
+    end
+
+    assert_response :redirect
+    assert_match(/Successfully imported/, flash[:notice])
+
+    # Pin the exact name: 0xC2 is "Â" in Windows-1252, so the byte has to survive as that
+    # character. Asserting only the "Expr" prefix would also pass if the byte were dropped
+    # or turned into a replacement character.
+    assert ShippingOption.exists?(name: "ExprÂess Shipping"),
+      "expected 0xC2 to be transcoded to Â, got #{ShippingOption.pluck(:name).inspect}"
+  end
+
+  test "process_import preserves valid UTF-8 when the upload also carries a corrupt byte" do
+    # The corruption case: a file that is mostly valid UTF-8 but ends on a truncated
+    # sequence. Re-reading the whole file as Windows-1252 because of that one byte turns
+    # "Café" into "Café", and refusing to transcode at all makes CSV.parse raise. Only
+    # scrubbing the damaged byte while leaving the intact multi-byte characters alone
+    # imports the row under its real name.
+    csv_bytes = CSV_IMPORT_HEADER.b +
+      "Caf\xC3\xA9 Express,US,CA,0,5,9.99,5.00\n".b +
+      "Se\xC3\xB1or Freight,US,NY,0,5,9.99,5.00\n".b +
+      "Ac\xC3me Freight,US,TX,0,5,9.99,5.00\n".b
+    uploaded = uploaded_csv(csv_bytes, "mixed_encoding.csv")
+
+    assert_difference -> { Rate.count }, 3 do
+      post process_import_rate_tables_url, params: { dri: "test-dri", csv_file: uploaded }
+    end
+
+    assert_response :redirect
+    assert ShippingOption.exists?(name: "Café Express"),
+      "expected the UTF-8 name to survive the corrupt byte, got #{ShippingOption.pluck(:name).inspect}"
+    assert ShippingOption.exists?(name: "Señor Freight"),
+      "expected the UTF-8 name to survive the corrupt byte, got #{ShippingOption.pluck(:name).inspect}"
+    assert_not ShippingOption.exists?(name: "Café Express"),
+      "the file must not be re-read as Windows-1252 because of one damaged byte"
+    assert ShippingOption.exists?(name: "Ac�me Freight"),
+      "expected only the damaged byte to be replaced, got #{ShippingOption.pluck(:name).inspect}"
+  end
+
+  test "process_import preserves valid UTF-8 multi-byte characters" do
+    # Guards against transcoding the upload as if every byte stood alone: a file that is
+    # already valid UTF-8 must pass through byte for byte, instead of having each byte of
+    # a multi-byte character turned into a replacement character.
+    csv_bytes = CSV_IMPORT_HEADER.b + "Caf\xC3\xA9 Express,US,CA,0,5,9.99,5.00\n".b
+    uploaded = uploaded_csv(csv_bytes, "utf8.csv")
+
+    assert_difference -> { Rate.count }, 1 do
+      post process_import_rate_tables_url, params: { dri: "test-dri", csv_file: uploaded }
+    end
+
+    assert_response :redirect
+    assert ShippingOption.exists?(name: "Café Express"), "expected the UTF-8 name to survive unchanged"
+  end
+
+  test "process_import transcodes a Windows-1252 upload without a BOM" do
+    csv_bytes = CSV_IMPORT_HEADER.b + "Se\xF1or Freight,US,NY,0,5,9.99,5.00\n".b
+    uploaded = uploaded_csv(csv_bytes, "cp1252.csv")
+
+    assert_difference -> { Rate.count }, 1 do
+      post process_import_rate_tables_url, params: { dri: "test-dri", csv_file: uploaded }
+    end
+
+    assert_response :redirect
+    assert ShippingOption.exists?(name: "Señor Freight"),
+      "expected 0xF1 to be transcoded to ñ, got #{ShippingOption.pluck(:name).inspect}"
+  end
+
+  test "process_import transcodes Windows-1252 content carrying a UTF-8 BOM" do
+    # Excel exports frequently carry a UTF-8 BOM and Windows-1252 high bytes. The BOM must
+    # be stripped at the byte level, before anything relabels it, and the 0xF1 byte must
+    # become "ñ" rather than a replacement character.
+    csv_bytes = "\xEF\xBB\xBF".b + CSV_IMPORT_HEADER.b + "Se\xF1or Freight,US,NY,0,5,9.99,5.00\n".b
+    uploaded = uploaded_csv(csv_bytes, "cp1252_bom.csv")
+
+    assert_difference -> { Rate.count }, 1 do
+      post process_import_rate_tables_url, params: { dri: "test-dri", csv_file: uploaded }
+    end
+
+    assert_response :redirect
+    assert ShippingOption.exists?(name: "Señor Freight"), "expected Windows-1252 bytes to be transcoded"
+  end
+
+private
+
+  def uploaded_csv(bytes, filename)
+    file = Tempfile.new([ "test_csv_import", ".csv" ])
+    file.binmode
+    file.write(bytes)
+    file.rewind
+    Rack::Test::UploadedFile.new(file.path, "text/csv", original_filename: filename)
+  end
 end

test/services/csv_encoding_test.rb

@@ -0,0 +1,94 @@
+require "test_helper"
+require "csv"
+
+class CsvEncodingTest < ActiveSupport::TestCase
+  BOM = "\xEF\xBB\xBF".b.freeze
+
+  test "returns valid UTF-8 content byte for byte unchanged" do
+    utf8_bytes = "Caf\xC3\xA9 Express,Se\xC3\xB1or Freight,\xE2\x82\xAC9.99".b
+
+    normalized = CsvEncoding.to_utf8(utf8_bytes)
+
+    assert_equal utf8_bytes, normalized.b
+    assert_equal "Café Express,Señor Freight,€9.99", normalized
+    assert_equal Encoding::UTF_8, normalized.encoding
+    assert_predicate normalized, :valid_encoding?
+  end
+
+  test "transcodes genuine Windows-1252 content instead of dropping its accents" do
+    normalized = CsvEncoding.to_utf8("Se\xF1or Freight".b)
+
+    assert_equal "Señor Freight", normalized
+  end
+
+  test "transcodes Windows-1252 content that uses several high bytes" do
+    normalized = CsvEncoding.to_utf8("Se\xF1or M\xFCller caf\xE9 \x93quoted\x94".b)
+
+    assert_equal "Señor Müller café “quoted”", normalized
+  end
+
+  test "keeps intact multi-byte characters when UTF-8 content carries a truncated sequence" do
+    # The regression this guards: an all-or-nothing valid_encoding? check lets a
+    # single truncated sequence re-read the whole file as Windows-1252, which
+    # turns every legitimate "é" into "é".
+    normalized = CsvEncoding.to_utf8("Caf\xC3\xA9 Express".b + "\xC3".b)
+
+    assert_equal "Café Express�", normalized
+    assert_not_includes normalized, "Ã", "the intact UTF-8 body must not be re-read as Windows-1252"
+  end
+
+  test "keeps intact multi-byte characters when a bad byte sits in the middle" do
+    normalized = CsvEncoding.to_utf8("Caf\xC3\xA9,\xC3,Se\xC3\xB1or,\xE2\x82\xAC5".b)
+
+    assert_equal "Café,�,Señor,€5", normalized
+  end
+
+  test "reads content as Windows-1252 when damaged sequences outnumber intact ones" do
+    # One accidental 0xC3 0xA9 pair inside an otherwise Windows-1252 file must not
+    # flip the whole file to UTF-8 and scrub away the three real accented letters.
+    normalized = CsvEncoding.to_utf8("Se\xF1or M\xFCller caf\xE9 \xC3\xA9".b)
+
+    assert_equal "Señor Müller café é", normalized
+  end
+
+  test "makes the reported Sentry bytes parseable instead of raising" do
+    sentry_bytes = "Expr\xC2ess Shipping,US,CA,0,5,9.99,5.00".b
+
+    assert_raises(CSV::InvalidEncodingError) do
+      CSV.parse(sentry_bytes.dup.force_encoding(Encoding::UTF_8))
+    end
+
+    normalized = CsvEncoding.to_utf8(sentry_bytes)
+
+    assert_equal "ExprÂess Shipping,US,CA,0,5,9.99,5.00", normalized
+    assert_nothing_raised { CSV.parse(normalized) }
+  end
+
+  test "strips the UTF-8 BOM whatever the rest of the content turns out to be" do
+    assert_equal "Café", CsvEncoding.to_utf8(BOM + "Caf\xC3\xA9".b)
+    assert_equal "Señor", CsvEncoding.to_utf8(BOM + "Se\xF1or".b)
+    assert_equal "Café�", CsvEncoding.to_utf8(BOM + "Caf\xC3\xA9".b + "\xC3".b)
+    assert_equal "ExprÂess", CsvEncoding.to_utf8(BOM + "Expr\xC2ess".b)
+    assert_equal "plain", CsvEncoding.to_utf8(BOM + "plain".b)
+    assert_equal "", CsvEncoding.to_utf8(BOM)
+  end
+
+  test "replaces the five Windows-1252 undefined bytes rather than raising" do
+    assert_nothing_raised do
+      assert_equal "A�����B", CsvEncoding.to_utf8("A\x81\x8D\x8F\x90\x9DB".b)
+    end
+
+    assert_equal "Señor�Freight", CsvEncoding.to_utf8("Se\xF1or\x81Freight".b)
+  end
+
+  test "is idempotent so a second pass never re-damages content" do
+    once = CsvEncoding.to_utf8("Se\xF1or\x81 Caf\xC3\xA9".b)
+
+    assert_equal once, CsvEncoding.to_utf8(once)
+  end
+
+  test "handles empty and nil content" do
+    assert_equal "", CsvEncoding.to_utf8("")
+    assert_nil CsvEncoding.to_utf8(nil)
+  end
+end