From 8846efe7ce08d39906db6b7a54321b998bc77a89 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 26 Jun 2020 12:29:49 +0100 Subject: [PATCH] [misc] RequestParser: restrict imageName to an allow list and add tests --- services/clsi/app/js/RequestParser.js | 2 +- services/clsi/config/settings.defaults.js | 10 +++ services/clsi/docker-compose-config.yml | 2 + .../test/acceptance/js/AllowedImageNames.js | 73 +++++++++++++++++++ .../clsi/test/unit/js/RequestParserTests.js | 38 ++++++++++ 5 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 services/clsi/test/acceptance/js/AllowedImageNames.js diff --git a/services/clsi/app/js/RequestParser.js b/services/clsi/app/js/RequestParser.js index f2be556af1..ecc92c0836 100644 --- a/services/clsi/app/js/RequestParser.js +++ b/services/clsi/app/js/RequestParser.js @@ -61,7 +61,7 @@ module.exports = RequestParser = { response.imageName = this._parseAttribute( 'imageName', compile.options.imageName, - { type: 'string' } + { type: 'string', validValues: settings.allowedImageNamesFlat } ) response.draft = this._parseAttribute('draft', compile.options.draft, { default: false, diff --git a/services/clsi/config/settings.defaults.js b/services/clsi/config/settings.defaults.js index 3e115e8bf5..fd2c28af1e 100644 --- a/services/clsi/config/settings.defaults.js +++ b/services/clsi/config/settings.defaults.js @@ -73,6 +73,16 @@ 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 diff --git a/services/clsi/docker-compose-config.yml b/services/clsi/docker-compose-config.yml index 392f8feae9..d1b72ee216 100644 --- a/services/clsi/docker-compose-config.yml +++ b/services/clsi/docker-compose-config.yml @@ -3,6 +3,7 @@ version: "2.3" services: dev: environment: + ALLOWED_IMAGE_NAMES_FLAT: "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_IMAGE_NAMES_FLAT: ${TEXLIVE_IMAGE} TEXLIVE_IMAGE: quay.io/sharelatex/texlive-full:2017.1 TEXLIVE_IMAGE_USER: "tex" SHARELATEX_CONFIG: /app/config/settings.defaults.coffee diff --git a/services/clsi/test/acceptance/js/AllowedImageNames.js b/services/clsi/test/acceptance/js/AllowedImageNames.js new file mode 100644 index 0000000000..1ea0a36b64 --- /dev/null +++ b/services/clsi/test/acceptance/js/AllowedImageNames.js @@ -0,0 +1,73 @@ +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 + }) + }) +}) diff --git a/services/clsi/test/unit/js/RequestParserTests.js b/services/clsi/test/unit/js/RequestParserTests.js index e2d8b02610..16955bb670 100644 --- a/services/clsi/test/unit/js/RequestParserTests.js +++ b/services/clsi/test/unit/js/RequestParserTests.js @@ -114,6 +114,44 @@ describe('RequestParser', function() { }) }) + describe('when image restrictions are present', function() { + beforeEach(function() { + this.settings.allowedImageNamesFlat = ['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']