Merge pull request #219 from overleaf/jpa-read-image-name-for-synctex

[misc] consume and validate a custom imageName for synctex requests

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 (
+    if (image && !isImageNameAllowed(image)) {
-      image &&
-      Settings.clsi &&
-      Settings.clsi.docker &&
-      Settings.clsi.docker.allowedImages &&
-      !Settings.clsi.docker.allowedImages.includes(image)
-    ) {
       return res.status(400).send('invalid image')
     }
     logger.log({ image, file, project_id }, 'word count request')

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,10 +452,12 @@ 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 (
+    CompileManager._runSynctex(
-      error,
+      project_id,
-      stdout
+      user_id,
-    ) {
+      command,
+      imageName,
+      function (error, stdout) {
         if (error != null) {
           return callback(error)
         }
@@ -455,11 +465,15 @@ module.exports = CompileManager = {
           { project_id, user_id, file_name, line, column, command, stdout },
           'synctex code output'
         )
-      return callback(null, CompileManager._parseSynctexFromCodeOutput(stdout))
+        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,10 +482,12 @@ 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 (
+    CompileManager._runSynctex(
-      error,
+      project_id,
-      stdout
+      user_id,
-    ) {
+      command,
+      imageName,
+      function (error, stdout) {
         if (error != null) {
           return callback(error)
         }
@@ -483,7 +499,8 @@ module.exports = CompileManager = {
           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
+        imageName ||
+          (Settings.clsi && Settings.clsi.docker
             ? Settings.clsi.docker.image
-          : undefined,
+            : undefined),
         timeout,
         {},
         compileGroup,

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')
         }
       )
     })

services/clsi/test/acceptance/js/SynctexTests.js

@@ -100,9 +100,7 @@ Hello world
           3,
           5,
           (error, body) => {
-            if (error != null) {
+            expect(String(error)).to.include('statusCode=404')
-              throw error
-            }
             expect(body).to.equal('Not Found')
             return done()
           }
@@ -117,9 +115,7 @@ Hello world
           100,
           200,
           (error, body) => {
-            if (error != null) {
+            expect(String(error)).to.include('statusCode=404')
-              throw error
-            }
             expect(body).to.equal('Not Found')
             return done()
           }
@@ -160,9 +156,7 @@ Hello world
           3,
           5,
           (error, body) => {
-            if (error != null) {
+            expect(String(error)).to.include('statusCode=404')
-              throw error
-            }
             expect(body).to.equal('Not Found')
             return done()
           }
@@ -177,9 +171,7 @@ Hello world
           100,
           200,
           (error, body) => {
-            if (error != null) {
+            expect(String(error)).to.include('statusCode=404')
-              throw error
-            }
             expect(body).to.equal('Not Found')
             return done()
           }

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))
       }

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 () {
+    tryImageNameValidation('wordcount', 'image')
-      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)
-        })
-      })
-    })
   })
 })

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)
+        })
+      })
     })
   })