Merge pull request #183 from overleaf/jpa-clsi-allowed-image-names

[misc] RequestParser: restrict imageName to an allow list and add tests

services/clsi/app/js/CompileController.js

@@ -218,6 +218,15 @@ 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)
+    ) {
+      return res.status(400).send('invalid image')
+    }
     logger.log({ image, file, project_id }, 'word count request')
 
     return CompileManager.wordcount(project_id, user_id, file, image, function(

services/clsi/app/js/DockerRunner.js

@@ -86,6 +86,13 @@ module.exports = DockerRunner = {
       ;({ image } = Settings.clsi.docker)
     }
 
+    if (
+      Settings.clsi.docker.allowedImages &&
+      !Settings.clsi.docker.allowedImages.includes(image)
+    ) {
+      return callback(new Error('image not allowed'))
+    }
+
     if (Settings.texliveImageNameOveride != null) {
       const img = image.split('/')
       image = `${Settings.texliveImageNameOveride}/${img[2]}`

services/clsi/app/js/RequestParser.js

@@ -61,7 +61,13 @@ module.exports = RequestParser = {
       response.imageName = this._parseAttribute(
         'imageName',
         compile.options.imageName,
-        { type: 'string' }
+        {
+          type: 'string',
+          validValues:
+            settings.clsi &&
+            settings.clsi.docker &&
+            settings.clsi.docker.allowedImages
+        }
       )
       response.draft = this._parseAttribute('draft', compile.options.draft, {
         default: false,

services/clsi/config/settings.defaults.js

@@ -129,6 +129,17 @@ if (process.env.DOCKER_RUNNER) {
     process.exit(1)
   }
 
+  if (process.env.ALLOWED_IMAGES) {
+    try {
+      module.exports.clsi.docker.allowedImages = process.env.ALLOWED_IMAGES.split(
+        ' '
+      )
+    } catch (error) {
+      console.error(error, 'could not apply allowed images setting')
+      process.exit(1)
+    }
+  }
+
   module.exports.path.synctexBaseDir = () => '/compile'
 
   module.exports.path.sandboxedCompilesHostDir = process.env.COMPILES_HOST_DIR

services/clsi/docker-compose-config.yml

@@ -3,6 +3,7 @@ version: "2.3"
 services:
   dev:
     environment:
+      ALLOWED_IMAGES: "quay.io/sharelatex/texlive-full:2017.1"
       TEXLIVE_IMAGE: quay.io/sharelatex/texlive-full:2017.1
       TEXLIVE_IMAGE_USER: "tex"
       SHARELATEX_CONFIG: /app/config/settings.defaults.coffee
@@ -18,6 +19,7 @@ services:
       
   ci:
     environment:
+      ALLOWED_IMAGES: ${TEXLIVE_IMAGE}
       TEXLIVE_IMAGE: quay.io/sharelatex/texlive-full:2017.1
       TEXLIVE_IMAGE_USER: "tex"
       SHARELATEX_CONFIG: /app/config/settings.defaults.coffee

services/clsi/test/acceptance/js/AllowedImageNames.js

@@ -0,0 +1,102 @@
+const Client = require('./helpers/Client')
+const ClsiApp = require('./helpers/ClsiApp')
+const { expect } = require('chai')
+
+describe('AllowedImageNames', function() {
+  beforeEach(function(done) {
+    this.project_id = Client.randomId()
+    this.request = {
+      options: {
+        imageName: undefined
+      },
+      resources: [
+        {
+          path: 'main.tex',
+          content: `\
+\\documentclass{article}
+\\begin{document}
+Hello world
+\\end{document}\
+`
+        }
+      ]
+    }
+    ClsiApp.ensureRunning(done)
+  })
+
+  describe('with a valid name', function() {
+    beforeEach(function(done) {
+      this.request.options.imageName = process.env.TEXLIVE_IMAGE
+
+      Client.compile(this.project_id, this.request, (error, res, body) => {
+        this.error = error
+        this.res = res
+        this.body = body
+        done(error)
+      })
+    })
+    it('should return success', function() {
+      expect(this.res.statusCode).to.equal(200)
+    })
+
+    it('should return a PDF', function() {
+      let pdf
+      try {
+        pdf = Client.getOutputFile(this.body, 'pdf')
+      } catch (e) {}
+      expect(pdf).to.exist
+    })
+  })
+
+  describe('with an invalid name', function() {
+    beforeEach(function(done) {
+      this.request.options.imageName = 'something/evil:1337'
+      Client.compile(this.project_id, this.request, (error, res, body) => {
+        this.error = error
+        this.res = res
+        this.body = body
+        done(error)
+      })
+    })
+    it('should return non success', function() {
+      expect(this.res.statusCode).to.not.equal(200)
+    })
+
+    it('should not return a PDF', function() {
+      let pdf
+      try {
+        pdf = Client.getOutputFile(this.body, 'pdf')
+      } catch (e) {}
+      expect(pdf).to.not.exist
+    })
+  })
+
+  describe('wordcount', function() {
+    beforeEach(function(done) {
+      Client.compile(this.project_id, this.request, done)
+    })
+    it('should error out with an invalid imageName', function() {
+      Client.wordcountWithImage(
+        this.project_id,
+        'main.tex',
+        'something/evil:1337',
+        (error, result) => {
+          expect(String(error)).to.include('statusCode=400')
+        }
+      )
+    })
+
+    it('should produce a texcout a valid imageName', function() {
+      Client.wordcountWithImage(
+        this.project_id,
+        'main.tex',
+        process.env.TEXLIVE_IMAGE,
+        (error, result) => {
+          expect(error).to.not.exist
+          expect(result).to.exist
+          expect(result.texcount).to.exist
+        }
+      )
+    })
+  })
+})

services/clsi/test/acceptance/js/helpers/Client.js

@@ -189,6 +189,11 @@ module.exports = Client = {
   },
 
   wordcount(project_id, file, callback) {
+    const image = undefined
+    Client.wordcountWithImage(project_id, file, image, callback)
+  },
+
+  wordcountWithImage(project_id, file, image, callback) {
     if (callback == null) {
       callback = function(error, pdfPositions) {}
     }
@@ -196,6 +201,7 @@ module.exports = Client = {
       {
         url: `${this.host}/project/${project_id}/wordcount`,
         qs: {
+          image,
           file
         }
       },
@@ -203,6 +209,9 @@ module.exports = Client = {
         if (error != null) {
           return callback(error)
         }
+        if (response.statusCode !== 200) {
+          return callback(new Error(`statusCode=${response.statusCode}`))
+        }
         return callback(null, JSON.parse(body))
       }
     )

services/clsi/test/unit/js/CompileControllerTests.js

@@ -12,6 +12,7 @@
 const SandboxedModule = require('sandboxed-module')
 const sinon = require('sinon')
 require('chai').should()
+const { expect } = require('chai')
 const modulePath = require('path').join(
   __dirname,
   '../../../app/js/CompileController'
@@ -287,21 +288,60 @@ describe('CompileController', function() {
       this.CompileManager.wordcount = sinon
         .stub()
         .callsArgWith(4, null, (this.texcount = ['mock-texcount']))
-      return this.CompileController.wordcount(this.req, this.res, this.next)
     })
 
     it('should return the word count of a file', function() {
+      this.CompileController.wordcount(this.req, this.res, this.next)
       return this.CompileManager.wordcount
         .calledWith(this.project_id, undefined, this.file, this.image)
         .should.equal(true)
     })
 
-    return it('should return the texcount info', function() {
+    it('should return the texcount info', function() {
+      this.CompileController.wordcount(this.req, this.res, this.next)
       return this.res.json
         .calledWith({
           texcount: this.texcount
         })
         .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)
+        })
+      })
+    })
   })
 })

services/clsi/test/unit/js/DockerRunnerTests.js

@@ -273,7 +273,7 @@ describe('DockerRunner', function() {
       })
     })
 
-    return describe('with image override', function() {
+    describe('with image override', function() {
       beforeEach(function() {
         this.Settings.texliveImageNameOveride = 'overrideimage.com/something'
         this.DockerRunner._runAndWaitForContainer = sinon
@@ -296,6 +296,62 @@ describe('DockerRunner', function() {
         return image.should.equal('overrideimage.com/something/image:2016.2')
       })
     })
+
+    describe('with image restriction', function() {
+      beforeEach(function() {
+        this.Settings.clsi.docker.allowedImages = [
+          'repo/image:tag1',
+          'repo/image:tag2'
+        ]
+        this.DockerRunner._runAndWaitForContainer = sinon
+          .stub()
+          .callsArgWith(3, null, (this.output = 'mock-output'))
+      })
+
+      describe('with a valid image', function() {
+        beforeEach(function() {
+          this.DockerRunner.run(
+            this.project_id,
+            this.command,
+            this.directory,
+            'repo/image:tag1',
+            this.timeout,
+            this.env,
+            this.compileGroup,
+            this.callback
+          )
+        })
+
+        it('should setup the container', function() {
+          this.DockerRunner._getContainerOptions.called.should.equal(true)
+        })
+      })
+
+      describe('with a invalid image', function() {
+        beforeEach(function() {
+          this.DockerRunner.run(
+            this.project_id,
+            this.command,
+            this.directory,
+            'something/different:evil',
+            this.timeout,
+            this.env,
+            this.compileGroup,
+            this.callback
+          )
+        })
+
+        it('should call the callback with an error', function() {
+          const err = new Error('image not allowed')
+          this.callback.called.should.equal(true)
+          this.callback.args[0][0].message.should.equal(err.message)
+        })
+
+        it('should not setup the container', function() {
+          this.DockerRunner._getContainerOptions.called.should.equal(false)
+        })
+      })
+    })
   })
 
   describe('run with _getOptions', function() {

services/clsi/test/unit/js/RequestParserTests.js

@@ -114,6 +114,48 @@ describe('RequestParser', function() {
     })
   })
 
+  describe('when image restrictions are present', function() {
+    beforeEach(function() {
+      this.settings.clsi = { docker: {} }
+      this.settings.clsi.docker.allowedImages = [
+        'repo/name:tag1',
+        'repo/name:tag2'
+      ]
+    })
+
+    describe('with imageName set to something invalid', function() {
+      beforeEach(function() {
+        const request = this.validRequest
+        request.compile.options.imageName = 'something/different:latest'
+        this.RequestParser.parse(request, (error, data) => {
+          this.error = error
+          this.data = data
+        })
+      })
+
+      it('should throw an error for imageName', function() {
+        expect(String(this.error)).to.include(
+          'imageName attribute should be one of'
+        )
+      })
+    })
+
+    describe('with imageName set to something valid', function() {
+      beforeEach(function() {
+        const request = this.validRequest
+        request.compile.options.imageName = 'repo/name:tag1'
+        this.RequestParser.parse(request, (error, data) => {
+          this.error = error
+          this.data = data
+        })
+      })
+
+      it('should set the imageName', function() {
+        this.data.imageName.should.equal('repo/name:tag1')
+      })
+    })
+  })
+
   describe('with flags set', function() {
     beforeEach(function() {
       this.validRequest.compile.options.flags = ['-file-line-error']