CSRF token: handle failed url-decoding (#1265)

getodk · Nov 9, 2024 · 9ba21ef · 9ba21ef
1 parent db0af6a
commit 9ba21ef
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 5 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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);
+      });
+    });
+  });
 });