Schema validation in custom transactions can easily be broken

[mudlee]

There is an AJV bug, that they consider as a feature. See here. Especially check the latest comment written by wigy.

As you initiate AJV with removeAdditional: true, it means if our custom transaction schema is similar to the one in the issue I linked, than it will easily fail if we use anyOf with additionalProperties: false in the items.

Example:

...
return schemas.extend(schemas.transactionBaseSchema, {
  $id: this.key,
  required: ['asset','type','typeGroup'],
  properties: {
    type: { transactionType: this.type, },
    typeGroup: { const: this.typeGroup, },
    amount: { bignumber: { minimum: 0, maximum: 0 } },
    asset: {
      required: ['operationAttempts'],
      additionalProperties: false,
      properties: {
        operationAttempts: {
          type: 'array',
          items: {
            anyOf: [
              { required: ['propA'], additionalProperties: false, properties: { propA: { type: 'string', const: 'A' } } },
              { required: ['propB'], additionalProperties: false, properties: { propA: { type: 'string', const: 'B' } } }
            ]
          }
        }
      }
    },
  },
});
...

Transaction:

{
...
operationAttempts: [{ propB: 'B' }]
...
}

If we send in a transaction (see above) where operationAttempts must mach on the second schema from anyOf, it will fail, as we the AJV is initialized with removeAdditional: true and we declared additionalProperties: false at all level. It will fail against the first schema, it will remove then the not wanted property (propB), hence it will fail against the second schema as well, because the object now does not have propB which is not the expected behavior.

Expected Behavior

I would expect that properties on data will not be removed during schema validation iteration.

Current Behavior

See above.

Possible Solution

Initiate AJV without removeAdditional: true.

[ghost]

Thanks for opening this issue! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

[air1one]

Hey thanks @mudlee for pointing out this issue. Indeed I wasn't expecting AJV to remove properties before checking against all possibilities defined in anyOf.

I checked for core repository and all schemas look "safe".
Now it would be nice to remove removeAdditional: true as you suggested, but it can have big impact, like blocks or transactions not being accepted anymore.
So we need to be very careful before doing this, I will check with the team see what we do.

[air1one]

Ok so after discussion with the team, we will not make this change now considering this is the last major iteration on v2, and with v3 the validation will be reworked and we will not have this issue anymore.
So closing this, thanks again for reporting 👍

[ghost]

This issue has been closed. If you wish to re-open it please provide additional information.

[wigy-opensource-developer]

Yeah, I somewhat expected this outcome, this decision makes complete sense. We will try to contribute to the documentation to save other devs the 3-4 hours we spent on narrowing down the validation issue we had.

In Ajv with this configuration if an asset has a heterogenous collection (something that needs anyOf) that needs a discriminator on each item, our current understanding is to use a key rather than a value for discriminating among the cases and hide all properties specific to that case under the key.

Something like this:

{
  "modes": [
    { "ftp": { "account": "blah" }},
    { "email": { "mailingList": "foobar" }}
  ]
}

Rather than this:

{
  "modes": [
    {
      "mode": "ftp",
      "account": "blah"
    },
    {
      "mode": "email",
      "mailingList": "foobar"
    }
  ]
}