refactor for ruma cow headers; update for ContentDisposition type

Signed-off-by: Jason Volk <jason@zemos.net>

src/api/client/media.rs

@@ -6,11 +6,7 @@ use axum::extract::State;
 use axum_client_ip::InsecureClientIp;
 use conduit::{
 	debug_info, debug_warn, err, info,
-	utils::{
+	utils::{self, content_disposition::make_content_disposition, math::ruma_from_usize},
-		self,
-		content_disposition::{content_disposition_type, make_content_disposition, sanitise_filename},
-		math::ruma_from_usize,
-	},
 	warn, Err, Error, Result,
 };
 use ruma::api::client::media::{
@@ -118,21 +114,14 @@ pub(crate) async fn create_content_route(
 
 	let mxc = format!("mxc://{}/{}", services.globals.server_name(), utils::random_string(MXC_LENGTH));
 
+	let content_disposition = make_content_disposition(None, body.content_type.as_deref(), body.filename.as_deref());
+
 	services
 		.media
 		.create(
 			Some(sender_user.clone()),
 			&mxc,
-			body.filename
+			Some(&content_disposition),
-				.as_ref()
-				.map(|filename| {
-					format!(
-						"{}; filename={}",
-						content_disposition_type(&body.content_type),
-						sanitise_filename(filename.to_owned())
-					)
-				})
-				.as_deref(),
 			body.content_type.as_deref(),
 			&body.file,
 		)
@@ -185,14 +174,14 @@ pub(crate) async fn get_content_route(
 		content_disposition,
 	}) = services.media.get(&mxc).await?
 	{
-		let content_disposition = Some(make_content_disposition(&content_type, content_disposition, None));
+		let content_disposition = make_content_disposition(content_disposition.as_ref(), content_type.as_deref(), None);
-		let file = content.expect("content");
 
+		let file = content.expect("content");
 		Ok(get_content::v3::Response {
 			file,
-			content_type,
+			content_type: content_type.map(Into::into),
-			content_disposition,
+			content_disposition: Some(content_disposition),
-			cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.to_owned()),
+			cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.into()),
 			cache_control: Some(CACHE_CONTROL_IMMUTABLE.into()),
 		})
 	} else if !services.globals.server_is_ours(&body.server_name) && body.allow_remote {
@@ -207,18 +196,15 @@ pub(crate) async fn get_content_route(
 		.await
 		.map_err(|e| err!(Request(NotFound(debug_warn!("Fetching media `{mxc}` failed: {e:?}")))))?;
 
-		let content_disposition = Some(make_content_disposition(
+		let content_disposition =
-			&response.content_type,
+			make_content_disposition(response.content_disposition.as_ref(), response.content_type.as_deref(), None);
-			response.content_disposition,
-			None,
-		));
 
 		Ok(get_content::v3::Response {
 			file: response.file,
 			content_type: response.content_type,
-			content_disposition,
+			content_disposition: Some(content_disposition),
-			cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.to_owned()),
+			cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.into()),
-			cache_control: Some(CACHE_CONTROL_IMMUTABLE.to_owned()),
+			cache_control: Some(CACHE_CONTROL_IMMUTABLE.into()),
 		})
 	} else {
 		Err!(Request(NotFound("Media not found.")))
@@ -268,18 +254,15 @@ pub(crate) async fn get_content_as_filename_route(
 		content_disposition,
 	}) = services.media.get(&mxc).await?
 	{
-		let content_disposition = Some(make_content_disposition(
+		let content_disposition =
-			&content_type,
+			make_content_disposition(content_disposition.as_ref(), content_type.as_deref(), Some(&body.filename));
-			content_disposition,
-			Some(body.filename.clone()),
-		));
 
 		let file = content.expect("content");
 		Ok(get_content_as_filename::v3::Response {
 			file,
-			content_type,
+			content_type: content_type.map(Into::into),
-			content_disposition,
+			content_disposition: Some(content_disposition),
-			cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.to_owned()),
+			cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.into()),
 			cache_control: Some(CACHE_CONTROL_IMMUTABLE.into()),
 		})
 	} else if !services.globals.server_is_ours(&body.server_name) && body.allow_remote {
@@ -294,17 +277,17 @@ pub(crate) async fn get_content_as_filename_route(
 		.await
 		{
 			Ok(remote_content_response) => {
-				let content_disposition = Some(make_content_disposition(
+				let content_disposition = make_content_disposition(
-					&remote_content_response.content_type,
+					remote_content_response.content_disposition.as_ref(),
-					remote_content_response.content_disposition,
+					remote_content_response.content_type.as_deref(),
 					None,
-				));
+				);
 
 				Ok(get_content_as_filename::v3::Response {
-					content_disposition,
+					content_disposition: Some(content_disposition),
 					content_type: remote_content_response.content_type,
 					file: remote_content_response.file,
-					cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.to_owned()),
+					cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.into()),
 					cache_control: Some(CACHE_CONTROL_IMMUTABLE.into()),
 				})
 			},
@@ -369,15 +352,15 @@ pub(crate) async fn get_content_thumbnail_route(
 		)
 		.await?
 	{
-		let content_disposition = Some(make_content_disposition(&content_type, content_disposition, None));
+		let content_disposition = make_content_disposition(content_disposition.as_ref(), content_type.as_deref(), None);
 		let file = content.expect("content");
 
 		Ok(get_content_thumbnail::v3::Response {
 			file,
-			content_type,
+			content_type: content_type.map(Into::into),
-			cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.to_owned()),
+			cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.into()),
 			cache_control: Some(CACHE_CONTROL_IMMUTABLE.into()),
-			content_disposition,
+			content_disposition: Some(content_disposition),
 		})
 	} else if !services.globals.server_is_ours(&body.server_name) && body.allow_remote {
 		if services
@@ -423,18 +406,18 @@ pub(crate) async fn get_content_thumbnail_route(
 					)
 					.await?;
 
-				let content_disposition = Some(make_content_disposition(
+				let content_disposition = make_content_disposition(
-					&get_thumbnail_response.content_type,
+					get_thumbnail_response.content_disposition.as_ref(),
-					get_thumbnail_response.content_disposition,
+					get_thumbnail_response.content_type.as_deref(),
 					None,
-				));
+				);
 
 				Ok(get_content_thumbnail::v3::Response {
 					file: get_thumbnail_response.file,
 					content_type: get_thumbnail_response.content_type,
-					cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.to_owned()),
+					cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.into()),
-					cache_control: Some(CACHE_CONTROL_IMMUTABLE.to_owned()),
+					cache_control: Some(CACHE_CONTROL_IMMUTABLE.into()),
-					content_disposition,
+					content_disposition: Some(content_disposition),
 				})
 			},
 			Err(e) => Err!(Request(NotFound(debug_warn!("Fetching media `{mxc}` failed: {e:?}")))),
@@ -495,18 +478,18 @@ async fn get_remote_content(
 		)
 		.await?;
 
-	let content_disposition = Some(make_content_disposition(
+	let content_disposition = make_content_disposition(
-		&content_response.content_type,
+		content_response.content_disposition.as_ref(),
-		content_response.content_disposition,
+		content_response.content_type.as_deref(),
 		None,
-	));
+	);
 
 	services
 		.media
 		.create(
 			None,
 			mxc,
-			content_disposition.as_deref(),
+			Some(&content_disposition),
 			content_response.content_type.as_deref(),
 			&content_response.file,
 		)
@@ -515,8 +498,8 @@ async fn get_remote_content(
 	Ok(get_content::v3::Response {
 		file: content_response.file,
 		content_type: content_response.content_type,
-		content_disposition,
+		content_disposition: Some(content_disposition),
-		cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.to_owned()),
+		cross_origin_resource_policy: Some(CORP_CROSS_ORIGIN.into()),
-		cache_control: Some(CACHE_CONTROL_IMMUTABLE.to_owned()),
+		cache_control: Some(CACHE_CONTROL_IMMUTABLE.into()),
 	})
 }

src/core/error/mod.rs

@@ -87,6 +87,8 @@ pub enum Error {
 	Config(&'static str, Cow<'static, str>),
 	#[error("{0}")]
 	Conflict(&'static str), // This is only needed for when a room alias already exists
+	#[error(transparent)]
+	ContentDisposition(#[from] ruma::http_headers::ContentDispositionParseError),
 	#[error("{0}")]
 	Database(Cow<'static, str>),
 	#[error("Remote server {0} responded with: {1}")]

src/core/utils/content_disposition.rs

@@ -1,7 +1,8 @@
-use crate::debug_info;
+use std::borrow::Cow;
 
-const ATTACHMENT: &str = "attachment";
+use ruma::http_headers::{ContentDisposition, ContentDispositionType};
-const INLINE: &str = "inline";
+
+use crate::debug_info;
 
 /// as defined by MSC2702
 const ALLOWED_INLINE_CONTENT_TYPES: [&str; 26] = [
@@ -38,42 +39,44 @@ const ALLOWED_INLINE_CONTENT_TYPES: [&str; 26] = [
 /// Content-Type against MSC2702 list of safe inline Content-Types
 /// (`ALLOWED_INLINE_CONTENT_TYPES`)
 #[must_use]
-pub fn content_disposition_type(content_type: &Option<String>) -> &'static str {
+pub fn content_disposition_type(content_type: Option<&str>) -> ContentDispositionType {
 	let Some(content_type) = content_type else {
 		debug_info!("No Content-Type was given, assuming attachment for Content-Disposition");
-		return ATTACHMENT;
+		return ContentDispositionType::Attachment;
 	};
 
 	// is_sorted is unstable
 	/* debug_assert!(ALLOWED_INLINE_CONTENT_TYPES.is_sorted(),
 	 * "ALLOWED_INLINE_CONTENT_TYPES is not sorted"); */
 
-	let content_type = content_type
+	let content_type: Cow<'_, str> = content_type
 		.split(';')
 		.next()
 		.unwrap_or(content_type)
-		.to_ascii_lowercase();
+		.to_ascii_lowercase()
+		.into();
 
 	if ALLOWED_INLINE_CONTENT_TYPES
-		.binary_search(&content_type.as_str())
+		.binary_search(&content_type.as_ref())
 		.is_ok()
 	{
-		INLINE
+		ContentDispositionType::Inline
 	} else {
-		ATTACHMENT
+		ContentDispositionType::Attachment
 	}
 }
 
 /// sanitises the file name for the Content-Disposition using
 /// `sanitize_filename` crate
 #[tracing::instrument(level = "debug")]
-pub fn sanitise_filename(filename: String) -> String {
+pub fn sanitise_filename(filename: &str) -> String {
-	let options = sanitize_filename::Options {
+	sanitize_filename::sanitize_with_options(
+		filename,
+		sanitize_filename::Options {
 			truncate: false,
 			..Default::default()
-	};
+		},
-
+	)
-	sanitize_filename::sanitize_with_options(filename, options)
 }
 
 /// creates the final Content-Disposition based on whether the filename exists
@@ -85,33 +88,13 @@ pub fn sanitise_filename(filename: String) -> String {
 ///
 /// else: `Content-Disposition: attachment/inline`
 pub fn make_content_disposition(
-	content_type: &Option<String>, content_disposition: Option<String>, req_filename: Option<String>,
+	content_disposition: Option<&ContentDisposition>, content_type: Option<&str>, filename: Option<&str>,
-) -> String {
+) -> ContentDisposition {
-	let filename: String;
+	ContentDisposition::new(content_disposition_type(content_type)).with_filename(
-
+		filename
-	if let Some(req_filename) = req_filename {
+			.or_else(|| content_disposition.and_then(|content_disposition| content_disposition.filename.as_deref()))
-		filename = sanitise_filename(req_filename);
+			.map(sanitise_filename),
-	} else {
+	)
-		filename = content_disposition.map_or_else(String::new, |content_disposition| {
-			let (_, filename) = content_disposition
-				.split_once("filename=")
-				.unwrap_or(("", ""));
-
-			if filename.is_empty() {
-				String::new()
-			} else {
-				sanitise_filename(filename.to_owned())
-			}
-		});
-	};
-
-	if !filename.is_empty() {
-		// Content-Disposition: attachment/inline; filename=filename.ext
-		format!("{}; filename={}", content_disposition_type(content_type), filename)
-	} else {
-		// Content-Disposition: attachment/inline
-		String::from(content_disposition_type(content_type))
-	}
 }
 
 #[cfg(test)]
@@ -136,4 +119,20 @@ mod tests {
 
 		assert_eq!(SANITISED, sanitize_filename::sanitize_with_options(SAMPLE, options.clone()));
 	}
+
+	#[test]
+	fn empty_sanitisation() {
+		use crate::utils::string::EMPTY;
+
+		let result = sanitize_filename::sanitize_with_options(
+			EMPTY,
+			sanitize_filename::Options {
+				windows: true,
+				truncate: true,
+				replacement: "",
+			},
+		);
+
+		assert_eq!(EMPTY, result);
+	}
 }

src/service/media/data.rs

@@ -2,7 +2,7 @@ use std::sync::Arc;
 
 use conduit::{debug, debug_info, utils::string_from_bytes, Error, Result};
 use database::{Database, Map};
-use ruma::api::client::error::ErrorKind;
+use ruma::{api::client::error::ErrorKind, http_headers::ContentDisposition};
 
 use super::preview::UrlPreviewData;
 
@@ -14,7 +14,7 @@ pub(crate) struct Data {
 
 #[derive(Debug)]
 pub(super) struct Metadata {
-	pub(super) content_disposition: Option<String>,
+	pub(super) content_disposition: Option<ContentDisposition>,
 	pub(super) content_type: Option<String>,
 	pub(super) key: Vec<u8>,
 }
@@ -29,8 +29,8 @@ impl Data {
 	}
 
 	pub(super) fn create_file_metadata(
-		&self, sender_user: Option<&str>, mxc: &str, width: u32, height: u32, content_disposition: Option<&str>,
+		&self, sender_user: Option<&str>, mxc: &str, width: u32, height: u32,
-		content_type: Option<&str>,
+		content_disposition: Option<&ContentDisposition>, content_type: Option<&str>,
 	) -> Result<Vec<u8>> {
 		let mut key = mxc.as_bytes().to_vec();
 		key.push(0xFF);
@@ -39,9 +39,9 @@ impl Data {
 		key.push(0xFF);
 		key.extend_from_slice(
 			content_disposition
-				.as_ref()
+				.map(ToString::to_string)
-				.map(|f| f.as_bytes())
+				.unwrap_or_default()
-				.unwrap_or_default(),
+				.as_bytes(),
 		);
 		key.push(0xFF);
 		key.extend_from_slice(
@@ -143,7 +143,8 @@ impl Data {
 		} else {
 			Some(
 				string_from_bytes(content_disposition_bytes)
-					.map_err(|_| Error::bad_database("Content Disposition in mediaid_file is invalid unicode."))?,
+					.map_err(|_| Error::bad_database("Content Disposition in mediaid_file is invalid unicode."))?
+					.parse()?,
 			)
 		};
 

src/service/media/mod.rs

@@ -9,7 +9,7 @@ use async_trait::async_trait;
 use base64::{engine::general_purpose, Engine as _};
 use conduit::{debug, debug_error, err, error, trace, utils, utils::MutexMap, Err, Result, Server};
 use data::{Data, Metadata};
-use ruma::{OwnedMxcUri, OwnedUserId};
+use ruma::{http_headers::ContentDisposition, OwnedMxcUri, OwnedUserId};
 use tokio::{
 	fs,
 	io::{AsyncReadExt, AsyncWriteExt, BufReader},
@@ -21,7 +21,7 @@ use crate::{client, globals, Dep};
 pub struct FileMeta {
 	pub content: Option<Vec<u8>>,
 	pub content_type: Option<String>,
-	pub content_disposition: Option<String>,
+	pub content_disposition: Option<ContentDisposition>,
 }
 
 pub struct Service {
@@ -65,7 +65,7 @@ impl crate::Service for Service {
 impl Service {
 	/// Uploads a file.
 	pub async fn create(
-		&self, sender_user: Option<OwnedUserId>, mxc: &str, content_disposition: Option<&str>,
+		&self, sender_user: Option<OwnedUserId>, mxc: &str, content_disposition: Option<&ContentDisposition>,
 		content_type: Option<&str>, file: &[u8],
 	) -> Result<()> {
 		// Width, Height = 0 if it's not a thumbnail

src/service/media/thumbnail.rs

@@ -2,7 +2,7 @@ use std::{cmp, io::Cursor, num::Saturating as Sat};
 
 use conduit::{checked, Result};
 use image::{imageops::FilterType, DynamicImage};
-use ruma::OwnedUserId;
+use ruma::{http_headers::ContentDisposition, OwnedUserId};
 use tokio::{
 	fs,
 	io::{AsyncReadExt, AsyncWriteExt},
@@ -14,7 +14,7 @@ impl super::Service {
 	/// Uploads or replaces a file thumbnail.
 	#[allow(clippy::too_many_arguments)]
 	pub async fn upload_thumbnail(
-		&self, sender_user: Option<OwnedUserId>, mxc: &str, content_disposition: Option<&str>,
+		&self, sender_user: Option<OwnedUserId>, mxc: &str, content_disposition: Option<&ContentDisposition>,
 		content_type: Option<&str>, width: u32, height: u32, file: &[u8],
 	) -> Result<()> {
 		let key = if let Some(user) = sender_user {
@@ -104,7 +104,7 @@ impl super::Service {
 			mxc,
 			width,
 			height,
-			data.content_disposition.as_deref(),
+			data.content_disposition.as_ref(),
 			data.content_type.as_deref(),
 		)?;