Skip to content

Add multiple file support #188

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

Merged
merged 1 commit into from
Jun 15, 2019
Merged

Add multiple file support #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ $ detect-secrets scan --update .secrets.baseline

### Command Line

`detect-secrets` is designed to be used as a git pre-commit hook, but you can also invoke `detect-secrets scan [path]` directly (`path` defaults to `.` if not specified).
`detect-secrets` is designed to be used as a git pre-commit hook, but you can also invoke `detect-secrets scan [path]` directly being `path` the file(s) and/or directory(ies) to scan (`path` defaults to `.` if not specified).

It should be noted that by default, `detect-secrets scan` only operates on files that are tracked by git. So if you intend to scan files outside of a git repository, you will need to pass the `--all-files` flag.

Expand Down
25 changes: 16 additions & 9 deletions detect_secrets/core/baseline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@
import re
import subprocess

from detect_secrets.core.log import get_logger
from detect_secrets.core.secrets_collection import SecretsCollection

log = get_logger(format_string='%(message)s')


def initialize(
path,
plugins,
exclude_files_regex=None,
exclude_lines_regex=None,
path='.',
scan_all_files=False,
):
"""Scans the entire codebase for secrets, and returns a
Expand All @@ -23,7 +26,7 @@ def initialize(

:type exclude_files_regex: str|None
:type exclude_lines_regex: str|None
:type path: str
:type path: list
:type scan_all_files: bool

:rtype: SecretsCollection
Expand All @@ -34,13 +37,17 @@ def initialize(
exclude_lines=exclude_lines_regex,
)

if os.path.isfile(path):
# This option allows for much easier adhoc usage.
files_to_scan = [path]
elif scan_all_files:
files_to_scan = _get_files_recursively(path)
else:
files_to_scan = _get_git_tracked_files(path)
files_to_scan = list()
for element in path:
if os.path.isdir(element):
if scan_all_files:
files_to_scan.extend(_get_files_recursively(element))
else:
files_to_scan.extend(_get_git_tracked_files(element))
elif os.path.isfile(element):
files_to_scan.append(element)
else:
log.error("detect-secrets: " + element + ": No such file or directory")

if not files_to_scan:
return output
Expand Down
2 changes: 1 addition & 1 deletion detect_secrets/core/usage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def add_arguments(self):
def _add_initialize_baseline_argument(self):
self.parser.add_argument(
'path',
nargs='?',
nargs='*',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure, but would it be possible to use '+'? So you wouldn't have to do

:type path: str|list

and the isinstance check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, as '+' generates an error message if there isn't at least one command-line argument present... I thought that as we are using default's, '*' would be a better fit. Running the test suite with '+' breaks with detect-secrets scan: error: too few arguments (Well, as intended!).

Maybe I'm missing something... But If you think it's an improvement I can take a closer look at it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

You’re completely right :)

From reading https://stackoverflow.com/questions/23490152/list-of-arguments-with-argparse#23490179 it seems it should be always be a list with *, so maybe that’s true?

Copy link
Contributor Author

@dgzlopes dgzlopes Jun 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes! Looks like all is passed as a list, even the default argument '.'

So the isInstance() might be redundant as when we pass (on initialize()) path='.' it's never used... Running scan without path(s) will trigger argparse's default.

This was closely related to #188 (comment) so maybe using the list as a default isn't that great... But I think '+' passes all as a list too! What's the best way to handle this?

Copy link
Collaborator

@KevinHock KevinHock Jun 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand, the

    if not isinstance(path, list):
        path = [path]

is always dead-code, so we can remove that.

From reading the code, it seems a path is always passed in to initialize, so there doesn't seem to be a need for a default argument I think, right?

If so we can change

def initialize(
    plugins,
    exclude_files_regex=None,
    exclude_lines_regex=None,
    path='.',
    scan_all_files=False,
):

to

def initialize(
    path,
    plugins,
    exclude_files_regex=None,
    exclude_lines_regex=None,
    scan_all_files=False,
):

Copy link
Contributor Author

@dgzlopes dgzlopes Jun 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

As I thought :) Thanks for the writeup. Updated the PR and fixed the failing tests. I hope It looks better now!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks awesome 👏

default='.',
help=(
'Scans the entire codebase and outputs a snapshot of '
Expand Down
43 changes: 35 additions & 8 deletions tests/core/baseline_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,26 @@ def setup(self):

def get_results(
self,
path='./test_data/files',
path=['./test_data/files'],
exclude_files_regex=None,
scan_all_files=False,
):
return baseline.initialize(
path,
self.plugins,
path=path,
exclude_files_regex=exclude_files_regex,
scan_all_files=scan_all_files,
).json()

@pytest.mark.parametrize(
'path',
[
'./test_data/files',
[
'./test_data/files',

# Test relative paths
'test_data/../test_data/files/tmp/..',
# Test relative paths
'test_data/../test_data/files/tmp/..',
],
],
)
def test_basic_usage(self, path):
Expand All @@ -58,6 +60,31 @@ def test_basic_usage(self, path):
assert len(results['test_data/files/file_with_secrets.py']) == 1
assert len(results['test_data/files/tmp/file_with_secrets.py']) == 2

def test_with_multiple_files(self):
results = self.get_results(path=[
'test_data/files/file_with_secrets.py',
'test_data/files/tmp/file_with_secrets.py',
])

assert len(results['test_data/files/file_with_secrets.py']) == 1
assert len(results['test_data/files/tmp/file_with_secrets.py']) == 2
assert 'test_data/files/file_with_secrets.py' in results
assert 'test_data/files/tmp/file_with_secrets.py' in results

def test_with_multiple_non_existent_files(self):
results = self.get_results(path=['non-existent-file.A', 'non-existent-file.B'])

# No expected results, because files don't exist
assert not results

def test_with_folders_and_files(self):
results = self.get_results(path=['test_data/', 'non-existent-file.B'])

assert 'test_data/files/file_with_secrets.py' in results
assert 'test_data/files/tmp/file_with_secrets.py' in results
assert 'test_data/files/file_with_no_secrets.py' not in results
assert 'non-existent-file.B' not in results

def test_exclude_regex(self):
results = self.get_results(exclude_files_regex='tmp*')

Expand All @@ -82,7 +109,7 @@ def test_no_files_in_git_repo(self):
),
),
):
results = self.get_results(path='will_be_mocked')
results = self.get_results(path=['will_be_mocked'])

assert not results

Expand All @@ -94,12 +121,12 @@ def test_single_non_tracked_git_file_should_work(self):
'Super hidden value "BEEF0123456789a"',
'detect_secrets.core.secrets_collection.codecs.open',
):
results = self.get_results('will_be_mocked')
results = self.get_results(path=['will_be_mocked'])

assert len(results['will_be_mocked']) == 1

def test_scan_all_files(self):
results = self.get_results(path='test_data/files', scan_all_files=True)
results = self.get_results(path=['test_data/files'], scan_all_files=True)
assert len(results.keys()) == 2


Expand Down
2 changes: 1 addition & 1 deletion tests/main_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def test_scan_with_rootdir(self, mock_baseline_initialize):
plugins=Any(tuple),
exclude_files_regex=None,
exclude_lines_regex=None,
path='test_data',
path=['test_data'],
scan_all_files=False,
)

Expand Down