import parsing

web/src/pages/import_export/common/csv.rs

@@ -1,51 +1,57 @@
 use crate::{i18n::Locale, tr};
 
+/// Parses the one CSV shape accepted by data transfer: UTF-8, comma separated,
+/// and every field enclosed in double quotes. Keeping one spelling makes the
+/// file self-describing and leaves commas inside values unambiguous.
 pub(crate) fn parse_csv(locale: Locale, input: &str) -> Result<Vec<Vec<String>>, String> {
-    // The byte-order mark Excel writes at the head of a "CSV UTF-8" file is
-    // encoding metadata, not a character of the first field, so decoding
-    // consumes it. Nothing else about the file is touched here: what the
-    // fields contain is the user's, and the import validates it rather than
-    // tidying it up.
     let input = input.strip_prefix('\u{feff}').unwrap_or(input);
+    if input.is_empty() {
+        return Err(tr!(locale, "import-err-empty"));
+    }
+
+    let mut chars = input.chars().peekable();
     let mut rows = Vec::new();
     let mut row = Vec::new();
-    let mut field = String::new();
-    let mut chars = input.chars().peekable();
-    let mut quoted = false;
-    while let Some(ch) = chars.next() {
-        match ch {
-            '"' if quoted && chars.peek() == Some(&'"') => {
-                field.push('"');
-                let _ = chars.next();
-            }
-            '"' => quoted = !quoted,
-            ',' if !quoted => row.push(std::mem::take(&mut field)),
-            '\n' if !quoted => {
-                if field.ends_with('\r') {
-                    field.pop();
-                }
-                row.push(std::mem::take(&mut field));
-                if row.iter().any(|value| !value.is_empty()) {
-                    rows.push(std::mem::take(&mut row));
-                } else {
-                    row.clear();
-                }
-            }
-            _ => field.push(ch),
+
+    loop {
+        match chars.next() {
+            Some('"') => {}
+            Some(_) => return Err(tr!(locale, "import-err-strict-csv")),
+            None if row.is_empty() => break,
+            None => return Err(tr!(locale, "import-err-strict-csv")),
+        }
+
+        let mut field = String::new();
+        loop {
+            match chars.next() {
+                Some('"') if chars.peek() == Some(&'"') => {
+                    field.push('"');
+                    let _ = chars.next();
+                }
+                Some('"') => break,
+                // A strict import row is one physical line. This also avoids
+                // hiding a missing closing quote many lines above the error.
+                Some('\n' | '\r') => return Err(tr!(locale, "import-err-strict-csv")),
+                Some(ch) => field.push(ch),
+                None => return Err(tr!(locale, "import-err-unterminated-quote")),
+            }
         }
-    }
-    if quoted {
-        return Err(tr!(locale, "import-err-unterminated-quote"));
-    }
-    if field.ends_with('\r') {
-        field.pop();
-    }
-    if !field.is_empty() || !row.is_empty() {
         row.push(field);
-        if row.iter().any(|value| !value.is_empty()) {
-            rows.push(row);
+
+        match chars.next() {
+            Some(',') => continue,
+            Some('\n') => rows.push(std::mem::take(&mut row)),
+            Some('\r') if chars.next() == Some('\n') => {
+                rows.push(std::mem::take(&mut row));
+            }
+            Some(_) => return Err(tr!(locale, "import-err-strict-csv")),
+            None => {
+                rows.push(row);
+                break;
+            }
         }
     }
+
     if rows.is_empty() {
         Err(tr!(locale, "import-err-empty"))
     } else {
@@ -53,18 +59,15 @@ pub(crate) fn parse_csv(locale: Locale, input: &str) -> Result<Vec<Vec<String>>,
     }
 }
 
+/// Writes the same fully quoted format the importer accepts.
 pub(crate) fn write_record(output: &mut String, fields: &[String]) {
     for (index, field) in fields.iter().enumerate() {
         if index > 0 {
             output.push(',');
         }
-        if field.contains([',', '"', '\n', '\r']) {
-            output.push('"');
-            output.push_str(&field.replace('"', "\"\""));
-            output.push('"');
-        } else {
-            output.push_str(field);
-        }
+        output.push('"');
+        output.push_str(&field.replace('"', "\"\""));
+        output.push('"');
     }
     output.push('\n');
 }
@@ -74,10 +77,48 @@ mod tests {
     use super::*;
 
     #[test]
-    fn quoted_multiline_csv_round_trips() {
-        let fields = vec!["name".to_string(), "hello, \"world\"\nnext".to_string()];
+    fn fully_quoted_csv_round_trips_commas_and_quotes() {
+        let fields = vec![
+            "Acme, s.r.o.".to_string(),
+            "Customer said \"send it today\"".to_string(),
+        ];
         let mut csv = String::new();
         write_record(&mut csv, &fields);
-        assert_eq!(parse_csv(crate::i18n::Locale::default(), &csv).unwrap(), vec![fields]);
+
+        assert_eq!(
+            csv,
+            "\"Acme, s.r.o.\",\"Customer said \"\"send it today\"\"\"\n"
+        );
+        assert_eq!(
+            parse_csv(crate::i18n::Locale::default(), &csv).unwrap(),
+            vec![fields]
+        );
+    }
+
+    #[test]
+    fn every_field_must_be_quoted() {
+        for csv in [
+            "name,num\nAcme,123\n",
+            "\"name\",num\n\"Acme\",\"123\"\n",
+            "\"name\",\"num\"\n\"Acme\",123\n",
+            "\"name\" ,\"num\"\n",
+        ] {
+            assert!(parse_csv(crate::i18n::Locale::default(), csv).is_err(), "{csv}");
+        }
+    }
+
+    #[test]
+    fn accepts_utf8_bom_and_crlf() {
+        let csv = "\u{feff}\"name\",\"num\"\r\n\"Acme\",\"123\"\r\n";
+        assert_eq!(
+            parse_csv(crate::i18n::Locale::default(), csv).unwrap(),
+            vec![vec!["name", "num"], vec!["Acme", "123"]]
+        );
+    }
+
+    #[test]
+    fn multiline_fields_are_rejected() {
+        let csv = "\"name\",\"note\"\n\"Acme\",\"first\nsecond\"\n";
+        assert!(parse_csv(crate::i18n::Locale::default(), csv).is_err());
     }
 }

web/src/pages/import_export/import/logic.rs

@@ -635,10 +635,18 @@ mod tests {
     /// header is left exactly as it arrived.
     #[test]
     fn the_byte_order_mark_is_decoded_away_and_nothing_else_is() {
-        let rows = parse_csv(Locale::default(), "\u{feff}label,amount\nAcme,10\n").unwrap();
+        let rows = parse_csv(
+            Locale::default(),
+            "\u{feff}\"label\",\"amount\"\n\"Acme\",\"10\"\n",
+        )
+        .unwrap();
         assert_eq!(rows[0], vec!["label", "amount"]);
 
-        let rows = parse_csv(Locale::default(), "LABEL, amount \nAcme,10\n").unwrap();
+        let rows = parse_csv(
+            Locale::default(),
+            "\"LABEL\",\" amount \"\n\"Acme\",\"10\"\n",
+        )
+        .unwrap();
         assert_eq!(rows[0], vec!["LABEL", " amount "]);
     }