fix(schema): Update createsSchema to disallow additional properties, improve tests [PDE-4948]

[kreddlear]

We currently have 2 tickets that have come up, reporting action keys that should be invalid:

• PDE-4948
• PDE-4889

I reproduced this issue via CLI, confirming it is currently possible to have an action with a key that's invalid (for example, 3create_tag or create_order:test) per the KeySchema and per the CreatesSchema patternProperties.

The cause seems to be that the library we're using, jsonschema, expects us to set additionalProperties: false if we want all properties that don't match the patternProperties regex to be rejected. TriggersSchema has this already, as does SearchOrCreatesSchema and most of the other schemas.

This MR adds that additionalProperties: false to the CreatesSchema so that invalid action keys will be properly rejected. It also adds a test to confirm this new behavior, and improves the other existing tests so that it's clearer what the errors that come up should be.

One downside: Ideally, when a dev tried to submit a trigger/action with an invalid key, we would surface the error instance.creates.tag_create.key does not match pattern "^[a-zA-Z]+[a-zA-Z0-9_]*$"' to make it clear what the issue is. However, jsonschema doesn't seem to have a way to error on bad property names - it just rejects them entirely via additionalProperties: false. So the current error users will see is instance.creates additionalProperty "3contact_by_tag" exists in instance when not allowed, which is not super clear, but better than allowing it to be uploaded.

[eliangcs]

Good catch! It looks like CreatesSchema has been always like this (without additionalProperties: false) since the initial commit 8 years ago. 🤯

[eliangcs]

@kreddlear I suppose we need to make the same change for SearchesSchema? It doesn't have additionalProperties: false either.

[kreddlear]

Good catch! It looks like CreatesSchema has been always like this (without additionalProperties: false) since the initial commit 8 years ago. 🤯

Yes, this blew my mind!

I suppose we need to make the same change for SearchesSchema? It doesn't have additionalProperties: false either.

@eliangcs Great catch. I've gone through all the other schemas, and updated a few others with type: object to also have that additionalProperties: false as well where it seems potentially problematic.

FYI there are some others that don't have it, but also that don't seem problematic, so I left them as-is. Those are:

• FieldChoicesSchema - we would just ignore other properties
• FieldChoiceWithLabelSchema - we would just ignore other properties
• FunctionSchema - would either be a string or one of FunctionRequireSchema or FunctionSourceSchema, both of which have additionalProperties: false

Happy to update them if you think that's best for safety though!

[eliangcs]

AFIAK, the others aren't causing trouble, so I think we can keep this PR as is. Thanks!

[rnegron]

@kreddlear Great find here, thank you!

In terms of releasing this, do you think this could be considered a "breaking schema change"? I believe I saw that only a few integrations might be leveraging additional properties here and it's not really something we want to continue supporting so it may not be "practically breaking"...

[kreddlear]

@rnegron I was trying to figure this out as well! I think since we don't want to and didn't intend to support it, it likely wouldn't be "practically breaking".

I think that's further supported by the fact that apparently these actions aren't actually usable in Zaps, so I don't think there would actually be anything to break, if that makes sense.