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

Keyword detector optimization #396

Closed
wants to merge 10 commits into from
Closed

Keyword detector optimization #396

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0bd4dda
Keyword plugin optimization
pablosnt Jan 8, 2021
1ed4084
Keyword plugin test adjustment
pablosnt Jan 8, 2021
4e6c2c9
New items in DENYLIST and FALSE_POSITIVES
pablosnt Jan 8, 2021
181e9a8
Keyword XML plugin implementation
pablosnt Jan 8, 2021
6814e49
Keyword XML plugin test
pablosnt Jan 8, 2021
010e929
Removal of probably_false_positive function
pablosnt Jan 28, 2021
e594e0d
Merge branch 'pre-v1-launch' into feature/v1-keyword-xml-plugin
pablosnt Jan 28, 2021
5a445dc
Correction of keyword regexes
pablosnt Jan 28, 2021
ca2c9ce
Keyword plugin correction
pablosnt Feb 4, 2021
25030de
Merge branch 'pre-v1-launch' into feature/v1-keyword-xml-plugin
pablosnt Feb 4, 2021
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
56 changes: 56 additions & 0 deletions tests/plugins/keyword_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,12 @@
'.js',
'.py',
'.swift',
'.tf',
'.c',
'.cpp',
'.cs',
'.sh',
'.ps1',
)

STANDARD_NEGATIVES = []
Expand Down Expand Up @@ -227,6 +233,40 @@ def test_analyze_objective_c_positives(self, file_content):
assert len(secrets) == 1
assert list(secrets)[0].secret_value == 'm{{h}o)p${e]nob(ody[finds>-_$#thisone}}'

@pytest.mark.parametrize(
'file_content',
FOLLOWED_BY_EQUAL_SIGNS_RE.get('positives').get('quotes_required')
+ FOLLOWED_BY_EQUAL_SIGNS_RE.get('positives').get('quotes_not_required'),
)
def test_analyze_properties_positives(self, file_content):
secrets = list(
KeywordDetector().analyze_line(
filename='mock_filename.properties',
line=file_content,
),
)

assert len(secrets) == 1
assert secrets[0].secret_value == 'm{{h}o)p${e]nob(ody[finds>-_$#thisone}}'

@pytest.mark.parametrize(
'file_content',
FOLLOWED_BY_EQUAL_SIGNS_RE.get('positives').get('quotes_required')
+ FOLLOWED_BY_EQUAL_SIGNS_RE.get('positives').get('quotes_not_required')
+ FOLLOWED_BY_COLON_RE.get('positives').get('quotes_required')
+ FOLLOWED_BY_COLON_RE.get('positives').get('quotes_not_required'),
)
def test_analyze_yaml_positives(self, file_content):
secrets = list(
KeywordDetector().analyze_line(
filename='mock_filename.yml',
line=file_content,
),
)

assert len(secrets) == 1
assert secrets[0].secret_value == 'm{{h}o)p${e]nob(ody[finds>-_$#thisone}}'

@pytest.mark.skip(
reason='TODO: false positive heuristics need to be migrated over to filters/*',
)
Expand Down Expand Up @@ -329,3 +369,19 @@ def test_analyze_example_negatives(self, file_content):
# Make it start with `<`, (and end with `>`) so it hits our false-positive check
line=file_content.replace('m{', '<').replace('}', '>'),
)

@pytest.mark.skip(
reason='TODO: false positive heuristics need to be migrated over to filters/*',
)
Comment on lines +285 to +287
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to skip this test? All filters have been migrated already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, when I have implemented this feature, the filters haven't been migrated already. I will commit it comming soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @domanchi, I have reviewed the default filters of detect-secrets:

"filters_used": [
    {
      "path": "detect_secrets.filters.allowlist.is_line_allowlisted"
    },
    {
      "path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies",
      "min_level": 2
    },
    {
      "path": "detect_secrets.filters.heuristic.is_likely_id_string"
    },
    {
      "path": "detect_secrets.filters.heuristic.is_potential_uuid"
    },
    {
      "path": "detect_secrets.filters.heuristic.is_sequential_string"
    }
  ],

In the default filters, we haven't found any adequate to our test's needs. I think that the most useful filter in this case, can be heuristic.is_templated_secret. We haven't found any references to this filter in the documentation and we think that it should be established by default. Are this tasks in your roadmap?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. I must have missed this during my migration effort. I'll cut a ticket for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

#405

@pytest.mark.parametrize(
'file_content',
FOLLOWED_BY_EQUAL_SIGNS_RE.get('negatives').get('quotes_required')
+ FOLLOWED_BY_EQUAL_SIGNS_RE.get('negatives').get('quotes_not_required'),
)
def test_analyze_properties_negatives(self, file_content):
assert not KeywordDetector().analyze_line(
filename='mock_filename.properties',

# Make it start with `<`, (and end with `>`) so it hits our false-positive check
line=file_content.replace('m{', '<').replace('}', '>'),
)