Tighten config validation schema of system.mail mailer_dsn

Created on 13 November 2023, 8 months ago
Updated 5 July 2024, 8 days ago

Problem/Motivation

The config validation schema introduced in Use structured DSN instead of URI in system.mail mailer_dsn Fixed could be tighter.

Steps to reproduce

Proposed resolution

The string fields could at least disallow control-characters and newlines similar to the label type 📌 Add validation constraint to `type: label` + `type: text`: disallow control characters Fixed (except maybe the password field).

If possible the structured DSN type should be reusable by contrib or custom config entities.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Fixed

Version

11.0 🔥

Component
Mail 

Last updated 2 days ago

No maintainer
Created by

🇨🇭Switzerland znerol

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @znerol
  • Status changed to Active 8 months ago
  • 🇨🇭Switzerland znerol

    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.

  • Pipeline finished with Failed
    8 months ago
    Total: 561s
    #54754
  • Pipeline finished with Success
    8 months ago
    Total: 910s
    #54773
  • 🇨🇭Switzerland znerol

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

  • Status changed to Needs review 8 months ago
  • 🇨🇭Switzerland znerol

    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.

  • Pipeline finished with Success
    8 months ago
    Total: 684s
    #54865
  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

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

  • Status changed to Needs review 8 months ago
  • 🇨🇭Switzerland znerol

    CR added.

  • Pipeline finished with Success
    8 months ago
    Total: 691s
    #55380
  • Status changed to RTBC 8 months ago
  • 🇺🇸United States smustgrave

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

  • Status changed to Needs work 7 months ago
  • 🇳🇿New Zealand quietone New Zealand

    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.

  • Pipeline finished with Failed
    2 months ago
    Total: 165s
    #171005
  • Status changed to Needs review 2 months ago
  • 🇨🇭Switzerland znerol

    Added tests.

  • Pipeline finished with Failed
    2 months ago
    Total: 183s
    #171037
  • Pipeline finished with Failed
    2 months ago
    Total: 206s
    #171051
  • Pipeline finished with Success
    2 months ago
    Total: 811s
    #171059
  • 🇨🇭Switzerland znerol

    Switched parent to #2952037

  • Status changed to Needs work about 2 months ago
  • 🇺🇸United States smustgrave

    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.

  • Pipeline finished with Success
    about 2 months ago
    Total: 574s
    #173981
  • Status changed to Needs review about 2 months ago
  • 🇨🇭Switzerland znerol

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

  • Pipeline finished with Failed
    about 2 months ago
    Total: 573s
    #174003
  • Pipeline finished with Success
    about 2 months ago
    Total: 581s
    #174033
  • 🇨🇭Switzerland znerol

    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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 997s
    #175328
  • 🇨🇭Switzerland znerol

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

  • Pipeline finished with Failed
    about 2 months ago
    Total: 179s
    #175785
  • Pipeline finished with Failed
    about 2 months ago
    Total: 869s
    #175789
  • Pipeline finished with Failed
    about 2 months ago
    Total: 996s
    #175820
  • 🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    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.

  • Status changed to Needs work about 2 months ago
  • 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.

  • Pipeline finished with Canceled
    about 2 months ago
    Total: 70s
    #177417
  • Status changed to Needs review about 2 months ago
  • Pipeline finished with Success
    about 2 months ago
    Total: 692s
    #177420
  • Status changed to Needs work about 2 months ago
  • 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.

  • Status changed to Needs review about 2 months ago
  • 🇨🇭Switzerland znerol

    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.

  • Status changed to RTBC about 1 month ago
  • 🇺🇸United States smustgrave

    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.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • 🇺🇸United States phenaproxima Massachusetts
  • 🇨🇭Switzerland znerol

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

  • Status changed to Needs work about 1 month ago
  • 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.

  • Status changed to Needs review about 1 month ago
  • 🇨🇭Switzerland znerol

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

  • Status changed to RTBC about 1 month ago
  • 🇺🇸United States smustgrave

    Seems to just be a rebase so will restore.

  • Pipeline finished with Success
    about 1 month ago
    Total: 775s
    #196570
  • Status changed to Needs work 16 days ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    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.

  • Status changed to Needs review 16 days ago
  • 🇨🇭Switzerland znerol

    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.

  • Status changed to RTBC 15 days ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

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

    • alexpott committed 87bf1eef on 11.x
      Issue #3401255 by znerol, smustgrave, borisson_, phenaproxima,...
  • Status changed to Fixed 15 days ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

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

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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 👍

Production build 0.69.0 2024