From 7ce028573ad223fe17dc9cc7265f08e7e99f7dd4 Mon Sep 17 00:00:00 2001 From: Manuel Trezza <5673677+mtrezza@users.noreply.github.com> Date: Fri, 24 Apr 2026 17:16:35 +0100 Subject: [PATCH 1/3] fix: GHSA-jpq4-7fmq-q5fj v9 --- spec/vulnerabilities.spec.js | 109 +++++++++++++++++++++++++++++++++++ src/RestWrite.js | 108 +++++++++++++++++++++++++--------- src/Routers/UsersRouter.js | 25 +++++--- 3 files changed, 207 insertions(+), 35 deletions(-) diff --git a/spec/vulnerabilities.spec.js b/spec/vulnerabilities.spec.js index e41e4f9120..c3f9296af0 100644 --- 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/RestWrite.js b/src/RestWrite.js index c6c8db969c..82a80f2c62 100644 --- a/src/RestWrite.js +++ b/src/RestWrite.js @@ -585,6 +585,15 @@ RestWrite.prototype.handleAuthData = async function (authData) { // No user found with provided authData we need to validate if (!results.length) { + // Capture the original authData before validation mutates the adapter's view, + // so that the optimistic lock below can detect single-use token consumption. + const originalAuthData = currentUserAuthData + ? Object.fromEntries( + Object.entries(currentUserAuthData).map(([k, v]) => + [k, v && typeof v === 'object' ? { ...v } : v] + ) + ) + : undefined; const { authData: validatedAuthData, authDataResponse } = await Auth.handleAuthDataValidation( authData, this @@ -592,6 +601,13 @@ RestWrite.prototype.handleAuthData = async function (authData) { this.authDataResponse = authDataResponse; // Replace current authData by the new validated one this.data.authData = validatedAuthData; + // For UPDATE paths (e.g. PUT /users/:id during MFA SMS enrollment confirmation), + // extend the update WHERE clause so concurrent single-use token consumers cannot + // both succeed; the no-user branch is reached because findUsersWithAuthData has + // no way to key on provider fields like the MFA pending token. + if (this.query && this.query.objectId && Object.keys(this.data.authData).length) { + applyAuthDataOptimisticLock(this.query, originalAuthData, this.data.authData); + } return; } @@ -658,20 +674,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 +699,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,11 +719,64 @@ 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); } } } }; +// 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. +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; +} + RestWrite.prototype.checkRestrictedFields = async function () { if (this.className !== '_User') { return; diff --git a/src/Routers/UsersRouter.js b/src/Routers/UsersRouter.js index 3ac099f0b0..0791d8efe3 100644 --- a/src/Routers/UsersRouter.js +++ b/src/Routers/UsersRouter.js @@ -314,20 +314,27 @@ 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. + // Optimistic locking: include each changed lockable 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 as arrays, or MFA SMS OTP tokens as strings). Only primitives + // and arrays are locked — Date and object values are skipped because their + // stored representation differs between storage adapters; locking a companion + // primitive (e.g. the MFA token string) is sufficient. + const isLockable = v => { + if (v === null || v === undefined) { return false; } + const t = typeof v; + return t === 'string' || t === 'number' || t === 'boolean' || Array.isArray(v); + }; 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]) - ) { + if (!isLockable(value)) { + continue; + } + if (JSON.stringify(value) !== JSON.stringify(validatedAuthData[provider]?.[field])) { query[`authData.${provider}.${field}`] = value; } } From a0a90ffde450689eaf907a2afd77843755960d5b Mon Sep 17 00:00:00 2001 From: Manuel Trezza <5673677+mtrezza@users.noreply.github.com> Date: Fri, 24 Apr 2026 18:52:14 +0100 Subject: [PATCH 2/3] refactor: drop no-user-found authData optimistic lock --- src/RestWrite.js | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/RestWrite.js b/src/RestWrite.js index 82a80f2c62..97353d2444 100644 --- a/src/RestWrite.js +++ b/src/RestWrite.js @@ -585,15 +585,6 @@ RestWrite.prototype.handleAuthData = async function (authData) { // No user found with provided authData we need to validate if (!results.length) { - // Capture the original authData before validation mutates the adapter's view, - // so that the optimistic lock below can detect single-use token consumption. - const originalAuthData = currentUserAuthData - ? Object.fromEntries( - Object.entries(currentUserAuthData).map(([k, v]) => - [k, v && typeof v === 'object' ? { ...v } : v] - ) - ) - : undefined; const { authData: validatedAuthData, authDataResponse } = await Auth.handleAuthDataValidation( authData, this @@ -601,13 +592,6 @@ RestWrite.prototype.handleAuthData = async function (authData) { this.authDataResponse = authDataResponse; // Replace current authData by the new validated one this.data.authData = validatedAuthData; - // For UPDATE paths (e.g. PUT /users/:id during MFA SMS enrollment confirmation), - // extend the update WHERE clause so concurrent single-use token consumers cannot - // both succeed; the no-user branch is reached because findUsersWithAuthData has - // no way to key on provider fields like the MFA pending token. - if (this.query && this.query.objectId && Object.keys(this.data.authData).length) { - applyAuthDataOptimisticLock(this.query, originalAuthData, this.data.authData); - } return; } From 0a9603f4946f7d6974dda80b0c5ca156509bd594 Mon Sep 17 00:00:00 2001 From: Manuel Trezza <5673677+mtrezza@users.noreply.github.com> Date: Sat, 25 Apr 2026 23:26:17 +0100 Subject: [PATCH 3/3] refactor: extract authData optimistic lock to shared util --- src/AuthDataLock.js | 47 ++++++++++++++++++++++++++++++++++++ src/RestWrite.js | 49 +------------------------------------- src/Routers/UsersRouter.js | 32 ++++--------------------- 3 files changed, 53 insertions(+), 75 deletions(-) create mode 100644 src/AuthDataLock.js diff --git a/src/AuthDataLock.js b/src/AuthDataLock.js new file mode 100644 index 0000000000..c809a8b3d8 --- /dev/null +++ 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 index 97353d2444..6fd6bff94b 100644 --- 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. @@ -713,54 +714,6 @@ RestWrite.prototype.handleAuthData = async function (authData) { } }; -// 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. -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; -} - RestWrite.prototype.checkRestrictedFields = async function () { if (this.className !== '_User') { return; diff --git a/src/Routers/UsersRouter.js b/src/Routers/UsersRouter.js index 0791d8efe3..34271a7dd7 100644 --- 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,33 +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 each changed lockable 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 as arrays, or MFA SMS OTP tokens as strings). Only primitives - // and arrays are locked — Date and object values are skipped because their - // stored representation differs between storage adapters; locking a companion - // primitive (e.g. the MFA token string) is sufficient. - const isLockable = v => { - if (v === null || v === undefined) { return false; } - const t = typeof v; - return t === 'string' || t === 'number' || t === 'boolean' || Array.isArray(v); - }; - 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 (!isLockable(value)) { - continue; - } - if (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) {