From 1c8d4a651985ac3451c60528d437552c55bc5eaf Mon Sep 17 00:00:00 2001 From: Mike Patnode Date: Sat, 26 Oct 2019 19:15:21 -0700 Subject: [PATCH] Move filename validation out of the Router and into the FilesAdaptor (#6157) * Move filename validation out of the Router and into the FilesAdaptor * Address PR comments * Update unittests to handle FilesAdapter interface change * Make validateFilename optional --- spec/AdaptableController.spec.js | 2 ++ spec/FilesController.spec.js | 21 +++++++++++ src/Adapters/Files/FilesAdapter.js | 44 +++++++++++++++++++++++ src/Adapters/Files/GridFSBucketAdapter.js | 6 +++- src/Adapters/Files/GridStoreAdapter.js | 6 +++- src/Controllers/FilesController.js | 9 ++++- src/Routers/FilesRouter.js | 27 +++++--------- 7 files changed, 93 insertions(+), 22 deletions(-) diff --git a/spec/AdaptableController.spec.js b/spec/AdaptableController.spec.js index ef3b891e..d0bfb164 100644 --- a/spec/AdaptableController.spec.js +++ b/spec/AdaptableController.spec.js @@ -64,6 +64,7 @@ describe('AdaptableController', () => { deleteFile: function() {}, getFileData: function() {}, getFileLocation: function() {}, + validateFilename: function() {}, }; expect(() => { new FilesController(adapter); @@ -77,6 +78,7 @@ describe('AdaptableController', () => { AGoodAdapter.prototype.deleteFile = function() {}; AGoodAdapter.prototype.getFileData = function() {}; AGoodAdapter.prototype.getFileLocation = function() {}; + AGoodAdapter.prototype.validateFilename = function() {}; const adapter = new AGoodAdapter(); expect(() => { diff --git a/spec/FilesController.spec.js b/spec/FilesController.spec.js index ad3a40e7..2ad6f861 100644 --- a/spec/FilesController.spec.js +++ b/spec/FilesController.spec.js @@ -4,6 +4,8 @@ const WinstonLoggerAdapter = require('../lib/Adapters/Logger/WinstonLoggerAdapte .WinstonLoggerAdapter; const GridFSBucketAdapter = require('../lib/Adapters/Files/GridFSBucketAdapter') .GridFSBucketAdapter; +const GridStoreAdapter = require('../lib/Adapters/Files/GridStoreAdapter') + .GridStoreAdapter; const Config = require('../lib/Config'); const FilesController = require('../lib/Controllers/FilesController').default; @@ -14,6 +16,7 @@ const mockAdapter = { deleteFile: () => {}, getFileData: () => {}, getFileLocation: () => 'xyz', + validateFilename: () => {}, }; // Small additional tests to improve overall coverage @@ -118,4 +121,22 @@ describe('FilesController', () => { done(); }); + + it('should reject slashes in file names', done => { + const gridStoreAdapter = new GridFSBucketAdapter( + 'mongodb://localhost:27017/parse' + ); + const fileName = 'foo/randomFileName.pdf'; + expect(gridStoreAdapter.validateFilename(fileName)).not.toBe(null); + done(); + }); + + it('should also reject slashes in file names', done => { + const gridStoreAdapter = new GridStoreAdapter( + 'mongodb://localhost:27017/parse' + ); + const fileName = 'foo/randomFileName.pdf'; + expect(gridStoreAdapter.validateFilename(fileName)).not.toBe(null); + done(); + }); }); diff --git a/src/Adapters/Files/FilesAdapter.js b/src/Adapters/Files/FilesAdapter.js index db02174c..8c92454c 100644 --- a/src/Adapters/Files/FilesAdapter.js +++ b/src/Adapters/Files/FilesAdapter.js @@ -8,12 +8,16 @@ // * deleteFile(filename) // * getFileData(filename) // * getFileLocation(config, filename) +// Adapter classes should implement the following functions: +// * validateFilename(filename) +// * handleFileStream(filename, req, res, contentType) // // Default is GridFSBucketAdapter, which requires mongo // and for the API server to be using the DatabaseController with Mongo // database adapter. import type { Config } from '../../Config'; +import Parse from 'parse/node'; /** * @module Adapters */ @@ -56,6 +60,46 @@ export class FilesAdapter { * @return {string} Absolute URL */ getFileLocation(config: Config, filename: string): string {} + + /** Validate a filename for this adapter type + * + * @param {string} filename + * + * @returns {null|Parse.Error} null if there are no errors + */ + // validateFilename(filename: string): ?Parse.Error {} + + /** Handles Byte-Range Requests for Streaming + * + * @param {string} filename + * @param {object} req + * @param {object} res + * @param {string} contentType + * + * @returns {Promise} Data for byte range + */ + // handleFileStream(filename: string, res: any, req: any, contentType: string): Promise +} + +/** + * Simple filename validation + * + * @param filename + * @returns {null|Parse.Error} + */ +export function validateFilename(filename): ?Parse.Error { + if (filename.length > 128) { + return new Parse.Error(Parse.Error.INVALID_FILE_NAME, 'Filename too long.'); + } + + const regx = /^[_a-zA-Z0-9][a-zA-Z0-9@. ~_-]*$/; + if (!filename.match(regx)) { + return new Parse.Error( + Parse.Error.INVALID_FILE_NAME, + 'Filename contains invalid characters.' + ); + } + return null; } export default FilesAdapter; diff --git a/src/Adapters/Files/GridFSBucketAdapter.js b/src/Adapters/Files/GridFSBucketAdapter.js index bc1b4ae9..28b238ef 100644 --- a/src/Adapters/Files/GridFSBucketAdapter.js +++ b/src/Adapters/Files/GridFSBucketAdapter.js @@ -8,7 +8,7 @@ // @flow-disable-next import { MongoClient, GridFSBucket, Db } from 'mongodb'; -import { FilesAdapter } from './FilesAdapter'; +import { FilesAdapter, validateFilename } from './FilesAdapter'; import defaults from '../../defaults'; export class GridFSBucketAdapter extends FilesAdapter { @@ -139,6 +139,10 @@ export class GridFSBucketAdapter extends FilesAdapter { } return this._client.close(false); } + + validateFilename(filename) { + return validateFilename(filename); + } } export default GridFSBucketAdapter; diff --git a/src/Adapters/Files/GridStoreAdapter.js b/src/Adapters/Files/GridStoreAdapter.js index 3eb50ee5..3d6bba10 100644 --- a/src/Adapters/Files/GridStoreAdapter.js +++ b/src/Adapters/Files/GridStoreAdapter.js @@ -9,7 +9,7 @@ // @flow-disable-next import { MongoClient, GridStore, Db } from 'mongodb'; -import { FilesAdapter } from './FilesAdapter'; +import { FilesAdapter, validateFilename } from './FilesAdapter'; import defaults from '../../defaults'; export class GridStoreAdapter extends FilesAdapter { @@ -110,6 +110,10 @@ export class GridStoreAdapter extends FilesAdapter { } return this._client.close(false); } + + validateFilename(filename) { + return validateFilename(filename); + } } // handleRangeRequest is licensed under Creative Commons Attribution 4.0 International License (https://creativecommons.org/licenses/by/4.0/). diff --git a/src/Controllers/FilesController.js b/src/Controllers/FilesController.js index 579e3e43..4d2b60ed 100644 --- a/src/Controllers/FilesController.js +++ b/src/Controllers/FilesController.js @@ -1,7 +1,7 @@ // FilesController.js import { randomHexString } from '../cryptoUtils'; import AdaptableController from './AdaptableController'; -import { FilesAdapter } from '../Adapters/Files/FilesAdapter'; +import { validateFilename, FilesAdapter } from '../Adapters/Files/FilesAdapter'; import path from 'path'; import mime from 'mime'; @@ -95,6 +95,13 @@ export class FilesController extends AdaptableController { handleFileStream(config, filename, req, res, contentType) { return this.adapter.handleFileStream(filename, req, res, contentType); } + + validateFilename(filename) { + if (typeof this.adapter.validateFilename === 'function') { + return this.adapter.validateFilename(filename); + } + return validateFilename(filename); + } } export default FilesController; diff --git a/src/Routers/FilesRouter.js b/src/Routers/FilesRouter.js index 72d032af..56fb5a3c 100644 --- a/src/Routers/FilesRouter.js +++ b/src/Routers/FilesRouter.js @@ -69,6 +69,11 @@ export class FilesRouter { } createHandler(req, res, next) { + const config = req.config; + const filesController = config.filesController; + const filename = req.params.filename; + const contentType = req.get('Content-type'); + if (!req.body || !req.body.length) { next( new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Invalid file upload.') @@ -76,28 +81,12 @@ export class FilesRouter { return; } - if (req.params.filename.length > 128) { - next( - new Parse.Error(Parse.Error.INVALID_FILE_NAME, 'Filename too long.') - ); + const error = filesController.validateFilename(filename); + if (error) { + next(error); return; } - if (!req.params.filename.match(/^[_a-zA-Z0-9][a-zA-Z0-9@\.\ ~_-]*$/)) { - next( - new Parse.Error( - Parse.Error.INVALID_FILE_NAME, - 'Filename contains invalid characters.' - ) - ); - return; - } - - const filename = req.params.filename; - const contentType = req.get('Content-type'); - const config = req.config; - const filesController = config.filesController; - filesController .createFile(config, filename, req.body, contentType) .then(result => {