diff --git a/tableauserverclient/models/user_item.py b/tableauserverclient/models/user_item.py index 85f0d96f1..71c3a67ff 100644 --- a/tableauserverclient/models/user_item.py +++ b/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 diff --git a/test/test_user_model.py b/test/test_user_model.py index ccb5cedc6..2e1e4d2fc 100644 --- a/test/test_user_model.py +++ b/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")