diff --git a/services/filestore/app/js/GcsPersistor.js b/services/filestore/app/js/GcsPersistor.js index a2525361c2..3a314b50c1 100644 --- a/services/filestore/app/js/GcsPersistor.js +++ b/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 } diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee index 0fe98effb1..7bb37db9de 100644 --- a/services/filestore/config/settings.defaults.coffee +++ b/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']? diff --git a/services/filestore/package-lock.json b/services/filestore/package-lock.json index 1d74c5d172..e858a9e0be 100644 --- a/services/filestore/package-lock.json +++ b/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", diff --git a/services/filestore/package.json b/services/filestore/package.json index 6f7d84c778..bbdd586f58 100644 --- a/services/filestore/package.json +++ b/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", diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js index 599d60155a..9da46db092 100644 --- a/services/filestore/test/acceptance/js/FilestoreTests.js +++ b/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) diff --git a/services/filestore/test/acceptance/js/TestConfig.js b/services/filestore/test/acceptance/js/TestConfig.js index fd7d0f034c..833a3b09be 100644 --- a/services/filestore/test/acceptance/js/TestConfig.js +++ b/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}') } } diff --git a/services/filestore/test/unit/js/GcsPersistorTests.js b/services/filestore/test/unit/js/GcsPersistorTests.js index a63296a18f..0e5f77bdf1 100644 --- a/services/filestore/test/unit/js/GcsPersistorTests.js +++ b/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() {