Mail_Mime breaks "Additional Message Headers" plugin #4966

Closed
rcubetrac opened this Issue Feb 8, 2016 · 8 comments

Comments

Projects
None yet
1 participant
@rcubetrac

Reported by Chumba on 8 Feb 2016 11:48 UTC as Trac ticket #1490657

Hi,

The "Additional Message Headers" plugin included with Roundcube doesn't work any longer as intended due to a change in the Mail_Mime class.

At least in Roundcube 1.1.4, in the included Mail_Mime, the property (as it once was)


var $_headers = array();

was changed to

protected $headers = array();

Consequently, the message_headers() method of the Additional Message Headers plugin doesn't properly reset the message headers, i.e.

args[= array();

now refers to the wrong property; yet

args['message']('message']->_headers)->headers = array();

wouldn't work either due to the property $headers now being protected.

As an improvement, it's possible to pass the $overwrite = true parameter to the headers() method, like this:

$args['message']->headers($headers, true);

This allows to overwrite existing headers; but it still doesn't allow to unset previously set headers (like the X-Sender header), as nulled headers from the Additional Message Headers would simply be ignored.

Not sure how to 100% fix this issue. It is possible to hook message_outgoing_headers instead, and have full control over the headers before they are being sent to the Mail_Mime object, but according to the docs, this hook is depreciated?

Keywords: Additional Message Headers
Migrated-From: http://trac.roundcube.net/ticket/1490657

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Feb 8, 2016

Milestone changed by @alecpl on 8 Feb 2016 16:04 UTC

later => 1.1.5

Milestone changed by @alecpl on 8 Feb 2016 16:04 UTC

later => 1.1.5

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Feb 8, 2016

Comment by @alecpl on 8 Feb 2016 16:09 UTC

Replying to Chumba:

$args['message']->headers($headers, true);

This allows to overwrite existing headers; but it still doesn't allow to unset previously set headers (like the X-Sender header), as nulled headers from the Additional Message Headers would simply be ignored.

I don't see why it wouldn't work. Or you mean it does not work with Mail_Mime < 1.9?

Comment by @alecpl on 8 Feb 2016 16:09 UTC

Replying to Chumba:

$args['message']->headers($headers, true);

This allows to overwrite existing headers; but it still doesn't allow to unset previously set headers (like the X-Sender header), as nulled headers from the Additional Message Headers would simply be ignored.

I don't see why it wouldn't work. Or you mean it does not work with Mail_Mime < 1.9?

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Feb 8, 2016

Comment by Chumba on 8 Feb 2016 16:58 UTC

Hi,

Replying to alec:

I don't see why it wouldn't work. Or you mean it does not work with Mail_Mime < 1.9?

I am using the Roundcube 1.1.4 "complete" package, which includes Mail_Mime (not sure what version, but I assume it is >= 1.9).

Before I was using Roundcube 1.0.1.

After I upgraded from 1.0.1 to 1.1.4, the "Additional Message Headers" plugin would no longer strip existing headers nor overwrite existing headers. It would only add new headers if configured to do so.

The reason is as explained in the post above. Additional Message Headers, even as shipped in Roundcube 1.1.4, assumes an older version of Mail_Mime (like the one distributed with Roundcube 1.0.1). It tries to empty the $_headers property, even though Mail_Mime doesn't use that property anymore (it is renamed to $headers). On top of that, it is no longer possible to overwrite the $headers property because it is protected and AFAIK Mail_Mime doesn't supply a setter to modify it directly.

Does this make more sense?

Feel free to give it a try. My config.inc.php for the plugin looks like this:

function guidv4()
{
    $data = openssl_random_pseudo_bytes(16);
    $data[= chr(ord($data[6](6])) & 0x0f | 0x40);
    $data[= chr(ord($data[8](8])) & 0x3f | 0x80);
    return strtoupper(vsprintf('%s%s-%s-%s-%s-%s%s%s', str_split(bin2hex($data), 4)));
}

$rcmail_config[= null;
$rcmail_config['additional_message_headers']('additional_message_headers']['X-Sender'])['Message-ID'] = '<' . guidv4() . '@my.mailserver.com>';

But neither the x-sender is removed from sending mail nor the message-id header replaced. The reason:

  • the "Additional Message Headers" plugin doesn't empty the existing $header property because its code is outdated (see above)
  • because the $header property isn't cleared, when the Mail_Mime headers() method is called, the new headers are "merged" (see the headers() code for details) with the existing entries in $headers. By default (unless the overwrite parameter is set) existing entries in $headers are not replaced. In addition, nulled headers (like x-sender above) are basically ignored, since they don't "overwrite" existing headers, i.e. the x-sender header will continue to exist.

I was considering rewriting the plugin to make use of the message_outgoing_headers hook; but the depreciation notice made me hesitate.

Comment by Chumba on 8 Feb 2016 16:58 UTC

Hi,

Replying to alec:

I don't see why it wouldn't work. Or you mean it does not work with Mail_Mime < 1.9?

I am using the Roundcube 1.1.4 "complete" package, which includes Mail_Mime (not sure what version, but I assume it is >= 1.9).

Before I was using Roundcube 1.0.1.

After I upgraded from 1.0.1 to 1.1.4, the "Additional Message Headers" plugin would no longer strip existing headers nor overwrite existing headers. It would only add new headers if configured to do so.

The reason is as explained in the post above. Additional Message Headers, even as shipped in Roundcube 1.1.4, assumes an older version of Mail_Mime (like the one distributed with Roundcube 1.0.1). It tries to empty the $_headers property, even though Mail_Mime doesn't use that property anymore (it is renamed to $headers). On top of that, it is no longer possible to overwrite the $headers property because it is protected and AFAIK Mail_Mime doesn't supply a setter to modify it directly.

Does this make more sense?

Feel free to give it a try. My config.inc.php for the plugin looks like this:

function guidv4()
{
    $data = openssl_random_pseudo_bytes(16);
    $data[= chr(ord($data[6](6])) & 0x0f | 0x40);
    $data[= chr(ord($data[8](8])) & 0x3f | 0x80);
    return strtoupper(vsprintf('%s%s-%s-%s-%s-%s%s%s', str_split(bin2hex($data), 4)));
}

$rcmail_config[= null;
$rcmail_config['additional_message_headers']('additional_message_headers']['X-Sender'])['Message-ID'] = '<' . guidv4() . '@my.mailserver.com>';

But neither the x-sender is removed from sending mail nor the message-id header replaced. The reason:

  • the "Additional Message Headers" plugin doesn't empty the existing $header property because its code is outdated (see above)
  • because the $header property isn't cleared, when the Mail_Mime headers() method is called, the new headers are "merged" (see the headers() code for details) with the existing entries in $headers. By default (unless the overwrite parameter is set) existing entries in $headers are not replaced. In addition, nulled headers (like x-sender above) are basically ignored, since they don't "overwrite" existing headers, i.e. the x-sender header will continue to exist.

I was considering rewriting the plugin to make use of the message_outgoing_headers hook; but the depreciation notice made me hesitate.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Feb 9, 2016

Comment by @alecpl on 9 Feb 2016 08:08 UTC

Fixed in 89a49dd(master) and c8023ac6b1, f915d15c4.

Comment by @alecpl on 9 Feb 2016 08:08 UTC

Fixed in 89a49dd(master) and c8023ac6b1, f915d15c4.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Feb 9, 2016

Status changed by @alecpl on 9 Feb 2016 08:08 UTC

new => closed

Status changed by @alecpl on 9 Feb 2016 08:08 UTC

new => closed

@rcubetrac rcubetrac closed this Feb 9, 2016

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Feb 9, 2016

Comment by Chumba on 9 Feb 2016 09:19 UTC

Thanks alec. Notice, this is only a partial fix as suggested in my comments above.

It won't allow Additional Message Headers to unset existing headers anymore (like in the example config how to remove x-sender).

Given that the Mail_Mime has protected the headers property and won't allow its direct setting or getting, perhaps it would be useful to reconsider the depreciation of message_outgoing_headers. At least unless there is another way how one can access and modify headers.

Comment by Chumba on 9 Feb 2016 09:19 UTC

Thanks alec. Notice, this is only a partial fix as suggested in my comments above.

It won't allow Additional Message Headers to unset existing headers anymore (like in the example config how to remove x-sender).

Given that the Mail_Mime has protected the headers property and won't allow its direct setting or getting, perhaps it would be useful to reconsider the depreciation of message_outgoing_headers. At least unless there is another way how one can access and modify headers.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Feb 9, 2016

Comment by @alecpl on 9 Feb 2016 09:24 UTC

Replying to Chumba:

It won't allow Additional Message Headers to unset existing headers anymore (like in the example config how to remove x-sender).

Unsetting headers works for me.

Comment by @alecpl on 9 Feb 2016 09:24 UTC

Replying to Chumba:

It won't allow Additional Message Headers to unset existing headers anymore (like in the example config how to remove x-sender).

Unsetting headers works for me.

@rcubetrac

This comment has been minimized.

Show comment
Hide comment
@rcubetrac

rcubetrac Feb 9, 2016

Comment by Chumba on 9 Feb 2016 11:48 UTC

Hi,

Replying to alec:

Unsetting headers works for me.

Thanks, you're right - I tested it again and this time it worked.

Comment by Chumba on 9 Feb 2016 11:48 UTC

Hi,

Replying to alec:

Unsetting headers works for me.

Thanks, you're right - I tested it again and this time it worked.

@rcubetrac rcubetrac added this to the 1.1.5 milestone Mar 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment