fix: binary files as part are corrupted when stripping carriage returns #26
Conversation
pgmillon
force-pushed
the
fix-binary-mimetype
branch
from
d449958
to
16b39f6
Compare
| . 'Content-Type: text/plain; charset=UTF-8' . "\n" | ||
| . 'Content-Transfer-Encoding: ' . $encoding . "\n" | ||
| . "\n" | ||
| . $result . "\n" |
There was a problem hiding this comment.
Same as above: changes to pre-existing test probably hiding a BC break 🤔
There was a problem hiding this comment.
I understand your concern, but don't have a clear answer.
Ripping-off carriage returns is harmful to binaries but people might have built solutions of their own to mitigate it and this fix might break them.
There was a problem hiding this comment.
TBH, if that's how things are, that would be an https://xkcd.com/1172/ :D
I just want a second review by somebody else, but if it's really like that, the bugfix is valid, and to be merged with the test changes too 👍
|
@pgmillon it's possible that this, being a bugfix, should go to |
|
Hi, |
|
We merge up from oldstable branches to latest branches. Since the last release is We don't merge regular bugs into older branches, since they need to be security issues, in order to qualify for that :) EDIT: I changed the base branch, but your commit will need to probably be cherry-picked on top of that, and then force-pushed here, so extraneous commits are gone. |
Signed-off-by: Pierre-Gildas MILLON <[email protected]>
pgmillon
force-pushed
the
fix-binary-mimetype
branch
from
16b39f6
to
983100e
Compare
|
Should be fine for 2.9.x now. |
|
Wait, I just found another bug : if the message sender uses \r\n to generate the message rather than just \n, then the splitMime is not working correctly and leaving a lonely \r. |
Signed-off-by: Pierre-Gildas MILLON <[email protected]>
|
There we go : in my original fix I assumed sender's EOL was \n but when testing with a server that was using \r\n, I noticed the intend to prevent binaries corruption was not met because of a trailing \r. |
|
Hi, |
There was a problem hiding this comment.
The changes make sense to me. While I have concerns about the test changes, your arguments for making them make sense, and the liklihood of breakage for users is limited to those who are already pre-processing parts on their own — which will primarily be limited to those trying to fix this very issue.
My main feedback is that I think you should make a few changes to make the "what" more clear: what you are capturing, what behavior you are testing, etc.
| throw new Exception\RuntimeException('Not a valid Mime Message: End Missing'); | ||
| } | ||
|
|
||
| // the remaining part also needs to be parsed: | ||
| $res[] = substr($body, $start, $p - $start); | ||
| $res[] = substr($body, $start, $matches[0][1] - $start - strlen($serverEOL)); |
There was a problem hiding this comment.
What does $matches[0][1] represent? Can you memoize it into a variable with a descriptive name, please?
| @@ -205,4 +205,20 @@ public function testSetContentRaisesInvalidArgumentExceptionForInvalidContentTyp | |||
| $this->expectException(Mime\Exception\InvalidArgumentException::class); | |||
| $part->setContent($content); | |||
| } | |||
|
|
|||
| public function testBinaryPart() | |||
There was a problem hiding this comment.
Give this a better name: testBinaryPartsAreCreatedAndParsedWithoutLoss() or similar.
weierophinney
added
Awaiting Author Updates
and removed
Awaiting Maintainer Response
labels
Description
When using a simple PNG binary as part of a message, the carriage return characters are ripped off, corrupting the binary.
Not ripping the carriage return characters have an impact in the parts splitting that was updated to use preg_match + PREG_OFFSET_CAPTURE as a replacement to strpos.
This was done the support either \r\n or \n with a regex rather than a more complex approach.
Working with a binary file highlighted the line feed that is added while reading each part, this was removed too a/ to avoid binary corruption and b/ a mime with no line feed shouldn't magically have one when reading.
A unit test was added to ensure further support of binaries.