Corrected handling of HTTP Basic edge cases

Previous implementation:
empty user-pass  -> error basic realm
no " " separator -> error 400
no : separator   -> error basic realm
empty password   -> error basic realm
empty username   -> error basic realm

New implementation:
empty user-pass  -> error 400
no " " separator -> error 400
no : separator   -> error 400
empty password   -> success
empty username   -> success

Also fixed passwords containing ':' being truncated.

This fixes:
- jaredhanson/passport-http#20
- jaredhanson/passport-http#41
- jaredhanson/passport-http#42
- jaredhanson/passport-http#63
- jaredhanson/passport-http#78

The new implemementation complies with
https://tools.ietf.org/html/rfc2617#section-2.

Author: MatthiasKunnen

lib/strategies/basic.js

@@ -3,6 +3,7 @@
  */
 const passport = require('passport-strategy');
 const util = require('util');
+const { splitFirst } = require('../string.util');
 
 /**
  * `BasicStrategy` constructor.
@@ -67,19 +68,14 @@ BasicStrategy.prototype.authenticate = function (req) {
 
     const scheme = parts[0];
     const userPass = Buffer.from(parts[1], 'base64').toString();
-    const credentials = userPass.split(':');
+    const [userid, password] = splitFirst(userPass, ':');
 
     if (!/Basic/i.test(scheme)) {
         return this.fail(this._challenge());
     }
-    if (credentials.length < 2) {
-        return this.fail(400);
-    }
 
-    const userid = credentials[0];
-    const password = credentials[1];
-    if (!userid || !password) {
-        return this.fail(this._challenge());
+    if (userid === undefined || password === undefined) {
+        return this.fail(400);
     }
 
     const self = this;

lib/string.util.js

@@ -0,0 +1,21 @@
+module.exports = {
+
+    /**
+     * Splits a string on the first occurrence of the provided separator.
+     * Returns an array with one or two elements.
+     * @param {String} string
+     * @param {String} separator
+     */
+    splitFirst: (string, separator) => {
+        const separatorIndex = string.indexOf(separator);
+
+        if (separatorIndex < 0) {
+            return [string];
+        }
+
+        return [
+            string.substring(0, separatorIndex),
+            string.substring(separatorIndex + 1),
+        ];
+    },
+};

test/strategies/basic-test.js

@@ -179,7 +179,7 @@ vows.describe('BasicStrategy').addBatch({
         },
     },
 
-    'strategy handling a request with credentials lacking a password': {
+    'strategy handling a request with credentials lacking the " " separator': {
         topic: function () {
             return new BasicStrategy(((userid, password, done) => {
                 done(null, {username: userid, password: password});
@@ -197,7 +197,7 @@ vows.describe('BasicStrategy').addBatch({
                 };
 
                 req.headers = {};
-                req.headers.authorization = 'Basic Ym9iOg==';
+                req.headers.authorization = 'Basic';
                 process.nextTick(() => {
                     strategy.authenticate(req);
                 });
@@ -206,12 +206,12 @@ vows.describe('BasicStrategy').addBatch({
             'should fail authentication with challenge': function (err, challenge) {
                 // fail action was called, resulting in test callback
                 assert.isNull(err);
-                assert.strictEqual(challenge, 'Basic realm="Users"');
+                assert.strictEqual(challenge, 400);
             },
         },
     },
 
-    'strategy handling a request with credentials lacking a username': {
+    'strategy handling a request with credentials containing an empty user-pass': {
         topic: function () {
             return new BasicStrategy(((userid, password, done) => {
                 done(null, {username: userid, password: password});
@@ -229,7 +229,7 @@ vows.describe('BasicStrategy').addBatch({
                 };
 
                 req.headers = {};
-                req.headers.authorization = 'Basic OnNlY3JldA==';
+                req.headers.authorization = 'Basic ';
                 process.nextTick(() => {
                     strategy.authenticate(req);
                 });
@@ -238,7 +238,139 @@ vows.describe('BasicStrategy').addBatch({
             'should fail authentication with challenge': function (err, challenge) {
                 // fail action was called, resulting in test callback
                 assert.isNull(err);
-                assert.strictEqual(challenge, 'Basic realm="Users"');
+                assert.strictEqual(challenge, 400);
+            },
+        },
+    },
+
+    'strategy handling a request with credentials lacking the ":" separator': {
+        topic: function () {
+            return new BasicStrategy(((userid, password, done) => {
+                done(null, {username: userid, password: password});
+            }));
+        },
+
+        'after augmenting with actions': {
+            topic: function (strategy) {
+                const req = {};
+                strategy.success = (user) => {
+                    this.callback(new Error('should not be called'));
+                };
+                strategy.fail = (challenge) => {
+                    this.callback(null, challenge);
+                };
+
+                req.headers = {};
+                req.headers.authorization = 'Basic Ym9i'; // bob
+                process.nextTick(() => {
+                    strategy.authenticate(req);
+                });
+            },
+
+            'should fail authentication with challenge': function (err, challenge) {
+                // fail action was called, resulting in test callback
+                assert.isNull(err);
+                assert.strictEqual(challenge, 400);
+            },
+        },
+    },
+
+    'strategy handling a request with credentials containing an empty username': {
+        topic: function () {
+            return new BasicStrategy(((userid, password, done) => {
+                done(null, {username: userid, password: password});
+            }));
+        },
+
+        'after augmenting with actions': {
+            topic: function (strategy) {
+                const req = {};
+                strategy.success = (user) => {
+                    this.callback(null, user);
+                };
+                strategy.fail = (challenge) => {
+                    this.callback(new Error('should not be called'));
+                };
+
+                req.headers = {};
+                req.headers.authorization = 'Basic OnBhc3N3b3Jk'; // :password
+                process.nextTick(() => {
+                    strategy.authenticate(req);
+                });
+            },
+
+            'should not generate an error': (err, user) => {
+                assert.isNull(err);
+            },
+            'should authenticate': (err, user) => {
+                assert.strictEqual(user.username, '');
+                assert.strictEqual(user.password, 'password');
+            },
+        },
+    },
+
+    'strategy handling a request with credentials containing an empty password': {
+        topic: function () {
+            return new BasicStrategy(((userid, password, done) => {
+                done(null, {username: userid, password: password});
+            }));
+        },
+
+        'after augmenting with actions': {
+            topic: function (strategy) {
+                const req = {};
+                strategy.success = (user) => {
+                    this.callback(null, user);
+                };
+                strategy.fail = (challenge) => {
+                    this.callback(new Error('should not be called'));
+                };
+
+                req.headers = {};
+                req.headers.authorization = 'Basic Ym9iOg=='; // bob:
+                process.nextTick(() => {
+                    strategy.authenticate(req);
+                });
+            },
+
+            'should not generate an error': (err, user) => {
+                assert.isNull(err);
+            },
+            'should authenticate': (err, user) => {
+                assert.strictEqual(user.username, 'bob');
+                assert.strictEqual(user.password, '');
+            },
+        },
+    },
+
+    'strategy handling a request containing a colon in the password': {
+        topic: function () {
+            return new BasicStrategy((userid, password, done) => {
+                done(null, {username: userid, password: password});
+            });
+        },
+        'after augmenting with actions': {
+            topic: function (strategy) {
+                const req = {};
+                strategy.success = user => {
+                    this.callback(null, user);
+                };
+                strategy.fail = () => {
+                    this.callback(new Error('should not be called'));
+                };
+
+                req.headers = {};
+                req.headers.authorization = 'Basic Ym9iOnNlY3JldDpwdw=='; // bob:secret:pw
+                process.nextTick(() => {
+                    strategy.authenticate(req);
+                });
+            },
+            'should not generate an error': (err, user) => {
+                assert.isNull(err);
+            },
+            'should authenticate': (err, user) => {
+                assert.strictEqual(user.username, 'bob');
+                assert.strictEqual(user.password, 'secret:pw');
             },
         },
     },

test/string.util-test.js

@@ -0,0 +1,24 @@
+const vows = require('vows');
+const assert = require('assert');
+const stringUtils = require('../lib/string.util');
+const {splitFirst} = stringUtils;
+
+vows.describe('string utils').addBatch({
+
+    'module': {
+        'should export splitFirst': () => {
+            assert.isFunction(stringUtils.splitFirst);
+        },
+    },
+
+    'splitFirst': {
+        'should split only first': () => {
+            assert.deepStrictEqual(splitFirst('bob:secret:pw', ':'), ['bob', 'secret:pw']);
+            assert.deepStrictEqual(splitFirst('a:bb:a:d', ':'), ['a', 'bb:a:d']);
+        },
+        'should handle non-existing seperator': () => {
+            assert.deepStrictEqual(splitFirst('abc', ':'), ['abc']);
+        },
+    },
+
+}).export(module);