-
-
Notifications
You must be signed in to change notification settings - Fork 758
Fix false negatives in hardcoded password detection (B105/B106) #1385
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,9 +9,9 @@ | |
| from bandit.core import issue | ||
| from bandit.core import test_properties as test | ||
|
|
||
| RE_WORDS = "(pas+wo?r?d|pass(phrase)?|pwd|token|secrete?)" | ||
| RE_WORDS = "(pas+wo?r?d|pass(phrase)?|pwd|token|secrete?|key)" | ||
| RE_CANDIDATES = re.compile( | ||
| "(^{0}$|_{0}_|^{0}_|_{0}$)".format(RE_WORDS), re.IGNORECASE | ||
| "(^{0}$|_{0}_|^{0}_|_{0}$|{0})".format(RE_WORDS), re.IGNORECASE | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -83,7 +83,13 @@ def hardcoded_password_string(context): | |
| if isinstance(node._bandit_parent, ast.Assign): | ||
| # looks for "candidate='some_string'" | ||
| for targ in node._bandit_parent.targets: | ||
| if isinstance(targ, ast.Name) and RE_CANDIDATES.search(targ.id): | ||
| if isinstance(targ, ast.Name): | ||
| normalized = re.sub( | ||
| r"(?<=[a-z])(?=[A-Z])|(?<=[A-Z])(?=[A-Z][a-z])", | ||
|
Comment on lines
+86
to
+88
|
||
| "_", | ||
| targ.id, | ||
| ).lower() | ||
| if isinstance(targ, ast.Name) and RE_CANDIDATES.search(normalized): | ||
| return _report(node.value) | ||
| elif isinstance(targ, ast.Attribute) and RE_CANDIDATES.search( | ||
| targ.attr | ||
|
|
@@ -143,6 +149,14 @@ def hardcoded_password_string(context): | |
| comp.comparators[0], ast.Constant | ||
| ) and isinstance(comp.comparators[0].value, str): | ||
| return _report(comp.comparators[0].value) | ||
| elif isinstance(comp.left, ast.Subscript): | ||
| slc = comp.left.slice | ||
| if isinstance(slc, ast.Constant) and isinstance(slc.value, str): | ||
| if RE_CANDIDATES.search(slc.value): | ||
| if isinstance( | ||
| comp.comparators[0], ast.Constant | ||
| ) and isinstance(comp.comparators[0].value, str): | ||
| return _report(comp.comparators[0].value) | ||
|
|
||
|
|
||
| @test.checks("Call") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE_CANDIDATESis now an unbounded substring match (|{0}) andRE_WORDSnow includeskey. This combination will dramatically increase false positives because it matches credential words inside unrelated identifiers and dict keys (e.g., a dict literal with key'key'or variables likekeyfile,monkey,compass, etc.). Since B105/B106/B107 all share this regex, it will also affect unrelated examples and likely break existing functional expectations.Suggestion: keep boundary-based matching for common/short terms (especially
passandkey), and if you need to catch patterns likeDATABASEPASSWORD, add a separate “strong word” substring check (e.g., only forpassword|passphrase|token|secret) or normalize identifiers (camelCase -> snake_case) and apply the existing boundary regex. Also consider scopingkeyto B106/B107 only (keyword args/defaults) rather than B105 dict/variable names to reduce noise.