feat: Remove original file stem from filenames in the blobstorage (#4309)

This way filenames in the blobstorage are just random hex numbers. This also allows us to get rid of the `sanitize-filename` dependency. This also requires `Param::Filename` to be set to "debug_logging*.xdc" for messages containing logging webxdc-s, otherwise they are not detected properly. This is done in "fix: Message::set_file_from_bytes(): Set Param::Filename", so don't forget to update senders as well.
2026-04-24 08:56:29 +03:00 · 2024-04-27 01:05:02 -03:00
parent 7391c8ddea
commit 204f747a54
15 changed files with 50 additions and 124 deletions
--- a/src/blob.rs
+++ b/src/blob.rs
@@ -56,8 +56,8 @@ impl<'a> BlobObject<'a> {
        data: &[u8],
    ) -> Result<BlobObject<'a>> {
        let blobdir = context.get_blobdir();
-        let (stem, ext) = BlobObject::sanitise_name(suggested_name);
-        let (name, mut file) = BlobObject::create_new_file(context, blobdir, &stem, &ext).await?;
+        let ext = BlobObject::get_extension(suggested_name);
+        let (name, mut file) = BlobObject::create_new_file(context, blobdir, &ext).await?;
        file.write_all(data).await.context("file write failure")?;

        // workaround a bug in async-std
@@ -77,12 +77,11 @@ impl<'a> BlobObject<'a> {
    async fn create_new_file(
        context: &Context,
        dir: &Path,
-        stem: &str,
        ext: &str,
    ) -> Result<(String, fs::File)> {
        let mut attempt = 0;
        loop {
-            let name = format!("{}-{:016x}{}", stem, rand::random::<u64>(), ext);
+            let name = format!("{:016x}{}", rand::random::<u64>(), ext);
            attempt += 1;
            let path = dir.join(&name);
            match fs::OpenOptions::new()
@@ -113,9 +112,9 @@ impl<'a> BlobObject<'a> {
        let mut src_file = fs::File::open(src)
            .await
            .with_context(|| format!("failed to open file {}", src.display()))?;
-        let (stem, ext) = BlobObject::sanitise_name(&src.to_string_lossy());
+        let ext = BlobObject::get_extension(&src.to_string_lossy());
        let (name, mut dst_file) =
-            BlobObject::create_new_file(context, context.get_blobdir(), &stem, &ext).await?;
+            BlobObject::create_new_file(context, context.get_blobdir(), &ext).await?;
        let name_for_err = name.clone();
        if let Err(err) = io::copy(&mut src_file, &mut dst_file).await {
            // Attempt to remove the failed file, swallow errors resulting from that.
@@ -139,10 +138,8 @@ impl<'a> BlobObject<'a> {
    ///
    /// If the source file is not a path to into the blob directory
    /// the file will be copied into the blob directory first.  If the
-    /// source file is already in the blobdir it will not be copied
-    /// and only be created if it is a valid blobname, that is no
-    /// subdirectory is used and [BlobObject::sanitise_name] does not
-    /// modify the filename.
+    /// source file is already in the blobdir (but not in a subdirectory)
+    /// it will not be copied and only be created if it is a valid blobname.
    ///
    /// Paths into the blob directory may be either defined by an absolute path
    /// or by the relative prefix `$BLOBDIR`.
@@ -159,8 +156,7 @@ impl<'a> BlobObject<'a> {
    /// Returns a [BlobObject] for an existing blob from a path.
    ///
    /// The path must designate a file directly in the blobdir and
-    /// must use a valid blob name.  That is after sanitisation the
-    /// name must still be the same, that means it must be valid UTF-8
+    /// must use a valid blob name. That means it must be valid UTF-8
    /// and not have any special characters in it.
    pub fn from_path(context: &'a Context, path: &Path) -> Result<BlobObject<'a>> {
        let rel_path = path
@@ -234,21 +230,8 @@ impl<'a> BlobObject<'a> {
        }
    }

-    /// Create a safe name based on a messy input string.
-    ///
-    /// The safe name will be a valid filename on Unix and Windows and
-    /// not contain any path separators.  The input can contain path
-    /// segments separated by either Unix or Windows path separators,
-    /// the rightmost non-empty segment will be used as name,
-    /// sanitised for special characters.
-    ///
-    /// The resulting name is returned as a tuple, the first part
-    /// being the stem or basename and the second being an extension,
-    /// including the dot.  E.g. "foo.txt" is returned as `("foo",
-    /// ".txt")` while "bar" is returned as `("bar", "")`.
-    ///
-    /// The extension part will always be lowercased.
-    fn sanitise_name(name: &str) -> (String, String) {
+    /// Get a file extension if any, including the dot, in lower case, otherwise an empty string.
+    fn get_extension(name: &str) -> String {
        let mut name = name.to_string();
        for part in name.rsplit('/') {
            if !part.is_empty() {
@@ -262,20 +245,12 @@ impl<'a> BlobObject<'a> {
                break;
            }
        }
-        let opts = sanitize_filename::Options {
-            truncate: true,
-            windows: true,
-            replacement: "",
-        };

-        let clean = sanitize_filename::sanitize_with_options(name, opts);
        // Let's take the tricky filename
        // "file.with_lots_of_characters_behind_point_and_double_ending.tar.gz" as an example.
        // Split it into "file" and "with_lots_of_characters_behind_point_and_double_ending.tar.gz":
-        let mut iter = clean.splitn(2, '.');
-
-        let stem: String = iter.next().unwrap_or_default().chars().take(64).collect();
-        // stem == "file"
+        let mut iter = name.splitn(2, '.');
+        iter.next();

        let ext_chars = iter.next().unwrap_or_default().chars();
        let ext: String = ext_chars
@@ -288,11 +263,10 @@ impl<'a> BlobObject<'a> {
        // ext == "d_point_and_double_ending.tar.gz"

        if ext.is_empty() {
-            (stem, "".to_string())
+            ext
        } else {
-            (stem, format!(".{ext}").to_lowercase())
-            // Return ("file", ".d_point_and_double_ending.tar.gz")
-            // which is not perfect but acceptable.
+            format!(".{ext}").to_lowercase()
+            // Return ".d_point_and_double_ending.tar.gz", which is not perfect but acceptable.
        }
    }

@@ -760,7 +734,7 @@ mod tests {
    async fn test_create() {
        let t = TestContext::new().await;
        let blob = BlobObject::create(&t, "foo", b"hello").await.unwrap();
-        let re = Regex::new("^foo-[[:xdigit:]]{16}$").unwrap();
+        let re = Regex::new("^[[:xdigit:]]{16}$").unwrap();
        assert!(re.is_match(blob.as_file_name()));
        let fname = t.get_blobdir().join(blob.as_file_name());
        let data = fs::read(fname).await.unwrap();
@@ -779,7 +753,7 @@ mod tests {
    async fn test_lowercase_ext() {
        let t = TestContext::new().await;
        let blob = BlobObject::create(&t, "foo.TXT", b"hello").await.unwrap();
-        let re = Regex::new("^\\$BLOBDIR/foo-[[:xdigit:]]{16}.txt$").unwrap();
+        let re = Regex::new("^\\$BLOBDIR/[[:xdigit:]]{16}.txt$").unwrap();
        assert!(re.is_match(blob.as_name()));
    }

@@ -787,7 +761,7 @@ mod tests {
    async fn test_as_file_name() {
        let t = TestContext::new().await;
        let blob = BlobObject::create(&t, "foo.txt", b"hello").await.unwrap();
-        let re = Regex::new("^foo-[[:xdigit:]]{16}.txt$").unwrap();
+        let re = Regex::new("^[[:xdigit:]]{16}.txt$").unwrap();
        assert!(re.is_match(blob.as_file_name()));
    }

@@ -795,7 +769,7 @@ mod tests {
    async fn test_as_rel_path() {
        let t = TestContext::new().await;
        let blob = BlobObject::create(&t, "foo.txt", b"hello").await.unwrap();
-        let re = Regex::new("^foo-[[:xdigit:]]{16}.txt$").unwrap();
+        let re = Regex::new("^[[:xdigit:]]{16}.txt$").unwrap();
        assert!(re.is_match(blob.as_rel_path().to_str().unwrap()));
    }

@@ -811,7 +785,7 @@ mod tests {
    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
    async fn test_create_dup() {
        let t = TestContext::new().await;
-        let re = Regex::new("^foo-[[:xdigit:]]{16}.txt$").unwrap();
+        let re = Regex::new("^[[:xdigit:]]{16}.txt$").unwrap();

        let blob = BlobObject::create(&t, "foo.txt", b"hello").await.unwrap();
        assert!(re.is_match(blob.as_rel_path().to_str().unwrap()));
@@ -832,7 +806,7 @@ mod tests {
        let blob = BlobObject::create(&t, "foo.tar.gz", b"hello")
            .await
            .unwrap();
-        let re = Regex::new("^foo-[[:xdigit:]]{16}.tar.gz$").unwrap();
+        let re = Regex::new("^[[:xdigit:]]{16}.tar.gz$").unwrap();
        assert!(re.is_match(blob.as_file_name()));
        let foo_path = t.get_blobdir().join(blob.as_file_name());
        assert!(foo_path.exists());
@@ -847,7 +821,6 @@ mod tests {
            } else {
                let name = fname.to_str().unwrap();
                println!("{name}");
-                assert!(name.starts_with("foo"));
                assert!(name.ends_with(".tar.gz"));
            }
        }
@@ -868,7 +841,7 @@ mod tests {
        let src = t.dir.path().join("src");
        fs::write(&src, b"boo").await.unwrap();
        let blob = BlobObject::create_and_copy(&t, src.as_ref()).await.unwrap();
-        let re = Regex::new("^\\$BLOBDIR/src-[[:xdigit:]]{16}$").unwrap();
+        let re = Regex::new("^\\$BLOBDIR/[[:xdigit:]]{16}$").unwrap();
        assert!(re.is_match(blob.as_name()));
        let data = fs::read(blob.to_abs_path()).await.unwrap();
        assert_eq!(data, b"boo");
@@ -890,7 +863,7 @@ mod tests {
        let blob = BlobObject::new_from_path(&t, src_ext.as_ref())
            .await
            .unwrap();
-        let re = Regex::new("^\\$BLOBDIR/external-[[:xdigit:]]{16}$").unwrap();
+        let re = Regex::new("^\\$BLOBDIR/[[:xdigit:]]{16}$").unwrap();
        assert!(re.is_match(blob.as_name()));
        let data = fs::read(blob.to_abs_path()).await.unwrap();
        assert_eq!(data, b"boo");
@@ -903,20 +876,6 @@ mod tests {
        assert_eq!(data, b"boo");
    }

-    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-    async fn test_create_from_name_long() {
-        let t = TestContext::new().await;
-        let src_ext = t.dir.path().join("autocrypt-setup-message-4137848473.html");
-        fs::write(&src_ext, b"boo").await.unwrap();
-        let blob = BlobObject::new_from_path(&t, src_ext.as_ref())
-            .await
-            .unwrap();
-        let re =
-            Regex::new("^\\$BLOBDIR/autocrypt-setup-message-4137848473-[[:xdigit:]]{16}.html$")
-                .unwrap();
-        assert!(re.is_match(blob.as_name()));
-    }
-
    #[test]
    fn test_is_blob_name() {
        assert!(BlobObject::is_acceptible_blob_name("foo"));
@@ -928,42 +887,24 @@ mod tests {
    }

    #[test]
-    fn test_sanitise_name() {
-        let (stem, ext) =
-            BlobObject::sanitise_name("Я ЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯ.txt");
+    fn test_get_extension() {
+        let ext = BlobObject::get_extension("Я ЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯЯ.txt");
        assert_eq!(ext, ".txt");
-        assert!(!stem.is_empty());

-        // the extensions are kept together as between stem and extension a number may be added -
-        // and `foo.tar.gz` should become `foo-1234.tar.gz` and not `foo.tar-1234.gz`
-        let (stem, ext) = BlobObject::sanitise_name("wot.tar.gz");
-        assert_eq!(stem, "wot");
+        let ext = BlobObject::get_extension("wot.tar.gz");
        assert_eq!(ext, ".tar.gz");

-        let (stem, ext) = BlobObject::sanitise_name(".foo.bar");
-        assert_eq!(stem, "");
+        let ext = BlobObject::get_extension(".foo.bar");
        assert_eq!(ext, ".foo.bar");

-        let (stem, ext) = BlobObject::sanitise_name("foo?.bar");
-        assert!(stem.contains("foo"));
-        assert!(!stem.contains('?'));
+        let ext = BlobObject::get_extension("foo?.bar");
        assert_eq!(ext, ".bar");

-        let (stem, ext) = BlobObject::sanitise_name("no-extension");
-        assert_eq!(stem, "no-extension");
+        let ext = BlobObject::get_extension("no-extension");
        assert_eq!(ext, "");

-        let (stem, ext) = BlobObject::sanitise_name("path/ignored\\this: is* forbidden?.c");
+        let ext = BlobObject::get_extension("path/ignored\\this: is* forbidden?.c");
        assert_eq!(ext, ".c");
-        assert!(!stem.contains("path"));
-        assert!(!stem.contains("ignored"));
-        assert!(stem.contains("this"));
-        assert!(stem.contains("forbidden"));
-        assert!(!stem.contains('/'));
-        assert!(!stem.contains('\\'));
-        assert!(!stem.contains(':'));
-        assert!(!stem.contains('*'));
-        assert!(!stem.contains('?'));
    }

    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
@@ -1012,7 +953,7 @@ mod tests {
        let avatar_blob = t.get_config(Config::Selfavatar).await.unwrap().unwrap();
        let blobdir = t.get_blobdir().to_str().unwrap();
        assert!(avatar_blob.starts_with(blobdir));
-        let re = Regex::new("avatar-[[:xdigit:]]{16}.jpg$").unwrap();
+        let re = Regex::new("[[:xdigit:]]{16}.jpg$").unwrap();
        assert!(re.is_match(&avatar_blob));
        let avatar_blob = Path::new(&avatar_blob);
        assert!(avatar_blob.exists());
@@ -1089,7 +1030,7 @@ mod tests {
        let avatar_blob = t.get_config(Config::Selfavatar).await.unwrap().unwrap();
        let blobdir = t.get_blobdir().to_str().unwrap();
        assert!(avatar_blob.starts_with(blobdir));
-        let re = Regex::new("avatar-[[:xdigit:]]{16}.png$").unwrap();
+        let re = Regex::new("[[:xdigit:]]{16}.png$").unwrap();
        assert!(re.is_match(&avatar_blob));
        assert!(Path::new(&avatar_blob).exists());
        assert_eq!(
--- a/src/chat.rs
+++ b/src/chat.rs
@@ -7082,7 +7082,7 @@ mod tests {
            msg.param.get(Param::Filename).unwrap(),
            "harmless_file.txt.exe"
        );
-        let re = Regex::new("^\\$BLOBDIR/harmless_file-[[:xdigit:]]{16}.txt.exe$").unwrap();
+        let re = Regex::new("^\\$BLOBDIR/[[:xdigit:]]{16}.txt.exe$").unwrap();
        assert!(re.is_match(msg.param.get(Param::File).unwrap()));
        Ok(())
    }
--- a/src/debug_logging.rs
+++ b/src/debug_logging.rs
@@ -9,7 +9,6 @@ use crate::tools::time;
 use crate::webxdc::StatusUpdateItem;
 use async_channel::{self as channel, Receiver, Sender};
 use serde_json::json;
-use std::path::PathBuf;
 use tokio::task;

 #[derive(Debug)]
@@ -98,7 +97,7 @@ pub async fn maybe_set_logging_xdc(
        context,
        msg.get_viewtype(),
        chat_id,
-        msg.param.get_path(Param::File, context).unwrap_or_default(),
+        msg.param.get(Param::Filename),
        msg.get_id(),
    )
    .await?;
@@ -111,18 +110,16 @@ pub async fn maybe_set_logging_xdc_inner(
    context: &Context,
    viewtype: Viewtype,
    chat_id: ChatId,
-    file: Option<PathBuf>,
+    file_name: Option<&str>,
    msg_id: MsgId,
 ) -> anyhow::Result<()> {
    if viewtype == Viewtype::Webxdc {
-        if let Some(file) = file {
-            if let Some(file_name) = file.file_name().and_then(|name| name.to_str()) {
-                if file_name.starts_with("debug_logging")
-                    && file_name.ends_with(".xdc")
-                    && chat_id.is_self_talk(context).await?
-                {
-                    set_debug_logging_xdc(context, Some(msg_id)).await?;
-                }
+        if let Some(file_name) = file_name {
+            if file_name.starts_with("debug_logging")
+                && file_name.ends_with(".xdc")
+                && chat_id.is_self_talk(context).await?
+            {
+                set_debug_logging_xdc(context, Some(msg_id)).await?;
            }
        }
    }
--- a/src/imex/transfer.rs
+++ b/src/imex/transfer.rs
@@ -651,8 +651,9 @@ mod tests {
        };
        let msg = Message::load_from_db(&ctx1, *msgid).await.unwrap();

+        assert_eq!(&msg.get_filename().unwrap(), "hello.txt");
        let path = msg.get_file(&ctx1).unwrap();
-        assert_eq!(path.with_file_name("hello.txt"), path);
+        assert_eq!(path.with_extension("txt"), path);
        let text = fs::read_to_string(&path).await.unwrap();
        assert_eq!(text, "i am attachment");

--- a/src/message.rs
+++ b/src/message.rs
@@ -606,7 +606,8 @@ impl Message {
        None
    }

-    /// Returns the full path to the file associated with a message.
+    /// Returns the full path to the file associated with a message. The filename isn't meaningful,
+    /// only the extension is preserved.
    pub fn get_file(&self, context: &Context) -> Option<PathBuf> {
        self.param.get_path(Param::File, context).unwrap_or(None)
    }
@@ -759,8 +760,6 @@ impl Message {
    }

    /// Returns original filename (as shown in chat).
-    ///
-    /// To get the full path, use [`Self::get_file()`].
    pub fn get_filename(&self) -> Option<String> {
        if let Some(name) = self.param.get(Param::Filename) {
            return Some(name.to_string());
--- a/src/mimeparser.rs
+++ b/src/mimeparser.rs
@@ -3455,7 +3455,7 @@ On 2020-10-25, Bob wrote:
        assert_eq!(msg.chat_blocked, Blocked::Request);
        assert_eq!(msg.state, MessageState::InFresh);
        assert_eq!(msg.get_filebytes(&t).await.unwrap().unwrap(), 2115);
-        assert!(msg.get_file(&t).is_some());
+        assert_eq!(msg.get_file(&t).unwrap().extension().unwrap(), "png");
        assert_eq!(msg.get_filename().unwrap(), "avatar64x64.png");
        assert_eq!(msg.get_width(), 64);
        assert_eq!(msg.get_height(), 64);
@@ -3801,7 +3801,7 @@ Message.
            mime_message.parts[0].msg,
            "this is a classic email – I attached the .EML file".to_string()
        );
-        let re = Regex::new("^\\$BLOBDIR/-[[:xdigit:]]{16}.eml$").unwrap();
+        let re = Regex::new("^\\$BLOBDIR/[[:xdigit:]]{16}.eml$").unwrap();
        assert!(re.is_match(mime_message.parts[0].param.get(Param::File).unwrap()));

        assert_eq!(mime_message.parts[0].org_filename, Some(".eml".to_string()));
--- a/src/param.rs
+++ b/src/param.rs
@@ -547,8 +547,6 @@ mod tests {
        assert!(p.get_blob(Param::File, &t, false).await.is_err());

        fs::write(fname, b"boo").await.unwrap();
-        let blob = p.get_blob(Param::File, &t, true).await.unwrap().unwrap();
-        assert!(blob.as_file_name().starts_with("foo"));

        // Blob in blobdir, expect blob.
        let bar_path = t.get_blobdir().join("bar");
--- a/src/receive_imf.rs
+++ b/src/receive_imf.rs
@@ -1577,9 +1577,7 @@ RETURNING id
            context,
            part.typ,
            chat_id,
-            part.param
-                .get_path(Param::File, context)
-                .unwrap_or_default(),
+            part.param.get(Param::Filename),
            *msg_id,
        )
        .await?;
--- a/src/receive_imf/tests.rs
+++ b/src/receive_imf/tests.rs
@@ -1593,7 +1593,6 @@ async fn test_pdf_filename_simple() {
    assert_eq!(msg.viewtype, Viewtype::File);
    assert_eq!(msg.text, "mail body");
    let file_path = msg.param.get(Param::File).unwrap();
-    assert!(file_path.starts_with("$BLOBDIR/simple"));
    assert!(file_path.ends_with(".pdf"));
 }

@@ -1609,7 +1608,6 @@ async fn test_pdf_filename_continuation() {
    assert_eq!(msg.viewtype, Viewtype::File);
    assert_eq!(msg.text, "mail body");
    let file_path = msg.param.get(Param::File).unwrap();
-    assert!(file_path.starts_with("$BLOBDIR/test pdf äöüß"));
    assert!(file_path.ends_with(".pdf"));
 }