frappe/semgrep-rules

Semgrep rules specific to Frappe Framework

Frappe Semgrep rules

Semgrep rules specific to Frappe Framework

These rules guard against typical mistakes or bad practices while working on Frappe Framework apps. Frappe's own apps also use this to simplify repetitive checks in code review process.

How to Use in my app

Github Action

You can use a GitHub Action to automatically validate changes with semgrep rules on all PRs.

name: Linters

on:
  pull_request: { }

jobs:
  linters:
    name: Frappe Linter
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3

      - name: Set up Python
        uses: actions/setup-python@v4
        with:
          python-version: '3.10'

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

      - name: Download semgrep
        run: pip install semgrep

      - name: Run Semgrep rules
        run: semgrep ci --config ./frappe-semgrep-rules/rules

Manually / running semgrep locally

• Install and verify that semgrep works semgrep --version
• clone the rules repository git clonse
• Run semgrep specifying rules folder as config semgrep --config=~/path/to/frappe-semgrep-rules/rules your_app_folder

Tip: You can optionally pass --severity=ERROR to ignore rules that produce warnings and only catch errors.

How to contribute new rules

• Read how Semgrep works: Getting started with semgrep rules
• Write a rule. Make sure it doesn't have too many false positives.
• Write positive and negative test cases for rule you are adding: Testing rules

semgrep-rules's Issues

discourage use of `get_doc` for new docs

Following are not equivalent, even though they seem to be

get_doc(...).insert()

d = new_doc()
d.update(...)
d.insert()

frappe/frappe#21162

flag non-translated text in alerts

example: frappe/frappe#16427

`enqueue_after_commit=True` should be set for triggers from doc hooks.

frappe/erpnext#35588

Semgrep error when iterating one and appending to another table.

Imagine having 2 child table and looping over the first one and appending to the second table in each iteration. Semgrep will throw frappe-modifying-child-tables-while-iterating error.

- 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/*"

Rule: hasattr / getattr with defaults shouldn't be used with `_dict`

example: frappe/frappe#17567

• Since _dict never raises attributeerror the default will never be used
• hasattr on _dict will always be true

semgrep alone probably isn't sufficient for this because of dynamic usage

Rules for code quality - `args` in argument

flag `doc.insert()` + `doc.save()`

a common issue that causes double computation.

Column labels don't have to be translatable

Assuming you want to translate the source string to the target language only once (and not re-translate the translation), the rules in https://github.com/frappe/semgrep-rules/blob/c0a1c4fcc4eb9fd7813187fa2420872cba8a3770/rules/report.py seem to be incorrect. Report column labels should not be translated in the backend since they will get translated in the frontend anyway.

One argument for this rule would be to mark translatable strings. But then the translation function will be run twice, what brings its own problems.

@ankush can we remove these rules? Or maybe the better way would be to remove the second translation in the frontend?

frappe.cache().set/get usage

This ain't safe for multitenancy

Example frappe/erpnext#32629