Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix --audit Crash on Missing File #56

Merged
merged 3 commits into from
Jul 12, 2018
Merged

Fix --audit Crash on Missing File #56

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d56db92
Skip files that do not exist when auditing the baseline
KevinHock Jul 10, 2018
d2b889f
Added a _clean_baseline_of_nonexistent_files function to audit_baseli…
KevinHock Jul 11, 2018
5c5c8bf
Change function name to _remove_nonexistent_files_from_baseline, re-o…
KevinHock Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion detect_secrets/core/audit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import print_function

import json
import os
import subprocess
import sys
import textwrap
Expand All @@ -19,18 +20,30 @@ class SecretNotFoundOnSpecifiedLineError(Exception):
pass


def _clean_baseline_of_nonexistent_files(baseline):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_remove_nonexistent_files_from_baseline? And it should return True if modified -- just so it's less negatives, so we don't get a case where:

if not baseline_wasnt_modified:
    do_something()

nit: let's throw this below audit_baseline function declaration, so it reads from top-down.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think I originally made it that but I changed it to wasnt_modified was to avoid the and not baseline_was_modified, which I read as slightly more confusing. This may be a weird preference of mine though, I'll change it ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm and not files_removed: looks nice 👍

baseline_wasnt_modified = True
for filename in baseline['results'].copy():
if not os.path.exists(filename):
del baseline['results'][filename]
baseline_wasnt_modified = False
return baseline_wasnt_modified


def audit_baseline(baseline_filename):
original_baseline = _get_baseline_from_file(baseline_filename)
if not original_baseline:
return

baseline_wasnt_modified = _clean_baseline_of_nonexistent_files(original_baseline)

current_secret_index = 0
results = defaultdict(list)
for filename, secret, total in _secret_generator(original_baseline):
_clear_screen()

if 'is_secret' not in secret:
current_secret_index += 1

try:
_print_context(
filename,
Expand All @@ -54,7 +67,7 @@ def audit_baseline(baseline_filename):
_handle_user_decision(decision, secret)
results[filename].append(secret)

if current_secret_index == 0:
if current_secret_index == 0 and baseline_wasnt_modified:
print('Nothing to audit!')
return

Expand Down
1 change: 0 additions & 1 deletion detect_secrets/core/baseline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ def merge_baseline(old_baseline, new_baseline):
:type new_baseline: dict
:param new_baseline: most recent scan
"""
# TODO: merge_results does not take into account `is_secret`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

new_baseline['results'] = merge_results(
old_baseline['results'],
new_baseline['results'],
Expand Down
13 changes: 10 additions & 3 deletions tests/core/audit_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,10 @@ def test_leapfrog_decision(self, mock_printer):

@contextmanager
def run_logic(self, inputs, modified_baseline=None, input_baseline=None):
with self.mock_env(inputs, baseline=input_baseline) as m:
with self.mock_env(
inputs,
baseline=input_baseline,
) as m:
audit.audit_baseline('will_be_mocked')

if not modified_baseline:
Expand All @@ -123,7 +126,7 @@ def mock_env(self, user_inputs=None, baseline=None):
'_get_baseline_from_file',
return_value=baseline,
), mock.patch.object(
# We mock this, because we don't really care about clearing
# We mock this because we don't really care about clearing
# screens for test cases.
audit,
'_clear_screen',
Expand All @@ -134,7 +137,11 @@ def mock_env(self, user_inputs=None, baseline=None):
), mock_user_input(
user_inputs,
), mock.patch.object(
# We mock this, so we don't need to do any file I/O.
# We mock this so we don't modify the baseline.
audit,
'_clean_baseline_of_nonexistent_files',
), mock.patch.object(
# We mock this so we don't need to do any file I/O.
audit,
'_save_baseline_to_file',
) as m:
Expand Down