Fix cargo deny commands so they scan the whole workspace#1925
Merged
Sebastian Thiel (Byron) merged 1 commit intoApr 5, 2025
Merged
Conversation
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.
Sebastian Thiel (Byron)
approved these changes
Apr 5, 2025
Sebastian Thiel (Byron)
left a comment
Member
There was a problem hiding this comment.
Thanks a lot!
Sebastian Thiel (Byron)
enabled auto-merge
April 5, 2025 01:37
Member
Author
There was a problem hiding this comment.
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. 🚀
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When
cargo denycommands are run from the top-level directory of a workspace, and no--manifest-pathoption is pased, they implicitly use the top-levelCargo.tomlfile as their manifest.In workspaces where this is a virtual manifest, i.e. those where the top-level
Cargo.tomldefines only the workspace and not also a specific crate, runningcargo denywithout specifying any roots implicitly acts as though--workspacehas been passed. (See https://embarkstudios.github.io/cargo-deny/cli/common.html for details.) In that situation,--workspacecan 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.tomlfile is not a virtual manifest. It defines a workspace, but it also defines thegitoxidecrate. As a result,cargo denycommands, as run on CI in thecargo-denyandcargo-deny-advisoriesjobs, and locally viajust audit, were not guaranteed to check the entire workspace. They used only thegitoxidecrate as a root for scanning, rather than using all crates defined in the workspace as roots as when--workspaceis used or implied.It looks like this was not detected for three reasons:
Most of the workspace was usually covered, especially on CI where, since no
argumentswere passed,cargo-deny-actionused the defualt of--all-features. Most transitive dependencies inside and outside of the gitoxide project seem to be used by thegitoxidecrate by in some feature configuration.The distinction between virtual and non-virtual top-level manifests of workspaces is subtle and not highlighted in the
cargo denydocumentation.In
EmbarkStudios/cargo-deny-action, the default value ofargumentscontains--all-featuresbut 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-levelCargo.tomldefines no crate, but not here.This was discovered in connection with #1924. It is why the
cargo-deny-advisoriesjob didn't catch RUSTSEC-2025-0022 (GHSA-4fcv-w3qc-ppgg) even as Dependabot did.To fix it, this adds
--workspaceexplicitly in bothcargo denyjobs run on CI, as well as in thejustfile.This also adds
--all-features:--all-featuresis itself no change from before, since it is the implicit value ofarguments. It has to be passed explicitly now thatargumentshas an explicit value.justfile, adding--all-featuresdoes (at least in principle) make a difference.