From 6c2af3c6bcd5dd0748ff5663f985a37501d64b2f Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Wed, 10 Feb 2021 14:15:28 +0100 Subject: [PATCH] fix: add middleware verifying content type - By default only accepts 'application/json' - Routes that need different content-type, can call post or put with additional arguments, one per content-type you need to support. --- lib/middleware/content_type_checker.js | 30 ++++++++++ lib/middleware/content_type_checker.test.js | 63 +++++++++++++++++++++ lib/routes/admin-api/archive.test.js | 9 ++- lib/routes/admin-api/feature.test.js | 34 +++++++++++ lib/routes/admin-api/strategy.test.js | 5 ++ lib/routes/client-api/register.test.js | 5 +- lib/routes/controller.js | 7 ++- package.json | 6 +- test/e2e/api/admin/strategy.e2e.test.js | 8 ++- 9 files changed, 157 insertions(+), 10 deletions(-) create mode 100644 lib/middleware/content_type_checker.js create mode 100644 lib/middleware/content_type_checker.test.js diff --git a/lib/middleware/content_type_checker.js b/lib/middleware/content_type_checker.js new file mode 100644 index 0000000000..3cfef8448c --- /dev/null +++ b/lib/middleware/content_type_checker.js @@ -0,0 +1,30 @@ +const DEFAULT_ACCEPTED_CONTENT_TYPE = 'application/json'; + +/** + * Builds an express middleware checking the content-type header + * returning 415 if the header is not either `application/json` or in the array + * passed into the function of valid content-types + * @param {String} acceptedContentTypes + * @returns {function(Request, Response, NextFunction): void} + */ +function requireContentType(...acceptedContentTypes) { + return (req, res, next) => { + const contentType = req.header('Content-Type'); + if ( + Array.isArray(acceptedContentTypes) && + acceptedContentTypes.length > 0 + ) { + if (acceptedContentTypes.includes(contentType)) { + next(); + } else { + res.status(415).end(); + } + } else if (DEFAULT_ACCEPTED_CONTENT_TYPE === contentType) { + next(); + } else { + res.status(415).end(); + } + }; +} + +module.exports = requireContentType; diff --git a/lib/middleware/content_type_checker.test.js b/lib/middleware/content_type_checker.test.js new file mode 100644 index 0000000000..cfa6c14951 --- /dev/null +++ b/lib/middleware/content_type_checker.test.js @@ -0,0 +1,63 @@ +const test = require('ava'); + +const requireContentType = require('./content_type_checker'); + +const mockRequest = contentType => ({ + header: name => { + if (name === 'Content-Type') { + return contentType; + } + return ''; + }, +}); + +const returns415 = t => { + return { + status: code => { + t.is(415, code); + return { + end: t.pass, + }; + }, + }; +}; + +const expectNoCall = t => { + return { + status: () => { + return { + end: t.fail, + }; + }, + }; +}; + +test('Content-type middleware should by default only support application/json', t => { + const middleware = requireContentType(); + middleware(mockRequest('application/json'), expectNoCall(t), t.pass); + middleware(mockRequest('text/plain'), returns415(t), t.fail); +}); + +test('Content-type middleware should allow adding custom supported types', t => { + const middleware = requireContentType('application/yaml'); + middleware(mockRequest('application/yaml'), expectNoCall(t), t.pass); + middleware(mockRequest('text/html'), returns415(t), t.fail); + middleware(mockRequest('text/plain'), returns415(t), t.fail); +}); + +test('adding custom supported types no longer supports default type', t => { + const middleware = requireContentType('application/yaml'); + middleware(mockRequest('application/json'), returns415(t), t.fail); +}); + +test('Should be able to add multiple content-types supported', t => { + const middleware = requireContentType( + 'application/json', + 'application/yaml', + 'form/multipart', + ); + middleware(mockRequest('application/json'), expectNoCall(t), t.pass); + middleware(mockRequest('application/yaml'), expectNoCall(t), t.pass); + middleware(mockRequest('form/multipart'), expectNoCall(t), t.pass); + middleware(mockRequest('text/plain'), returns415(t), t.fail); +}); diff --git a/lib/routes/admin-api/archive.test.js b/lib/routes/admin-api/archive.test.js index c49473c01c..52d1e8a7c2 100644 --- a/lib/routes/admin-api/archive.test.js +++ b/lib/routes/admin-api/archive.test.js @@ -79,7 +79,10 @@ test('should revive toggle', t => { strategies: [{ name: 'default' }], }); - return request.post(`${base}/api/admin/archive/revive/${name}`).expect(200); + return request + .post(`${base}/api/admin/archive/revive/${name}`) + .set('Content-Type', 'application/json') + .expect(200); }); test('should create event when reviving toggle', async t => { @@ -108,7 +111,9 @@ test('should create event when reviving toggle', async t => { 'test@test.com', ); - await request.post(`${base}/api/admin/archive/revive/${name}`); + await request + .post(`${base}/api/admin/archive/revive/${name}`) + .set('Content-Type', 'application/json'); const events = await eventStore.getEvents(); t.is(events.length, 3); diff --git a/lib/routes/admin-api/feature.test.js b/lib/routes/admin-api/feature.test.js index 611fafd955..7e959e11a7 100644 --- a/lib/routes/admin-api/feature.test.js +++ b/lib/routes/admin-api/feature.test.js @@ -110,6 +110,20 @@ test('should be allowed to use new toggle name', t => { .expect(200); }); +test('should get unsupported media-type when posting as form-url-encoded', t => { + t.plan(0); + const { request, base, perms } = getSetup(); + perms.withPermissions(CREATE_FEATURE); + + return request + .post(`${base}/api/admin/features`) + .type('form') + .send({ name: 'new.name' }) + .send({ enabled: true }) + .send({ strategies: [{ name: 'default' }] }) + .expect(415); +}); + test('should be allowed to have variants="null"', t => { t.plan(0); const { request, base, perms } = getSetup(); @@ -185,6 +199,23 @@ test('should require at least one strategy when updating a feature toggle', t => .expect(400); }); +test('updating a feature toggle also requires application/json as content-type', t => { + t.plan(0); + const { request, featureToggleStore, base, perms } = getSetup(); + perms.withPermissions(UPDATE_FEATURE); + featureToggleStore.createFeature({ + name: 'ts', + strategies: [{ name: 'default' }], + }); + + return request + .put(`${base}/api/admin/features/ts`) + .type('form') + .send({ name: 'ts' }) + .send({ strategies: [{ name: 'default' }] }) + .expect(415); +}); + test('valid feature names should pass validation', t => { t.plan(0); const { request, base, perms } = getSetup(); @@ -320,6 +351,7 @@ test('should toggle on', t => { return request .post(`${base}/api/admin/features/toggle.disabled/toggle/on`) + .set('Content-Type', 'application/json') .expect('Content-Type', /json/) .expect(200) .expect(res => { @@ -340,6 +372,7 @@ test('should toggle off', t => { return request .post(`${base}/api/admin/features/toggle.enabled/toggle/off`) + .set('Content-Type', 'application/json') .expect('Content-Type', /json/) .expect(200) .expect(res => { @@ -360,6 +393,7 @@ test('should toggle', t => { return request .post(`${base}/api/admin/features/toggle.disabled/toggle`) + .set('Content-Type', 'application/json') .expect('Content-Type', /json/) .expect(200) .expect(res => { diff --git a/lib/routes/admin-api/strategy.test.js b/lib/routes/admin-api/strategy.test.js index a99677ef51..2efe1cc56c 100644 --- a/lib/routes/admin-api/strategy.test.js +++ b/lib/routes/admin-api/strategy.test.js @@ -206,6 +206,7 @@ test('deprecating a strategy works', async t => { await request .post(`${base}/api/admin/strategies/${name}/deprecate`) + .set('Content-Type', 'application/json') .send() .expect(200); return request @@ -220,6 +221,7 @@ test('deprecating a non-existent strategy yields 404', t => { perms.withPermissions(UPDATE_STRATEGY); return request .post(`${base}/api/admin/strategies/non-existent-strategy/deprecate`) + .set('Content-Type', 'application/json') .expect(404); }); @@ -232,6 +234,7 @@ test('reactivating a strategy works', async t => { await request .post(`${base}/api/admin/strategies/${name}/reactivate`) + .set('Content-Type', 'application/json') .send() .expect(200); return request @@ -246,6 +249,7 @@ test('reactivating a non-existent strategy yields 404', t => { perms.withPermissions(UPDATE_STRATEGY); return request .post(`${base}/api/admin/strategies/non-existent-strategy/reactivate`) + .set('Content-Type', 'application/json') .expect(404); }); test(`deprecating 'default' strategy will yield 403`, t => { @@ -254,5 +258,6 @@ test(`deprecating 'default' strategy will yield 403`, t => { perms.withPermissions(UPDATE_STRATEGY); return request .post(`${base}/api/admin/strategies/default/deprecate`) + .set('Content-Type', 'application/json') .expect(403); }); diff --git a/lib/routes/client-api/register.test.js b/lib/routes/client-api/register.test.js index 230b4fd403..785584dd27 100644 --- a/lib/routes/client-api/register.test.js +++ b/lib/routes/client-api/register.test.js @@ -65,7 +65,10 @@ test('should register client without sdkVersin', t => { test('should require appName field', t => { t.plan(0); const { request } = getSetup(); - return request.post('/api/client/register').expect(400); + return request + .post('/api/client/register') + .set('Content-Type', 'application/json') + .expect(400); }); test('should require strategies field', t => { diff --git a/lib/routes/controller.js b/lib/routes/controller.js index 6bfa1a3de5..ab839af1de 100644 --- a/lib/routes/controller.js +++ b/lib/routes/controller.js @@ -2,6 +2,7 @@ const { Router } = require('express'); const checkPermission = require('../middleware/permission-checker'); +const requireContentType = require('../middleware/content_type_checker'); /** * Base class for Controllers to standardize binding to express Router. */ @@ -20,18 +21,20 @@ class Controller { ); } - post(path, handler, permission) { + post(path, handler, permission, ...acceptedContentTypes) { this.app.post( path, checkPermission(this.config, permission), + requireContentType(...acceptedContentTypes), handler.bind(this), ); } - put(path, handler, permission) { + put(path, handler, permission, ...acceptedContentTypes) { this.app.put( path, checkPermission(this.config, permission), + requireContentType(...acceptedContentTypes), handler.bind(this), ); } diff --git a/package.json b/package.json index 4bb1754fa7..090270ba41 100644 --- a/package.json +++ b/package.json @@ -128,12 +128,10 @@ }, "lint-staged": { "*.js": [ - "eslint --fix", - "git add" + "eslint --fix" ], "*.{json,css,md}": [ - "prettier --write", - "git add" + "prettier --write" ] }, "husky": { diff --git a/test/e2e/api/admin/strategy.e2e.test.js b/test/e2e/api/admin/strategy.e2e.test.js index 576ee0c53e..0e438908f1 100644 --- a/test/e2e/api/admin/strategy.e2e.test.js +++ b/test/e2e/api/admin/strategy.e2e.test.js @@ -130,6 +130,7 @@ test.serial('deprecating a strategy works', async t => { .expect(201); await request .post(`/api/admin/strategies/${name}/deprecate`) + .set('Content-Type', 'application/json') .send() .expect(200); await request @@ -149,6 +150,7 @@ test.serial('can reactivate a deprecated strategy', async t => { .expect(201); await request .post(`/api/admin/strategies/${name}/deprecate`) + .set('Content-Type', 'application/json') .send() .expect(200); await request @@ -158,6 +160,7 @@ test.serial('can reactivate a deprecated strategy', async t => { .expect(res => t.is(res.body.deprecated, true)); await request .post(`/api/admin/strategies/${name}/reactivate`) + .set('Content-Type', 'application/json') .send() .expect(200); await request @@ -170,7 +173,10 @@ test.serial('can reactivate a deprecated strategy', async t => { test.serial('cannot deprecate default strategy', async t => { t.plan(0); const request = await setupApp(stores); - await request.post('/api/admin/strategies/default/deprecate').expect(403); + await request + .post('/api/admin/strategies/default/deprecate') + .set('Content-Type', 'application/json') + .expect(403); }); test.serial('can update a exiting strategy with deprecated', async t => {