From b94df9952ba88e56ac4847a18e11401b60071867 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Wed, 18 Dec 2019 09:34:25 +0000 Subject: [PATCH 1/3] Add OError --- services/filestore/npm-shrinkwrap.json | 5 +++++ services/filestore/package.json | 1 + 2 files changed, 6 insertions(+) diff --git a/services/filestore/npm-shrinkwrap.json b/services/filestore/npm-shrinkwrap.json index d636bbe990..fb1ada1128 100644 --- a/services/filestore/npm-shrinkwrap.json +++ b/services/filestore/npm-shrinkwrap.json @@ -368,6 +368,11 @@ } } }, + "@overleaf/o-error": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/@overleaf/o-error/-/o-error-2.1.0.tgz", + "integrity": "sha512-Zd9sks9LrLw8ErHt/cXeWIkyxWAqNAvNGn7wIjLQJH6TTEEW835PWOhpch+hQwwWsTxWIx/JDj+IpZ3ouw925g==" + }, "@protobufjs/aspromise": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/@protobufjs/aspromise/-/aspromise-1.1.2.tgz", diff --git a/services/filestore/package.json b/services/filestore/package.json index f9bba8a746..47aa4d0d96 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -20,6 +20,7 @@ "test:unit:_run": "mocha --recursive --reporter spec $@ test/unit/js" }, "dependencies": { + "@overleaf/o-error": "^2.1.0", "async": "~0.2.10", "aws-sdk": "^2.1.39", "body-parser": "^1.2.0", From f19d82369d91f06cbc4f0f80b532fd0ea6cc408d Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Wed, 18 Dec 2019 09:52:18 +0000 Subject: [PATCH 2/3] Bump eslintrc up again to last-known-compatible version with eslit-prettier --- services/filestore/.eslintrc | 2 +- services/filestore/npm-shrinkwrap.json | 130 +++++++++++++++---------- services/filestore/package.json | 2 +- 3 files changed, 78 insertions(+), 56 deletions(-) diff --git a/services/filestore/.eslintrc b/services/filestore/.eslintrc index e65cee6619..6bd234a2dc 100644 --- a/services/filestore/.eslintrc +++ b/services/filestore/.eslintrc @@ -9,7 +9,7 @@ "prettier/standard" ], "parserOptions": { - "ecmaVersion": 6 + "ecmaVersion": 2017 }, "plugins": [ "mocha", diff --git a/services/filestore/npm-shrinkwrap.json b/services/filestore/npm-shrinkwrap.json index fb1ada1128..ef0f78fc15 100644 --- a/services/filestore/npm-shrinkwrap.json +++ b/services/filestore/npm-shrinkwrap.json @@ -1455,47 +1455,48 @@ "integrity": "sha1-G2HAViGQqN/2rjuyzwIAyhMLhtQ=" }, "eslint": { - "version": "5.16.0", - "resolved": "https://registry.npmjs.org/eslint/-/eslint-5.16.0.tgz", - "integrity": "sha512-S3Rz11i7c8AA5JPv7xAH+dOyq/Cu/VXHiHXBPOU1k/JAM5dXqQPt3qcrhpHSorXmrpu2g0gkIBVXAqCpzfoZIg==", + "version": "6.4.0", + "resolved": "https://registry.npmjs.org/eslint/-/eslint-6.4.0.tgz", + "integrity": "sha512-WTVEzK3lSFoXUovDHEbkJqCVPEPwbhCq4trDktNI6ygs7aO41d4cDT0JFAT5MivzZeVLWlg7vHL+bgrQv/t3vA==", "dev": true, "requires": { "@babel/code-frame": "^7.0.0", - "ajv": "^6.9.1", + "ajv": "^6.10.0", "chalk": "^2.1.0", "cross-spawn": "^6.0.5", "debug": "^4.0.1", "doctrine": "^3.0.0", - "eslint-scope": "^4.0.3", - "eslint-utils": "^1.3.1", - "eslint-visitor-keys": "^1.0.0", - "espree": "^5.0.1", + "eslint-scope": "^5.0.0", + "eslint-utils": "^1.4.2", + "eslint-visitor-keys": "^1.1.0", + "espree": "^6.1.1", "esquery": "^1.0.1", "esutils": "^2.0.2", "file-entry-cache": "^5.0.1", "functional-red-black-tree": "^1.0.1", - "glob": "^7.1.2", + "glob-parent": "^5.0.0", "globals": "^11.7.0", "ignore": "^4.0.6", "import-fresh": "^3.0.0", "imurmurhash": "^0.1.4", - "inquirer": "^6.2.2", - "js-yaml": "^3.13.0", + "inquirer": "^6.4.1", + "is-glob": "^4.0.0", + "js-yaml": "^3.13.1", "json-stable-stringify-without-jsonify": "^1.0.1", "levn": "^0.3.0", - "lodash": "^4.17.11", + "lodash": "^4.17.14", "minimatch": "^3.0.4", "mkdirp": "^0.5.1", "natural-compare": "^1.4.0", "optionator": "^0.8.2", - "path-is-inside": "^1.0.2", "progress": "^2.0.0", "regexpp": "^2.0.1", - "semver": "^5.5.1", - "strip-ansi": "^4.0.0", - "strip-json-comments": "^2.0.1", + "semver": "^6.1.2", + "strip-ansi": "^5.2.0", + "strip-json-comments": "^3.0.1", "table": "^5.2.3", - "text-table": "^0.2.0" + "text-table": "^0.2.0", + "v8-compile-cache": "^2.0.3" }, "dependencies": { "debug": { @@ -1507,19 +1508,11 @@ "ms": "^2.1.1" } }, - "glob": { - "version": "7.1.6", - "resolved": "https://registry.npmjs.org/glob/-/glob-7.1.6.tgz", - "integrity": "sha512-LwaxwyZ72Lk7vZINtNNrywX0ZuLyStrdDtabefZKAY5ZGJhVtgdznluResxNmPitE0SAO+O26sWTHeKSI2wMBA==", - "dev": true, - "requires": { - "fs.realpath": "^1.0.0", - "inflight": "^1.0.4", - "inherits": "2", - "minimatch": "^3.0.4", - "once": "^1.3.0", - "path-is-absolute": "^1.0.0" - } + "lodash": { + "version": "4.17.15", + "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.15.tgz", + "integrity": "sha512-8xOcRHvCjnocdS5cpwXQXVzmmh5e5+saE2QGoeQmbKmRS6J3VQppPOIt0MnmE+4xlZoumy0GPG0D0MVIQbNA1A==", + "dev": true }, "ms": { "version": "2.1.2", @@ -1528,19 +1521,10 @@ "dev": true }, "semver": { - "version": "5.7.1", - "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz", - "integrity": "sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ==", + "version": "6.3.0", + "resolved": "https://registry.npmjs.org/semver/-/semver-6.3.0.tgz", + "integrity": "sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw==", "dev": true - }, - "strip-ansi": { - "version": "4.0.0", - "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-4.0.0.tgz", - "integrity": "sha1-qEeQIusaw2iocTibY1JixQXuNo8=", - "dev": true, - "requires": { - "ansi-regex": "^3.0.0" - } } } }, @@ -1703,9 +1687,9 @@ "dev": true }, "eslint-scope": { - "version": "4.0.3", - "resolved": "https://registry.npmjs.org/eslint-scope/-/eslint-scope-4.0.3.tgz", - "integrity": "sha512-p7VutNr1O/QrxysMo3E45FjYDTeXBy0iTltPFNSqKAIfjDSXC+4dj+qfyuD8bfAXrW/y6lW3O76VaYNPKfpKrg==", + "version": "5.0.0", + "resolved": "https://registry.npmjs.org/eslint-scope/-/eslint-scope-5.0.0.tgz", + "integrity": "sha512-oYrhJW7S0bxAFDvWqzvMPRm6pcgcnWc4QnofCAqRTRfQC0JcwenzGglTtsLyIuuWFfkqDG9vz67cnttSd53djw==", "dev": true, "requires": { "esrecurse": "^4.1.0", @@ -1728,14 +1712,22 @@ "dev": true }, "espree": { - "version": "5.0.1", - "resolved": "https://registry.npmjs.org/espree/-/espree-5.0.1.tgz", - "integrity": "sha512-qWAZcWh4XE/RwzLJejfcofscgMc9CamR6Tn1+XRXNzrvUSSbiAjGOI/fggztjIi7y9VLPqnICMIPiGyr8JaZ0A==", + "version": "6.1.2", + "resolved": "https://registry.npmjs.org/espree/-/espree-6.1.2.tgz", + "integrity": "sha512-2iUPuuPP+yW1PZaMSDM9eyVf8D5P0Hi8h83YtZ5bPc/zHYjII5khoixIUTMO794NOY8F/ThF1Bo8ncZILarUTA==", "dev": true, "requires": { - "acorn": "^6.0.7", - "acorn-jsx": "^5.0.0", - "eslint-visitor-keys": "^1.0.0" + "acorn": "^7.1.0", + "acorn-jsx": "^5.1.0", + "eslint-visitor-keys": "^1.1.0" + }, + "dependencies": { + "acorn": { + "version": "7.1.0", + "resolved": "https://registry.npmjs.org/acorn/-/acorn-7.1.0.tgz", + "integrity": "sha512-kL5CuoXA/dgxlBbVrflsflzQ3PAas7RYZB52NOm/6839iVYJgKMJ3cQJD+t2i5+qFa8h3MDpEOJiS64E8JLnSQ==", + "dev": true + } } }, "esprima": { @@ -2194,6 +2186,15 @@ "path-is-absolute": "^1.0.0" } }, + "glob-parent": { + "version": "5.1.0", + "resolved": "https://registry.npmjs.org/glob-parent/-/glob-parent-5.1.0.tgz", + "integrity": "sha512-qjtRgnIVmOfnKUE3NJAQEdk+lKrxfw8t5ke7SXtfMTHcjsBfOfWXCQfdb30zfDoZQ2IRSIiidmjtbHZPZ++Ihw==", + "dev": true, + "requires": { + "is-glob": "^4.0.1" + } + }, "globals": { "version": "11.12.0", "resolved": "https://registry.npmjs.org/globals/-/globals-11.12.0.tgz", @@ -2506,12 +2507,27 @@ "integrity": "sha1-mqIOtq7rv/d/vTPnTKAbM1gdOhY=", "dev": true }, + "is-extglob": { + "version": "2.1.1", + "resolved": "https://registry.npmjs.org/is-extglob/-/is-extglob-2.1.1.tgz", + "integrity": "sha1-qIwCU1eR8C7TfHahueqXc8gz+MI=", + "dev": true + }, "is-fullwidth-code-point": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/is-fullwidth-code-point/-/is-fullwidth-code-point-2.0.0.tgz", "integrity": "sha1-o7MKXE8ZkYMWeqq5O+764937ZU8=", "dev": true }, + "is-glob": { + "version": "4.0.1", + "resolved": "https://registry.npmjs.org/is-glob/-/is-glob-4.0.1.tgz", + "integrity": "sha512-5G0tKtBTFImOqDnLB2hG6Bp2qcKEFduo4tZu9MT/H6NQv/ghhy30o55ufafxJ/LdH79LLs2Kfrn85TLKyA7BUg==", + "dev": true, + "requires": { + "is-extglob": "^2.1.1" + } + }, "is-promise": { "version": "2.1.0", "resolved": "https://registry.npmjs.org/is-promise/-/is-promise-2.1.0.tgz", @@ -4974,9 +4990,9 @@ "dev": true }, "strip-json-comments": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/strip-json-comments/-/strip-json-comments-2.0.1.tgz", - "integrity": "sha1-PFMZQukIwml8DsNEhYwobHygpgo=", + "version": "3.0.1", + "resolved": "https://registry.npmjs.org/strip-json-comments/-/strip-json-comments-3.0.1.tgz", + "integrity": "sha512-VTyMAUfdm047mwKl+u79WIdrZxtFtn+nBxHeb844XBQ9uMNTuTHdx2hc5RiAJYqwTj3wc/xe5HLSdJSkJ+WfZw==", "dev": true }, "supports-color": { @@ -5233,6 +5249,12 @@ "resolved": "https://registry.npmjs.org/utils-merge/-/utils-merge-1.0.1.tgz", "integrity": "sha1-n5VxD1CiZ5R7LMwSR0HBAoQn5xM=" }, + "v8-compile-cache": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/v8-compile-cache/-/v8-compile-cache-2.1.0.tgz", + "integrity": "sha512-usZBT3PW+LOjM25wbqIlZwPeJV+3OSz3M1k1Ws8snlW39dZyYL9lOGC5FgPVHfk0jKmjiDV8Z0mIbVQPiwFs7g==", + "dev": true + }, "validate-npm-package-license": { "version": "3.0.4", "resolved": "https://registry.npmjs.org/validate-npm-package-license/-/validate-npm-package-license-3.0.4.tgz", diff --git a/services/filestore/package.json b/services/filestore/package.json index 47aa4d0d96..fd03757072 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -48,7 +48,7 @@ "babel-eslint": "^10.0.3", "bunyan": "^1.3.5", "chai": "4.2.0", - "eslint": "^5.16.0", + "eslint": "^6.4.0", "eslint-config-prettier": "^6.7.0", "eslint-config-standard": "^14.1.0", "eslint-plugin-chai-expect": "^2.1.0", From eacad771127857339b84cd7e9c7455bd8c3e68f9 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Wed, 18 Dec 2019 10:01:59 +0000 Subject: [PATCH 3/3] Cleanup SafeExec --- services/filestore/app/js/Errors.js | 52 +++++-- services/filestore/app/js/SafeExec.js | 90 ++++++------ .../filestore/test/unit/js/SafeExecTests.js | 134 ++++++++++-------- 3 files changed, 161 insertions(+), 115 deletions(-) diff --git a/services/filestore/app/js/Errors.js b/services/filestore/app/js/Errors.js index f3bc8e37eb..4231571cb3 100644 --- a/services/filestore/app/js/Errors.js +++ b/services/filestore/app/js/Errors.js @@ -1,16 +1,38 @@ -/* eslint-disable - no-proto, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -let Errors -var NotFoundError = function(message) { - const error = new Error(message) - error.name = 'NotFoundError' - error.__proto__ = NotFoundError.prototype - return error -} -NotFoundError.prototype.__proto__ = Error.prototype +const OError = require('@overleaf/o-error') -module.exports = Errors = { NotFoundError } +// Error class for legacy errors so they inherit OError while staying +// backward-compatible (can be instantiated with string as argument instead +// of object) +class BackwardCompatibleError extends OError { + constructor(messageOrOptions) { + let options + if (typeof messageOrOptions === 'string') { + options = { message: messageOrOptions } + } else if (!messageOrOptions) { + options = {} + } else { + options = messageOrOptions + } + super(options) + } +} + +class NotFoundError extends BackwardCompatibleError {} +class ConversionsDisabledError extends BackwardCompatibleError {} + +class FailedCommandError extends OError { + constructor(command, code, stdout, stderr) { + super({ + message: 'command failed with error exit code', + info: { + command, + code + } + }) + this.stdout = stdout + this.stderr = stderr + this.code = code + } +} + +module.exports = { NotFoundError, FailedCommandError, ConversionsDisabledError } diff --git a/services/filestore/app/js/SafeExec.js b/services/filestore/app/js/SafeExec.js index dbc1576a88..5f079fa474 100644 --- a/services/filestore/app/js/SafeExec.js +++ b/services/filestore/app/js/SafeExec.js @@ -1,21 +1,8 @@ -/* eslint-disable - camelcase, - handle-callback-err, - no-return-assign, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const _ = require('underscore') const logger = require('logger-sharelatex') -const child_process = require('child_process') +const childProcess = require('child_process') const Settings = require('settings-sharelatex') +const { ConversionsDisabledError, FailedCommandError } = require('./Errors') // execute a command in the same way as 'exec' but with a timeout that // kills all child processes @@ -23,36 +10,32 @@ const Settings = require('settings-sharelatex') // we spawn the command with 'detached:true' to make a new process // group, then we can kill everything in that process group. -module.exports = function(command, options, callback) { - if (callback == null) { - callback = function(err, stdout, stderr) {} - } +module.exports = safeExec +module.exports.promises = safeExecPromise + +// options are {timeout: number-of-milliseconds, killSignal: signal-name} +function safeExec(command, options, callback) { if (!Settings.enableConversions) { - const error = new Error('Image conversions are disabled') - return callback(error) + return callback( + new ConversionsDisabledError('image conversions are disabled') + ) } - // options are {timeout: number-of-milliseconds, killSignal: signal-name} - const [cmd, ...args] = Array.from(command) + const [cmd, ...args] = command - const child = child_process.spawn(cmd, args, { detached: true }) + const child = childProcess.spawn(cmd, args, { detached: true }) let stdout = '' let stderr = '' - const cleanup = _.once(function(err) { - if (killTimer != null) { - clearTimeout(killTimer) - } - return callback(err, stdout, stderr) - }) + let killTimer - if (options.timeout != null) { - var killTimer = setTimeout(function() { + if (options.timeout) { + killTimer = setTimeout(function() { try { // use negative process id to kill process group - return process.kill(-child.pid, options.killSignal || 'SIGTERM') + process.kill(-child.pid, options.killSignal || 'SIGTERM') } catch (error) { - return logger.log( + logger.log( { process: child.pid, kill_error: error }, 'error killing process' ) @@ -60,14 +43,41 @@ module.exports = function(command, options, callback) { }, options.timeout) } - child.on('close', function(code, signal) { - const err = code ? new Error(`exit status ${code}`) : signal - return cleanup(err) + const cleanup = _.once(function(err) { + if (killTimer) { + clearTimeout(killTimer) + } + callback(err, stdout, stderr) }) - child.on('error', err => cleanup(err)) + child.on('close', function(code, signal) { + if (code || signal) { + return cleanup( + new FailedCommandError(command, code || signal, stdout, stderr) + ) + } - child.stdout.on('data', chunk => (stdout += chunk)) + cleanup() + }) - return child.stderr.on('data', chunk => (stderr += chunk)) + child.on('error', err => { + cleanup(err) + }) + child.stdout.on('data', chunk => { + stdout += chunk + }) + child.stderr.on('data', chunk => { + stderr += chunk + }) +} + +function safeExecPromise(command, options) { + return new Promise((resolve, reject) => { + safeExec(command, options, (err, stdout, stderr) => { + if (err) { + reject(err) + } + resolve({ stdout, stderr }) + }) + }) } diff --git a/services/filestore/test/unit/js/SafeExecTests.js b/services/filestore/test/unit/js/SafeExecTests.js index 2b629947f5..077964ead7 100644 --- a/services/filestore/test/unit/js/SafeExecTests.js +++ b/services/filestore/test/unit/js/SafeExecTests.js @@ -1,95 +1,109 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const { assert } = require('chai') -const sinon = require('sinon') const chai = require('chai') const should = chai.should() const { expect } = chai -const modulePath = '../../../app/js/SafeExec.js' +const modulePath = '../../../app/js/SafeExec' const SandboxedModule = require('sandboxed-module') describe('SafeExec', function() { + let settings, options, safeExec + beforeEach(function() { - this.settings = { enableConversions: true } - this.safe_exec = SandboxedModule.require(modulePath, { + settings = { enableConversions: true } + options = { timeout: 10 * 1000, killSignal: 'SIGTERM' } + + safeExec = SandboxedModule.require(modulePath, { requires: { 'logger-sharelatex': { log() {}, err() {} }, - 'settings-sharelatex': this.settings + 'settings-sharelatex': settings } }) - return (this.options = { timeout: 10 * 1000, killSignal: 'SIGTERM' }) }) - return describe('safe_exec', function() { + describe('safeExec', function() { it('should execute a valid command', function(done) { - return this.safe_exec( - ['/bin/echo', 'hello'], - this.options, - (err, stdout, stderr) => { - stdout.should.equal('hello\n') - should.not.exist(err) - return done() - } - ) + safeExec(['/bin/echo', 'hello'], options, (err, stdout, stderr) => { + stdout.should.equal('hello\n') + stderr.should.equal('') + should.not.exist(err) + done() + }) }) it('should error when conversions are disabled', function(done) { - this.settings.enableConversions = false - return this.safe_exec( - ['/bin/echo', 'hello'], - this.options, - (err, stdout, stderr) => { - expect(err).to.exist - return done() - } - ) + settings.enableConversions = false + safeExec(['/bin/echo', 'hello'], options, err => { + expect(err).to.exist + done() + }) }) it('should execute a command with non-zero exit status', function(done) { - return this.safe_exec( - ['/usr/bin/env', 'false'], - this.options, - (err, stdout, stderr) => { - stdout.should.equal('') - stderr.should.equal('') - err.message.should.equal('exit status 1') - return done() - } - ) + safeExec(['/usr/bin/env', 'false'], options, err => { + expect(err).to.exist + expect(err.name).to.equal('FailedCommandError') + expect(err.code).to.equal(1) + expect(err.stdout).to.equal('') + expect(err.stderr).to.equal('') + done() + }) }) it('should handle an invalid command', function(done) { - return this.safe_exec( - ['/bin/foobar'], - this.options, - (err, stdout, stderr) => { - err.code.should.equal('ENOENT') - return done() - } - ) + safeExec(['/bin/foobar'], options, err => { + err.code.should.equal('ENOENT') + done() + }) }) - return it('should handle a command that runs too long', function(done) { - return this.safe_exec( + it('should handle a command that runs too long', function(done) { + safeExec( ['/bin/sleep', '10'], { timeout: 500, killSignal: 'SIGTERM' }, - (err, stdout, stderr) => { - err.should.equal('SIGTERM') - return done() + err => { + expect(err).to.exist + expect(err.name).to.equal('FailedCommandError') + expect(err.code).to.equal('SIGTERM') + done() } ) }) }) + + describe('as a promise', function() { + beforeEach(function() { + safeExec = safeExec.promises + }) + + it('should execute a valid command', async function() { + const { stdout, stderr } = await safeExec(['/bin/echo', 'hello'], options) + + stdout.should.equal('hello\n') + stderr.should.equal('') + }) + + it('should throw a ConversionsDisabledError when appropriate', async function() { + settings.enableConversions = false + try { + await safeExec(['/bin/echo', 'hello'], options) + } catch (err) { + expect(err.name).to.equal('ConversionsDisabledError') + return + } + expect('method did not throw an error').not.to.exist + }) + + it('should throw a FailedCommandError when appropriate', async function() { + try { + await safeExec(['/usr/bin/env', 'false'], options) + } catch (err) { + expect(err.name).to.equal('FailedCommandError') + expect(err.code).to.equal(1) + return + } + expect('method did not throw an error').not.to.exist + }) + }) })