zapier / zapier-platform

The SDK for you to build an integration on Zapier
https://platform.zapier.com
Other
344 stars 188 forks source link

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

Closed kreddlear closed 4 months ago

kreddlear commented 4 months ago

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

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 commented 4 months ago

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

kreddlear commented 4 months ago

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:

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

kreddlear commented 4 months ago

@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.