diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index ef7318ac35..7aaf242c8f 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -7,8 +7,8 @@ const Metrics = require('@overleaf/metrics') const logger = require('@overleaf/logger') const querystring = require('querystring') const Settings = require('@overleaf/settings') -const basicAuth = require('basic-auth-connect') -const crypto = require('crypto') +const basicAuth = require('basic-auth') +const tsscmp = require('tsscmp') const UserHandler = require('../User/UserHandler') const UserSessionsManager = require('../User/UserSessionsManager') const SessionStoreManager = require('../../infrastructure/SessionStoreManager') @@ -29,6 +29,20 @@ function send401WithChallenge(res) { res.sendStatus(401) } +function checkCredentials(userDetailsMap, user, password) { + const expectedPassword = userDetailsMap.get(user) + const userExists = userDetailsMap.has(user) && expectedPassword // user exists with a non-null password + const isValid = userExists && tsscmp(expectedPassword, password) + if (!isValid) { + logger.err({ user }, 'invalid login details') + } + Metrics.inc('security.http-auth.check-credentials', 1, { + path: userExists ? 'known-user' : 'unknown-user', + status: isValid ? 'pass' : 'fail', + }) + return isValid +} + const AuthenticationController = { serializeUser(user, callback) { if (!user._id || !user.email) { @@ -335,17 +349,20 @@ const AuthenticationController = { }, requireBasicAuth: function (userDetails) { - return basicAuth(function (user, pass) { - const expectedPassword = userDetails[user] - const isValid = - expectedPassword && - expectedPassword.length === pass.length && - crypto.timingSafeEqual(Buffer.from(expectedPassword), Buffer.from(pass)) - if (!isValid) { - logger.err({ user }, 'invalid login details') + const userDetailsMap = new Map(Object.entries(userDetails)) + return function (req, res, next) { + const credentials = basicAuth(req) + if ( + !credentials || + !checkCredentials(userDetailsMap, credentials.name, credentials.pass) + ) { + send401WithChallenge(res) + Metrics.inc('security.http-auth', 1, { status: 'reject' }) + } else { + Metrics.inc('security.http-auth', 1, { status: 'accept' }) + next() } - return isValid - }) + } }, requirePrivateApiAuth() { diff --git a/services/web/package-lock.json b/services/web/package-lock.json index 756b5c9765..97bda4ce75 100644 --- a/services/web/package-lock.json +++ b/services/web/package-lock.json @@ -14084,11 +14084,6 @@ } } }, - "basic-auth-connect": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/basic-auth-connect/-/basic-auth-connect-1.0.0.tgz", - "integrity": "sha512-kiV+/DTgVro4aZifY/hwRwALBISViL5NP4aReaR2EVJEObpbUBHIkdJh/YpcoEiYt7nBodZ6U2ajZeZvSxUCCg==" - }, "batch": { "version": "0.6.1", "resolved": "https://registry.npmjs.org/batch/-/batch-0.6.1.tgz", @@ -21109,7 +21104,7 @@ "find-up": { "version": "2.1.0", "resolved": "https://registry.npmjs.org/find-up/-/find-up-2.1.0.tgz", - "integrity": "sha1-RdG35QbHF93UgndaK3eSCjwMV6c=", + "integrity": "sha512-NWzkk0jSJtTt08+FBFMvXoeZnOJD+jTtsRmBYbAIzJdX6l7dLgR7CTubCM5/eDdPUBvLCeVasP1brfVR/9/EZQ==", "dev": true, "requires": { "locate-path": "^2.0.0" @@ -26182,7 +26177,7 @@ "locate-path": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/locate-path/-/locate-path-2.0.0.tgz", - "integrity": "sha1-K1aLJl7slExtnA3pw9u7ygNUzY4=", + "integrity": "sha512-NCI2kiDkyR7VeEKm27Kda/iQHyKJe1Bu0FlTbYp3CqJu+9IFe9bLyAjMxf5ZDDbEg+iMPzB5zYyUTSm8wVTKmA==", "dev": true, "requires": { "p-locate": "^2.0.0", @@ -29624,7 +29619,7 @@ "p-locate": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/p-locate/-/p-locate-2.0.0.tgz", - "integrity": "sha1-IKAQOyIqcMj9OcwuWAaA893l7EM=", + "integrity": "sha512-nQja7m7gSKuewoVRen45CtVfODR3crN3goVQ0DDZ9N3yHxgpkuBhZqsaiotSQRrADUrne346peY7kT3TSACykg==", "dev": true, "requires": { "p-limit": "^1.1.0" @@ -29642,7 +29637,7 @@ "p-try": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/p-try/-/p-try-1.0.0.tgz", - "integrity": "sha1-y8ec26+P1CKOE/Yh8rGiN8GyB7M=", + "integrity": "sha512-U1etNYuMJoIz3ZXSrrySFjsXQTWOx2/jdi86L+2pRvph/qMKL6sbcCYdH23fqsbm8TH2Gn0OybpT4eSFlCVHww==", "dev": true } } diff --git a/services/web/package.json b/services/web/package.json index a7e449cbd9..35ba819426 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -91,7 +91,7 @@ "archiver": "4.0.1", "async": "0.6.2", "backbone": "^1.3.3", - "basic-auth-connect": "^1.0.0", + "basic-auth": "^2.0.1", "bcrypt": "^5.0.0", "body-parser": "^1.19.0", "bootstrap": "^3.4.1", diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 8ec78545a1..0f95ee0012 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -644,8 +644,8 @@ describe('AuthenticationController', function () { this.req.headers = { authorization: `Basic ${Buffer.from('user:nope').toString('base64')}`, } - this.req.end = status => { - expect(status).to.equal('Unauthorized') + this.req.sendStatus = status => { + expect(status).to.equal(401) done() } this.middleware(this.req, this.req) @@ -657,8 +657,8 @@ describe('AuthenticationController', function () { 'base64' )}`, } - this.req.end = status => { - expect(status).to.equal('Unauthorized') + this.req.sendStatus = status => { + expect(status).to.equal(401) done() } this.middleware(this.req, this.req) @@ -670,8 +670,45 @@ describe('AuthenticationController', function () { 'base64' )}`, } - this.req.end = status => { - expect(status).to.equal('Unauthorized') + this.req.sendStatus = status => { + expect(status).to.equal(401) + done() + } + this.middleware(this.req, this.req) + }) + + it('should fail with empty user and password of "undefined"', function (done) { + this.req.headers = { + authorization: `Basic ${Buffer.from(`:undefined`).toString( + 'base64' + )}`, + } + this.req.sendStatus = status => { + expect(status).to.equal(401) + done() + } + this.middleware(this.req, this.req) + }) + + it('should fail with empty user and empty password', function (done) { + this.req.headers = { + authorization: `Basic ${Buffer.from(`:`).toString('base64')}`, + } + this.req.sendStatus = status => { + expect(status).to.equal(401) + done() + } + this.middleware(this.req, this.req) + }) + + it('should fail with a user that is not a valid property', function (done) { + this.req.headers = { + authorization: `Basic ${Buffer.from( + `constructor:[Function ]` + ).toString('base64')}`, + } + this.req.sendStatus = status => { + expect(status).to.equal(401) done() } this.middleware(this.req, this.req)