Mask password columns in invalid import lines, fall back to comma split for malformed rows

Invalid lines returned by validate_file_for_import now have the password
column and the one after it masked, so a password containing an unquoted
comma does not expose its fragment. include_passwords=True returns the
lines as written in the file.

Rows that are not well-formed CSV fall back to a plain comma split instead
of raising csv.Error, so they keep importing.

Written with Claude Code

Author: jacalata

tableauserverclient/models/user_item.py

@@ -524,15 +524,25 @@ def create_user_from_line(line: str):
 
         @staticmethod
         def _parse_csv_line(line: str) -> list[str]:
-            values = next(csv.reader([line], strict=True, skipinitialspace=True), [])
+            try:
+                values = next(csv.reader([line], strict=True, skipinitialspace=True), [])
+            except csv.Error:
+                # Not well-formed CSV (e.g. an unmatched quote in a password). Split on
+                # commas as unquoted files always have, so these rows keep importing.
+                values = line.strip().split(",")
             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
+        # Return the number of valid lines and a list of all the invalid lines.
+        # Invalid lines have the password column and the column after it masked with "***"
+        # (a password containing one unquoted comma spills into the next column);
+        # pass include_passwords=True to get them exactly as they appear in the file.
         @staticmethod
-        def validate_file_for_import(csv_file: io.TextIOWrapper, logger) -> tuple[int, list[str]]:
+        def validate_file_for_import(
+            csv_file: io.TextIOWrapper, logger, include_passwords: bool = False
+        ) -> tuple[int, list[str]]:
             num_valid_lines = 0
             invalid_lines = []
             csv_file.seek(0)  # set to start of file in case it has been read earlier
@@ -546,44 +556,36 @@ def validate_file_for_import(csv_file: io.TextIOWrapper, logger) -> tuple[int, l
                     num_valid_lines += 1
                 except Exception as exc:
                     logger.debug(f"Error parsing user {username}: {exc}")
-                    invalid_lines.append(UserItem.CSVImport._redact_password_column(line))
+                    invalid_lines.append(line if include_passwords else UserItem.CSVImport._mask_password_columns(line))
                 line = csv_file.readline()
             return num_valid_lines, invalid_lines
 
-        # Single source of truth for "is this column (or, for a malformed row that
-        # shifted fields, anything at or after it) sensitive enough to mask before
-        # it reaches a log handler or an exposed invalid-lines entry." Both
-        # _redact_password_column (whole-line redaction) and
-        # _validate_import_line_or_throw (per-column debug logging) key off this
-        # instead of each re-deriving the PASS boundary independently.
+        # Number of columns masked in returned invalid lines, starting at the password column.
+        _MASKED_COLUMN_COUNT = 2
+
+        # Return the line with the password column and the one after it replaced by "***".
+        # Other fields are re-serialized (whitespace trimmed, quoting normalized); the line ending is kept.
+        @staticmethod
+        def _mask_password_columns(line: str) -> str:
+            body = line.rstrip("\r\n")
+            ending = line[len(body) :]
+            fields = UserItem.CSVImport._parse_csv_line(body)
+            first = UserItem.CSVImport.ColumnType.PASS.value
+            if len(fields) <= first:
+                return line
+            for i in range(first, min(first + UserItem.CSVImport._MASKED_COLUMN_COUNT, len(fields))):
+                fields[i] = "***"
+            output = io.StringIO()
+            csv.writer(output, lineterminator="").writerow(fields)
+            return output.getvalue() + ending
+
+        # True for the password column and every column after it. A row whose password
+        # contains an unquoted comma shifts password fragments into later columns, so
+        # all of them are kept out of log output.
         @staticmethod
         def _is_sensitive_column(column_index: int) -> bool:
             return column_index >= UserItem.CSVImport.ColumnType.PASS.value
 
-        # Return a copy of a raw CSV line with the password column replaced by "***".
-        # Callers that log or expose invalid rows will not disclose the credential.
-        # Preserves the original line ending (\r\n or \n) so log output and
-        # returned rows stay byte-identical apart from the redaction.
-        @staticmethod
-        def _redact_password_column(line: str) -> str:
-            if line.endswith("\r\n"):
-                body, ending = line[:-2], "\r\n"
-            elif line.endswith("\n"):
-                body, ending = line[:-1], "\n"
-            else:
-                body, ending = line, ""
-            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] = "***"
-                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
         @staticmethod
@@ -641,10 +643,13 @@ def _validate_import_line_or_throw(incoming, logger) -> None:
                 # Never call _validate_attribute_value for PASS: its ValueError
                 # embeds the raw value, and a future PR that adds password-format
                 # checks to PASS's allowlist would leak the password through the
-                # exception message. Belt-and-braces even though PASS's allowlist
-                # is currently empty.
+                # exception message.
                 if not skip_validation and column != UserItem.CSVImport.ColumnType.PASS:
-                    UserItem.CSVImport._validate_attribute_value(value, valid, column)
+                    try:
+                        UserItem.CSVImport._validate_attribute_value(value, valid, column)
+                    except ValueError:
+                        # The rejected value may be a password fragment from a shifted row.
+                        raise ValueError(f"Invalid value in column {column.name}") from None
 
         # Given a restricted set of possible values, confirm the item is in that set
         @staticmethod

test/test_user_model.py

@@ -197,37 +197,71 @@ def test_password_not_logged_when_line_invalid(caplog: pytest.LogCaptureFixture)
     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 len(invalid) == 1
-    assert secret not in invalid[0], f"Password leaked into returned invalid_lines: {invalid[0]!r}"
+    assert invalid == ["jsmith,***,***,not-a-real-license,site,yes,jsmith@example.com"]
     combined = "\n".join(record.getMessage() for record in caplog.records)
     assert secret not in combined, f"Password leaked into logs on invalid row: {combined!r}"
 
 
-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."""
+def test_quoted_comma_password_not_logged(caplog: pytest.LogCaptureFixture) -> None:
+    """A quoted comma in a password must not reach the logs; the invalid row is returned as written."""
     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):
         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]
+    assert invalid == ["jsmith,***,***,not-a-real-license,site,yes,jsmith@example.com"]
     combined = "\n".join(record.getMessage() for record in caplog.records)
     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}'])
+def test_include_passwords_returns_invalid_lines_verbatim() -> None:
+    line = "jsmith,hunter2,John Smith,not-a-real-license,site,yes,jsmith@example.com\n"
+    valid, invalid = TSC.UserItem.CSVImport.validate_file_for_import(
+        _mock_file_content([line]), logger, include_passwords=True
+    )
+    assert valid == 0
+    assert invalid == [line]
+
+
+def test_mask_password_columns_helper() -> None:
+    mask = TSC.UserItem.CSVImport._mask_password_columns
+    assert mask("jsmith,hunter2,fname,creator\n") == "jsmith,***,***,creator\n"
+    assert mask("jsmith,hunter2,fname,creator\r\n") == "jsmith,***,***,creator\r\n"
+    assert mask("jsmith,hunter2,fname") == "jsmith,***,***"
+    assert mask("jsmith,hunter2\r\n") == "jsmith,***\r\n"
+    assert mask("jsmith,,fname") == "jsmith,***,***"
+    assert mask("jsmith,") == "jsmith,***"
+    assert mask('jsmith,"pass,word",fname,creator') == "jsmith,***,***,creator"
+    # No password column: returned unchanged
+    assert mask("jsmith") == "jsmith"
+    assert mask("jsmith\n") == "jsmith\n"
+
+
+def test_unmatched_quote_row_still_parses() -> None:
+    """A row that is not well-formed CSV is split on commas instead of aborting the import."""
+    user = TSC.UserItem.CSVImport.create_user_from_line('username,pa"ss,fname,creator,none,yes,email')
+    assert user is not None
+    assert user.fullname == "fname"
+    user = TSC.UserItem.CSVImport.create_user_from_line('username,"unterminated,fname,creator,none,yes,email')
+    assert user is not None
+    assert user.fullname == "fname"
+
+
+def test_shifted_password_fragment_not_logged(caplog: pytest.LogCaptureFixture) -> None:
+    """An unquoted comma shifts password fragments into later columns; they must not reach the logs."""
+    line = "jsmith,hunter2,SECRETTAIL,TAILFRAGMENT,site,yes,jsmith@example.com"
+    test_data = _mock_file_content([line])
     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
+    # Only two columns are masked, so a fragment past an unquoted second comma stays in the returned row.
+    assert invalid == ["jsmith,***,***,TAILFRAGMENT,site,yes,jsmith@example.com"]
+    combined = " ".join(record.getMessage() for record in caplog.records).lower()
+    assert "tailfragment" not in combined
+    assert "secrettail" not in combined
+    assert "column license" in combined
 
 
 def test_password_not_in_validate_attribute_value_error(
@@ -269,27 +303,6 @@ def spy(item: str, possible_values: list, column_type) -> None:
     assert secret not in combined, f"Password leaked into logs: {combined!r}"
 
 
-def test_redact_password_column_helper() -> None:
-    """Unit-level coverage for _redact_password_column across newline and edge cases."""
-    redact = TSC.UserItem.CSVImport._redact_password_column
-    # LF-terminated
-    assert redact("jsmith,hunter2,fname\n") == "jsmith,***,fname\n"
-    # CRLF-terminated (the \r rides with the last field, ending is preserved)
-    assert redact("jsmith,hunter2,fname\r\n") == "jsmith,***,fname\r\n"
-    # CRLF where password IS the last field: the \r must not be silently
-    # dropped when the password value is replaced.
-    assert redact("jsmith,hunter2\r\n") == "jsmith,***\r\n"
-    # No trailing newline
-    assert redact("jsmith,hunter2,fname") == "jsmith,***,fname"
-    # Empty password field: still replaced (unconditional mask)
-    assert redact("jsmith,,fname") == "jsmith,***,fname"
-    # Trailing comma with nothing after: column 1 exists as empty string, gets masked
-    assert redact("jsmith,") == "jsmith,***"
-    # Single column: no password to redact; return line unchanged
-    assert redact("jsmith") == "jsmith"
-    assert redact("jsmith\n") == "jsmith\n"
-
-
 def test_validate_mixed_case_license() -> None:
     # Regression: issue #1809 - 'Viewer' (capital V) was rejected by case-sensitive check
     TSC.UserItem.CSVImport._validate_import_line_or_throw("username, pword, fname, Viewer, None, no, email", logger)