[misc] apply review feedback

- move setting into clsi.docker namespace
- rename the variable for images to allowedImages / ALLOWED_IMAGES
- add an additional check for the image name into the DockerRunner

Co-Authored-By: Brian Gough <brian.gough@overleaf.com>

services/clsi/app/js/CompileController.js

@@ -220,8 +220,10 @@ module.exports = CompileController = {
     const { image } = req.query
     if (
       image &&
-      Settings.allowedImageNamesFlat &&
+      Settings.clsi &&
-      Settings.allowedImageNamesFlat.indexOf(image) === -1
+      Settings.clsi.docker &&
+      Settings.clsi.docker.allowedImages &&
+      !Settings.clsi.docker.allowedImages.includes(image)
     ) {
       return res.status(400).send('invalid image')
     }

services/clsi/app/js/DockerRunner.js

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

services/clsi/app/js/RequestParser.js

@@ -61,7 +61,13 @@ module.exports = RequestParser = {
       response.imageName = this._parseAttribute(
         'imageName',
         compile.options.imageName,
-        { type: 'string', validValues: settings.allowedImageNamesFlat }
+        {
+          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

@@ -73,16 +73,6 @@ if (process.env.ALLOWED_COMPILE_GROUPS) {
     process.exit(1)
   }
 }
-if (process.env.ALLOWED_IMAGE_NAMES_FLAT) {
-  try {
-    module.exports.allowedImageNamesFlat = process.env.ALLOWED_IMAGE_NAMES_FLAT.split(
-      ' '
-    )
-  } catch (error) {
-    console.error(error, 'could not apply allowed image names setting')
-    process.exit(1)
-  }
-}
 
 if (process.env.DOCKER_RUNNER) {
   let seccompProfilePath
@@ -139,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,7 +3,7 @@ version: "2.3"
 services:
   dev:
     environment:
-      ALLOWED_IMAGE_NAMES_FLAT: "quay.io/sharelatex/texlive-full:2017.1"
+      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
@@ -19,7 +19,7 @@ services:
       
   ci:
     environment:
-      ALLOWED_IMAGE_NAMES_FLAT: ${TEXLIVE_IMAGE}
+      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/unit/js/CompileControllerTests.js

@@ -306,9 +306,10 @@ describe('CompileController', function() {
         .should.equal(true)
     })
 
-    describe('when allowedImageNamesFlat is set', function() {
+    describe('when allowedImages is set', function() {
       beforeEach(function() {
-        this.Settings.allowedImageNamesFlat = [
+        this.Settings.clsi = { docker: {} }
+        this.Settings.clsi.docker.allowedImages = [
           'repo/image:tag1',
           'repo/image:tag2'
         ]

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

@@ -116,7 +116,11 @@ describe('RequestParser', function() {
 
   describe('when image restrictions are present', function() {
     beforeEach(function() {
-      this.settings.allowedImageNamesFlat = ['repo/name:tag1', 'repo/name:tag2']
+      this.settings.clsi = { docker: {} }
+      this.settings.clsi.docker.allowedImages = [
+        'repo/name:tag1',
+        'repo/name:tag2'
+      ]
     })
 
     describe('with imageName set to something invalid', function() {