diff --git a/presidio-cli/README.md b/presidio-cli/README.md index 9547aeeef6..5b1f54821a 100644 --- a/presidio-cli/README.md +++ b/presidio-cli/README.md @@ -70,7 +70,7 @@ Configuration file supports the following parameters in a yaml file: - allow - list of tokens that should not be marked as PII. -- threshold - only show problems/findings whose scores are at or above this threshold. +- threshold - only show problems/findings whose scores are at or above this threshold. Must be a number between 0 and 1. Note: a file requires at least one parameter to be set. @@ -163,19 +163,19 @@ tests/conftest.py 37:33 0.85 PERSON ``` -- github - similar to diff function in github +- github - [GitHub Actions workflow commands](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands) that create a `warning` or `error` annotation for each finding ```shell presidio -d "entities: - PERSON" -f github tests/conftest.py # result ::group::tests/conftest.py -::0.85 file=tests/conftest.py,line=34,col=58::34:58 [PERSON] -::0.85 file=tests/conftest.py,line=37,col=33::37:33 [PERSON] +::warning file=tests/conftest.py,line=34,col=58::34:58 [PERSON] score=0.85 +::warning file=tests/conftest.py,line=37,col=33::37:33 [PERSON] score=0.85 ::endgroup:: ``` -- colored - standard output format but with colors +- colored - standard output format but with colors: error scores are red, warning scores are yellow - parsable - easy to parse automaticaly @@ -191,6 +191,31 @@ presidio -d "entities: - github, if run on github - environment variables `GITHUB_ACTIONS` and `GITHUB_WORKFLOW` are set - colored, otherwise +### Warnings and errors + +Each finding has a level based on its score: + +- error - the score is 1.0 +- warning - the score is below 1.0 + +Use `--no-warnings` to output only error-level findings: + +```shell +presidio --no-warnings tests/ +``` + +### Exit codes + +- `0` - no findings were output +- `1` - at least one finding was output, or the configuration is invalid +- `2` - invalid command-line arguments + +Findings filtered out by `threshold` or `--no-warnings` do not affect the exit code. To report findings without failing a CI step, ignore the exit code: + +```shell +presidio . || true +``` + ### List of all parameters Simply run the following to get a list of all available options for the CLI: diff --git a/presidio-cli/presidio_cli/analyzer.py b/presidio-cli/presidio_cli/analyzer.py index e593203282..47eb28e89b 100644 --- a/presidio-cli/presidio_cli/analyzer.py +++ b/presidio-cli/presidio_cli/analyzer.py @@ -61,6 +61,8 @@ def __init__(self, line: int, recognizer_result: RecognizerResult) -> None: self.type = self.recognizer_result["entity_type"] # Score as a probability determined by the model self.score = self.recognizer_result["score"] + #: Severity: "error" for a full-confidence finding, "warning" otherwise + self.level = "error" if self.score >= 1.0 else "warning" def _analyze( diff --git a/presidio-cli/presidio_cli/cli.py b/presidio-cli/presidio_cli/cli.py index e9cae6ea63..aea1e2c72d 100644 --- a/presidio-cli/presidio_cli/cli.py +++ b/presidio-cli/presidio_cli/cli.py @@ -53,7 +53,7 @@ def standard_color(problem: PIIProblem) -> str: """ line = " \033[2m%d:%d\033[0m" % (problem.line, problem.column) line += max(20 - len(line), 0) * " " - if problem.score < 1: # warning + if problem.level == "warning": line += "\033[33m%s\033[0m" % problem.score else: line += "\033[31m%s\033[0m" % problem.score @@ -66,19 +66,34 @@ def standard_color(problem: PIIProblem) -> str: @staticmethod def github(problem: PIIProblem, filename: str) -> str: """ - Output the problem in git-diff-like format. + Output the problem as a GitHub Actions warning or error workflow command. :param problem: PIIProblem to be formatted. :param filename: Filename where the problem occurs. + :return: Workflow command that creates an annotation for the problem. """ - line = ( - f"::{str(problem.score)} file={filename},line={format(problem.line)}," - + f"col={format(problem.column)}::{format(problem.line)}" - + f":{format(problem.column)} [{problem.type}]" - ) + message = f"{problem.line}:{problem.column} [{problem.type}]" + message += f" score={problem.score}" if problem.explanation: - line += problem.explanation - return line + message += f" ({problem.explanation})" + # Drop the ./ that `presidio .` adds, as run() does before analyze() + if filename.startswith(("./", ".\\")): + filename = filename[2:] + file = _escape_github_property(filename) + return ( + f"::{problem.level} file={file},line={problem.line},col={problem.column}" + f"::{_escape_github_data(message)}" + ) + + +def _escape_github_data(value: str) -> str: + """Escape the message of a GitHub Actions workflow command.""" + return value.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + + +def _escape_github_property(value: str) -> str: + """Escape a property value of a GitHub Actions workflow command.""" + return _escape_github_data(value).replace(":", "%3A").replace(",", "%2C") def threshold_value(value: str) -> float: @@ -113,7 +128,7 @@ def show_problems( file: str, args_format: str, no_warn: bool, -): +) -> int: """ Show formatted output of discovered problems. @@ -121,8 +136,9 @@ def show_problems( :param file: processed filename for 'stdin' :param args_format: format in which to output discovered problems :param no_warn: whether to output only error level problems + :return: number of problems that were output """ - max_level = 0 + prob_num = 0 first = True if args_format == "auto": @@ -134,11 +150,12 @@ def show_problems( for problem in problems: if no_warn and (problem.level != "error"): continue + prob_num += 1 if args_format == "parsable": print(Format.parsable(problem)) elif args_format == "github": if first: - print("::group::%s" % file) + print("::group::%s" % _escape_github_data(file)) first = False print(Format.github(problem, file)) elif args_format == "colored": @@ -158,7 +175,7 @@ def show_problems( if not first and args_format != "parsable": print("") - return max_level + return prob_num def find_files_recursively( @@ -269,7 +286,7 @@ def run() -> None: except Exception: traceback.print_exc() continue - prob_num = show_problems( + prob_num += show_problems( problems, file, args_format=args.format, no_warn=args.no_warnings ) @@ -279,7 +296,7 @@ def run() -> None: except EnvironmentError as e: print(e, file=sys.stderr) sys.exit(1) - prob_num = show_problems( + prob_num += show_problems( problems, "stdin", args_format=args.format, diff --git a/presidio-cli/presidio_cli/config.py b/presidio-cli/presidio_cli/config.py index 1e6ac337e1..9b3d60a861 100644 --- a/presidio-cli/presidio_cli/config.py +++ b/presidio-cli/presidio_cli/config.py @@ -100,12 +100,22 @@ def parse(self, raw_content: str) -> None: self.entities = self.analyzer.get_supported_entities() if "threshold" in conf: - if not 0 <= float(self.threshold) <= 1: + try: + # YAML loads true/yes/on as booleans, which float() would accept + if isinstance(conf["threshold"], bool): + raise TypeError("threshold is a boolean") + threshold = float(conf["threshold"]) + except (TypeError, ValueError, OverflowError) as e: + raise PresidioCLIConfigError( + f"Invalid threshold value: {conf['threshold']}. " + "Threshold must be a number" + ) from e + if not 0 <= threshold <= 1: raise PresidioCLIConfigError( - f"Invalid threshold value: {self.threshold}. " + f"Invalid threshold value: {conf['threshold']}. " f"Threshold must be between 0 and 1" ) - self.threshold = float(conf["threshold"]) + self.threshold = threshold if "allow" in conf: self.allow_list = conf["allow"] if "language" in conf: diff --git a/presidio-cli/tests/test_analyzer.py b/presidio-cli/tests/test_analyzer.py index ebdb2ca4d5..5fa2d9acba 100644 --- a/presidio-cli/tests/test_analyzer.py +++ b/presidio-cli/tests/test_analyzer.py @@ -1,5 +1,6 @@ import pytest -from presidio_cli.analyzer import analyze, line_generator +from presidio_analyzer import RecognizerResult +from presidio_cli.analyzer import PIIProblem, analyze, line_generator def test_line_generator(): @@ -59,3 +60,13 @@ def test_analyze_with_allow_list(en_core_web_lg, config, config_with_allow_list) def test_analyze_type_error(en_core_web_lg, config): with pytest.raises(TypeError): analyze({}, config) + + +@pytest.mark.parametrize( + ("score", "level"), + [(1.0, "error"), (0.99, "warning"), (0.85, "warning"), (0.0, "warning")], +) +def test_pii_problem_level_is_error_only_for_full_confidence(score, level): + problem = PIIProblem(1, RecognizerResult("PERSON", 0, 5, score)) + + assert problem.level == level diff --git a/presidio-cli/tests/test_cli.py b/presidio-cli/tests/test_cli.py index 08f6a5383e..5adafa4de2 100644 --- a/presidio-cli/tests/test_cli.py +++ b/presidio-cli/tests/test_cli.py @@ -2,28 +2,19 @@ import os import pytest from io import StringIO +from presidio_analyzer import RecognizerResult from presidio_cli import cli +from presidio_cli.analyzer import PIIProblem from presidio_cli.config import PresidioCLIConfig as RealPresidioCLIConfig @pytest.fixture() -def problems(mocker): - problem1 = mocker.Mock( - line=1, - column=7, - score=1.0, - type="PERSON", - explanation=None, - recognizer_result={}, - ) - problem2 = mocker.Mock( - line=2, - column=17, - score=0.85, - type="PERSON", - explanation="some example", - recognizer_result={}, - ) +def problems(): + problem1 = PIIProblem(1, RecognizerResult("CREDIT_CARD", 6, 25, 1.0)) + problem2 = PIIProblem(2, RecognizerResult("PERSON", 16, 26, 0.85)) + # The analyzer only returns explanations when asked for its decision + # process, so set one here to exercise the formatters' explanation branch. + problem2.explanation = "some example" return [problem1, problem2] @@ -73,7 +64,61 @@ def test_show_problems(arg_format, problems): filepath = "./example.txt" rc = cli.show_problems(problems, filepath, arg_format, False) + assert rc == 2 + + +@pytest.mark.parametrize("arg_format", ["standard", "colored", "github", "parsable"]) +def test_show_problems_no_warnings_outputs_only_errors(arg_format, problems, capsys): + rc = cli.show_problems(problems, "example.txt", arg_format, True) + + out = capsys.readouterr().out + assert rc == 1 + assert "CREDIT_CARD" in out + assert "PERSON" not in out + + +def test_show_problems_no_warnings_without_errors_returns_zero(problems, capsys): + rc = cli.show_problems(problems[1:], "example.txt", "standard", True) + assert rc == 0 + assert capsys.readouterr().out == "" + + +def test_standard_color_marks_errors_red_and_warnings_yellow(problems): + error, warning = problems + + assert "\033[31m1.0\033[0m" in cli.Format.standard_color(error) + assert "\033[33m0.85\033[0m" in cli.Format.standard_color(warning) + + +def test_github_format_emits_annotation_commands(problems, capsys): + cli.show_problems(problems, "dir/a,b:c%.txt", "github", False) + + assert capsys.readouterr().out.splitlines() == [ + "::group::dir/a,b:c%25.txt", + "::error file=dir/a%2Cb%3Ac%25.txt,line=1,col=7::1:7 [CREDIT_CARD] score=1.0", + "::warning file=dir/a%2Cb%3Ac%25.txt,line=2,col=17::" + "2:17 [PERSON] score=0.85 (some example)", + "::endgroup::", + "", + ] + + +def test_github_format_escapes_message_data(): + problem = PIIProblem(1, RecognizerResult("PERSON", 0, 5, 0.5)) + problem.explanation = "50%\r\nsure" + + assert cli.Format.github(problem, "a\nb.txt") == ( + "::warning file=a%0Ab.txt,line=1,col=1::" + "1:1 [PERSON] score=0.5 (50%25%0D%0Asure)" + ) + + +@pytest.mark.parametrize("filename", ["./example.txt", ".\\example.txt"]) +def test_github_format_drops_current_dir_prefix(problems, filename): + assert cli.Format.github(problems[0], filename).startswith( + "::error file=example.txt,line=1,col=7::" + ) def test_show_problems_auto_gh(problems, monkeypatch): @@ -82,7 +127,7 @@ def test_show_problems_auto_gh(problems, monkeypatch): filepath = "./example.txt" rc = cli.show_problems(problems, filepath, "auto", False) - assert rc == 0 + assert rc == 2 def test_show_problems_auto_color(problems, monkeypatch, mocker): @@ -90,7 +135,7 @@ def test_show_problems_auto_color(problems, monkeypatch, mocker): mocker.patch("sys.stdout") filepath = "./example.txt" rc = cli.show_problems(problems, filepath, "auto", False) - assert rc == 0 + assert rc == 2 def test_run_current_dir(temp_workspace, mocker): @@ -98,7 +143,7 @@ def test_run_current_dir(temp_workspace, mocker): mocker.patch("sys.argv", ["", "."]) ec = mocker.patch("sys.exit") cli.run() - ec.assert_called_once_with(0) + ec.assert_called_once_with(1) def test_run_with_config(temp_workspace, mocker): @@ -109,7 +154,62 @@ def test_run_with_config(temp_workspace, mocker): mocker.patch("sys.argv", ["-c", ".presidiocli", "."]) ec = mocker.patch("sys.exit") cli.run() - ec.assert_called_once_with(0) + ec.assert_called_once_with(1) + + +def test_run_no_warnings_reports_only_errors( + temp_workspace, mocker, monkeypatch, capsys +): + monkeypatch.chdir(temp_workspace) + mocker.patch("sys.argv", ["", "--no-warnings", "-f", "standard", "."]) + ec = mocker.patch("sys.exit") + + cli.run() + + out = capsys.readouterr().out + assert "CREDIT_CARD" in out + assert "PERSON" not in out + ec.assert_called_once_with(1) + + +def test_run_exit_code_counts_problems_in_every_file( + mocker, problems, tmp_path, monkeypatch +): + (tmp_path / "a.txt").write_text("a") + (tmp_path / "b.txt").write_text("b") + monkeypatch.chdir(tmp_path) + mocked_args = mocker.Mock(**make_args(files=("a.txt", "b.txt"), format="standard")) + mocker.patch("presidio_cli.cli.PresidioCLIConfig", return_value=make_conf(mocker)) + mocker.patch( + "presidio_cli.cli.find_files_recursively", return_value=["a.txt", "b.txt"] + ) + mocker.patch("presidio_cli.cli.analyze", side_effect=[problems, []]) + mocker.patch("argparse.ArgumentParser.parse_args", return_value=mocked_args) + ec = mocker.patch("sys.exit") + + cli.run() + + ec.assert_called_once_with(1) + + +@pytest.mark.parametrize(("no_warnings", "exit_code"), [(False, 1), (True, 0)]) +def test_run_exit_code_ignores_problems_hidden_by_no_warnings( + mocker, problems, tmp_path, monkeypatch, no_warnings, exit_code +): + (tmp_path / "a.txt").write_text("a") + monkeypatch.chdir(tmp_path) + mocked_args = mocker.Mock( + **make_args(files=("a.txt",), format="standard", no_warnings=no_warnings) + ) + mocker.patch("presidio_cli.cli.PresidioCLIConfig", return_value=make_conf(mocker)) + mocker.patch("presidio_cli.cli.find_files_recursively", return_value=["a.txt"]) + mocker.patch("presidio_cli.cli.analyze", return_value=problems[1:]) + mocker.patch("argparse.ArgumentParser.parse_args", return_value=mocked_args) + ec = mocker.patch("sys.exit") + + cli.run() + + ec.assert_called_once_with(exit_code) def test_run_preserves_config_threshold_when_flag_is_omitted(mocker): @@ -213,3 +313,15 @@ def test_run_with_stdin(mocker): ec = mocker.patch("sys.exit") cli.run() ec.assert_called_once_with(0) + + +def test_run_with_stdin_exits_with_one_when_problems_are_found(mocker, problems): + mocked_args = mocker.Mock(**make_args(stdin=True, files=(), format="standard")) + mocker.patch("presidio_cli.cli.PresidioCLIConfig", return_value=make_conf(mocker)) + mocker.patch("presidio_cli.cli.find_files_recursively", return_value=[]) + mocker.patch("presidio_cli.cli.analyze", return_value=problems) + mocker.patch("argparse.ArgumentParser.parse_args", return_value=mocked_args) + mocker.patch("sys.stdin", StringIO("Example input")) + ec = mocker.patch("sys.exit") + cli.run() + ec.assert_called_once_with(1) diff --git a/presidio-cli/tests/test_config.py b/presidio-cli/tests/test_config.py index 7d78ae7977..f5d2f7bae2 100644 --- a/presidio-cli/tests/test_config.py +++ b/presidio-cli/tests/test_config.py @@ -61,3 +61,30 @@ def test_run_with_ignored_path(temp_workspace): assert new.is_file_ignored("./dos.yml") assert new.is_file_ignored("./.git/hooks/README.sample") assert not new.is_file_ignored("notignored") + + +@pytest.mark.parametrize( + ("value", "message"), + [ + ("5", "Invalid threshold value: 5. Threshold must be between 0 and 1"), + ("-0.1", "Invalid threshold value: -0.1. Threshold must be between 0 and 1"), + (".nan", "Invalid threshold value: nan. Threshold must be between 0 and 1"), + ("abc", "Invalid threshold value: abc. Threshold must be a number"), + ("[]", "Invalid threshold value: []. Threshold must be a number"), + ("true", "Invalid threshold value: True. Threshold must be a number"), + pytest.param( + "1" + "0" * 400, + f"Invalid threshold value: 1{'0' * 400}. Threshold must be a number", + id="int-too-large-for-float", + ), + ], +) +def test_invalid_threshold_raises_config_error(value, message): + with pytest.raises(config.PresidioCLIConfigError) as excinfo: + config.PresidioCLIConfig(f"threshold: {value}\n") + assert str(excinfo.value) == message + + +@pytest.mark.parametrize(("value", "expected"), [("0", 0.0), ("0.7", 0.7), ("1", 1.0)]) +def test_threshold_is_read_from_config(value, expected): + assert config.PresidioCLIConfig(f"threshold: {value}\n").threshold == expected