bug: remove hardcoded API token and session secret from app.js

README.md

@@ -170,6 +170,19 @@ module.exports = {
 And then require the configuration file and use it to initialize the session.
 However, that still maintains the secret information inside another file, and Snyk Code will warn you about it.
 
+This particular issue has since been fixed: the session is now configured through `utils.session_secret()` and
+`utils.session_cookie()`, which read the following environment variables:
+
+| Variable | Description |
+| --- | --- |
+| `SESSION_SECRET` | Signing secret, at least 32 characters. Required when `NODE_ENV=production` (the app refuses to start otherwise); outside production a random ephemeral secret is generated and sessions do not survive a restart. |
+| `SESSION_COOKIE_SECURE` | `true`/`false` override for the cookie `secure` flag. Defaults to on when `NODE_ENV=production`, off otherwise (so local HTTP development keeps working). |
+
+The cookie is always sent with `httpOnly: true` and `sameSite: 'lax'`.
+
+An unused API token literal that was also declared in `app.js` and printed to the console on startup has been
+removed; the leaked value should be considered compromised and rotated wherever it was issued.
+
 Another case we can discuss here in session management, is that the cookie setting is initialized with `secure: true` which means it will only be transmitted over HTTPS connections. However, there's no `httpOnly` flag set to true, which means that the default false value of it makes the cookie accessible via JavaScript. Snyk Code highlights this potential security misconfiguration so we can fix it. We can note that Snyk Code shows this as a quality information, and not as a security error.
 
 Snyk Code will also find hardcoded secrets in source code that isn't part of the application logic, such as `tests/` or `examples/` folders. We have a case of that in this application with the `tests/authentication.component.spec.js` file. In the finding, Snyk Code will tag it as `InTest`, `Tests`, or `Mock`, which help us easily triage it and indeed ignore this finding as it isn't actually a case of information exposure.

app.js

@@ -24,6 +24,7 @@
 var dustHelpers = require('dustjs-helpers');
 var cons = require('consolidate');
 const hbs = require('hbs')
+var utils = require('./utils');
 
 var app = express();
 var routes = require('./routes');
@@ -40,9 +41,9 @@
 app.use(logger('dev'));
 app.use(methodOverride());
 app.use(session({
-  secret: 'keyboard cat',
+  secret: utils.session_secret(),
   name: 'connect.sid',
-  cookie: { path: '/' }
+  cookie: utils.session_cookie()
 }))
 app.use(bodyParser.json());
 app.use(bodyParser.urlencoded({ extended: false }));
@@ -80,9 +81,6 @@
   app.use(errorHandler());
 }
 
-var token = 'SECRET_TOKEN_f8ed84e8f41e4146403dd4a6bbcea5e418d23a9';
-console.log('token: ' + token);
-
 http.createServer(app).listen(app.get('port'), function () {
   console.log('Express server listening on port ' + app.get('port'));
 });

tests/session-config.test.js

@@ -0,0 +1,62 @@
+var fs = require('fs');
+var path = require('path');
+var test = require('tap').test;
+var utils = require('../utils');
+
+test('session secret is not hard-coded in app.js', function (t) {
+  var app = fs.readFileSync(path.join(__dirname, '..', 'app.js'), 'utf8');
+  t.notMatch(app, /secret\s*:\s*['"`]/, 'no literal session secret in app.js');
+  t.match(app, /secret:\s*utils\.session_secret\(\)/, 'secret comes from utils');
+  t.match(app, /cookie:\s*utils\.session_cookie\(\)/, 'cookie comes from utils');
+  t.end();
+});
+
+test('no hard-coded token is defined or logged in app.js', function (t) {
+  var app = fs.readFileSync(path.join(__dirname, '..', 'app.js'), 'utf8');
+  t.notMatch(app, /SECRET_TOKEN/, 'no token literal in app.js');
+  t.notMatch(app, /token\s*=\s*['"`]/, 'no token assigned from a literal');
+  t.notMatch(app, /console\.log\([^)]*token/i, 'no token written to the log');
+  t.end();
+});
+
+test('session_secret uses SESSION_SECRET when strong enough', function (t) {
+  var secret = 'a'.repeat(32);
+  t.equal(utils.session_secret({ SESSION_SECRET: secret }), secret);
+  t.end();
+});
+
+test('session_secret rejects weak/missing secret in production', function (t) {
+  t.throws(function () {
+    utils.session_secret({ NODE_ENV: 'production' });
+  }, 'throws when unset in production');
+  t.throws(function () {
+    utils.session_secret({ NODE_ENV: 'production', SESSION_SECRET: 'short' });
+  }, 'throws when too short in production');
+  t.end();
+});
+
+test('session_secret falls back to a random secret outside production', function (t) {
+  var first = utils.session_secret({});
+  var second = utils.session_secret({});
+  t.ok(first.length >= 32, 'generated secret is long enough');
+  t.not(first, 'keyboard cat', 'generated secret is not the known value');
+  t.not(first, second, 'generated secret is random per call');
+  t.end();
+});
+
+test('session_cookie sets hardening flags', function (t) {
+  var dev = utils.session_cookie({});
+  t.equal(dev.httpOnly, true, 'httpOnly set');
+  t.equal(dev.sameSite, 'lax', 'sameSite set');
+  t.equal(dev.path, '/', 'path preserved');
+  t.equal(dev.secure, false, 'not secure outside production by default');
+
+  t.equal(utils.session_cookie({ NODE_ENV: 'production' }).secure, true,
+    'secure in production');
+  t.equal(utils.session_cookie({ SESSION_COOKIE_SECURE: 'true' }).secure, true,
+    'secure can be forced on');
+  t.equal(
+    utils.session_cookie({ NODE_ENV: 'production', SESSION_COOKIE_SECURE: 'false' }).secure,
+    false, 'secure can be forced off');
+  t.end();
+});

utils.js

@@ -1,3 +1,7 @@
+var crypto = require('crypto');
+
+var MIN_SESSION_SECRET_LENGTH = 32;
+
 module.exports = {
 
   ran_no : function ( min, max ){
@@ -24,5 +28,42 @@ module.exports = {
     res.setHeader( 'Content-Type', 'text/plain' );
     res.setHeader( 'Content-Length', body.length );
     res.end( body );
+  },
+
+  session_secret : function ( env ){
+    env = env || process.env;
+    var secret = env.SESSION_SECRET;
+
+    if( secret && secret.length >= MIN_SESSION_SECRET_LENGTH ){
+      return secret;
+    }
+
+    if( env.NODE_ENV === 'production' ){
+      throw new Error( 'SESSION_SECRET must be set to at least ' +
+        MIN_SESSION_SECRET_LENGTH + ' characters in production' );
+    }
+
+    console.warn( 'SESSION_SECRET is unset or too short; generating an ' +
+      'ephemeral session secret. Sessions will not survive a restart.' );
+
+    return crypto.randomBytes( 32 ).toString( 'hex' );
+  },
+
+  session_cookie : function ( env ){
+    env = env || process.env;
+
+    var secure = env.NODE_ENV === 'production';
+    if( env.SESSION_COOKIE_SECURE === 'true' ){
+      secure = true;
+    } else if( env.SESSION_COOKIE_SECURE === 'false' ){
+      secure = false;
+    }
+
+    return {
+      path     : '/',
+      httpOnly : true,
+      sameSite : 'lax',
+      secure   : secure
+    };
   }
 };