diff --git a/app/controllers/rates_controller.rb b/app/controllers/rates_controller.rb index 396be6a2..c101d2df 100644 --- a/app/controllers/rates_controller.rb +++ b/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")) diff --git a/app/services/csv_encoding.rb b/app/services/csv_encoding.rb new file mode 100644 index 00000000..e36e522b --- /dev/null +++ b/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 diff --git a/app/services/rate_csv_import_service.rb b/app/services/rate_csv_import_service.rb index 5ef72bf3..5a4fa4ba 100644 --- a/app/services/rate_csv_import_service.rb +++ b/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) diff --git a/test/controllers/rates_controller_test.rb b/test/controllers/rates_controller_test.rb index f2e38b52..90141265 100644 --- a/test/controllers/rates_controller_test.rb +++ b/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 diff --git a/test/services/csv_encoding_test.rb b/test/services/csv_encoding_test.rb new file mode 100644 index 00000000..3638c191 --- /dev/null +++ b/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 diff --git a/test/services/rate_csv_import_service_test.rb b/test/services/rate_csv_import_service_test.rb index 1963e95f..0d756ffd 100644 --- a/test/services/rate_csv_import_service_test.rb +++ b/test/services/rate_csv_import_service_test.rb @@ -1,6 +1,8 @@ require "test_helper" class RateCsvImportServiceTest < ActiveSupport::TestCase + CSV_HEADER_BYTES = "shipping_method,country,region,min_range_lbs,max_range_lbs,flat_rate,min_charge\n".b.freeze + def setup @company = companies(:acme) @shipping_option = shipping_options(:express_shipping) @@ -712,8 +714,72 @@ def setup assert_equal 1, result[:imported_count] end + # The controller normalizes an upload before it ever reaches this service, so these + # cover the direct-call path: anything constructing the service itself (a job, a + # console session, a future caller) has to get the same transcoding. + test "should transcode Windows-1252 bytes when called directly" do + file = create_binary_csv_file(CSV_HEADER_BYTES + "Se\xF1or Freight,US,NY,0,5,9.99,5.00\n".b) + service = RateCsvImportService.new(company: @company, file: file) + + result = service.call + + assert result[:success], "Expected success but got: #{result[:message]} #{result[:errors]}" + assert_equal 1, result[:imported_count] + assert @company.shipping_options.exists?(name: "Señor Freight"), + "expected 0xF1 to be transcoded to ñ, got #{@company.shipping_options.pluck(:name).inspect}" + end + + test "should transcode Windows-1252 bytes carrying a UTF-8 BOM when called directly" do + bytes = "\xEF\xBB\xBF".b + CSV_HEADER_BYTES + "Se\xF1or Freight,US,NY,0,5,9.99,5.00\n".b + file = create_binary_csv_file(bytes) + service = RateCsvImportService.new(company: @company, file: file) + + result = service.call + + assert result[:success], "Expected success but got: #{result[:message]} #{result[:errors]}" + assert @company.shipping_options.exists?(name: "Señor Freight"), + "expected the BOM to be stripped and 0xF1 transcoded, got #{@company.shipping_options.pluck(:name).inspect}" + end + + test "should import bytes that are not valid UTF-8 when called directly" do + file = create_binary_csv_file(CSV_HEADER_BYTES + "Expr\xC2ess Shipping,US,CA,0,5,9.99,5.00\n".b) + service = RateCsvImportService.new(company: @company, file: file) + + result = service.call + + assert result[:success], "Expected success but got: #{result[:message]} #{result[:errors]}" + assert @company.shipping_options.exists?(name: "ExprÂess Shipping"), + "expected 0xC2 to be transcoded to Â, got #{@company.shipping_options.pluck(:name).inspect}" + end + + test "should keep intact UTF-8 characters when the file also holds a corrupt byte" do + bytes = CSV_HEADER_BYTES + + "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 + file = create_binary_csv_file(bytes) + service = RateCsvImportService.new(company: @company, file: file) + + result = service.call + + assert result[:success], "Expected success but got: #{result[:message]} #{result[:errors]}" + assert_equal 3, result[:imported_count] + assert @company.shipping_options.exists?(name: "Café Express"), + "expected the UTF-8 name to survive, got #{@company.shipping_options.pluck(:name).inspect}" + assert_not @company.shipping_options.exists?(name: "Café Express"), + "one damaged byte must not re-read the whole file as Windows-1252" + end + private + def create_binary_csv_file(bytes) + file = Tempfile.new([ "test_encoding", ".csv" ]) + file.binmode + file.write(bytes) + file.rewind + Rack::Test::UploadedFile.new(file.path, "text/csv", original_filename: "encoding_test.csv") + end + def create_csv_file(content) file = Tempfile.new([ "test", ".csv" ]) file.write(content)