fix: clean errors, add validation & reduce exposed (#88)

Author: a335926a

.gitignore

@@ -1 +1,2 @@
-node_modules/
+node_modules/
+yarn.lock

src/index.html

@@ -206,6 +206,6 @@ <h1>Rendertron</h1>
     }
     </script>
 
-    <link rel="import" href="node_modules/progress-bar-element/progress-bar.html">
+    <link rel="import" href="progress-bar.html">
   </body>
 </html>

src/main.js

@@ -17,24 +17,29 @@
 'use strict';
 
 const assert = require('assert');
-const renderer = require('./renderer');
-const chromeLauncher = require('chrome-launcher');
-const express = require('express');
 const fs = require('fs');
-const compression = require('compression');
-const path = require('path');
 const https = require('https');
-const app = express();
-const cache = require('./cache');
+const path = require('path');
+const url = require('url');
+const chromeLauncher = require('chrome-launcher');
+const compression = require('compression');
+const express = require('express');
 const now = require('performance-now');
 const uuidv4 = require('uuid/v4');
+const cache = require('./cache');
+const renderer = require('./renderer');
+
+const app = express();
+
+const CONFIG_PATH = path.resolve(__dirname, '../config.json');
+const PROGRESS_BAR_PATH = path.resolve(__dirname, '../node_modules/progress-bar-element/progress-bar.html');
+const PORT = process.env.PORT || '3000';
 
-// Load config from config.json if it exists.
 let config = {};
-const configPath = path.resolve(__dirname, '../config.json');
 
-if (fs.existsSync(configPath)) {
-  config = JSON.parse(fs.readFileSync(configPath));
+// Load config from config.json if it exists.
+if (fs.existsSync(CONFIG_PATH)) {
+  config = JSON.parse(fs.readFileSync(CONFIG_PATH));
   assert(config instanceof Object);
 }
 
@@ -55,21 +60,24 @@ app.setConfig = (newConfig) => {
 };
 
 app.use(compression());
-
-app.use('/node_modules', express.static(path.resolve(__dirname, '../node_modules')));
+app.use('/progress-bar.html', express.static(PROGRESS_BAR_PATH));
 
 app.get('/', (request, response) => {
   response.sendFile(path.resolve(__dirname, 'index.html'));
 });
 
-function isRestricted(url) {
-  if (!config['renderOnly'])
-    return false;
+function isRestricted(urlReq) {
+  const protocol = (url.parse(urlReq).protocol || '');
+
+  if (!protocol.match(/^https?/)) return true;
+  if (!config['renderOnly']) return false;
+
   for (let i = 0; i < config['renderOnly'].length; i++) {
-    if (url.startsWith(config['renderOnly'][i])) {
+    if (urlReq.startsWith(config['renderOnly'][i])) {
       return false;
     }
   }
+
   return true;
 }
 
@@ -100,10 +108,8 @@ app.get('/render/:url(*)', async(request, response) => {
     response.status(result.status).send(result.body);
     track('render', now() - start);
   } catch (err) {
-    let message = `Cannot render ${request.params.url}`;
-    if (err && err.message)
-      message += ` - "${err.message}"`;
-    response.status(400).send(message);
+    response.status(400).send('Cannot render requested URL');
+    console.error(err);
   }
 });
 
@@ -124,19 +130,16 @@ app.get('/screenshot/:url(*)', async(request, response) => {
     response.end(img);
     track('screenshot', now() - start);
   } catch (err) {
-    let message = `Cannot render ${request.params.url}`;
-    if (err && err.message)
-      message += ` - "${err.message}"`;
-    response.status(400).send(message);
+    response.status(400).send('Cannot render requested URL');
+    console.error(err);
   }
 });
 
 app.get('/_ah/health', (request, response) => response.send('OK'));
 
-app.get('/_ah/stop', async(request, response) => {
+app.stop = async() => {
   await config.chrome.kill();
-  response.send('OK');
-});
+};
 
 const appPromise = chromeLauncher.launch({
   chromeFlags: ['--headless', '--disable-gpu', '--remote-debugging-address=0.0.0.0'],
@@ -147,10 +150,9 @@ const appPromise = chromeLauncher.launch({
   config.port = chrome.port;
   // Don't open a port when running from inside a module (eg. tests). Importing
   // module can control this.
-  const port = process.env.PORT || '3000';
   if (!module.parent) {
-    app.listen(port, function() {
-      console.log('Listening on port', port);
+    app.listen(PORT, function() {
+      console.log('Listening on port', PORT);
     });
   }
   return app;
@@ -162,6 +164,7 @@ const appPromise = chromeLauncher.launch({
 
 
 let exceptionCount = 0;
+
 async function logUncaughtError(error) {
   console.error('Uncaught exception');
   console.error(error);
@@ -170,7 +173,7 @@ async function logUncaughtError(error) {
   if (exceptionCount > 5) {
     console.log(`Detected ${exceptionCount} errors, shutting instance down`);
     if (config && config.chrome)
-      await config.chrome.kill();
+      await app.stop();
     process.exit(1);
   }
 }

src/renderer.js

@@ -26,8 +26,7 @@ class Renderer {
      */
     function getStatusCode() {
       const metaElement = document.querySelector('meta[name="render:status_code"]');
-      if (!metaElement)
-        return undefined;
+      if (!metaElement) return;
       return parseInt(metaElement.getAttribute('content')) || undefined;
     }
 

test/app-test.js

@@ -23,12 +23,19 @@ const express = require('express');
 
 const app = express();
 app.use(express.static(path.resolve(__dirname, 'resources')));
+
+const appInstances = [];
 const testBase = 'http://localhost:1234/';
 
 test.before(async(t) => {
   await app.listen(1234);
 });
 
+test.after.always(async(t) => {
+  for (let app of appInstances) {
+    await app.stop();
+  }
+});
 
 /**
  * This deletes server from the require cache and reloads
@@ -41,6 +48,7 @@ async function createServer(config) {
   const app = await require('../src/main.js');
   if (config)
     app.setConfig(config);
+  appInstances.push(app);
   return request(app);
 }
 
@@ -113,15 +121,15 @@ test('server status code should be forwarded', async(t) => {
 
 test('http status code should be able to be set via a meta tag', async(t) => {
   const server = await createServer();
-  const testFile = path.resolve(__dirname, 'resources/http-meta-status-code.html');
-  const res = await server.get('/render/file://' + testFile + '?wc-inject-shadydom=true');
+  const testFile = 'http-meta-status-code.html';
+  const res = await server.get(`/render/${testBase}${testFile}?wc-inject-shadydom=true`);
   t.is(res.status, 400);
 });
 
 test('http status codes need to be respected from top to bottom', async(t) => {
   const server = await createServer();
-  const testFile = path.resolve(__dirname, 'resources/http-meta-status-code-multiple.html');
-  const res = await server.get('/render/file://' + testFile + '?wc-inject-shadydom=true');
+  const testFile = 'http-meta-status-code-multiple.html';
+  const res = await server.get(`/render/${testBase}${testFile}?wc-inject-shadydom=true`);
   t.is(res.status, 401);
 });
 
@@ -136,7 +144,7 @@ test('screenshot is an image', async(t) => {
 test('invalid url fails', async(t) => {
   const server = await createServer();
   const res = await server.get(`/render/abc`);
-  t.is(res.status, 400);
+  t.is(res.status, 403);
 });
 
 test('unknown url fails', async(t) => {
@@ -145,6 +153,12 @@ test('unknown url fails', async(t) => {
   t.is(res.status, 400);
 });
 
+test('file url fails', async(t) => {
+  const server = await createServer();
+  const res = await server.get(`/render/file:///dev/fd/0`);
+  t.is(res.status, 403);
+});
+
 test('explicit render event ends early', async(t) => {
   const server = await createServer();
   const res = await server.get(`/render/${testBase}explicit-render-event.html`);