Skip to content

Fix cargo deny commands so they scan the whole workspace #1925

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

Merged
merged 1 commit into from
Apr 5, 2025

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Apr 5, 2025

When cargo deny commands are run from the top-level directory of a workspace, and no --manifest-path option is pased, they implicitly use the top-level Cargo.toml file as their manifest.

In workspaces where this is a virtual manifest, i.e. those where the top-level Cargo.toml defines only the workspace and not also a specific crate, running cargo deny without specifying any roots implicitly acts as though --workspace has been passed. (See https://embarkstudios.github.io/cargo-deny/cli/common.html for details.) In that situation, --workspace can be omitted and all crates in the workspace are still treated as roots for the scan.

But the gitoxide repository's top-level workspace's Cargo.toml file is not a virtual manifest. It defines a workspace, but it also defines the gitoxide crate. As a result, cargo deny commands, as run on CI in the cargo-deny and cargo-deny-advisories jobs, and locally via just audit, were not guaranteed to check the entire workspace. They used only the gitoxide crate as a root for scanning, rather than using all crates defined in the workspace as roots as when --workspace is used or implied.

It looks like this was not detected for three reasons:

  1. Most of the workspace was usually covered, especially on CI where, since no arguments were passed, cargo-deny-action used the defualt of --all-features. Most transitive dependencies inside and outside of the gitoxide project seem to be used by the gitoxide crate by in some feature configuration.

  2. The distinction between virtual and non-virtual top-level manifests of workspaces is subtle and not highlighted in the cargo deny documentation.

  3. In EmbarkStudios/cargo-deny-action, the default value of arguments contains --all-features but not --workspace (nor any other options). Intuitively it feels like the default value would scan all crates in the repository that the action is being run on. That does happen in the perhaps more common case that the top-level Cargo.toml defines no crate, but not here.

This was discovered in connection with #1924. It is why the cargo-deny-advisories job didn't catch RUSTSEC-2025-0022 (GHSA-4fcv-w3qc-ppgg) even as Dependabot did.

To fix it, this adds --workspace explicitly in both cargo deny jobs run on CI, as well as in the justfile.

This also adds --all-features:

  • On CI, adding --all-features is itself no change from before, since it is the implicit value of arguments. It has to be passed explicitly now that arguments has an explicit value.
  • In the justfile, adding --all-features does (at least in principle) make a difference.

When `cargo deny` commands are run from the top-level directory of
a workspace, and no `--manifest-path` option is pased, they
implicitly use the top-level `Cargo.toml` file as their manifest.

In workspaces where this is a virtual manifest, i.e. those where
the top-level `Cargo.toml` defines only the workspace and not also
a specific crate, running `cargo deny` without specifying any roots
implicitly acts as though `--workspace` has been passed. (See
https://embarkstudios.github.io/cargo-deny/cli/common.html for
details.) In that situation, `--workspace` can be omitted and all
crates in the workspace are still treated as roots for the scan.

But the gitoxide repository's top-level workspace's `Cargo.toml`
file is not a virtual manifest. It defines a workspace, but it also
defines the `gitoxide` crate. As a result, `cargo deny` commands,
as run on CI in the `cargo-deny` and `cargo-deny-advisories` jobs,
and locally via `just audit`, were not guaranteed to check the
entire workspace. They used only the `gitoxide` crate as a root for
scanning, rather than using all crates defined in the workspace as
roots as when `--workspace` is used or implied.

It looks like this was not detected for three reasons:

1. Most of the workspace was usually covered, especially on CI
   where, since no `arguments` were passed, `cargo-deny-action`
   used the defualt of `--all-features`. *Most* transitive
   dependencies inside and outside of the gitoxide project seem to
   be used by the `gitoxide` crate by in some feature
   configuration.

2. The distinction between virtual and non-virtual top-level
   manifests of workspaces is subtle and not highlighted in the
   `cargo deny` documentation.

3. In `EmbarkStudios/cargo-deny-action`, the default value of
   `arguments` contains `--all-features` but not `--workspace` (nor
   any other options). Intuitively it feels like the default value
   would scan all crates in the repository that the action is being
   run on. That does happen in the perhaps more common case that
   the top-level `Cargo.toml` defines no crate, but not here.

This was discovered in connection with GitoxideLabs#1924. It is why the
`cargo-deny-advisories` job didn't catch RUSTSEC-2025-0022
(GHSA-4fcv-w3qc-ppgg) even as Dependabot did.

To fix it, this adds `--workspace` explicitly in both `cargo deny`
jobs run on CI, as well as in the `justfile`.

This also adds `--all-features`:

- On CI, adding `--all-features` is itself no change from before,
  since it is the implicit value of `arguments`. It has to be
  passed explicitly now that `arguments` has an explicit value.

- In the `justfile`, adding `--all-features` does (at least in
  principle) make a difference.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Byron Byron enabled auto-merge April 5, 2025 01:37
Copy link
Member Author

@EliahKagan EliahKagan left a comment

Choose a reason for hiding this comment

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

No problem!

Examining the cargo-deny-advisories output reveals that, as of these changes, it now reports RUSTSEC-2025-0022, the missing advisory it had not been reporting before. 🚀

@Byron Byron merged commit 2ef2006 into GitoxideLabs:main Apr 5, 2025
20 of 21 checks passed
@EliahKagan EliahKagan deleted the run-ci/deny-workspace branch April 5, 2025 01:51
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