Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mielecloud] E-Mail check is not RFC compliant #11050

Closed
1358 opened this issue Jul 23, 2021 · 10 comments · Fixed by #11073
Closed

[mielecloud] E-Mail check is not RFC compliant #11050

1358 opened this issue Jul 23, 2021 · 10 comments · Fixed by #11073
Labels
bug An unexpected problem or unintended behavior of an add-on

Comments

@1358
Copy link

1358 commented Jul 23, 2021

The mail address syntax check is not RFC compliant at all. It rejects valid mail addresses for no reason.
Miele simply uses <input type="email"> and there is no reason to be more restrictive here...

@1358 1358 added the bug An unexpected problem or unintended behavior of an add-on label Jul 23, 2021
@1358 1358 changed the title [mielecloud] E-Mai check is not RFC compliant [mielecloud] E-Mail check is not RFC compliant Jul 23, 2021
@kaikreuzer
Copy link
Member

@BjoernLange Could you comment here?

@BjoernLange
Copy link
Contributor

I have to admit that the check in the currently stable 3.1.0 binding version is problematic, but we already changed that so everything that follows is about the current state. A version that includes these changes can be found here.

The MDN documentation says that the <input type="email"> uses the following regex for validation:

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

In #10928 the validation regex was changed to

[a-zA-Z_0-9.+%-]{3,}@([a-zA-Z_0-9-]+[.])+[a-z]+)

for both, frontend and backend. The significant differences are:

  • The binding doesn't allow some special characters in the local-part
  • The binding enforces the local-part to be at least 3 characters long
  • The binding enforces a . in the domain part that separates the domain from the top level domain
  • The binding enforces a top level domain composed of lower case letters
  • The binding has no length restriction on the individual parts of the domain while the MDN regex has a length restriction of 63 characters

Imo with #10928 integrated the situation is not as bad as described by @1358 as most e-mail addresses will work fine. However, it is absolutely right that the validation should be as close as possible to the validation Miele is doing when accepting new account registrations. As we can only see what is going on in the frontend, the validation they are using has to be at least as strict as the validation provided by <input type="email"> but the backend may add further restrictions. Thus, I will ask Miele whether there are further restrictions. It may take a while until I get a response because my contacts are on vacation right now. In the meantime I would propose to rename the issue to something like "Binding e-mail validation should be equal to Miele e-mail validation".

@lsiepel
Copy link
Contributor

lsiepel commented Jul 30, 2021

Why do you want to validate it for more than not empty?
Why not let the Miele service handle the validation. Just pass the address to the service and catch the validation error (if any) and pass it back to the binding? Maybe there is something specific why you want to validate it in the binding, but in general mimic a cloud service's validation is maintenance and error prone.

@BjoernLange
Copy link
Contributor

Why do you want to validate it for more than not empty?
Why not let the Miele service handle the validation. Just pass the address to the service and catch the validation error (if any) and pass it back to the binding?

tl;dr: We cannot validate the e-mail address via the Miele Cloud.

That would also be my favorite solution but sadly things don't work like this. The Miele Cloud Binding uses OAuth2 for authentication with the Miele Cloud. Thus, the binding does never work with the login data needed to sign into the Miele Cloud. We do not have access to the e-mail address (nor password) that is used for the paired account and there is no way to verify that the user provided the e-mail address linked to the account.

That's also why the initial contribution of the binding was missing the e-mail parameter, but it was added during the review process because openHAB needs at least one thing parameter to identify a thing. See here for details.

Given these circumstances it seems reasonable to me to mimic the validation of the Miele Cloud. I also assume that the validation of the e-mail address isn't changed often or even at all. But before starting a lengthy discussion on this, I would like to wait for a response from Miele on how they validate the e-mail address. Maybe it is just the same validation as provided by <input type="email"> which would make a discussion on whether to implement this or not obsolete. In that case I would just do it.

@1358
Copy link
Author

1358 commented Jul 31, 2021

To be honest, I'm really annoyed.
First you ban me because all my TLDs contain a "-".
Then you ban me because the local-part I use for my Miele account consists only of 2 ascii-chars. (e.g. me@some-where.tld)
And then you justify to the non-standard-behaviour. Why? What is the problem with all the mail addresses I use for more than 10 years exactly?
Where does [a-zA-Z_0-9.+%-]{3,}@([a-zA-Z_0-9-]+[.])+[a-z]+) come from? Rolling dices!?

Don't get me wrong, but there is an internet standard defining how an email address should look like. It is defined in RFC 2821 and especially RFC 2822 Section 3.4.1 (https://datatracker.ietf.org/doc/html/rfc2822#section-3.4.1) (and of cause the recent versions 5335+5336)
It is not the binding's job to tell people what standard-compliant addresses it "likes" and they have to use because it kinda feels uncomfortable with other ones and rejects them... Like rolling dices.
There is even a validator-implementation recommendation in RFC 3639 Section 3 (https://datatracker.ietf.org/doc/html/rfc3696#section-3)

If the binding wants to validate email addresses: Do it right (i.e. standard compliant) or leave it completely.

Can you please explain why do you want to "validate" a mail address at all? Why is this necceccary at all? Providing a mail address that does not exist or has no mailbox (e.g. "noreply@...") and providing a mail address with "typos" (that are "catched" by your regex-pattern) does not affect the OAuth2-Path (as both fail, and the error handling is the same) at all? And if the account does not exist at all the error handling codepath is also already existant?

<input type="email"> is also not intended as Validation (and btw. browser dependent, mozilla is no reference here) but as hint to the user that there are probably typos.
I was even more suprised that you re-validate your non-standard-compliant regex-pattern also in the backend so this issue was not workaroundable easily with browser-debugging-tools.

BTW: The validation of the "Miele Cloud" has to be RFC compliant as well. Thats why we have standards. To get around exactly that discussion you just started. It is called "standard" because it is a standard everyone that participants has to comply.
If Miele has a non-standarad-compliant stack regarding mail addresses they have to fix it, too.

Like @lsiepel already mentioned there is no reason to "validate" something in the binding itself.

@1358
Copy link
Author

1358 commented Jul 31, 2021

tl:dr: We do not have to "mimic" someone (quirky), we have to be standard compliant. Thats what standards for.

@kaikreuzer
Copy link
Member

@BjoernLange I would agree with @1358 here. As the e-mail address isn't atm actively used by the binding at all, even a typo in it wouldn't harm the functionality. And it should not be up to a binding to implement any custom e-mail syntax validation - if something is desired, we should probably rather use a basic check like InternetAddress.validate().

I'd suggest to simply remove the validation logic from the binding.

@BjoernLange
Copy link
Contributor

I'd suggest to simply remove the validation logic from the binding.

I'm perfectly fine with that and will happily do so, @kaikreuzer.

@lsiepel
Copy link
Contributor

lsiepel commented Jul 31, 2021

Thanks!

@BjoernLange
Copy link
Contributor

I opened #11073 for this purpose, @kaikreuzer. Maybe you can do a quick review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants