Follow-up to #2819987: Download mail attachments on click

Problem/Motivation

Header::toArray() adds field value separation by comma which is the rule for HTTP headers, not for MIME headers.

Proposed resolution

Removing it could cause overwrite in the header... What we could do is to replace toArray() with getFields() and then leave to the caller to do what is necessary.

Also, EmailDisplayController uses toArray() call to create HTTP headers from the given MIME headers. Let's just Content-Type, Content-Disposition MIME header values as HTTP ones.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 remove_field_value-2820667-8.patch4.8 KBmbovan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mbovan created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Priority: Normal » Major

Yes, we can't passthru headers like we do now.

We need to treat each header cleanly on its purpose and decide what to do with it.
And we need test coverage for many different cases and validate their headers as expectations are.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Active » Postponed

Ah, you propose to do the test coverage separate:
#2820669: Improve tests for attachment downloads

This is kindof related / overlapping. I think we should do the test coverage then first so we have proper expectations.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Related issues: +#2820669: Improve tests for attachment downloads

connecting, so un-postponing works.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Postponed » Active

Lots of updates recently. Is this still pending?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented

Originally we passed all headers into HTTP headers, but that's very dangerous and wrong since those are MIME headers and not HTTP headers - and untrusted anyways. For instance i could cause an open redirect by injecting a Location: header. :-)

So we need to treat header by header manually with output.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented

Yeah, we fixed this for the raw message access, but it still applies for the general attachment case.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Active » Needs review
FileSize
remove_field_value-2820667-8.patch4.8 KB

Converted toArray() to getFields().

Also, made header related updates in EmailDisplayController::getAttachment().

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Needs review » Needs work

No :-)

The implementation in toArray was perfectly right and it adds value. It follows the RFC that defined it.

+++ b/tests/modules/inmail_test/src/Controller/EmailDisplayController.php
@@ -81,15 +82,7 @@ class EmailDisplayController extends ControllerBase {
+    return new Response($decoded_body, Response::HTTP_OK, $this->getHeaders($entity));

The problem is here. You are simply not allowed to do a HTTP response and passing in the headers from the original entity.

Again, i could send you a mail with this:
"Location: example.com"

Then if you visit the download URL with a browser, you are redirected. This is a security issue, called open redirect.

Every header you want to pass to the browser from the mail needs to be identified by name and with validated properties.
Don't pass headers forward you don't know.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs work » Needs review 
+++ b/src/MIME/MimeHeader.php
@@ -154,24 +154,10 @@ class MimeHeader {
-        // There can be a case of header fields the same name and it's possible
-        // to combine them by adding a comma.
-        // @see https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
-        $headers[$field_name] .= ',' . $field['body'];
The implementation in toArray was perfectly right and it adds value. It follows the RFC that defined it.

Well, it's defined by the following HTTP-related RFC standard: https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2.
I couldn't find if it applies for MIME messages. Most likely, no... On the other side, it could cause problems with some header fields (e.g. "Received" header).


+++ b/tests/modules/inmail_test/src/Controller/EmailDisplayController.php
@@ -81,15 +82,7 @@ class EmailDisplayController extends ControllerBase {
+    return new Response($decoded_body, Response::HTTP_OK, $this->getHeaders($entity));

@@ -118,4 +111,30 @@ class EmailDisplayController extends ControllerBase {
+    return [
+      'Content-Disposition' => $content_disposition,
+      'Content-Type' => $content_type,
+      'Content-Transfer-Encoding' => $entity->getContentTransferEncoding(),
+    ];

Re #9.2: I think that's what we are preventing with #8.

  • miro_dietiker committed 0e58dba on 8.x-1.x authored by mbovan 
    Issue #2820667 by mbovan: Remove field value comma separation in header'...
miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Needs review » Fixed

OK great, Examples also look pretty fine about header checks.
I was wrong with both parts. :-)

I'm only not sure about image delivery. If an image is contained with disposition attachment, we still pass it forward as inline.

Status: Fixed » Closed (fixed)

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