jenkins-bot has submitted this change and it was merged.
Change subject: Update to service-template-node v0.2.2
......................................................................
Update to service-template-node v0.2.2
- Logging: log only whitelisted headers for privacy reasons
- Routes: fully-asynchronous route loading
- Bump dependencies' versions
Change-Id: I7103b471dde02ae431410f0b8f881735727615b8
---
M .gitignore
M .jshintignore
M app.js
M config.dev.yaml
M lib/util.js
M package.json
M test/features/app/app.js
M test/utils/server.js
8 files changed, 145 insertions(+), 81 deletions(-)
Approvals:
Mobrovac: Looks good to me, approved
Physikerwelt: Looks good to me, but someone else must approve
jenkins-bot: Verified
diff --git a/.gitignore b/.gitignore
index 767da0e..19c009f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,4 +1,5 @@
Dockerfile
+.idea/
coverage
npm-debug.log
Makefile
diff --git a/.jshintignore b/.jshintignore
index b111d44..a4c38eb 100644
--- a/.jshintignore
+++ b/.jshintignore
@@ -1,3 +1,4 @@
coverage
node_modules
-test
\ No newline at end of file
+test
+static
diff --git a/app.js b/app.js
index 5e83979..4eb4360 100644
--- a/app.js
+++ b/app.js
@@ -58,15 +58,28 @@
}
}
+ // set up header whitelisting for logging
+ if(!app.conf.log_header_whitelist) {
+ app.conf.log_header_whitelist = [
+ 'cache-control', 'content-type', 'content-length', 'if-match',
+ 'user-agent', 'x-request-id'
+ ];
+ }
+ app.conf.log_header_whitelist = new RegExp('^(?:' +
app.conf.log_header_whitelist.map(function(item) {
+ return item.trim();
+ }).join('|') + ')$', 'i');
+
// set up the spec
if(!app.conf.spec) {
app.conf.spec = __dirname + '/spec.yaml';
}
- try {
- app.conf.spec = yaml.safeLoad(fs.readFileSync(app.conf.spec));
- } catch(e) {
- app.logger.log('warn/spec', 'Could not load the spec: ' + e);
- app.conf.spec = {};
+ if(app.conf.spec.constructor !== Object) {
+ try {
+ app.conf.spec = yaml.safeLoad(fs.readFileSync(app.conf.spec));
+ } catch(e) {
+ app.logger.log('warn/spec', 'Could not load the spec: ' + e);
+ app.conf.spec = {};
+ }
}
if(!app.conf.spec.swagger) {
app.conf.spec.swagger = '2.0';
@@ -86,15 +99,19 @@
// set the CORS and CSP headers
app.all('*', function(req, res, next) {
if(app.conf.cors !== false) {
- res.header('Access-Control-Allow-Origin', app.conf.cors);
- res.header('Access-Control-Allow-Headers', 'Accept,
X-Requested-With, Content-Type');
+ res.header('access-control-allow-origin', app.conf.cors);
+ res.header('access-control-allow-headers', 'accept,
x-requested-with, content-type');
+ res.header('access-control-expose-headers', 'etag');
}
- res.header('X-XSS-Protection', '1; mode=block');
- res.header('X-Content-Type-Options', 'nosniff');
- res.header('X-Frame-Options', 'SAMEORIGIN');
- res.header('Content-Security-Policy', app.conf.csp);
- res.header('X-Content-Security-Policy', app.conf.csp);
- res.header('X-WebKit-CSP', app.conf.csp);
+ res.header('x-xss-protection', '1; mode=block');
+ res.header('x-content-type-options', 'nosniff');
+ res.header('x-frame-options', 'SAMEORIGIN');
+ res.header('content-security-policy', app.conf.csp);
+ res.header('x-content-security-policy', app.conf.csp);
+ res.header('x-webkit-csp', app.conf.csp);
+
+ sUtil.initAndLogRequest(req, app);
+
next();
});
@@ -136,35 +153,40 @@
// get the list of files in routes/
return fs.readdirAsync(__dirname + '/routes')
- .map(function (fname) {
- // ... and then load each route
- // but only if it's a js file
- if(!/\.js$/.test(fname)) {
- return;
- }
- // import the route file
- var route = require(__dirname + '/routes/' + fname);
- route = route(app);
- // check that the route exports the object we need
- if(route.constructor !== Object || !route.path || !route.router ||
!(route.api_version || route.skip_domain)) {
- throw new TypeError('routes/' + fname + ' does not export the
correct object!');
- }
- // wrap the route handlers with Promise.try() blocks
- sUtil.wrapRouteHandlers(route.router, app);
- // determine the path prefix
- var prefix = '';
- if(!route.skip_domain) {
- prefix = '/:domain/v' + route.api_version;
- }
- // all good, use that route
- app.use(prefix + route.path, route.router);
- }).then(function () {
- // catch errors
- sUtil.setErrorHandler(app);
- // route loading is now complete, return the app object
- return BBPromise.resolve(app);
- });
-
+ .map(function (fname) {
+ return BBPromise.try(function () {
+ // ... and then load each route
+ // but only if it's a js file
+ if(!/\.js$/.test(fname)) {
+ return undefined;
+ }
+ // import the route file
+ var route = require(__dirname + '/routes/' + fname);
+ return route(app);
+ }).then(function (route) {
+ if(route === undefined) {
+ return undefined;
+ }
+ // check that the route exports the object we need
+ if (route.constructor !== Object || !route.path ||
!route.router || !(route.api_version || route.skip_domain)) {
+ throw new TypeError('routes/' + fname + ' does not export
the correct object!');
+ }
+ // wrap the route handlers with Promise.try() blocks
+ sUtil.wrapRouteHandlers(route.router);
+ // determine the path prefix
+ var prefix = '';
+ if(!route.skip_domain) {
+ prefix = '/:domain/v' + route.api_version;
+ }
+ // all good, use that route
+ app.use(prefix + route.path, route.router);
+ });
+ }).then(function () {
+ // catch errors
+ sUtil.setErrorHandler(app);
+ // route loading is now complete, return the app object
+ return BBPromise.resolve(app);
+ });
}
/**
@@ -177,16 +199,18 @@
// return a promise which creates an HTTP server,
// attaches the app to it, and starts accepting
// incoming client requests
+ var server;
return new BBPromise(function (resolve) {
- http.createServer(app).listen(
+ server = http.createServer(app).listen(
app.conf.port,
app.conf.interface,
resolve
);
}).then(function () {
- app.logger.log('info',
- 'Worker ' + process.pid + ' listening on ' +
app.conf.interface + ':' + app.conf.port);
- });
+ app.logger.log('info',
+ 'Worker ' + process.pid + ' listening on ' + app.conf.interface +
':' + app.conf.port);
+ return server;
+ });
}
diff --git a/config.dev.yaml b/config.dev.yaml
index 36c8356..ee54695 100644
--- a/config.dev.yaml
+++ b/config.dev.yaml
@@ -47,6 +47,16 @@
# no_proxy_list:
# - domain1.com
# - domain2.org
+ # the list of incoming request headers that can be logged; if left empty,
+ # the following headers are allowed: cache-control, content-length,
+ # content-type, if-match, user-agent, x-request-id
+ # log_header_whitelist:
+ # - cache-control
+ # - content-length
+ # - content-type
+ # - if-match
+ # - user-agent
+ # - x-request-id
# list of enabled renders
svg: true
img: true
diff --git a/lib/util.js b/lib/util.js
index 5b6bbb6..16133dd 100644
--- a/lib/util.js
+++ b/lib/util.js
@@ -45,14 +45,15 @@
/**
* Generates an object suitable for logging out of a request object
*
- * @param {Request} the request
+ * @param {Request} req the request
+ * @param {RegExp} whitelistRE the RegExp used to filter headers
* @return {Object} an object containing the key components of the request
*/
-function reqForLog(req) {
+function reqForLog(req, whitelistRE) {
- return {
+ var ret = {
url: req.originalUrl,
- headers: req.headers,
+ headers: {},
method: req.method,
params: req.params,
query: req.query,
@@ -61,13 +62,23 @@
remotePort: req.connection.remotePort
};
+ if(req.headers && whitelistRE) {
+ Object.keys(req.headers).forEach(function(hdr) {
+ if(whitelistRE.test(hdr)) {
+ ret.headers[hdr] = req.headers[hdr];
+ }
+ });
+ }
+
+ return ret;
+
}
/**
* Serialises an error object in a form suitable for logging
*
- * @param {Error} the error to serialise
+ * @param {Error} err error to serialise
* @return {Object} the serialised version of the error
*/
function errForLog(err) {
@@ -93,27 +104,21 @@
}
-
/**
* Wraps all of the given router's handler functions with
* promised try blocks so as to allow catching all errors,
* regardless of whether a handler returns/uses promises
* or not.
*
- * @param {Router} the router object
- * @param {Application} the application object
+ * @param {Router} router object
*/
-function wrapRouteHandlers(router, app) {
+function wrapRouteHandlers(router) {
router.stack.forEach(function(routerLayer) {
routerLayer.route.stack.forEach(function(layer) {
var origHandler = layer.handle;
layer.handle = function(req, res, next) {
BBPromise.try(function() {
- req.headers = req.headers || {};
- req.headers['x-request-id'] = req.headers['x-request-id']
|| generateRequestId();
- req.logger = app.logger.child({request_id:
req.headers['x-request-id']});
- req.logger.log('trace/req', {req: reqForLog(req), msg:
'incoming request'});
return origHandler(req, res, next);
})
.catch(next);
@@ -219,7 +224,7 @@
/**
* Creates a new router with some default options.
*
- * @param {Object} opts additional options to pass to express.Router()
+ * @param {Object=} opts additional options to pass to express.Router()
* @return {Router} a new router object
*/
function createRouter(opts) {
@@ -239,8 +244,23 @@
}
+/**
+ * Adds logger to the request and logs it
+ *
+ * @param {*} req request object
+ * @param {Application} app application object
+ */
+function initAndLogRequest(req, app) {
+ req.headers = req.headers || {};
+ req.headers['x-request-id'] = req.headers['x-request-id'] ||
generateRequestId();
+ req.logger = app.logger.child({request_id: req.headers['x-request-id']});
+ req.logger.log('trace/req', {req: reqForLog(req,
app.conf.log_header_whitelist), msg: 'incoming request'});
+}
+
+
module.exports = {
HTTPError: HTTPError,
+ initAndLogRequest: initAndLogRequest,
wrapRouteHandlers: wrapRouteHandlers,
setErrorHandler: setErrorHandler,
router: createRouter
diff --git a/package.json b/package.json
index 98b5f71..0fcef61 100644
--- a/package.json
+++ b/package.json
@@ -2,7 +2,7 @@
"name": "mathoid",
"version": "0.2.8",
"description": "Render TeX to SVG and MathML using MathJax. Based on
svgtex.",
- "main": "./mathoid.js",
+ "main": "./app.js",
"scripts": {
"start": "service-runner",
"test": "mocha",
@@ -32,31 +32,31 @@
"contributors": [],
"license": "Apache-2.0",
"bugs": {
- "url": "https://phabricator.wikimedia.org/tag/service-template-node/";
+ "url": "https://phabricator.wikimedia.org/tag/mathoid/";
},
"homepage": "https://github.com/wikimedia/mathoid";,
"dependencies": {
"bluebird": "~2.8.2",
- "body-parser": "^1.13.2",
- "bunyan": "^1.4.0",
+ "body-parser": "^1.14.1",
+ "bunyan": "^1.5.1",
"cassandra-uuid": "^0.0.2",
- "compression": "^1.5.1",
- "domino": "^1.0.18",
- "express": "^4.13.1",
- "js-yaml": "^3.3.1",
+ "compression": "^1.6.0",
+ "domino": "^1.0.19",
+ "express": "^4.13.3",
+ "js-yaml": "^3.4.3",
"preq": "^0.4.4",
- "service-runner": "^0.2.1",
+ "service-runner": "^0.2.11",
"texvcjs": "git+https://github.com/wikimedia/texvcjs";,
"MathJax-node":
"git+https://github.com/wikimedia/MathJax-node#mathoid-0-2-8";,
"texvcinfo": "^0.3.1"
},
"devDependencies": {
- "coveralls": "2.11.2",
- "istanbul": "^0.3.17",
- "mocha": "^2.2.5",
+ "extend": "^3.0.0",
+ "istanbul": "^0.3.22",
+ "mocha": "^2.3.3",
"mocha-jshint": "^2.2.3",
- "mocha-lcov-reporter": "^0.0.2",
- "swagger-router": "^0.1.1"
+ "mocha-lcov-reporter": "^1.0.0",
+ "swagger-router": "^0.2.0"
},
"deploy": {
"target": "ubuntu",
diff --git a/test/features/app/app.js b/test/features/app/app.js
index ae538df..5f9927d 100644
--- a/test/features/app/app.js
+++ b/test/features/app/app.js
@@ -31,7 +31,8 @@
}).then(function(res) {
assert.deepEqual(res.status, 200);
assert.deepEqual(res.headers['access-control-allow-origin'], '*');
- assert.notDeepEqual(res.headers['access-control-allow-headers'],
undefined);
+ assert.deepEqual(!!res.headers['access-control-allow-headers'],
true);
+ assert.deepEqual(!!res.headers['access-control-expose-headers'],
true);
});
});
diff --git a/test/utils/server.js b/test/utils/server.js
index 9ae0e28..cb56ea1 100644
--- a/test/utils/server.js
+++ b/test/utils/server.js
@@ -8,9 +8,10 @@
var BBPromise = require('bluebird');
var ServiceRunner = require('service-runner');
var logStream = require('./logStream');
-var fs = require('fs');
-var assert = require('./assert');
-var yaml = require('js-yaml');
+var fs = require('fs');
+var assert = require('./assert');
+var yaml = require('js-yaml');
+var extend = require('extend');
// set up the configuration
@@ -19,7 +20,8 @@
};
// build the API endpoint URI by supposing the actual service
// is the last one in the 'services' list in the config file
-var myService = config.conf.services[config.conf.services.length - 1];
+var myServiceIdx = config.conf.services.length - 1;
+var myService = config.conf.services[myServiceIdx];
config.uri = 'http://localhost:' + myService.conf.port + '/';
// no forking, run just one process when testing
config.conf.num_workers = 0;
@@ -29,8 +31,10 @@
level: 'trace',
stream: logStream()
};
+// make a deep copy of it for later reference
+var origConfig = extend(true, {}, config);
-var stop = function () {};
+var stop = function () {};
var options = null;
var runner = new ServiceRunner();
@@ -43,6 +47,9 @@
console.log('server options changed; restarting');
stop();
options = _options;
+ // set up the config
+ config = extend(true, {}, origConfig);
+ extend(true, config.conf.services[myServiceIdx].conf, options);
return runner.run(config.conf)
.then(function(servers) {
var server = servers[0];
--
To view, visit https://gerrit.wikimedia.org/r/247257
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I7103b471dde02ae431410f0b8f881735727615b8
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/services/mathoid
Gerrit-Branch: master
Gerrit-Owner: Mobrovac <[email protected]>
Gerrit-Reviewer: Mobrovac <[email protected]>
Gerrit-Reviewer: Physikerwelt <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
