feat(storage): add safeguards to prevent file overwrites (#916)

.changeset/green-pets-juggle.md

@@ -0,0 +1,5 @@
+---
+"@papra/app": patch
+---
+
+Added low-level safeguards in the document storage service to prevent overwriting existing files

apps/papra-server/src/modules/documents/storage/document-storage.errors.ts

@@ -5,3 +5,9 @@ export const createFileNotFoundError = createErrorFactory({
   code: 'documents.storage.file_not_found',
   statusCode: 404,
 });
+
+export const createFileAlreadyExistsInStorageError = createErrorFactory({
+  message: 'File already exists',
+  code: 'documents.storage.file_already_exists',
+  statusCode: 409,
+});

apps/papra-server/src/modules/documents/storage/drivers/az-blob/az-blob.models.ts

@@ -0,0 +1,7 @@
+export function isAzureBlobAlreadyExistsError({ error }: { error: Error & { code?: unknown; statusCode?: unknown } }) {
+  return error.code === 'BlobAlreadyExists' || error.statusCode === 409;
+}
+
+export function isAzureBlobNotFoundError({ error }: { error: Error & { code?: unknown; statusCode?: unknown } }) {
+  return error.code === 'BlobNotFound' || error.statusCode === 404;
+}

apps/papra-server/src/modules/documents/storage/drivers/az-blob/az-blob.storage-driver.ts

@@ -2,15 +2,12 @@ import type { Readable } from 'node:stream';
 import { BlobServiceClient, StorageSharedKeyCredential } from '@azure/storage-blob';
 
 import { safely } from '@corentinth/chisels';
-import { createFileNotFoundError } from '../../document-storage.errors';
+import { createFileAlreadyExistsInStorageError, createFileNotFoundError } from '../../document-storage.errors';
 import { defineStorageDriver } from '../drivers.models';
+import { isAzureBlobAlreadyExistsError, isAzureBlobNotFoundError } from './az-blob.models';
 
 export const AZ_BLOB_STORAGE_DRIVER_NAME = 'azure-blob' as const;
 
-function isAzureBlobNotFoundError(error: Error): boolean {
-  return ('statusCode' in error && error.statusCode === 404) || ('code' in error && error.code === 'BlobNotFound');
-}
-
 export const azBlobStorageDriverFactory = defineStorageDriver(({ documentStorageConfig }) => {
   const { accountName, accountKey, containerName, connectionString } = documentStorageConfig.drivers.azureBlob;
 
@@ -24,14 +21,20 @@ export const azBlobStorageDriverFactory = defineStorageDriver(({ documentStorage
     name: AZ_BLOB_STORAGE_DRIVER_NAME,
     getClient: () => blobServiceClient,
     saveFile: async ({ fileStream, storageKey }) => {
-      await getBlockBlobClient({ storageKey }).uploadStream(fileStream);
+      const [, error] = await safely(getBlockBlobClient({ storageKey }).uploadStream(fileStream, undefined, undefined, { conditions: { ifNoneMatch: '*' } })); // Love those undefined :chef_kiss:
+
+      if (error) {
+        throw isAzureBlobAlreadyExistsError({ error })
+          ? createFileAlreadyExistsInStorageError()
+          : error;
+      }
 
       return { storageKey };
     },
     getFileStream: async ({ storageKey }) => {
       const [response, error] = await safely(getBlockBlobClient({ storageKey }).download());
 
-      if (error && isAzureBlobNotFoundError(error)) {
+      if (error && isAzureBlobNotFoundError({ error })) {
         throw createFileNotFoundError();
       }
 
@@ -46,7 +49,7 @@ export const azBlobStorageDriverFactory = defineStorageDriver(({ documentStorage
     deleteFile: async ({ storageKey }) => {
       const [, error] = await safely(getBlockBlobClient({ storageKey }).delete());
 
-      if (error && isAzureBlobNotFoundError(error)) {
+      if (error && isAzureBlobNotFoundError({ error })) {
         throw createFileNotFoundError();
       }
 
@@ -57,7 +60,7 @@ export const azBlobStorageDriverFactory = defineStorageDriver(({ documentStorage
     fileExists: async ({ storageKey }) => {
       const [, error] = await safely(getBlockBlobClient({ storageKey }).getProperties());
 
-      if (error && isAzureBlobNotFoundError(error)) {
+      if (error && isAzureBlobNotFoundError({ error })) {
         return false;
       }
 

apps/papra-server/src/modules/documents/storage/drivers/drivers.test-suite.ts

@@ -2,7 +2,7 @@ import type { StorageDriver, StorageServices } from './drivers.models';
 import { Buffer } from 'node:buffer';
 import { describe, expect, test } from 'vitest';
 import { collectReadableStreamToString, createReadableStream } from '../../../shared/streams/readable-stream';
-import { createFileNotFoundError } from '../document-storage.errors';
+import { createFileAlreadyExistsInStorageError, createFileNotFoundError } from '../document-storage.errors';
 import { wrapWithEncryptionLayer } from '../encryption/document-encryption.services';
 
 export function runDriverTestSuites({ createDriver: createDriverBase, timeout, retry }: { createDriver: () => Promise<{ driver: StorageDriver; [Symbol.asyncDispose]: () => Promise<void> }>; timeout?: number; retry?: number }) {
@@ -30,7 +30,7 @@ export function runDriverTestSuites({ createDriver: createDriverBase, timeout, r
       },
     },
   ].forEach(({ createStorageService, name }) => {
-    describe(name, () => {
+    describe.concurrent(name, () => {
       test('the driver should support uploading, retrieving and deleting files', { timeout, retry }, async () => {
         await using resource = await createStorageService();
 
@@ -51,6 +51,18 @@ export function runDriverTestSuites({ createDriver: createDriverBase, timeout, r
         // Check that the file exists
         expect(await storageServices.fileExists({ storageKey: 'files/test.txt' })).to.eql(true);
 
+        // Try to save another file with the same storage key and expect an error
+        await expect(storageServices.saveFile({
+          fileName: 'test.txt',
+          mimeType: 'text/plain',
+          storageKey: 'files/test.txt',
+          fileStream: createReadableStream({ content: 'Lorem ipsum' }),
+        })).rejects.toThrow(createFileAlreadyExistsInStorageError());
+
+        // Ensure that the original file is still intact after the failed attempt to overwrite it
+        const { fileStream: fileStreamAfterError } = await storageServices.getFileStream({ ...storageContext, storageKey: 'files/test.txt' });
+        expect(await collectReadableStreamToString({ stream: fileStreamAfterError })).to.eql('Hello, world!');
+
         // Delete the file
         await storageServices.deleteFile({ storageKey: 'files/test.txt' });
         await expect(storageServices.getFileStream({ storageKey: 'files/test.txt' })).rejects.toThrow(createFileNotFoundError());

apps/papra-server/src/modules/documents/storage/drivers/fs/fs.storage-driver.errors.ts

@@ -1,7 +0,0 @@
-import { createErrorFactory } from '../../../../shared/errors/errors';
-
-export const createFileAlreadyExistsError = createErrorFactory({
-  message: 'The file already exists.',
-  code: 'document.file_already_exists',
-  statusCode: 409,
-});

apps/papra-server/src/modules/documents/storage/drivers/fs/fs.storage-driver.int.test.ts

@@ -4,10 +4,9 @@ import { tmpdir } from 'node:os';
 import path, { join } from 'node:path';
 import { afterEach, beforeEach, describe, expect, test } from 'vitest';
 import { createReadableStream } from '../../../../shared/streams/readable-stream';
-import { createFileNotFoundError } from '../../document-storage.errors';
+import { createFileAlreadyExistsInStorageError, createFileNotFoundError } from '../../document-storage.errors';
 import { runDriverTestSuites } from '../drivers.test-suite';
 import { fsStorageDriverFactory } from './fs.storage-driver';
-import { createFileAlreadyExistsError } from './fs.storage-driver.errors';
 
 const createTmpDirectory = async () => fs.promises.mkdtemp(join(tmpdir(), 'tests-'));
 const deleteTmpDirectory = async (tmpDirectory: string) => fs.promises.rm(tmpDirectory, { recursive: true });
@@ -75,7 +74,7 @@ describe('storage driver', () => {
             mimeType: 'text/plain',
             storageKey: 'org_1/text-file.txt',
           }),
-        ).rejects.toThrow(createFileAlreadyExistsError());
+        ).rejects.toThrow(createFileAlreadyExistsInStorageError());
       });
     });
 

apps/papra-server/src/modules/documents/storage/drivers/fs/fs.storage-driver.ts

@@ -1,10 +1,10 @@
 import fs from 'node:fs';
 import { dirname, join } from 'node:path';
 import { get } from 'lodash-es';
+import { isFileAlreadyExistsError } from '../../../../shared/fs/fs.models';
 import { checkFileExists, deleteFile, ensureDirectoryExists } from '../../../../shared/fs/fs.services';
-import { createFileNotFoundError } from '../../document-storage.errors';
+import { createFileAlreadyExistsInStorageError, createFileNotFoundError } from '../../document-storage.errors';
 import { defineStorageDriver } from '../drivers.models';
-import { createFileAlreadyExistsError } from './fs.storage-driver.errors';
 
 export const FS_STORAGE_DRIVER_NAME = 'filesystem' as const;
 
@@ -18,15 +18,11 @@ export const fsStorageDriverFactory = defineStorageDriver(({ documentStorageConf
     saveFile: async ({ fileStream, storageKey }) => {
       const { storagePath } = getStoragePath({ storageKey });
 
-      const fileExists = await checkFileExists({ path: storagePath });
-
-      if (fileExists) {
-        throw createFileAlreadyExistsError();
-      }
-
       await ensureDirectoryExists({ path: dirname(storagePath) });
 
-      const writeStream = fs.createWriteStream(storagePath);
+      // 'wx' flag ensures that the file is created exclusively and fails if it already exists
+      const writeStream = fs.createWriteStream(storagePath, { flags: 'wx' });
+
       fileStream.pipe(writeStream);
 
       return new Promise((resolve, reject) => {
@@ -35,7 +31,11 @@ export const fsStorageDriverFactory = defineStorageDriver(({ documentStorageConf
         });
 
         writeStream.on('error', (error) => {
-          reject(error);
+          const rejection = isFileAlreadyExistsError({ error })
+            ? createFileAlreadyExistsInStorageError()
+            : error;
+
+          reject(rejection);
         });
 
         // Listen for errors on the input stream as well

apps/papra-server/src/modules/documents/storage/drivers/memory/memory.storage-driver.ts

@@ -1,6 +1,6 @@
 import type { Buffer } from 'node:buffer';
 import { collectReadableStreamToBuffer, createReadableStream } from '../../../../shared/streams/readable-stream';
-import { createFileNotFoundError } from '../../document-storage.errors';
+import { createFileAlreadyExistsInStorageError, createFileNotFoundError } from '../../document-storage.errors';
 import { defineStorageDriver } from '../drivers.models';
 
 export const IN_MEMORY_STORAGE_DRIVER_NAME = 'in-memory' as const;
@@ -8,14 +8,16 @@ export const IN_MEMORY_STORAGE_DRIVER_NAME = 'in-memory' as const;
 export const inMemoryStorageDriverFactory = defineStorageDriver(() => {
   const storage: Map<string, { content: Buffer; mimeType: string; fileName: string }> = new Map();
 
-  const fileExists = async ({ storageKey }: { storageKey: string }) => {
-    return storage.has(storageKey);
-  };
+  const fileExists = ({ storageKey }: { storageKey: string }) => storage.has(storageKey);
 
   return {
     name: IN_MEMORY_STORAGE_DRIVER_NAME,
 
     saveFile: async ({ fileStream, storageKey, mimeType, fileName }) => {
+      if (fileExists({ storageKey })) {
+        throw createFileAlreadyExistsInStorageError();
+      }
+
       const content = await collectReadableStreamToBuffer({ stream: fileStream });
 
       storage.set(storageKey, { content, mimeType, fileName });
@@ -36,16 +38,14 @@ export const inMemoryStorageDriverFactory = defineStorageDriver(() => {
     },
 
     deleteFile: async ({ storageKey }) => {
-      const exists = await fileExists({ storageKey });
-
-      if (!exists) {
+      if (!fileExists({ storageKey })) {
         throw createFileNotFoundError();
       }
 
       storage.delete(storageKey);
     },
 
-    fileExists,
+    fileExists: async ({ storageKey }) => fileExists({ storageKey }),
 
     _getStorage: () => storage,
   };

apps/papra-server/src/modules/documents/storage/drivers/s3/s3.storage-driver.ts

@@ -4,7 +4,7 @@ import { DeleteObjectCommand, GetObjectCommand, HeadObjectCommand, S3Client } fr
 import { Upload } from '@aws-sdk/lib-storage';
 import { safely } from '@corentinth/chisels';
 import { isString } from '../../../../shared/utils';
-import { createFileNotFoundError } from '../../document-storage.errors';
+import { createFileAlreadyExistsInStorageError, createFileNotFoundError } from '../../document-storage.errors';
 import { defineStorageDriver } from '../drivers.models';
 
 export const S3_STORAGE_DRIVER_NAME = 's3' as const;
@@ -50,12 +50,18 @@ export const s3StorageDriverFactory = defineStorageDriver(({ documentStorageConf
     name: S3_STORAGE_DRIVER_NAME,
     getClient: () => s3Client,
     saveFile: async ({ fileStream, storageKey }) => {
+      if (await fileExists({ storageKey })) {
+        // Not very atomic, TOCTOU issue here, but from some tests, If-None-Match header with '*' doesn't seem to work reliably with Upload
+        throw createFileAlreadyExistsInStorageError();
+      }
+
       const upload = new Upload({
         client: s3Client,
         params: {
           Bucket: bucketName,
           Key: storageKey,
           Body: fileStream,
+          IfNoneMatch: '*',
         },
       });
 

apps/papra-server/src/modules/shared/fs/fs.models.test.ts

@@ -0,0 +1,17 @@
+import { castError } from '@corentinth/chisels';
+import { describe, expect, test } from 'vitest';
+import { isFileAlreadyExistsError } from './fs.models';
+
+describe('fs models', () => {
+  describe('isFileAlreadyExistsError', () => {
+    test('errors about file already existing have either code "EEXIST" or "ERROR_FILE_EXISTS" or an errno of -17', () => {
+      expect(isFileAlreadyExistsError({ error: castError({ code: 'EEXIST' }) })).toBe(true);
+      expect(isFileAlreadyExistsError({ error: castError({ code: 'ERROR_FILE_EXISTS' }) })).toBe(true);
+      expect(isFileAlreadyExistsError({ error: castError({ errno: -17 }) })).toBe(true);
+
+      expect(isFileAlreadyExistsError({ error: castError({ code: 'ENOENT' }) })).toBe(false);
+      expect(isFileAlreadyExistsError({ error: castError({ errno: -2 }) })).toBe(false);
+      expect(isFileAlreadyExistsError({ error: castError({}) })).toBe(false);
+    });
+  });
+});

apps/papra-server/src/modules/shared/fs/fs.models.ts

@@ -10,3 +10,19 @@ export function isCrossDeviceError({ error }: { error: Error & { code?: unknown
     'ERROR_NOT_SAME_DEVICE', // Windows
   ].includes(error.code);
 }
+
+export function isFileAlreadyExistsError({ error }: { error: Error & { code?: unknown; errno?: unknown } }) {
+  if (
+    'code' in error
+    && isString(error.code)
+    && ['EEXIST', 'ERROR_FILE_EXISTS'].includes(error.code)
+  ) {
+    return true;
+  }
+
+  if (isNil(error.errno)) {
+    return false;
+  }
+
+  return error.errno === -17;
+}