From 7aee5261d50233affd274265d81e1a6fafaa8f5f Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Thu, 26 Dec 2019 15:10:22 -0500 Subject: [PATCH 1/2] Remove some usage of `unwrap()` This commit reduces the usage of unwrap within the request/response lifecycle, and documents a few remaining cases. Usage of unwrap in tests, binaries, or server boot code is generally okay and was not reviewed. Remaining work: * Some usage of unwrap in `git.rs` and `srcc/tasks/` background jobs. This code is not directly in the request/response lifecycle, but should be reviewed. * Current usage in middleware seems okay, but could be documented. In most cases, such as with the handler in `AroundMiddleware`, the conduit API ensures a handler is set. --- src/controllers/krate/search.rs | 3 ++- src/controllers/user/me.rs | 16 +++++------ src/controllers/user/other.rs | 5 +++- src/middleware/block_traffic.rs | 1 + src/models/badge.rs | 1 + src/models/keyword.rs | 1 + src/models/team.rs | 7 ++++- src/s3/Cargo.toml | 1 + src/s3/error.rs | 30 +++++++++++++++++++++ src/s3/lib.rs | 48 ++++++++++++++++++--------------- src/uploaders.rs | 16 +++++++---- src/util.rs | 7 +++++ 12 files changed, 98 insertions(+), 38 deletions(-) create mode 100644 src/s3/error.rs diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 05aaa6c9bc1..7a50cb89223 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -7,6 +7,7 @@ use crate::controllers::cargo_prelude::*; use crate::controllers::helpers::Paginate; use crate::models::{Crate, CrateBadge, CrateOwner, CrateVersions, OwnerKind, Version}; use crate::schema::*; +use crate::util::errors::{bad_request, ChainError}; use crate::views::EncodableCrate; use crate::models::krate::{canon_crate_name, ALL_COLUMNS}; @@ -110,7 +111,7 @@ pub fn search(req: &mut dyn Request) -> AppResult { letter .chars() .next() - .unwrap() + .chain_error(|| bad_request("letter value must contain 1 character"))? .to_lowercase() .collect::() ); diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 2643163db13..f0bc1ad4d40 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -4,7 +4,7 @@ use crate::controllers::frontend_prelude::*; use crate::controllers::helpers::*; use crate::email; -use crate::util::errors::AppError; +use crate::util::errors::{AppError, ChainError}; use crate::models::{ CrateOwner, Email, Follow, NewEmail, OwnerKind, User, Version, VersionOwnerAction, @@ -141,12 +141,10 @@ pub fn update_user(req: &mut dyn Request) -> AppResult { let user_update: UserUpdate = serde_json::from_str(&body).map_err(|_| bad_request("invalid json request"))?; - if user_update.user.email.is_none() { - return Err(bad_request("empty email rejected")); - } - - let user_email = user_update.user.email.unwrap(); - let user_email = user_email.trim(); + let user_email = match &user_update.user.email { + Some(email) => email.trim(), + None => return Err(bad_request("empty email rejected")), + }; if user_email == "" { return Err(bad_request("empty email rejected")); @@ -199,7 +197,9 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult { use diesel::update; let user = req.user()?; - let name = &req.params()["user_id"].parse::().ok().unwrap(); + let name = &req.params()["user_id"] + .parse::() + .chain_error(|| bad_request("invalid user_id"))?; let conn = req.db_conn()?; // need to check if current user matches user to be updated diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index 3f69cf6e8e8..7ee1a94215f 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -2,6 +2,7 @@ use crate::controllers::frontend_prelude::*; use crate::models::{CrateOwner, OwnerKind, User}; use crate::schema::{crate_owners, crates, users}; +use crate::util::errors::ChainError; use crate::views::EncodablePublicUser; /// Handles the `GET /users/:user_id` route. @@ -28,7 +29,9 @@ pub fn show(req: &mut dyn Request) -> AppResult { pub fn stats(req: &mut dyn Request) -> AppResult { use diesel::dsl::sum; - let user_id = &req.params()["user_id"].parse::().ok().unwrap(); + let user_id = &req.params()["user_id"] + .parse::() + .chain_error(|| bad_request("invalid user_id"))?; let conn = req.db_conn()?; let data = CrateOwner::by_owner_kind(OwnerKind::User) diff --git a/src/middleware/block_traffic.rs b/src/middleware/block_traffic.rs index c234f7ce7ac..690799dae67 100644 --- a/src/middleware/block_traffic.rs +++ b/src/middleware/block_traffic.rs @@ -54,6 +54,7 @@ impl Handler for BlockTraffic { Please open an issue at https://github.com/rust-lang/crates.io \ or email help@crates.io \ and provide the request id {}", + // Heroku should always set this header req.headers().find("X-Request-Id").unwrap()[0] ); let mut headers = HashMap::new(); diff --git a/src/models/badge.rs b/src/models/badge.rs index c6c986b8e78..675a18b8665 100644 --- a/src/models/badge.rs +++ b/src/models/badge.rs @@ -109,6 +109,7 @@ impl Queryable for Badge { impl Badge { pub fn encodable(self) -> EncodableBadge { + // The serde attributes on Badge ensure it can be deserialized to EncodableBadge serde_json::from_value(serde_json::to_value(self).unwrap()).unwrap() } diff --git a/src/models/keyword.rs b/src/models/keyword.rs index 24149a50ac6..03cb0fbe2f5 100644 --- a/src/models/keyword.rs +++ b/src/models/keyword.rs @@ -53,6 +53,7 @@ impl Keyword { if name.is_empty() { return false; } + // unwrap is okay because name is non-empty name.chars().next().unwrap().is_alphanumeric() && name .chars() diff --git a/src/models/team.rs b/src/models/team.rs index f1a2f92f123..0baf95cf504 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -68,6 +68,10 @@ impl<'a> NewTeam<'a> { impl Team { /// Tries to create the Team in the DB (assumes a `:` has already been found). + /// + /// # Panics + /// + /// This function will panic if login contains less than 2 `:` characters. pub fn create_or_update( app: &App, conn: &PgConnection, @@ -76,10 +80,11 @@ impl Team { ) -> AppResult { // must look like system:xxxxxxx let mut chunks = login.split(':'); + // unwrap is okay, split on an empty string still has 1 chunk match chunks.next().unwrap() { // github:rust-lang:owners "github" => { - // Ok to unwrap since we know one ":" is contained + // unwrap is documented above as part of the calling contract let org = chunks.next().unwrap(); let team = chunks.next().ok_or_else(|| { cargo_err( diff --git a/src/s3/Cargo.toml b/src/s3/Cargo.toml index 7617ab30e64..88755aa4e02 100644 --- a/src/s3/Cargo.toml +++ b/src/s3/Cargo.toml @@ -6,6 +6,7 @@ authors = ["Alex Crichton "] license = "MIT OR Apache-2.0" repository = "https://github.com/rust-lang/crates.io" description = "Interaction between crates.io and S3 for storing crate files" +edition = "2018" [lib] diff --git a/src/s3/error.rs b/src/s3/error.rs new file mode 100644 index 00000000000..158eb87163c --- /dev/null +++ b/src/s3/error.rs @@ -0,0 +1,30 @@ +use std::fmt; + +use openssl::error::ErrorStack; +use reqwest::Error as ReqwestError; + +pub enum Error { + Openssl(ErrorStack), + Reqwest(ReqwestError), +} + +impl From for Error { + fn from(stack: ErrorStack) -> Self { + Self::Openssl(stack) + } +} + +impl From for Error { + fn from(error: ReqwestError) -> Self { + Self::Reqwest(error) + } +} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Openssl(stack) => stack.fmt(f), + Self::Reqwest(error) => error.fmt(f), + } + } +} diff --git a/src/s3/lib.rs b/src/s3/lib.rs index 6b99ed57163..623815a7af7 100644 --- a/src/s3/lib.rs +++ b/src/s3/lib.rs @@ -1,16 +1,15 @@ #![warn(clippy::all, rust_2018_idioms)] -extern crate base64; -extern crate chrono; -extern crate openssl; -extern crate reqwest; - use base64::encode; use chrono::prelude::Utc; +use openssl::error::ErrorStack; use openssl::hash::MessageDigest; use openssl::pkey::PKey; use openssl::sign::Signer; -use reqwest::header; +use reqwest::{header, Body, Client, Response}; + +mod error; +pub use error::Error; #[derive(Clone, Debug)] pub struct Bucket { @@ -40,20 +39,20 @@ impl Bucket { pub fn put( &self, - client: &reqwest::Client, + client: &Client, path: &str, content: R, content_length: u64, content_type: &str, extra_headers: header::HeaderMap, - ) -> reqwest::Result { + ) -> Result { let path = if path.starts_with('/') { &path[1..] } else { path }; let date = Utc::now().to_rfc2822(); - let auth = self.auth("PUT", &date, path, "", content_type); + let auth = self.auth("PUT", &date, path, "", content_type)?; let url = self.url(path); client @@ -62,23 +61,20 @@ impl Bucket { .header(header::CONTENT_TYPE, content_type) .header(header::DATE, date) .headers(extra_headers) - .body(reqwest::Body::sized(content, content_length)) + .body(Body::sized(content, content_length)) .send()? .error_for_status() + .map_err(Into::into) } - pub fn delete( - &self, - client: &reqwest::Client, - path: &str, - ) -> reqwest::Result { + pub fn delete(&self, client: &Client, path: &str) -> Result { let path = if path.starts_with('/') { &path[1..] } else { path }; let date = Utc::now().to_rfc2822(); - let auth = self.auth("DELETE", &date, path, "", ""); + let auth = self.auth("DELETE", &date, path, "", "")?; let url = self.url(path); client @@ -87,6 +83,7 @@ impl Bucket { .header(header::AUTHORIZATION, auth) .send()? .error_for_status() + .map_err(Into::into) } pub fn host(&self) -> String { @@ -101,7 +98,14 @@ impl Bucket { ) } - fn auth(&self, verb: &str, date: &str, path: &str, md5: &str, content_type: &str) -> String { + fn auth( + &self, + verb: &str, + date: &str, + path: &str, + md5: &str, + content_type: &str, + ) -> Result { let string = format!( "{verb}\n{md5}\n{ty}\n{date}\n{headers}{resource}", verb = verb, @@ -112,12 +116,12 @@ impl Bucket { resource = format!("/{}/{}", self.name, path) ); let signature = { - let key = PKey::hmac(self.secret_key.as_bytes()).unwrap(); - let mut signer = Signer::new(MessageDigest::sha1(), &key).unwrap(); - signer.update(string.as_bytes()).unwrap(); - encode(&signer.sign_to_vec().unwrap()[..]) + let key = PKey::hmac(self.secret_key.as_bytes())?; + let mut signer = Signer::new(MessageDigest::sha1(), &key)?; + signer.update(string.as_bytes())?; + encode(&signer.sign_to_vec()?[..]) }; - format!("AWS {}:{}", self.access_key, signature) + Ok(format!("AWS {}:{}", self.access_key, signature)) } fn url(&self, path: &str) -> String { diff --git a/src/uploaders.rs b/src/uploaders.rs index 90412ce7482..8bd869fd2e4 100644 --- a/src/uploaders.rs +++ b/src/uploaders.rs @@ -1,5 +1,6 @@ use conduit::Request; use flate2::read::GzDecoder; +use openssl::error::ErrorStack; use openssl::hash::{Hasher, MessageDigest}; use reqwest::header; @@ -88,6 +89,11 @@ impl Uploader { /// Uploads a file using the configured uploader (either `S3`, `Local`). /// /// It returns the path of the uploaded file. + /// + /// # Panics + /// + /// This function can panic on an `Self::Local` during development. + /// Production and tests use `Self::S3` which should not panic. pub fn upload( &self, client: &reqwest::Client, @@ -135,7 +141,7 @@ impl Uploader { let mut body = Vec::new(); LimitErrorReader::new(req.body(), maximums.max_upload_size).read_to_end(&mut body)?; verify_tarball(krate, vers, &body, maximums.max_unpack_size)?; - let checksum = hash(&body); + let checksum = hash(&body)?; let content_length = body.len() as u64; let content = Cursor::new(body); let mut extra_headers = header::HeaderMap::new(); @@ -221,8 +227,8 @@ fn verify_tarball( Ok(()) } -fn hash(data: &[u8]) -> Vec { - let mut hasher = Hasher::new(MessageDigest::sha256()).unwrap(); - hasher.update(data).unwrap(); - hasher.finish().unwrap().to_vec() +fn hash(data: &[u8]) -> Result, ErrorStack> { + let mut hasher = Hasher::new(MessageDigest::sha256())?; + hasher.update(data)?; + Ok(hasher.finish()?.to_vec()) } diff --git a/src/util.rs b/src/util.rs index e16471a41ca..8dac77a5a69 100644 --- a/src/util.rs +++ b/src/util.rs @@ -17,6 +17,13 @@ mod request_helpers; mod request_proxy; pub mod rfc3339; +/// Serialize a value to JSON and build a status 200 Response +/// +/// This helper sets appropriate values for `Content-Type` and `Content-Length`. +/// +/// # Panics +/// +/// This function will panic if serialization fails. pub fn json_response(t: &T) -> Response { let json = serde_json::to_string(t).unwrap(); let mut headers = HashMap::new(); From a318cb365d8db7c7edde16fd24b425b5cf0705e6 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Fri, 3 Jan 2020 20:37:53 -0500 Subject: [PATCH 2/2] impl Debug and std::error::Error for s3::Error --- src/s3/error.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/s3/error.rs b/src/s3/error.rs index 158eb87163c..8e5d65ce6cb 100644 --- a/src/s3/error.rs +++ b/src/s3/error.rs @@ -3,6 +3,7 @@ use std::fmt; use openssl::error::ErrorStack; use reqwest::Error as ReqwestError; +#[derive(Debug)] pub enum Error { Openssl(ErrorStack), Reqwest(ReqwestError), @@ -28,3 +29,5 @@ impl fmt::Display for Error { } } } + +impl std::error::Error for Error {}