Validate key names when deleting directory with a configurable regex

services/filestore/app/js/GcsPersistor.js

@@ -191,7 +191,7 @@ async function deleteFile(bucketName, key) {
 }
 
 async function deleteDirectory(bucketName, key) {
-  if (!key.match(/^[a-z0-9_-]+/i)) {
+  if (!key.match(settings.filestore.gcs.directoryKeyRegex)) {
     throw new NotFoundError({
       message: 'deleteDirectoryKey is invalid or missing',
       info: { bucketName, key }

services/filestore/config/settings.defaults.coffee

@@ -39,6 +39,8 @@ 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}")
 
 		s3:
 			if process.env['AWS_ACCESS_KEY_ID']? or process.env['S3_BUCKET_CREDENTIALS']?

services/filestore/package-lock.json

@@ -1415,6 +1415,16 @@
       "resolved": "https://registry.npmjs.org/bintrees/-/bintrees-1.0.1.tgz",
       "integrity": "sha512-tbaUB1QpTIj4cKY8c1rvNAvEQXA+ekzHmbe4jzNfW3QWsF9GnnP/BRWyl6/qqS53heoYJ93naaFcm/jooONH8g=="
     },
+    "bl": {
+      "version": "2.2.0",
+      "resolved": "https://registry.npmjs.org/bl/-/bl-2.2.0.tgz",
+      "integrity": "sha512-wbgvOpqopSr7uq6fJrLH8EsvYMJf9gzfo2jCsL2eTy75qXPukA4pCgHamOQkZtY5vmfVtjB+P3LNlMHW5CEZXA==",
+      "dev": true,
+      "requires": {
+        "readable-stream": "^2.3.5",
+        "safe-buffer": "^5.1.1"
+      }
+    },
     "body-parser": {
       "version": "1.19.0",
       "resolved": "https://registry.npmjs.org/body-parser/-/body-parser-1.19.0.tgz",
@@ -1453,6 +1463,12 @@
       "integrity": "sha512-qhAVI1+Av2X7qelOfAIYwXONood6XlZE/fXaBSmW/T5SzLAmCgzi+eiWE7fUvbHaeNBQH13UftjpXxsfLkMpgw==",
       "dev": true
     },
+    "bson": {
+      "version": "1.1.3",
+      "resolved": "https://registry.npmjs.org/bson/-/bson-1.1.3.tgz",
+      "integrity": "sha512-TdiJxMVnodVS7r0BdL42y/pqC9cL2iKynVwA0Ho3qbsQYr428veL3l7BQyuqiw+Q5SqqoT0m4srSY/BlZ9AxXg==",
+      "dev": true
+    },
     "buffer": {
       "version": "4.9.1",
       "resolved": "https://registry.npmjs.org/buffer/-/buffer-4.9.1.tgz",
@@ -1854,6 +1870,12 @@
       "resolved": "https://registry.npmjs.org/delayed-stream/-/delayed-stream-1.0.0.tgz",
       "integrity": "sha512-ZySD7Nf91aLB0RxL4KGrKHBXl7Eds1DAmEdcoVawXnLD7SDhpNgtuII2aAkg7a7QS41jxPSZ17p4VdGnMHk3MQ=="
     },
+    "denque": {
+      "version": "1.4.1",
+      "resolved": "https://registry.npmjs.org/denque/-/denque-1.4.1.tgz",
+      "integrity": "sha512-OfzPuSZKGcgr96rf1oODnfjqBFmr1DVoc/TrItj3Ohe0Ah1C5WX5Baquw/9U9KovnQ88EqmJbD66rKYUQYN1tQ==",
+      "dev": true
+    },
     "depd": {
       "version": "1.1.2",
       "resolved": "https://registry.npmjs.org/depd/-/depd-1.1.2.tgz",
@@ -3501,6 +3523,13 @@
       "resolved": "https://registry.npmjs.org/media-typer/-/media-typer-0.3.0.tgz",
       "integrity": "sha512-dq+qelQ9akHpcOl/gUVRTxVIOkAJ1wR3QAvb4RsVjS8oVoFjDGTc679wJYmUmknUF5HwMLOgb5O+a3KxfWapPQ=="
     },
+    "memory-pager": {
+      "version": "1.5.0",
+      "resolved": "https://registry.npmjs.org/memory-pager/-/memory-pager-1.5.0.tgz",
+      "integrity": "sha512-ZS4Bp4r/Zoeq6+NLJpP+0Zzm0pR8whtGPf1XExKLJBAczGMnSi3It14OiNCStjQjM6NU1okjQGSxgEZN8eBYKg==",
+      "dev": true,
+      "optional": true
+    },
     "merge-descriptors": {
       "version": "1.0.1",
       "resolved": "https://registry.npmjs.org/merge-descriptors/-/merge-descriptors-1.0.1.tgz",
@@ -3653,6 +3682,20 @@
       "integrity": "sha512-bV7f+6l2QigeBBZSM/6yTNq4P2fNpSWj/0e7jQcy87A8e7o2nAfP/34/2ky5Vw4B9S446EtIhodAzkFCcR4dQg==",
       "optional": true
     },
+    "mongodb": {
+      "version": "3.5.4",
+      "resolved": "https://registry.npmjs.org/mongodb/-/mongodb-3.5.4.tgz",
+      "integrity": "sha512-xGH41Ig4dkSH5ROGezkgDbsgt/v5zbNUwE3TcFsSbDc6Qn3Qil17dhLsESSDDPTiyFDCPJRpfd4887dtsPgKtA==",
+      "dev": true,
+      "requires": {
+        "bl": "^2.2.0",
+        "bson": "^1.1.1",
+        "denque": "^1.4.1",
+        "require_optional": "^1.0.1",
+        "safe-buffer": "^5.1.2",
+        "saslprep": "^1.0.0"
+      }
+    },
     "ms": {
       "version": "2.0.0",
       "resolved": "https://registry.npmjs.org/ms/-/ms-2.0.0.tgz",
@@ -4939,6 +4982,30 @@
       "integrity": "sha512-AKGr4qvHiryxRb19m3PsLRGuKVAbJLUD7E6eOaHkfKhwc+vSgVOCY5xNvm9EkolBKTOf0GrQAZKLimOCz81Khg==",
       "dev": true
     },
+    "require_optional": {
+      "version": "1.0.1",
+      "resolved": "https://registry.npmjs.org/require_optional/-/require_optional-1.0.1.tgz",
+      "integrity": "sha512-qhM/y57enGWHAe3v/NcwML6a3/vfESLe/sGM2dII+gEO0BpKRUkWZow/tyloNqJyN6kXSl3RyyM8Ll5D/sJP8g==",
+      "dev": true,
+      "requires": {
+        "resolve-from": "^2.0.0",
+        "semver": "^5.1.0"
+      },
+      "dependencies": {
+        "resolve-from": {
+          "version": "2.0.0",
+          "resolved": "https://registry.npmjs.org/resolve-from/-/resolve-from-2.0.0.tgz",
+          "integrity": "sha1-lICrIOlP+h2egKgEx+oUdhGWa1c=",
+          "dev": true
+        },
+        "semver": {
+          "version": "5.7.1",
+          "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz",
+          "integrity": "sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ==",
+          "dev": true
+        }
+      }
+    },
     "resolve": {
       "version": "1.15.1",
       "resolved": "https://registry.npmjs.org/resolve/-/resolve-1.15.1.tgz",
@@ -5041,6 +5108,16 @@
         "stack-trace": "0.0.9"
       }
     },
+    "saslprep": {
+      "version": "1.0.3",
+      "resolved": "https://registry.npmjs.org/saslprep/-/saslprep-1.0.3.tgz",
+      "integrity": "sha512-/MY/PEMbk2SuY5sScONwhUDsV2p77Znkb/q3nSVstq/yQzYJOH/Azh29p9oJLsl3LnQwSvZDKagDGBsBwSooag==",
+      "dev": true,
+      "optional": true,
+      "requires": {
+        "sparse-bitfield": "^3.0.3"
+      }
+    },
     "sax": {
       "version": "1.2.1",
       "resolved": "https://registry.npmjs.org/sax/-/sax-1.2.1.tgz",
@@ -5205,6 +5282,16 @@
       "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.6.1.tgz",
       "integrity": "sha512-UjgapumWlbMhkBgzT7Ykc5YXUT46F0iKu8SGXq0bcwP5dz/h0Plj6enJqjz1Zbq2l5WaqYnrVbwWOWMyF3F47g=="
     },
+    "sparse-bitfield": {
+      "version": "3.0.3",
+      "resolved": "https://registry.npmjs.org/sparse-bitfield/-/sparse-bitfield-3.0.3.tgz",
+      "integrity": "sha1-/0rm5oZWBWuks+eSqzM004JzyhE=",
+      "dev": true,
+      "optional": true,
+      "requires": {
+        "memory-pager": "^1.0.2"
+      }
+    },
     "spdx-correct": {
       "version": "3.1.0",
       "resolved": "https://registry.npmjs.org/spdx-correct/-/spdx-correct-3.1.0.tgz",

services/filestore/package.json

@@ -56,6 +56,7 @@
     "eslint-plugin-promise": "^4.2.1",
     "eslint-plugin-standard": "^4.0.1",
     "mocha": "5.2.0",
+    "mongodb": "^3.5.4",
     "prettier-eslint": "^9.0.1",
     "prettier-eslint-cli": "^5.0.0",
     "sandboxed-module": "2.0.3",

services/filestore/test/acceptance/js/FilestoreTests.js

@@ -14,6 +14,7 @@ const { promisify } = require('util')
 const { Storage } = require('@google-cloud/storage')
 const streamifier = require('streamifier')
 chai.use(require('chai-as-promised'))
+const { ObjectId } = require('mongodb')
 
 const fsWriteFile = promisify(fs.writeFile)
 const fsStat = promisify(fs.stat)
@@ -48,7 +49,6 @@ const BackendSettings = require('./TestConfig')
 describe('Filestore', function() {
   this.timeout(1000 * 10)
   const filestoreUrl = `http://localhost:${Settings.internal.filestore.port}`
-  const directoryName = 'directory'
 
   // redefine the test suite for every available backend
   Object.keys(BackendSettings).forEach(backend => {
@@ -84,7 +84,7 @@ describe('Filestore', function() {
             `${metricPrefix}_egress`
           )
         }
-        projectId = `acceptance_tests_${Math.random()}`
+        projectId = ObjectId().toString()
       })
 
       it('should send a 200 for the status endpoint', async function() {
@@ -107,8 +107,8 @@ describe('Filestore', function() {
           '/tmp/filestore_acceptance_tests_file_read.txt'
 
         beforeEach(async function() {
-          fileId = Math.random()
-          fileUrl = `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileId}`
+          fileId = ObjectId().toString()
+          fileUrl = `${filestoreUrl}/project/${projectId}/file/${fileId}`
           constantFileContent = [
             'hello world',
             `line 2 goes here ${Math.random()}`,
@@ -188,16 +188,16 @@ describe('Filestore', function() {
         })
 
         it('should be able to copy files', async function() {
-          const newProjectID = `acceptance_tests_copied_project_${Math.random()}`
-          const newFileId = Math.random()
-          const newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${directoryName}%2F${newFileId}`
+          const newProjectID = ObjectId().toString()
+          const newFileId = ObjectId().toString()
+          const newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${newFileId}`
           const opts = {
             method: 'put',
             uri: newFileUrl,
             json: {
               source: {
                 project_id: projectId,
-                file_id: `${directoryName}/${fileId}`
+                file_id: fileId
               }
             }
           }
@@ -260,7 +260,6 @@ describe('Filestore', function() {
 
       describe('with multiple files', function() {
         let fileIds, fileUrls
-        const directoryName = 'directory'
         const localFileReadPaths = [
           '/tmp/filestore_acceptance_tests_file_read_1.txt',
           '/tmp/filestore_acceptance_tests_file_read_2.txt'
@@ -286,10 +285,10 @@ describe('Filestore', function() {
         })
 
         beforeEach(async function() {
-          fileIds = [Math.random(), Math.random()]
+          fileIds = [ObjectId().toString(), ObjectId().toString()]
           fileUrls = [
-            `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileIds[0]}`,
-            `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileIds[1]}`
+            `${filestoreUrl}/project/${projectId}/file/${fileIds[0]}`,
+            `${filestoreUrl}/project/${projectId}/file/${fileIds[1]}`
           ]
 
           const writeStreams = [
@@ -325,8 +324,8 @@ describe('Filestore', function() {
         let fileId, fileUrl, largeFileContent, error
 
         beforeEach(async function() {
-          fileId = Math.random()
-          fileUrl = `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileId}`
+          fileId = ObjectId().toString()
+          fileUrl = `${filestoreUrl}/project/${projectId}/file/${fileId}`
 
           largeFileContent = '_wombat_'.repeat(1024 * 1024) // 8 megabytes
           largeFileContent += Math.random()
@@ -359,8 +358,8 @@ describe('Filestore', function() {
 
           beforeEach(async function() {
             constantFileContent = `This is a file in a different S3 bucket ${Math.random()}`
-            fileId = Math.random().toString()
-            bucketName = Math.random().toString()
+            fileId = ObjectId().toString()
+            bucketName = ObjectId().toString()
             fileUrl = `${filestoreUrl}/bucket/${bucketName}/key/${fileId}`
 
             const s3ClientSettings = {
@@ -448,9 +447,9 @@ describe('Filestore', function() {
 
           beforeEach(function() {
             constantFileContent = `This is yet more file content ${Math.random()}`
-            fileId = Math.random().toString()
-            fileKey = `${projectId}/${directoryName}/${fileId}`
-            fileUrl = `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileId}`
+            fileId = ObjectId().toString()
+            fileKey = `${projectId}/${fileId}`
+            fileUrl = `${filestoreUrl}/project/${projectId}/file/${fileId}`
 
             bucket = Settings.filestore.stores.user_files
             fallbackBucket = Settings.filestore.fallback.buckets[bucket]
@@ -532,10 +531,10 @@ describe('Filestore', function() {
               let newFileId, newFileUrl, newFileKey, opts
 
               beforeEach(function() {
-                const newProjectID = `acceptance_tests_copied_project_${Math.random()}`
-                newFileId = Math.random()
-                newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${directoryName}%2F${newFileId}`
-                newFileKey = `${newProjectID}/${directoryName}/${newFileId}`
+                const newProjectID = ObjectId().toString()
+                newFileId = ObjectId().toString()
+                newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${newFileId}`
+                newFileKey = `${newProjectID}/${newFileId}`
 
                 opts = {
                   method: 'put',
@@ -543,7 +542,7 @@ describe('Filestore', function() {
                   json: {
                     source: {
                       project_id: projectId,
-                      file_id: `${directoryName}/${fileId}`
+                      file_id: fileId
                     }
                   }
                 }
@@ -668,7 +667,7 @@ describe('Filestore', function() {
               await expectPersistorNotToHaveFile(
                 app.persistor.fallbackPersistor,
                 fallbackBucket,
-                `${projectId}/${directoryName}/${fileId}`
+                `${projectId}/${fileId}`
               )
             })
           })
@@ -757,8 +756,8 @@ describe('Filestore', function() {
         )
 
         beforeEach(async function() {
-          fileId = Math.random()
-          fileUrl = `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileId}`
+          fileId = ObjectId().toString()
+          fileUrl = `${filestoreUrl}/project/${projectId}/file/${fileId}`
           const stat = await fsStat(localFileReadPath)
           localFileSize = stat.size
           const writeStream = request.post(fileUrl)

services/filestore/test/acceptance/js/TestConfig.js

@@ -23,7 +23,8 @@ function gcsConfig() {
   return {
     apiEndpoint: process.env.GCS_API_ENDPOINT,
     apiScheme: process.env.GCS_API_SCHEME,
-    projectId: 'fake'
+    projectId: 'fake',
+    directoryKeyRegex: new RegExp('^[0-9a-fA-F]{24}/[0-9a-fA-F]{24}')
   }
 }
 

services/filestore/test/unit/js/GcsPersistorTests.js

@@ -3,6 +3,7 @@ const chai = require('chai')
 const { expect } = chai
 const modulePath = '../../../app/js/GcsPersistor.js'
 const SandboxedModule = require('sandboxed-module')
+const { ObjectId } = require('mongodb')
 
 const Errors = require('../../../app/js/Errors')
 
@@ -41,6 +42,9 @@ describe('GcsPersistorTests', function() {
         backend: 'gcs',
         stores: {
           user_files: 'user_files'
+        },
+        gcs: {
+          directoryKeyRegex: /^[0-9a-fA-F]{24}\/[0-9a-fA-F]{24}/
         }
       }
     }
@@ -512,15 +516,17 @@ describe('GcsPersistorTests', function() {
   })
 
   describe('deleteDirectory', function() {
+    const directoryName = `${ObjectId()}/${ObjectId()}`
     describe('with valid parameters', function() {
       beforeEach(async function() {
-        return GcsPersistor.promises.deleteDirectory(bucket, key)
+        console.log(key)
+        return GcsPersistor.promises.deleteDirectory(bucket, directoryName)
       })
 
       it('should delete the objects in the directory', function() {
         expect(Storage.prototype.bucket).to.have.been.calledWith(bucket)
         expect(GcsBucket.deleteFiles).to.have.been.calledWith({
-          directory: key,
+          directory: directoryName,
           force: true
         })
       })
@@ -532,7 +538,7 @@ describe('GcsPersistorTests', function() {
       beforeEach(async function() {
         GcsBucket.deleteFiles = sinon.stub().rejects(genericError)
         try {
-          await GcsPersistor.promises.deleteDirectory(bucket, key)
+          await GcsPersistor.promises.deleteDirectory(bucket, directoryName)
         } catch (err) {
           error = err
         }
@@ -546,6 +552,22 @@ 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() {