diff --git a/spec/CloudCode.spec.js b/spec/CloudCode.spec.js index 941d896aae..ca2f03c8b3 100644 --- a/spec/CloudCode.spec.js +++ b/spec/CloudCode.spec.js @@ -153,6 +153,27 @@ describe('Cloud Code', () => { ); }); + it('does not hang when a beforeSave object has a malformed pointer (#7490)', async () => { + Parse.Cloud.beforeSave('Stuff', () => {}); + + // A pointer with an empty objectId cannot be serialised, which used to throw + // inside the trigger response handler and leave the request hanging forever. + // The SDK rejects such a pointer client-side, so send it as a raw REST body. + const response = await request({ + method: 'POST', + url: 'http://localhost:8378/1/classes/Stuff', + headers: { + 'X-Parse-Application-Id': 'test', + 'X-Parse-Master-Key': 'test', + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ Owner: { __type: 'Pointer', className: '_User', objectId: '' } }), + }).catch(e => e); + + expect(response.status).toBe(400); + expect(response.data.code).toBe(Parse.Error.SCRIPT_FAILED); + }); + it('returns an error', done => { Parse.Cloud.define('cloudCodeWithError', () => { /* eslint-disable no-undef */ diff --git a/src/triggers.js b/src/triggers.js index f66d96f942..4a49bd3a7c 100644 --- a/src/triggers.js +++ b/src/triggers.js @@ -551,7 +551,12 @@ export function maybeRunAfterFindTrigger( } return responseFromTrigger; }) - .then(success, error); + .then(success, error) + // See maybeRunTrigger: a throw in the success/error path must reject rather + // than leave this promise unsettled and hang the request (#7490). + .catch(e => + reject(resolveError(e, { code: Parse.Error.SCRIPT_FAILED, message: 'Script failed.' })) + ); }).then(resultsAsJSON => { logTriggerAfterHook( triggerType, @@ -917,7 +922,7 @@ async function builtInTriggerValidator(options, request, auth) { // Resolves to an object, empty or containing an object key. A beforeSave // trigger will set the object key to the rest format object to save. // originalParseObject is optional, we only need that for before/afterSave functions -export function maybeRunTrigger( +export async function maybeRunTrigger( triggerType, auth, parseObject, @@ -926,100 +931,110 @@ export function maybeRunTrigger( context ) { if (!parseObject) { - return Promise.resolve({}); + return {}; } - return new Promise(function (resolve, reject) { - var trigger = getTrigger(parseObject.className, triggerType, config.applicationId); - if (!trigger) { return resolve(); } - var request = getRequestObject( - triggerType, - auth, - parseObject, - originalParseObject, - config, - context - ); - var { success, error } = getResponseObject( - request, - object => { - logTriggerSuccessBeforeHook( - triggerType, - parseObject.className, - parseObject.toJSON(), - object, - auth, - triggerType.startsWith('after') - ? config.logLevels.triggerAfter - : config.logLevels.triggerBeforeSuccess - ); - if ( - triggerType === Types.beforeSave || - triggerType === Types.afterSave || - triggerType === Types.beforeDelete || - triggerType === Types.afterDelete - ) { - Object.assign(context, request.context); - } - resolve(object); - }, - error => { - logTriggerErrorBeforeHook( + const trigger = getTrigger(parseObject.className, triggerType, config.applicationId); + if (!trigger) { + return; + } + const request = getRequestObject( + triggerType, + auth, + parseObject, + originalParseObject, + config, + context + ); + + try { + await maybeRunValidator(request, `${triggerType}.${parseObject.className}`, auth); + + // AfterSave and afterDelete triggers can return a promise, which is awaited + // here so trigger execution is synced with the RestWrite.execute() call. If + // they do not return a promise, they run async code parallel to it. + let triggerResult; + if (request.skipWithMasterKey) { + triggerResult = undefined; + } else { + const returned = trigger(request); + if ( + triggerType === Types.afterSave || + triggerType === Types.afterDelete || + triggerType === Types.afterLogin + ) { + logTriggerAfterHook( triggerType, parseObject.className, parseObject.toJSON(), auth, - error, - config.logLevels.triggerBeforeError + config.logLevels.triggerAfter ); - reject(error); } - ); + if (triggerType === Types.beforeSave) { + // beforeSave is expected to return null (nothing); only a returned object + // with an `object` key (e.g. from an express before hook) is honoured. + const response = + returned && typeof returned.then === 'function' ? await returned : null; + triggerResult = response && response.object ? response : null; + } else { + triggerResult = await returned; + } + } - // AfterSave and afterDelete triggers can return a promise, which if they - // do, needs to be resolved before this promise is resolved, - // so trigger execution is synced with RestWrite.execute() call. - // If triggers do not return a promise, they can run async code parallel - // to the RestWrite.execute() call. - return Promise.resolve() - .then(() => { - return maybeRunValidator(request, `${triggerType}.${parseObject.className}`, auth); - }) - .then(() => { - if (request.skipWithMasterKey) { - return Promise.resolve(); - } - const promise = trigger(request); - if ( - triggerType === Types.afterSave || - triggerType === Types.afterDelete || - triggerType === Types.afterLogin - ) { - logTriggerAfterHook( - triggerType, - parseObject.className, - parseObject.toJSON(), - auth, - config.logLevels.triggerAfter - ); - } - // beforeSave is expected to return null (nothing) - if (triggerType === Types.beforeSave) { - if (promise && typeof promise.then === 'function') { - return promise.then(response => { - // response.object may come from express routing before hook - if (response && response.object) { - return response; - } - return null; - }); - } - return null; - } + // getResponseObject shapes the trigger's return value into the REST response. + // Serialising an object with a malformed pointer can throw here; awaiting the + // wrapped promise lets that reject cleanly instead of hanging forever (#7490). + const response = await new Promise((resolve, reject) => { + const { success, error } = getResponseObject(request, resolve, reject); + try { + success(triggerResult); + } catch (e) { + error(e); + } + }); - return promise; - }) - .then(success, error); - }); + logTriggerSuccessBeforeHook( + triggerType, + parseObject.className, + parseObject.toJSON(), + response, + auth, + triggerType.startsWith('after') + ? config.logLevels.triggerAfter + : config.logLevels.triggerBeforeSuccess + ); + if ( + triggerType === Types.beforeSave || + triggerType === Types.afterSave || + triggerType === Types.beforeDelete || + triggerType === Types.afterDelete + ) { + Object.assign(context, request.context); + } + return response; + } catch (e) { + const resolvedError = resolveError(e, { + code: Parse.Error.SCRIPT_FAILED, + message: 'Script failed. Unknown error.', + }); + let objectForLog; + try { + objectForLog = parseObject.toJSON(); + } catch { + // A malformed object (e.g. a pointer with an empty objectId) cannot be + // serialised; log without it rather than mask the original error (#7490). + objectForLog = { className: parseObject.className }; + } + logTriggerErrorBeforeHook( + triggerType, + parseObject.className, + objectForLog, + auth, + resolvedError, + config.logLevels.triggerBeforeError + ); + throw resolvedError; + } } // Converts a REST-format object to a Parse.Object