fix: MFA SMS one-time password accepted twice under concurrent login ([GHSA-jpq4-7fmq-q5fj](GHSA-jpq4-7fmq-q5fj)) (#10448)

mtrezza · web-flow · commit 725be0d602ba · 2026-04-26T03:10:27.000+01:00
diff --git a/spec/vulnerabilities.spec.js b/spec/vulnerabilities.spec.js
@@ -4995,6 +4995,115 @@ describe('Vulnerabilities', () => {
     });
   });
 
+  describe('(GHSA-jpq4-7fmq-q5fj) SMS MFA single-use token reuse via concurrent requests', () => {
+    const mfaHeaders = {
+      'X-Parse-Application-Id': 'test',
+      'X-Parse-REST-API-Key': 'rest',
+      'Content-Type': 'application/json',
+    };
+
+    let sentToken;
+
+    beforeEach(async () => {
+      sentToken = null;
+      await reconfigureServer({
+        auth: {
+          mfa: {
+            enabled: true,
+            options: ['SMS'],
+            algorithm: 'SHA1',
+            digits: 6,
+            period: 30,
+            sendSMS: token => {
+              sentToken = token;
+            },
+          },
+        },
+      });
+    });
+
+    async function setupSmsMfaUser() {
+      const user = await Parse.User.signUp('smsmfauser', 'password123');
+      // Enroll SMS MFA
+      await request({
+        method: 'PUT',
+        url: `http://localhost:8378/1/users/${user.id}`,
+        headers: {
+          ...mfaHeaders,
+          'X-Parse-Session-Token': user.getSessionToken(),
+        },
+        body: JSON.stringify({
+          authData: { mfa: { mobile: '+15551234567' } },
+        }),
+      });
+      const enrollToken = sentToken;
+      // Confirm enrollment with the received OTP
+      await request({
+        method: 'PUT',
+        url: `http://localhost:8378/1/users/${user.id}`,
+        headers: {
+          ...mfaHeaders,
+          'X-Parse-Session-Token': user.getSessionToken(),
+        },
+        body: JSON.stringify({
+          authData: { mfa: { mobile: '+15551234567', token: enrollToken } },
+        }),
+      });
+      sentToken = null;
+      return user;
+    }
+
+    async function requestLoginOtp(username, password) {
+      try {
+        await request({
+          method: 'POST',
+          url: 'http://localhost:8378/1/login',
+          headers: mfaHeaders,
+          body: JSON.stringify({
+            username,
+            password,
+            authData: { mfa: { token: 'request' } },
+          }),
+        });
+      } catch (_err) {
+        // Expected: adapter throws "Please enter the token"
+      }
+      return sentToken;
+    }
+
+    it('rejects concurrent logins using the same SMS MFA OTP', async () => {
+      const user = await setupSmsMfaUser();
+      const otp = await requestLoginOtp('smsmfauser', 'password123');
+      expect(otp).toBeDefined();
+
+      const loginWithOtp = () =>
+        request({
+          method: 'POST',
+          url: 'http://localhost:8378/1/login',
+          headers: mfaHeaders,
+          body: JSON.stringify({
+            username: 'smsmfauser',
+            password: 'password123',
+            authData: { mfa: { token: otp } },
+          }),
+        });
+
+      const results = await Promise.allSettled(Array(10).fill().map(() => loginWithOtp()));
+
+      const succeeded = results.filter(r => r.status === 'fulfilled');
+      const failed = results.filter(r => r.status === 'rejected');
+
+      // Exactly one request should succeed; all others should fail
+      expect(succeeded.length).toBe(1);
+      expect(failed.length).toBe(9);
+
+      // Verify the OTP has been consumed
+      await user.fetch({ useMasterKey: true });
+      const mfa = user.get('authData').mfa;
+      expect(mfa.token).toBeUndefined();
+    });
+  });
+
   describe('(GHSA-p2w6-rmh7-w8q3) SQL Injection via aggregate and distinct field names in PostgreSQL adapter', () => {
     const headers = {
       'Content-Type': 'application/json',
diff --git a/src/AuthDataLock.js b/src/AuthDataLock.js
@@ -0,0 +1,47 @@
+// Apply optimistic locking for authData provider field changes. For each lockable
+// top-level field in the original authData whose value differs from the incoming
+// value, add an equality constraint for the original value to the update WHERE
+// clause. Concurrent requests racing the same single-use token will only allow the
+// first update to match; subsequent updates miss and surface as OBJECT_NOT_FOUND.
+//
+// Only fields whose values round-trip cleanly through both storage adapters are
+// locked: primitives (string, number, boolean) and arrays. Date values and nested
+// objects are skipped because their JSON representation differs between the
+// MongoDB and Postgres adapters, and because Parse Server's query-key validator
+// rejects deeper paths containing characters like `+` (e.g. phone-number keys).
+// Locking the consumed single-use credential (the MFA token string or the
+// recovery-code array) is sufficient — its removal invalidates the WHERE clause
+// for concurrent writers.
+export function applyAuthDataOptimisticLock(query, originalAuthData, newAuthData) {
+  if (!originalAuthData) {
+    return;
+  }
+  for (const provider of Object.keys(newAuthData)) {
+    const original = originalAuthData[provider];
+    if (!original || typeof original !== 'object') {
+      continue;
+    }
+    for (const [field, value] of Object.entries(original)) {
+      if (!isLockableAuthDataValue(value)) {
+        continue;
+      }
+      if (JSON.stringify(value) !== JSON.stringify(newAuthData[provider]?.[field])) {
+        query[`authData.${provider}.${field}`] = value;
+      }
+    }
+  }
+}
+
+function isLockableAuthDataValue(value) {
+  if (value === null || value === undefined) {
+    return false;
+  }
+  const t = typeof value;
+  if (t === 'string' || t === 'number' || t === 'boolean') {
+    return true;
+  }
+  if (Array.isArray(value)) {
+    return true;
+  }
+  return false;
+}
diff --git a/src/RestWrite.js b/src/RestWrite.js
@@ -17,6 +17,7 @@ import _ from 'lodash';
 import logger from './logger';
 import { requiredColumns } from './Controllers/SchemaController';
 import { createSanitizedError } from './Error';
+import { applyAuthDataOptimisticLock } from './AuthDataLock';
 
 // query and data are both provided in REST API format. So data
 // types are encoded by plain old objects.
@@ -658,20 +659,20 @@ RestWrite.prototype.handleAuthData = async function (authData) {
         this.authDataResponse = res.authDataResponse;
       }
 
+      // Capture original authData before mutating userResult via the response reference
+      const originalAuthData = userResult?.authData
+        ? Object.fromEntries(
+          Object.entries(userResult.authData).map(([k, v]) =>
+            [k, v && typeof v === 'object' ? { ...v } : v]
+          )
+        )
+        : undefined;
+
       // IF we are in login we'll skip the database operation / beforeSave / afterSave etc...
       // we need to set it up there.
       // We are supposed to have a response only on LOGIN with authData, so we skip those
       // If we're not logging in, but just updating the current user, we can safely skip that part
       if (this.response) {
-        // Capture original authData before mutating userResult via the response reference
-        const originalAuthData = userResult?.authData
-          ? Object.fromEntries(
-            Object.entries(userResult.authData).map(([k, v]) =>
-              [k, v && typeof v === 'object' ? { ...v } : v]
-            )
-          )
-          : undefined;
-
         // Assign the new authData in the response
         Object.keys(mutatedAuthData).forEach(provider => {
           this.response.response.authData[provider] = mutatedAuthData[provider];
@@ -683,24 +684,11 @@ RestWrite.prototype.handleAuthData = async function (authData) {
         // Then we're good for the user, early exit of sorts
         if (Object.keys(this.data.authData).length) {
           const query = { objectId: this.data.objectId };
-          // Optimistic locking: include the original array fields in the WHERE clause
+          // Optimistic locking: include each changed original field in the WHERE clause
           // for providers whose data is being updated. This prevents concurrent requests
-          // from both succeeding when consuming single-use tokens (e.g. MFA recovery codes).
-          if (originalAuthData) {
-            for (const provider of Object.keys(this.data.authData)) {
-              const original = originalAuthData[provider];
-              if (original && typeof original === 'object') {
-                for (const [field, value] of Object.entries(original)) {
-                  if (
-                    Array.isArray(value) &&
-                    JSON.stringify(value) !== JSON.stringify(this.data.authData[provider]?.[field])
-                  ) {
-                    query[`authData.${provider}.${field}`] = value;
-                  }
-                }
-              }
-            }
-          }
+          // from both succeeding when consuming single-use tokens (e.g. MFA recovery codes
+          // as arrays, or MFA SMS OTP tokens as strings).
+          applyAuthDataOptimisticLock(query, originalAuthData, this.data.authData);
           try {
             await this.config.database.update(
               this.className,
@@ -716,6 +704,11 @@ RestWrite.prototype.handleAuthData = async function (authData) {
             throw error;
           }
         }
+      } else if (this.query && this.data.authData && Object.keys(this.data.authData).length) {
+        // UPDATE path (e.g. PUT /users/:id during linked-provider re-auth): apply
+        // the same optimistic lock to the subsequent runDatabaseOperation update so
+        // concurrent single-use token consumers cannot both succeed.
+        applyAuthDataOptimisticLock(this.query, originalAuthData, this.data.authData);
       }
     }
   }
diff --git a/src/Routers/UsersRouter.js b/src/Routers/UsersRouter.js
@@ -18,6 +18,7 @@ import { promiseEnsureIdempotency } from '../middlewares';
 import RestWrite from '../RestWrite';
 import { logger } from '../logger';
 import { createSanitizedError } from '../Error';
+import { applyAuthDataOptimisticLock } from '../AuthDataLock';
 
 export class UsersRouter extends ClassesRouter {
   className() {
@@ -314,26 +315,10 @@ export class UsersRouter extends ClassesRouter {
     // If we have some new validated authData update directly
     if (validatedAuthData && Object.keys(validatedAuthData).length) {
       const query = { objectId: user.objectId };
-      // Optimistic locking: include the original array fields in the WHERE clause
-      // for providers whose data is being updated. This prevents concurrent requests
-      // from both succeeding when consuming single-use tokens (e.g. MFA recovery codes).
-      // Only array fields need locking — element removal is vulnerable to TOCTOU;
-      // scalar fields are simply overwritten and don't have concurrency issues.
-      if (user.authData) {
-        for (const provider of Object.keys(validatedAuthData)) {
-          const original = user.authData[provider];
-          if (original && typeof original === 'object') {
-            for (const [field, value] of Object.entries(original)) {
-              if (
-                Array.isArray(value) &&
-                JSON.stringify(value) !== JSON.stringify(validatedAuthData[provider]?.[field])
-              ) {
-                query[`authData.${provider}.${field}`] = value;
-              }
-            }
-          }
-        }
-      }
+      // Prevent concurrent requests from both succeeding when consuming single-use
+      // tokens (e.g. MFA recovery codes or SMS OTP tokens) by extending the update
+      // WHERE clause with the original values of changed primitive/array fields.
+      applyAuthDataOptimisticLock(query, user.authData, validatedAuthData);
       try {
         await req.config.database.update('_User', query, { authData: validatedAuthData }, {});
       } catch (error) {
