Fix static file Last-Modified headers

Author: lovasoa

src/filesystem.rs

@@ -36,6 +36,32 @@ impl FileSystem {
         }
     }
 
+    pub async fn last_modified(
+        &self,
+        app_state: &AppState,
+        path: &Path,
+        priviledged: bool,
+    ) -> anyhow::Result<DateTime<Utc>> {
+        let local_path = self.safe_local_path(app_state, path, priviledged)?;
+        let local_result = tokio::fs::metadata(&local_path)
+            .await
+            .and_then(|metadata| metadata.modified())
+            .map(DateTime::<Utc>::from);
+        match (local_result, &self.db_fs_queries) {
+            (Ok(last_modified), _) => Ok(last_modified),
+            (Err(e), Some(db_fs)) if is_path_missing_error(&e) => {
+                db_fs.last_modified_in_db(app_state, path).await
+            }
+            (Err(e), _) => {
+                let status = io_error_status(&e)
+                    .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR);
+                Err(e).with_status(status).with_context(|| {
+                    format!("Unable to read local file metadata for {}", path.display())
+                })
+            }
+        }
+    }
+
     pub async fn modified_since(
         &self,
         app_state: &AppState,
@@ -258,6 +284,7 @@ pub struct DbFsQueries {
     was_modified: AnyStatement<'static>,
     read_file: AnyStatement<'static>,
     exists: AnyStatement<'static>,
+    last_modified: AnyStatement<'static>,
 }
 
 impl DbFsQueries {
@@ -286,6 +313,7 @@ impl DbFsQueries {
             was_modified: Self::make_was_modified_query(db).await?,
             read_file: Self::make_read_file_query(db).await?,
             exists: Self::make_exists_query(db).await?,
+            last_modified: Self::make_last_modified_query(db).await?,
         })
     }
 
@@ -300,7 +328,7 @@ impl DbFsQueries {
 
     async fn make_was_modified_query(db: &Database) -> anyhow::Result<AnyStatement<'static>> {
         let was_modified_query = format!(
-            "SELECT 1 from sqlpage_files WHERE last_modified >= {} AND path = {}",
+            "SELECT 1 from sqlpage_files WHERE last_modified > {} AND path = {}",
             make_placeholder(db.info.kind, 1),
             make_placeholder(db.info.kind, 2)
         );
@@ -331,6 +359,39 @@ impl DbFsQueries {
         db.prepare_with(&exists_query, param_types).await
     }
 
+    async fn make_last_modified_query(db: &Database) -> anyhow::Result<AnyStatement<'static>> {
+        let last_modified_query = format!(
+            "SELECT last_modified from sqlpage_files WHERE path = {}",
+            make_placeholder(db.info.kind, 1),
+        );
+        let param_types: &[AnyTypeInfo; 1] = &[<str as Type<Postgres>>::type_info().into()];
+        db.prepare_with(&last_modified_query, param_types).await
+    }
+
+    async fn last_modified_in_db(
+        &self,
+        app_state: &AppState,
+        path: &Path,
+    ) -> anyhow::Result<DateTime<Utc>> {
+        self.last_modified
+            .query_as::<(DateTime<Utc>,)>()
+            .bind(path.display().to_string())
+            .fetch_optional(&app_state.db.connection)
+            .await
+            .map_err(anyhow::Error::from)
+            .and_then(|last_modified| {
+                last_modified
+                    .map(|(last_modified,)| last_modified)
+                    .ok_or_else(|| {
+                        ErrorWithStatus {
+                            status: actix_web::http::StatusCode::NOT_FOUND,
+                        }
+                        .into()
+                    })
+            })
+            .with_context(|| format!("Unable to get the modification time of {}", path.display()))
+    }
+
     async fn file_modified_since_in_db(
         &self,
         app_state: &AppState,
@@ -475,5 +536,16 @@ async fn test_sql_file_read_utf8() -> anyhow::Result<()> {
         "File should not be modified since one hour in the future"
     );
 
+    let last_modified = fs
+        .last_modified(&state, "unit test file.txt".as_ref(), false)
+        .await?;
+    let was_modified = fs
+        .modified_since(&state, "unit test file.txt".as_ref(), last_modified, false)
+        .await?;
+    assert!(
+        !was_modified,
+        "A file should not be considered modified since its exact modification time"
+    );
+
     Ok(())
 }

src/webserver/http.rs

@@ -33,14 +33,12 @@ use crate::webserver::routing::RoutingAction::{
 use crate::webserver::routing::{AppFileStore, calculate_route};
 use actix_web::body::MessageBody;
 use anyhow::{Context, bail};
-use chrono::{DateTime, Utc};
 use futures_util::StreamExt;
 use futures_util::stream::Stream;
 use std::borrow::Cow;
 use std::path::PathBuf;
 use std::pin::Pin;
 use std::sync::Arc;
-use std::time::SystemTime;
 use tokio::sync::mpsc;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
@@ -441,17 +439,19 @@ async fn serve_file(
     if_modified_since: Option<IfModifiedSince>,
 ) -> actix_web::Result<HttpResponse> {
     let path = strip_site_prefix(path, state);
-    if let Some(IfModifiedSince(date)) = if_modified_since {
-        let since = DateTime::<Utc>::from(SystemTime::from(date));
-        let modified = state
-            .file_system
-            .modified_since(state, path.as_ref(), since, false)
-            .await
-            .with_context(|| format!("Unable to get modification time of file {path:?}"))
-            .map_err(|e| anyhow_err_to_actix(e, state))?;
-        if !modified {
-            return Ok(HttpResponse::NotModified().finish());
-        }
+    let last_modified = state
+        .file_system
+        .last_modified(state, path.as_ref(), false)
+        .await
+        .with_context(|| format!("Unable to get modification time of file {path:?}"))
+        .map_err(|e| anyhow_err_to_actix(e, state))?;
+    let last_modified = HttpDate::from(last_modified.into());
+    if let Some(IfModifiedSince(date)) = if_modified_since
+        && last_modified <= date
+    {
+        return Ok(HttpResponse::NotModified()
+            .insert_header(LastModified(last_modified))
+            .finish());
     }
     state
         .file_system
@@ -466,7 +466,7 @@ async fn serve_file(
                         .first()
                         .map_or_else(ContentType::octet_stream, ContentType),
                 )
-                .insert_header(LastModified(HttpDate::from(SystemTime::now())))
+                .insert_header(LastModified(last_modified))
                 .body(b)
         })
 }

tests/requests/mod.rs

@@ -248,3 +248,43 @@ async fn test_missing_multipart_content_disposition_returns_bad_request() -> act
 }
 
 mod webhook_hmac;
+
+#[actix_web::test]
+async fn static_file_uses_source_last_modified_for_revalidation() -> anyhow::Result<()> {
+    use actix_web::http::header;
+    use actix_web::http::header::HttpDate;
+    use std::time::{SystemTime, UNIX_EPOCH};
+
+    let unique = SystemTime::now().duration_since(UNIX_EPOCH)?.as_nanos();
+    let web_root = std::env::temp_dir().join(format!("sqlpage-last-modified-{unique}"));
+    std::fs::create_dir_all(&web_root)?;
+    let asset_path = web_root.join("asset.txt");
+    std::fs::write(&asset_path, "hello static world\n")?;
+
+    let expected = HttpDate::from(std::fs::metadata(&asset_path)?.modified()?);
+    let mut config = crate::common::test_config();
+    config.web_root = web_root.clone();
+    let app_data = crate::common::make_app_data_from_config(config).await;
+
+    let req = crate::common::get_request_to_with_data("/asset.txt", app_data.clone())
+        .await?
+        .to_srv_request();
+    let response = main_handler(req).await?;
+    assert_eq!(response.status(), StatusCode::OK);
+    let last_modified = response
+        .headers()
+        .get(header::LAST_MODIFIED)
+        .expect("static files should have a Last-Modified header")
+        .to_str()?;
+    assert_eq!(last_modified, expected.to_string());
+
+    let req = crate::common::get_request_to_with_data("/asset.txt", app_data)
+        .await?
+        .insert_header((header::IF_MODIFIED_SINCE, last_modified))
+        .to_srv_request();
+    let response = main_handler(req).await?;
+    assert_eq!(response.status(), StatusCode::NOT_MODIFIED);
+
+    std::fs::remove_dir_all(web_root)?;
+    Ok(())
+}