clippy

src/cache/disk.rs

@@ -13,6 +13,7 @@ use futures::StreamExt;
 use log::LevelFilter;
 use md5::digest::generic_array::GenericArray;
 use md5::{Digest, Md5};
+use sodiumoxide::hex;
 use sqlx::sqlite::SqliteConnectOptions;
 use sqlx::{ConnectOptions, Sqlite, SqlitePool, Transaction};
 use tokio::fs::{remove_file, rename, File};
@@ -185,27 +186,7 @@ async fn db_listener(
             for item in items {
                 debug!("deleting file due to exceeding cache size");
                 size_freed += item.size as u64;
-                tokio::spawn(async move {
-                    let key = item.id;
-                    if let Err(e) = remove_file(key.clone()).await {
-                        match e.kind() {
-                            std::io::ErrorKind::NotFound => {
-                                let hash = Md5Hash(*GenericArray::from_slice(key.as_bytes()));
-                                let path: PathBuf = hash.into();
-                                if let Err(e) = remove_file(&path).await {
-                                    warn!(
-                                        "Failed to delete file `{}` from cache: {}",
-                                        path.to_string_lossy(),
-                                        e
-                                    );
-                                }
-                            }
-                            _ => {
-                                warn!("Failed to delete file `{}` from cache: {}", &key, e);
-                            }
-                        }
-                    }
-                });
+                tokio::spawn(remove_file_handler(item.id));
             }
 
             cache.disk_cur_size.fetch_sub(size_freed, Ordering::Release);
@@ -213,6 +194,43 @@ async fn db_listener(
     }
 }
 
+/// Returns if a file was successfully deleted.
+async fn remove_file_handler(key: String) -> bool {
+    let error = if let Err(e) = remove_file(&key).await {
+        e
+    } else {
+        return true;
+    };
+
+    if error.kind() != std::io::ErrorKind::NotFound {
+        warn!("Failed to delete file `{}` from cache: {}", &key, error);
+        return false;
+    }
+
+    if let Ok(bytes) = hex::decode(&key) {
+        if bytes.len() != 16 {
+            warn!("Failed to delete file `{}`; invalid hash size.", &key);
+            return false;
+        }
+
+        let hash = Md5Hash(*GenericArray::from_slice(&bytes));
+        let path: PathBuf = hash.into();
+        if let Err(e) = remove_file(&path).await {
+            warn!(
+                "Failed to delete file `{}` from cache: {}",
+                path.to_string_lossy(),
+                e
+            );
+            false
+        } else {
+            true
+        }
+    } else {
+        warn!("Failed to delete file `{}`; not a md5hash.", &key);
+        false
+    }
+}
+
 #[instrument(level = "debug", skip(transaction))]
 async fn handle_db_get(entry: &Path, transaction: &mut Transaction<'_, Sqlite>) {
     let key = entry.as_os_str().to_str();
@@ -384,6 +402,61 @@ impl CallbackCache for DiskCache {
     }
 }
 
+#[cfg(test)]
+mod remove_file_handler {
+
+    use std::error::Error;
+
+    use tempfile::tempdir;
+    use tokio::fs::{create_dir_all, remove_dir_all};
+
+    use super::*;
+
+    #[tokio::test]
+    async fn should_not_panic_on_invalid_path() {
+        assert!(!remove_file_handler("/this/is/a/non-existent/path/".to_string()).await);
+    }
+
+    #[tokio::test]
+    async fn should_not_panic_on_invalid_hash() {
+        assert!(!remove_file_handler("68b329da9893e34099c7d8ad5cb9c940".to_string()).await);
+    }
+
+    #[tokio::test]
+    async fn should_not_panic_on_malicious_hashes() {
+        assert!(!remove_file_handler("68b329da9893e34".to_string()).await);
+        assert!(
+            !remove_file_handler("68b329da9893e34099c7d8ad5cb9c940aaaaaaaaaaaaaaaaaa".to_string())
+                .await
+        );
+    }
+
+    #[tokio::test]
+    async fn should_delete_existing_file() -> Result<(), Box<dyn Error>> {
+        let temp_dir = tempdir()?;
+        let mut dir_path = temp_dir.path().to_path_buf();
+        dir_path.push("abc123.png");
+
+        // create a file, it can be empty
+        File::create(&dir_path).await?;
+
+        assert!(remove_file_handler(dir_path.to_string_lossy().into_owned()).await);
+
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn should_delete_existing_hash() -> Result<(), Box<dyn Error>> {
+        create_dir_all("b/8/6").await?;
+        File::create("b/8/6/68b329da9893e34099c7d8ad5cb9c900").await?;
+
+        assert!(remove_file_handler("68b329da9893e34099c7d8ad5cb9c900".to_string()).await);
+
+        remove_dir_all("b").await?;
+        Ok(())
+    }
+}
+
 #[cfg(test)]
 mod disk_cache {
     use std::error::Error;

src/cache/fs.rs

@@ -48,8 +48,8 @@ use super::{CacheKey, CacheStream, ImageMetadata, ENCRYPTION_KEY};
 pub(super) async fn read_file(
     file: File,
 ) -> Option<Result<(CacheStream, Option<XNonce>, ImageMetadata), std::io::Error>> {
-    let mut file_0 = file.try_clone().await.unwrap();
-    let file_1 = file.try_clone().await.unwrap();
+    let mut file_0 = file.try_clone().await.ok()?;
+    let file_1 = file.try_clone().await.ok()?;
 
     // Try reading decrypted header first...
     let mut deserializer = serde_json::Deserializer::from_reader(file.into_std().await);

src/config.rs

@@ -73,11 +73,11 @@ pub fn load_config() -> Result<Config, ConfigError> {
         Ordering::Release,
     );
 
-    config.proxy.clone().map(|socket| {
+    if let Some(socket) = config.proxy.clone() {
         USE_PROXY
             .set(socket)
             .expect("USE_PROXY to be set only by this function");
-    });
+    }
 
     DISABLE_CERT_VALIDATION.store(
         config

src/metrics.rs

@@ -104,20 +104,20 @@ pub async fn load_geo_ip_data(license_key: ClientSecret) -> Result<(), DbLoadErr
     // Check date of db
     let db_date_created = metadata(DB_PATH)
         .ok()
-        .and_then(|metadata| match metadata.created() {
-            Ok(time) => Some(time),
-            Err(_) => {
+        .and_then(|metadata| {
+            if let Ok(time) = metadata.created() {
+                Some(time)
+            } else {
                 debug("fs didn't report birth time, fall back to last modified instead");
                 metadata.modified().ok()
             }
         })
         .unwrap_or(SystemTime::UNIX_EPOCH);
-    let duration = match SystemTime::now().duration_since(db_date_created) {
-        Ok(time) => Duration::from_std(time).expect("duration to fit"),
-        Err(_) => {
-            warn!("Clock may have gone backwards?");
-            Duration::max_value()
-        }
+    let duration = if let Ok(time) = SystemTime::now().duration_since(db_date_created) {
+        Duration::from_std(time).expect("duration to fit")
+    } else {
+        warn!("Clock may have gone backwards?");
+        Duration::max_value()
     };
 
     // DB expired, fetch a new one
@@ -172,14 +172,12 @@ async fn fetch_db(license_key: ClientSecret) -> Result<(), DbLoadError> {
 }
 
 pub fn record_country_visit(country: Option<Country>) {
-    let iso_code = if let Some(country) = country {
+    let iso_code = country.map_or("unknown", |country| {
         country
             .country
             .and_then(|c| c.iso_code)
             .unwrap_or("unknown")
-    } else {
-        "unknown"
-    };
+    });
 
     COUNTRY_VISIT_COUNTER
         .get_metric_with_label_values(&[iso_code])

src/state.rs

@@ -146,9 +146,10 @@ impl ServerState {
     pub fn init_offline() -> Self {
         assert!(OFFLINE_MODE.load(Ordering::Acquire));
         Self {
-            precomputed_key: PrecomputedKey::from_slice(&[41; PRECOMPUTEDKEYBYTES]).unwrap(),
-            image_server: Url::from_file_path("/dev/null").unwrap(),
-            url: Url::from_str("http://localhost").unwrap(),
+            precomputed_key: PrecomputedKey::from_slice(&[41; PRECOMPUTEDKEYBYTES])
+                .expect("expect offline config to work"),
+            image_server: Url::from_file_path("/dev/null").expect("expect offline config to work"),
+            url: Url::from_str("http://localhost").expect("expect offline config to work"),
             url_overridden: false,
         }
     }
@@ -163,8 +164,16 @@ impl ResolvesServerCert for DynamicServerCert {
         // TODO: wait for actix-web to use a new version of rustls so we can
         // remove cloning the certs all the time
         Some(CertifiedKey {
-            cert: TLS_CERTS.get().unwrap().load().as_ref().clone(),
-            key: TLS_SIGNING_KEY.get().unwrap().load_full(),
+            cert: TLS_CERTS
+                .get()
+                .expect("tls cert to exist")
+                .load()
+                .as_ref()
+                .clone(),
+            key: TLS_SIGNING_KEY
+                .get()
+                .expect("tls signing key to exist")
+                .load_full(),
             ocsp: None,
             sct_list: None,
         })