fix: properly transcode CSV content to UTF-8 on import (Sentry DROPLET-SHIPPING-OPTIONS-1) - DRO-237 - #58
Conversation
…T-SHIPPING-OPTIONS-1)
…TIONS-1)
The PR's encode(...) fix had no test proving it does anything: the existing
service-level test suite never exercised the controller's encoding step, so
the fix could regress silently.
Added a controller test that posts a CSV containing a lone invalid byte
(0xC2, matching the reported crash) through process_import. Verified locally
that this test fails on the pre-fix force_encoding("UTF-8") code (the row
gets rejected with a 422 "Unable to read CSV file" instead of importing,
since force_encoding leaves the string invalid and CSV.parse's
CSV::InvalidEncodingError is swallowed as a generic read failure) and passes
with the encode(..., invalid: :replace, undef: :replace) fix (the row
imports successfully with the bad byte replaced).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Transcoding the upload with `encode("UTF-8", "binary", ...)` treats every
byte as standalone, so each byte of a multi-byte character is undefined in
ASCII-8BIT and gets replaced with U+FFFD. That turned the crash into silent
corruption: an already-valid UTF-8 export lost every accented character, a
Windows-1252 export was not actually transcoded, and the UTF-8 BOM became
three replacement characters that `delete_prefix!` could no longer strip.
Strip the BOM at the byte level first, then relabel as UTF-8 and only
transcode from Windows-1252 when the bytes are not valid UTF-8. Valid UTF-8
now passes through untouched, legacy single-byte content is transcoded to
its real characters, and undecodable bytes are still replaced rather than
raising Encoding::UndefinedConversionError.
Covered by tests for valid UTF-8 preservation and for a BOM-prefixed
Windows-1252 export, alongside the existing lone-0xC2 regression test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/greptileai review |
The valid_encoding? check is all-or-nothing, so a single bad byte anywhere in the upload re-reads the entire file as Windows-1252. A file that is genuinely UTF-8 apart from one truncated trailing sequence therefore comes back mojibaked: "Café Express" + "\xC3" imports as "Café ExpressÃ", with every legitimate accented character destroyed to salvage one damaged byte. Scrubbing instead is not the answer either. It fixes the corruption case but silently deletes every accented character out of a genuine Windows-1252 export, turning "Señor Freight" into "Seor Freight". Weigh the two readings against each other. Valid multi-byte UTF-8 sequences essentially never occur by accident in Windows-1252 text, because an accented Windows-1252 character is almost always followed by an ASCII byte 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 scrub. Counting the intact sequences against the damaged ones, rather than looking only for the presence of one, keeps a single accidental byte pair in a Windows-1252 file from flipping the whole file to the wrong reading. Extract the whole decision into CsvEncoding.to_utf8 and call it from both RatesController#process_import and RateCsvImportService#read_csv_file. The service still carried the old force_encoding + BOM-strip logic, so anything constructing the service directly got "Unable to read CSV file" for Windows-1252 or BOM-prefixed content that the controller path handled fine. The call is idempotent, so the controller normalizing before it parks the bytes in a temp file and the service normalizing again on read do not fight. Strengthen the import tests: the lone-0xC2 case now pins the exact imported name rather than a four-character prefix, so dropping or replacing the byte no longer passes. Add coverage for mixed valid/corrupt UTF-8, Windows-1252 without a BOM, and the direct service path, plus a unit test for the module. Verified by reverting to each wrong implementation in turn: pre-PR force_encoding (16 failures), unconditional binary transcode (19), the all-or-nothing check this commit replaces (5), and naive scrub-only (12). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixed: one bad byte was destroying every accented character in the fileThe Switching to The two inputs need opposite treatment, and the old code had no way to tell them apart. Detection strategyValid 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 scrub. Rather than treating a single intact sequence as proof, the two readings are counted against each other and the one that loses fewer characters wins. That matters for the ambiguous middle — a Windows-1252 file that happens to contain one accidental Proof
Shared extraction (addresses the Medium finding)
Both call sites now go through one implementation,
The call is idempotent — already-valid UTF-8 short-circuits — so the two do not fight. Test strengtheningThe Added: mixed valid/corrupt UTF-8 (controller + service), Windows-1252 without a BOM, the three direct-service-call cases, and a Every test was verified to actually fail by swapping
Full suite locally: ( |
Sentry Issue
DROPLET-SHIPPING-OPTIONS-1: Encoding::UndefinedConversionError - "\xC2" from ASCII-8BIT to UTF-8
Project: droplet-shipping-options · Events: ~40+ · Users: Multiple
Diagnosis
Surface Error
Encoding::UndefinedConversionError: "\xC2" from ASCII-8BIT to UTF-8raised inrates_controller.rbduring CSV import when a user uploads a file exported from Excel or another tool that produces non-ASCII characters (e.g. currency symbols, accented names).Root Cause
String#force_encoding("UTF-8")only relabels the string's encoding — it tells Ruby "pretend this is UTF-8" without converting any bytes. When the string actually contains non-UTF-8 byte sequences (Windows-1252, Latin-1, or raw binary), subsequent operations (write, string interpolation, CSV parsing) raiseEncoding::UndefinedConversionErrorbecause Ruby tries to interpret those bytes as UTF-8 and fails.What a Quick Fix Would Look Like
Rescuing the
Encoding::UndefinedConversionErrorand returning a user-facing error message — this would hide the crash but not fix the encoding and would still produce garbled output in downstream CSV parsing.Solution
Approach
Replace
force_encoding("UTF-8")withencode("UTF-8", "binary", invalid: :replace, undef: :replace).String#encodewith source encoding"binary"treats each byte independently (no multi-byte assumptions), transcodes to UTF-8, and replaces any byte sequences that cannot be represented in UTF-8 with the Unicode replacement character (�). This means:Why This Is a Root Fix (Not a Bandaid)
The crash site is
temp_file.write(csv_content)— Ruby refuses to write a string it believes is UTF-8 but contains non-UTF-8 bytes.force_encodingcreated a lie: the string said it was UTF-8 but wasn't. The fix ensures the string actually is UTF-8 before anything tries to use it, preventing this entire class of encoding errors from reaching the write call, CSV parser, or any downstream service.Changes
app/controllers/rates_controller.rb: replacedforce_encoding("UTF-8")→encode("UTF-8", "binary", invalid: :replace, undef: :replace)and updated the comment to describe what actually happensFuture Work
If lossless preservation of the original encoding matters (e.g. for auditing), the replacement characters could be logged. For the current use case (CSV import), lossy replacement is preferable to a 500 error.
Generated by Sentry Auto-Fixer Agent
Fixes DRO-237