diff --git a/lib/http/preprocessors.js b/lib/http/preprocessors.js index 75b2a7688..bf195736a 100644 --- a/lib/http/preprocessors.js +++ b/lib/http/preprocessors.js @@ -9,7 +9,7 @@ const { verifyPassword, isValidToken } = require('../util/crypto'); const { isBlank, isPresent, noop, without } = require('../util/util'); -const { isTrue } = require('../util/http'); +const { isTrue, urlDecode } = require('../util/http'); const oidc = require('../util/oidc'); const Problem = require('../util/problem'); const { QueryOptions } = require('../util/db'); @@ -122,10 +122,10 @@ const authHandler = ({ Sessions, Users, Auth }, context) => { // if authentication failed anyway, just do nothing. if ((cxt == null) || !cxt.auth.session.isDefined()) return; - // if csrf missing or mismatch; fail outright. - const csrf = cxt.body.__csrf; - if (isBlank(csrf) || (cxt.auth.session.get().csrf !== decodeURIComponent(csrf))) + const csrf = urlDecode(cxt.body.__csrf); + if (csrf.isEmpty() || isBlank(csrf.get()) || (cxt.auth.session.get().csrf !== csrf.get())) { return reject(Problem.user.authenticationFailed()); + } // delete the token off the body so it doesn't mess with downstream // payload expectations. diff --git a/lib/util/http.js b/lib/util/http.js index 1536284ff..b5300e87e 100644 --- a/lib/util/http.js +++ b/lib/util/http.js @@ -10,7 +10,9 @@ // Helper functions that relate to the HTTP/service layer of the application. const { parse, URL } = require('url'); +const { map, tryCatch } = require('ramda'); const { isBlank } = require('./util'); +const Option = require('./option'); const sanitize = require('sanitize-filename'); @@ -24,6 +26,8 @@ const isFalse = (x) => (!isBlank(x) && typeof x === 'string' && (x.toLowerCase() // Returns just the pathname of the URL, omitting querystring and other non-path decoration. const urlPathname = (x) => parse(x).pathname; +const urlDecode = tryCatch(map(Option.of, decodeURIComponent), Option.none); + //////////////////////////////////////////////////////////////////////////////// // OUTPUT HELPERS @@ -128,7 +132,7 @@ const withEtag = (serverEtag, fn) => (request, response) => { module.exports = { isTrue, isFalse, urlPathname, success, contentType, xml, atom, json, contentDisposition, redirect, - urlWithQueryParams, url, + urlWithQueryParams, url, urlDecode, withEtag }; diff --git a/test/unit/http/preprocessors.js b/test/unit/http/preprocessors.js index 868c12d41..78eb6a6d3 100644 --- a/test/unit/http/preprocessors.js +++ b/test/unit/http/preprocessors.js @@ -303,6 +303,18 @@ describe('preprocessors', () => { ) )).should.be.rejectedWith(Problem, { problemCode: 401.2 })); + it('should reject cookie auth with invalid CSRF token for non-GET requests', () => + Promise.resolve(authHandler( + { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, + new Context( + createRequest({ method: 'POST', headers: { + 'X-Forwarded-Proto': 'https', + Cookie: 'session=alohomora' + }, body: { __csrf: '%ea' }, cookies: { session: 'alohomora' } }), + { fieldKey: Option.none() } + ) + )).should.be.rejectedWith(Problem, { problemCode: 401.2 })); + it('should do nothing on cookie auth with incorrect session token for non-GET requests', () => Promise.resolve(authHandler( { Auth, Sessions: mockSessionsWithCsrf('alohomora', 'secretcsrf') }, diff --git a/test/unit/util/http.js b/test/unit/util/http.js index 3277e5491..818d15d1f 100644 --- a/test/unit/util/http.js +++ b/test/unit/util/http.js @@ -1,5 +1,6 @@ const appRoot = require('app-root-path'); const http = require(appRoot + '/lib/util/http'); +const Option = require(appRoot + '/lib/util/option'); describe('util/http', () => { describe('isTrue', () => { @@ -96,5 +97,33 @@ describe('util/http', () => { urlWithQueryParams('/path?x=1&y=2&z=3', { x: null, z: undefined }).should.equal('/path?y=2'); }); }); + + describe('urlDecode()', () => { + const { urlDecode } = http; + + [ + [ '', '' ], + [ '%20', ' ' ], + [ 'abc123', 'abc123' ], + ].forEach(([ decodable, expected ]) => { + it(`should successfully decode '${decodable}' to Option.of('${expected}')`, () => { + const decoded = urlDecode(decodable); + (decoded instanceof Option).should.equal(true); + decoded.isDefined().should.equal(true); + decoded.get().should.equal(expected); + }); + }); + + [ + '%', + '%ae', + ].forEach(undecodable => { + it(`should decode '${undecodable}' to Option.None`, () => { + const decoded = urlDecode(undecodable); + (decoded instanceof Option).should.equal(true); + decoded.isEmpty().should.equal(true); + }); + }); + }); });