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

isValidHost: don't try to validate hostnames #2684

Closed
otheus opened this issue May 16, 2022 · 4 comments
Closed

isValidHost: don't try to validate hostnames #2684

otheus opened this issue May 16, 2022 · 4 comments

Comments

@otheus
Copy link

otheus commented May 16, 2022

|| !preg_match('/^([a-zA-Z\d.-]*|\[[a-fA-F\d:]+\])$/', $host)
is simply a bad practice. You're trying to validate hostnames with a regex that doesn't conform to all past or future RFCs. It's good to validate against the empty string, and to classify ip4, ipv6, etc, but the regex here doesn't conform to valid hostnames used outside of DNS. And there are plenty of reasons to use hostnames that are not used with DNS, for instance, docker hosts, internal or unroutable hosts, etc.

@Synchro
Copy link
Member

Synchro commented May 16, 2022

You are ignoring the context in which this is used. It is not intended to be exhaustive, and it even says so in the code. That specific check is used prior to several more detailed but more expensive checks. If a name fails that check then we can be pretty sure it's going to fail the more detailed ones, and we've saved ourselves the bother. Can you provide an example of a valid hostname that fails this check but passes the others?

@otheus
Copy link
Author

otheus commented Jun 6, 2022

I'm not ignoring the context in which it is used. Quite the opposite: the validation is ignoring possible contexts in which the user of the library (or maintainer of the application for which the library is included) is used. If it is not intended to be exhaustive, then it should not be a fatal error for the check to not pass. In other words, as I have stated: don't validate (except for potential security exploits and exception-causing empty or null values). The OS will validate the hostname or the upstream mail service will validate.

@Synchro
Copy link
Member

Synchro commented Jun 6, 2022

How do you propose to resolve the conflict that (for example) dotless addresses that are valid locally are invalid elsewhere, but there is no way to tell which context you are in?

Do you have a constructive suggestion? Or a PR?

@otheus
Copy link
Author

otheus commented Jun 6, 2022 via email

@Synchro Synchro closed this as completed Sep 21, 2022
@PHPMailer PHPMailer locked as too heated and limited conversation to collaborators Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants