Semgrep: Filter out unsupported rules

[nbrahms]

Is your feature request related to a problem? Please describe.
Right now, new rules can be published that do not work on the system's version of semgrep. In this case, semgrep will just crash with a failure.

Describe the solution you'd like
Any method to support filtering out rules with unsupported features would work.

E.g. a rule with pattern-regex when run on semgrep 0.7.

Describe alternatives you've considered
I can imagine:

• Add a flag to ignore unrecognized rule keys
• Merely warn, but do not fail, by default, on unrecognized rule keys
• Add a rule parameter to specify the semgrep minimum version

[DrewDennison]

cc @rcoh who is thinking about pattern versions

[DrewDennison]

IMO it should crash with --strict otherwise ignore invalid rule and warn

[mschwager]

Here's what I'm thinking, add two new keys:

• minVersion - semgrep must have a newer version than this value, inclusively. E.g. minVersion: "1.3.0" means semgrep must be 1.3.0 or newer. Here we can warn older versions that they're using unsupported functionality.
• maxVersion - semgrep must have a earlier version than this value, inclusively. E.g. maxVersion: "1.8.0" means semgrep must be 1.8.0 or earlier. Useful for gracefully deprecating behavior.

With this in mind, we:

1. Check rules for their respective version requirements, and "fail open" (run them if we find none).
2. In non-strict mode we print a debug message that we're skipping any rules that don't meet the version requirements.
3. In strict mode we fail the run if we find any rules that don't meet the version requirements.
4. Finally we run the rule based on the above criteria.

Am I missing any failure/edge-cases here? Does this sound reasonable?

[nbrahms]

I'd amend (1) to print a warning, rather than error, if there is no version requirement and an unsupported YAML key is encountered.

[nbrahms]

Other than that, I'm on-board with this solution 🚀

[rcoh]

I did a design doc for this a few days ago, pasted below:

As updates are made to the Semgrep Rules language & semgrep-core expression syntax, we need a way to gracefully notify users if their version of semgrep is too old to run certain rules. Notable examples include:

• pattern-where-regex
• https://github.com/returntocorp/semgrep/issues/777

This will become more problematic as people run versions of semgrep in CI that are infrequently updated while the semgrep-rules repo undergoes frequent development to use the latest features available.

Summary

The apiVersion key will be added to all rules in semgrep-rules at the top level.

It represents the earliest version of semgrep that supports this rule. semgrep --test will error if the API Version set does not match the minimum version required by features used. Semgrep will gracefully ignore rules it does not support and warn to stderr. In --strict mode, an unsupported rule is an error.

Implementation Details

1. The baseline version will be 0.6.1 if no newer features are used.
2. A mapping of key → minimum version will be maintained inside of semgrep so that rule authors don’t need to remember that pattern-where-regex was introduced in 0.8.0. The apiVersion can either be added as an autofix or simply as a test failure message that the author can add.
3. semgrep-core will need to be modified to emit the minimum version for a given pattern. cc https://quip.com/dGFAEABSieb

Unfortunately, this can’t be fully automatic as there are some changes which can’t be detected via static analysis, for example, the recent changes that allow $A = x and return x to both be matched by x. To account for this case, CI will actually run all rules against their minimum specified version. This will hopefully catch most issues where a newer version of semgrep becomes implicitly required.

Pros of this strategy

Maintaining an apiVersion in this way enables a number of nice features down the road:

1. The ability to make breaking non-breaking changes by allowing different behavior based on rule version
2. The ability to automatically update rules to new syntax
3. The ability to test specific rules on their declared version to make sure that it’s actually correct.

Other Options Considered

• features: [a,b,c] to opt into specific experimental features: This doesn’t really address the core use case, and can be easily simulated with a version like 0.8.0-experimental to enable testing of bleeding edge features
• minSemgrepVersion: apiVersion is consistent with other YAML based languages like kubernetes , helm, and skaffold

[mschwager]

I did a design doc for this a few days ago, pasted below:

Ahh, good catch. There seems to be some gotchas in different approaches here - I'll give everything a review :+1:

[mschwager]

I've been getting a lot of mileage out of the Docker Compose file versioning scheme doc.

The apiVersion key will be added to all rules in semgrep-rules at the top level.

It represents the earliest version of semgrep that supports this rule.

It looks like both Docker Compose and Kubernetes configuration file version value is totally divorced from the version of the application itself. E.g. I'm currently using Docker engine version 19.03.11, Docker Compose version 1.25.4, and Docker Compose file format version 3.8. Is there a reason you'd like to tie our apiVersion to the version of semgrep itself?

I'm trying to imagine why the two are independent in the Docker Compose case. It looks like the Compose file format is tied to a Docker engine version. We don't really have this concern because we're not building on an external piece of software like Docker Compose is doing with the Docker engine. Although I still think it may be useful to keep the rule file version separate from the semgrep version so we can easily bump semgrep versions without worrying about the rule file version. Thoughts?

I wouldn't mind loosely following the Docker Compose scheme for a few reasons: 1.) it's fairly simple, someone can read and digest the information in ~15min, 2.) it's a similar system with YAML configuration files that may move independently of the application itself. With that in mind, my general plan would be to: 1.) loosely follow the Docker Compose scheme, 2.) rule file versions are independent of semgrep versions, although rule file version bumps will point to a semgrep version like the Compose compatibility matrix, 3.) lack of a version key (the current system) is considered the legacy version 1 so we can deal with that case separately, and we'll start with version 2. Thoughts?

I'd like to take a bit of extra time here to thoughtfully plan this out since we'll be dealing with this decision for a long time and don't want to paint ourselves in a corner and have to choose a new scheme in 6 months.

[rcoh]

On divorcing apiVersion from semgrep-version

I considered divorcing them, however, it just seemed like it would add unnecessary confusion and make it harder to figure out if the version of semgrep you had would work with the rule. I guess I just imagine us adding new keys / features on a weekly basis. The compatibility matrix just seems like an unnecessary piece of confusion for users.

To be honest, figuring out why some helm chart doesn't work on your k8s version is a pretty big pain (because the api versions evolve totally separately).

In my design, I certainly intended to be able to bump a semgrep version without updating the version of any existing rules -- in a sense the version means "minimumVersion." (perhaps this means we should rename the key "miniumumVersion"?

I think a hybrid approach may be ideal:

• For versions < 1.0, semgrep version and apiVersion are tightly coupled. During this time, we'll be frequently adding new features. Decoupling them means having to look at a table all the time and is an extra piece of friction for someone trying to update their CI version of semgrep to support some rule. ("This rule doesn't work, oh it's version 7. pip install semgrep==7. Oh that doesn't exist. Google google google. Oh here's the compatability matrix. 7 => 0.9.1").

• For versions >= 1.0, semgrep version and apiVersion become loosely coupled. During this time, breaking changes that require a specific version of semgrep to run a rule will be rarer. Most rules can just live at apiVersion: 1 and all will be well.

[DrewDennison]

How about as a v0 we:

1. catch the exception, warn, and continue (I think doable in just python but might need a bit of ocaml work to add to an "errors" key in the output)
2. print a message saying "seems to be unsupported features in one or more of your rules. Try upgrading semgrep to latest version"
3. circle back to the proprosal above in here when we tackle 901?

[DrewDennison]

seems like large maintenance overhead to support version compatibility logic while semgrep is still pre 1.0

[mschwager]

Yup, I split rule versioning out into #901

[mschwager]

I may be misunderstanding the issue, but it looks like we already warn and skip unsupported rules. Here's my test case:

$ ls configs/
test-child.yaml  test-multi.yaml  test-parent.yaml

$ cat configs/*
rules:
- id: test-child
  message: test-child
  severity: WARNING
  languages: [javascript]
  pattern-either:
    - pattern-unknown: $X(...)
rules:
- id: test-multi1
  message: test-multi1
  severity: WARNING
  languages: [javascript]
  pattern-unknown: $X(...)
- id: test-multi2
  message: test-multi2
  severity: WARNING
  languages: [javascript]
  pattern: $X(...)
rules:
- id: test-parent
  message: test-parent
  severity: WARNING
  languages: [javascript]
  pattern-unknown: $X(...)

$ python -m semgrep --config configs/ test.js
configs/test-child.yaml: inside rule id test-child, pattern fields can't look like this: invalid pattern name: pattern-unknown, valid pattern names are ['pattern-inside', 'pattern-not-inside', 'pattern-either', 'pattern-not', 'pattern', 'patterns', 'pattern-where-python', 'fix', 'equivalences', 'pattern-regex']
configs/test-multi.yaml has an invalid top-level rule key {'pattern-unknown'} at rule id test-multi1, can only have: ['equivalences', 'fix', 'id', 'languages', 'message', 'metadata', 'paths', 'pattern', 'pattern-either', 'pattern-regex', 'patterns', 'severity']
configs/test-parent.yaml has an invalid top-level rule key {'pattern-unknown'} at rule id test-parent, can only have: ['equivalences', 'fix', 'id', 'languages', 'message', 'metadata', 'paths', 'pattern', 'pattern-either', 'pattern-regex', 'patterns', 'severity']
test.js
WARNING rule:configs.test-multi2: test-multi2
2:    foo('bar', 1)
3:    foo('zzz', 2)
4:    foo('bar', 3)
5:    foo('not-bar', 1)

This ignores all unsupported rules while still running the correctly formatted one: test-multi2. We can even squelch the warnings by redirecting stderr:

$ python -m semgrep --config configs/ test.js 2> /dev/null
test.js
WARNING rule:configs.test-multi2: test-multi2
2:    foo('bar', 1)
3:    foo('zzz', 2)
4:    foo('bar', 3)
5:    foo('not-bar', 1)

Am I missing something?

[mschwager]

This ticket specifically mentions 0.7.0. Looks like sometime between then and now it was fixed:

$ ./semgrep --version
0.7.0
$ ./semgrep --config configs/ test.js 
semgrep encountered an error: invalid pattern name: pattern-unknown, valid pattern names are ['pattern-inside', 'pattern-not-inside', 'pattern-either', 'pattern-not', 'pattern', 'patterns', 'pattern-where-python', 'fix', 'equivalences']; this is not your fault. An error occurred while invoking the semgrep engine; please help us fix this by filing an an issue at https://semgrep.dev

[mschwager]

Closing out after a conversation with @nbrahms. Looks like this was fixed per the above two comments.