From 56e1022da6f260eb5a91e9de090b58d9dbd58cf2 Mon Sep 17 00:00:00 2001 From: Vika Date: Wed, 2 Mar 2022 23:39:44 +0300 Subject: database: code cleanup --- src/database/file/mod.rs | 77 ++++++++++++++++++++++++++++++++---------------- src/database/mod.rs | 7 ++++- 2 files changed, 58 insertions(+), 26 deletions(-) (limited to 'src') diff --git a/src/database/file/mod.rs b/src/database/file/mod.rs index d1227d8..a33e7c4 100644 --- a/src/database/file/mod.rs +++ b/src/database/file/mod.rs @@ -1,3 +1,4 @@ +#![warn(clippy::unwrap_used)] use crate::database::{filter_post, ErrorKind, Result, Storage, StorageError}; use std::fs::{File, OpenOptions}; use std::io::{ErrorKind as IOErrorKind, Seek, SeekFrom, Read, Write}; @@ -81,6 +82,7 @@ fn path_relative_from(path: &Path, base: &Path) -> Option { } } +#[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { #[test] fn test_relative_path_resolving() { @@ -92,8 +94,17 @@ mod tests { } } +// TODO: Check that the path ACTUALLY IS INSIDE THE ROOT FOLDER +// This could be checked by completely resolving the path +// and checking if it has a common prefix fn url_to_path(root: &Path, url: &str) -> PathBuf { - url_to_relative_path(url).to_path(root) + let path = url_to_relative_path(url).to_logical_path(root); + if !path.starts_with(root) { + // TODO: handle more gracefully + panic!("Security error: {:?} is not a prefix of {:?}", path, root) + } else { + path + } } fn url_to_relative_path(url: &str) -> relative_path::RelativePathBuf { @@ -105,7 +116,7 @@ fn url_to_relative_path(url: &str) -> relative_path::RelativePathBuf { } fn modify_post(post: &serde_json::Value, update: &serde_json::Value) -> Result { - let mut add_keys: HashMap = HashMap::new(); + let mut add_keys: HashMap> = HashMap::new(); let mut remove_keys: Vec = vec![]; let mut remove_values: HashMap> = HashMap::new(); let mut post = post.clone(); @@ -134,7 +145,7 @@ fn modify_post(post: &serde_json::Value, update: &serde_json::Value) -> Result Result Result( user: &'_ Option, storage: &S, ) { - let url = feed["properties"]["uid"][0].as_str().unwrap(); - if let Some(author) = feed["properties"]["author"].clone().as_array() { + let url = feed["properties"]["uid"][0] + .as_str() + .expect("MF2 value should have a UID set! Check if you used normalize_mf2 before recording the post!"); + if let Some(author) = feed["properties"]["author"].as_array().cloned() { if !feed["type"] .as_array() - .unwrap() + .expect("MF2 value should have a type set!") .iter() .any(|i| i == "h-card") { @@ -246,7 +262,11 @@ async fn hydrate_author( }) .collect::>() .await; - feed["properties"].as_object_mut().unwrap()["author"] = json!(author_list); + if let Some(props) = feed["properties"].as_object_mut() { + props["author"] = json!(author_list); + } else { + feed["properties"] = json!({"author": author_list}); + } } } } @@ -258,14 +278,20 @@ impl Storage for FileStorage { async fn post_exists(&self, url: &str) -> Result { let path = url_to_path(&self.root_dir, url); debug!("Checking if {:?} exists...", path); + #[allow(clippy::unwrap_used)] // JoinHandle captures panics, this closure shouldn't panic Ok(spawn_blocking(move || path.is_file()).await.unwrap()) } async fn get_post(&self, url: &str) -> Result> { let path = url_to_path(&self.root_dir, url); + // TODO: check that the path actually belongs to the dir of user who requested it + // it's not like you CAN access someone else's private posts with it + // so it's not exactly a security issue, but it's still not good debug!("Opening {:?}", path); // Use exclusively synchronous operations to never transfer a lock over an await boundary + #[allow(clippy::unwrap_used)] // JoinHandle captures panics, this closure shouldn't panic tokio::time::timeout(Duration::from_secs(IO_TIMEOUT), spawn_blocking(move || { + #[warn(clippy::unwrap_used)] match File::open(&path) { Ok(file) => { let lock = RwLock::new(file); @@ -303,7 +329,9 @@ impl Storage for FileStorage { let post_json = post.to_string(); let post_path = path.clone(); // Use exclusively synchronous operations to never transfer a lock over an await boundary + #[allow(clippy::unwrap_used)] // JoinHandle captures panics, this closure shouldn't panic tokio::time::timeout(Duration::from_secs(IO_TIMEOUT), spawn_blocking(move || { + #[warn(clippy::unwrap_used)] let parent = post_path.parent().unwrap().to_owned(); if !parent.is_dir() { std::fs::create_dir_all(post_path.parent().unwrap())?; @@ -325,10 +353,8 @@ impl Storage for FileStorage { Result::Ok(()) })).await?.unwrap()?; - if post["properties"]["url"].is_array() { - for url in post["properties"]["url"] - .as_array() - .unwrap() + if let Some(urls) = post["properties"]["url"].as_array() { + for url in urls .iter() .map(|i| i.as_str().unwrap()) { @@ -559,12 +585,13 @@ impl Storage for FileStorage { async fn get_setting(&self, setting: &'_ str, user: &'_ str) -> Result { log::debug!("User for getting settings: {}", user); - let url = http_types::Url::parse(user).expect("Couldn't parse a URL"); + let url = warp::http::Uri::try_from(user).expect("Couldn't parse a URL"); let mut path = relative_path::RelativePathBuf::new(); - path.push(url.origin().ascii_serialization()); + path.push(url.authority().unwrap().to_string()); path.push("settings"); let path = path.to_path(&self.root_dir); + log::debug!("Getting settings from {:?}", &path); let setting = setting.to_string(); tokio::time::timeout(Duration::from_secs(IO_TIMEOUT), spawn_blocking(move || { let lock = RwLock::new(File::open(path)?); @@ -584,9 +611,9 @@ impl Storage for FileStorage { } async fn set_setting(&self, setting: &'_ str, user: &'_ str, value: &'_ str) -> Result<()> { - let url = http_types::Url::parse(user).expect("Couldn't parse a URL"); + let url = warp::http::Uri::try_from(user).expect("Couldn't parse a URL"); let mut path = relative_path::RelativePathBuf::new(); - path.push(url.origin().ascii_serialization()); + path.push(url.authority().unwrap().to_string()); path.push("settings"); let path = path.to_path(&self.root_dir); @@ -618,7 +645,7 @@ impl Storage for FileStorage { serde_json::from_str(&content)? }; - settings.insert(setting.to_string(), value.to_string()); + settings.insert(setting, value); (&mut *guard).seek(SeekFrom::Start(0))?; (*guard).set_len(0)?; (&mut *guard).write_all(serde_json::to_string(&settings)?.as_bytes())?; diff --git a/src/database/mod.rs b/src/database/mod.rs index 836d6c3..e7baaa8 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -4,8 +4,11 @@ use serde::{Deserialize, Serialize}; mod file; pub use crate::database::file::FileStorage; +#[cfg(test)] mod memory; -pub(crate) use crate::database::memory::MemoryStorage; +#[cfg(test)] +pub use crate::database::memory::MemoryStorage; + /// Data structure representing a Micropub channel in the ?q=channels output. #[derive(Serialize, Deserialize, PartialEq, Debug)] @@ -44,6 +47,8 @@ pub struct StorageError { kind: ErrorKind, } +impl warp::reject::Reject for StorageError {} + impl std::error::Error for StorageError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { self.source -- cgit 1.4.1