Add application-level dimension limiting, bail on failed processes

src/config.rs

@@ -51,6 +51,22 @@ pub(crate) struct Config {
     )]
     max_file_size: usize,
 
+    #[structopt(
+        long,
+        env = "PICTRS_MAX_IMAGE_WIDTH",
+        help = "Specify the maximum width in pixels allowed on an image",
+        default_value = "10000"
+    )]
+    max_image_width: usize,
+
+    #[structopt(
+        long,
+        env = "PICTRS_MAX_IMAGE_HEIGHT",
+        help = "Specify the maximum width in pixels allowed on an image",
+        default_value = "10000"
+    )]
+    max_image_height: usize,
+
     #[structopt(
         long,
         env = "PICTRS_API_KEY",
@@ -86,6 +102,14 @@ impl Config {
         self.max_file_size
     }
 
+    pub(crate) fn max_width(&self) -> usize {
+        self.max_image_width
+    }
+
+    pub(crate) fn max_height(&self) -> usize {
+        self.max_image_height
+    }
+
     pub(crate) fn api_key(&self) -> Option<&str> {
         self.api_key.as_deref()
     }

src/magick.rs

@@ -1,8 +1,7 @@
 use crate::{config::Format, stream::Process};
 use actix_web::web::Bytes;
-use std::process::Stdio;
 use tokio::{
-    io::{AsyncRead, AsyncReadExt, AsyncWriteExt},
+    io::{AsyncRead, AsyncReadExt},
     process::Command,
 };
 
@@ -13,6 +12,9 @@ pub(crate) enum MagickError {
 
     #[error("Invalid format")]
     Format,
+
+    #[error("Image too large")]
+    Dimensions,
 }
 
 pub(crate) enum ValidInputType {
@@ -124,46 +126,8 @@ fn parse_details(s: std::borrow::Cow<'_, str>) -> Result<Details, MagickError> {
     })
 }
 
-pub(crate) async fn input_type_bytes(mut input: Bytes) -> Result<ValidInputType, MagickError> {
+pub(crate) async fn input_type_bytes(input: Bytes) -> Result<ValidInputType, MagickError> {
-    let mut child = Command::new("magick")
+    details_bytes(input).await?.validate_input()
-        .args(["identify", "-ping", "-format", "%m\n", "-"])
-        .stdin(Stdio::piped())
-        .stdout(Stdio::piped())
-        .spawn()?;
-
-    let mut stdin = child.stdin.take().unwrap();
-    let mut stdout = child.stdout.take().unwrap();
-
-    stdin.write_all_buf(&mut input).await?;
-    drop(stdin);
-
-    let mut vec = Vec::new();
-    stdout.read_to_end(&mut vec).await?;
-    drop(stdout);
-
-    child.wait().await?;
-
-    let s = String::from_utf8_lossy(&vec);
-    parse_input_type(s)
-}
-
-fn parse_input_type(s: std::borrow::Cow<'_, str>) -> Result<ValidInputType, MagickError> {
-    let mut lines = s.lines();
-    let first = lines.next();
-
-    let opt = lines.fold(first, |acc, item| match acc {
-        Some(prev) if prev == item => Some(prev),
-        _ => None,
-    });
-
-    match opt {
-        Some("MP4") => Ok(ValidInputType::Mp4),
-        Some("GIF") => Ok(ValidInputType::Gif),
-        Some("PNG") => Ok(ValidInputType::Png),
-        Some("JPEG") => Ok(ValidInputType::Jpeg),
-        Some("WEBP") => Ok(ValidInputType::Webp),
-        _ => Err(MagickError::Format),
-    }
 }
 
 pub(crate) fn process_image_write_read(
@@ -181,6 +145,25 @@ pub(crate) fn process_image_write_read(
     Ok(process.write_read(input).unwrap())
 }
 
+impl Details {
+    fn validate_input(&self) -> Result<ValidInputType, MagickError> {
+        if self.width > crate::CONFIG.max_width() || self.height > crate::CONFIG.max_height() {
+            return Err(MagickError::Dimensions);
+        }
+
+        let input_type = match (self.mime_type.type_(), self.mime_type.subtype()) {
+            (mime::VIDEO, mime::MP4 | mime::MPEG) => ValidInputType::Mp4,
+            (mime::IMAGE, mime::GIF) => ValidInputType::Gif,
+            (mime::IMAGE, mime::PNG) => ValidInputType::Png,
+            (mime::IMAGE, mime::JPEG) => ValidInputType::Jpeg,
+            (mime::IMAGE, subtype) if subtype.as_str() == "webp" => ValidInputType::Webp,
+            _ => return Err(MagickError::Format),
+        };
+
+        Ok(input_type)
+    }
+}
+
 impl From<std::num::ParseIntError> for MagickError {
     fn from(_: std::num::ParseIntError) -> MagickError {
         MagickError::Format

src/stream.rs

@@ -8,6 +8,9 @@ use std::{
 };
 use tokio::io::{AsyncRead, AsyncWriteExt, ReadBuf};
 
+#[derive(Debug)]
+struct StatusError;
+
 pub(crate) struct Process {
     child: tokio::process::Child,
 }
@@ -38,9 +41,25 @@ impl Process {
 
         let (tx, rx) = tokio::sync::oneshot::channel();
 
+        let mut child = self.child;
+
         actix_rt::spawn(async move {
             if let Err(e) = stdin.write_all_buf(&mut input).await {
                 let _ = tx.send(e);
+                return;
+            }
+            drop(stdin);
+
+            match child.wait().await {
+                Ok(status) => {
+                    if !status.success() {
+                        let _ =
+                            tx.send(std::io::Error::new(std::io::ErrorKind::Other, &StatusError));
+                    }
+                }
+                Err(e) => {
+                    let _ = tx.send(e);
+                }
             }
         });
 
@@ -60,9 +79,25 @@ impl Process {
 
         let (tx, rx) = tokio::sync::oneshot::channel();
 
+        let mut child = self.child;
+
         actix_rt::spawn(async move {
             if let Err(e) = tokio::io::copy(&mut input_reader, &mut stdin).await {
                 let _ = tx.send(e);
+                return;
+            }
+            drop(stdin);
+
+            match child.wait().await {
+                Ok(status) => {
+                    if !status.success() {
+                        let _ =
+                            tx.send(std::io::Error::new(std::io::ErrorKind::Other, &StatusError));
+                    }
+                }
+                Err(e) => {
+                    let _ = tx.send(e);
+                }
             }
         });
 
@@ -122,3 +157,11 @@ where
             .map_err(UploadError::from)
     }
 }
+
+impl std::fmt::Display for StatusError {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        write!(f, "Command failed with bad status")
+    }
+}
+
+impl std::error::Error for StatusError {}