Tighten config validation schema of system.mail mailer_dsn

Postponed on ✨ Use structured DSN instead of URI in system.mail mailer_dsn Fixed .

According to RFC 3986 Appendix A, syntax validation of the scheme key seems possible with a regex.

ABNF for scheme is:

   scheme        = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )

Translated to a regex: /[a-z][a-z0-9+\-\.]*/i

The host part seems tricky just with a regex. It may be an IP address literal or alternatively it may be a string of alphanumeric plus quite some punctuation characters. The host part may even contain percent escaped characters. Also the IP address literals are allowed in numerous different formats.

I think that for the host field it would be best to implement a custom validator.

There is no restriction for the user and password fields. The parse_url and urldecode (as used by DSN::fromString()) happily accept various control characters (including "%00"). Remote systems probably wouldn't accept user names and passwords with newlines and tabs, but from a spec point of view, such strings are acceptable.

Same for the option values. They actually can contain anything.

There is also RFC 6874 specifying the correct syntax for IPv6 zone ids.

It looks like parse_url() accepts a wide range of characters in the host part which are clearly not allowed according to the RFC. E.g., non-ascii letters, the ampersand, colons outside of brackets, etc. However, it replaces control characters with an underscore (including newlines).

Hence, let's reuse the regex from the label data type for the host field.

For the new schema type could we get a change record.

CR added.

Thanks! The config move to it's own type seems fine to me.

This is more than just a config move, it is also adding constraints. And as such I think those new constraints need to be accompanied with tests.

Added tests.

Switched parent to #2952037

Left a comment on the MR.

Don't think we will need an upgrade path as the types don't seem to be changing but not 100% on that.

I added the FullyValidatable constraint to the @system.mail@ config. But honestly, I'm not quite sure on which level this should be added (config level or core datatype level, or even/additionally in @mailer_dsn.options.x@).

Gist of a slack conversation with @penyaskito:

@znerol saw that, but still think hostname should re-use symfony validator instead of just validating no control characters. The stricter the better. And even if we don't, I'm pretty sure we will need to reuse the symfony constraints at some point.

Turned out that the Hostname validator isn't quite enough, since the host parameter also should accept IP addresses. I've investigated whether it would be possible to implement this using the AtLeastOneOf and a combination of Hostname and Ip. But that doesn't allow for IPv6 literals (enclosed in brackets).

I guess it would be best to just introduce an UriHost constraint, which implements RFC 3986 section 3.2.2.

Added tests for the new UriHost constraint. Note that filter_var($value, \FILTER_VALIDATE_DOMAIN, \FILTER_FLAG_HOSTNAME) accepts every valid IPv4, thus removed the explicit call to filter_var($value, \FILTER_VALIDATE_IP, \FILTER_FLAG_IPV4).

I love the new test coverage here, this is really good. I had one remark on the type definition, but I'm not sure if that would be better.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Now that 🐛 A revert has cause cspell to fail due to the word yarhar Active is fixed, this shouldn't fail the needs review queue bot anymore.

Ran test-only feature https://git.drupalcode.org/issue/drupal-3401255/-/jobs/1638536 which shows coverage.

Appears all feedback has been addressed

I reviewed the CR and detail is on point.

Believe this one to be good to go.

znerol → credited kunal.sachdev → .

znerol → credited penyaskito → .

znerol → credited phenaproxima → .

Copied credits over from 📌 Add validation constraints to system.mail Needs work

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

znerol → changed the visibility of the branch 11.x to hidden.

Seems to just be a rebase so will restore.

I think if we make password and user are now nullable I think we should consider whether we allow blanks. And if we don;t allow blanks then we might need a update path.

I feel that the proposed changes do have some potential for breakage. Keep in mind that transports are pluggable. And there are some transports in the wild which are using HTTPS to talk directly to an API instead of SMTP.

I did encounter API keys in the past, which leveraged basic auth in weird ways. In one example the whole API key went into the user part, and the password part was supposed to be left blank. See this random bug report on python requests which is referring to that method.

For that reason, I'd prefer if we wouldn't add the NotBlank constraints, unless there are strong reasons for it.

Turns out my concerns are invalid - thanks @znerol - I think the rtbc in #34 stands.

Committed 87bf1ee and pushed to 11.x. Thanks!

Very nice — I hadn't seen this while working on 📌 Add validation constraints to system.mail Needs work , but #17 is a good reason to not reuse Symfony's existing Hostname validation constraint 👍

Automatically closed - issue fixed for 2 weeks with no activity.