fix(payload-schemas): add custom RegEx validation for 'email'

[oscard0m]

Description

• Added custom validation format for emails in ajv
• Added a new schema example to validate emails with special characters are validated
• Regenerate index.json

Context

Current JSON Schema validation for emails does not support emails with this format.

EDIT:

- Current JSON Schema validation for `emails` does not support emails with this format.
+ emails aren't compliant with the RFC, not because of anything related to the JSON schema itself.
+ RFC 5322 Section 3.4 defines valid email addresses here: https://datatracker.ietf.org/doc/html/rfc5322#section-3.4

Example of not supported format:
41898282+github-actions[bot]@users.noreply.github.com

Fixes #453

[oscard0m]

@timrogers @wolfy1339 before fixing the Regex I references in a code comment, I wanted to validate with you this solution proposal.

What do you think?

[wolfy1339]

You don't need to specify it that way, you can simply pass in a regex for validation directly in the schema itself using the pattern property

https://json-schema.org/understanding-json-schema/reference/string.html#pattern

[wolfy1339]

The current JSON Schema specification does not cover e-mails with this format.

It's because the emails aren't compliant with the RFC, not because of anything related to the JSON schema itself.

RFC 5322 Section 3.4 defines valid email addresses

I just want that to be obvious, so we are all on the same page and don't blame the wrong people.

---

Also, don't forget to re-generate the index.json file

[timrogers]

It's because the emails aren't compliant with the RFC, not because of anything related to the JSON schema itself.

RFC 5322 Section 3.4 defines valid email addresses

I agree on this. Personally, I think we should just remove format: "email" and say it is a string, rather than changing how emails are validated here.

From the JSON Schema standard's perspective, it is a string as it doesn't conform to the email RFC.

[wolfy1339]

Here's a modified version of AJV's regex for email validation that works with the emails we have

^[a-z0-9!#$%&'*+/=?^_`{|}~\-\[\]]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~\-\[\]]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$

[oscard0m]

Here's a modified version of AJV's regex for email validation that works with the emails we have

^[a-z0-9!#$%&'*+/=?^_`{|}~\-\[\]]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~\-\[\]]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$

Could you guide me on which part of the code are we supposed to add this pattern property? I'm a bit lost right now. @wolfy1339

I agree on this. Personally, I think we should just remove format: "email" and say it is a string, rather than changing how emails are validated here.

From the JSON Schema standard's perspective, it is a string as it doesn't conform to the email RFC.

@timrogers wolfy already got a regex to cover our use-case, I would prefer to use that so we don't allow non-valid e-mails and add extend the regex to cover our use-case. Any downsides of using it?

[wolfy1339]

Could you guide me on which part of the code are we supposed to add this pattern property? I'm a bit lost right now.
@wolfy1339

webhooks/payload-schemas/api.github.com/common/committer.schema.json

Line 11 in f2f7ffb

"oneOf": [{ "type": "string", "format": "email" }, { "type": "null" }]

Instead of having "format": "email", you would use pattern with the regex
https://json-schema.org/understanding-json-schema/reference/string.html#pattern

[timrogers]

@timrogers wolfy already got a regex to cover our use-case, I would prefer to use that so we don't allow non-valid e-mails and add extend the regex to cover our use-case. Any downsides of using it?

The downside, in my opinion, is that people might be using these schemas in another language with another JSON Schema validation tool.

If someone uses another tool, it won't know that we have "redefined" format: "email" to use our own custom regex, so the other tool will consider webhooks to be invalid according to the schema.

[wolfy1339]

The downside, in my opinion, is that people might be using these schemas in another language with another JSON Schema validation tool.

If someone uses another tool, it won't know that we have "redefined" format: "email" to use our own custom regex, so the other tool will consider webhooks to be invalid according to the schema.

Totally agree.

Which is why I suggested the pattern key directly in the schema

[timrogers]

Totally agree.

Which is why I suggested the pattern key directly in the schema

Ah, makes sense - I missed "in the schema" in your comment above. I think that would be the best way of doing it: don't use the email format and define our own pattern instead.

[oscard0m]

I had to double escape some characters (due to JSON but the RegExp is not matching after escaping). Any clue on what I'm doing wrong @wolfy1339 ?

---

Should we migrate the rest of schemas with email to this pattern?

[wolfy1339]

I had to double escape some characters (due to JSON but the RegExp is not matching after escaping). Any clue on what I'm doing wrong @wolfy1339 ?

The double escapes are changing the meaning of the RegExp. The backslashes are escapes themselves for characters in the RegExp, and by escaping them, you're changing their meaning to be a backslash.

That's all I can think of

[G-Rath]

Honestly I don't think we should be messing with a pattern regexp like that, as it's a good way to cause more trouble (the size of it alone is huge even before you think about all the subpatterns).

If that field is meant to hold valid emails then they should be valid according to the spec; otherwise they are a string and should not be validated further than that. The third option I'd be happy with is matching whatever validation that git itself uses which I would hope is either nothing or RFC 5322 but I wouldn't be surprised if it wasn't as email validation is notorious for being done incorrectly by everyone...

[oscard0m]

I had to double escape some characters (due to JSON but the RegExp is not matching after escaping). Any clue on what I'm doing wrong @wolfy1339 ?

The double escapes are changing the meaning of the RegExp. The backslashes are escapes themselves for characters in the RegExp, and by escaping them, you're changing their meaning to be a backslash.

That's all I can think of

This is the issue I'm getting, invalid JSON basically:

---

Honestly I don't think we should be messing with a pattern regexp like that, as it's a good way to cause more trouble (the size of it alone is huge even before you think about all the subpatterns).

If that field is meant to hold valid emails then they should be valid according to the spec; otherwise they are a string and should not be validated further than that. The third option I'd be happy with is matching whatever validation that git itself uses which I would hope is either nothing or RFC 5322 but I wouldn't be surprised if it wasn't as email validation is notorious for being done incorrectly by everyone...

I can agree the RegEx is very long and hard to follow but basically, we are extending the one we were already using with ajv-format.

Your point of not following a standard is interesting but at least there are some basic patterns of an email address we should be covering, right?

[G-Rath]

Your point of not following a standard is interesting but at least there are some basic patterns of an email address we should be covering, right?

My point is we should be matching whatever is using that field because that's what actually matters or otherwise should not be validating it at all - anything else means that we are being incorrect.

Sometimes that is fine because the fix can be straightforward, but in this case it's a honking big RegExp that we've sourced from somewhere else i.e. consider if someone else comes along and presents another email that is considered invalid by this pattern but works with git - we'll need to safely modify the pattern again to cover that in addition to what we currently consider valid.

(also consider the time you've already spent on trying to get this working)

[oscard0m]

I see your point @G-Rath. It's very valid IMO.

👍🏽 thumbs up for just removing the format for email: #678

🚀 for using our own RegEx pattern: #677

---

@wolfy1339 @G-Rath @timrogers @gr2m @nickfloyd

[wolfy1339]

If we are to remove it, we should also document it somewhere

[oscard0m]

Created associated PR here already in case we end up deciding on this solution: #678

I am still checking why this Regex is not working as a personal challenge 🤓 and pushing the working solution.

[oscard0m]

Created associated PR here already in case we end up deciding on this solution: #678

I am still checking why this Regex is not working as a personal challenge 🤓 and pushing the working solution.

Found the issue, we were missing the case insensitive flag. JSON Schemas (from what I found) do not support flags like \i for RegEx so I had to emulate that by adding A-Za-z everywhere 🤮

[wolfy1339]

For reasons listed above, i think it's best to leave this issue alone.

As GitHub has published it's own OpenAPI spec for webhooks, efforts will be concentrated on the current community spec and transitioning to the official spec