liuch / dmarc-srg

A php parser, viewer and summary report generator for incoming DMARC reports.
GNU General Public License v3.0
218 stars 32 forks source link

Fix Mail::DMARC truncated filename #34

Closed smeinecke closed 1 year ago

smeinecke commented 1 year ago

Attachment extension check fails if a report is send by Mail::DMARC as the filename gets truncated:

Content-Type: application/gzip;
 name=fastmail.com!domain.de!1664582400!1664668799!738011312.xml.gz
Content-Transfer-Encoding: base64
Content-Disposition: inline;
 filename*0=fastmail.com!domain.de!1664582400!1664668799!738011312.xm;
 filename*1=l.gz
Liuch\DmarcSrg\Mail\MailAttachment Object
(
    [conn:Liuch\DmarcSrg\Mail\MailAttachment:private] => IMAP\Connection Object
        (
        )

    [filename:Liuch\DmarcSrg\Mail\MailAttachment:private] => fastmail.com!domain.de!1657238400!1657324799!686941312.x...
    [bytes:Liuch\DmarcSrg\Mail\MailAttachment:private] => 740
    [number:Liuch\DmarcSrg\Mail\MailAttachment:private] => 2
    [mnumber:Liuch\DmarcSrg\Mail\MailAttachment:private] => 2943
    [encoding] => 3
)
object(stdClass)#12 (13) {
  ["type"]=>
  int(3)
  ["encoding"]=>
  int(3)
  ["ifsubtype"]=>
  int(1)
  ["subtype"]=>
  string(4) "GZIP"
  ["ifdescription"]=>
  int(0)
  ["ifid"]=>
  int(0)
  ["bytes"]=>
  int(740)
  ["ifdisposition"]=>
  int(1)
  ["disposition"]=>
  string(6) "inline"
  ["ifdparameters"]=>
  int(1)
  ["dparameters"]=>
  array(2) {
    [0]=>
    object(stdClass)#13 (2) {
      ["attribute"]=>
      string(8) "filename"
      ["value"]=>
      string(67) "fastmail.com!domain.de!1657238400!1657324799!686941312.x..."
    }
    [1]=>
    object(stdClass)#14 (2) {
      ["attribute"]=>
      string(8) "filename"
      ["value"]=>
      string(69) "fastmail.com!domain.de!1657238400!1657324799!686941312.xml.gz"
    }
  }
  ["ifparameters"]=>
  int(1)
  ["parameters"]=>
  array(1) {
    [0]=>
    object(stdClass)#15 (2) {
      ["attribute"]=>
      string(4) "name"
      ["value"]=>
      string(69) "fastmail.com!domain.de!1657238400!1657324799!686941312.xml.gz"
    }
  }
}

As a hotfix, just check if filename in ifparameters is longer. We probably should fix the getAttribute method to always use the last (and complete) element.

Later, I will add a PR to use mime_content_type if PHP fileinfo extension is found to get compression type instead of the file extension of the attachment.

liuch commented 1 year ago

It is very strange. I wonder why two classes with the same attribute are added to the dparameters array. Is this a bug in the PHP or I didn't understand something?

We probably should fix the getAttribute method to always use the last (and complete) element.

I agree with you, it should be fixed this way because the issue is directly related to extracting the attribute.

smeinecke commented 1 year ago

It is very strange. I wonder why two classes with the same attribute are added to the dparameters array. Is this a bug in the PHP or I didn't understand something?

I am not sure why imap_fetchstructure does this. I just checked the documentation and it's not mentioned. I also checked the rfc2184 which shows this as valid writing of the parameter.

We probably should fix the getAttribute method to always use the last (and complete) element.

I agree with you, it should be fixed this way because the issue is directly related to extracting the attribute.

Cool, in commit https://github.com/liuch/dmarc-srg/pull/34/commits/45ad43daee52533ac4b1b0ef0d32814381694712 I changed getAttribute to return last found value.

Just out of curiosity, why is $this->structure defined in MailMessage? I did not found that its used anywhere.

liuch commented 1 year ago

Just out of curiosity, why is $this->structure defined in MailMessage? I did not found that its used anywhere.

I have no idea why I did this and I didn't find any mention of these changes in my git history. It seems to be the result of some kind of refactoring at the initial stage of implementation. So I can't satisfy your curiosity and mine. Thanks for pointing this out, I'll fix it.