Security checklist

CGI/cgi-conf.js

@@ -75,6 +75,9 @@ exports.get_config = (cgi_config, script_name) => {
 		} else {
 			let message = "repository output is required, but not all values are provided.\n"
 			let message2 = "are needed: ghname, ghemail, ghtoken, ghrepo, ghpath\n"
+			if(retval.ghtoken !== undefined) retval.ghtoken = "(hidden)";
+			if(retval.ghemail !== undefined) retval.ghemail = "(hidden)";
+			if(retval.ghname !== undefined)  retval.ghname  = "(hidden)";
 			throw new Error(message + message2 + JSON.stringify(retval, null, 2));
 		}
 	}

CGI/driver/presets.js

@@ -205,6 +205,9 @@ function store_preset() {
  * There is, however, a CORS issue. The IRC log does not have the CORS header set and I could not
  * get `fetch` work properly with the relevant header (why???). For now I use the
  * `https://cors-anywhere.herokuapp.com` trick, and I may have to come back to this later.
+ * 
+ * Note also that the general part includes some checks for the URL used in fetch, it is not necessary here. Indeed
+ * the fetch is used exclusively on a URL generated internally using the W3C setup, so there is no danger there.
  */
 function load_log() {
 	set_input_url = function(date, group) {

conf.js

@@ -146,6 +146,9 @@ exports.get_config = () => {
 		} else {
 			let message = "repository output is required, but not all values are provided.\n"
 			let message2 = "are needed: ghname, ghemail, ghtoken, ghrepo, ghpath\n"
+			if(retval.ghtoken !== undefined) retval.ghtoken = "(hidden)";
+			if(retval.ghemail !== undefined) retval.ghemail = "(hidden)";
+			if(retval.ghname !== undefined)  retval.ghname  = "(hidden)";
 			throw new Error(message + message2 + JSON.stringify(retval, null, 2));
 		}
 	}

convert.js

@@ -1,7 +1,8 @@
 "use strict";
 
-const _   = require('underscore');
-const url = require('url');
+const _    = require('underscore');
+const url  = require('url');
+const safe = require('safe-regex');
 
 const JEKYLL_NONE		= "none";
 const JEKYLL_MARKDOWN	= "md";
@@ -499,15 +500,18 @@ exports.to_markdown = (body, config) => {
 				// the array index later...)
 				let r = get_change_request(line.content);
 	  			if(r !== null) {
-	  				change_requests.push({
-	  					lineno : index,
-	  					from   : r[1],
-	  					to     : r[2],
-	  					g      : r[3] === "g",
-	  					G      : r[3] === "G",
-						valid  : true
-	  				});
-	  				line.content = marker;
+					// Check whether the 'from' field is 'safe', ie, it does not create RegExp Denial of Service attack
+					if(safe(r[1])) {
+						change_requests.push({
+							lineno : index,
+							from   : r[1],
+							to     : r[2],
+							g      : r[3] === "g",
+							G      : r[3] === "G",
+						  	valid  : true
+						});
+						line.content = marker;  
+					}
 				}
 	  			return line
 			})

io.js

@@ -12,11 +12,12 @@
  * For the moment, the default (ie, 'master') branch is used.
  *
  */
-const url     = require('url');
-const fetch   = require('node-fetch');
-const fs      = require('fs');
-const _       = require('underscore');
-const octokat = require('octokat');
+const url      = require('url');
+const fetch    = require('node-fetch');
+const fs       = require('fs');
+const _        = require('underscore');
+const octokat  = require('octokat');
+const validUrl = require('valid-url'); 
 
 /**
  * Get the IRC log. The input provided in the configuration is examined whether it is a URL (in which case
@@ -37,9 +38,13 @@ exports.get_irc_log = (conf) => {
 			fetch(conf.input)
 				.then((response) => {
 					if(response.ok) {
-						return response.text()
+						if( response.headers.get('content-type') === 'text/plain' ) {
+							return response.text();							
+						} else {
+							reject(`IRC log must be of type text/plain`);
+						}
 					} else {
-						throw new Error(`HTTP response ${response.status}: ${response.statusText}`);
+						reject(`HTTP response ${response.status}: ${response.statusText}`);
 					}
 				})
 				.then((body) => {
@@ -86,19 +91,31 @@ exports.get_nick_mapping = (conf) => {
 	}
 	return new Promise((resolve, reject) => {
 		if(conf.nicknames) {
-			if(url.parse(conf.nicknames).protocol !== null) {
+			let address = check_url(conf.nicknames)
+			if(address !== null) {
 				// This is a resource on the Web that must be fetched
-				fetch(conf.nicknames)
+				fetch(address)
 					.then((response) => {
 						if(response.ok) {
-							return response.json()
+							// I need to check whether the returned data is genuine json; however,
+							// alas!, github does not set the content type of the returned raw data
+							// to json, ie, the response header cannot be used for that purpose.
+							// Instead, the value is sent to a next step to parse it explicitly
+							return response.text();							
 						} else {
-							throw new Error(`HTTP response ${response.status}: ${response.statusText}`);
+							reject(`HTTP response ${response.status}: ${response.statusText}`);
 						}
 					})
 					.then((body) => {
-						// Resolve the returned Promise
-						resolve(lower_nicks(body));
+						// Try to parse the content as JSON and, if it works, that is almost 
+						// the final result, module turn all the nicknames to lowercase
+						let json_content = {};
+						try {
+							json_content = JSON.parse(body);
+						} catch(err) {
+							throw new Error(`JSON parsing error in ${conf.nicknames}: ${err}`)
+						}
+						resolve(lower_nicks(json_content));
 					})
 					.catch((err) => {
 						reject(`problem accessing remote file ${conf.nicknames}: ${err.message}`)
@@ -108,9 +125,17 @@ exports.get_nick_mapping = (conf) => {
 				// though, I believe, a sync function would work just as well
 				fs.readFile(conf.nicknames, 'utf-8', (err, body) => {
 					if(err) {
-						reject(`problem access local file ${conf.nicknames}: ${err}`);
+						reject(`problem accessing local file ${conf.nicknames}: ${err}`);
 					} else {
-						resolve(lower_nicks(JSON.parse(body)));
+						// Try to parse the content as JSON and, if it works, that is almost 
+						// the final result, module turn all the nicknames to lowercase
+						let json_content = {};
+						try {
+							json_content = JSON.parse(body);
+						} catch(err) {
+							throw new Error(`JSON parsing error in ${conf.nicknames}: ${err}`)
+						}
+						resolve(lower_nicks(json_content));
 					}
 				});
 			}
@@ -158,6 +183,60 @@ exports.output_minutes = (minutes, conf) => {
 }
 
 
+/** 
+* Basic sanity check on the URL.
+*
+* The function returns a (possibly slightly modified) version of the URL if everything is fine, or a null value if
+* the input argument is not a URL (but should be used as a filename)
+*
+* There might be errors, however, in the case it is a URL. In such cases the function raises an exception; this
+* should be caught to end all processing.
+*
+* The checks are as follows:
+*
+* 1. Check whether the protocol is http(s). Other protocols are not accepted (actually rejected by fetch, too)
+* 2. Run the URL through a valid-url check, which looks at the validity of the URL in terms of characters used, for example
+* 3. Check that the port (if specified) is in the allowed range, ie, > 1024
+* 4. Don't allow localhost in a CGI answer...
+* 
+* @param {string} address: the URL to be checked.
+* @return {string}: the URL itself (which might be slightly improved by the valid-url method) or null this is, in fact, not a URL
+* @throws {exception}: if it pretends to be a URL, but it is not acceptable for some reasons.
+*/
+function check_url(address) {
+    let parsed = url.parse(address);
+    if( parsed.protocol === null ) {
+        // This is not a URl, should be used as a file name
+        return null;
+    }
+    // Check whether we use the right protocol
+    if( _.contains(["http:", "https:"], parsed.protocol) === false ) {
+        throw new Error(`Only http(s) url-s are accepted (${address})`)
+    }
+
+    // Run through the URL validator
+    let retval = validUrl.isWebUri(address);
+    if( retval === undefined ) {
+        throw new Error(`The url ${address} isn't valid`)
+    }
+
+    // Check the port
+    if(parsed.port !== null && parsed.port <= 1024) {
+        throw new Error(`Unsafe port number used in ${address} (${parsed.port})`)
+    }
+
+	// Don't allow local host in a CGI script...
+	// (In Bratt's python script (<http://dev.w3.org/2004/PythonLib-IH/checkremote.py>) this step was much
+	// more complex, and has not yet been reproduced here...
+    if( parsed.hostname === "localhost" || parsed.hostname === "127.0.0.1" ) {
+        throw new Error(`Localhost is not accepted in ${address}`)        
+    }
+
+    // If we got this far, this is a proper URL, ready to be used.
+    return retval;
+}
+
+
 /**
  * Committing new data on the github repo
  *

package.json

@@ -32,7 +32,9 @@
     "moment": "^2.18.1",
     "node-fetch": "^1.7.1",
     "octokat": "^0.9.0",
-    "underscore": "^1.8.3"
+    "safe-regex": "^1.1.0",
+    "underscore": "^1.8.3",
+    "valid-url": "^1.0.9"
   },
   "devDependencies": {
     "mocha": "3.5.0",