Serve gzipped version of console asset if it exists (#1345)

* serve gzipped version of asset if it exists * fix clippy, only send gzip if accept-encoding header says to * don't bother finding gzipped file if we're not going to serve it anyway * code golf, thanks @jgallagher * debatably simplify by passing around PathBuf instead of Vec<String>
oxidecomputer · Jul 5, 2022 · 600c2c9 · 600c2c9
1 parent 2b1c005
commit 600c2c9
Show file tree

Hide file tree

Showing 6 changed files with 148 additions and 70 deletions.
diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs
@@ -619,8 +619,21 @@ pub async fn console_settings_page(
     console_index_or_login_redirect(rqctx).await
 }
 
+/// Make a new PathBuf with `.gz` on the end
+fn with_gz_ext(path: &PathBuf) -> PathBuf {
+    let mut new_path = path.clone();
+    let new_ext = match path.extension().map(|ext| ext.to_str()) {
+        Some(Some(curr_ext)) => format!("{curr_ext}.gz"),
+        _ => "gz".to_string(),
+    };
+    new_path.set_extension(new_ext);
+    new_path
+}
+
 /// Fetch a static asset from `<static_dir>/assets`. 404 on virtually all
-/// errors. No auth. NO SENSITIVE FILES.
+/// errors. No auth. NO SENSITIVE FILES. Will serve a gzipped version if the
+/// `.gz` file is present in the directory and `Accept-Encoding: gzip` is
+/// present on the request.
 #[endpoint {
    method = GET,
    path = "/assets/{path:.*}",
@@ -631,34 +644,64 @@ pub async fn asset(
     path_params: Path<RestPathParam>,
 ) -> Result<Response<Body>, HttpError> {
     let apictx = rqctx.context();
-    let path = path_params.into_inner().path;
+    let path = PathBuf::from_iter(path_params.into_inner().path);
 
-    let file = match &apictx.console_config.static_dir {
-        // important: we only serve assets from assets/ within static_dir
-        Some(static_dir) => find_file(path, &static_dir.join("assets")),
-        _ => Err(not_found("static_dir undefined")),
-    }?;
-    let file_contents = tokio::fs::read(&file)
-        .await
-        .map_err(|e| not_found(&format!("accessing {:?}: {:#}", file, e)))?;
+    // Bail unless the extension is allowed
+    let ext = path
+        .extension()
+        .map_or_else(|| OsString::from("disallowed"), |ext| ext.to_os_string());
+    if !ALLOWED_EXTENSIONS.contains(&ext) {
+        return Err(not_found("file extension not allowed"));
+    }
+
+    // We only serve assets from assets/ within static_dir
+    let assets_dir = &apictx
+        .console_config
+        .static_dir
+        .as_ref()
+        .ok_or_else(|| not_found("static_dir undefined"))?
+        .join("assets");
+
+    let request = &rqctx.request.lock().await;
+    let accept_encoding = request.headers().get(http::header::ACCEPT_ENCODING);
+    let accept_gz = accept_encoding.map_or(false, |val| {
+        val.to_str().map_or(false, |s| s.contains("gzip"))
+    });
+
+    // If req accepts gzip and we have a gzipped version, serve that. Otherwise
+    // fall back to non-gz. If neither file found, bubble up 404.
+    let (path_to_read, set_content_encoding_gzip) =
+        match accept_gz.then(|| find_file(&with_gz_ext(&path), &assets_dir)) {
+            Some(Ok(gzipped_path)) => (gzipped_path, true),
+            _ => (find_file(&path, &assets_dir)?, false),
+        };
 
-    // Derive the MIME type from the file name
-    let content_type = mime_guess::from_path(&file)
-        .first()
-        .map_or_else(|| "text/plain".to_string(), |m| m.to_string());
+    // File read is the same regardless of gzip
+    let file_contents = tokio::fs::read(&path_to_read).await.map_err(|e| {
+        not_found(&format!("accessing {:?}: {:#}", path_to_read, e))
+    })?;
 
-    Ok(Response::builder()
+    // Derive the MIME type from the file name (can't use path_to_read because
+    // it might end with .gz)
+    let content_type = path.file_name().map_or("text/plain", |f| {
+        mime_guess::from_path(f).first_raw().unwrap_or("text/plain")
+    });
+
+    let mut resp = Response::builder()
         .status(StatusCode::OK)
-        .header(http::header::CONTENT_TYPE, &content_type)
-        .header(http::header::CACHE_CONTROL, cache_control_header_value(apictx))
-        .body(file_contents.into())?)
+        .header(http::header::CONTENT_TYPE, content_type)
+        .header(http::header::CACHE_CONTROL, cache_control_value(apictx));
+
+    if set_content_encoding_gzip {
+        resp = resp.header(http::header::CONTENT_ENCODING, "gzip");
+    }
+
+    Ok(resp.body(file_contents.into())?)
 }
 
-fn cache_control_header_value(apictx: &Arc<ServerContext>) -> String {
-    format!(
-        "max-age={}",
-        apictx.console_config.cache_control_max_age.num_seconds()
-    )
+fn cache_control_value(apictx: &Arc<ServerContext>) -> String {
+    let max_age = apictx.console_config.cache_control_max_age.num_seconds();
+    format!("max-age={max_age}")
 }
 
 pub async fn serve_console_index(
@@ -676,7 +719,7 @@ pub async fn serve_console_index(
     Ok(Response::builder()
         .status(StatusCode::OK)
         .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8")
-        .header(http::header::CACHE_CONTROL, cache_control_header_value(apictx))
+        .header(http::header::CACHE_CONTROL, cache_control_value(apictx))
         .body(file_contents.into())?)
 }
 
@@ -694,22 +737,11 @@ lazy_static! {
     );
 }
 
-fn file_ext_allowed(path: &PathBuf) -> bool {
-    let ext = path
-        .extension()
-        .map(|ext| ext.to_os_string())
-        .unwrap_or_else(|| OsString::from("disallowed"));
-    ALLOWED_EXTENSIONS.contains(&ext)
-}
-
 /// Starting from `root_dir`, follow the segments of `path` down the file tree
 /// until we find a file (or not). Do not follow symlinks.
-fn find_file(
-    path: Vec<String>,
-    root_dir: &PathBuf,
-) -> Result<PathBuf, HttpError> {
+fn find_file(path: &PathBuf, root_dir: &PathBuf) -> Result<PathBuf, HttpError> {
     let mut current = root_dir.to_owned(); // start from `root_dir`
-    for segment in &path {
+    for segment in path.into_iter() {
         // If we hit a non-directory thing already and we still have segments
         // left in the path, bail. We have nowhere to go.
         if !current.is_dir() {
@@ -734,10 +766,6 @@ fn find_file(
         return Err(not_found("expected a non-directory"));
     }
 
-    if !file_ext_allowed(&current) {
-        return Err(not_found("file extension not allowed"));
-    }
-
     Ok(current)
 }
 
@@ -747,34 +775,34 @@ mod test {
     use http::StatusCode;
     use std::{env::current_dir, path::PathBuf};
 
-    fn get_path(path_str: &str) -> Vec<String> {
-        path_str.split("/").map(|s| s.to_string()).collect()
-    }
-
     #[test]
     fn test_find_file_finds_file() {
         let root = current_dir().unwrap();
-        let file = find_file(get_path("tests/static/assets/hello.txt"), &root);
+        let file =
+            find_file(&PathBuf::from("tests/static/assets/hello.txt"), &root);
         assert!(file.is_ok());
-        let file = find_file(get_path("tests/static/index.html"), &root);
+        let file = find_file(&PathBuf::from("tests/static/index.html"), &root);
         assert!(file.is_ok());
     }
 
     #[test]
     fn test_find_file_404_on_nonexistent() {
         let root = current_dir().unwrap();
-        let error = find_file(get_path("tests/static/nonexistent.svg"), &root)
-            .unwrap_err();
+        let error =
+            find_file(&PathBuf::from("tests/static/nonexistent.svg"), &root)
+                .unwrap_err();
         assert_eq!(error.status_code, StatusCode::NOT_FOUND);
         assert_eq!(error.internal_message, "failed to get file metadata",);
     }
 
     #[test]
     fn test_find_file_404_on_nonexistent_nested() {
         let root = current_dir().unwrap();
-        let error =
-            find_file(get_path("tests/static/a/b/c/nonexistent.svg"), &root)
-                .unwrap_err();
+        let error = find_file(
+            &PathBuf::from("tests/static/a/b/c/nonexistent.svg"),
+            &root,
+        )
+        .unwrap_err();
         assert_eq!(error.status_code, StatusCode::NOT_FOUND);
         assert_eq!(error.internal_message, "failed to get file metadata")
     }
@@ -783,7 +811,7 @@ mod test {
     fn test_find_file_404_on_directory() {
         let root = current_dir().unwrap();
         let error =
-            find_file(get_path("tests/static/assets/a_directory"), &root)
+            find_file(&PathBuf::from("tests/static/assets/a_directory"), &root)
                 .unwrap_err();
         assert_eq!(error.status_code, StatusCode::NOT_FOUND);
         assert_eq!(error.internal_message, "expected a non-directory");
@@ -803,7 +831,7 @@ mod test {
             .is_symlink());
 
         // so we 404
-        let error = find_file(get_path(path_str), &root).unwrap_err();
+        let error = find_file(&PathBuf::from(path_str), &root).unwrap_err();
         assert_eq!(error.status_code, StatusCode::NOT_FOUND);
         assert_eq!(error.internal_message, "attempted to follow a symlink");
     }
@@ -817,23 +845,8 @@ mod test {
         assert!(root.join(PathBuf::from(path_str)).exists());
 
         // but it 404s because the path goes through a symlink
-        let error = find_file(get_path(path_str), &root).unwrap_err();
+        let error = find_file(&PathBuf::from(path_str), &root).unwrap_err();
         assert_eq!(error.status_code, StatusCode::NOT_FOUND);
         assert_eq!(error.internal_message, "attempted to follow a symlink");
     }
-
-    #[test]
-    fn test_find_file_404_on_disallowed_ext() {
-        let root = current_dir().unwrap();
-        let error =
-            find_file(get_path("tests/static/assets/blocked.ext"), &root)
-                .unwrap_err();
-        assert_eq!(error.status_code, StatusCode::NOT_FOUND);
-        assert_eq!(error.internal_message, "file extension not allowed",);
-
-        let error = find_file(get_path("tests/static/assets/no_ext"), &root)
-            .unwrap_err();
-        assert_eq!(error.status_code, StatusCode::NOT_FOUND);
-        assert_eq!(error.internal_message, "file extension not allowed");
-    }
 }
diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs
@@ -85,6 +85,7 @@ impl<'a> RequestBuilder<'a> {
             expected_status: None,
             allowed_headers: Some(vec![
                 http::header::CACHE_CONTROL,
+                http::header::CONTENT_ENCODING,
                 http::header::CONTENT_LENGTH,
                 http::header::CONTENT_TYPE,
                 http::header::DATE,

diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs
@@ -221,11 +221,72 @@ async fn test_assets(cptestctx: &ControlPlaneTestContext) {
 
     // existing file is returned
     let resp = RequestBuilder::new(&testctx, Method::GET, "/assets/hello.txt")
+        .expect_status(Some(StatusCode::OK))
         .execute()
         .await
         .expect("failed to get existing file");
 
     assert_eq!(resp.body, "hello there".as_bytes());
+    // make sure we're not including the gzip header on non-gzipped files
+    assert_eq!(resp.headers.get(http::header::CONTENT_ENCODING), None);
+
+    // file in a directory is returned
+    let resp = RequestBuilder::new(
+        &testctx,
+        Method::GET,
+        "/assets/a_directory/another_file.txt",
+    )
+    .expect_status(Some(StatusCode::OK))
+    .execute()
+    .await
+    .expect("failed to get existing file");
+
+    assert_eq!(resp.body, "some words".as_bytes());
+    // make sure we're not including the gzip header on non-gzipped files
+    assert_eq!(resp.headers.get(http::header::CONTENT_ENCODING), None);
+
+    // file with only gzipped version 404s if request doesn't have accept-encoding: gzip
+    let _ = RequestBuilder::new(&testctx, Method::GET, "/assets/gzip-only.txt")
+        .expect_status(Some(StatusCode::NOT_FOUND))
+        .execute()
+        .await
+        .expect("failed to 404 on gzip file without accept-encoding: gzip");
+
+    // file with only gzipped version is returned if request accepts gzip
+    let resp =
+        RequestBuilder::new(&testctx, Method::GET, "/assets/gzip-only.txt")
+            .header(http::header::ACCEPT_ENCODING, "gzip")
+            .expect_status(Some(StatusCode::OK))
+            .expect_response_header(http::header::CONTENT_ENCODING, "gzip")
+            .execute()
+            .await
+            .expect("failed to get existing file");
+
+    assert_eq!(resp.body, "nothing but gzip".as_bytes());
+
+    // file with both gzip and not returns gzipped if request accepts gzip
+    let resp =
+        RequestBuilder::new(&testctx, Method::GET, "/assets/gzip-and-not.txt")
+            .header(http::header::ACCEPT_ENCODING, "gzip")
+            .expect_status(Some(StatusCode::OK))
+            .expect_response_header(http::header::CONTENT_ENCODING, "gzip")
+            .execute()
+            .await
+            .expect("failed to get existing file");
+
+    assert_eq!(resp.body, "pretend this is gzipped beep boop".as_bytes());
+
+    // returns non-gzipped if request doesn't accept gzip
+    let resp =
+        RequestBuilder::new(&testctx, Method::GET, "/assets/gzip-and-not.txt")
+            .expect_status(Some(StatusCode::OK))
+            .execute()
+            .await
+            .expect("failed to get existing file");
+
+    assert_eq!(resp.body, "not gzipped but I know a guy".as_bytes());
+    // make sure we're not including the gzip header on non-gzipped files
+    assert_eq!(resp.headers.get(http::header::CONTENT_ENCODING), None);
 }
 
 #[tokio::test]

diff --git a/nexus/tests/static/assets/gzip-and-not.txt b/nexus/tests/static/assets/gzip-and-not.txt
@@ -0,0 +1 @@
+not gzipped but I know a guy
diff --git a/nexus/tests/static/assets/gzip-and-not.txt.gz b/nexus/tests/static/assets/gzip-and-not.txt.gz
@@ -0,0 +1 @@
+pretend this is gzipped beep boop
diff --git a/nexus/tests/static/assets/gzip-only.txt.gz b/nexus/tests/static/assets/gzip-only.txt.gz
@@ -0,0 +1 @@
+nothing but gzip