Skip to content

CI: add cargo-audit #3250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CI: add cargo-audit #3250

wants to merge 1 commit into from

Conversation

dannywillems
Copy link
Member

@dannywillems dannywillems commented Jun 4, 2025

See initial PR in Mina: MinaProtocol/mina#16525

Opened #3251

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workflows should be related to triggers, not jobs. You can have multiple jobs per trigger. There are some workflows that we may want to independently change the trigger and that's when it makes the most sense to have a separate workflow with the same trigger.
Currently, these are all the workflows that we have defined:

benches-mina-prover-set-baseline.yml
benches-mina-prover.yml
benches.yml
ci-nightly.yml
ci.yml
fmt.yml
o1vm-ci.yml
o1vm-upload-mips-build.yml
saffron.yml
wasm.yml

saffron and o1vm are definitely separate projects where it probably makes sense to have separate workflows. ci and ci-nightly are based on different triggers. I would argue that fmt should just be a part of ci...

Is there anywhere else that you think this job could be more appropriately placed? Why not make it part of ci?

Comment on lines +16 to +20
- name: Install Rust
uses: actions-rs/toolchain@v1
with:
toolchain: stable
override: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deprecated, please use the same pattern that's in the rest of the CI jobs.

Suggested change
- name: Install Rust
uses: actions-rs/toolchain@v1
with:
toolchain: stable
override: true
- name: Use shared Rust toolchain setting up steps
uses: ./.github/actions/toolchain-shared
with:
rust_toolchain_version: stable

Comment on lines +22 to +23
- name: Install cargo-audit
run: cargo install cargo-audit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Install cargo-audit
run: cargo install cargo-audit
- uses: taiki-e/install-action@v2
with:
tool: cargo-audit

Comment on lines +25 to +35
- name: Run cargo audit
run: cargo audit --json > audit.json || true

- name: Check for critical vulnerabilities
run: |
CRITICAL_COUNT=$(jq '[.vulnerabilities.list[] | select(.advisory.severity == "critical")] | length' audit.json)
echo "Found $CRITICAL_COUNT critical vulnerabilities"
if [ "$CRITICAL_COUNT" -gt 0 ]; then
echo "Critical vulnerabilities detected!"
exit 1
fi
Copy link
Contributor

@richardpringle richardpringle Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... it looks like you're doing this in hard mode...

Why not just do cargo audit? It won't fail on warnings.

But I just went to look up the docs to see what kind of severity it will fail on and the docs say to use
https://github.com/rustsec/audit-check

(ref: https://github.com/rustsec/rustsec/blob/main/cargo-audit/README.md#using-cargo-audit-on-github-action)


- name: Upload audit report artifact
if: always()
uses: actions/upload-artifact@v4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to do this? Do the logs get deleted? Is anyone actually going to pull these?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants