Semgrep: Feature - Add the ability to specify a maximum or expected number of violation before triggering an `ERROR`

[j-martin]

Describe the solution you'd like

For example, if the rule is violated 10 times it's a WARNING, but the 11 time it's ERROR . The idea is to progressively reduce the usage of a rule, ratcheting down the number progressively, and preventing new infractions.

For example, we want to deprecate the use of a library in our code without having to do a full rewrite.

On top of that, it would be nice if the target (expected) value would also trigger an ERROR if the violation count is lower than the target value to ensure as we remove the offending code we update the target value accordingly.

I am imagining those two options to be

For a random rule I copied of the docs (I do not endorse using md5 or sha1):

rules:
  - id: insecure-crypto-usage
    pattern-either:
      - pattern: hashlib.md5(...)
      - pattern: hashlib.sha1(...)
    message: "insecure cryptography hashing function"
    languages: [python]
    expected-maximum: 1
    severity: ERROR

rules:
  - id: insecure-crypto-usage
    pattern-either:
      - pattern: hashlib.md5(...)
      - pattern: hashlib.sha1(...)
    message: "insecure cryptography hashing function"
    languages: [python]
    expected-violations=2
    severity: ERROR

Describe alternatives you've considered

I ended up writing my own terrible linter, I could invoke semgrep from it, but at this point a plain regex is good enough right now.
https://gist.github.com/j-martin/1d32157d9f27afb48840397c3745de54

Additional context
Discussion: https://r2c-community.slack.com/archives/C018NJRRCJ0/p1604689682050900

I have been told it should be easy for me to implement. At this point, I am curious about what should be keywords names or what would be the preferred syntax to define those.

[mschwager]

Hi @j-martin,

Thanks for the feature request. I wonder if we can take a step back and consider other approaches to achieve your goals.

The idea is to progressively reduce the usage of a rule, ratcheting down the number progressively, and preventing new infractions. For example, we want to deprecate the use of a library in our code without having to do a full rewrite.

Instead of specifying a specific number of expected violations, would the following work:

1. Run Semgrep with the rule(s) you're considering
2. Mark all existing violations as nosem, thus ignoring them for the time being
3. Enable the Semgrep rule(s)
4. Slowly burn down the existing violations ignored with nosem

This allows you to ignore existing violations and "stop the bleeding" by preventing new violations from entering the codebase. Another advantage with this approach is it keeps the ignore annotation close to the violation itself, which makes it easier to track. This approach also avoids edge-cases like a violation is simultaneously removed and another is added at the same time, which will maintain the same number, but you haven't stopped new violations.

Does this solve your use-case?

[j-martin]

@mschwager I almost feel silly for asking the feature now since the workflow you are suggesting is exactly what we've been doing with every other linters. I'll close the issue.

[dlukeomalley]

@j-martin The idea is a good one regardless, thank you for filing the issue!

@mschwager @j-martin The Semgrep CI runner also only shows new issues introduced by a PR, which I think acts in the way you'd want (avoiding the need for adding nosem comments).

[j-martin]

@dlukeomalley in my case, I'd rather have the // nosem: do-not-do-that peppered as a reminder.

[dlukeomalley]

@j-martin makes sense. Do you have a sense of how you'll add // nosem to all of your findings and/or how receptive the rest of your team would be to that type of change?

The idea of establishing a "baseline" is really interesting to me, but the mechanics of it feel cumbersome both from the perspective of the work involved AND the buy-in required from the rest of your team. Curious what we could do in Semgrep to make it a smoother process for everyone.

[j-martin]

I've added manually in bulk via emacs+helm/ag+iedit, I could have done it with find + sed or if I wanted to be fancy, write a mock rule and add semgrep --autofix. My rule was simple enough for grep so sed would have been fine.

Culturally, we already do it for pylint, pmd, spotbugs, mypy, shellcheck, eslint, etc. so it was not an issue at all. I think the // no**** is expected from any kind of linter.

As far as adoption, my (internal) PR already got approved to add semgrep into our CI, and people already want to use it for Typescript and Python now.