Redact password column from CSV import logging and error output - #1862
Open
jacalata wants to merge 1 commit into
Open
Redact password column from CSV import logging and error output#1862jacalata wants to merge 1 commit into
jacalata wants to merge 1 commit into
Conversation
`UserItem.CSVImport.validate_file_for_import` and `_validate_import_line_or_throw` wrote the raw CSV line -- including the password column -- to any caller-supplied logger at INFO/DEBUG level, and the whole raw line was pushed into the `invalid_lines` list returned to callers when a row failed validation. Anyone using the sample logger config or forwarding logs to a centralized system would see clear-text passwords in the log stream. Changes: - `validate_file_for_import` logs only the username (column 0) at DEBUG, and calls a new `_redact_password_column` helper before appending an invalid row to the returned list. - `_validate_import_line_or_throw` masks the PASS column value as `***` before logging it. Other column values still logged as-is for debugging. - Both callers changed from INFO to DEBUG for these per-row messages; large imports were spamming operator-visible logs. - Two regression tests capture logs and returned invalid_lines to assert the secret never appears in either place, plus a positive assertion that a `***` masked value IS logged so a future refactor that just removes the log line entirely doesn't pass. Fixes #1829. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens UserItem.CSVImport to prevent accidental disclosure of the CSV password column via logging and error/invalid-line output, addressing security issue #1829.
Changes:
- Restrict row-level import logs to username-only and emit them at DEBUG level.
- Redact the password column (
PASS) when returninginvalid_lines, and mask thePASSvalue in per-column DEBUG logs. - Add regression/unit tests covering log redaction behavior and helper edge cases; document the embedded-comma limitation; update changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tableauserverclient/models/user_item.py | Adds password redaction helper and updates CSV import logging/invalid-line handling to avoid password disclosure. |
| test/test_user_model.py | Adds regression tests ensuring passwords do not appear in DEBUG logs or invalid row output; adds unit tests for redaction helper behavior. |
| CHANGELOG.md | Documents the security fix for CSV import password logging/output behavior (Fixes #1829). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+497
to
+502
| trailing_newline = "\n" if line.endswith("\n") else "" | ||
| fields = line.rstrip("\n").split(",") | ||
| pass_index = UserItem.CSVImport.ColumnType.PASS.value | ||
| if len(fields) > pass_index: | ||
| fields[pass_index] = "***" | ||
| return ",".join(fields) + trailing_newline |
Comment on lines
+197
to
+200
| # CRLF-terminated (the \r rides with the last field, ending is preserved) | ||
| assert redact("jsmith,hunter2,fname\r\n") == "jsmith,***,fname\r\n" | ||
| # No trailing newline | ||
| assert redact("jsmith,hunter2,fname") == "jsmith,***,fname" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two paths in
UserItem.CSVImportthat could leak the password column from a user-import CSV file:_validate_import_line_or_throwloggedlogger.info(f"Reading user {line[:4]}"). The intent was to avoid printing the password, but slicing four characters only obscures usernames shorter than four characters -- longer usernames leak part of the row, and if the layout ever changed the password itself could reach the log.validate_file_for_importreturned raw lines in itsinvalid_lineslist when a row failed validation. Any caller that logged or surfaced those lines would expose the password unmasked._validate_import_line_or_throwprinted the PASS column value verbatim.Fixes #1829.
What changes
invalid_linesentries pass through a new_redact_password_columnhelper that replaces column 1 with***, preserving the line's original line ending.***.The redaction helper uses the same naive
str.split(",")parsing already used throughout the file. A password value containing commas is misaligned across columns; only the fragment landing in column 1 is masked. A test documents this limitation -- a proper CSV parser would be a larger change and is out of scope here.Test plan
test/test_user_model.pycovering: DEBUG log on a valid row (positive-and-negative assertion so a fix that only deletes the log line would fail), invalid-row path (both the returnedinvalid_linesand the logger output), embedded-comma limitation, and unit-level edges on_redact_password_column(LF, CRLF, no newline, empty password, trailing comma, single column).🤖 Generated with Claude Code