Fix CSV-aware user password redaction

Co-authored-by: bcantoni <58372+bcantoni@users.noreply.github.com>

Author: Copilot

tableauserverclient/models/user_item.py

@@ -1,3 +1,4 @@
+import csv
 import io
 import warnings
 import xml.etree.ElementTree as ET
@@ -471,7 +472,7 @@ class ColumnType(IntEnum):
         def create_user_from_line(line: str):
             if line is None or line is False or line == "\n" or line == "":
                 return None
-            values: list[str] = list(map(str.strip, line.strip().split(",")))
+            values = UserItem.CSVImport._parse_csv_line(line)
             if len(values) > UserItem.CSVImport.COLUMN_COUNT:
                 raise ValueError("Too many attributes for user import")
             username = values[UserItem.CSVImport.ColumnType.USERNAME]
@@ -521,6 +522,13 @@ def create_user_from_line(line: str):
                 )
             return user
 
+        @staticmethod
+        def _parse_csv_line(line: str) -> list[str]:
+            values = next(csv.reader([line], strict=True), [])
+            if not values:
+                values = [""]
+            return list(map(str.strip, values))
+
         # Read through an entire CSV file meant for user import
         # Return the number of valid lines and a list of all the invalid lines
         @staticmethod
@@ -554,11 +562,17 @@ def _redact_password_column(line: str) -> str:
                 body, ending = line[:-1], "\n"
             else:
                 body, ending = line, ""
-            fields = body.split(",")
+            try:
+                fields = UserItem.CSVImport._parse_csv_line(body)
+            except csv.Error:
+                return "<invalid CSV row redacted>" + ending
             pass_index = UserItem.CSVImport.ColumnType.PASS.value
             if len(fields) > pass_index:
                 fields[pass_index] = "***"
-            return ",".join(fields) + ending
+                output = io.StringIO()
+                csv.writer(output, lineterminator="").writerow(fields)
+                return output.getvalue() + ending
+            return line
 
         # Some fields in the import file are restricted to specific values
         # Iterate through each field and validate the given value against hardcoded constraints
@@ -577,7 +591,7 @@ def _validate_import_line_or_throw(incoming, logger) -> None:
                 list(UserItem.CSVImport._AUTH_CANONICAL.values()),  # auth - normalized before comparison
             ]
 
-            line = list(map(str.strip, incoming.split(",")))
+            line = UserItem.CSVImport._parse_csv_line(incoming)
             if len(line) > UserItem.CSVImport.COLUMN_COUNT:
                 raise ValueError("Too many attributes for user import")
             username = line[UserItem.CSVImport.ColumnType.USERNAME.value]

test/test_user_model.py

@@ -203,25 +203,33 @@ def test_password_not_logged_when_line_invalid(caplog: pytest.LogCaptureFixture)
     assert secret not in combined, f"Password leaked into logs on invalid row: {combined!r}"
 
 
-def test_password_with_comma_partially_masks(caplog: pytest.LogCaptureFixture) -> None:
-    """A password containing commas is misaligned by the naive split parser: only the
-    portion that lands in column 1 gets masked. The remaining fragments still leak.
-    This documents the limitation — fully protecting passwords with embedded commas
-    requires a proper CSV parser — but confirms that the column-1 mask holds even
-    when the password value contains a comma."""
-    line = "jsmith,hunter2,SECRETTAIL,creator,site,yes,jsmith@example.com"
+def test_password_with_comma_is_fully_redacted(caplog: pytest.LogCaptureFixture) -> None:
+    """A quoted comma in a password must not leak from invalid row output or logs."""
+    line = 'jsmith,"hunter2,SECRETTAIL",John Smith,not-a-real-license,site,yes,jsmith@example.com'
+    test_data = _mock_file_content([line])
     with caplog.at_level(logging.DEBUG, logger=logger.name):
-        try:
-            TSC.UserItem.CSVImport._validate_import_line_or_throw(line, logger)
-        except Exception:
-            pass  # misaligned columns are expected to fail validation
+        valid, invalid = TSC.UserItem.CSVImport.validate_file_for_import(test_data, logger)
+    assert valid == 0
+    assert len(invalid) == 1
+    assert "hunter2" not in invalid[0]
+    assert "SECRETTAIL" not in invalid[0]
     combined = "\n".join(record.getMessage() for record in caplog.records)
-    # Column 1 ("hunter2") is masked; the fragment that spilled into column 2
-    # ("SECRETTAIL") is not — this is the documented limitation.
     assert "hunter2" not in combined
+    assert "SECRETTAIL" not in combined
     assert _mask_present(caplog.records)
 
 
+def test_malformed_csv_row_is_not_returned_unredacted(caplog: pytest.LogCaptureFixture) -> None:
+    secret = "SECRETTAIL"
+    test_data = _mock_file_content([f'jsmith,"{secret}'])
+    with caplog.at_level(logging.DEBUG, logger=logger.name):
+        valid, invalid = TSC.UserItem.CSVImport.validate_file_for_import(test_data, logger)
+    assert valid == 0
+    assert invalid == ["<invalid CSV row redacted>"]
+    combined = "\n".join(record.getMessage() for record in caplog.records)
+    assert secret not in combined
+
+
 def test_password_not_in_validate_attribute_value_error(
     caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch
 ) -> None:
@@ -310,6 +318,12 @@ def test_create_user_with_auth_column() -> None:
     assert user.auth_setting == "SAML", f"Expected SAML, got {user.auth_setting}"
 
 
+def test_create_user_with_comma_in_password() -> None:
+    user = TSC.UserItem.CSVImport.create_user_from_line('username,"pass,word",fname,creator,none,yes,email')
+    assert user is not None
+    assert user.fullname == "fname"
+
+
 def test_too_many_columns_raises() -> None:
     with pytest.raises(ValueError):
         TSC.UserItem.CSVImport.create_user_from_line("u, p, n, creator, none, yes, email, SAML, extra")