fix(h5p-server): multiple down- and uploads don't lead to long filena…

…mes (#1866)

packages/h5p-server/src/TemporaryFileManager.ts

@@ -1,11 +1,9 @@
 import { ReadStream } from 'fs';
-import path from 'path';
-import { nanoid } from 'nanoid';
 import { Readable } from 'stream';
 
-import H5pError from './helpers/H5pError';
 import Logger from './helpers/Logger';
 import { IH5PConfig, ITemporaryFileStorage, IUser } from './types';
+import FilenameGenerator from './helpers/FilenameGenerator';
 
 const log = new Logger('TemporaryFileManager');
 
@@ -135,29 +133,12 @@ export default class TemporaryFileManager {
         filename: string,
         user: IUser
     ): Promise<string> {
-        let attempts = 0;
-        let filenameAttempt = '';
-        let exists = false;
-        const cleanedFilename = this.storage.sanitizeFilename
-            ? this.storage.sanitizeFilename(filename)
-            : filename;
-        const dirname = path.dirname(cleanedFilename);
-        do {
-            filenameAttempt = `${
-                dirname && dirname !== '.' ? `${dirname}/` : ''
-            }${path.basename(
-                cleanedFilename,
-                path.extname(cleanedFilename)
-            )}-${nanoid(8)}${path.extname(cleanedFilename)}`;
-            exists = await this.storage.fileExists(filenameAttempt, user);
-            attempts += 1;
-        } while (attempts < 5 && exists); // only try 5 times
-        if (exists) {
-            log.error(`Cannot determine a unique filename for ${filename}`);
-            throw new H5pError('error-generating-unique-temporary-filename', {
-                name: filename
-            });
-        }
-        return filenameAttempt;
+        return FilenameGenerator(
+            filename,
+            this.storage.sanitizeFilename
+                ? (f) => this.storage.sanitizeFilename(f)
+                : (f) => f,
+            (f) => this.storage.fileExists(f, user)
+        );
     }
 }

packages/h5p-server/src/helpers/FilenameGenerator.ts

@@ -38,7 +38,9 @@ export default async (
     actualFilename = sanitize(actualFilename);
     const dirname = upath.dirname(actualFilename);
     do {
-        filenameAttempt = `${dirname ? `${dirname}/` : ''}${upath.basename(
+        filenameAttempt = `${
+            dirname && dirname !== '.' ? `${dirname}/` : ''
+        }${upath.basename(
             actualFilename,
             upath.extname(actualFilename)
         )}-${nanoid()}${upath.extname(actualFilename)}`;

packages/h5p-server/src/implementation/fs/DirectoryTemporaryFileStorage.ts

@@ -150,13 +150,13 @@ export default class DirectoryTemporaryFileStorage
      * (e.g. "images/image1.png")
      * @returns the clean filename
      */
-    public sanitizeFilename(filename: string): string {
+    public sanitizeFilename = (filename: string): string => {
         return sanitizeFilename(
             filename,
             this.maxFileLength,
             this.options?.invalidCharactersRegexp
         );
-    }
+    };
 
     public async saveFile(
         filename: string,

packages/h5p-server/src/implementation/fs/FileContentStorage.ts

@@ -471,11 +471,11 @@ export default class FileContentStorage implements IContentStorage {
      * (e.g. "images/image1.png")
      * @returns the clean filename
      */
-    public sanitizeFilename(filename: string): string {
+    public sanitizeFilename = (filename: string): string => {
         return sanitizeFilename(
             filename,
             this.maxFileLength,
             this.options?.invalidCharactersRegexp
         );
-    }
+    };
 }

packages/h5p-server/test/H5PEditor.uploadingPackages.test.ts

@@ -1,11 +1,14 @@
+/* eslint-disable no-await-in-loop */
+
 import fsExtra from 'fs-extra';
 import path from 'path';
-import { withDir } from 'tmp-promise';
+import { withDir, file, FileResult } from 'tmp-promise';
 
 import User from './User';
-
 import { createH5PEditor } from './helpers/H5PEditor';
 
+import { ContentMetadata } from '../src/ContentMetadata';
+
 describe('H5PEditor', () => {
     it('returns metadata and parameters of package uploads', async () => {
         await withDir(
@@ -78,4 +81,59 @@ describe('H5PEditor', () => {
             { keep: false, unsafeCleanup: true }
         );
     });
+
+    it('keeps filenames short in multiple uploads', async () => {
+        // Regression check for a bug in which content filenames became very
+        // long when users exported and uploaded h5ps repeatedly. (#1852)
+        await withDir(
+            async ({ path: tempDirPath }) => {
+                const { h5pEditor } = createH5PEditor(tempDirPath);
+                const user = new User();
+                let currentFilename = path.resolve(
+                    'test/data/validator/valid2.h5p'
+                );
+                let tempFile: FileResult;
+
+                for (let x = 0; x < 10; x++) {
+                    // Upload h5p
+                    let fileBuffer = await fsExtra.readFile(currentFilename);
+                    const { metadata, parameters } =
+                        await h5pEditor.uploadPackage(fileBuffer, user);
+
+                    // Delete old h5p file on disk if it exists
+                    await tempFile?.cleanup();
+
+                    const contentId = await h5pEditor.saveOrUpdateContent(
+                        undefined,
+                        parameters,
+                        metadata,
+                        ContentMetadata.toUbername(metadata),
+                        user
+                    );
+
+                    // Download h5p
+                    tempFile = await file({ keep: false, postfix: '.h5p' });
+                    currentFilename = tempFile.path;
+                    const writeStream =
+                        fsExtra.createWriteStream(currentFilename);
+                    const packageFinishedPromise = new Promise<void>(
+                        (resolve) => {
+                            writeStream.on('close', () => {
+                                resolve();
+                            });
+                        }
+                    );
+                    await h5pEditor.exportContent(contentId, writeStream, user);
+                    await packageFinishedPromise;
+                    writeStream.close();
+
+                    // CHeck if filename remains short
+                    expect(
+                        /^earth-[0-9a-z]+\.jpg$/i.test(parameters?.image?.path)
+                    ).toBe(true);
+                }
+            },
+            { keep: false, unsafeCleanup: true }
+        );
+    });
 });

packages/h5p-server/test/helpers/FilenameGenerator.test.ts

@@ -0,0 +1,54 @@
+import filenameGenerator from '../../src/helpers/FilenameGenerator';
+
+describe('FilenameGenerator', () => {
+    it('generates correct filenames', async () => {
+        const newFilename = await filenameGenerator(
+            'test.jpg',
+            (str) => str,
+            () => new Promise((res) => res(false))
+        );
+        expect(/^test-[0-9a-z]{8}\.jpg$/i.test(newFilename)).toEqual(true);
+    });
+
+    it('fixes name collisions', async () => {
+        let isFirst = true;
+        let firstTry = '';
+        const newFilename = await filenameGenerator(
+            'test.jpg',
+            (str) => str,
+            (f) =>
+                new Promise((res) => {
+                    if (isFirst) {
+                        firstTry = f;
+                        isFirst = false;
+                        return res(true);
+                    }
+                    return res(false);
+                })
+        );
+        expect(/^test-[0-9a-z]{8}\.jpg$/i.test(newFilename)).toEqual(true);
+        expect(firstTry).not.toEqual(newFilename);
+    });
+
+    it('detects postfixes and replaces them', async () => {
+        const newFilename = await filenameGenerator(
+            'test-1234ABCD.jpg',
+            (str) => str,
+            () => new Promise((res) => res(false))
+        );
+        expect(/^test-[0-9a-z]{8}\.jpg$/i.test(newFilename)).toEqual(true);
+    });
+
+    it("doesn't append multiple postfixes", async () => {
+        let filename = 'test-1234ABCD.jpg';
+        for (let x = 0; x < 100; x++) {
+            // eslint-disable-next-line no-await-in-loop
+            filename = await filenameGenerator(
+                filename,
+                (str) => str,
+                () => new Promise((res) => res(false))
+            );
+        }
+        expect(/^test-[0-9a-z]{8}\.jpg$/i.test(filename)).toEqual(true);
+    });
+});