diff --git a/services/clsi/app/js/CompileController.js b/services/clsi/app/js/CompileController.js index b0ce3bb664..ddf83d7c8e 100644 --- a/services/clsi/app/js/CompileController.js +++ b/services/clsi/app/js/CompileController.js @@ -21,6 +21,12 @@ const ProjectPersistenceManager = require('./ProjectPersistenceManager') const logger = require('logger-sharelatex') const Errors = require('./Errors') +function isImageNameAllowed(imageName) { + const ALLOWED_IMAGES = + Settings.clsi && Settings.clsi.docker && Settings.clsi.docker.allowedImages + return !ALLOWED_IMAGES || ALLOWED_IMAGES.includes(imageName) +} + module.exports = CompileController = { compile(req, res, next) { if (next == null) { @@ -165,14 +171,21 @@ module.exports = CompileController = { const { file } = req.query const line = parseInt(req.query.line, 10) const column = parseInt(req.query.column, 10) + const { imageName } = req.query const { project_id } = req.params const { user_id } = req.params + + if (imageName && !isImageNameAllowed(imageName)) { + return res.status(400).send('invalid image') + } + return CompileManager.syncFromCode( project_id, user_id, file, line, column, + imageName, function (error, pdfPositions) { if (error != null) { return next(error) @@ -191,14 +204,20 @@ module.exports = CompileController = { const page = parseInt(req.query.page, 10) const h = parseFloat(req.query.h) const v = parseFloat(req.query.v) + const { imageName } = req.query const { project_id } = req.params const { user_id } = req.params + + if (imageName && !isImageNameAllowed(imageName)) { + return res.status(400).send('invalid image') + } return CompileManager.syncFromPdf( project_id, user_id, page, h, v, + imageName, function (error, codePositions) { if (error != null) { return next(error) @@ -218,13 +237,7 @@ module.exports = CompileController = { const { project_id } = req.params const { user_id } = req.params const { image } = req.query - if ( - image && - Settings.clsi && - Settings.clsi.docker && - Settings.clsi.docker.allowedImages && - !Settings.clsi.docker.allowedImages.includes(image) - ) { + if (image && !isImageNameAllowed(image)) { return res.status(400).send('invalid image') } logger.log({ image, file, project_id }, 'word count request') diff --git a/services/clsi/app/js/CompileManager.js b/services/clsi/app/js/CompileManager.js index 25741005f8..c801062595 100644 --- a/services/clsi/app/js/CompileManager.js +++ b/services/clsi/app/js/CompileManager.js @@ -431,7 +431,15 @@ module.exports = CompileManager = { }) }, // directory exists - syncFromCode(project_id, user_id, file_name, line, column, callback) { + syncFromCode( + project_id, + user_id, + file_name, + line, + column, + imageName, + callback + ) { // If LaTeX was run in a virtual environment, the file path that synctex expects // might not match the file path on the host. The .synctex.gz file however, will be accessed // wherever it is on the host. @@ -444,22 +452,28 @@ module.exports = CompileManager = { const compileDir = getCompileDir(project_id, user_id) const synctex_path = `${base_dir}/output.pdf` const command = ['code', synctex_path, file_path, line, column] - CompileManager._runSynctex(project_id, user_id, command, function ( - error, - stdout - ) { - if (error != null) { - return callback(error) + CompileManager._runSynctex( + project_id, + user_id, + command, + imageName, + function (error, stdout) { + if (error != null) { + return callback(error) + } + logger.log( + { project_id, user_id, file_name, line, column, command, stdout }, + 'synctex code output' + ) + return callback( + null, + CompileManager._parseSynctexFromCodeOutput(stdout) + ) } - logger.log( - { project_id, user_id, file_name, line, column, command, stdout }, - 'synctex code output' - ) - return callback(null, CompileManager._parseSynctexFromCodeOutput(stdout)) - }) + ) }, - syncFromPdf(project_id, user_id, page, h, v, callback) { + syncFromPdf(project_id, user_id, page, h, v, imageName, callback) { if (callback == null) { callback = function (error, filePositions) {} } @@ -468,22 +482,25 @@ module.exports = CompileManager = { const base_dir = Settings.path.synctexBaseDir(compileName) const synctex_path = `${base_dir}/output.pdf` const command = ['pdf', synctex_path, page, h, v] - CompileManager._runSynctex(project_id, user_id, command, function ( - error, - stdout - ) { - if (error != null) { - return callback(error) + CompileManager._runSynctex( + project_id, + user_id, + command, + imageName, + function (error, stdout) { + if (error != null) { + return callback(error) + } + logger.log( + { project_id, user_id, page, h, v, stdout }, + 'synctex pdf output' + ) + return callback( + null, + CompileManager._parseSynctexFromPdfOutput(stdout, base_dir) + ) } - logger.log( - { project_id, user_id, page, h, v, stdout }, - 'synctex pdf output' - ) - return callback( - null, - CompileManager._parseSynctexFromPdfOutput(stdout, base_dir) - ) - }) + ) }, _checkFileExists(dir, filename, callback) { @@ -513,7 +530,7 @@ module.exports = CompileManager = { }) }, - _runSynctex(project_id, user_id, command, callback) { + _runSynctex(project_id, user_id, command, imageName, callback) { if (callback == null) { callback = function (error, stdout) {} } @@ -533,9 +550,10 @@ module.exports = CompileManager = { compileName, command, directory, - Settings.clsi && Settings.clsi.docker - ? Settings.clsi.docker.image - : undefined, + imageName || + (Settings.clsi && Settings.clsi.docker + ? Settings.clsi.docker.image + : undefined), timeout, {}, compileGroup, diff --git a/services/clsi/test/acceptance/js/AllowedImageNames.js b/services/clsi/test/acceptance/js/AllowedImageNames.js index 7e38954e9c..8107273fe0 100644 --- a/services/clsi/test/acceptance/js/AllowedImageNames.js +++ b/services/clsi/test/acceptance/js/AllowedImageNames.js @@ -71,6 +71,78 @@ Hello world }) }) + describe('syncToCode', function () { + beforeEach(function (done) { + Client.compile(this.project_id, this.request, done) + }) + it('should error out with an invalid imageName', function () { + Client.syncFromCodeWithImage( + this.project_id, + 'main.tex', + 3, + 5, + 'something/evil:1337', + (error, body) => { + expect(String(error)).to.include('statusCode=400') + expect(body).to.equal('invalid image') + } + ) + }) + + it('should produce a mapping a valid imageName', function () { + Client.syncFromCodeWithImage( + this.project_id, + 'main.tex', + 3, + 5, + process.env.TEXLIVE_IMAGE, + (error, result) => { + expect(error).to.not.exist + expect(result).to.deep.equal({ + pdf: [ + { page: 1, h: 133.77, v: 134.76, height: 6.92, width: 343.71 } + ] + }) + } + ) + }) + }) + + describe('syncToPdf', function () { + beforeEach(function (done) { + Client.compile(this.project_id, this.request, done) + }) + it('should error out with an invalid imageName', function () { + Client.syncFromPdfWithImage( + this.project_id, + 'main.tex', + 100, + 200, + 'something/evil:1337', + (error, body) => { + expect(String(error)).to.include('statusCode=400') + expect(body).to.equal('invalid image') + } + ) + }) + + it('should produce a mapping a valid imageName', function () { + Client.syncFromPdfWithImage( + this.project_id, + 1, + 100, + 200, + process.env.TEXLIVE_IMAGE, + (error, result) => { + expect(error).to.not.exist + expect(result).to.deep.equal({ + code: [{ file: 'main.tex', line: 3, column: -1 }] + }) + } + ) + }) + }) + describe('wordcount', function () { beforeEach(function (done) { Client.compile(this.project_id, this.request, done) @@ -80,8 +152,9 @@ Hello world this.project_id, 'main.tex', 'something/evil:1337', - (error, result) => { + (error, body) => { expect(String(error)).to.include('statusCode=400') + expect(body).to.equal('invalid image') } ) }) diff --git a/services/clsi/test/acceptance/js/SynctexTests.js b/services/clsi/test/acceptance/js/SynctexTests.js index e636912bea..206ada45d1 100644 --- a/services/clsi/test/acceptance/js/SynctexTests.js +++ b/services/clsi/test/acceptance/js/SynctexTests.js @@ -100,9 +100,7 @@ Hello world 3, 5, (error, body) => { - if (error != null) { - throw error - } + expect(String(error)).to.include('statusCode=404') expect(body).to.equal('Not Found') return done() } @@ -117,9 +115,7 @@ Hello world 100, 200, (error, body) => { - if (error != null) { - throw error - } + expect(String(error)).to.include('statusCode=404') expect(body).to.equal('Not Found') return done() } @@ -160,9 +156,7 @@ Hello world 3, 5, (error, body) => { - if (error != null) { - throw error - } + expect(String(error)).to.include('statusCode=404') expect(body).to.equal('Not Found') return done() } @@ -177,9 +171,7 @@ Hello world 100, 200, (error, body) => { - if (error != null) { - throw error - } + expect(String(error)).to.include('statusCode=404') expect(body).to.equal('Not Found') return done() } diff --git a/services/clsi/test/acceptance/js/helpers/Client.js b/services/clsi/test/acceptance/js/helpers/Client.js index 7cd0c1148b..43825ae415 100644 --- a/services/clsi/test/acceptance/js/helpers/Client.js +++ b/services/clsi/test/acceptance/js/helpers/Client.js @@ -69,6 +69,10 @@ module.exports = Client = { }, syncFromCode(project_id, file, line, column, callback) { + Client.syncFromCodeWithImage(project_id, file, line, column, '', callback) + }, + + syncFromCodeWithImage(project_id, file, line, column, imageName, callback) { if (callback == null) { callback = function (error, pdfPositions) {} } @@ -76,6 +80,7 @@ module.exports = Client = { { url: `${this.host}/project/${project_id}/sync/code`, qs: { + imageName, file, line, column @@ -86,12 +91,19 @@ module.exports = Client = { if (error != null) { return callback(error) } + if (response.statusCode !== 200) { + return callback(new Error(`statusCode=${response.statusCode}`), body) + } return callback(null, body) } ) }, syncFromPdf(project_id, page, h, v, callback) { + Client.syncFromPdfWithImage(project_id, page, h, v, '', callback) + }, + + syncFromPdfWithImage(project_id, page, h, v, imageName, callback) { if (callback == null) { callback = function (error, pdfPositions) {} } @@ -99,6 +111,7 @@ module.exports = Client = { { url: `${this.host}/project/${project_id}/sync/pdf`, qs: { + imageName, page, h, v @@ -109,6 +122,9 @@ module.exports = Client = { if (error != null) { return callback(error) } + if (response.statusCode !== 200) { + return callback(new Error(`statusCode=${response.statusCode}`), body) + } return callback(null, body) } ) @@ -208,7 +224,7 @@ module.exports = Client = { return callback(error) } if (response.statusCode !== 200) { - return callback(new Error(`statusCode=${response.statusCode}`)) + return callback(new Error(`statusCode=${response.statusCode}`), body) } return callback(null, JSON.parse(body)) } diff --git a/services/clsi/test/unit/js/CompileControllerTests.js b/services/clsi/test/unit/js/CompileControllerTests.js index d8c7c12609..6f256d2955 100644 --- a/services/clsi/test/unit/js/CompileControllerTests.js +++ b/services/clsi/test/unit/js/CompileControllerTests.js @@ -18,6 +18,48 @@ const modulePath = require('path').join( ) const tk = require('timekeeper') +function tryImageNameValidation(method, imageNameField) { + describe('when allowedImages is set', function () { + beforeEach(function () { + this.Settings.clsi = { docker: {} } + this.Settings.clsi.docker.allowedImages = [ + 'repo/image:tag1', + 'repo/image:tag2' + ] + this.res.send = sinon.stub() + this.res.status = sinon.stub().returns({ send: this.res.send }) + + this.CompileManager[method].reset() + }) + + describe('with an invalid image', function () { + beforeEach(function () { + this.req.query[imageNameField] = 'something/evil:1337' + this.CompileController[method](this.req, this.res, this.next) + }) + it('should return a 400', function () { + expect(this.res.status.calledWith(400)).to.equal(true) + }) + it('should not run the query', function () { + expect(this.CompileManager[method].called).to.equal(false) + }) + }) + + describe('with a valid image', function () { + beforeEach(function () { + this.req.query[imageNameField] = 'repo/image:tag1' + this.CompileController[method](this.req, this.res, this.next) + }) + it('should not return a 400', function () { + expect(this.res.status.calledWith(400)).to.equal(false) + }) + it('should run the query', function () { + expect(this.CompileManager[method].called).to.equal(true) + }) + }) + }) +} + describe('CompileController', function () { beforeEach(function () { this.CompileController = SandboxedModule.require(modulePath, { @@ -248,7 +290,7 @@ describe('CompileController', function () { this.CompileManager.syncFromCode = sinon .stub() - .callsArgWith(5, null, (this.pdfPositions = ['mock-positions'])) + .yields(null, (this.pdfPositions = ['mock-positions'])) return this.CompileController.syncFromCode(this.req, this.res, this.next) }) @@ -264,13 +306,15 @@ describe('CompileController', function () { .should.equal(true) }) - return it('should return the positions', function () { + it('should return the positions', function () { return this.res.json .calledWith({ pdf: this.pdfPositions }) .should.equal(true) }) + + tryImageNameValidation('syncFromCode', 'imageName') }) describe('syncFromPdf', function () { @@ -289,7 +333,7 @@ describe('CompileController', function () { this.CompileManager.syncFromPdf = sinon .stub() - .callsArgWith(5, null, (this.codePositions = ['mock-positions'])) + .yields(null, (this.codePositions = ['mock-positions'])) return this.CompileController.syncFromPdf(this.req, this.res, this.next) }) @@ -299,13 +343,15 @@ describe('CompileController', function () { .should.equal(true) }) - return it('should return the positions', function () { + it('should return the positions', function () { return this.res.json .calledWith({ code: this.codePositions }) .should.equal(true) }) + + tryImageNameValidation('syncFromPdf', 'imageName') }) return describe('wordcount', function () { @@ -340,42 +386,6 @@ describe('CompileController', function () { .should.equal(true) }) - describe('when allowedImages is set', function () { - beforeEach(function () { - this.Settings.clsi = { docker: {} } - this.Settings.clsi.docker.allowedImages = [ - 'repo/image:tag1', - 'repo/image:tag2' - ] - this.res.send = sinon.stub() - this.res.status = sinon.stub().returns({ send: this.res.send }) - }) - - describe('with an invalid image', function () { - beforeEach(function () { - this.req.query.image = 'something/evil:1337' - this.CompileController.wordcount(this.req, this.res, this.next) - }) - it('should return a 400', function () { - expect(this.res.status.calledWith(400)).to.equal(true) - }) - it('should not run the query', function () { - expect(this.CompileManager.wordcount.called).to.equal(false) - }) - }) - - describe('with a valid image', function () { - beforeEach(function () { - this.req.query.image = 'repo/image:tag1' - this.CompileController.wordcount(this.req, this.res, this.next) - }) - it('should not return a 400', function () { - expect(this.res.status.calledWith(400)).to.equal(false) - }) - it('should run the query', function () { - expect(this.CompileManager.wordcount.called).to.equal(true) - }) - }) - }) + tryImageNameValidation('wordcount', 'image') }) }) diff --git a/services/clsi/test/unit/js/CompileManagerTests.js b/services/clsi/test/unit/js/CompileManagerTests.js index e33783a185..2d40bdd64a 100644 --- a/services/clsi/test/unit/js/CompileManagerTests.js +++ b/services/clsi/test/unit/js/CompileManagerTests.js @@ -394,13 +394,14 @@ describe('CompileManager', function () { this.stdout = `NODE\t${this.page}\t${this.h}\t${this.v}\t${this.width}\t${this.height}\n` this.CommandRunner.run = sinon .stub() - .callsArgWith(7, null, { stdout: this.stdout }) + .yields(null, { stdout: this.stdout }) return this.CompileManager.syncFromCode( this.project_id, this.user_id, this.file_name, this.line, this.column, + '', this.callback ) }) @@ -428,7 +429,7 @@ describe('CompileManager', function () { .should.equal(true) }) - return it('should call the callback with the parsed output', function () { + it('should call the callback with the parsed output', function () { return this.callback .calledWith(null, [ { @@ -441,6 +442,44 @@ describe('CompileManager', function () { ]) .should.equal(true) }) + + describe('with a custom imageName', function () { + const customImageName = 'foo/bar:tag-0' + beforeEach(function () { + this.CommandRunner.run.reset() + this.CompileManager.syncFromCode( + this.project_id, + this.user_id, + this.file_name, + this.line, + this.column, + customImageName, + this.callback + ) + }) + + it('should execute the synctex binary in a custom docker image', function () { + const synctex_path = `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}/output.pdf` + const file_path = `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}/${this.file_name}` + this.CommandRunner.run + .calledWith( + `${this.project_id}-${this.user_id}`, + [ + '/opt/synctex', + 'code', + synctex_path, + file_path, + this.line, + this.column + ], + `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}`, + customImageName, + 60000, + {} + ) + .should.equal(true) + }) + }) }) return describe('syncFromPdf', function () { @@ -460,6 +499,7 @@ describe('CompileManager', function () { this.page, this.h, this.v, + '', this.callback ) }) @@ -479,7 +519,7 @@ describe('CompileManager', function () { .should.equal(true) }) - return it('should call the callback with the parsed output', function () { + it('should call the callback with the parsed output', function () { return this.callback .calledWith(null, [ { @@ -490,6 +530,36 @@ describe('CompileManager', function () { ]) .should.equal(true) }) + + describe('with a custom imageName', function () { + const customImageName = 'foo/bar:tag-1' + beforeEach(function () { + this.CommandRunner.run.reset() + this.CompileManager.syncFromPdf( + this.project_id, + this.user_id, + this.page, + this.h, + this.v, + customImageName, + this.callback + ) + }) + + it('should execute the synctex binary in a custom docker image', function () { + const synctex_path = `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}/output.pdf` + this.CommandRunner.run + .calledWith( + `${this.project_id}-${this.user_id}`, + ['/opt/synctex', 'pdf', synctex_path, this.page, this.h, this.v], + `${this.Settings.path.compilesDir}/${this.project_id}-${this.user_id}`, + customImageName, + 60000, + {} + ) + .should.equal(true) + }) + }) }) })