From a104f9681ca20567fdd14cf3858ac072d528e8cf Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Tue, 11 Oct 2022 09:22:29 +0200 Subject: [PATCH] Warn on `unwrap`, remove last remaining instances Signed-off-by: Alexis Sellier --- radicle/src/git.rs | 14 ++++++++------ radicle/src/identity/project.rs | 23 ++++++++++++++++------- radicle/src/keystore.rs | 25 +++++++++++++++++++------ radicle/src/lib.rs | 1 + radicle/src/profile.rs | 6 +++--- radicle/src/ssh.rs | 2 +- radicle/src/storage.rs | 2 +- radicle/src/storage/git.rs | 13 ++++--------- radicle/src/storage/git/transport.rs | 2 ++ radicle/src/test.rs | 1 + radicle/src/test/storage.rs | 2 +- 11 files changed, 57 insertions(+), 34 deletions(-) diff --git a/radicle/src/git.rs b/radicle/src/git.rs index e5c064cc..01fabcb7 100644 --- a/radicle/src/git.rs +++ b/radicle/src/git.rs @@ -45,8 +45,7 @@ pub enum ListRefsError { impl From<&RemoteId> for RefString { fn from(id: &RemoteId) -> Self { - // PublicKey strings contain only legal characters. - RefString::try_from(id.to_string()).unwrap() + RefString::try_from(id.to_string()).expect("encoded public keys are valid ref strings") } } @@ -137,7 +136,7 @@ pub fn initial_commit<'a>( let tree_id = repo.index()?.write_tree()?; let tree = repo.find_tree(tree_id)?; let oid = repo.commit(None, sig, sig, "Initial commit", &tree, &[])?; - let commit = repo.find_commit(oid).unwrap(); + let commit = repo.find_commit(oid)?; Ok(commit) } @@ -154,20 +153,23 @@ pub fn commit<'a>( let tree_id = repo.index()?.write_tree()?; let tree = repo.find_tree(tree_id)?; let oid = repo.commit(Some(target.as_str()), &sig, &sig, message, &tree, &[parent])?; - let commit = repo.find_commit(oid).unwrap(); + let commit = repo.find_commit(oid)?; Ok(commit) } /// Push the refs to the radicle remote. pub fn push(repo: &git2::Repository) -> Result<(), git2::Error> { + // NOTE: This function is going away soon. + #![allow(clippy::unwrap_used)] + let mut remote = repo.find_remote("rad")?; - let refspecs = remote.push_refspecs().unwrap(); + let refspecs = remote.push_refspecs()?; let refspec = refspecs.into_iter().next().unwrap().unwrap(); // The `git2` crate doesn't seem to support push refspecs with '*' in them, // so we manually replace it with the current branch. - let head = repo.head().unwrap(); + let head = repo.head()?; let branch = head.shorthand().unwrap(); let refspec = refspec.replace('*', branch); diff --git a/radicle/src/identity/project.rs b/radicle/src/identity/project.rs index f5a1deb1..48cafaf9 100644 --- a/radicle/src/identity/project.rs +++ b/radicle/src/identity/project.rs @@ -270,8 +270,8 @@ impl Doc { } } - pub fn from_json(bytes: &[u8]) -> Result { - serde_json::from_slice(bytes) + pub fn from_json(bytes: &[u8]) -> Result { + serde_json::from_slice(bytes).map_err(DocError::from) } pub fn verified(self) -> Result, VerificationError> { @@ -375,10 +375,16 @@ pub enum IdentityError { MismatchedRoot(Oid), #[error("commit signature for {0} is invalid: {1}")] InvalidSignature(PublicKey, crypto::Error), + #[error("commit message for {0} is invalid")] + InvalidCommitMessage(Oid), + #[error("commit trailers for {0} are invalid: {1}")] + InvalidCommitTrailers(Oid, trailers::Error), #[error("quorum not reached: {0} signatures for a threshold of {1}")] QuorumNotReached(usize, usize), #[error("identity document error: {0}")] Doc(#[from] DocError), + #[error("the document root is missing")] + MissingRoot, } #[derive(Clone, Debug, PartialEq, Eq)] @@ -426,10 +432,10 @@ impl Identity { let mut history = repo.revwalk(head)?.collect::>(); // Retrieve root document. - let root_oid = history.pop().unwrap()?.into(); + let root_oid = history.pop().ok_or(IdentityError::MissingRoot)??.into(); let root_blob = Doc::blob_at(root_oid, repo)?; let root: git::Oid = root_blob.id().into(); - let trusted = Doc::from_json(root_blob.content()).unwrap(); + let trusted = Doc::from_json(root_blob.content())?; let revision = history.len() as u32; let mut trusted = trusted.verified()?; @@ -442,11 +448,14 @@ impl Identity { let blob = Doc::blob_at(oid.into(), repo)?; let untrusted = Doc::from_json(blob.content()).map_err(DocError::from)?; let untrusted = untrusted.verified()?; - let commit = repo.commit(oid.into())?.unwrap(); - let msg = commit.message_raw().unwrap(); + let commit = repo.commit(oid.into())?; + let msg = commit + .message_raw() + .ok_or_else(|| IdentityError::InvalidCommitMessage(oid.into()))?; // Keys that signed the *current* document version. - signatures = trailers::parse_signatures(msg).unwrap(); + signatures = trailers::parse_signatures(msg) + .map_err(|e| IdentityError::InvalidCommitTrailers(oid.into(), e))?; for (pk, sig) in &signatures { if let Err(err) = pk.verify(blob.content(), sig) { return Err(IdentityError::InvalidSignature(*pk, err)); diff --git a/radicle/src/keystore.rs b/radicle/src/keystore.rs index 9e7caa17..c9aa80a1 100644 --- a/radicle/src/keystore.rs +++ b/radicle/src/keystore.rs @@ -5,8 +5,21 @@ use std::os::unix::fs::DirBuilderExt; use std::os::unix::prelude::OpenOptionsExt; use std::path::{Path, PathBuf}; +use thiserror::Error; + +use crate::crypto; use crate::crypto::{PublicKey, SecretKey}; +#[derive(Debug, Error)] +pub enum Error { + #[error(transparent)] + Io(#[from] io::Error), + #[error("invalid key file format: {0}")] + FromUtf8Error(#[from] std::string::FromUtf8Error), + #[error("invalid key format: {0}")] + Crypto(#[from] crypto::Error), +} + pub struct UnsafeKeystore { path: PathBuf, } @@ -18,7 +31,7 @@ impl UnsafeKeystore { } } - pub fn put(&mut self, public: &PublicKey, secret: &SecretKey) -> Result<(), io::Error> { + pub fn put(&mut self, public: &PublicKey, secret: &SecretKey) -> Result<(), Error> { // TODO: Zeroize secret key. let public = public.to_pem(); let secret = secret.to_pem(); @@ -45,14 +58,14 @@ impl UnsafeKeystore { Ok(()) } - pub fn get(&self) -> Result<(PublicKey, SecretKey), io::Error> { + pub fn get(&self) -> Result<(PublicKey, SecretKey), Error> { let public = fs::read(self.path.join("radicle.pub"))?; - let public = String::from_utf8(public).unwrap(); - let public = PublicKey::from_pem(&public).unwrap(); + let public = String::from_utf8(public)?; + let public = PublicKey::from_pem(&public)?; let secret = fs::read(self.path.join("radicle"))?; - let secret = String::from_utf8(secret).unwrap(); - let secret = SecretKey::from_pem(&secret).unwrap(); + let secret = String::from_utf8(secret)?; + let secret = SecretKey::from_pem(&secret)?; Ok((public, secret)) } diff --git a/radicle/src/lib.rs b/radicle/src/lib.rs index 1bb8cefa..1091006e 100644 --- a/radicle/src/lib.rs +++ b/radicle/src/lib.rs @@ -1,4 +1,5 @@ #![allow(clippy::match_like_matches_macro)] +#![cfg_attr(not(test), warn(clippy::unwrap_used))] pub mod collections; pub mod crypto; pub mod git; diff --git a/radicle/src/profile.rs b/radicle/src/profile.rs index 6ffb6c85..3f8feee9 100644 --- a/radicle/src/profile.rs +++ b/radicle/src/profile.rs @@ -14,7 +14,7 @@ use std::path::PathBuf; use std::{env, io}; use crate::crypto::{KeyPair, PublicKey, SecretKey, Signature, Signer}; -use crate::keystore::UnsafeKeystore; +use crate::keystore::{Error, UnsafeKeystore}; use crate::node; use crate::storage::git::Storage; @@ -42,7 +42,7 @@ pub struct Profile { } impl Profile { - pub fn init(keypair: KeyPair) -> Result { + pub fn init(keypair: KeyPair) -> Result { let home = self::home()?; let mut keystore = UnsafeKeystore::new(&home.join("keys")); let public = keypair.pk.into(); @@ -61,7 +61,7 @@ impl Profile { }) } - pub fn load() -> Result { + pub fn load() -> Result { let home = self::home()?; let (public, secret) = UnsafeKeystore::new(&home.join("keys")).get()?; let signer = UnsafeSigner { public, secret }; diff --git a/radicle/src/ssh.rs b/radicle/src/ssh.rs index 268c37c1..6b147da9 100644 --- a/radicle/src/ssh.rs +++ b/radicle/src/ssh.rs @@ -144,7 +144,7 @@ impl radicle_ssh::key::Private for SecretKey { let public = r.read_string()?; let pair = r.read_string()?; let _comment = r.read_string()?; - let key = crypto::SecretKey::from_slice(pair).unwrap(); + let key = crypto::SecretKey::from_slice(pair)?; if public != key.public_key().as_ref() { return Err(SecretKeyError::Mismatch); diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index 5956f03f..df124357 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -247,7 +247,7 @@ pub trait ReadRepository { remote: &RemoteId, reference: &RefStr, ) -> Result; - fn commit(&self, oid: Oid) -> Result, git2::Error>; + fn commit(&self, oid: Oid) -> Result; fn revwalk(&self, head: Oid) -> Result; fn reference_oid(&self, remote: &RemoteId, reference: &RefStr) -> Result; fn references(&self, remote: &RemoteId) -> Result; diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index 6be8e5ed..6c5c131d 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -28,7 +28,6 @@ pub static REMOTES_GLOB: Lazy = pub static SIGNATURES_GLOB: Lazy = Lazy::new(|| refspec::pattern!("refs/remotes/*/radicle/signature")); -// FIXME: Should this be here? #[derive(Error, Debug)] pub enum ProjectError { #[error("identity branches diverge from each other")] @@ -424,14 +423,10 @@ impl ReadRepository for Repository { self.backend.find_reference(&name).map_err(git::Error::from) } - fn commit(&self, oid: Oid) -> Result, git2::Error> { - self.backend.find_commit(oid.into()).map(Some).or_else(|e| { - if git::ext::is_not_found_err(&e) { - Ok(None) - } else { - Err(e) - } - }) + fn commit(&self, oid: Oid) -> Result { + self.backend + .find_commit(oid.into()) + .map_err(git::Error::from) } fn revwalk(&self, head: Oid) -> Result { diff --git a/radicle/src/storage/git/transport.rs b/radicle/src/storage/git/transport.rs index 4bb729c2..ea02200f 100644 --- a/radicle/src/storage/git/transport.rs +++ b/radicle/src/storage/git/transport.rs @@ -52,10 +52,12 @@ impl Smart { /// Take a stream from the map. /// This makes the stream unavailable until it is re-inserted. pub fn take(&self, id: &Id) -> Option { + #[allow(clippy::unwrap_used)] self.streams.lock().unwrap().remove(id) } pub fn insert(&self, id: Id, stream: Stream) { + #[allow(clippy::unwrap_used)] self.streams.lock().unwrap().insert(id, stream); } } diff --git a/radicle/src/test.rs b/radicle/src/test.rs index 2fd776b2..0ca2dccd 100644 --- a/radicle/src/test.rs +++ b/radicle/src/test.rs @@ -1,3 +1,4 @@ +#![allow(clippy::unwrap_used)] pub mod arbitrary; pub mod assert; pub mod fixtures; diff --git a/radicle/src/test/storage.rs b/radicle/src/test/storage.rs index 58457965..cd403e5c 100644 --- a/radicle/src/test/storage.rs +++ b/radicle/src/test/storage.rs @@ -97,7 +97,7 @@ impl ReadRepository for MockRepository { todo!() } - fn commit(&self, _oid: Oid) -> Result, git2::Error> { + fn commit(&self, _oid: Oid) -> Result { todo!() }