Support pre-commit configs for auditing?

[woodruffw]

Raised by @henryiii.

pre-commit and prek are popular git hook managers/runners. They allow users to define a .pre-commit-config.yaml file that configures one or more plugins to run. For example:

repos:
  - repo: https://github.com/henryiii/cibuildwheel
    rev: 58a0b274ea29c1e7899d45ab324b4ccdfc78d17d
    hooks:
      - id: mine

In the above rev is a Git reference, and is also an impostor commit. In principle this makes it a good candidate for flagging by the impostor-commit audit.

Some considerations:

• Is this the only audit we could currently run on pre-commit, or are there other security audits that make sense to run?
• prek also supports prek.toml, which would be outside of zizmor's current scope (we're focused on YAML at the moment). This doesn't seem to be widely adopted at the moment, but if it were to be then we'd only be able to audit some portion of prek usage.
• This would be zizmor's first non-GitHub-specific audit input, which would mean a change in policy in terms of what I'm supporting. It's also a bit of a risk insofar as I know a lot less about pre-commit than I do about GitHub Actions/Dependabot.

Alternatives:

• This is possibly a better fit for a separate tool, either a pre-existing one or something new. OTOH tool proliferation is bad.

Opening this for discussion for now, since I'm not sure I want to support this (but I do agree it's a good idea). I think a significant motivator for me supporting this will be whether there are other things we can detect to improve the security of the pre-commit ecosystem, rather than just a single audit (impostor-commit).

[henryiii]

You could check the pre-commit autoupdate --freeze / prek auto-update --freeze style (which I think Dependabot also supports, though I'm just now starting to try that out, because I want to try the 7 day cooldown, which pre-commit.ci doesn't have):

• rev: 58a0b274ea29c1e7899d45ab324b4ccdfc78d17d # frozen: v1.2.3 - ensure tag and SHA match
• You could check for pinned hashes instead of directly using tags (rev: v1.2.3 is valid, in fact rev: main is valid but strongly discouraged, pre-commit caches environments on this value, so it's assumed to be fixed)

[henryiii]

Another thought if you were looking for more ideas - I just noticed superfluous-actions (from #1800), and one thing that could be added is renamed/replaced checks. (I already do this in sp-repo-review) For example, the ones there:

• "https://github.com/psf/black" -> "https://github.com/psf/black-pre-commit-mirror"
• "https://github.com/asottile/blacken-docs" -> "https://github.com/adamchainz/blacken-docs"
• "https://github.com/charliermarsh/ruff-pre-commit" -> "https://github.com/astral-sh/ruff-pre-commit"
• "https://github.com/pre-commit/mirrors-prettier" -> "https://github.com/rbubley/mirrors-prettier"
• "https://github.com/executablebooks/mdformat" -> "https://github.com/hukkin/mdformat"

In sp-repo-review, renames are only checked for recommendations; for example, flake8 isn't there (though I really should add it back as an alternative to ruff, I do that for black), but it did get renamed IIRC.

If it's just a simple move, like ruff, then the rename is really not that important, but a few of those, like mirrors-prettier, are not simple renames and the old one is abandoned.

I don't see any other existing checks in sp-repo-review that would make sense for security.

PS: unrelated, but TOML parsing in Rust is excellent. :)

[woodruffw]

PS: unrelated, but TOML parsing in Rust is excellent. :)

Yep! The issue there isn't actually the parsing (Rust's YAML parsers are good enough as well), but span-aware parsing that also preserves comments. I think toml_edit can do this but I haven't looked super closely at it.

(Within zizmor, there are actually two YAML parsers: we use serde-yaml to load models for analysis, and then tree-sitter-yaml for span-aware parsing + fixes. I suspect a similar split would need to happen for TOML.)