search

trailofbits / semgrep-rules

Semgrep queries developed by Trail of Bits.
GNU Affero General Public License v3.0
330 stars 32 forks source link

Fix broken validation CI step #52

Closed mschwager closed 9 months ago

mschwager commented 9 months ago

The validate CI step is broken because it thinks our GHA YAML files are Semgrep rules: https://github.com/trailofbits/semgrep-rules/actions/runs/7656054543/job/21278776025?pr=49. This PR changes validation to only validate top-level, non-hidden directories.

mschwager commented 9 months ago

LGTM. Wondering why the --exclude flag does not work here.

Good question, I originally thought that would work too. The problem is that --exclude is for target files (e.g. Python code), not Semgrep rule files. The --validate flag is a weird one because it does not operate on any target files. This is why we have to specify multiple --config flags in this hacky way.

Another thought might be: can we use --exclude-rule? But the problem here is that the YAML files being considered as rules (the GitHub Action files) are not valid rules, so Semgrep errors out before --exclude-rule could do its thing.

Another option here would be removing the --validate step. IIRC calling --test performs all the same actions as --validate, except calling p/semgrep-rule-lints on the rules. And we do that manually later in the CI job anyway.

GrosQuildu commented 9 months ago

Validate does runs a combination of Semgrep rules and OCaml checks. You say that test runs OCaml checks (whatever these are)? Anyway I would rather keep validate as separate step - it could be confusing if we error/warn in test about technical bug in a rule rather than expected semantic bug.

Let's merge imho.