Throwing exceptions from a *.create.validate callback makes validation pass

[jimrandomh]

In vulcan-lib/lib/server/mutators.js, createMutator uses runCallbacks to run <typename>.create.validate callbacks. Unfortunately, runCallbacks handles errors in a way that is very bad for validation: if a callback throws an exception, it continues running and produces a return value that looks like success. Thus, if a validator throws an error to indicate that validation has failed, createMutator will misinterpret this as a success.

This almost certainly interacts with existing application code to produce security vulnerabilities.

[SachaG]

Yeah I see your point… the idea is that those callbacks shouldn't throw errors, but instead add error objects to the validationErrors array so they can all be aggregated together and shown to the user at the same time. But that can be unintuitive, and in any case error throwing should probably actually throw the error here.

It should be fixable though. If you set error.break = true on your error before throwing it the callback loop should in turn throw it properly. Here's the relevant bit of code:

https://github.com/VulcanJS/Vulcan/blob/devel/packages/vulcan-lib/lib/modules/callbacks.js#L123-L132

[jimrandomh]

No, that isn't a fix, because exceptions also come from things that aren't throw statements, such as trying to access missing fields.

[jimrandomh]

It's fine to require putting things in validationErrors if you want a nice error message. But if a validation function crashes, it is very important that it fail closed, not open.

[SachaG]

Good point. How about specifying in the runCallbacks options that errors should always be thrown then? So something like:

    validationErrors = await runCallbacks({
      name: '*.create.validate',
      iterator: validationErrors,
      properties,
      options: {
        break: true
      }
    });

I'd accept a PR to add that feature (either to the devel branch, or ideally devel-apollo1 branch first).