Browse Source
Moving semgrep rules out of repos as it's unnecessary to maintain same ruleset for different repos and different branches.develop
Ankush Menat
3 years ago
committed by
GitHub
GitHub
14 changed files with 4 additions and 570 deletions
@ -1,38 +0,0 @@ |
|||
# Semgrep linting |
|||
|
|||
## What is semgrep? |
|||
Semgrep or "semantic grep" is language agnostic static analysis tool. In simple terms semgrep is syntax-aware `grep`, so unlike regex it doesn't get confused by different ways of writing same thing or whitespaces or code split in multiple lines etc. |
|||
|
|||
Example: |
|||
|
|||
To check if a translate function is using f-string or not the regex would be `r"_\(\s*f[\"']"` while equivalent rule in semgrep would be `_(f"...")`. As semgrep knows grammer of language it takes care of unnecessary whitespace, type of quotation marks etc. |
|||
|
|||
You can read more such examples in `.github/helper/semgrep_rules` directory. |
|||
|
|||
# Why/when to use this? |
|||
We want to maintain quality of contributions, at the same time remembering all the good practices can be pain to deal with while evaluating contributions. Using semgrep if you can translate "best practice" into a rule then it can automate the task for us. |
|||
|
|||
## Running locally |
|||
|
|||
Install semgrep using homebrew `brew install semgrep` or pip `pip install semgrep`. |
|||
|
|||
To run locally use following command: |
|||
|
|||
`semgrep --config=.github/helper/semgrep_rules [file/folder names]` |
|||
|
|||
## Testing |
|||
semgrep allows testing the tests. Refer to this page: https://semgrep.dev/docs/writing-rules/testing-rules/ |
|||
|
|||
When writing new rules you should write few positive and few negative cases as shown in the guide and current tests. |
|||
|
|||
To run current tests: `semgrep --test --test-ignore-todo .github/helper/semgrep_rules` |
|||
|
|||
|
|||
## Reference |
|||
|
|||
If you are new to Semgrep read following pages to get started on writing/modifying rules: |
|||
|
|||
- https://semgrep.dev/docs/getting-started/ |
|||
- https://semgrep.dev/docs/writing-rules/rule-syntax |
|||
- https://semgrep.dev/docs/writing-rules/pattern-examples/ |
|||
- https://semgrep.dev/docs/writing-rules/rule-ideas/#common-use-cases |
|||
@ -1,64 +0,0 @@ |
|||
import frappe |
|||
from frappe import _ |
|||
|
|||
from frappe.model.document import Document |
|||
|
|||
|
|||
# ruleid: frappe-modifying-but-not-comitting |
|||
def on_submit(self): |
|||
if self.value_of_goods == 0: |
|||
frappe.throw(_('Value of goods cannot be 0')) |
|||
self.status = 'Submitted' |
|||
|
|||
|
|||
# ok: frappe-modifying-but-not-comitting |
|||
def on_submit(self): |
|||
if self.value_of_goods == 0: |
|||
frappe.throw(_('Value of goods cannot be 0')) |
|||
self.status = 'Submitted' |
|||
self.db_set('status', 'Submitted') |
|||
|
|||
# ok: frappe-modifying-but-not-comitting |
|||
def on_submit(self): |
|||
if self.value_of_goods == 0: |
|||
frappe.throw(_('Value of goods cannot be 0')) |
|||
x = "y" |
|||
self.status = x |
|||
self.db_set('status', x) |
|||
|
|||
|
|||
# ok: frappe-modifying-but-not-comitting |
|||
def on_submit(self): |
|||
x = "y" |
|||
self.status = x |
|||
self.save() |
|||
|
|||
# ruleid: frappe-modifying-but-not-comitting-other-method |
|||
class DoctypeClass(Document): |
|||
def on_submit(self): |
|||
self.good_method() |
|||
self.tainted_method() |
|||
|
|||
def tainted_method(self): |
|||
self.status = "uptate" |
|||
|
|||
|
|||
# ok: frappe-modifying-but-not-comitting-other-method |
|||
class DoctypeClass(Document): |
|||
def on_submit(self): |
|||
self.good_method() |
|||
self.tainted_method() |
|||
|
|||
def tainted_method(self): |
|||
self.status = "update" |
|||
self.db_set("status", "update") |
|||
|
|||
# ok: frappe-modifying-but-not-comitting-other-method |
|||
class DoctypeClass(Document): |
|||
def on_submit(self): |
|||
self.good_method() |
|||
self.tainted_method() |
|||
self.save() |
|||
|
|||
def tainted_method(self): |
|||
self.status = "uptate" |
|||
@ -1,163 +0,0 @@ |
|||
# This file specifies rules for correctness according to how frappe doctype data model works. |
|||
|
|||
rules: |
|||
- id: frappe-modifying-but-not-comitting |
|||
patterns: |
|||
- pattern: | |
|||
def $METHOD(self, ...): |
|||
... |
|||
self.$ATTR = ... |
|||
- pattern-not: | |
|||
def $METHOD(self, ...): |
|||
... |
|||
self.$ATTR = ... |
|||
... |
|||
self.db_set(..., self.$ATTR, ...) |
|||
- pattern-not: | |
|||
def $METHOD(self, ...): |
|||
... |
|||
self.$ATTR = $SOME_VAR |
|||
... |
|||
self.db_set(..., $SOME_VAR, ...) |
|||
- pattern-not: | |
|||
def $METHOD(self, ...): |
|||
... |
|||
self.$ATTR = $SOME_VAR |
|||
... |
|||
self.save() |
|||
- metavariable-regex: |
|||
metavariable: '$ATTR' |
|||
# this is negative look-ahead, add more attrs to ignore like (ignore|ignore_this_too|ignore_me) |
|||
regex: '^(?!ignore_linked_doctypes|status_updater)(.*)$' |
|||
- metavariable-regex: |
|||
metavariable: "$METHOD" |
|||
regex: "(on_submit|on_cancel)" |
|||
message: | |
|||
DocType modified in self.$METHOD. Please check if modification of self.$ATTR is commited to database. |
|||
languages: [python] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-modifying-but-not-comitting-other-method |
|||
patterns: |
|||
- pattern: | |
|||
class $DOCTYPE(...): |
|||
def $METHOD(self, ...): |
|||
... |
|||
self.$ANOTHER_METHOD() |
|||
... |
|||
|
|||
def $ANOTHER_METHOD(self, ...): |
|||
... |
|||
self.$ATTR = ... |
|||
- pattern-not: | |
|||
class $DOCTYPE(...): |
|||
def $METHOD(self, ...): |
|||
... |
|||
self.$ANOTHER_METHOD() |
|||
... |
|||
|
|||
def $ANOTHER_METHOD(self, ...): |
|||
... |
|||
self.$ATTR = ... |
|||
... |
|||
self.db_set(..., self.$ATTR, ...) |
|||
- pattern-not: | |
|||
class $DOCTYPE(...): |
|||
def $METHOD(self, ...): |
|||
... |
|||
self.$ANOTHER_METHOD() |
|||
... |
|||
|
|||
def $ANOTHER_METHOD(self, ...): |
|||
... |
|||
self.$ATTR = $SOME_VAR |
|||
... |
|||
self.db_set(..., $SOME_VAR, ...) |
|||
- pattern-not: | |
|||
class $DOCTYPE(...): |
|||
def $METHOD(self, ...): |
|||
... |
|||
self.$ANOTHER_METHOD() |
|||
... |
|||
self.save() |
|||
def $ANOTHER_METHOD(self, ...): |
|||
... |
|||
self.$ATTR = ... |
|||
- metavariable-regex: |
|||
metavariable: "$METHOD" |
|||
regex: "(on_submit|on_cancel)" |
|||
message: | |
|||
self.$ANOTHER_METHOD is called from self.$METHOD, check if changes to self.$ATTR are commited to database. |
|||
languages: [python] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-print-function-in-doctypes |
|||
pattern: print(...) |
|||
message: | |
|||
Did you mean to leave this print statement in? Consider using msgprint or logger instead of print statement. |
|||
languages: [python] |
|||
severity: WARNING |
|||
paths: |
|||
include: |
|||
- "*/**/doctype/*" |
|||
|
|||
- id: frappe-modifying-child-tables-while-iterating |
|||
pattern-either: |
|||
- pattern: | |
|||
for $ROW in self.$TABLE: |
|||
... |
|||
self.remove(...) |
|||
- pattern: | |
|||
for $ROW in self.$TABLE: |
|||
... |
|||
self.append(...) |
|||
message: | |
|||
Child table being modified while iterating on it. |
|||
languages: [python] |
|||
severity: ERROR |
|||
paths: |
|||
include: |
|||
- "*/**/doctype/*" |
|||
|
|||
- id: frappe-same-key-assigned-twice |
|||
pattern-either: |
|||
- pattern: | |
|||
{..., $X: $A, ..., $X: $B, ...} |
|||
- pattern: | |
|||
dict(..., ($X, $A), ..., ($X, $B), ...) |
|||
- pattern: | |
|||
_dict(..., ($X, $A), ..., ($X, $B), ...) |
|||
message: | |
|||
key `$X` is uselessly assigned twice. This could be a potential bug. |
|||
languages: [python] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-manual-commit |
|||
patterns: |
|||
- pattern: frappe.db.commit() |
|||
- pattern-not-inside: | |
|||
try: |
|||
... |
|||
except ...: |
|||
... |
|||
message: | |
|||
Manually commiting a transaction is highly discouraged. Read about the transaction model implemented by Frappe Framework before adding manual commits: https://frappeframework.com/docs/user/en/api/database#database-transaction-model If you think manual commit is required then add a comment explaining why and `// nosemgrep` on the same line. |
|||
paths: |
|||
exclude: |
|||
- "**/patches/**" |
|||
- "**/demo/**" |
|||
languages: [python] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-using-db-sql |
|||
pattern-either: |
|||
- pattern: frappe.db.sql(...) |
|||
- pattern: frappe.db.sql_ddl(...) |
|||
- pattern: frappe.db.sql_list(...) |
|||
paths: |
|||
exclude: |
|||
- "test_*.py" |
|||
message: | |
|||
The PR contains a SQL query that may be re-written with frappe.qb (https://frappeframework.com/docs/user/en/api/query-builder) or the Database API (https://frappeframework.com/docs/user/en/api/database) |
|||
languages: [python] |
|||
severity: ERROR |
|||
@ -1,15 +0,0 @@ |
|||
from frappe import _ |
|||
|
|||
|
|||
# ruleid: frappe-missing-translate-function-in-report-python |
|||
{"label": "Field Label"} |
|||
|
|||
# ruleid: frappe-missing-translate-function-in-report-python |
|||
dict(label="Field Label") |
|||
|
|||
|
|||
# ok: frappe-missing-translate-function-in-report-python |
|||
{"label": _("Field Label")} |
|||
|
|||
# ok: frappe-missing-translate-function-in-report-python |
|||
dict(label=_("Field Label")) |
|||
@ -1,34 +0,0 @@ |
|||
rules: |
|||
- id: frappe-missing-translate-function-in-report-python |
|||
paths: |
|||
include: |
|||
- "**/report" |
|||
exclude: |
|||
- "**/regional" |
|||
pattern-either: |
|||
- patterns: |
|||
- pattern: | |
|||
{..., "label": "...", ...} |
|||
- pattern-not: | |
|||
{..., "label": _("..."), ...} |
|||
- patterns: |
|||
- pattern: dict(..., label="...", ...) |
|||
- pattern-not: dict(..., label=_("..."), ...) |
|||
message: | |
|||
All user facing text must be wrapped in translate function. Please refer to translation documentation. https://frappeframework.com/docs/user/en/guides/basics/translations |
|||
languages: [python] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-translated-values-in-business-logic |
|||
paths: |
|||
include: |
|||
- "**/report" |
|||
patterns: |
|||
- pattern-inside: | |
|||
{..., filters: [...], ...} |
|||
- pattern: | |
|||
{..., options: [..., __("..."), ...], ...} |
|||
message: | |
|||
Using translated values in options field will require you to translate the values while comparing in business logic. Instead of passing translated labels provide objects that contain both label and value. e.g. { label: __("Option value"), value: "Option value"} |
|||
languages: [javascript] |
|||
severity: ERROR |
|||
@ -1,6 +0,0 @@ |
|||
def function_name(input): |
|||
# ruleid: frappe-codeinjection-eval |
|||
eval(input) |
|||
|
|||
# ok: frappe-codeinjection-eval |
|||
eval("1 + 1") |
|||
@ -1,10 +0,0 @@ |
|||
rules: |
|||
- id: frappe-codeinjection-eval |
|||
patterns: |
|||
- pattern-not: eval("...") |
|||
- pattern: eval(...) |
|||
message: | |
|||
Detected the use of eval(). eval() can be dangerous if used to evaluate |
|||
dynamic content. Avoid it or use safe_eval(). |
|||
languages: [python] |
|||
severity: ERROR |
|||
@ -1,44 +0,0 @@ |
|||
// ruleid: frappe-translation-empty-string
|
|||
__("") |
|||
// ruleid: frappe-translation-empty-string
|
|||
__('') |
|||
|
|||
// ok: frappe-translation-js-formatting
|
|||
__('Welcome {0}, get started with ERPNext in just a few clicks.', [full_name]); |
|||
|
|||
// ruleid: frappe-translation-js-formatting
|
|||
__(`Welcome ${full_name}, get started with ERPNext in just a few clicks.`); |
|||
|
|||
// ok: frappe-translation-js-formatting
|
|||
__('This is fine'); |
|||
|
|||
|
|||
// ok: frappe-translation-trailing-spaces
|
|||
__('This is fine'); |
|||
|
|||
// ruleid: frappe-translation-trailing-spaces
|
|||
__(' this is not ok '); |
|||
// ruleid: frappe-translation-trailing-spaces
|
|||
__('this is not ok '); |
|||
// ruleid: frappe-translation-trailing-spaces
|
|||
__(' this is not ok'); |
|||
|
|||
// ok: frappe-translation-js-splitting
|
|||
__('You have {0} subscribers in your mailing list.', [subscribers.length]) |
|||
|
|||
// todoruleid: frappe-translation-js-splitting
|
|||
__('You have') + subscribers.length + __('subscribers in your mailing list.') |
|||
|
|||
// ruleid: frappe-translation-js-splitting
|
|||
__('You have' + 'subscribers in your mailing list.') |
|||
|
|||
// ruleid: frappe-translation-js-splitting
|
|||
__('You have {0} subscribers' + |
|||
'in your mailing list', [subscribers.length]) |
|||
|
|||
// ok: frappe-translation-js-splitting
|
|||
__("Ctrl+Enter to add comment") |
|||
|
|||
// ruleid: frappe-translation-js-splitting
|
|||
__('You have {0} subscribers \ |
|||
in your mailing list', [subscribers.length]) |
|||
@ -1,61 +0,0 @@ |
|||
# Examples taken from https://frappeframework.com/docs/user/en/translations |
|||
# This file is used for testing the tests. |
|||
|
|||
from frappe import _ |
|||
|
|||
full_name = "Jon Doe" |
|||
# ok: frappe-translation-python-formatting |
|||
_('Welcome {0}, get started with ERPNext in just a few clicks.').format(full_name) |
|||
|
|||
# ruleid: frappe-translation-python-formatting |
|||
_('Welcome %s, get started with ERPNext in just a few clicks.' % full_name) |
|||
# ruleid: frappe-translation-python-formatting |
|||
_('Welcome %(name)s, get started with ERPNext in just a few clicks.' % {'name': full_name}) |
|||
|
|||
# ruleid: frappe-translation-python-formatting |
|||
_('Welcome {0}, get started with ERPNext in just a few clicks.'.format(full_name)) |
|||
|
|||
|
|||
subscribers = ["Jon", "Doe"] |
|||
# ok: frappe-translation-python-formatting |
|||
_('You have {0} subscribers in your mailing list.').format(len(subscribers)) |
|||
|
|||
# ruleid: frappe-translation-python-splitting |
|||
_('You have') + len(subscribers) + _('subscribers in your mailing list.') |
|||
|
|||
# ruleid: frappe-translation-python-splitting |
|||
_('You have {0} subscribers \ |
|||
in your mailing list').format(len(subscribers)) |
|||
|
|||
# ok: frappe-translation-python-splitting |
|||
_('You have {0} subscribers') \ |
|||
+ 'in your mailing list' |
|||
|
|||
# ruleid: frappe-translation-trailing-spaces |
|||
msg = _(" You have {0} pending invoice ") |
|||
# ruleid: frappe-translation-trailing-spaces |
|||
msg = _("You have {0} pending invoice ") |
|||
# ruleid: frappe-translation-trailing-spaces |
|||
msg = _(" You have {0} pending invoice") |
|||
|
|||
# ok: frappe-translation-trailing-spaces |
|||
msg = ' ' + _("You have {0} pending invoices") + ' ' |
|||
|
|||
# ruleid: frappe-translation-python-formatting |
|||
_(f"can not format like this - {subscribers}") |
|||
# ruleid: frappe-translation-python-splitting |
|||
_(f"what" + f"this is also not cool") |
|||
|
|||
|
|||
# ruleid: frappe-translation-empty-string |
|||
_("") |
|||
# ruleid: frappe-translation-empty-string |
|||
_('') |
|||
|
|||
|
|||
class Test: |
|||
# ok: frappe-translation-python-splitting |
|||
def __init__( |
|||
args |
|||
): |
|||
pass |
|||
@ -1,64 +0,0 @@ |
|||
rules: |
|||
- id: frappe-translation-empty-string |
|||
pattern-either: |
|||
- pattern: _("") |
|||
- pattern: __("") |
|||
message: | |
|||
Empty string is useless for translation. |
|||
Please refer: https://frappeframework.com/docs/user/en/translations |
|||
languages: [python, javascript, json] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-translation-trailing-spaces |
|||
pattern-either: |
|||
- pattern: _("=~/(^[ \t]+|[ \t]+$)/") |
|||
- pattern: __("=~/(^[ \t]+|[ \t]+$)/") |
|||
message: | |
|||
Trailing or leading whitespace not allowed in translate strings. |
|||
Please refer: https://frappeframework.com/docs/user/en/translations |
|||
languages: [python, javascript, json] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-translation-python-formatting |
|||
pattern-either: |
|||
- pattern: _("..." % ...) |
|||
- pattern: _("...".format(...)) |
|||
- pattern: _(f"...") |
|||
message: | |
|||
Only positional formatters are allowed and formatting should not be done before translating. |
|||
Please refer: https://frappeframework.com/docs/user/en/translations |
|||
languages: [python] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-translation-js-formatting |
|||
patterns: |
|||
- pattern: __(`...`) |
|||
- pattern-not: __("...") |
|||
message: | |
|||
Template strings are not allowed for text formatting. |
|||
Please refer: https://frappeframework.com/docs/user/en/translations |
|||
languages: [javascript, json] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-translation-python-splitting |
|||
pattern-either: |
|||
- pattern: _(...) + _(...) |
|||
- pattern: _("..." + "...") |
|||
- pattern-regex: '[\s\.]_\([^\)]*\\\s*' # lines broken by `\` |
|||
- pattern-regex: '[\s\.]_\(\s*\n' # line breaks allowed by python for using ( ) |
|||
message: | |
|||
Do not split strings inside translate function. Do not concatenate using translate functions. |
|||
Please refer: https://frappeframework.com/docs/user/en/translations |
|||
languages: [python] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-translation-js-splitting |
|||
pattern-either: |
|||
- pattern-regex: '__\([^\)]*[\\]\s+' |
|||
- pattern: __('...' + '...', ...) |
|||
- pattern: __('...') + __('...') |
|||
message: | |
|||
Do not split strings inside translate function. Do not concatenate using translate functions. |
|||
Please refer: https://frappeframework.com/docs/user/en/translations |
|||
languages: [javascript, json] |
|||
severity: ERROR |
|||
@ -1,9 +0,0 @@ |
|||
|
|||
// ok: frappe-missing-translate-function-js
|
|||
frappe.msgprint('{{ _("Both login and password required") }}'); |
|||
|
|||
// ruleid: frappe-missing-translate-function-js
|
|||
frappe.msgprint('What'); |
|||
|
|||
// ok: frappe-missing-translate-function-js
|
|||
frappe.throw(' {{ _("Both login and password required") }}. '); |
|||
@ -1,31 +0,0 @@ |
|||
import frappe |
|||
from frappe import msgprint, throw, _ |
|||
|
|||
|
|||
# ruleid: frappe-missing-translate-function-python |
|||
throw("Error Occured") |
|||
|
|||
# ruleid: frappe-missing-translate-function-python |
|||
frappe.throw("Error Occured") |
|||
|
|||
# ruleid: frappe-missing-translate-function-python |
|||
frappe.msgprint("Useful message") |
|||
|
|||
# ruleid: frappe-missing-translate-function-python |
|||
msgprint("Useful message") |
|||
|
|||
|
|||
# ok: frappe-missing-translate-function-python |
|||
translatedmessage = _("Hello") |
|||
|
|||
# ok: frappe-missing-translate-function-python |
|||
throw(translatedmessage) |
|||
|
|||
# ok: frappe-missing-translate-function-python |
|||
msgprint(translatedmessage) |
|||
|
|||
# ok: frappe-missing-translate-function-python |
|||
msgprint(_("Helpful message")) |
|||
|
|||
# ok: frappe-missing-translate-function-python |
|||
frappe.throw(_("Error occured")) |
|||
@ -1,30 +0,0 @@ |
|||
rules: |
|||
- id: frappe-missing-translate-function-python |
|||
pattern-either: |
|||
- patterns: |
|||
- pattern: frappe.msgprint("...", ...) |
|||
- pattern-not: frappe.msgprint(_("..."), ...) |
|||
- patterns: |
|||
- pattern: frappe.throw("...", ...) |
|||
- pattern-not: frappe.throw(_("..."), ...) |
|||
message: | |
|||
All user facing text must be wrapped in translate function. Please refer to translation documentation. https://frappeframework.com/docs/user/en/guides/basics/translations |
|||
languages: [python] |
|||
severity: ERROR |
|||
|
|||
- id: frappe-missing-translate-function-js |
|||
pattern-either: |
|||
- patterns: |
|||
- pattern: frappe.msgprint("...", ...) |
|||
- pattern-not: frappe.msgprint(__("..."), ...) |
|||
# ignore microtemplating e.g. msgprint("{{ _("server side translation") }}") |
|||
- pattern-not: frappe.msgprint("=~/\{\{.*\_.*\}\}/i", ...) |
|||
- patterns: |
|||
- pattern: frappe.throw("...", ...) |
|||
- pattern-not: frappe.throw(__("..."), ...) |
|||
# ignore microtemplating |
|||
- pattern-not: frappe.throw("=~/\{\{.*\_.*\}\}/i", ...) |
|||
message: | |
|||
All user facing text must be wrapped in translate function. Please refer to translation documentation. https://frappeframework.com/docs/user/en/guides/basics/translations |
|||
languages: [javascript] |
|||
severity: ERROR |
|||
Loading…
Reference in new issue