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

FR: Web frontend for cargo vet certify #554

Open
djkoloski opened this issue Sep 20, 2023 · 2 comments
Open

FR: Web frontend for cargo vet certify #554

djkoloski opened this issue Sep 20, 2023 · 2 comments

Comments

@djkoloski
Copy link

This is a very large feature request. To be clear, I don't expect the cargo-vet team to take on this work. I hope this can be the start of a conversation about this kind of tooling.

We do unsafe code reviews for crates on Fuchsia. When we do, it's usually 1-3 people auditing every usage of unsafe in a crate. Our current process for this is:

  1. Create a git commit that adds or bumps the version of the crate.
  2. Leave a bunch of review comments in gerrit. These are a mix of unsafe justification (this is sound and here's why) and soundness issues (this is unsound and here's why).
  3. Once we've reviewed every usage of unsafe, we create a separate commit summarizing our audit in our audits.toml and check that in.
  4. Finally, we check in the change introducing the new crate (or updated version of an existing crate).

This has a few pitfalls:

  • We have to manually search for unsafe uses and there have been times when we've missed a few.
  • Because we have to go out of our way to certify, we sometimes forget to actually check in an audit. We could feasibly fix this with more process / actually using cargo vet right. We are very far from perfect right now. But I still think this would be a process improvement.
  • We regularly update several crates in a single commit, which means that comments from multiple crates get mixed together.
  • Because we version our crates in paths, the diffs are not always very clean. Files that should be marked as renamed often get marked as 100% deleted and then 100% added at another path. So it's not easy to navigate.
  • There's no comment filtering by anything other than resolved/unresolved. I want to send our audit notes upstream in a format that is easy for maintainers to incorporate.
    • Nothing would make me happier than if someone could click a button and add our review notes as safety comments.

My ideal tool would be something like this:

  • Web interface
  • Plug in the crate and version (or delta) to start a review
  • unsafe gets detected so we don't miss them during review
  • Share link with multiple people to add comments
  • Tag comments so upstream maintainers can address them easily (e.g. "safety comment", "soundness issue", "logic issue", etc)
  • Finishing a review:
    • Updates an audits.toml that works with cargo vet (the reviews we do on Fuchsia get checked in to our googlesource repo, then get aggregated with other audits at https://github.com/google/rust-crate-audits)
    • Generates an artifact we can either link people to or ship elsewhere (e.g. structured notes, markdown, or a permalink to the review)
  • Open-source so others can use it to audit crates (auditing difficulty is slowing adoption)

I suspect that others would benefit from a such a tool. We might have some Fuchsians with spare time who could contribute to developing it, and I would be willing to help out as well.

@bholley
Copy link
Collaborator

bholley commented Sep 22, 2023

Interesting!

We got partway through a web UI for the certify flow last year (see #293 and #330) but never saw it through to completion. We were intentionally leaving the review experience as out-of-scope (because building a good code-review tool is hard, and we're primarily leaning on sourcegraph for that), but I'm certainly open to something more ambitious if folks are excited to work on it!

@cemoktra
Copy link

cemoktra commented May 22, 2024

i'm want to add here, that recently source graph is a pain in the ass. a lot of diffs just show as "empty repository". While the command line alternative exists, auditing multiple thousand of lines via the local diff is a pain in the ass too.

maybe considering a configuration for local UI tools would be sufficient

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

No branches or pull requests

3 participants