Sessions.getByBearerToken(): check format before hitting db (#1263)

getodk · Nov 7, 2024 · a1c49cb · a1c49cb
1 parent a0cb196
commit a1c49cb
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 6 deletions.
diff --git a/lib/http/preprocessors.js b/lib/http/preprocessors.js
@@ -7,7 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { verifyPassword } = require('../util/crypto');
+const { verifyPassword, isValidToken } = require('../util/crypto');
 const { isBlank, isPresent, noop, without } = require('../util/util');
 const { isTrue } = require('../util/http');
 const oidc = require('../util/oidc');
@@ -53,7 +53,7 @@ const authHandler = ({ Sessions, Users, Auth }, context) => {
     // should explain /somewhere/.)
 
     const key = context.fieldKey.get();
-    if (!/^[a-z0-9!$]{64}$/i.test(key)) return reject(Problem.user.authenticationFailed());
+    if (!isValidToken(key)) return reject(Problem.user.authenticationFailed());
 
     return Sessions.getByBearerToken(key)
       .then(getOrReject(Problem.user.insufficientRights()))

diff --git a/lib/model/query/sessions.js b/lib/model/query/sessions.js
@@ -10,9 +10,10 @@
 const { sql } = require('slonik');
 const { map } = require('ramda');
 const { Actor, Session } = require('../frames');
-const { generateToken } = require('../../util/crypto');
+const { generateToken, isValidToken } = require('../../util/crypto');
 const { unjoiner } = require('../../util/db');
 const { construct } = require('../../util/util');
+const Option = require('../../util/option');
 
 const aDayFromNow = () => {
   const date = new Date();
@@ -27,11 +28,11 @@ returning *`)
   .then(construct(Session));
 
 const _unjoiner = unjoiner(Session, Actor);
-const getByBearerToken = (token) => ({ maybeOne }) => maybeOne(sql`
+const getByBearerToken = (token) => ({ maybeOne }) => (isValidToken(token) ? maybeOne(sql`
 select ${_unjoiner.fields} from sessions
 join actors on actors.id=sessions."actorId"
 where token=${token} and sessions."expiresAt" > now()`)
-  .then(map(_unjoiner));
+  .then(map(_unjoiner)) : Option.none());
 
 const terminateByActorId = (actorId, current = undefined) => ({ run }) =>
   run(sql`DELETE FROM sessions WHERE "actorId"=${actorId}

diff --git a/lib/util/crypto.js b/lib/util/crypto.js
@@ -43,6 +43,8 @@ const generateToken = () => randomBytes(48).toString('base64')
   .replace(/\//g, '!')
   .replace(/\+/g, '$');
 
+const isValidToken = (token) => /^[A-Za-z0-9!$]{64}$/.test(token);
+
 
 ////////////////////////////////////////////////////////////////////////////////
 // HASH UTIL
@@ -307,7 +309,7 @@ const submissionDecryptor = (keys, passphrases) =>
 
 
 module.exports = {
-  hashPassword, verifyPassword, generateToken,
+  hashPassword, verifyPassword, generateToken, isValidToken,
   shasum, sha256sum, md5sum, digestWith,
   getPrivate, generateManagedKey, generateVersionSuffix,
   stripPemEnvelope,

diff --git a/test/unit/util/crypto.js b/test/unit/util/crypto.js
@@ -51,6 +51,33 @@ describe('util/crypto', () => {
     });
   });
 
+  describe('isValidToken()', () => {
+    const { generateToken, isValidToken } = crypto;
+
+    [
+      generateToken(), generateToken(), generateToken(), generateToken(),
+      generateToken(), generateToken(), generateToken(), generateToken(),
+      generateToken(), generateToken(), generateToken(), generateToken(),
+      generateToken(), generateToken(), generateToken(), generateToken(),
+    ].forEach(validToken => {
+      it(`should return true for valid token '${validToken}'`, () => {
+        isValidToken(validToken).should.be.true();
+      });
+    });
+
+    [
+      undefined,
+      null,
+      '',
+      generateToken() + 'a',
+      generateToken().substr(1),
+    ].forEach(invalidToken => {
+      it(`return false for invalid token '${invalidToken}'`, () => {
+        isValidToken(invalidToken).should.be.false();
+      });
+    });
+  });
+
   describe('generateVersionSuffix', () => {
     const { generateVersionSuffix } = crypto;
     it('should generate a suffix', () => {