Fix unnecessarily and erroneous whitespace changes when using --anonymize-passwords and --sensitive-words

[cdtomkins]

Closes #213.

Fixes unnecessarily and erroneous whitespace changes when using --anonymize-passwords and/or --sensitive-words.

---

This change is 

[cdtomkins]

@ratulm / @dhalperi I think this solves #213 - please take a look.

[dhalperi]

@dhalperi reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

---

a discussion (no related file):
Please add new tests that failed before and pass now.

[cdtomkins]

Please add new tests that failed before and pass now.

Done!

Before:

=========================== short test summary info ============================
FAILED tests/unit/test_sensitive_item_removal.py::test_pwd_removal_preserve_single_inline_whitespace[  ]
FAILED tests/unit/test_sensitive_item_removal.py::test_pwd_removal_preserve_single_inline_whitespace[   ]
FAILED tests/unit/test_sensitive_item_removal.py::test_pwd_removal_preserve_single_inline_whitespace[          ]
FAILED tests/unit/test_sensitive_item_removal.py::test_pwd_removal_preserve_multiple_inline_whitespace[  ]
FAILED tests/unit/test_sensitive_item_removal.py::test_pwd_removal_preserve_multiple_inline_whitespace[   ]
FAILED tests/unit/test_sensitive_item_removal.py::test_pwd_removal_preserve_multiple_inline_whitespace[          ]
FAILED tests/unit/test_sensitive_item_removal.py::test_pwd_removal_preserve_different_inline_whitespace[ -  ]
FAILED tests/unit/test_sensitive_item_removal.py::test_pwd_removal_preserve_different_inline_whitespace[    -   ]
FAILED tests/unit/test_sensitive_item_removal.py::test_pwd_removal_preserve_different_inline_whitespace[      -       ]
========== 9 failed, 2443 passed, 15 skipped, 588 warnings in 11.79s ===========

After

=================================== 2452 passed, 15 skipped in 9.90s ===================================

[dhalperi]

@dhalperi reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @cdtomkins)

---

tests/unit/test_sensitive_item_removal.py line 655 at r3 (raw file):

    pwd_lookup = {}
    processed_line = replace_matching_item(regexes, config_line, pwd_lookup, SALT)
    assert processed_line == f"{line}{whitespace}{line}"

I'm trying to figure out what these are testing. I think nothing, and certainly nothing meriting 3 different new tests. They're all asserting that the processed line == the original line (just indirectly).

I don't think we need any of these tests as written. Instead, want to see a test that has ~3 lines in it:

assert that anonymize(" * fooo bar *") is unchanged
assert that anonymize(" * some password *") == " * some ASBCLJLKRJ# *" -- anonymization of secrets preserves spaces
assert that anonymize(" * some sensitive_word *") == " * some ASBCLJLKRJ# *" -- anonymization of keywords preserves spaces

[dhalperi]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @cdtomkins)

---

tests/unit/test_sensitive_item_removal.py line 655 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

I'm trying to figure out what these are testing. I think nothing, and certainly nothing meriting 3 different new tests. They're all asserting that the processed line == the original line (just indirectly).

I don't think we need any of these tests as written. Instead, want to see a test that has ~3 lines in it:

assert that anonymize(" * fooo bar *") is unchanged
assert that anonymize(" * some password *") == " * some ASBCLJLKRJ# *" -- anonymization of secrets preserves spaces
assert that anonymize(" * some sensitive_word *") == " * some ASBCLJLKRJ# *" -- anonymization of keywords preserves spaces

code review tool deleted all my spaces visually, trying again.

assert that anonymize(" *     fooo bar   *") is unchanged
assert that anonymize(" *     some       password   *") == " *     some       ASBCLJLKRJ#   *" -- anonymization of secrets preserves spaces
assert that anonymize(" *     some       sensitive_word   *") == " *     some       ASBCLJLKRJ#   *" -- anonymization of keywords preserves spaces

[cdtomkins]

I feel like the tests I wrote are valid; they test space handling with:

• single space
• two spaces
• N spaces

In 3 clear scenarios that failed before and work now:

• single_inline_whitespace
• multiple_inline_whitespace (i.e. more than one unusual space)
• preserve_different_inline_whitespace (i.e. more than one unusual space of different sizes).

The alternative tests you're proposing relate to anonymisation of passwords, which is already tested in other tests and is not the code I changed.

Also, handling as three separate tests seems like good practice to be able to map the test to the specific objective clearly. What's the downside?

[cdtomkins]

@ratulm / @dhalperi Please review my last comment. As I said, I'm happy to implement the test as you requested. Please confirm if so. Let's get this fix merged.

[dhalperi]

So

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @cdtomkins)

---

tests/unit/test_sensitive_item_removal.py line 655 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

code review tool deleted all my spaces visually, trying again.

assert that anonymize(" *     fooo bar   *") is unchanged
assert that anonymize(" *     some       password   *") == " *     some       ASBCLJLKRJ#   *" -- anonymization of secrets preserves spaces
assert that anonymize(" *     some       sensitive_word   *") == " *     some       ASBCLJLKRJ#   *" -- anonymization of keywords preserves spaces

So one reason I asked for tests that didn't hide what they were testing beyond python magic is that the code doesn't actually work. For example, this test fails:

diff --git a/tests/unit/test_sensitive_item_removal.py b/tests/unit/test_sensitive_item_removal.py
index 8b60b82..bebfa32 100644
--- a/tests/unit/test_sensitive_item_removal.py
+++ b/tests/unit/test_sensitive_item_removal.py
@@ -563,9 +563,16 @@ def test_pwd_removal_with_whitespace(regexes):
     """Test removal of password when a sensitive line contains extra whitespace."""
     sensitive_text = "RemoveMe"
     sensitive_line = "     password   0      \t{}".format(sensitive_text)
-    assert sensitive_text not in replace_matching_item(
-        regexes, sensitive_line, {}, SALT
-    )
+    pwd_lookup = {}
+    processed_line = replace_matching_item(regexes, sensitive_line, pwd_lookup, SALT)
+
+    # Verify sensitive text is removed
+    assert sensitive_text not in processed_line
+
+    # Verify whitespace is preserved
+    anon_val = _anonymize_value(sensitive_text, pwd_lookup, {}, SALT)
+    expected_line = "     password   0      \t{}".format(anon_val)
+    assert processed_line == expected_line
 
 
 @pytest.mark.parametrize(

[dhalperi]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @cdtomkins)

---

tests/unit/test_sensitive_item_removal.py line 655 at r3 (raw file):

    pwd_lookup = {}
    processed_line = replace_matching_item(regexes, config_line, pwd_lookup, SALT)
    assert processed_line == f"{line}{whitespace}{line}"

If we did want to keep this test as-is, it should be more clear to show that the line didn't change.

Suggestion:

assert processed_line == config_line

[cdtomkins]

1. I have improved the new tests' readability following the suggestion about assert processed_line == config_line, and tweaking them a bit otherwise too, thanks. Feel free to remove any tests that you think are excessive. We could also collapse them into a single test if you prefer, although personally I think they're better as-is.
2. I have improved the handling of tabs in whitespace. The test you specified passes now.

Please take another look.

[cdtomkins]

@ratulm / @dhalperi Please review my last comment.

[dhalperi]

@dhalperi reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion

---

netconan/sensitive_item_removal.py line 442 at r5 (raw file):

def _restore_spaces(line, target_whitespace_strings, leading, trailing):
    """Restore whitespace between words according to target_whitespace_strings. Leading/trailing characters are preserved."""
    parts = line.split()

I think I'm missing something here.

1. Why is default split() safe vs using the existing _split_line function?
2. We need some kind of check and error handling for the case when line.split() doesn't produce the same number of parts as the input.
3. I don't understand yet why given the previous two, this isn't as simple as: return leading + parts[0] + flatten(zip(parts[1:], target)) + trailing (this is pseudocode).

So for 3 -- why is the body of this function so complicated?