From d720d6affaadeeac54b6d418b4bdf2be8e6b4dd2 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Mon, 17 Jan 2022 10:19:53 +0000 Subject: [PATCH] Merge pull request #6317 from overleaf/jpa-send-explicit-content-type [web] send explicit content type in responses GitOrigin-RevId: d5aeaba57a7d2fc053fbf5adc2299fb46e435341 --- services/web/Makefile | 4 + .../src/Features/Captcha/CaptchaMiddleware.js | 2 +- .../CollaboratorsInviteController.js | 2 +- .../src/Features/Compile/CompileController.js | 20 ++--- .../Features/Contacts/ContactController.js | 2 +- .../src/Features/Contacts/ContactRouter.js | 2 +- .../Features/Documents/DocumentController.js | 4 +- .../Downloads/ProjectDownloadsController.js | 14 ++-- .../src/Features/Errors/ErrorController.js | 5 +- .../src/Features/Errors/HttpErrorHandler.js | 15 ++-- .../Features/FileStore/FileStoreController.js | 5 +- .../HealthCheck/HealthCheckController.js | 7 +- .../src/Features/History/HistoryController.js | 6 +- .../InactiveData/InactiveProjectController.js | 2 +- .../LinkedFiles/LinkedFilesController.js | 82 +++++++++++-------- .../Notifications/NotificationsController.js | 2 +- .../src/Features/Project/ProjectController.js | 6 +- .../Features/Spelling/SpellingController.js | 4 +- .../Uploads/ProjectUploadController.js | 12 +-- .../UserMembershipController.js | 5 +- .../web/app/src/infrastructure/Response.js | 48 +++++++++++ services/web/app/src/router.js | 23 ++++-- services/web/bin/lint_flag_res_send_usage | 47 +++++++++++ services/web/package.json | 1 + .../test/acceptance/src/LinkedFilesTests.js | 7 +- .../test/acceptance/src/mocks/MockChatApi.js | 4 +- .../test/acceptance/src/mocks/MockClsiApi.js | 13 +-- .../acceptance/src/mocks/MockFilestoreApi.js | 5 +- .../src/mocks/MockProjectHistoryApi.js | 5 +- .../acceptance/src/mocks/MockRecurlyApi.js | 44 ++++++---- .../acceptance/src/mocks/MockV1HistoryApi.js | 21 +++-- .../AuthenticationControllerTests.js | 2 +- .../src/Compile/CompileControllerTests.js | 4 +- .../src/Contact/ContactControllerTests.js | 7 +- .../ProjectDownloadsControllerTests.js | 23 +++--- .../src/FileStore/FileStoreControllerTests.js | 32 +++----- .../NotificationsControllerTests.js | 2 +- .../src/Project/ProjectControllerTests.js | 6 +- .../References/ReferencesControllerTests.js | 1 - .../src/Spelling/SpellingControllerTests.js | 15 +--- .../Uploads/ProjectUploadControllerTests.js | 50 ++++++----- .../UserMembershipControllerTests.js | 7 +- .../web/test/unit/src/helpers/MockResponse.js | 46 +++++++++-- 43 files changed, 390 insertions(+), 224 deletions(-) create mode 100644 services/web/app/src/infrastructure/Response.js create mode 100755 services/web/bin/lint_flag_res_send_usage diff --git a/services/web/Makefile b/services/web/Makefile index ddcfb1538e..d22ae7d901 100644 --- a/services/web/Makefile +++ b/services/web/Makefile @@ -423,6 +423,10 @@ lint: lint_locales lint_locales: bin/lint_locales +lint: lint_flag_res_send_usage +lint_flag_res_send_usage: + bin/lint_flag_res_send_usage + lint_in_docker: $(RUN_LINT_FORMAT) make lint -j --output-sync diff --git a/services/web/app/src/Features/Captcha/CaptchaMiddleware.js b/services/web/app/src/Features/Captcha/CaptchaMiddleware.js index 47f97e4214..e082b24834 100644 --- a/services/web/app/src/Features/Captcha/CaptchaMiddleware.js +++ b/services/web/app/src/Features/Captcha/CaptchaMiddleware.js @@ -47,7 +47,7 @@ module.exports = CaptchaMiddleware = { { statusCode: response.statusCode, body }, 'failed recaptcha siteverify request' ) - return res.status(400).send({ + return res.status(400).json({ errorReason: 'cannot_verify_user_not_robot', message: { text: 'Sorry, we could not verify that you are not a robot. Please check that Google reCAPTCHA is not being blocked by an ad blocker or firewall.', diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js b/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js index e29bae96f5..cbc1083b0c 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js @@ -131,7 +131,7 @@ module.exports = CollaboratorsInviteController = { { projectId, email, sendingUserId }, 'invalid email address' ) - return res.status(400).send({ errorReason: 'invalid_email' }) + return res.status(400).json({ errorReason: 'invalid_email' }) } return CollaboratorsInviteController._checkRateLimit( sendingUserId, diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index dc2d3d48de..95a2707627 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -117,7 +117,7 @@ module.exports = CompileController = { if (error != null) { return next(error) } - return res.status(200).send() + return res.sendStatus(200) }) }, @@ -159,15 +159,12 @@ module.exports = CompileController = { if (error != null) { return next(error) } - res.contentType('application/json') - return res.status(200).send( - JSON.stringify({ - status, - outputFiles, - clsiServerId, - validationProblems, - }) - ) + return res.json({ + status, + outputFiles, + clsiServerId, + validationProblems, + }) } ) }, @@ -564,8 +561,7 @@ module.exports = CompileController = { if (error != null) { return next(error) } - res.contentType('application/json') - return res.send(body) + return res.json(body) } ) }) diff --git a/services/web/app/src/Features/Contacts/ContactController.js b/services/web/app/src/Features/Contacts/ContactController.js index 861f7329bb..0544f38e59 100644 --- a/services/web/app/src/Features/Contacts/ContactController.js +++ b/services/web/app/src/Features/Contacts/ContactController.js @@ -70,7 +70,7 @@ module.exports = ContactsController = { contacts = contacts.concat( ...Array.from(additional_contacts || []) ) - return res.send({ + return res.json({ contacts, }) } diff --git a/services/web/app/src/Features/Contacts/ContactRouter.js b/services/web/app/src/Features/Contacts/ContactRouter.js index 2c5236edb0..ca45498449 100644 --- a/services/web/app/src/Features/Contacts/ContactRouter.js +++ b/services/web/app/src/Features/Contacts/ContactRouter.js @@ -11,7 +11,7 @@ function contactsAuthenticationMiddleware() { if (SessionManager.isUserLoggedIn(req.session)) { next() } else { - res.send({ contacts: [] }) + res.json({ contacts: [] }) } } } diff --git a/services/web/app/src/Features/Documents/DocumentController.js b/services/web/app/src/Features/Documents/DocumentController.js index aa00235c49..069f139f91 100644 --- a/services/web/app/src/Features/Documents/DocumentController.js +++ b/services/web/app/src/Features/Documents/DocumentController.js @@ -5,6 +5,7 @@ const ProjectEntityHandler = require('../Project/ProjectEntityHandler') const ProjectEntityUpdateHandler = require('../Project/ProjectEntityUpdateHandler') const logger = require('@overleaf/logger') const _ = require('lodash') +const { plainTextResponse } = require('../../infrastructure/Response') function getDocument(req, res, next) { const { Project_id: projectId, doc_id: docId } = req.params @@ -47,8 +48,7 @@ function getDocument(req, res, next) { return next(error) } if (plain) { - res.type('text/plain') - res.send(lines.join('\n')) + plainTextResponse(res, lines.join('\n')) } else { const projectHistoryId = _.get(project, 'overleaf.history.id') const projectHistoryDisplay = _.get( diff --git a/services/web/app/src/Features/Downloads/ProjectDownloadsController.js b/services/web/app/src/Features/Downloads/ProjectDownloadsController.js index 74a7f5b437..387643bdc9 100644 --- a/services/web/app/src/Features/Downloads/ProjectDownloadsController.js +++ b/services/web/app/src/Features/Downloads/ProjectDownloadsController.js @@ -17,6 +17,7 @@ const Metrics = require('@overleaf/metrics') const ProjectGetter = require('../Project/ProjectGetter') const ProjectZipStreamManager = require('./ProjectZipStreamManager') const DocumentUpdaterHandler = require('../DocumentUpdater/DocumentUpdaterHandler') +const { prepareZipAttachment } = require('../../infrastructure/Response') module.exports = ProjectDownloadsController = { downloadProject(req, res, next) { @@ -41,10 +42,7 @@ module.exports = ProjectDownloadsController = { if (error != null) { return next(error) } - res.setContentDisposition('attachment', { - filename: `${project.name}.zip`, - }) - res.contentType('application/zip') + prepareZipAttachment(res, `${project.name}.zip`) return stream.pipe(res) } ) @@ -69,10 +67,10 @@ module.exports = ProjectDownloadsController = { if (error != null) { return next(error) } - res.setContentDisposition('attachment', { - filename: `Overleaf Projects (${project_ids.length} items).zip`, - }) - res.contentType('application/zip') + prepareZipAttachment( + res, + `Overleaf Projects (${project_ids.length} items).zip` + ) return stream.pipe(res) } ) diff --git a/services/web/app/src/Features/Errors/ErrorController.js b/services/web/app/src/Features/Errors/ErrorController.js index 338b526035..48e41253bf 100644 --- a/services/web/app/src/Features/Errors/ErrorController.js +++ b/services/web/app/src/Features/Errors/ErrorController.js @@ -4,6 +4,7 @@ const logger = require('@overleaf/logger') const SessionManager = require('../Authentication/SessionManager') const SamlLogHandler = require('../SamlLog/SamlLogHandler') const HttpErrorHandler = require('./HttpErrorHandler') +const { plainTextResponse } = require('../../infrastructure/Response') module.exports = ErrorController = { notFound(req, res) { @@ -62,11 +63,11 @@ module.exports = ErrorController = { } else if (error instanceof Errors.InvalidError) { logger.warn({ err: error, url: req.url }, 'invalid error') res.status(400) - res.send(error.message) + plainTextResponse(res, error.message) } else if (error instanceof Errors.InvalidNameError) { logger.warn({ err: error, url: req.url }, 'invalid name error') res.status(400) - res.send(error.message) + plainTextResponse(res, error.message) } else if (error instanceof Errors.SAMLSessionDataMissing) { logger.warn( { err: error, url: req.url }, diff --git a/services/web/app/src/Features/Errors/HttpErrorHandler.js b/services/web/app/src/Features/Errors/HttpErrorHandler.js index b706583111..6807d82f1f 100644 --- a/services/web/app/src/Features/Errors/HttpErrorHandler.js +++ b/services/web/app/src/Features/Errors/HttpErrorHandler.js @@ -1,5 +1,6 @@ const logger = require('@overleaf/logger') const Settings = require('@overleaf/settings') +const { plainTextResponse } = require('../../infrastructure/Response') function renderJSONError(res, message, info = {}) { if (info.message) { @@ -23,7 +24,7 @@ function handleGeneric500Error(req, res, statusCode, message) { case 'json': return renderJSONError(res, message) default: - return res.send('internal server error') + return plainTextResponse(res, 'internal server error') } } @@ -38,7 +39,7 @@ function handleGeneric400Error(req, res, statusCode, message, info = {}) { case 'json': return renderJSONError(res, message, info) default: - return res.send('client error') + return plainTextResponse(res, 'client error') } } @@ -90,7 +91,7 @@ module.exports = HttpErrorHandler = { case 'json': return renderJSONError(res, message, info) default: - return res.send('conflict') + return plainTextResponse(res, 'conflict') } }, @@ -102,7 +103,7 @@ module.exports = HttpErrorHandler = { case 'json': return renderJSONError(res, message, info) default: - return res.send('restricted') + return plainTextResponse(res, 'restricted') } }, @@ -114,7 +115,7 @@ module.exports = HttpErrorHandler = { case 'json': return renderJSONError(res, message, info) default: - return res.send('not found') + return plainTextResponse(res, 'not found') } }, @@ -129,7 +130,7 @@ module.exports = HttpErrorHandler = { case 'json': return renderJSONError(res, message, info) default: - return res.send('unprocessable entity') + return plainTextResponse(res, 'unprocessable entity') } }, @@ -155,7 +156,7 @@ module.exports = HttpErrorHandler = { case 'json': return renderJSONError(res, message, {}) default: - return res.send(message) + return plainTextResponse(res, message) } }, } diff --git a/services/web/app/src/Features/FileStore/FileStoreController.js b/services/web/app/src/Features/FileStore/FileStoreController.js index 7db347fae0..77e3df9cac 100644 --- a/services/web/app/src/Features/FileStore/FileStoreController.js +++ b/services/web/app/src/Features/FileStore/FileStoreController.js @@ -3,6 +3,7 @@ const logger = require('@overleaf/logger') const FileStoreHandler = require('./FileStoreHandler') const ProjectLocator = require('../Project/ProjectLocator') const Errors = require('../Errors/Errors') +const { preparePlainTextResponse } = require('../../infrastructure/Response') module.exports = { getFile(req, res) { @@ -34,7 +35,7 @@ module.exports = { } // mobile safari will try to render html files, prevent this if (isMobileSafari(userAgent) && isHtml(file)) { - res.setHeader('Content-Type', 'text/plain') + preparePlainTextResponse(res) } res.setContentDisposition('attachment', { filename: file.name }) stream.pipe(res) @@ -57,7 +58,7 @@ module.exports = { } return } - res.set('Content-Length', fileSize) + res.setHeader('Content-Length', fileSize) res.status(200).end() }) }, diff --git a/services/web/app/src/Features/HealthCheck/HealthCheckController.js b/services/web/app/src/Features/HealthCheck/HealthCheckController.js index 4f8a2eddcd..7f742d932e 100644 --- a/services/web/app/src/Features/HealthCheck/HealthCheckController.js +++ b/services/web/app/src/Features/HealthCheck/HealthCheckController.js @@ -12,7 +12,6 @@ module.exports = { check(req, res, next) { if (!settings.siteIsOpen || !settings.editorIsOpen) { // always return successful health checks when site is closed - res.contentType('application/json') res.sendStatus(200) } else { // detach from express for cleaner stack traces @@ -92,9 +91,6 @@ module.exports = { }, } -function prettyJSON(blob) { - return JSON.stringify(blob, null, 2) + '\n' -} async function runSmokeTestsDetached(req, res) { function isAborted() { return req.aborted @@ -120,6 +116,5 @@ async function runSmokeTestsDetached(req, res) { response = { stats, error: err.message } } if (isAborted()) return - res.contentType('application/json') - res.status(status).send(prettyJSON(response)) + res.status(status).json(response) } diff --git a/services/web/app/src/Features/History/HistoryController.js b/services/web/app/src/Features/History/HistoryController.js index 7482d676e8..2fd6364acd 100644 --- a/services/web/app/src/Features/History/HistoryController.js +++ b/services/web/app/src/Features/History/HistoryController.js @@ -12,6 +12,7 @@ const ProjectDetailsHandler = require('../Project/ProjectDetailsHandler') const ProjectEntityUpdateHandler = require('../Project/ProjectEntityUpdateHandler') const RestoreManager = require('./RestoreManager') const { pipeline } = require('stream') +const { prepareZipAttachment } = require('../../infrastructure/Response') module.exports = HistoryController = { selectHistoryApi(req, res, next) { @@ -414,10 +415,7 @@ module.exports = HistoryController = { delete response.headers['content-disposition'] delete response.headers['content-type'] res.status(response.statusCode) - res.setContentDisposition('attachment', { - filename: `${name}.zip`, - }) - res.contentType('application/zip') + prepareZipAttachment(res, `${name}.zip`) pipeline(response, res, err => { if (err) { logger.warn( diff --git a/services/web/app/src/Features/InactiveData/InactiveProjectController.js b/services/web/app/src/Features/InactiveData/InactiveProjectController.js index c3f913e6b9..d48e30992e 100644 --- a/services/web/app/src/Features/InactiveData/InactiveProjectController.js +++ b/services/web/app/src/Features/InactiveData/InactiveProjectController.js @@ -26,7 +26,7 @@ module.exports = { if (err != null) { return res.sendStatus(500) } else { - return res.send(projectsDeactivated) + return res.json(projectsDeactivated) } } ) diff --git a/services/web/app/src/Features/LinkedFiles/LinkedFilesController.js b/services/web/app/src/Features/LinkedFiles/LinkedFilesController.js index 991c5b23b6..6a5fcc5bc5 100644 --- a/services/web/app/src/Features/LinkedFiles/LinkedFilesController.js +++ b/services/web/app/src/Features/LinkedFiles/LinkedFilesController.js @@ -37,6 +37,7 @@ const { FileCannotRefreshError, } = require('./LinkedFilesErrors') const Modules = require('../../infrastructure/Modules') +const { plainTextResponse } = require('../../infrastructure/Response') module.exports = LinkedFilesController = { Agents: _.extend( @@ -138,55 +139,70 @@ module.exports = LinkedFilesController = { handleError(error, req, res, next) { if (error instanceof AccessDeniedError) { - return res.status(403).send('You do not have access to this project') + res.status(403) + plainTextResponse(res, 'You do not have access to this project') } else if (error instanceof BadDataError) { - return res.status(400).send('The submitted data is not valid') + res.status(400) + plainTextResponse(res, 'The submitted data is not valid') } else if (error instanceof BadEntityTypeError) { - return res.status(400).send('The file is the wrong type') + res.status(400) + plainTextResponse(res, 'The file is the wrong type') } else if (error instanceof SourceFileNotFoundError) { - return res.status(404).send('Source file not found') + res.status(404) + plainTextResponse(res, 'Source file not found') } else if (error instanceof ProjectNotFoundError) { - return res.status(404).send('Project not found') + res.status(404) + plainTextResponse(res, 'Project not found') } else if (error instanceof V1ProjectNotFoundError) { - return res - .status(409) - .send( - 'Sorry, the source project is not yet imported to Overleaf v2. Please import it to Overleaf v2 to refresh this file' - ) + res.status(409) + plainTextResponse( + res, + 'Sorry, the source project is not yet imported to Overleaf v2. Please import it to Overleaf v2 to refresh this file' + ) } else if (error instanceof CompileFailedError) { - return res - .status(422) - .send(res.locals.translate('generic_linked_file_compile_error')) + res.status(422) + plainTextResponse( + res, + res.locals.translate('generic_linked_file_compile_error') + ) } else if (error instanceof OutputFileFetchFailedError) { - return res.status(404).send('Could not get output file') + res.status(404) + plainTextResponse(res, 'Could not get output file') } else if (error instanceof UrlFetchFailedError) { - return res - .status(422) - .send( - `Your URL could not be reached (${error.statusCode} status code). Please check it and try again.` - ) + res.status(422) + plainTextResponse( + res, + `Your URL could not be reached (${error.statusCode} status code). Please check it and try again.` + ) } else if (error instanceof InvalidUrlError) { - return res - .status(422) - .send('Your URL is not valid. Please check it and try again.') + res.status(422) + plainTextResponse( + res, + 'Your URL is not valid. Please check it and try again.' + ) } else if (error instanceof NotOriginalImporterError) { - return res - .status(400) - .send('You are not the user who originally imported this file') + res.status(400) + plainTextResponse( + res, + 'You are not the user who originally imported this file' + ) } else if (error instanceof FeatureNotAvailableError) { - return res.status(400).send('This feature is not enabled on your account') + res.status(400) + plainTextResponse(res, 'This feature is not enabled on your account') } else if (error instanceof RemoteServiceError) { - return res.status(502).send('The remote service produced an error') + res.status(502) + plainTextResponse(res, 'The remote service produced an error') } else if (error instanceof FileCannotRefreshError) { - return res.status(400).send('This file cannot be refreshed') + res.status(400) + plainTextResponse(res, 'This file cannot be refreshed') } else if (error.message === 'project_has_too_many_files') { - return res.status(400).send('too many files') + res.status(400) + plainTextResponse(res, 'too many files') } else if (/\bECONNREFUSED\b/.test(error.message)) { - return res - .status(500) - .send('Importing references is not currently available') + res.status(500) + plainTextResponse(res, 'Importing references is not currently available') } else { - return next(error) + next(error) } }, } diff --git a/services/web/app/src/Features/Notifications/NotificationsController.js b/services/web/app/src/Features/Notifications/NotificationsController.js index 7c71c037be..8873222ac1 100644 --- a/services/web/app/src/Features/Notifications/NotificationsController.js +++ b/services/web/app/src/Features/Notifications/NotificationsController.js @@ -21,7 +21,7 @@ module.exports = { return notification } ) - res.send(unreadNotifications) + res.json(unreadNotifications) } ) }, diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index bedb40e138..9351f53d51 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -249,7 +249,7 @@ const ProjectController = { const { projectName } = req.body logger.log({ projectId, projectName }, 'cloning project') if (!SessionManager.isUserLoggedIn(req.session)) { - return res.send({ redir: '/register' }) + return res.json({ redir: '/register' }) } const currentUser = SessionManager.getSessionUser(req.session) const { first_name: firstName, last_name: lastName, email } = currentUser @@ -265,7 +265,7 @@ const ProjectController = { }) return next(err) } - res.send({ + res.json({ name: project.name, project_id: project._id, owner_ref: project.owner_ref, @@ -306,7 +306,7 @@ const ProjectController = { if (err != null) { return next(err) } - res.send({ + res.json({ project_id: project._id, owner_ref: project.owner_ref, owner: { diff --git a/services/web/app/src/Features/Spelling/SpellingController.js b/services/web/app/src/Features/Spelling/SpellingController.js index 3cf3f2972d..22daa50b3f 100644 --- a/services/web/app/src/Features/Spelling/SpellingController.js +++ b/services/web/app/src/Features/Spelling/SpellingController.js @@ -27,14 +27,14 @@ module.exports = { if (url === '/check') { if (!language) { logger.error('"language" field should be included for spell checking') - return res.status(422).send(JSON.stringify({ misspellings: [] })) + return res.status(422).json({ misspellings: [] }) } if (!languageCodeIsSupported(language)) { // this log statement can be changed to 'error' once projects with // unsupported languages are removed from the DB logger.info({ language }, 'language not supported') - return res.status(422).send(JSON.stringify({ misspellings: [] })) + return res.status(422).json({ misspellings: [] }) } } diff --git a/services/web/app/src/Features/Uploads/ProjectUploadController.js b/services/web/app/src/Features/Uploads/ProjectUploadController.js index 74e7a99db6..db97abeab3 100644 --- a/services/web/app/src/Features/Uploads/ProjectUploadController.js +++ b/services/web/app/src/Features/Uploads/ProjectUploadController.js @@ -60,7 +60,7 @@ module.exports = ProjectUploadController = { }) } } else { - return res.send({ success: true, project_id: project._id }) + return res.json({ success: true, project_id: project._id }) } } ) @@ -77,7 +77,7 @@ module.exports = ProjectUploadController = { { projectId: project_id, fileName: name }, 'bad name when trying to upload file' ) - return res.status(422).send({ + return res.status(422).json({ success: false, error: 'invalid_filename', }) @@ -106,20 +106,20 @@ module.exports = ProjectUploadController = { 'error uploading file' ) if (error.name === 'InvalidNameError') { - return res.status(422).send({ + return res.status(422).json({ success: false, error: 'invalid_filename', }) } else if (error.message === 'project_has_too_many_files') { - return res.status(422).send({ + return res.status(422).json({ success: false, error: 'project_has_too_many_files', }) } else { - return res.status(422).send({ success: false }) + return res.status(422).json({ success: false }) } } else { - return res.send({ + return res.json({ success: true, entity_id: entity != null ? entity._id : undefined, entity_type: entity != null ? entity.type : undefined, diff --git a/services/web/app/src/Features/UserMembership/UserMembershipController.js b/services/web/app/src/Features/UserMembership/UserMembershipController.js index c106783e46..3a5853a145 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipController.js +++ b/services/web/app/src/Features/UserMembership/UserMembershipController.js @@ -14,6 +14,7 @@ const SessionManager = require('../Authentication/SessionManager') const UserMembershipHandler = require('./UserMembershipHandler') const Errors = require('../Errors/Errors') const EmailHelper = require('../Helpers/EmailHelper') +const { csvAttachment } = require('../../infrastructure/Response') const CSVParser = require('json2csv').Parser module.exports = { @@ -146,9 +147,7 @@ module.exports = { return next(error) } const csvParser = new CSVParser({ fields }) - res.header('Content-Disposition', 'attachment; filename=Group.csv') - res.contentType('text/csv') - return res.send(csvParser.parse(users)) + csvAttachment(res, csvParser.parse(users), 'Group.csv') } ) }, diff --git a/services/web/app/src/infrastructure/Response.js b/services/web/app/src/infrastructure/Response.js new file mode 100644 index 0000000000..607dbaedc5 --- /dev/null +++ b/services/web/app/src/infrastructure/Response.js @@ -0,0 +1,48 @@ +function csvAttachment(res, body, filename) { + if (!filename || !filename.endsWith('.csv')) { + throw new Error('filename must end with .csv') + } + // res.attachment sets both content-type and content-disposition headers. + res.attachment(filename) + res.setHeader('X-Content-Type-Options', 'nosniff') + res.send(body) +} + +function preparePlainTextResponse(res) { + res.setHeader('X-Content-Type-Options', 'nosniff') + res.contentType('text/plain; charset=utf-8') +} + +function plainTextResponse(res, body) { + preparePlainTextResponse(res) + res.send(body) +} + +function xmlResponse(res, body) { + res.setHeader('X-Content-Type-Options', 'nosniff') + res.contentType('application/xml; charset=utf-8') + res.send(body) +} + +function prepareZipAttachment(res, filename) { + if (!filename || !filename.endsWith('.zip')) { + throw new Error('filename must end with .zip') + } + // res.attachment sets both content-type and content-disposition headers. + res.attachment(filename) + res.setHeader('X-Content-Type-Options', 'nosniff') +} + +function zipAttachment(res, body, filename) { + prepareZipAttachment(res, filename) + res.send(body) +} + +module.exports = { + csvAttachment, + plainTextResponse, + preparePlainTextResponse, + prepareZipAttachment, + xmlResponse, + zipAttachment, +} diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index bc9a125486..603731750a 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -61,6 +61,7 @@ const { const logger = require('@overleaf/logger') const _ = require('underscore') const { expressify } = require('./util/promises') +const { plainTextResponse } = require('./infrastructure/Response') module.exports = { initialize } @@ -1018,23 +1019,27 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { AdminController.unregisterServiceWorker ) - privateApiRouter.get('/perfTest', (req, res) => res.send('hello')) + privateApiRouter.get('/perfTest', (req, res) => { + plainTextResponse(res, 'hello') + }) publicApiRouter.get('/status', (req, res) => { if (!Settings.siteIsOpen) { - res.send('web site is closed (web)') + plainTextResponse(res, 'web site is closed (web)') } else if (!Settings.editorIsOpen) { - res.send('web editor is closed (web)') + plainTextResponse(res, 'web editor is closed (web)') } else { - res.send('web sharelatex is alive (web)') + plainTextResponse(res, 'web sharelatex is alive (web)') } }) - privateApiRouter.get('/status', (req, res) => - res.send('web sharelatex is alive (api)') - ) + privateApiRouter.get('/status', (req, res) => { + plainTextResponse(res, 'web sharelatex is alive (api)') + }) // used by kubernetes health-check and acceptance tests - webRouter.get('/dev/csrf', (req, res) => res.send(res.locals.csrfToken)) + webRouter.get('/dev/csrf', (req, res) => { + plainTextResponse(res, res.locals.csrfToken) + }) publicApiRouter.get( '/health_check', @@ -1085,7 +1090,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { const projectId = req.params.Project_id const sendRes = _.once(function (statusCode, message) { res.status(statusCode) - res.send(message) + plainTextResponse(res, message) ClsiCookieManager.clearServerId(projectId) }) // force every compile to a new server // set a timeout diff --git a/services/web/bin/lint_flag_res_send_usage b/services/web/bin/lint_flag_res_send_usage new file mode 100755 index 0000000000..763504de59 --- /dev/null +++ b/services/web/bin/lint_flag_res_send_usage @@ -0,0 +1,47 @@ +#!/bin/bash + +set -e + +POTENTIAL_SEND_USAGE=$(\ + grep \ + --files-with-matches \ + --recursive \ + app.js \ + app/ \ + modules/*/app \ + test/acceptance/ \ + modules/*/test/acceptance/ \ + --regex "\.send\b" \ + --regex "\bsend(" \ +) +HELPER_MODULE="app/src/infrastructure/Response.js" +if [[ "$POTENTIAL_SEND_USAGE" == "$HELPER_MODULE" ]]; then + exit 0 +fi + +for file in ${POTENTIAL_SEND_USAGE}; do + if [[ "$file" == "$HELPER_MODULE" ]]; then + continue + fi + + cat <&2 + +ERROR: $file contains a potential use of 'res.send'. + +--- +$(grep -n -C 3 "$file" --regex "\.send\b" --regex "\bsend(") +--- + +Using 'res.send' is prone to introducing XSS vulnerabilities. + +Consider using 'res.json' or one of the helpers in $HELPER_MODULE. + +If this is a false-positive, consider using a more specific name than 'send' + for your newly introduced function. + +Links: + - https://github.com/overleaf/internal/issues/6268 + +MSG + exit 1 +done diff --git a/services/web/package.json b/services/web/package.json index 4654e1bc29..194f2c0576 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -224,6 +224,7 @@ "chai-exclude": "^2.0.3", "chaid": "^1.0.2", "cheerio": "^1.0.0-rc.3", + "content-disposition": "^0.5.0", "copy-webpack-plugin": "^5.1.1", "css-loader": "^3.5.2", "es6-promise": "^4.2.8", diff --git a/services/web/test/acceptance/src/LinkedFilesTests.js b/services/web/test/acceptance/src/LinkedFilesTests.js index afcbee7a22..f36f1a8daa 100644 --- a/services/web/test/acceptance/src/LinkedFilesTests.js +++ b/services/web/test/acceptance/src/LinkedFilesTests.js @@ -7,12 +7,15 @@ const Settings = require('@overleaf/settings') const User = require('./helpers/User').promises const express = require('express') +const { + plainTextResponse, +} = require('../../../app/src/infrastructure/Response') const LinkedUrlProxy = express() LinkedUrlProxy.get('/', (req, res, next) => { if (req.query.url === 'http://example.com/foo') { - return res.send('foo foo foo') + return plainTextResponse(res, 'foo foo foo') } else if (req.query.url === 'http://example.com/bar') { - return res.send('bar bar bar') + return plainTextResponse(res, 'bar bar bar') } else { return res.sendStatus(404) } diff --git a/services/web/test/acceptance/src/mocks/MockChatApi.js b/services/web/test/acceptance/src/mocks/MockChatApi.js index ed0b51188e..522af5e56e 100644 --- a/services/web/test/acceptance/src/mocks/MockChatApi.js +++ b/services/web/test/acceptance/src/mocks/MockChatApi.js @@ -6,7 +6,7 @@ class MockChatApi extends AbstractMockApi { } getGlobalMessages(req, res) { - res.send(this.projects[req.params.project_id] || []) + res.json(this.projects[req.params.project_id] || []) } sendGlobalMessage(req, res) { @@ -19,7 +19,7 @@ class MockChatApi extends AbstractMockApi { } this.projects[projectId] = this.projects[projectId] || [] this.projects[projectId].push(message) - res.sendStatus(201).send(Object.assign({ room_id: projectId }, message)) + res.json(Object.assign({ room_id: projectId }, message)) } applyRoutes() { diff --git a/services/web/test/acceptance/src/mocks/MockClsiApi.js b/services/web/test/acceptance/src/mocks/MockClsiApi.js index 7ca60ad0d1..1a29eb8a58 100644 --- a/services/web/test/acceptance/src/mocks/MockClsiApi.js +++ b/services/web/test/acceptance/src/mocks/MockClsiApi.js @@ -1,8 +1,11 @@ const AbstractMockApi = require('./AbstractMockApi') +const { + plainTextResponse, +} = require('../../../../app/src/infrastructure/Response') class MockClsiApi extends AbstractMockApi { static compile(req, res) { - res.status(200).send({ + res.json({ compile: { status: 'success', error: null, @@ -36,9 +39,9 @@ class MockClsiApi extends AbstractMockApi { (req, res) => { const filename = req.params[0] if (filename === 'project.pdf') { - res.status(200).send('mock-pdf') + plainTextResponse(res, 'mock-pdf') } else if (filename === 'project.log') { - res.status(200).send('mock-log') + plainTextResponse(res, 'mock-log') } else { res.sendStatus(404) } @@ -48,12 +51,12 @@ class MockClsiApi extends AbstractMockApi { this.app.get( '/project/:project_id/user/:user_id/build/:build_id/output/:output_path', (req, res) => { - res.status(200).send('hello') + plainTextResponse(res, 'hello') } ) this.app.get('/project/:project_id/status', (req, res) => { - res.status(200).send() + res.sendStatus(200) }) } } diff --git a/services/web/test/acceptance/src/mocks/MockFilestoreApi.js b/services/web/test/acceptance/src/mocks/MockFilestoreApi.js index c3ba66ded1..339af7ae31 100644 --- a/services/web/test/acceptance/src/mocks/MockFilestoreApi.js +++ b/services/web/test/acceptance/src/mocks/MockFilestoreApi.js @@ -1,4 +1,7 @@ const AbstractMockApi = require('./AbstractMockApi') +const { + plainTextResponse, +} = require('../../../../app/src/infrastructure/Response') class MockFilestoreApi extends AbstractMockApi { reset() { @@ -24,7 +27,7 @@ class MockFilestoreApi extends AbstractMockApi { this.app.get('/project/:projectId/file/:fileId', (req, res) => { const { projectId, fileId } = req.params const { content } = this.files[projectId][fileId] - res.send(content) + plainTextResponse(res, content) }) // handle file copying diff --git a/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js b/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js index 2468aacb0a..63dd10c904 100644 --- a/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js +++ b/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js @@ -1,6 +1,9 @@ const AbstractMockApi = require('./AbstractMockApi') const _ = require('lodash') const { ObjectId } = require('mongodb') +const { + plainTextResponse, +} = require('../../../../app/src/infrastructure/Response') class MockProjectHistoryApi extends AbstractMockApi { reset() { @@ -64,7 +67,7 @@ class MockProjectHistoryApi extends AbstractMockApi { const { projectId, version, pathname } = req.params const key = `${projectId}:${version}:${pathname}` if (this.oldFiles[key] != null) { - res.send(this.oldFiles[key]) + plainTextResponse(res, this.oldFiles[key]) } else { res.sendStatus(404) } diff --git a/services/web/test/acceptance/src/mocks/MockRecurlyApi.js b/services/web/test/acceptance/src/mocks/MockRecurlyApi.js index b522820950..81cc4927e9 100644 --- a/services/web/test/acceptance/src/mocks/MockRecurlyApi.js +++ b/services/web/test/acceptance/src/mocks/MockRecurlyApi.js @@ -1,5 +1,6 @@ const AbstractMockApi = require('./AbstractMockApi') const SubscriptionController = require('../../../../app/src/Features/Subscription/SubscriptionController') +const { xmlResponse } = require('../../../../app/src/infrastructure/Response') class MockRecurlyApi extends AbstractMockApi { reset() { @@ -28,9 +29,11 @@ class MockRecurlyApi extends AbstractMockApi { this.app.get('/subscriptions/:id', (req, res) => { const subscription = this.getMockSubscriptionById(req.params.id) if (!subscription) { - res.status(404).end() + res.sendStatus(404) } else { - res.send(`\ + xmlResponse( + res, + `\ ${subscription.planCode} ${subscription.currency} @@ -42,22 +45,26 @@ class MockRecurlyApi extends AbstractMockApi { ${subscription.trial_ends_at} \ -`) +` + ) } }) this.app.get('/accounts/:id', (req, res) => { const subscription = this.getMockSubscriptionByAccountId(req.params.id) if (!subscription) { - res.status(404).end() + res.sendStatus(404) } else { - res.send(`\ + xmlResponse( + res, + `\ ${req.params.id} ${subscription.account.hosted_login_token} ${subscription.account.email} \ -`) +` + ) } }) @@ -67,15 +74,18 @@ class MockRecurlyApi extends AbstractMockApi { (req, res) => { const subscription = this.getMockSubscriptionByAccountId(req.params.id) if (!subscription) { - res.status(404).end() + res.sendStatus(404) } else { Object.assign(subscription.account, req.body.account) - res.send(`\ + xmlResponse( + res, + `\ ${req.params.id} ${subscription.account.email} \ -`) +` + ) } } ) @@ -83,15 +93,18 @@ class MockRecurlyApi extends AbstractMockApi { this.app.get('/coupons/:code', (req, res) => { const coupon = this.coupons[req.params.code] if (!coupon) { - res.status(404).end() + res.sendStatus(404) } else { - res.send(`\ + xmlResponse( + res, + `\ ${req.params.code} ${coupon.name || ''} ${coupon.description || ''} \ -`) +` + ) } }) @@ -107,11 +120,14 @@ class MockRecurlyApi extends AbstractMockApi { ` } - res.send(`\ + xmlResponse( + res, + `\ ${redemptionsListXml} \ -`) +` + ) }) } } diff --git a/services/web/test/acceptance/src/mocks/MockV1HistoryApi.js b/services/web/test/acceptance/src/mocks/MockV1HistoryApi.js index 14049d9def..4d550a8988 100644 --- a/services/web/test/acceptance/src/mocks/MockV1HistoryApi.js +++ b/services/web/test/acceptance/src/mocks/MockV1HistoryApi.js @@ -1,5 +1,9 @@ const AbstractMockApi = require('./AbstractMockApi') const { EventEmitter } = require('events') +const { + zipAttachment, + prepareZipAttachment, +} = require('../../../../app/src/infrastructure/Response') class MockV1HistoryApi extends AbstractMockApi { reset() { @@ -13,10 +17,10 @@ class MockV1HistoryApi extends AbstractMockApi { this.app.get( '/api/projects/:project_id/version/:version/zip', (req, res, next) => { - res.header('content-disposition', 'attachment; name=project.zip') - res.header('content-type', 'application/octet-stream') - res.send( - `Mock zip for ${req.params.project_id} at version ${req.params.version}` + zipAttachment( + res, + `Mock zip for ${req.params.project_id} at version ${req.params.version}`, + 'project.zip' ) } ) @@ -27,13 +31,14 @@ class MockV1HistoryApi extends AbstractMockApi { if (!(this.fakeZipCall++ > 0)) { return res.sendStatus(404) } - res.header('content-disposition', 'attachment; name=project.zip') - res.header('content-type', 'application/octet-stream') if (req.params.version === '42') { - return res.send( - `Mock zip for ${req.params.project_id} at version ${req.params.version}` + return zipAttachment( + res, + `Mock zip for ${req.params.project_id} at version ${req.params.version}`, + 'project.zip' ) } + prepareZipAttachment(res, 'project.zip') const writeChunk = () => { res.write('chunk' + this.sentChunks++) } diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index a026a50180..f7df3412b3 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -481,7 +481,7 @@ describe('AuthenticationController', function () { describe('requireOauth', function () { beforeEach(function () { - this.res.send = sinon.stub() + this.res.json = sinon.stub() this.res.status = sinon.stub().returns(this.res) this.res.sendStatus = sinon.stub() this.middleware = this.AuthenticationController.requireOauth() diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index 15cbcc3b71..e0fd562c0e 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/services/web/test/unit/src/Compile/CompileControllerTests.js @@ -784,7 +784,7 @@ describe('CompileController', function () { .yields(null, { content: 'body' }) this.req.params = { Project_id: this.project_id } this.req.query = { clsiserverid: 'node-42' } - this.res.send = sinon.stub() + this.res.json = sinon.stub() this.res.contentType = sinon.stub() return this.CompileController.wordCount(this.req, this.res, this.next) }) @@ -796,7 +796,7 @@ describe('CompileController', function () { }) it('should return a 200 and body', function () { - return this.res.send.calledWith({ content: 'body' }).should.equal(true) + return this.res.json.calledWith({ content: 'body' }).should.equal(true) }) }) }) diff --git a/services/web/test/unit/src/Contact/ContactControllerTests.js b/services/web/test/unit/src/Contact/ContactControllerTests.js index 2af38b0dd8..668703c337 100644 --- a/services/web/test/unit/src/Contact/ContactControllerTests.js +++ b/services/web/test/unit/src/Contact/ContactControllerTests.js @@ -15,6 +15,7 @@ const sinon = require('sinon') const { assert, expect } = require('chai') const modulePath = '../../../../app/src/Features/Contacts/ContactController.js' const SandboxedModule = require('sandboxed-module') +const MockResponse = require('../helpers/MockResponse') describe('ContactController', function () { beforeEach(function () { @@ -30,9 +31,7 @@ describe('ContactController', function () { this.next = sinon.stub() this.req = {} - this.res = {} - this.res.status = sinon.stub().returns(this.req) - return (this.res.send = sinon.stub()) + this.res = new MockResponse() }) describe('getContacts', function () { @@ -105,7 +104,7 @@ describe('ContactController', function () { }) it('should return a formatted list of contacts in contact list order, without holding accounts', function () { - return this.res.send.args[0][0].contacts.should.deep.equal([ + return this.res.json.args[0][0].contacts.should.deep.equal([ { id: 'contact-1', email: 'joe@example.com', diff --git a/services/web/test/unit/src/Downloads/ProjectDownloadsControllerTests.js b/services/web/test/unit/src/Downloads/ProjectDownloadsControllerTests.js index 4c9f0c3575..c41afb8015 100644 --- a/services/web/test/unit/src/Downloads/ProjectDownloadsControllerTests.js +++ b/services/web/test/unit/src/Downloads/ProjectDownloadsControllerTests.js @@ -46,8 +46,6 @@ describe('ProjectDownloadsController', function () { .stub() .callsArgWith(1, null, this.stream) this.req.params = { Project_id: this.project_id } - this.res.contentType = sinon.stub() - this.res.header = sinon.stub() this.project_name = 'project name with accênts' this.ProjectGetter.getProject = sinon .stub() @@ -92,9 +90,11 @@ describe('ProjectDownloadsController', function () { }) it('should name the downloaded file after the project', function () { - return this.res.setContentDisposition - .calledWith('attachment', { filename: `${this.project_name}.zip` }) - .should.equal(true) + this.res.headers.should.deep.equal({ + 'Content-Disposition': `attachment; filename="${this.project_name}.zip"`, + 'Content-Type': 'application/zip', + 'X-Content-Type-Options': 'nosniff', + }) }) it('should record the action via Metrics', function () { @@ -110,8 +110,6 @@ describe('ProjectDownloadsController', function () { .callsArgWith(1, null, this.stream) this.project_ids = ['project-1', 'project-2'] this.req.query = { project_ids: this.project_ids.join(',') } - this.res.contentType = sinon.stub() - this.res.header = sinon.stub() this.DocumentUpdaterHandler.flushMultipleProjectsToMongo = sinon .stub() .callsArgWith(1) @@ -146,11 +144,12 @@ describe('ProjectDownloadsController', function () { }) it('should name the downloaded file after the project', function () { - return this.res.setContentDisposition - .calledWith('attachment', { - filename: 'Overleaf Projects (2 items).zip', - }) - .should.equal(true) + this.res.headers.should.deep.equal({ + 'Content-Disposition': + 'attachment; filename="Overleaf Projects (2 items).zip"', + 'Content-Type': 'application/zip', + 'X-Content-Type-Options': 'nosniff', + }) }) it('should record the action via Metrics', function () { diff --git a/services/web/test/unit/src/FileStore/FileStoreControllerTests.js b/services/web/test/unit/src/FileStore/FileStoreControllerTests.js index 77d00f419f..259a583f27 100644 --- a/services/web/test/unit/src/FileStore/FileStoreControllerTests.js +++ b/services/web/test/unit/src/FileStore/FileStoreControllerTests.js @@ -2,6 +2,7 @@ const { expect } = require('chai') const sinon = require('sinon') const SandboxedModule = require('sandboxed-module') const Errors = require('../../../../app/src/Features/Errors/Errors') +const MockResponse = require('../helpers/MockResponse') const MODULE_PATH = '../../../../app/src/Features/FileStore/FileStoreController.js' @@ -33,12 +34,7 @@ describe('FileStoreController', function () { return undefined }, } - this.res = { - set: sinon.stub().returnsThis(), - setHeader: sinon.stub(), - setContentDisposition: sinon.stub(), - status: sinon.stub().returnsThis(), - } + this.res = new MockResponse() this.file = { name: 'myfile.png' } }) @@ -108,9 +104,7 @@ describe('FileStoreController', function () { describe('from a non-ios browser', function () { it('should not set Content-Type', function (done) { this.stream.pipe = des => { - this.res.setHeader - .calledWith('Content-Type', 'text/plain') - .should.equal(false) + this.res.headers.should.deep.equal({}) done() } this.controller.getFile(this.req, this.res) @@ -128,9 +122,10 @@ describe('FileStoreController', function () { it("should set Content-Type to 'text/plain'", function (done) { this.stream.pipe = des => { - this.res.setHeader - .calledWith('Content-Type', 'text/plain') - .should.equal(true) + this.res.headers.should.deep.equal({ + 'Content-Type': 'text/plain; charset=utf-8', + 'X-Content-Type-Options': 'nosniff', + }) done() } this.controller.getFile(this.req, this.res) @@ -148,9 +143,10 @@ describe('FileStoreController', function () { it("should set Content-Type to 'text/plain'", function (done) { this.stream.pipe = des => { - this.res.setHeader - .calledWith('Content-Type', 'text/plain') - .should.equal(true) + this.res.headers.should.deep.equal({ + 'Content-Type': 'text/plain; charset=utf-8', + 'X-Content-Type-Options': 'nosniff', + }) done() } this.controller.getFile(this.req, this.res) @@ -183,9 +179,7 @@ describe('FileStoreController', function () { it('Should not set the Content-type', function (done) { this.stream.pipe = des => { - this.res.setHeader - .calledWith('Content-Type', 'text/plain') - .should.equal(false) + this.res.headers.should.deep.equal({}) done() } this.controller.getFile(this.req, this.res) @@ -208,7 +202,7 @@ describe('FileStoreController', function () { this.res.end = () => { expect(this.res.status.lastCall.args).to.deep.equal([200]) - expect(this.res.set.lastCall.args).to.deep.equal([ + expect(this.res.header.lastCall.args).to.deep.equal([ 'Content-Length', expectedFileSize, ]) diff --git a/services/web/test/unit/src/Notifications/NotificationsControllerTests.js b/services/web/test/unit/src/Notifications/NotificationsControllerTests.js index dd1104d698..9ef8faa312 100644 --- a/services/web/test/unit/src/Notifications/NotificationsControllerTests.js +++ b/services/web/test/unit/src/Notifications/NotificationsControllerTests.js @@ -45,7 +45,7 @@ describe('NotificationsController', function () { .stub() .callsArgWith(1, null, allNotifications) this.controller.getAllUnreadNotifications(this.req, { - send: body => { + json: body => { body.should.deep.equal(allNotifications) this.handler.getUserNotifications.calledWith(userId).should.equal(true) done() diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index 412bb8cb0a..a98942d22f 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -343,7 +343,7 @@ describe('ProjectController', function () { describe('cloneProject', function () { it('should call the project duplicator', function (done) { - this.res.send = json => { + this.res.json = json => { this.ProjectDuplicator.duplicate .calledWith(this.user, this.project_id, this.projectName) .should.equal(true) @@ -357,7 +357,7 @@ describe('ProjectController', function () { describe('newProject', function () { it('should call the projectCreationHandler with createExampleProject', function (done) { this.req.body.template = 'example' - this.res.send = json => { + this.res.json = json => { this.ProjectCreationHandler.createExampleProject .calledWith(this.user._id, this.projectName) .should.equal(true) @@ -371,7 +371,7 @@ describe('ProjectController', function () { it('should call the projectCreationHandler with createBasicProject', function (done) { this.req.body.template = 'basic' - this.res.send = json => { + this.res.json = json => { this.ProjectCreationHandler.createExampleProject.called.should.equal( false ) diff --git a/services/web/test/unit/src/References/ReferencesControllerTests.js b/services/web/test/unit/src/References/ReferencesControllerTests.js index c013e97a8b..faace5493c 100644 --- a/services/web/test/unit/src/References/ReferencesControllerTests.js +++ b/services/web/test/unit/src/References/ReferencesControllerTests.js @@ -43,7 +43,6 @@ describe('ReferencesController', function () { } this.res = new MockResponse() this.res.json = sinon.stub() - this.res.send = sinon.stub() this.res.sendStatus = sinon.stub() return (this.fakeResponseData = { projectId: this.projectId, diff --git a/services/web/test/unit/src/Spelling/SpellingControllerTests.js b/services/web/test/unit/src/Spelling/SpellingControllerTests.js index ae450280ff..ef3fe2f53a 100644 --- a/services/web/test/unit/src/Spelling/SpellingControllerTests.js +++ b/services/web/test/unit/src/Spelling/SpellingControllerTests.js @@ -1,5 +1,6 @@ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') +const MockResponse = require('../helpers/MockResponse') const modulePath = require('path').join( __dirname, '../../../../app/src/Features/Spelling/SpellingController.js' @@ -52,11 +53,7 @@ describe('SpellingController', function () { headers: { Host: SPELLING_HOST }, } - this.res = {} - this.res.send = sinon.stub() - this.res.status = sinon.stub().returns(this.res) - this.res.end = sinon.stub() - this.res.json = sinon.stub() + this.res = new MockResponse() }) describe('proxyRequestToSpellingApi', function () { @@ -104,9 +101,7 @@ describe('SpellingController', function () { }) it('should return an empty misspellings array', function () { - this.res.send - .calledWith(JSON.stringify({ misspellings: [] })) - .should.equal(true) + this.res.json.calledWith({ misspellings: [] }).should.equal(true) }) it('should return a 422 status', function () { @@ -142,9 +137,7 @@ describe('SpellingController', function () { }) it('should return an empty misspellings array', function () { - this.res.send - .calledWith(JSON.stringify({ misspellings: [] })) - .should.equal(true) + this.res.json.calledWith({ misspellings: [] }).should.equal(true) }) it('should return a 422 status', function () { diff --git a/services/web/test/unit/src/Uploads/ProjectUploadControllerTests.js b/services/web/test/unit/src/Uploads/ProjectUploadControllerTests.js index 798f1331e9..7b7f9dc37e 100644 --- a/services/web/test/unit/src/Uploads/ProjectUploadControllerTests.js +++ b/services/web/test/unit/src/Uploads/ProjectUploadControllerTests.js @@ -100,10 +100,12 @@ describe('ProjectUploadController', function () { }) it('should return a successful response to the FileUploader client', function () { - return expect(this.res.body).to.deep.equal({ - success: true, - project_id: this.project_id, - }) + return expect(this.res.body).to.deep.equal( + JSON.stringify({ + success: true, + project_id: this.project_id, + }) + ) }) it('should record the time taken to do the upload', function () { @@ -200,11 +202,13 @@ describe('ProjectUploadController', function () { }) it('should return a successful response to the FileUploader client', function () { - return expect(this.res.body).to.deep.equal({ - success: true, - entity_id: this.entity._id, - entity_type: 'file', - }) + return expect(this.res.body).to.deep.equal( + JSON.stringify({ + success: true, + entity_id: this.entity._id, + entity_type: 'file', + }) + ) }) it('should time the request', function () { @@ -225,9 +229,11 @@ describe('ProjectUploadController', function () { }) it('should return an unsuccessful response to the FileUploader client', function () { - return expect(this.res.body).to.deep.equal({ - success: false, - }) + return expect(this.res.body).to.deep.equal( + JSON.stringify({ + success: false, + }) + ) }) }) @@ -240,10 +246,12 @@ describe('ProjectUploadController', function () { }) it('should return an unsuccessful response to the FileUploader client', function () { - return expect(this.res.body).to.deep.equal({ - success: false, - error: 'project_has_too_many_files', - }) + return expect(this.res.body).to.deep.equal( + JSON.stringify({ + success: false, + error: 'project_has_too_many_files', + }) + ) }) }) @@ -254,10 +262,12 @@ describe('ProjectUploadController', function () { }) it('should return a a non success response', function () { - return expect(this.res.body).to.deep.equal({ - success: false, - error: 'invalid_filename', - }) + return expect(this.res.body).to.deep.equal( + JSON.stringify({ + success: false, + error: 'invalid_filename', + }) + ) }) }) }) diff --git a/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js b/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js index 803fe7e0a4..b983c1b33c 100644 --- a/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js +++ b/services/web/test/unit/src/UserMembership/UserMembershipControllerTests.js @@ -280,9 +280,6 @@ describe('UserMembershipController', function () { this.req.entity = this.subscription this.req.entityConfig = EntityConfigs.groupManagers this.res = new MockResponse() - this.res.contentType = sinon.stub() - this.res.header = sinon.stub() - this.res.send = sinon.stub() return this.UserMembershipController.exportCsv(this.req, this.res) }) @@ -295,14 +292,14 @@ describe('UserMembershipController', function () { }) it('should set the correct content type on the request', function () { - return assertCalledWith(this.res.contentType, 'text/csv') + return assertCalledWith(this.res.contentType, 'text/csv; charset=utf-8') }) it('should name the exported csv file', function () { return assertCalledWith( this.res.header, 'Content-Disposition', - 'attachment; filename=Group.csv' + 'attachment; filename="Group.csv"' ) }) diff --git a/services/web/test/unit/src/helpers/MockResponse.js b/services/web/test/unit/src/helpers/MockResponse.js index 6a0777d06e..93f759fa71 100644 --- a/services/web/test/unit/src/helpers/MockResponse.js +++ b/services/web/test/unit/src/helpers/MockResponse.js @@ -12,14 +12,13 @@ * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ const sinon = require('sinon') +const Path = require('path') +const contentDisposition = require('content-disposition') class MockResponse { static initClass() { + // Added via ExpressLocals. this.prototype.setContentDisposition = sinon.stub() - - this.prototype.header = sinon.stub() - - this.prototype.contentType = sinon.stub() } constructor() { @@ -28,6 +27,18 @@ class MockResponse { this.returned = false this.headers = {} this.locals = {} + + sinon.spy(this, 'contentType') + sinon.spy(this, 'header') + sinon.spy(this, 'json') + sinon.spy(this, 'send') + sinon.spy(this, 'sendStatus') + sinon.spy(this, 'status') + sinon.spy(this, 'render') + } + + header(field, val) { + this.headers[field] = val } render(template, variables) { @@ -100,7 +111,7 @@ class MockResponse { } this.statusCode = status this.returned = true - this.type = 'application/json' + this.contentType('application/json') if (status >= 200 && status < 300) { this.success = true } else { @@ -120,7 +131,7 @@ class MockResponse { } setHeader(header, value) { - return (this.headers[header] = value) + this.header(header, value) } setTimeout(timout) { @@ -133,8 +144,29 @@ class MockResponse { } } + attachment(filename) { + switch (Path.extname(filename)) { + case '.csv': + this.contentType('text/csv; charset=utf-8') + break + case '.zip': + this.contentType('application/zip') + break + default: + throw new Error('unexpected extension') + } + this.header('Content-Disposition', contentDisposition(filename)) + return this + } + + contentType(type) { + this.header('Content-Type', type) + this.type = type + return this + } + type(type) { - return (this.type = type) + return this.contentType(type) } } MockResponse.initClass()