Mobrovac has uploaded a new change for review.
https://gerrit.wikimedia.org/r/247279
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
Bug: T111709
Change-Id: I72c09303e09b40ab56a442ad8e80f6f7453d2c99
---
M .gitignore
M app.js
M config.dev.yaml
M lib/util.js
M package.json
M test/utils/headers.js
M test/utils/server.js
7 files changed, 135 insertions(+), 71 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/services/mobileapps
refs/changes/79/247279/1
diff --git a/.gitignore b/.gitignore
index e4e5956..5792c40 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,7 +1,7 @@
Dockerfile
+.idea/
coverage
config.yaml
node_modules
npm-debug.log
.DS_Store
-/.idea/
diff --git a/app.js b/app.js
index 434e77b..707a90b 100644
--- a/app.js
+++ b/app.js
@@ -52,15 +52,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';
@@ -80,15 +93,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();
});
@@ -118,29 +135,34 @@
function loadRoutes (app) {
// 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);
+ return fs.readdirAsync(__dirname + '/routes').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);
@@ -149,6 +171,7 @@
});
}
+
/**
* Creates and start the service's web server
@@ -160,8 +183,9 @@
// 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
@@ -169,10 +193,12 @@
}).then(function () {
app.logger.log('info',
'Worker ' + process.pid + ' listening on ' + app.conf.interface +
':' + app.conf.port);
+ return server;
});
}
+
/**
* The service's entry point. It takes over the configuration
* options and the logger- and metrics-reporting objects from
diff --git a/config.dev.yaml b/config.dev.yaml
index ef07a32..0d21504 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
restbase_uri: https://restbase.wikimedia.org
# whether to print extra debug info
debug: true
diff --git a/lib/util.js b/lib/util.js
index b729ab8..18c0886 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);
@@ -209,7 +214,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) {
@@ -229,8 +234,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 7f746e3..6816883 100644
--- a/package.json
+++ b/package.json
@@ -30,30 +30,31 @@
"Marko Obrovac <[email protected]>",
"Gabriel Wicke <[email protected]>"
],
- "license": "Apache2",
+ "license": "Apache-2.0",
"bugs": {
"url": "https://phabricator.wikimedia.org/tag/mobile_content_service/";
},
"homepage": "https://www.mediawiki.org/wiki/RESTBase_services_for_apps";,
"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",
- "underscore": "~1.8.3"
+ "service-runner": "^0.2.11",
+ "underscore": "^1.8.3"
},
"devDependencies": {
- "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": "debian",
diff --git a/test/utils/headers.js b/test/utils/headers.js
index 38508a6..5d3cc08 100644
--- a/test/utils/headers.js
+++ b/test/utils/headers.js
@@ -13,7 +13,7 @@
assert.contentType(res, expContentType);
assert.deepEqual(!!res.headers.etag, true, 'No ETag header
present');
assert.deepEqual(res.headers['access-control-allow-origin'], '*');
- assert.deepEqual(res.headers['access-control-allow-headers'],
'Accept, X-Requested-With, Content-Type');
+ assert.deepEqual(res.headers['access-control-allow-headers'],
'accept, x-requested-with, content-type');
assert.deepEqual(res.headers['content-security-policy'],
"default-src 'self'; object-src 'none'; media-src *; img-src
*; style-src *; frame-ancestors 'self'");
assert.deepEqual(res.headers['x-content-security-policy'],
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/247279
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I72c09303e09b40ab56a442ad8e80f6f7453d2c99
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/services/mobileapps
Gerrit-Branch: master
Gerrit-Owner: Mobrovac <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
