From 87271c85a741086753636837471bcc9f99e04235 Mon Sep 17 00:00:00 2001 From: Edward Shen Date: Thu, 15 Jul 2021 19:03:39 -0400 Subject: [PATCH] Fix deleting legacy names --- src/cache/disk.rs | 116 +++++++++++++++++++++++++++++++++++++--------- src/cache/fs.rs | 4 +- src/state.rs | 19 ++++++-- 3 files changed, 111 insertions(+), 28 deletions(-) diff --git a/src/cache/disk.rs b/src/cache/disk.rs index 56c903f..78428ab 100644 --- a/src/cache/disk.rs +++ b/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,44 @@ async fn db_listener( } } +/// Returns if a file was successfully deleted. +async fn remove_file_handler(key: String) -> bool { + if let Err(e) = remove_file(&key).await { + match e.kind() { + std::io::ErrorKind::NotFound => { + if let Ok(bytes) = hex::decode(&key) { + if bytes.len() == 16 { + 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 `{}`; invalid hash size.", &key); + false + } + } else { + warn!("Failed to delete file `{}`; not a md5hash.", &key); + false + } + } + _ => { + warn!("Failed to delete file `{}` from cache: {}", &key, e); + false + } + } + } else { + true + } +} + #[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 +403,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> { + 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> { + 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; diff --git a/src/cache/fs.rs b/src/cache/fs.rs index e264fae..44b9c5d 100644 --- a/src/cache/fs.rs +++ b/src/cache/fs.rs @@ -48,8 +48,8 @@ use super::{CacheKey, CacheStream, ImageMetadata, ENCRYPTION_KEY}; pub(super) async fn read_file( file: File, ) -> Option, 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); diff --git a/src/state.rs b/src/state.rs index 390f658..ccec3b5 100644 --- a/src/state.rs +++ b/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, })