From f0ff115ab0ed0ca520b6ffc6c01b30441ce0a0c5 Mon Sep 17 00:00:00 2001 From: Gareth Visagie Date: Wed, 21 Sep 2016 18:32:25 +0100 Subject: [PATCH 1/5] chore: pass config into relay This makes testing much easier, as we can pass in the config rather than relying on it being correct at the point that relay was initially required (or having to resort to proxyquire...) --- lib/client/index.js | 1 + lib/client/socket.js | 4 ++-- lib/relay.js | 3 +-- lib/server/index.js | 1 + lib/server/socket.js | 4 ++-- test/unit/relay-response.test.js | 10 ++++++---- 6 files changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/client/index.js b/lib/client/index.js index 3a09b1680..65d10d3e6 100644 --- a/lib/client/index.js +++ b/lib/client/index.js @@ -9,6 +9,7 @@ module.exports = ({ port = null, config = {}, filters = {} }) => { id: config.brokerId, url: config.brokerUrl, filters: filters.private, + config, }); // start the local webserver to listen for relay requests diff --git a/lib/client/socket.js b/lib/client/socket.js index 4cf110c1f..16b05683d 100644 --- a/lib/client/socket.js +++ b/lib/client/socket.js @@ -10,7 +10,7 @@ const Socket = Primus.createSocket({ } }); -module.exports = ({ url, id, filters }) => { +module.exports = ({ url, id, filters, config }) => { if (!id) { // null, undefined, empty, etc. debug('missing client id'); const error = new ReferenceError('BROKER_ID is required to successfully identify itself to the server'); @@ -29,7 +29,7 @@ module.exports = ({ url, id, filters }) => { debug('connecting to %s', url); - const response = relay.response(filters); + const response = relay.response(filters, config); // RS note: this bind doesn't feel right, it feels like a sloppy way of // getting the filters into the request function. diff --git a/lib/relay.js b/lib/relay.js index 0d3c97b81..0f0fe76f5 100644 --- a/lib/relay.js +++ b/lib/relay.js @@ -4,7 +4,6 @@ const parse = require('url').parse; const format = require('url').format; const Filters = require('./filters'); const replace = require('./replace-vars'); -const config = require('./config'); module.exports = { request: requestHandler, @@ -40,7 +39,7 @@ function requestHandler(filterRules) { }; } -function responseHandler(filterRules) { +function responseHandler(filterRules, config) { const filters = Filters(filterRules); const debug = require('debug')('broker:' + (process.env.BROKER_TYPE || 'relay')); diff --git a/lib/server/index.js b/lib/server/index.js index dc3b190ea..f66fff67c 100644 --- a/lib/server/index.js +++ b/lib/server/index.js @@ -12,6 +12,7 @@ module.exports = ({ config = {}, port = null, filters = {} }) => { const { io, connections } = socket({ server, filters: filters.private, + config, }); app.all('/broker/:id/*', (req, res, next) => { diff --git a/lib/server/socket.js b/lib/server/socket.js index 574742157..efae631a3 100644 --- a/lib/server/socket.js +++ b/lib/server/socket.js @@ -3,12 +3,12 @@ const Emitter = require('primus-emitter'); const debug = require('debug')('broker:server'); const relay = require('../relay'); -module.exports = ({ server, filters }) => { +module.exports = ({ server, filters, config }) => { const io = new Primus(server, { transformer: 'engine.io', parser: 'JSON' }); io.plugin('emitter', Emitter); const connections = new Map(); - const response = relay.response(filters); + const response = relay.response(filters, config); io.on('error', error => console.error(error.stack)); io.on('offline', () => console.error('Internet access has gone offline')); diff --git a/test/unit/relay-response.test.js b/test/unit/relay-response.test.js index dd7dfe8ad..fe8293462 100644 --- a/test/unit/relay-response.test.js +++ b/test/unit/relay-response.test.js @@ -10,8 +10,10 @@ tap.beforeEach(done => { }); test('relay swaps values found in BROKER_VAR_SUB', t => { - process.env.HOST = 'localhost'; - process.env.PORT = '8001'; + const config = { + HOST: 'localhost', + PORT: '8001', + }; const relay = proxyquire('../../lib/relay', { 'request': (options, fn) => { @@ -23,7 +25,7 @@ test('relay swaps values found in BROKER_VAR_SUB', t => { const route = relay([{ method: 'any', url: '/*' - }]); + }], config); route({ url: '/', @@ -36,7 +38,7 @@ test('relay swaps values found in BROKER_VAR_SUB', t => { }, () => { t.equal(spy.callCount, 1, 'request placed'); const arg = spy.args[0][0]; - t.equal(arg.body.url, `${process.env.HOST}:${process.env.PORT}/webhook`); + t.equal(arg.body.url, `${config.HOST}:${config.PORT}/webhook`); t.end(); }); From 3479aab1b8ff7378b743228363b2a733d3e3227c Mon Sep 17 00:00:00 2001 From: Gareth Visagie Date: Tue, 20 Sep 2016 13:54:43 +0100 Subject: [PATCH 2/5] test: overhaul functional tests Updates the functional tests to better describe full end-to-end proxying of requests, in both directions. Also overhauls the healthcheck test based on changes to the functional test. --- test/fixtures/client/filters.json | 26 +++--- test/fixtures/server/filters.json | 21 +++-- test/functional/client-server.test.js | 52 +++++++++--- test/functional/healthcheck.test.js | 85 ++++++++++--------- test/functional/no-filter.test.js | 11 +-- .../{index.test.js => server-client.test.js} | 67 ++++++++++----- test/utils.js | 21 +++-- 7 files changed, 177 insertions(+), 106 deletions(-) rename test/functional/{index.test.js => server-client.test.js} (52%) diff --git a/test/fixtures/client/filters.json b/test/fixtures/client/filters.json index 06423af00..17af12d8b 100644 --- a/test/fixtures/client/filters.json +++ b/test/fixtures/client/filters.json @@ -1,21 +1,27 @@ { "private": [ { - "path": "/magic-path/${secret}/package.json", + "path": "/echo-param/${param}", + "method": "GET", + "origin": "http://localhost:${originPort}" + }, + + { + "path": "/echo-body", "method": "POST", - "origin": "http://localhost:${port}" + "origin": "http://localhost:${originPort}" } ], "public": [ { - "path": "/magic-path/${secret}/package.json", - "method": "POST", - "origin": "http://localhost:${port}" + "path": "/echo-param/${param}", + "method": "GET" }, - { - "path": "/service/:package", - "method": "GET", - "origin": "http://localhost:${port}" - } + + { + "path": "/echo-body", + "method": "POST" + } + ] } diff --git a/test/fixtures/server/filters.json b/test/fixtures/server/filters.json index b2db9969b..17af12d8b 100644 --- a/test/fixtures/server/filters.json +++ b/test/fixtures/server/filters.json @@ -1,24 +1,27 @@ { "private": [ { - "path": "/service/:package", + "path": "/echo-param/${param}", "method": "GET", - "origin": "http://localhost:${port}" + "origin": "http://localhost:${originPort}" + }, + + { + "path": "/echo-body", + "method": "POST", + "origin": "http://localhost:${originPort}" } ], "public": [ { - "path": "/service/:package", - "method": "GET", - "origin": "http://localhost:${port}" + "path": "/echo-param/${param}", + "method": "GET" }, { - "path": "/magic-path/${secret}/package.json", - "method": "POST", - "origin": "http://localhost:${port}" + "path": "/echo-body", + "method": "POST" } ] - } diff --git a/test/functional/client-server.test.js b/test/functional/client-server.test.js index e8de55771..fd47e281b 100644 --- a/test/functional/client-server.test.js +++ b/test/functional/client-server.test.js @@ -6,36 +6,62 @@ const request = require('request'); const app = require('../../lib'); const root = __dirname; -const { port, localPort: servicePort } = require('../utils')(tap); +const { port, echoServerPort } = require('../utils')(tap); -test('internal sends request through client', t => { +test('proxy requests originating from behind the broker client', t => { + /** + * 1. start broker in server mode + * 2. start broker in client mode and join (1) + * 3. run local http server that replicates "private server" + * 4. send requests to **client** + * + * Note: server is forwarding requests to echo-server defined in test/util.js + */ - // same setup as normal - process.chdir(path.resolve(root, '../fixtures/server')); process.env.ACCEPT = 'filters.json'; - process.env.PORT = servicePort; + + process.chdir(path.resolve(root, '../fixtures/server')); process.env.BROKER_TYPE = 'server'; + process.env.ORIGIN_PORT = echoServerPort; const serverPort = port(); const server = app.main({ port: serverPort }); process.chdir(path.resolve(root, '../fixtures/client')); - process.env.BROKER_URL = `http://localhost:${serverPort}`; - process.env.BROKER_ID = '12345'; process.env.BROKER_TYPE = 'client'; + process.env.BROKER_ID = '12345'; + process.env.BROKER_URL = `http://localhost:${serverPort}`; const clientPort = port(); - // invalidate the config require const client = app.main({ port: clientPort }); // wait for the client to successfully connect to the server and identify itself server.io.once('connection', socket => { socket.once('identify', () => { - t.plan(2); + t.plan(4); + + t.test('successfully broker POST', t => { + const url = `http://localhost:${clientPort}/echo-body`; + const body = { some: { example: 'json' }}; + request({ url, method: 'post', json: true, body }, (err, res) => { + t.equal(res.statusCode, 200, '200 statusCode'); + t.same(res.body, body, 'body brokered'); + t.end(); + }); + }); - t.test('client can forward requests FROM internal service', t => { - const url = `http://localhost:${clientPort}/service/test1`; - request({ url, method: 'get', json: true }, (err, res) => { + t.test('successfully broker GET', t => { + const url = `http://localhost:${clientPort}/echo-param/xyz`; + request({ url, method: 'get' }, (err, res) => { t.equal(res.statusCode, 200, '200 statusCode'); - t.equal(res.body, 'test1', 'body correct'); + t.equal(res.body, 'xyz', 'body brokered'); + t.end(); + }); + }); + + t.test('block invalid request', t => { + const url = `http://localhost:${clientPort}/not-allowed`; + request({ url, 'method': 'post', json: true }, (err, res, body) => { + t.equal(res.statusCode, 401, '401 statusCode'); + t.equal(body, 'blocked', '"blocked" body: ' + body); t.end(); }); }); diff --git a/test/functional/healthcheck.test.js b/test/functional/healthcheck.test.js index 592f337a4..34a947f18 100644 --- a/test/functional/healthcheck.test.js +++ b/test/functional/healthcheck.test.js @@ -3,61 +3,66 @@ const test = require('tap-only'); const path = require('path'); const request = require('request'); const app = require('../../lib'); +const root = __dirname; -const { port, localPort } = require('../utils')(tap); +const { port } = require('../utils')(tap); -test('server healthcheck', t => { +test('proxy requests originating from behind the broker client', t => { /** * 1. start broker in server mode - * 2. send healthcheck request to server, assert HTTP 200 and `{ ok: true }` + * 2. start broker in client mode and join (1) + * 3. check /healthcheck on client and server */ - const root = __dirname; + process.env.ACCEPT = 'filters.json'; process.chdir(path.resolve(root, '../fixtures/server')); - const serverPort = port(); process.env.BROKER_TYPE = 'server'; - process.env.ACCEPT = 'filters.json'; + const serverPort = port(); const server = app.main({ port: serverPort }); - const url = `http://localhost:${serverPort}/healthcheck`; - request({url, json: true }, (err, res) => { - if (err) { - t.fail(err); - } + process.chdir(path.resolve(root, '../fixtures/client')); + process.env.BROKER_TYPE = 'client'; + process.env.BROKER_ID = '12345'; + process.env.BROKER_URL = `http://localhost:${serverPort}`; + const clientPort = port(); + const client = app.main({ port: clientPort }); + + t.plan(3); + + t.test('server healthcheck', t => { + const url = `http://localhost:${serverPort}/healthcheck`; + request({url, json: true }, (err, res) => { + if (err) { return t.threw(err); } - t.equal(res.statusCode, 200, '200 statusCode'); - t.equal(res.body['ok'], true, '{ ok: true } in body'); - server.close(); - t.end(); + t.equal(res.statusCode, 200, '200 statusCode'); + t.equal(res.body['ok'], true, '{ ok: true } in body'); + t.end(); + }); }); -}); -test('client healthcheck', t => { - /** - * 1. start broker in client mode - * 2. send healthcheck request to server, assert HTTP 200 and `{ ok: true }` - */ + // wait for the client to successfully connect to the server and identify itself + server.io.once('connection', socket => { + socket.once('identify', () => { + t.test('client healthcheck', t => { + const url = `http://localhost:${clientPort}/healthcheck`; + request({url, json: true }, (err, res) => { + if (err) { return t.threw(err); } - const root = __dirname; + t.equal(res.statusCode, 200, '200 statusCode'); + t.equal(res.body['ok'], true, '{ ok: true } in body'); + t.end(); + }); + }); - process.chdir(path.resolve(root, '../fixtures/client')); - const serverPort = port(); - process.env.SECRET = 'secret'; - process.env.PORT = localPort; - process.env.ACCEPT = 'filters.json'; - process.env.BROKER_TYPE = 'client'; - const client = app.main({ port: port() }); - - const url = `http://localhost:${localPort}/healthcheck`; - request({url, json: true }, (err, res) => { - if (err) { - t.fail(err); - } - - t.equal(res.statusCode, 200, '200 statusCode'); - t.equal(res.body['ok'], true, '{ ok: true } in body'); - client.close(); - t.end(); + t.test('clean up', t => { + client.close(); + setTimeout(() => { + server.close(); + t.ok('sockets closed'); + t.end(); + }, 100); + }); + }); }); }); diff --git a/test/functional/no-filter.test.js b/test/functional/no-filter.test.js index d98da45e4..641a778c3 100644 --- a/test/functional/no-filter.test.js +++ b/test/functional/no-filter.test.js @@ -5,7 +5,7 @@ const path = require('path'); const request = require('request'); const app = require('../../lib'); -const { port, localPort } = require('../utils')(tap); +const { port, echoServerPort } = require('../utils')(tap); test('no filters broker', t => { /** @@ -16,7 +16,7 @@ test('no filters broker', t => { */ const root = __dirname; - process.env.ACCEPT = ''; + process.env.ACCEPT = ''; // no filters provided! process.chdir(path.resolve(root, '../fixtures/server')); const serverPort = port(); @@ -24,7 +24,7 @@ test('no filters broker', t => { process.chdir(path.resolve(root, '../fixtures/client')); process.env.SECRET = 'secret'; - process.env.PORT = localPort; + process.env.ORIGIN_PORT = echoServerPort; process.env.BROKER_URL = `http://localhost:${serverPort}`; process.env.BROKER_ID = '12345'; const client = app.main({ port: port() }); @@ -35,10 +35,11 @@ test('no filters broker', t => { t.plan(2); t.test('successfully broker with no filter should reject', t => { - const url = `http://localhost:${serverPort}/broker/${id}/magic-path/x/package.json`; + const url = `http://localhost:${serverPort}/broker/${id}/echo-body`; + const body = { test: 'body' }; request({ url, method: 'post', json: true }, (err, res) => { t.equal(res.statusCode, 401, '401 statusCode'); - t.notEqual(res.body, true, 'body not true'); + t.notSame(res.body, body, 'body not echoed'); t.end(); }); }); diff --git a/test/functional/index.test.js b/test/functional/server-client.test.js similarity index 52% rename from test/functional/index.test.js rename to test/functional/server-client.test.js index 74d7a1c78..7107f9877 100644 --- a/test/functional/index.test.js +++ b/test/functional/server-client.test.js @@ -4,51 +4,78 @@ const test = require('tap-only'); const path = require('path'); const request = require('request'); const app = require('../../lib'); +const root = __dirname; -const { port, localPort } = require('../utils')(tap); +const { port, echoServerPort } = require('../utils')(tap); -test('simple end to end proxying', t => { +test('proxy requests originating from behind the broker server', t => { /** * 1. start broker in server mode * 2. start broker in client mode and join (1) - * 3. run local http server that replicates "private serevr" - * 4. send request to server for X file + * 3. run local http server that replicates "private server" + * 4. send requests to **server** + * + * Note: client is forwarding requests to echo-server defined in test/util.js */ - const root = __dirname; + process.env.ACCEPT = 'filters.json'; process.chdir(path.resolve(root, '../fixtures/server')); - const serverPort = port(); process.env.BROKER_TYPE = 'server'; - process.env.ACCEPT = 'filters.json'; + const serverPort = port(); const server = app.main({ port: serverPort }); process.chdir(path.resolve(root, '../fixtures/client')); - process.env.SECRET = 'secret'; - process.env.PORT = localPort; - process.env.ACCEPT = 'filters.json'; - process.env.BROKER_URL = `http://localhost:${serverPort}`; - process.env.BROKER_ID = '12345'; process.env.BROKER_TYPE = 'client'; + process.env.BROKER_ID = '12345'; + process.env.BROKER_URL = `http://localhost:${serverPort}`; + process.env.ORIGIN_PORT = echoServerPort; const client = app.main({ port: port() }); // wait for the client to successfully connect to the server and identify itself server.io.on('connection', socket => { socket.on('identify', id => { + t.plan(6); - t.plan(4); + t.test('successfully broker POST', t => { + const url = `http://localhost:${serverPort}/broker/${id}/echo-body`; + const body = { some: { example: 'json' }}; + request({ url, method: 'post', json: true, body }, (err, res) => { + t.equal(res.statusCode, 200, '200 statusCode'); + t.same(res.body, body, 'body brokered'); + t.end(); + }); + }); - t.test('successfully broker', t => { - const url = `http://localhost:${serverPort}/broker/${id}/magic-path/x/package.json`; - request({ url, method: 'post', json: true }, (err, res) => { + t.test('successfully broker GET', t => { + const url = `http://localhost:${serverPort}/broker/${id}/echo-param/xyz`; + request({ url, method: 'get' }, (err, res) => { t.equal(res.statusCode, 200, '200 statusCode'); - t.equal(res.body, true, 'body true'); + t.equal(res.body, 'xyz', 'body brokered'); t.end(); }); }); - t.test('filtered request to broker', t => { - const url = `http://localhost:${serverPort}/broker/${id}/magic-path/x/random.json`; + // the variable substitution takes place in the broker client + t.test('variable subsitution', t => { + const url = `http://localhost:${serverPort}/broker/${id}/echo-body`; + const body = { + BROKER_VAR_SUB: ['swap.me'], + swap: { me: '${BROKER_TYPE}:${BROKER_ID}' }, + }; + request({ url, method: 'post', json: true, body }, (err, res) => { + const swappedBody = { + BROKER_VAR_SUB: ['swap.me'], + swap: { me: 'client:12345' }, + }; + t.equal(res.statusCode, 200, '200 statusCode'); + t.same(res.body, swappedBody, 'body brokered'); + t.end(); + }); + }) + + t.test('block invalid request', t => { + const url = `http://localhost:${serverPort}/broker/${id}/not-allowed`; request({ url, 'method': 'post', json: true }, (err, res, body) => { t.equal(res.statusCode, 401, '401 statusCode'); t.equal(body, 'blocked', '"blocked" body: ' + body); @@ -57,7 +84,7 @@ test('simple end to end proxying', t => { }); t.test('bad broker id', t => { - const url = `http://localhost:${serverPort}/broker/${id}XXX/magic-path/x/random.json`; + const url = `http://localhost:${serverPort}/broker/${id}XXX/echo-body`; request({ url, 'method': 'post', json: true }, (err, res) => { t.equal(res.statusCode, 404, '404 statusCode'); t.end(); diff --git a/test/utils.js b/test/utils.js index e14c4652c..667bdcd0a 100644 --- a/test/utils.js +++ b/test/utils.js @@ -7,21 +7,24 @@ function port() { } // this is our fake local and private web server -const localPort = port(); -const { app:localServer, server } = webserver({ - http: true, - port: localPort, +const echoServerPort = port(); +const { app: echoServer, server } = webserver({ + port: echoServerPort, }); -localServer.get('/service/:param', (req, res) => { +echoServer.get('/echo-param/:param', (req, res) => { res.send(req.params.param); }); -localServer.post('/magic-path/secret/package.json', (req, res) => { - res.send(true); +echoServer.post('/echo-body/:param?', (req, res) => { + const contentType = req.get('Content-Type'); + if (contentType) { + res.type(contentType); + } + res.send(req.body); }); -localServer.all('*', (req, res) => { +echoServer.all('*', (req, res) => { res.send(false); }); @@ -32,7 +35,7 @@ module.exports = (tap) => { }); return { - localPort, + echoServerPort, port, server }; From 16ab1b636fccf9bf9cc94e3f2868c9259fbd151f Mon Sep 17 00:00:00 2001 From: Gareth Visagie Date: Wed, 21 Sep 2016 17:19:02 +0100 Subject: [PATCH 3/5] test: add tests for body-based filtering The filters setup for the tests perform all filtering on the client, as that is our expected use-case. --- test/fixtures/client/filters.json | 26 +++++++++++++++++++++++++- test/fixtures/server/filters.json | 6 +++--- test/functional/client-server.test.js | 27 +++++++++++++++++++++++++-- test/functional/server-client.test.js | 27 +++++++++++++++++++++++++-- 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/test/fixtures/client/filters.json b/test/fixtures/client/filters.json index 17af12d8b..0f2dd35fc 100644 --- a/test/fixtures/client/filters.json +++ b/test/fixtures/client/filters.json @@ -10,6 +10,19 @@ "path": "/echo-body", "method": "POST", "origin": "http://localhost:${originPort}" + }, + + { + "path": "/echo-body/filtered", + "method": "POST", + "origin": "http://localhost:${originPort}", + "valid": [ + { + "//": "accept requests with 'proxy.*: please' in their body", + "path": "proxy.*", + "value": "please" + } + ] } ], "public": [ @@ -21,7 +34,18 @@ { "path": "/echo-body", "method": "POST" - } + }, + { + "path": "/echo-body/filtered", + "method": "POST", + "valid": [ + { + "//": "accept requests with 'proxy.*: please' in their body", + "path": "proxy.*", + "value": "please" + } + ] + } ] } diff --git a/test/fixtures/server/filters.json b/test/fixtures/server/filters.json index 17af12d8b..00c71dfa7 100644 --- a/test/fixtures/server/filters.json +++ b/test/fixtures/server/filters.json @@ -7,19 +7,19 @@ }, { - "path": "/echo-body", + "path": "/echo-body/:param?", "method": "POST", "origin": "http://localhost:${originPort}" } ], "public": [ { - "path": "/echo-param/${param}", + "path": "/*", "method": "GET" }, { - "path": "/echo-body", + "path": "/*", "method": "POST" } diff --git a/test/functional/client-server.test.js b/test/functional/client-server.test.js index fd47e281b..ec685a83b 100644 --- a/test/functional/client-server.test.js +++ b/test/functional/client-server.test.js @@ -36,7 +36,7 @@ test('proxy requests originating from behind the broker client', t => { // wait for the client to successfully connect to the server and identify itself server.io.once('connection', socket => { socket.once('identify', () => { - t.plan(4); + t.plan(6); t.test('successfully broker POST', t => { const url = `http://localhost:${clientPort}/echo-body`; @@ -57,7 +57,8 @@ test('proxy requests originating from behind the broker client', t => { }); }); - t.test('block invalid request', t => { + // the filtering happens in the broker client + t.test('block request for non-whitelisted url', t => { const url = `http://localhost:${clientPort}/not-allowed`; request({ url, 'method': 'post', json: true }, (err, res, body) => { t.equal(res.statusCode, 401, '401 statusCode'); @@ -66,6 +67,28 @@ test('proxy requests originating from behind the broker client', t => { }); }); + // the filtering happens in the broker client + t.test('allow request for valid url with valid body', t => { + const url = `http://localhost:${clientPort}/echo-body/filtered`; + const body = { proxy: { me: 'please' }}; + request({ url, method: 'post', json: true, body }, (err, res) => { + t.equal(res.statusCode, 200, '200 statusCode'); + t.same(res.body, body, 'body brokered'); + t.end(); + }); + }); + + // the filtering happens in the broker client + t.test('block request for valid url with invalid body', t => { + const url = `http://localhost:${clientPort}/echo-body/filtered`; + const body = { proxy: { me: 'now!' }}; + request({ url, 'method': 'post', json: true, body }, (err, res, body) => { + t.equal(res.statusCode, 401, '401 statusCode'); + t.equal(body, 'blocked', '"blocked" body: ' + body); + t.end(); + }); + }); + t.test('clean up', t => { client.close(); setTimeout(() => { diff --git a/test/functional/server-client.test.js b/test/functional/server-client.test.js index 7107f9877..251b4c5f1 100644 --- a/test/functional/server-client.test.js +++ b/test/functional/server-client.test.js @@ -35,7 +35,7 @@ test('proxy requests originating from behind the broker server', t => { // wait for the client to successfully connect to the server and identify itself server.io.on('connection', socket => { socket.on('identify', id => { - t.plan(6); + t.plan(8); t.test('successfully broker POST', t => { const url = `http://localhost:${serverPort}/broker/${id}/echo-body`; @@ -74,7 +74,8 @@ test('proxy requests originating from behind the broker server', t => { }); }) - t.test('block invalid request', t => { + // the filtering happens in the broker client + t.test('block request for non-whitelisted url', t => { const url = `http://localhost:${serverPort}/broker/${id}/not-allowed`; request({ url, 'method': 'post', json: true }, (err, res, body) => { t.equal(res.statusCode, 401, '401 statusCode'); @@ -83,6 +84,28 @@ test('proxy requests originating from behind the broker server', t => { }); }); + // the filtering happens in the broker client + t.test('allow request for valid url with valid body', t => { + const url = `http://localhost:${serverPort}/broker/${id}/echo-body/filtered`; + const body = { proxy: { me: 'please' }}; + request({ url, method: 'post', json: true, body }, (err, res) => { + t.equal(res.statusCode, 200, '200 statusCode'); + t.same(res.body, body, 'body brokered'); + t.end(); + }); + }); + + // the filtering happens in the broker client + t.test('block request for valid url with invalid body', t => { + const url = `http://localhost:${serverPort}/broker/${id}/echo-body/filtered`; + const body = { proxy: { me: 'now!' }}; + request({ url, 'method': 'post', json: true, body }, (err, res, body) => { + t.equal(res.statusCode, 401, '401 statusCode'); + t.equal(body, 'blocked', '"blocked" body: ' + body); + t.end(); + }); + }); + t.test('bad broker id', t => { const url = `http://localhost:${serverPort}/broker/${id}XXX/echo-body`; request({ url, 'method': 'post', json: true }, (err, res) => { From 0790c23b87ab42fca5dd5189dff1ef06ae600683 Mon Sep 17 00:00:00 2001 From: Gareth Visagie Date: Wed, 21 Sep 2016 17:26:41 +0100 Subject: [PATCH 4/5] test: test proxying exact bytes of POST bodies --- test/functional/client-server.test.js | 17 ++++++++++++++++- test/functional/server-client.test.js | 17 ++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/test/functional/client-server.test.js b/test/functional/client-server.test.js index ec685a83b..9fbea760a 100644 --- a/test/functional/client-server.test.js +++ b/test/functional/client-server.test.js @@ -36,7 +36,7 @@ test('proxy requests originating from behind the broker client', t => { // wait for the client to successfully connect to the server and identify itself server.io.once('connection', socket => { socket.once('identify', () => { - t.plan(6); + t.plan(7); t.test('successfully broker POST', t => { const url = `http://localhost:${clientPort}/echo-body`; @@ -48,6 +48,21 @@ test('proxy requests originating from behind the broker client', t => { }); }); + t.test('successfully broker exact bytes of POST body', t => { + const url = `http://localhost:${clientPort}/echo-body`; + // stringify the JSON unusually to ensure an unusual exact body + const body = Buffer.from( + JSON.stringify({ some: { example: 'json' }}, null, 5) + ); + const headers = { 'Content-Type': 'application/json' }; + request({ url, method: 'post', headers, body }, (err, res) => { + const responseBody = Buffer.from(res.body); + t.equal(res.statusCode, 200, '200 statusCode'); + t.same(responseBody, body, 'body brokered exactly'); + t.end(); + }); + }); + t.test('successfully broker GET', t => { const url = `http://localhost:${clientPort}/echo-param/xyz`; request({ url, method: 'get' }, (err, res) => { diff --git a/test/functional/server-client.test.js b/test/functional/server-client.test.js index 251b4c5f1..2bfa981e9 100644 --- a/test/functional/server-client.test.js +++ b/test/functional/server-client.test.js @@ -35,7 +35,7 @@ test('proxy requests originating from behind the broker server', t => { // wait for the client to successfully connect to the server and identify itself server.io.on('connection', socket => { socket.on('identify', id => { - t.plan(8); + t.plan(9); t.test('successfully broker POST', t => { const url = `http://localhost:${serverPort}/broker/${id}/echo-body`; @@ -47,6 +47,21 @@ test('proxy requests originating from behind the broker server', t => { }); }); + t.test('successfully broker exact bytes of POST body', t => { + const url = `http://localhost:${serverPort}/broker/${id}/echo-body`; + // stringify the JSON unusually to ensure an unusual exact body + const body = Buffer.from( + JSON.stringify({ some: { example: 'json' }}, null, 5) + ); + const headers = { 'Content-Type': 'application/json' }; + request({ url, method: 'post', headers, body }, (err, res) => { + const responseBody = Buffer.from(res.body); + t.equal(res.statusCode, 200, '200 statusCode'); + t.same(responseBody, body, 'body brokered exactly'); + t.end(); + }); + }); + t.test('successfully broker GET', t => { const url = `http://localhost:${serverPort}/broker/${id}/echo-param/xyz`; request({ url, method: 'get' }, (err, res) => { From 6629896ec8bd5e07ad998c266f5af613a4848f9d Mon Sep 17 00:00:00 2001 From: Gareth Visagie Date: Wed, 21 Sep 2016 20:34:03 +0100 Subject: [PATCH 5/5] fix: preserve exact bytes of request bodies (if possible) --- lib/client/socket.js | 2 +- lib/filters/index.js | 5 ++++- lib/relay.js | 26 +++++++++++++++++--------- lib/server/socket.js | 2 +- lib/try-json-parse.js | 7 +++++++ lib/webserver.js | 20 +++++++++++++++++++- package.json | 1 + test/unit/filters.test.js | 18 ++++++++++-------- test/unit/relay-response.test.js | 12 +++++++----- test/unit/try-json-parse.test.js | 25 +++++++++++++++++++++++++ 10 files changed, 92 insertions(+), 26 deletions(-) create mode 100644 lib/try-json-parse.js create mode 100644 test/unit/try-json-parse.test.js diff --git a/lib/client/socket.js b/lib/client/socket.js index 16b05683d..5f7d9f285 100644 --- a/lib/client/socket.js +++ b/lib/client/socket.js @@ -4,7 +4,7 @@ const relay = require('../relay'); const httpErrors = require('../http-errors'); const Socket = Primus.createSocket({ transformer: 'engine.io', - parser: 'JSON', + parser: 'EJSON', plugin: { 'emitter': require('primus-emitter'), } diff --git a/lib/filters/index.js b/lib/filters/index.js index 6f692f345..71adae324 100644 --- a/lib/filters/index.js +++ b/lib/filters/index.js @@ -1,6 +1,7 @@ const pathRegexp = require('path-to-regexp'); const undefsafe = require('undefsafe'); const replace = require('../replace-vars'); +const tryJSONParse = require('../try-json-parse'); // reads config that defines module.exports = ruleSource => { @@ -76,9 +77,11 @@ module.exports = ruleSource => { } if (valid && req.body) { + const parsedBody = tryJSONParse(req.body); + // validate against the body const isValid = valid.some(({ path, value }) => { - return undefsafe(req.body, path, value); + return undefsafe(parsedBody, path, value); }); if (!isValid) { diff --git a/lib/relay.js b/lib/relay.js index 0f0fe76f5..73e1d9f08 100644 --- a/lib/relay.js +++ b/lib/relay.js @@ -4,6 +4,7 @@ const parse = require('url').parse; const format = require('url').format; const Filters = require('./filters'); const replace = require('./replace-vars'); +const tryJSONParse = require('./try-json-parse'); module.exports = { request: requestHandler, @@ -33,7 +34,9 @@ function requestHandler(filterRules) { headers: req.headers, }, response => { debug('%s %s (%s)', req.method, req.url, response.status); - res.status(response.status).send(response.body); + res.status(response.status) + .set(response.headers) + .send(response.body); }); }); }; @@ -78,12 +81,17 @@ function responseHandler(filterRules, config) { // if the request is all good - and at this point it is, we'll check // whether we want to do variable substitution on the body - if (body && body.BROKER_VAR_SUB) { - debug('variable swap on ', body.BROKER_VAR_SUB); - for (const path of body.BROKER_VAR_SUB) { - let source = undefsafe(body, path); // get the value - source = replace(source, config); // replace the variables - undefsafe(body, path, source); // put it back in + if (body) { + const parsedBody = tryJSONParse(body); + + if (parsedBody.BROKER_VAR_SUB) { + debug('variable swap on ', parsedBody.BROKER_VAR_SUB); + for (const path of parsedBody.BROKER_VAR_SUB) { + let source = undefsafe(parsedBody, path); // get the value + source = replace(source, config); // replace the variables + undefsafe(parsedBody, path, source); // put it back in + } + body = JSON.stringify(parsedBody); } } @@ -101,7 +109,6 @@ function responseHandler(filterRules, config) { headers: headers, method, body, - json: true, }, (error, response, body) => { debug('%s %s (%s)', method, result, (response || { statusCode: 500 }).statusCode); @@ -116,7 +123,8 @@ function responseHandler(filterRules, config) { emit({ status: response.statusCode, - body: body + body: body, + headers: response.headers }); }); }); diff --git a/lib/server/socket.js b/lib/server/socket.js index efae631a3..842b37b5f 100644 --- a/lib/server/socket.js +++ b/lib/server/socket.js @@ -4,7 +4,7 @@ const debug = require('debug')('broker:server'); const relay = require('../relay'); module.exports = ({ server, filters, config }) => { - const io = new Primus(server, { transformer: 'engine.io', parser: 'JSON' }); + const io = new Primus(server, { transformer: 'engine.io', parser: 'EJSON' }); io.plugin('emitter', Emitter); const connections = new Map(); diff --git a/lib/try-json-parse.js b/lib/try-json-parse.js new file mode 100644 index 000000000..08b33caa5 --- /dev/null +++ b/lib/try-json-parse.js @@ -0,0 +1,7 @@ +module.exports = (data) => { + try { + return JSON.parse(Buffer.from(data)); + } catch (e) { + return {}; + } +} diff --git a/lib/webserver.js b/lib/webserver.js index 4e899943e..56352a307 100644 --- a/lib/webserver.js +++ b/lib/webserver.js @@ -5,13 +5,31 @@ const debug = require('debug')('broker'); module.exports = main; +// bodyparser < 2 initializes req.body to {} for requests with no body. This +// breaks later serialization, so this pair of middlewares ensures that requests +// with no body have req.body = undefined. This matches the as-yet-unreleased +// bodyparser 2.x behaviour. +const EmptyBody = Symbol('Empty Body'); +const markEmptyRequestBody = (req, res, next) => { + req.body = req.body || EmptyBody; + next(); +}; +const stripEmptyRequestBody = (req, res, next) => { + if (req.body === EmptyBody) { + delete req.body; + } + next(); +} + function main({ key = null, cert = null, port = 7341 } = {}, altPort = null) { const http = (!key && !cert); // no https if there's no certs const https = http ? require('http') : require('https'); const app = express(); app.disable('x-powered-by'); - app.use(bodyParser.json()); + app.use(markEmptyRequestBody); + app.use(bodyParser.raw({ type: '*/*' })); + app.use(stripEmptyRequestBody); app.use('/healthcheck', require('./healthcheck')); if (altPort) { diff --git a/package.json b/package.json index afee7a828..ce79eadf3 100644 --- a/package.json +++ b/package.json @@ -45,6 +45,7 @@ "clite": "^0.3.0", "debug": "^2.2.0", "dotenv": "^2.0.0", + "ejson": "^2.1.2", "engine.io": "^1.6.11", "engine.io-client": "^1.6.11", "express": "^4.14.0", diff --git a/test/unit/filters.test.js b/test/unit/filters.test.js index fb732f1db..6d8da4d1e 100644 --- a/test/unit/filters.test.js +++ b/test/unit/filters.test.js @@ -1,6 +1,8 @@ const test = require('tap').test; const Filters = require('../../lib/filters'); +const jsonBuffer = (body) => Buffer.from(JSON.stringify(body)); + test('filter on body', t => { const filter = Filters(require(__dirname + '/../fixtures/relay.json')); @@ -10,13 +12,13 @@ test('filter on body', t => { filter({ url: '/', method: 'POST', - body: { + body: jsonBuffer({ commits: [ { modified: ['package.json', 'file1.txt'] } ] - } + }) }, (error, res) => { t.equal(error, null, 'no error'); t.equal(res, '/', 'allows the path request'); @@ -25,7 +27,7 @@ test('filter on body', t => { filter({ url: '/', method: 'POST', - body: { + body: jsonBuffer({ commits: [ { modified: ['file2.txt'] @@ -34,7 +36,7 @@ test('filter on body', t => { modified: ['.snyk', 'file1.txt'] } ] - } + }) }, (error, res) => { t.equal(error, null, 'no error'); t.equal(res, '/', 'allows the path request'); @@ -43,7 +45,7 @@ test('filter on body', t => { filter({ url: '/', method: 'POST', - body: { + body: jsonBuffer({ commits: [ { modified: ['file2.txt'] @@ -52,7 +54,7 @@ test('filter on body', t => { modified: ['file3.txt', 'file1.txt'] } ] - } + }) }, (error, res) => { t.equal(error.message, 'blocked', 'has been blocked'); t.equal(res, undefined, 'no follow allowed'); @@ -61,9 +63,9 @@ test('filter on body', t => { filter({ url: '/', method: 'POST', - body: { + body: jsonBuffer({ commits: [] - } + }) }, error => { t.equal(error.message, 'blocked', 'has been blocked'); }); diff --git a/test/unit/relay-response.test.js b/test/unit/relay-response.test.js index fe8293462..bda4a504e 100644 --- a/test/unit/relay-response.test.js +++ b/test/unit/relay-response.test.js @@ -27,18 +27,20 @@ test('relay swaps values found in BROKER_VAR_SUB', t => { url: '/*' }], config); + const body = { + BROKER_VAR_SUB: ['url'], + url: '${HOST}:${PORT}/webhook' + }; + route({ url: '/', method: 'POST', - body: { - BROKER_VAR_SUB: ['url'], - url: '${HOST}:${PORT}/webhook' - }, + body: Buffer.from(JSON.stringify(body)), headers: {}, }, () => { t.equal(spy.callCount, 1, 'request placed'); const arg = spy.args[0][0]; - t.equal(arg.body.url, `${config.HOST}:${config.PORT}/webhook`); + t.equal(JSON.parse(arg.body).url, `${config.HOST}:${config.PORT}/webhook`); t.end(); }); diff --git a/test/unit/try-json-parse.test.js b/test/unit/try-json-parse.test.js new file mode 100644 index 000000000..fc088cd4d --- /dev/null +++ b/test/unit/try-json-parse.test.js @@ -0,0 +1,25 @@ +const test = require('tap').test; + +const tryJSONParse = require('../../lib/try-json-parse'); + +test('tryJSONParse', t => { + const data = { + number: '123', + animals: ['dog', 'cat'], + complex: { nested: 'data' }, + }; + + const dataAsJson = JSON.stringify(data); + const dataAsBuffer = Buffer.from(dataAsJson); + const dataAsUint8Array = Uint8Array.from(dataAsBuffer); // primus with EJSON returns data as Uint8Arrays + + t.same(tryJSONParse(dataAsJson), data, 'Strings parse'); + t.same(tryJSONParse(dataAsBuffer), data, 'Buffers parse'); + t.same(tryJSONParse(dataAsUint8Array), data, 'Uint8Arrays parse'); + t.same(tryJSONParse(undefined), {}, 'undefined parses as empty'); + t.same(tryJSONParse(null), {}, 'null parses as empty'); + t.same(tryJSONParse(data), {}, 'objects parse as empty'); + t.same(tryJSONParse("nonsense"), {}, 'malformed strings parse as empty'); + + t.end(); +});