Properly handle out-of-bounds range requests

src/main.rs

@@ -5,7 +5,10 @@ use actix_web::{
     web, App, HttpResponse, HttpResponseBuilder, HttpServer,
 };
 use awc::Client;
-use futures_util::{stream::once, Stream};
+use futures_util::{
+    stream::{empty, once},
+    Stream,
+};
 use once_cell::sync::{Lazy, OnceCell};
 use std::{
     collections::HashSet,
@@ -437,40 +440,34 @@ where
 
     let (details, bytes) = CancelSafeProcessor::new(thumbnail_path.clone(), process_fut).await?;
 
-    match range {
+    let (builder, stream) = if let Some(range_header) = range {
-        Some(range_header) => {
+        if let Some(range) = range_header.single_bytes_range() {
-            if !range_header.is_bytes() {
+            if let Some(content_range) = range.to_content_range(bytes.len() as u64) {
-                return Err(UploadError::Range.into());
-            }
-
-            if range_header.is_empty() {
-                Err(UploadError::Range.into())
-            } else if range_header.len() == 1 {
-                let range = range_header.ranges().next().unwrap();
-                let content_range = range.to_content_range(bytes.len() as u64);
-                let stream = range.chop_bytes(bytes);
                 let mut builder = HttpResponse::PartialContent();
                 builder.insert_header(content_range);
+                let stream = range.chop_bytes(bytes);
 
-                Ok(srv_response(
+                (builder, Either::left(Either::left(stream)))
-                    builder,
-                    stream,
-                    details.content_type(),
-                    7 * DAYS,
-                    details.system_time(),
-                ))
             } else {
-                Err(UploadError::Range.into())
+                (
+                    HttpResponse::RangeNotSatisfiable(),
+                    Either::left(Either::right(empty())),
+                )
             }
+        } else {
+            return Err(UploadError::Range.into());
         }
-        None => Ok(srv_response(
+    } else {
-            HttpResponse::Ok(),
+        (HttpResponse::Ok(), Either::right(once(ready(Ok(bytes)))))
-            once(ready(Ok(bytes) as Result<_, Error>)),
+    };
-            details.content_type(),
+
-            7 * DAYS,
+    Ok(srv_response(
-            details.system_time(),
+        builder,
-        )),
+        stream,
-    }
+        details.content_type(),
+        7 * DAYS,
+        details.system_time(),
+    ))
 }
 
 /// Fetch file details
@@ -541,39 +538,34 @@ async fn ranged_file_resp<S: Store>(
 where
     Error: From<S::Error>,
 {
-    let (builder, stream) = match range {
+    let (builder, stream) = if let Some(range_header) = range {
         //Range header exists - return as ranged
-        Some(range_header) => {
+        if let Some(range) = range_header.single_bytes_range() {
-            if !range_header.is_bytes() {
+            let len = store.len(&identifier).await?;
-                return Err(UploadError::Range.into());
-            }
-
-            if range_header.is_empty() {
-                return Err(UploadError::Range.into());
-            } else if range_header.len() == 1 {
-                let len = store.len(&identifier).await?;
-
-                let range = range_header.ranges().next().unwrap();
 
+            if let Some(content_range) = range.to_content_range(len) {
                 let mut builder = HttpResponse::PartialContent();
-                builder.insert_header(range.to_content_range(len));
+                builder.insert_header(content_range);
 
                 (
                     builder,
-                    Either::left(map_error::map_crate_error(
+                    Either::left(Either::left(map_error::map_crate_error(
                         range.chop_store(store, identifier).await?,
-                    )),
+                    ))),
                 )
             } else {
-                return Err(UploadError::Range.into());
+                (
+                    HttpResponse::RangeNotSatisfiable(),
+                    Either::left(Either::right(empty())),
+                )
             }
+        } else {
+            return Err(UploadError::Range.into());
         }
+    } else {
         //No Range header in the request - return the entire document
-        None => {
+        let stream = map_error::map_crate_error(store.to_stream(&identifier, None, None).await?);
-            let stream =
+        (HttpResponse::Ok(), Either::right(stream))
-                map_error::map_crate_error(store.to_stream(&identifier, None, None).await?);
-            (HttpResponse::Ok(), Either::right(stream))
-        }
     };
 
     Ok(srv_response(

src/range.rs

@@ -28,20 +28,38 @@ pub(crate) struct RangeHeader {
 }
 
 impl Range {
-    pub(crate) fn to_content_range(&self, instance_length: u64) -> ContentRange {
+    pub(crate) fn to_content_range(&self, instance_length: u64) -> Option<ContentRange> {
         match self {
-            Range::Start(start) => ContentRange(ContentRangeSpec::Bytes {
+            Range::Start(start) => {
-                range: Some((*start, instance_length)),
+                if *start >= instance_length {
-                instance_length: Some(instance_length),
+                    return None;
-            }),
+                }
-            Range::SuffixLength(from_start) => ContentRange(ContentRangeSpec::Bytes {
+
-                range: Some((0, *from_start)),
+                Some(ContentRange(ContentRangeSpec::Bytes {
-                instance_length: Some(instance_length),
+                    range: Some((*start, instance_length - *start)),
-            }),
+                    instance_length: Some(instance_length),
-            Range::Segment(start, end) => ContentRange(ContentRangeSpec::Bytes {
+                }))
-                range: Some((*start, *end)),
+            }
-                instance_length: Some(instance_length),
+            Range::SuffixLength(from_start) => {
-            }),
+                if *from_start > instance_length {
+                    return None;
+                }
+
+                Some(ContentRange(ContentRangeSpec::Bytes {
+                    range: Some((0, *from_start)),
+                    instance_length: Some(instance_length),
+                }))
+            }
+            Range::Segment(start, end) => {
+                if *start >= instance_length || *end > instance_length {
+                    return None;
+                }
+
+                Some(ContentRange(ContentRangeSpec::Bytes {
+                    range: Some((*start, *end)),
+                    instance_length: Some(instance_length),
+                }))
+            }
         }
     }
 
@@ -76,20 +94,12 @@ impl Range {
 }
 
 impl RangeHeader {
-    pub(crate) fn is_bytes(&self) -> bool {
+    pub(crate) fn single_bytes_range(&self) -> Option<&'_ Range> {
-        self.unit == "bytes"
+        if self.ranges.len() == 1 && self.unit == "bytes" {
-    }
+            self.ranges.iter().next()
-
+        } else {
-    pub(crate) fn ranges(&self) -> impl Iterator<Item = &'_ Range> + '_ {
+            None
-        self.ranges.iter()
+        }
-    }
-
-    pub(crate) fn len(&self) -> usize {
-        self.ranges.len()
-    }
-
-    pub(crate) fn is_empty(&self) -> bool {
-        self.ranges.is_empty()
     }
 }