Relative URLs in mails should be converted to absolute ones

I can take this on if it has stalled.

I started looking into this however I actually think this is not an issue for core. The only mail plugin we have in core calls MailFormatHelper::htmlToText() which calls MailFormatHelper:: htmlToMailUrls() which ensures that any links in the mail are absolute.

Looking at MailManager::mail() I think it is the mail plugin's responsibility to ensure any links are absolute when it does:

    // Format the message body.
    $message = $system->format($message);

That said perhaps we should repurpose this issue for documentation?

Hm, possibly. It's still an API/behavior change for everyone who sends HTML mails that happened in a patch update, though.

But if #88183: Relative URLs in feeds should be converted to absolute ones isn't critical, then I guess this isn't either.

@Berdir yep it was an API behaviour change but given that it has occurred the question is what is the best way to go forward. Phpmail::format() takes steps to ensure all urls are absolute.

We have a few choices I guess...

1. Call something for every plugin after the ->format() call that does something similar to #88183: Relative URLs in feeds should be converted to absolute ones
2. Call something a method on plugins that implement it, after the ->format() call (advantage being that PhpMail does not have a performance penalty).
3. Add the documentation suggested here

Option 1 means that we kinda revert the API breaks caused by #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send but it does NOT feel like the path to go down now.

In discussing with @Berdir on IRC he pointed out that

not sure what you mean with option 2, but sounds like a new optional interface + maybe a trait, so contrib modules would have to change anyway. not much different to
#3
, really?
so it's probably not 2 or 3 but 2 and 3? document it and provide an easy way for them to handle this?

So maybe option two should be more like:
2. Add a new method to MailInterface to convert relative urls to absolute - plugins like core's PhpMail can no-op because they've got it covered in their ::format() (because it removes all HTML

I suggested this in IRC @berdir asked why do we want to break the SMTP module but actually that module has an option $this->smtpConfig->get('smtp_allowhtml'); if this is FALSE it does the same as core's PhpMail if it is TRUE it'll need to do something about relative urls.

The more I investigate the more I'm in favour of option 2 with an interface break so the plugin authors know they have to do something.

As discussed in IRC, I can live with option 3 if we provide a trait or so that does the work and you just have to call it, and announce this in the release notes.

2 seems a bit much to me (breaking all mail plugins to enforce them to consider this), it would break those modules for many users that are not even sending html (and just use smtp or swiftmailer to send through smtp), and sending a mail is probably not something you'd test in a patch update?

If you feel strongly about that then I can live that, for me personally, it doesn't make a difference.. I have to fix and update swiftmailer anyway.

Discussed with @catch and we agreed on a variation of option 2 - adding a new interface and using trigger error - so that mail plugin author's can find out something is wrong but also so live sites don't break.

We also agreed that this is not critical as core's mail plugin is doing things correctly. Demoting to major since it is a regression that we've introduced to 8.x cycle and the best solution requires a soft API break.

There might another approach for Mail plugins instead of having to fix all the links. I think I recall once ensuring that HTML emails displayed correctly using the base element. Note this does not affect the outcome here - it just offers another solution for example plugin to explore.

Just trying out some ideas, what if we'd do something like this? No parsing overhead if the input is not actually some kind of HTML, but it would still work also for PhpMail if the markup is then converted to plaintext and contains relative links.

Also no additional methods or interfaces necessary.

Given that it would at least not "break" any kind of existing simple strings, but it would allow modules, like simplenews, to opt into that behaviour basically.

Also causes problems for #2842780: Add a token for the site logo

@Berdir, is the proposed solution direction of #14 still valid? I could re-roll it, but it may be that core has other methods by now?

Yes, that is still valid, I'm not aware of any changes here. Would be awesome if you could look into tests.

I'll see if I can come up with something then.

@Berdir and @Wim Leers,

I am a bit confused on what to test. The main use cases for desiring absolute URLs in mails appears to come from #2931548: CDN and images in emails: absolute URLs necessary and #2842780: Add a token for the site logo and those are about adding images in mails.

However, the only mail backend that is provided with Drupal, is PhpMail, which uses MailFormatHelper::htmlToText to strip everything except for <a> <em> <i> <strong> <b> <br> <p> <blockquote> <ul> <ol> <li> <dl> <dt> <dd> <h1> <h2> <h3> <h4> <h5> <h6> <hr>. Of those elements, only <a> can contain a URL, but MailFormatHelper::htmlToMailUrls should already take care of making that absolute.

I could write a simple test to ensure that relative urls in anchors are indeed converted into absolute as claimed, but that would not help #2931548: CDN and images in emails: absolute URLs necessary and #2842780: Add a token for the site logo. On the other hand, implementing a totally different mail backend that supports images seems a bit out of scope.

IMO these related issues are not as related as assumed, and their problems cannot be solved within this issue. Or am I missing something?

implementing a totally different mail backend that supports images seems a bit out of scope.

You just have to stop PhpMail from converting the HTML to plain text, so here's your implementation:

use Drupal\Core\Mail\Plugin\Mail\TestMailCollector;

/**
 * Modifies the Drupal mail system to send HTML emails.
 *
 * @Mail(
 *   id = "test_html_mail",
 *   label = @Translation("Test mailer"),
 *   description = @Translation("Sends the message as HTML, using PHP's native mail() function.")
 * )
 */
class TestMail extends TestMailCollector {

  /**
   * Overrides PhpMail::format() to prevent it from stripping out the HTML.
   *
   * @param array $message
   *   A message array, as described in hook_mail_alter().
   *
   * @return string
   *   The formatted $message.
   */
  public function format(array $message) {
    $message['body'] = implode("\n\n", $message['body']);
    return $message;
  }

}

Note: TestMailCollector extends PhpMail and overrides the mail() method to capture mail in a state variable rather than trying to send it using PHP's mail() function.

Oh, and of course you have to tell your test to use the new plugin:

    \Drupal::configFactory()->getEditable('system.mail')->set('interface.default', 'test_html_mail')->save();

Yes exactly, we need a html test mail plugin that doesn't do the HTML conversion. There is also one in #2223967: Do not decode a contact message twice that is pretty similar to the example .

Thanks for the guidance. This should help me to compose a working patch. I hope to have something by the end of the weekend :)

Oke, here we go.

I copied the mail plugin from #2223967: Do not decode a contact message twice (and cleaned up the description to better match what it does).

I have implemented hook_mail in simpletest, to include a render_from_message_param mail key, which takes the 'message' parameter and changes it into a body part. (Might be useful for other tests too.)

/**
 * Implements hook_mail().
 */
function simpletest_mail($key, &$message, $params) {
  switch ($key) {
    case 'render_from_message_param':
      $message['body'][] = render($params['message']);
      break;
  }
}

I then implemented the changes from Berdir's patch in MailManager.php, but that failed when plain strings containing urls were provided as mail body parts. Therefor I chose to remove the check for MarkupInterface (why was that required?) and implement it like this:

// Attempt to convert relative URLs to absolute.
foreach ($message['body'] as &$body_part) {
  $body_part = Markup::create(Html::transformRootRelativeUrlsToAbsolute((string) $body_part, \Drupal::request()->getSchemeAndHttpHost()));
}

Finally, I had started working on a second test to assert that rendered elements are properly converted as well. But I could not get it to generate any output. And I doubt if is necessary at all. It is attached as well, and if you believe it is good to have I can have another look.

1. +++ b/core/modules/simpletest/simpletest.module
   @@ -751,6 +751,17 @@ function simpletest_clean_results_table($test_id = NULL) {
   + */
   +function simpletest_mail($key, &$message, $params) {
   

   I think this should be in a test module, not in simpletest itself. I see the test already depends on a similar alter hook here, but we already converted this test to phpunit and we eventually want to get rid of (almost) everything in simpletest.module, so sooner or later, we will have to move it.

2. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/TestHtmlMailCollector.php
   @@ -0,0 +1,32 @@
   +class TestHtmlMailCollector extends TestMailCollector {
   

   kind of the same here.

   by being a plugin, this is picked up by e.g. mailsystem.module and the user can configure this in the UI, which is weird.

3. +++ b/core/modules/simpletest/simpletest.module
   @@ -751,6 +751,17 @@ function simpletest_clean_results_table($test_id = NULL) {
   +      $message['body'][] = render($params['message']);
   

   render should be deprecated like drupal_render() don't know why it isn't.

   Lets use \Drupal::service('renderer')->renderPlain() because render() bubbles things up into the main context could leave things like placeholders, since this will be sent out as-is, we need to fully render it.

I then implemented the changes from Berdir's patch in MailManager.php, but that failed when plain strings containing urls were provided as mail body parts. Therefor I chose to remove the check for MarkupInterface (why was that required?) and implement it like this:

Markup stuff is tricky. If we accept any input as valid and allowed html, then something like a comment notification might result in showing user input as html in a mail client. While those have to protect themself already against arbitrary mails with html, a user might not expect something strange from a trusted system notification e-mail.

What kind of plain strings with urls did you have? HTML that's not a MarkupInterface is imho a bug. See for example \Drupal\swiftmailer\Plugin\Mail\SwiftMailer::massageMessageBody(), anything that is not (safe) Markup is escaped. Your change would completely sidetrack that as everything would then be Markup. We already fixed things in core (contact e-mails) to be compatible with that.

Thanks for the feedback, I will adjust the patch accordingly.

What kind of plain strings with urls did you have?

It did not encounter any specifically, but in the past I have seen them at least in several contrib modules. I will make sure to include the check for the MarkupInterface.

Finally, I had started working on a second test to assert that rendered elements are properly converted as well. But I could not get it to generate any output. And I doubt if is necessary at all.

Yes, I do think it's necessary, because that's where you test the image URLs.

The construct that was broken by #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send and which causes a problem in #2842780: Add a token for the site logo and elsewhere, and which prompted this issue is:

    $logo = array(
      '#theme' => 'image',
      '#uri' =>  Url::fromUri($uri, ['absolute' => TRUE])->toString(),
    );

since this always produces a relative URL when $uri is a local image.

It is this usage which we have to ensure is fixed, so I think this should be tested explicitly, as you do in your second test.

@TR, well, yes and no (this is the cause of my confusion as well).

My reasoning against is that the coverson to absolute happens after format, meaning that all render elements should have been converted to html already right?

So if we test for the replacement in any random html tag, we should be good.

In theory, if you could really test any random HTML tag, then yes, <img> tags would be covered by that.

However, testing all possible HTML tags isn't practical - your test makes assumptions, such as URLs will be found only in href or src attributes, that aren't strictly true (what about <form action="/relative/url"> ?

So I think that there should be a specific test for the bug we know about - the '#theme' => 'image', thing - AND a general test like you have which attempts to cover all other unknown cases.

The specific test proves the bug and the fix, while the general test attempts to protect us against other similar bugs being introduced.

I can contrive several ways the general test could potentially fail to catch the <img> bug that we intend to fix. For instance, the general test is not checking self-closing tags like img, so potentially the relative->absolute conversion fails on these and won't be caught. Likewise, the general test has only one tag attribute, so potentially the conversion fails if the URL that needs converting is in the second rather than first attribute, etc.

The point is, we do have a specific bug and a specific failure, so we should have a specific test for the failure and fix. Because adding a specific test is just a few extra lines of code in your current test, I don't see any reason to leave it out.

Thank you both for the feedback. This is the improved version.

Added the base_path to the expected path pattern to ensure the expected path matches with the actual path. Furthermore the coding standards errors that slipped through have been resolved.

And obviously the base path was needed for the second round of assertions as well...

That was a random fail. Retesting showed up green. What do you think?

@Berdir, could you have another look please?

Rebased.

Hi,
following patch is #49 + codesniffer_fixes.patch reported in #51
hope this will help it to be green :)

@izus an interdiff would make it 1000 times easier to review and rtbc your changes.

Hi,
@jonathanshaw
here is the interdiff between #49 and #52
Thanks

Adding review credits for TR for helping with the test case

Committed as 530f7ac and pushed to 8.7.x.

Cherry-picked as f2915d9 and pushed to 8.6.x.

i need patch for drupal 9.2 any off them i can use?

@AnnaE1990 This feature is already included in Drupal 9.2, if you are having trouble please open a new issue with full information on what's not working.