Skip to content
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

cargo deny: Add new exceptions #30424

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Conversation

def-
Copy link
Contributor

@def- def- commented Nov 11, 2024

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@def- def- requested review from ParkMyCar and jkosh44 November 11, 2024 16:44
@def- def- enabled auto-merge November 11, 2024 16:44
Comment on lines +234 to +239
# `derivative` is unmaintained; consider using an alternative (unmaintained)
"RUSTSEC-2024-0388",
# Multiple soundness issues
"RUSTSEC-2024-0379",
# `instant` is unmaintained, and the author recommends using the maintained [`web-time`] crate instead.
"RUSTSEC-2024-0384",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we switch to the alternatives instead of ignoring these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely, but I didn't want to do it myself, I consider this list to be a big TODO basically

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my question is, why even add these exceptions in the first place? Is it breaking CI or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have a "security" pipeline which will ping me every day about this if I don't silence it: https://buildkite.com/materialize/security

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the commit history of this file, it doesn't look like we have a great track record of actually resolving these issues and removing the ignores. I don't mean to throw a wrench in this and I'm not at all familiar with the process but it sounds like something might be slightly broken with the process.

Maybe the security pings should be going somewhere else, but if our current process is to silence the pings without resolving them, then it doesn't sound like they're that useful in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

"Multiple soundness issues" for float parsing is a bit scary, I put up #30426 to remove our use. Otherwise IMO the "unmaintained" issues aren't great but aren't too bad either

Copy link
Member

Choose a reason for hiding this comment

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

Whoops sorry I didn't realize auto merge was enabled before I accepted the PR. If possible @def- would love your help in getting #30426 merged

Comment on lines +234 to +239
# `derivative` is unmaintained; consider using an alternative (unmaintained)
"RUSTSEC-2024-0388",
# Multiple soundness issues
"RUSTSEC-2024-0379",
# `instant` is unmaintained, and the author recommends using the maintained [`web-time`] crate instead.
"RUSTSEC-2024-0384",
Copy link
Member

Choose a reason for hiding this comment

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

"Multiple soundness issues" for float parsing is a bit scary, I put up #30426 to remove our use. Otherwise IMO the "unmaintained" issues aren't great but aren't too bad either

@def- def- merged commit 4b99f6e into MaterializeInc:main Nov 11, 2024
8 checks passed
@def- def- deleted the pr-cargo-advisory branch November 11, 2024 18: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.

3 participants