Browse Source

ci: move semgrep rules out of repo

version-14
Ankush Menat 3 years ago
parent
commit
488cb31265
12 changed files with 5 additions and 519 deletions
  1. +0
    -38
      .github/helper/semgrep_rules/README.md
  2. +0
    -64
      .github/helper/semgrep_rules/frappe_correctness.py
  3. +0
    -146
      .github/helper/semgrep_rules/frappe_correctness.yml
  4. +0
    -6
      .github/helper/semgrep_rules/security.py
  5. +0
    -25
      .github/helper/semgrep_rules/security.yml
  6. +0
    -44
      .github/helper/semgrep_rules/translate.js
  7. +0
    -61
      .github/helper/semgrep_rules/translate.py
  8. +0
    -64
      .github/helper/semgrep_rules/translate.yml
  9. +0
    -9
      .github/helper/semgrep_rules/ux.js
  10. +0
    -31
      .github/helper/semgrep_rules/ux.py
  11. +0
    -30
      .github/helper/semgrep_rules/ux.yml
  12. +5
    -1
      .github/workflows/semgrep.yml

+ 0
- 38
.github/helper/semgrep_rules/README.md View File

@@ -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

+ 0
- 64
.github/helper/semgrep_rules/frappe_correctness.py View File

@@ -1,64 +0,0 @@
import frappe
from frappe import _, flt

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"

+ 0
- 146
.github/helper/semgrep_rules/frappe_correctness.yml View File

@@ -1,146 +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-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

+ 0
- 6
.github/helper/semgrep_rules/security.py View File

@@ -1,6 +0,0 @@
def function_name(input):
# ruleid: frappe-codeinjection-eval
eval(input)

# ok: frappe-codeinjection-eval
eval("1 + 1")

+ 0
- 25
.github/helper/semgrep_rules/security.yml View File

@@ -1,25 +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

- id: frappe-sqli-format-strings
patterns:
- pattern-inside: |
@frappe.whitelist()
def $FUNC(...):
...
- pattern-either:
- pattern: frappe.db.sql("..." % ...)
- pattern: frappe.db.sql(f"...", ...)
- pattern: frappe.db.sql("...".format(...), ...)
message: |
Detected use of raw string formatting for SQL queries. This can lead to sql injection vulnerabilities. Refer security guidelines - https://github.com/frappe/erpnext/wiki/Code-Security-Guidelines
languages: [python]
severity: WARNING

+ 0
- 44
.github/helper/semgrep_rules/translate.js View File

@@ -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])

+ 0
- 61
.github/helper/semgrep_rules/translate.py View File

@@ -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

+ 0
- 64
.github/helper/semgrep_rules/translate.yml View File

@@ -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

+ 0
- 9
.github/helper/semgrep_rules/ux.js View File

@@ -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") }}. ');

+ 0
- 31
.github/helper/semgrep_rules/ux.py View File

@@ -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"))

+ 0
- 30
.github/helper/semgrep_rules/ux.yml View File

@@ -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

+ 5
- 1
.github/workflows/semgrep.yml View File

@@ -9,10 +9,14 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2

- name: Download Semgrep rules
run: git clone --depth 1 https://github.com/frappe/frappe-semgrep-rules.git

- uses: returntocorp/semgrep-action@v1
env:
SEMGREP_TIMEOUT: 120
with:
config: >-
r/python.lang.correctness
.github/helper/semgrep_rules
./frappe-semgrep-rules/rules

Loading…
Cancel
Save