Redact password column from CSV import logging and error output

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,26 +522,70 @@ def create_user_from_line(line: str):
                 )
             return user
 
+        @staticmethod
+        def _parse_csv_line(line: str) -> list[str]:
+            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
             line: str = csv_file.readline()
             while line and line != "":
+                # Log only the username (column 0); the rest of the line contains the password (column 1) and other PII.
+                username = line.partition(",")[0].strip()
                 try:
-                    # do not print passwords
-                    logger.info(f"Reading user {line[:4]}")
+                    logger.debug(f"Reading user {username}")
                     UserItem.CSVImport._validate_import_line_or_throw(line, logger)
                     num_valid_lines += 1
                 except Exception as exc:
-                    logger.info(f"Error parsing {line[:4]}: {exc}")
-                    invalid_lines.append(line)
+                    logger.debug(f"Error parsing user {username}: {exc}")
+                    invalid_lines.append(line if include_passwords else UserItem.CSVImport._mask_password_columns(line))
                 line = csv_file.readline()
             return num_valid_lines, invalid_lines
 
+        # 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
+
         # Some fields in the import file are restricted to specific values
         # Iterate through each field and validate the given value against hardcoded constraints
         @staticmethod
@@ -558,16 +603,16 @@ 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]
             logger.debug(f"> details - {username}")
             UserItem.validate_username_or_throw(username)
             for i in range(1, len(line)):
+                column = UserItem.CSVImport.ColumnType(i)
                 value = line[i]
                 valid = _valid_attributes[i]
-                column = UserItem.CSVImport.ColumnType(i)
                 # normalize case for fields with a restricted value set
                 skip_validation = False
                 if valid:
@@ -591,9 +636,20 @@ def _validate_import_line_or_throw(incoming, logger) -> None:
                             skip_validation = True
                     else:
                         value = value.lower()
-                logger.debug(f"column {column.name}: {value}")
-                if not skip_validation:
-                    UserItem.CSVImport._validate_attribute_value(value, valid, column)
+                # A malformed row can shift password fragments into later columns;
+                # keep every field at or after PASS out of the logs.
+                safe_value = "***" if UserItem.CSVImport._is_sensitive_column(column.value) else value
+                logger.debug(f"column {column.name}: {safe_value}")
+                # 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.
+                if not skip_validation and column != UserItem.CSVImport.ColumnType.PASS:
+                    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

@@ -171,6 +171,138 @@ def test_validate_usernames_file() -> None:
     assert valid == 5, f"Exactly 5 of the lines were valid, counted {valid + len(invalid)}"
 
 
+def _mask_present(records: list) -> bool:
+    combined = "\n".join(record.getMessage() for record in records)
+    return "PASS" in combined and "***" in combined
+
+
+def test_password_not_logged_at_debug(caplog: pytest.LogCaptureFixture) -> None:
+    """Regression test for #1829: passwords must not appear in DEBUG logs."""
+    secret = "hunter2SUPERSECRET"
+    line = f"jsmith,{secret},John Smith,creator,site,yes,jsmith@example.com"
+    with caplog.at_level(logging.DEBUG, logger=logger.name):
+        TSC.UserItem.CSVImport._validate_import_line_or_throw(line, logger)
+    combined = "\n".join(record.getMessage() for record in caplog.records)
+    assert secret not in combined, f"Password leaked into logs: {combined!r}"
+    # Positive assertion: something references the PASS column and something is
+    # masked as ***, so a "fix" that only removed the log line would not pass.
+    assert _mask_present(caplog.records), f"Expected masked PASS log line; got: {combined!r}"
+
+
+def test_password_not_logged_when_line_invalid(caplog: pytest.LogCaptureFixture) -> None:
+    """Regression test for #1829: passwords must not appear when a row fails to validate."""
+    secret = "hunter2SUPERSECRET"
+    line = f"jsmith,{secret},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 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_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 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_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
+    # 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(
+    caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Regression test: _validate_import_line_or_throw must never call
+    _validate_attribute_value for the PASS column. Today PASS has an empty
+    allowlist so the validator returns early, but if a future PR adds
+    password-format checks (length, complexity, banned chars) the raw password
+    would leak through ValueError("Invalid value {item} for {column_type}").
+    Guarantee the invariant at the caller so it doesn't depend on the allowlist
+    staying empty."""
+    secret = "hunter2SUPERSECRET"
+    line = f"jsmith,{secret},John Smith,creator,site,yes,jsmith@example.com"
+
+    # Spy that records every call and simulates a future PR that added a
+    # non-empty allowlist to PASS (any value not in the allowlist raises).
+    calls: list[tuple] = []
+    original = TSC.UserItem.CSVImport._validate_attribute_value
+
+    def spy(item: str, possible_values: list, column_type) -> None:
+        calls.append((item, column_type))
+        if column_type == TSC.UserItem.CSVImport.ColumnType.PASS:
+            # Simulate a hypothetical password-format check.
+            raise ValueError(f"Invalid value {item} for {column_type}")
+        return original(item, possible_values, column_type)
+
+    monkeypatch.setattr(TSC.UserItem.CSVImport, "_validate_attribute_value", spy)
+
+    with caplog.at_level(logging.DEBUG, logger=logger.name):
+        # If PASS were passed to the validator, the spy would raise with the
+        # raw secret embedded in the ValueError. It must not raise.
+        TSC.UserItem.CSVImport._validate_import_line_or_throw(line, logger)
+
+    pass_calls = [c for c in calls if c[1] == TSC.UserItem.CSVImport.ColumnType.PASS]
+    assert not pass_calls, f"_validate_attribute_value was called for PASS: {pass_calls!r}"
+
+    combined = "\n".join(record.getMessage() for record in caplog.records)
+    assert secret not in combined, f"Password leaked into logs: {combined!r}"
+
+
 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)
@@ -199,6 +331,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")