From fc80aa3954e6823c7970adf63e4bced6594cf97f Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Sat, 14 Mar 2020 14:31:30 +0000 Subject: [PATCH] Move directory key validation into FileHandler --- services/filestore/app/js/FileHandler.js | 14 ++++++++++++- services/filestore/app/js/GcsPersistor.js | 7 ------- .../filestore/config/settings.defaults.coffee | 2 -- .../test/unit/js/FileHandlerTests.js | 21 +++++++++++++++++-- .../test/unit/js/GcsPersistorTests.js | 16 -------------- 5 files changed, 32 insertions(+), 28 deletions(-) diff --git a/services/filestore/app/js/FileHandler.js b/services/filestore/app/js/FileHandler.js index 02831fa3d0..9b592df34e 100644 --- a/services/filestore/app/js/FileHandler.js +++ b/services/filestore/app/js/FileHandler.js @@ -5,7 +5,7 @@ const LocalFileWriter = require('./LocalFileWriter') const FileConverter = require('./FileConverter') const KeyBuilder = require('./KeyBuilder') const ImageOptimiser = require('./ImageOptimiser') -const { ConversionError } = require('./Errors') +const { ConversionError, WriteError } = require('./Errors') module.exports = { insertFile: callbackify(insertFile), @@ -24,12 +24,24 @@ module.exports = { async function insertFile(bucket, key, stream) { const convertedKey = KeyBuilder.getConvertedFolderKey(key) + if (!convertedKey.match(/^[0-9a-f]{24}\/[0-9a-f]{24}/i)) { + throw new WriteError({ + message: 'key does not match validation regex', + info: { bucket, key, convertedKey } + }) + } await PersistorManager.promises.deleteDirectory(bucket, convertedKey) await PersistorManager.promises.sendStream(bucket, key, stream) } async function deleteFile(bucket, key) { const convertedKey = KeyBuilder.getConvertedFolderKey(key) + if (!convertedKey.match(/^[0-9a-f]{24}\/[0-9a-f]{24}/i)) { + throw new WriteError({ + message: 'key does not match validation regex', + info: { bucket, key, convertedKey } + }) + } await Promise.all([ PersistorManager.promises.deleteFile(bucket, key), PersistorManager.promises.deleteDirectory(bucket, convertedKey) diff --git a/services/filestore/app/js/GcsPersistor.js b/services/filestore/app/js/GcsPersistor.js index 29286ef505..bc46153983 100644 --- a/services/filestore/app/js/GcsPersistor.js +++ b/services/filestore/app/js/GcsPersistor.js @@ -204,13 +204,6 @@ async function deleteFile(bucketName, key) { } async function deleteDirectory(bucketName, key) { - if (!key.match(settings.filestore.gcs.directoryKeyRegex)) { - throw new NotFoundError({ - message: 'deleteDirectoryKey is invalid or missing', - info: { bucketName, key } - }) - } - try { const [files] = await storage .bucket(bucketName) diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee index 24bce087ff..6b5238e552 100644 --- a/services/filestore/config/settings.defaults.coffee +++ b/services/filestore/config/settings.defaults.coffee @@ -40,8 +40,6 @@ settings = apiEndpoint: process.env['GCS_API_ENDPOINT'] apiScheme: process.env['GCS_API_SCHEME'] projectId: process.env['GCS_PROJECT_ID'] - # only keys that match this regex can be deleted - directoryKeyRegex: new RegExp(process.env['GCS_DIRECTORY_KEY_REGEX'] || "^[0-9a-fA-F]{24}/[0-9a-fA-F]{24}") unlockBeforeDelete: process.env['GCS_UNLOCK_BEFORE_DELETE'] == "true" # unlock an event-based hold before deleting. default false deletedBucketSuffix: process.env['GCS_DELETED_BUCKET_SUFFIX'] # if present, copy file to another bucket on delete. default null diff --git a/services/filestore/test/unit/js/FileHandlerTests.js b/services/filestore/test/unit/js/FileHandlerTests.js index 623ed440b0..9692521531 100644 --- a/services/filestore/test/unit/js/FileHandlerTests.js +++ b/services/filestore/test/unit/js/FileHandlerTests.js @@ -3,6 +3,7 @@ const chai = require('chai') const { expect } = chai const modulePath = '../../../app/js/FileHandler.js' const SandboxedModule = require('sandboxed-module') +const { ObjectId } = require('mongodb') chai.use(require('sinon-chai')) chai.use(require('chai-as-promised')) @@ -24,8 +25,8 @@ describe('FileHandler', function() { } const bucket = 'my_bucket' - const key = 'key/here' - const convertedFolderKey = 'convertedFolder' + const key = `${ObjectId()}/${ObjectId()}` + const convertedFolderKey = `${ObjectId()}/${ObjectId()}` const sourceStream = 'sourceStream' const convertedKey = 'convertedKey' const readStream = { @@ -112,6 +113,14 @@ describe('FileHandler', function() { done() }) }) + + it('should throw an error when the key is in the wrong format', function(done) { + KeyBuilder.getConvertedFolderKey.returns('wombat') + FileHandler.insertFile(bucket, key, stream, err => { + expect(err).to.exist + done() + }) + }) }) describe('deleteFile', function() { @@ -135,6 +144,14 @@ describe('FileHandler', function() { done() }) }) + + it('should throw an error when the key is in the wrong format', function(done) { + KeyBuilder.getConvertedFolderKey.returns('wombat') + FileHandler.deleteFile(bucket, key, err => { + expect(err).to.exist + done() + }) + }) }) describe('getFile', function() { diff --git a/services/filestore/test/unit/js/GcsPersistorTests.js b/services/filestore/test/unit/js/GcsPersistorTests.js index f02589c389..cd95bf1e20 100644 --- a/services/filestore/test/unit/js/GcsPersistorTests.js +++ b/services/filestore/test/unit/js/GcsPersistorTests.js @@ -556,22 +556,6 @@ describe('GcsPersistorTests', function() { expect(error.cause).to.equal(genericError) }) }) - - describe('when the directory name is in the wrong format', function() { - let error - - beforeEach(async function() { - try { - await GcsPersistor.promises.deleteDirectory(bucket, 'carbonara') - } catch (err) { - error = err - } - }) - - it('should throw a NotFoundError', function() { - expect(error).to.be.an.instanceOf(Errors.NotFoundError) - }) - }) }) describe('directorySize', function() {