From 0b95256ee46dd0ab1c01ea62ea6e0aa41b5ee8a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javier=20Mendiara=20Ca=C3=B1ardo?=
 <javier.mendiaracanardo@telefonica.com>
Date: Sun, 30 Apr 2017 00:02:23 +0200
Subject: [PATCH] feat(html): Reply with html

---
 README.md                    | 73 ++++++++++++---------------
 lib/therror-connect.js       | 81 +++++++++++++++++++++++++-----
 package.json                 |  8 +--
 test/therror-connect.spec.js | 97 ++++++++++++++++++++++++++++++++++++
 yarn.lock                    | 28 +++++------
 5 files changed, 218 insertions(+), 69 deletions(-)

diff --git a/README.md b/README.md
index 05b8fff..2e618f7 100644
--- a/README.md
+++ b/README.md
@@ -2,7 +2,7 @@
 
 therror-express implements a connect/express error handler middleware for [Therror.ServerError](https://github.com/therror/therror)
 
-Logs all errors (by default) and replies with an error payload with only the error relevant information. Currently supports [content negotiation](https://en.wikipedia.org/wiki/Content_negotiation) for `text/plain` and `application/json`.
+Logs all errors (by default) and replies with an error payload with only the error relevant information. Currently supports [content negotiation](https://en.wikipedia.org/wiki/Content_negotiation) for `text/html`, `text/plain` and `application/json`.
 
 It's written in ES6, for node >= 4 
 
@@ -22,45 +22,23 @@ const connect = require('connect');
 
 let app = connect();
 
-// The last one middleware added to your express app
-app.use(errorHandler({
-  log: true, // use the `log` method in the ServerError to log it (default: true)
-  development: process.env.NODE_ENV === 'development' // return stack traces and causes in the payload (default: false),
-  unexpectedClass: Therror.ServerError.InternalServerError // When a strange thing reaches this middleware trying to behave as an error (such a Number, String, obj..), this error class will be instantiated, logged, and returned to the client. 
-}));
+// The last one middleware added to your app
+app.use(errorHandler()); // Some options can be provided. See below
 ```
 
-### Full Example
+### Customize html with express
 ```js
-const Therror = require('therror'),
-      errorHandler = require('therror-connect');
-
-Therror.Loggable.logger = require('logops');
-
-app.use(
- function(req, res, next) {
-   user = { id: 12, email: 'john.doe@mailinator.com' };
-   next(new Therror.ServerError.Unauthorized('User ${id} not authorized', user));
- },
- errorHandler()
-);
-/* Writes log:  
-      UnauthorizedError: User 12 not authorized
-      UnauthorizedError: User 12 not authorized { id: 12, email: 'john.doe@mailinator.com' }
-          at Object.<anonymous> (/Users/javier/Documents/Proyectos/logops/deleteme.js:17:11)
-          at Module._compile (module.js:409:26)
-          at Object.Module._extensions..js (module.js:416:10)
-          at Module.load (module.js:343:32)
-          at Function.Module._load (module.js:300:12)
-          at Function.Module.runMain (module.js:441:10)
-          at startup (node.js:139:18)
-          at node.js:968:3
-
- Replies: 
-        401
-        { error: 'UnauthorizedError',
-         message: 'User 12 not authorized' }
-*/
+const express = require('express');
+const app = express();
+
+app.set('views', path.join(__dirname, 'views'));
+app.set('view engine', 'ejs');
+
+app.use(errorHandler({
+  render: function(data, req, res, cb) {
+    res.render(`/errors/${data.statusCode}`, data, cb);
+  }
+}));
 ```
 
 ### API
@@ -71,14 +49,29 @@ let errorHandler = require('therror-connect');
 Creates the middleware configured with the provided `options` object
 
 **`options.log`** `[Boolean]` can be
- * `true`: logs the error using the `error.log` method. _default_
+ * `true`: logs the error using the `error.log({req, res})` method. _default_
  * `false`: does nothing. 
  
 **`options.development`** `[Boolean]` can be
  * `false`: Dont add stack traces and development info to the payload. _default_
- * `true`: Add development info to the payload. 
+ * `true`: Add development info to the responses. 
  
 **`options.unexpectedClass`** `[class]` The `Therror.ServerError` class to instantiate when an unmanegeable error reaches the middleware. _defaults to `Therror.ServerError.InternalServerError`_
+
+**`options.render`** `Function` to customize the sent html sent. 
+```js
+function render(data, req, res, cb) {
+  // data.error: the error instance. 
+  // data.name: error name. Eg: UnauthorizedError
+  // data.message: error message. Eg: User 12 not authorized 
+  // data.statusCode: associated statusCode to the message. Eg: 401
+  // data.stack: looong string with the stacktrace (if options.development === true; else '')
+
+  // req: Incoming http request
+  // res: Outgoing http response. Warning! don't send the html, give it to the callback
+  // cb: function(err, html) callback to call with the html
+}
+```
  
 ## Peer Projects
 * [therror](https://github.com/therror/therror): The Therror library, easy errors for nodejs
@@ -86,7 +79,7 @@ Creates the middleware configured with the provided `options` object
 
 ## LICENSE
 
-Copyright 2016 [Telefónica I+D](http://www.tid.es)
+Copyright 2016,2017 [Telefónica I+D](http://www.tid.es)
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
diff --git a/lib/therror-connect.js b/lib/therror-connect.js
index 56fa7e7..a17c8ae 100644
--- a/lib/therror-connect.js
+++ b/lib/therror-connect.js
@@ -1,6 +1,6 @@
 /**
  * @license
- * Copyright 2016 Telefónica I+D
+ * Copyright 2016,2017 Telefónica I+D
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,15 +17,18 @@
 
 'use strict';
 
-const accepts = require('accepts'),
-      serr = require('serr'),
-      _ = require('lodash'),
-      stringify = require('json-stringify-safe'),
-      Therror = require('therror'),
-      util = require('util');
+const accepts = require('accepts');
+const serr = require('serr');
+const _ = require('lodash');
+const stringify = require('json-stringify-safe');
+const Therror = require('therror');
+const util = require('util');
+const escapeHtml = require('escape-html');
 
 module.exports = errorHandler;
 
+class HtmlRenderError extends Therror.ServerError.InternalServerError {}
+
 function errorHandler(options) {
 
   let opts = options || /* istanbul ignore next */ {};
@@ -36,13 +39,19 @@ function errorHandler(options) {
 
   let UnexpectedErrorClass = opts.unexpectedClass || Therror.ServerError.InternalServerError;
 
+  let render = opts.render || renderTherrorConnect;
+
   if (!isServerTherror(UnexpectedErrorClass.prototype)) {
     throw new Therror('You must provide a ServerError error');
   }
 
   return function errorHandlerMiddleware(err, req, res, next) {
 
-    if (!err.isTherror || !isServerTherror(err)) {
+    if (err instanceof HtmlRenderError) {
+      // An error in userland was raised when rendering using the provided options.render
+      // use our safe one
+      render = renderTherrorConnect;
+    } else if (!err.isTherror || !isServerTherror(err)) {
       err = new UnexpectedErrorClass(err);
     }
 
@@ -61,6 +70,7 @@ function errorHandler(options) {
     res.setHeader('Content-Security-Policy', 'default-src \'self\'');
 
     // respect err.statusCode
+    // TODO check it's a number
     res.statusCode = err.statusCode;
 
     if (req.method === 'HEAD') {
@@ -72,7 +82,7 @@ function errorHandler(options) {
     // negotiate
     let accept = accepts(req);
     // the order of this list is significant; should be server preferred order
-    switch (accept.type(['json'])) {
+    switch (accept.type(['json', 'html'])) {
       case 'json':
         response = err.toPayload();
         if (development) {
@@ -82,8 +92,37 @@ function errorHandler(options) {
           // Split stack in lines for better developer readability
           response.$$delevelopmentInfo.stack = response.$$delevelopmentInfo.stack.split('\n');
         }
-        res.setHeader('Content-Type', 'application/json; charset=utf-8');
         response = stringify(response);
+
+        res.setHeader('Content-Type', 'application/json; charset=utf-8');
+        end(res, response);
+        break;
+
+      case 'html':
+        let payload = err.toPayload();
+        let data = {
+          error: err,
+          name: escapeHtml(payload.error),
+          message: escapeHtml(payload.message),
+          statusCode: err.statusCode,
+          stack: development ? escapeHtml(serr(err).toString(true)) : ''
+        };
+
+        try {
+          render(data, req, res, htmlCb);
+        } catch (err) {
+          return htmlCb(err);
+        }
+
+        function htmlCb(err, html) {
+          if (err) {
+            let error = new HtmlRenderError(err, 'Cannot render with provided renderer');
+            return errorHandlerMiddleware(error, req, res);
+          }
+          res.setHeader('Content-Type', 'text/html; charset=utf-8');
+          end(res, html);
+        }
+
         break;
 
       default:
@@ -94,13 +133,17 @@ function errorHandler(options) {
         }
 
         res.setHeader('Content-Type', 'text/plain; charset=utf-8');
+        end(res, response);
         break;
     }
-    res.setHeader('Content-Length', Buffer.byteLength(response, 'utf8'));
-    res.end(response);
   };
 }
 
+function end(res, response) {
+  res.setHeader('Content-Length', Buffer.byteLength(response, 'utf8'));
+  res.end(response);
+}
+
 // node 6 adds the stacktrace when toStringing an error
 function errToString(err) {
   let payload = err.toPayload();
@@ -111,3 +154,17 @@ function isServerTherror(obj) {
   return _.isFunction(obj.toPayload) && !_.isUndefined(obj.statusCode);
 }
 
+function renderTherrorConnect(data, req, res, cb) {
+  return cb(null, `<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Error ${data.statusCode}</title>
+  </head>
+  <body>
+    <h1>${data.name} <i>(${data.statusCode})</i></h1>
+    <h2>${data.message}</h2>
+    <pre>${data.stack}</pre>
+  </body>
+</html>`);
+}
diff --git a/package.json b/package.json
index 177359e..4a70f6a 100644
--- a/package.json
+++ b/package.json
@@ -33,7 +33,7 @@
     "test": "mocha -R spec test/environment.js 'test/**/*.spec.js'"
   },
   "devDependencies": {
-    "chai": "^3.0.0",
+    "chai": "^3.5.0",
     "coveralls": "^2.11.2",
     "eslint": "^3.19.0",
     "istanbul": "^0.4.2",
@@ -41,7 +41,7 @@
     "mocha": "^3.3.0",
     "release-it": "^2.4.0",
     "sinon": "^2.1.0",
-    "sinon-chai": "^2.8.0",
+    "sinon-chai": "^2.9.0",
     "supertest": "^3.0.0",
     "therror": "^3.0.0"
   },
@@ -50,8 +50,10 @@
   },
   "dependencies": {
     "accepts": "^1.3.3",
+    "escape-html": "^1.0.3",
     "json-stringify-safe": "^5.0.1",
     "lodash": "^4.1.0",
-    "serr": "^1.0.0"
+    "serr": "^1.0.0",
+    "var": "^0.2.0"
   }
 }
diff --git a/test/therror-connect.spec.js b/test/therror-connect.spec.js
index 17b9703..1b738eb 100644
--- a/test/therror-connect.spec.js
+++ b/test/therror-connect.spec.js
@@ -137,6 +137,90 @@ describe('errorHandler()', function() {
             }), done);
       });
     });
+
+    describe('when client accepts text/html', function() {
+      it('should return default html', function(done) {
+        var error = new Therror.ServerError.NotFound('boom!');
+        var server = createServer(error);
+        request(server)
+            .get('/')
+            .set('Accept', 'text/html')
+            .expect('Content-Type', /text\/html/)
+            .expect(404, `<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Error 404</title>
+  </head>
+  <body>
+    <h1>NotFound <i>(404)</i></h1>
+    <h2>boom!</h2>
+    <pre></pre>
+  </body>
+</html>`, done);
+      });
+
+      it('should return user defined html', function(done) {
+        var error = new Therror.ServerError.NotFound('boom!');
+        var server = createServer(error, {
+          render(data, req, res, next) {
+            next(null, '<hello>')
+          }
+        });
+        request(server)
+            .get('/')
+            .set('Accept', 'text/html')
+            .expect('Content-Type', /text\/html/)
+            .expect(404, '<hello>', done);
+      });
+
+      it('should pass precomputed arguments to the render function', function(done) {
+        var error = new Therror.ServerError.NotFound('<p>boom!</p>');
+        var server = createServer(error, {
+          render(data, req, res, next) {
+            expect(data.error).to.be.eql(error);
+            expect(data.name).to.be.eql('NotFound');
+            expect(data.message).to.be.eql('&lt;p&gt;boom!&lt;/p&gt;');
+            expect(data.statusCode).to.be.eql(404);
+            expect(data.stack).to.be.eql('');
+            next(null, '<hello>');
+          }
+        });
+        request(server)
+            .get('/')
+            .set('Accept', 'text/html')
+            .expect('Content-Type', /text\/html/, done);
+      });
+
+      it('should return default html when the user render fails async', function(done) {
+        var error = new Therror.ServerError.NotFound('boom!');
+        var server = createServer(error, {
+          render(data, req, res, next) {
+            next(new Error('RenderError'));
+          }
+        });
+        request(server)
+            .get('/')
+            .set('Accept', 'text/html')
+            .expect('Content-Type', /text\/html/)
+            .expect(500, /<!DOCTYPE html>/, done);
+      });
+
+       it('should return default html when the user render fails sync', function(done) {
+        var error = new Therror.ServerError.NotFound('boom!');
+        var server = createServer(error, {
+          render(data, req, res, next) {
+            throw new Error('RenderError');
+          }
+        });
+        request(server)
+            .get('/')
+            .set('Accept', 'text/html')
+            .expect('Content-Type', /text\/html/)
+            .expect(500, /<!DOCTYPE html>/, done);
+      });
+      
+    });
   });
 
   describe('in "development" environment', function() {
@@ -168,6 +252,19 @@ describe('errorHandler()', function() {
             }), done);
       });
     });
+
+    describe('when client accepts text/html', function() {
+      it('should return default html with dev info ', function(done) {
+        var error = new Therror.ServerError.NotFound('boom!');
+        var server = createServer(error, { development: true });
+        request(server)
+            .get('/')
+            .set('Accept', 'text/html')
+            .expect('Content-Type', /text\/html/)
+            //.expect(404,  /.*<pre>(.+)<\/pre>/, done);
+            .expect(404,  /<pre>(.|\n)+<\/pre>/, done);
+      });
+    })
   });
 
   describe('logging', function() {
diff --git a/yarn.lock b/yarn.lock
index 733147c..fb085ec 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -208,7 +208,7 @@ center-align@^0.1.1:
     align-text "^0.1.3"
     lazy-cache "^1.0.3"
 
-chai@^3.0.0:
+chai@^3.5.0:
   version "3.5.0"
   resolved "https://registry.yarnpkg.com/chai/-/chai-3.5.0.tgz#4d02637b067fe958bdbfdd3a40ec56fef7373247"
   dependencies:
@@ -521,6 +521,10 @@ es6-weak-map@^2.0.1:
     es6-iterator "^2.0.1"
     es6-symbol "^3.1.1"
 
+escape-html@^1.0.3:
+  version "1.0.3"
+  resolved "https://registry.yarnpkg.com/escape-html/-/escape-html-1.0.3.tgz#0258eae4d3d0c0974de1c169188ef0051d1d1988"
+
 escape-string-regexp@1.0.5, escape-string-regexp@^1.0.2, escape-string-regexp@^1.0.5:
   version "1.0.5"
   resolved "https://registry.yarnpkg.com/escape-string-regexp/-/escape-string-regexp-1.0.5.tgz#1b61c0562190a8dff6ae3bb2cf0200ca130b86d4"
@@ -1219,7 +1223,7 @@ lodash.keys@^3.0.0:
     lodash.isarguments "^3.0.0"
     lodash.isarray "^3.0.0"
 
-lodash@4.17.4, lodash@^4.0.0, lodash@^4.1.0, lodash@^4.3.0:
+lodash@4.17.4, lodash@^4.0.0, lodash@^4.1.0, lodash@^4.15.0, lodash@^4.3.0:
   version "4.17.4"
   resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.4.tgz#78203a4d1c328ae1d86dca6460e369b57f4055ae"
 
@@ -1416,10 +1420,6 @@ path-is-inside@^1.0.1:
   version "1.0.2"
   resolved "https://registry.yarnpkg.com/path-is-inside/-/path-is-inside-1.0.2.tgz#365417dede44430d1c11af61027facf074bdfc53"
 
-path-parse@^1.0.5:
-  version "1.0.5"
-  resolved "https://registry.yarnpkg.com/path-parse/-/path-parse-1.0.5.tgz#3c1adf871ea9cd6c9431b6ea2bd74a0ff055c4c1"
-
 path-to-regexp@^1.7.0:
   version "1.7.0"
   resolved "https://registry.yarnpkg.com/path-to-regexp/-/path-to-regexp-1.7.0.tgz#59fde0f435badacba103a84e9d3bc64e96b9937d"
@@ -1592,16 +1592,10 @@ resolve-from@^1.0.0:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/resolve-from/-/resolve-from-1.0.1.tgz#26cbfe935d1aeeeabb29bc3fe5aeb01e93d44226"
 
-resolve@1.1.x:
+resolve@1.1.x, resolve@^1.1.6:
   version "1.1.7"
   resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.1.7.tgz#203114d82ad2c5ed9e8e0411b3932875e889e97b"
 
-resolve@^1.1.6:
-  version "1.3.3"
-  resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.3.3.tgz#655907c3469a8680dc2de3a275a8fdd69691f0e5"
-  dependencies:
-    path-parse "^1.0.5"
-
 restore-cursor@^1.0.1:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/restore-cursor/-/restore-cursor-1.0.1.tgz#34661f46886327fed2991479152252df92daa541"
@@ -1682,7 +1676,7 @@ signal-exit@^3.0.2:
   version "3.0.2"
   resolved "https://registry.yarnpkg.com/signal-exit/-/signal-exit-3.0.2.tgz#b5fdc08f1287ea1178628e415e25132b73646c6d"
 
-sinon-chai@^2.8.0:
+sinon-chai@^2.9.0:
   version "2.9.0"
   resolved "https://registry.yarnpkg.com/sinon-chai/-/sinon-chai-2.9.0.tgz#34d820042bc9661a14527130d401eb462c49bb84"
 
@@ -1983,6 +1977,12 @@ uuid@^3.0.0:
   version "3.0.1"
   resolved "https://registry.yarnpkg.com/uuid/-/uuid-3.0.1.tgz#6544bba2dfda8c1cf17e629a3a305e2bb1fee6c1"
 
+var@^0.2.0:
+  version "0.2.0"
+  resolved "https://registry.yarnpkg.com/var/-/var-0.2.0.tgz#f847bea60a997de7f0b8c664598222236778a6e5"
+  dependencies:
+    lodash "^4.15.0"
+
 verror@1.3.6:
   version "1.3.6"
   resolved "https://registry.yarnpkg.com/verror/-/verror-1.3.6.tgz#cff5df12946d297d2baaefaa2689e25be01c005c"